diff --git a/client/internal/dns/host_windows.go b/client/internal/dns/host_windows.go index d20fdd1d6..2852dddb9 100644 --- a/client/internal/dns/host_windows.go +++ b/client/internal/dns/host_windows.go @@ -35,6 +35,8 @@ var ( // exported so a diagnostic reader reports the same locations that are written. const ( // NRPTKeyPrefix starts the name of every NRPT rule key this client creates. + // Older versions used different layouts under the same prefix: a single + // unsuffixed key, then one key per domain, now one key per batch of domains. NRPTKeyPrefix = "NetBird-Match" // DNSPolicyConfigRoot holds the NRPT rules of the local policy store. @@ -89,7 +91,6 @@ type registryConfigurator struct { guid string routingAll bool gpo bool - nrptEntryCount int origNameservers []netip.Addr } @@ -322,14 +323,9 @@ func (r *registryConfigurator) applyDNSConfig(config HostDNSConfig, stateManager } if len(matchDomains) != 0 { - count, err := r.addDNSMatchPolicy(matchDomains, config.ServerIP) - // Update count even on error to ensure cleanup covers partially created rules - r.nrptEntryCount = count - if err != nil { + if err := r.addDNSMatchPolicy(matchDomains, config.ServerIP); err != nil { return fmt.Errorf("add dns match policy: %w", err) } - } else { - r.nrptEntryCount = 0 } r.updateState(stateManager) @@ -345,9 +341,8 @@ func (r *registryConfigurator) applyDNSConfig(config HostDNSConfig, stateManager func (r *registryConfigurator) updateState(stateManager *statemanager.Manager) { if err := stateManager.UpdateState(&ShutdownState{ - Guid: r.guid, - GPO: r.gpo, - NRPTEntryCount: r.nrptEntryCount, + Guid: r.guid, + GPO: r.gpo, }); err != nil { log.Errorf("failed to update shutdown state: %s", err) } @@ -362,7 +357,7 @@ func (r *registryConfigurator) addDNSSetupForAll(ip netip.Addr) error { return nil } -func (r *registryConfigurator) addDNSMatchPolicy(domains []string, ip netip.Addr) (int, error) { +func (r *registryConfigurator) addDNSMatchPolicy(domains []string, ip netip.Addr) error { // if the gpo key is present, we need to put our DNS settings there, otherwise our config might be ignored // see https://learn.microsoft.com/en-us/openspecs/windows_protocols/ms-gpnrpt/8cc31cb9-20cb-4140-9e85-3e08703b4745 @@ -379,19 +374,17 @@ func (r *registryConfigurator) addDNSMatchPolicy(domains []string, ip netip.Addr gpoPath := fmt.Sprintf("%s-%d", gpoDnsPolicyConfigMatchPath, ruleIndex) if err := r.configureDNSPolicy(localPath, batchDomains, ip); err != nil { - return ruleIndex, fmt.Errorf("configure DNS Local policy for rule %d: %w", ruleIndex, err) + return fmt.Errorf("configure DNS Local policy for rule %d: %w", ruleIndex, err) } - // Increment immediately so the caller's cleanup path knows about this rule - ruleIndex++ - if r.gpo { if err := r.configureDNSPolicy(gpoPath, batchDomains, ip); err != nil { - return ruleIndex, fmt.Errorf("configure gpo DNS policy for rule %d: %w", ruleIndex-1, err) + return fmt.Errorf("configure gpo DNS policy for rule %d: %w", ruleIndex, err) } } - log.Debugf("added NRPT rule %d with %d domains", ruleIndex-1, len(batchDomains)) + log.Debugf("added NRPT rule %d with %d domains", ruleIndex, len(batchDomains)) + ruleIndex++ } if r.gpo { @@ -401,7 +394,7 @@ func (r *registryConfigurator) addDNSMatchPolicy(domains []string, ip netip.Addr } log.Infof("added %d NRPT rules for %d domains", ruleIndex, len(domains)) - return ruleIndex, nil + return nil } func (r *registryConfigurator) configureDNSPolicy(policyPath string, domains []string, ip netip.Addr) error { @@ -534,28 +527,28 @@ func (r *registryConfigurator) restoreHostDNS() error { return nil } +// removeDNSMatchPolicies deletes every NRPT rule this client may have created, +// from the local and the GPO policy store. The rules are found by enumerating +// the registry, the only authoritative record of what was written. Cleanup must +// not depend on a rule count: the in-memory one is scoped to a single +// registryConfigurator and the persisted one is deleted on every clean +// disconnect, and a rule left behind keeps resolving names over an interface +// that is gone, until reboot discards the volatile key. func (r *registryConfigurator) removeDNSMatchPolicies() error { var merr *multierror.Error - // Try to remove the base entries (for backward compatibility) - if err := removeRegistryKeyFromDNSPolicyConfig(dnsPolicyConfigMatchPath); err != nil { - merr = multierror.Append(merr, fmt.Errorf("remove local base entry: %w", err)) - } - - if err := removeRegistryKeyFromDNSPolicyConfig(gpoDnsPolicyConfigMatchPath); err != nil { - merr = multierror.Append(merr, fmt.Errorf("remove GPO base entry: %w", err)) - } - - for i := 0; i < r.nrptEntryCount; i++ { - localPath := fmt.Sprintf("%s-%d", dnsPolicyConfigMatchPath, i) - gpoPath := fmt.Sprintf("%s-%d", gpoDnsPolicyConfigMatchPath, i) - - if err := removeRegistryKeyFromDNSPolicyConfig(localPath); err != nil { - merr = multierror.Append(merr, fmt.Errorf("remove local entry %d: %w", i, err)) + for _, root := range []string{DNSPolicyConfigRoot, GPODNSPolicyConfigRoot} { + names, err := listNRPTRuleKeys(root) + if err != nil { + merr = multierror.Append(merr, fmt.Errorf("list rule keys under %s: %w", root, err)) + continue } - if err := removeRegistryKeyFromDNSPolicyConfig(gpoPath); err != nil { - merr = multierror.Append(merr, fmt.Errorf("remove GPO entry %d: %w", i, err)) + for _, name := range names { + path := root + `\` + name + if err := removeRegistryKeyFromDNSPolicyConfig(path); err != nil { + merr = multierror.Append(merr, fmt.Errorf("remove entry %s: %w", path, err)) + } } } @@ -570,6 +563,39 @@ func (r *registryConfigurator) restoreUncleanShutdownDNS() error { return r.restoreHostDNS() } +// listNRPTRuleKeys returns the names of our NRPT rule keys under a policy store +// root. An absent root holds nothing to clean up, which is the normal state of +// the GPO store on a machine without DNS Client policy. +func listNRPTRuleKeys(root string) ([]string, error) { + k, err := registry.OpenKey(registry.LOCAL_MACHINE, root, registry.ENUMERATE_SUB_KEYS) + switch { + case errors.Is(err, registry.ErrNotExist), errors.Is(err, syscall.ERROR_PATH_NOT_FOUND): + // the GPO store is absent on a machine without DNS client policy + log.Debugf("HKEY_LOCAL_MACHINE\\%s does not exist", root) + return nil, nil + case err != nil: + // any other failure has to reach the caller: reporting no rules would + // report a successful cleanup while leaving the rules in place + return nil, fmt.Errorf("open HKEY_LOCAL_MACHINE\\%s: %w", root, err) + } + defer closer(k) + + names, err := k.ReadSubKeyNames(-1) + if err != nil { + return nil, fmt.Errorf("read subkey names: %w", err) + } + + var ruleKeys []string + for _, name := range names { + // registry key names are case insensitive + if strings.HasPrefix(strings.ToLower(name), strings.ToLower(NRPTKeyPrefix)) { + ruleKeys = append(ruleKeys, name) + } + } + + return ruleKeys, nil +} + func removeRegistryKeyFromDNSPolicyConfig(regKeyPath string) error { k, err := registry.OpenKey(registry.LOCAL_MACHINE, regKeyPath, registry.QUERY_VALUE) if err != nil { diff --git a/client/internal/dns/host_windows_test.go b/client/internal/dns/host_windows_test.go index 3cd2b1bd5..861613c95 100644 --- a/client/internal/dns/host_windows_test.go +++ b/client/internal/dns/host_windows_test.go @@ -25,7 +25,7 @@ func TestNRPTEntriesCleanupOnConfigChange(t *testing.T) { // Create a test interface registry key so updateSearchDomains doesn't fail testGUID := "{12345678-1234-1234-1234-123456789ABC}" - interfacePath := `SYSTEM\CurrentControlSet\Services\Tcpip\Parameters\Interfaces\` + testGUID + interfacePath := InterfaceConfigPath + `\` + testGUID testKey, _, err := registry.CreateKey(registry.LOCAL_MACHINE, interfacePath, registry.SET_VALUE) require.NoError(t, err, "Should create test interface registry key") testKey.Close() @@ -56,7 +56,7 @@ func TestNRPTEntriesCleanupOnConfigChange(t *testing.T) { require.NoError(t, err) // Verify 3 NRPT rules exist - assert.Equal(t, 3, cfg.nrptEntryCount, "Should create 3 NRPT rules for 125 domains") + assert.Equal(t, 3, countNRPTRuleKeys(t), "Should create 3 NRPT rules for 125 domains") for i := 0; i < 3; i++ { exists, err := registryKeyExists(fmt.Sprintf("%s-%d", dnsPolicyConfigMatchPath, i)) require.NoError(t, err) @@ -81,7 +81,7 @@ func TestNRPTEntriesCleanupOnConfigChange(t *testing.T) { require.NoError(t, err) // Verify first 2 NRPT rules exist - assert.Equal(t, 2, cfg.nrptEntryCount, "Should create 2 NRPT rules for 75 domains") + assert.Equal(t, 2, countNRPTRuleKeys(t), "Should create 2 NRPT rules for 75 domains") for i := 0; i < 2; i++ { exists, err := registryKeyExists(fmt.Sprintf("%s-%d", dnsPolicyConfigMatchPath, i)) require.NoError(t, err) @@ -106,9 +106,65 @@ func registryKeyExists(path string) (bool, error) { return true, nil } +// TestNRPTCleanupWithoutRuleCount verifies that rules written by a previous run +// are removed by a configurator that has no record of how many there are: an +// unclean exit loses the in-memory count and a clean disconnect deletes the +// persisted one, so cleanup cannot depend on either. +func TestNRPTCleanupWithoutRuleCount(t *testing.T) { + if testing.Short() { + t.Skip("skipping registry integration test in short mode") + } + + defer cleanupRegistryKeys(t) + cleanupRegistryKeys(t) + + testIP := netip.MustParseAddr("100.64.0.1") + + // 75 domains produce two indexed rules, as the current layout does + domains := make([]string, 75) + for i := range domains { + domains[i] = fmt.Sprintf(".domain%d.com", i+1) + } + + previousRun := ®istryConfigurator{} + require.NoError(t, previousRun.addDNSMatchPolicy(domains, testIP)) + + // the unsuffixed key an older version would have written + require.NoError(t, previousRun.configureDNSPolicy(dnsPolicyConfigMatchPath, []string{".legacy.example.com"}, testIP)) + + // a policy owned by someone else, which cleanup must not touch + foreignPath := DNSPolicyConfigRoot + `\DnsPolicyConfigTestForeign` + foreignKey, _, err := registry.CreateKey(registry.LOCAL_MACHINE, foreignPath, registry.SET_VALUE) + require.NoError(t, err, "Should create foreign policy key") + foreignKey.Close() + defer func() { + _ = registry.DeleteKey(registry.LOCAL_MACHINE, foreignPath) + }() + + require.Equal(t, 3, countNRPTRuleKeys(t), "Should have two indexed rules and the legacy one") + + // a configurator that never applied a DNS config, as one built after a + // restart or from a shutdown state without a count is + freshRun := ®istryConfigurator{} + require.NoError(t, freshRun.removeDNSMatchPolicies()) + + assert.Equal(t, 0, countNRPTRuleKeys(t), "Should remove every rule left by the previous run") + + exists, err := registryKeyExists(foreignPath) + require.NoError(t, err) + assert.True(t, exists, "Should not remove a policy that is not ours") +} + +func countNRPTRuleKeys(t *testing.T) int { + t.Helper() + + names, err := listNRPTRuleKeys(DNSPolicyConfigRoot) + require.NoError(t, err, "Should list NRPT rule keys") + return len(names) +} + func cleanupRegistryKeys(*testing.T) { - // Clean up more entries to account for batching tests with many domains - cfg := ®istryConfigurator{nrptEntryCount: 20} + cfg := ®istryConfigurator{} _ = cfg.removeDNSMatchPolicies() } @@ -125,7 +181,7 @@ func TestNRPTDomainBatching(t *testing.T) { // Create a test interface registry key so updateSearchDomains doesn't fail testGUID := "{12345678-1234-1234-1234-123456789ABC}" - interfacePath := `SYSTEM\CurrentControlSet\Services\Tcpip\Parameters\Interfaces\` + testGUID + interfacePath := InterfaceConfigPath + `\` + testGUID testKey, _, err := registry.CreateKey(registry.LOCAL_MACHINE, interfacePath, registry.SET_VALUE) require.NoError(t, err, "Should create test interface registry key") testKey.Close() @@ -193,7 +249,7 @@ func TestNRPTDomainBatching(t *testing.T) { require.NoError(t, err) // Verify that exactly expectedRuleCount rules were created - assert.Equal(t, tc.expectedRuleCount, cfg.nrptEntryCount, + assert.Equal(t, tc.expectedRuleCount, countNRPTRuleKeys(t), "Should create %d NRPT rules for %d domains", tc.expectedRuleCount, tc.domainCount) // Verify all expected rules exist diff --git a/client/internal/dns/unclean_shutdown_windows.go b/client/internal/dns/unclean_shutdown_windows.go index 24a9eca50..ab0b2cc63 100644 --- a/client/internal/dns/unclean_shutdown_windows.go +++ b/client/internal/dns/unclean_shutdown_windows.go @@ -5,9 +5,8 @@ import ( ) type ShutdownState struct { - Guid string - GPO bool - NRPTEntryCount int + Guid string + GPO bool } func (s *ShutdownState) Name() string { @@ -16,9 +15,8 @@ func (s *ShutdownState) Name() string { func (s *ShutdownState) Cleanup() error { manager := ®istryConfigurator{ - guid: s.Guid, - gpo: s.GPO, - nrptEntryCount: s.NRPTEntryCount, + guid: s.Guid, + gpo: s.GPO, } if err := manager.restoreUncleanShutdownDNS(); err != nil { diff --git a/client/internal/ebpf/ebpf/manager_linux.go b/client/internal/ebpf/ebpf/manager_linux.go index 7520a6387..64a3e5b54 100644 --- a/client/internal/ebpf/ebpf/manager_linux.go +++ b/client/internal/ebpf/ebpf/manager_linux.go @@ -2,17 +2,21 @@ package ebpf import ( _ "embed" + "fmt" "net" "sync" "github.com/cilium/ebpf/link" "github.com/cilium/ebpf/rlimit" log "github.com/sirupsen/logrus" + "golang.org/x/sys/unix" "github.com/netbirdio/netbird/client/internal/ebpf/manager" ) const ( + xdpProgName = "nb_xdp_prog" + mapKeyFeatures uint32 = 0 featureFlagWGProxy = 0b00000001 @@ -68,21 +72,50 @@ func (tf *GeneralManager) loadXdp() error { return err } - // load pre-compiled programs into the kernel. - err = loadBpfObjects(&tf.bpfObjs, nil) + // lo has no native XDP, so the program runs in generic mode. Unless it + // declares multi-buffer support the kernel must linearize every non-linear + // skb before running it. Loopback packets are up to 64 KB, so that is a + // contiguous GFP_ATOMIC allocation per packet, and when it fails the packet + // is dropped before the program runs, stalling local TCP connections. + // Multi-buffer XDP in generic mode requires kernel 6.3, so fall back to a + // plain attach when the kernel rejects it. + err = tf.attachXdp(iFace.Index, true) + if err == nil { + return nil + } + log.Debugf("failed to attach multi-buffer xdp program, retrying without it: %s", err) + + return tf.attachXdp(iFace.Index, false) +} + +func (tf *GeneralManager) attachXdp(iFaceIndex int, multiBuffer bool) error { + spec, err := loadBpf() if err != nil { - return err + return fmt.Errorf("load bpf spec: %w", err) + } + + if multiBuffer { + prog, ok := spec.Programs[xdpProgName] + if !ok { + return fmt.Errorf("program %s not found in bpf spec", xdpProgName) + } + prog.Flags |= unix.BPF_F_XDP_HAS_FRAGS + } + + if err := spec.LoadAndAssign(&tf.bpfObjs, nil); err != nil { + return fmt.Errorf("load bpf objects: %w", err) } tf.link, err = link.AttachXDP(link.XDPOptions{ Program: tf.bpfObjs.NbXdpProg, - Interface: iFace.Index, + Interface: iFaceIndex, }) - if err != nil { - _ = tf.bpfObjs.Close() + if closeErr := tf.bpfObjs.Close(); closeErr != nil { + log.Debugf("failed to close bpf objects after xdp attach error: %s", closeErr) + } tf.link = nil - return err + return fmt.Errorf("attach xdp: %w", err) } return nil } diff --git a/go.mod b/go.mod index 03a41628d..9ce842401 100644 --- a/go.mod +++ b/go.mod @@ -19,7 +19,7 @@ require ( github.com/spf13/cobra v1.10.2 github.com/spf13/pflag v1.0.10 github.com/vishvananda/netlink v1.3.1 - golang.org/x/crypto v0.54.0 + golang.org/x/crypto v0.55.0 golang.org/x/sys v0.47.0 golang.zx2c4.com/wireguard v0.0.0-20231211153847-12269c276173 golang.zx2c4.com/wireguard/wgctrl v0.0.0-20241231184526-a9ab2273dd10 @@ -128,9 +128,9 @@ require ( go.uber.org/zap v1.27.0 goauthentik.io/api/v3 v3.2023051.3 golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f - golang.org/x/mobile v0.0.0-20251113184115-a159579294ab - golang.org/x/mod v0.37.0 - golang.org/x/net v0.56.0 + golang.org/x/mobile v0.0.0-20260816165457-f98cc9b3c733 + golang.org/x/mod v0.39.0 + golang.org/x/net v0.58.0 golang.org/x/oauth2 v0.36.0 golang.org/x/sync v0.22.0 golang.org/x/term v0.45.0 @@ -314,8 +314,8 @@ require ( go.opentelemetry.io/otel/trace v1.43.0 // indirect go.uber.org/multierr v1.11.0 // indirect go.yaml.in/yaml/v2 v2.4.3 // indirect - golang.org/x/text v0.40.0 // indirect - golang.org/x/tools v0.47.0 // indirect + golang.org/x/text v0.41.0 // indirect + golang.org/x/tools v0.49.0 // indirect golang.zx2c4.com/wintun v0.0.0-20230126152724-0fa3db229ce2 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260319201613-d00831a3d3e7 // indirect gopkg.in/square/go-jose.v2 v2.6.0 // indirect diff --git a/go.sum b/go.sum index 61e1bbc8b..64bee213c 100644 --- a/go.sum +++ b/go.sum @@ -730,13 +730,13 @@ golang.org/x/crypto v0.18.0/go.mod h1:R0j02AL6hcrfOiy9T4ZYp/rcWeMxM3L6QYxlOuEG1m golang.org/x/crypto v0.19.0/go.mod h1:Iy9bg/ha4yyC70EfRS8jz+B6ybOBKMaSxLj6P6oBDfU= golang.org/x/crypto v0.23.0/go.mod h1:CKFgDieR+mRhux2Lsu27y0fO304Db0wZe70UKqHu0v8= golang.org/x/crypto v0.31.0/go.mod h1:kDsLvtWBEx7MV9tJOj9bnXsPbxwJQ6csT/x4KIN4Ssk= -golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= -golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= +golang.org/x/crypto v0.55.0 h1:+KWHjbgOaAQ66dh/YlkZKHlz9ZUlq61AFirAR9ntP8M= +golang.org/x/crypto v0.55.0/go.mod h1:uq0V9dE/fzQuJtbnL+2EhWOE63vo164FY8xqEnV9xis= golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f h1:W3F4c+6OLc6H2lb//N1q4WpJkhzJCK5J6kUi1NTVXfM= golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f/go.mod h1:J1xhfL/vlindoeF/aINzNzt2Bket5bjo9sdOYzOsU80= golang.org/x/lint v0.0.0-20200302205851-738671d3881b/go.mod h1:3xt1FjdF8hUf6vQPIChWIBhFzV8gjjsPE/fR3IyQdNY= -golang.org/x/mobile v0.0.0-20251113184115-a159579294ab h1:Iqyc+2zr7aGyLuEadIm0KRJP0Wwt+fhlXLa51Fxf1+Q= -golang.org/x/mobile v0.0.0-20251113184115-a159579294ab/go.mod h1:Eq3Nh/5pFSWug2ohiudJ1iyU59SO78QFuh4qTTN++I0= +golang.org/x/mobile v0.0.0-20260816165457-f98cc9b3c733 h1:XKMObIaAElmkdO+4SQh1iCfzwciZHJi1OblnX9BED9k= +golang.org/x/mobile v0.0.0-20260816165457-f98cc9b3c733/go.mod h1:jMwjxoDSx9jqhNaZqPnr6nnKzb7cs+Dy1Czk7wdX+R8= golang.org/x/mod v0.1.1-0.20191105210325-c90efee705ee/go.mod h1:QqPTAvyqsEbceGzBzNggFXnrqF1CaUcvgkdR5Ot7KZg= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= @@ -746,8 +746,8 @@ golang.org/x/mod v0.8.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.12.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/mod v0.15.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= golang.org/x/mod v0.17.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c= -golang.org/x/mod v0.37.0 h1:vF1DjpVEshcIqoEaauuHebaLk1O1forxjxBaVn884JQ= -golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0= +golang.org/x/mod v0.39.0 h1:UF5zwQdCRRUpHfyPwr7d4UrGiVeldIsogtzWVnczL74= +golang.org/x/mod v0.39.0/go.mod h1:bvIbwjQ0HUFFf5AKukeeYQG4ZBUG9yxQbR9aEweIwYY= golang.org/x/net v0.0.0-20180906233101-161cd47e91fd/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4= golang.org/x/net v0.0.0-20190404232315-eb5bcb51f2a3/go.mod h1:t9HGtf8HONx5eT2rtn7q6eTqICYqUVnKs3thJo3Qplg= golang.org/x/net v0.0.0-20190603091049-60506f45cf65/go.mod h1:HSz+uSET+XFnRR8LxR5pz3Of3rY3CfYBVs4xY44aLks= @@ -766,8 +766,8 @@ golang.org/x/net v0.15.0/go.mod h1:idbUs1IY1+zTqbi8yxTbhexhEEk5ur9LInksu6HrEpk= golang.org/x/net v0.20.0/go.mod h1:z8BVo6PvndSri0LbOE3hAn0apkU+1YvI6E70E9jsnvY= golang.org/x/net v0.21.0/go.mod h1:bIjVDfnllIU7BJ2DNgfnXvpSvtn8VRwhlsaeUTyUS44= golang.org/x/net v0.25.0/go.mod h1:JkAGAh7GEvH74S6FOH42FLoXpXbE/aqXSrIQjXgsiwM= -golang.org/x/net v0.56.0 h1:Rw8j/hFzGvJUZwNBXnAtf5sVDVt+65SK2C7IxCxZt5o= -golang.org/x/net v0.56.0/go.mod h1:D3Ku6r+V6JROoZK144D2XfMHFcMq/0zSfLelVTCFKec= +golang.org/x/net v0.58.0 h1:ynWG7rqYi4ccpTEuPZ2QGWHktVEM9DMCj9yzDE0Q7To= +golang.org/x/net v0.58.0/go.mod h1:YwCddHnFlT7eLQqVprV19OnhLGtc5xOKgE0RyqgfWAU= golang.org/x/oauth2 v0.8.0/go.mod h1:yr7u4HXZRm1R1kBWqr/xKNqewf0plRYoB7sla+BCIXE= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= @@ -845,8 +845,8 @@ golang.org/x/text v0.13.0/go.mod h1:TvPlkZtksWOMsz7fbANvkp4WM8x/WCo/om8BMLbz+aE= golang.org/x/text v0.14.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU= golang.org/x/text v0.21.0/go.mod h1:4IBbMaMmOPCJ8SecivzSH54+73PCFmPWxNTLm+vZkEQ= -golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= -golang.org/x/text v0.40.0/go.mod h1:hpnzDAfGV753zIKo+wk3u1bVKCGPbrnF7+7LBF/UHVY= +golang.org/x/text v0.41.0 h1:vz/seA0lnX87Othu2f/0L24RcgrXD9/YFTSuGjj3rH8= +golang.org/x/text v0.41.0/go.mod h1:jvf1O8ajNzZqhSrQBPbutR/EB83Cc0CFrezNQIwbb5M= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= @@ -860,8 +860,8 @@ golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc golang.org/x/tools v0.6.0/go.mod h1:Xwgl3UAJ/d3gWutnCtw505GrjyAbvKui8lOU390QaIU= golang.org/x/tools v0.13.0/go.mod h1:HvlwmtVNQAhOuCjW7xxvovg8wbNq7LwfXh/k7wXUl58= golang.org/x/tools v0.21.1-0.20240508182429-e35e4ccd0d2d/go.mod h1:aiJjzUbINMkxbQROHiO6hDPo2LHcIPhhQsa9DLh0yGk= -golang.org/x/tools v0.47.0 h1:7Kn5x/d1svx/PzryTsqeoZN4TZwqeH5pGWjefhLi/1Q= -golang.org/x/tools v0.47.0/go.mod h1:dFHnyTvFWY212G+h7ZY4Vsp/K3U4/7W9TyVaAul8uCA= +golang.org/x/tools v0.49.0 h1:3NI7VXzL9+1WZD52Dx2ttoPwD5DWrFGpl9mFZDlmisI= +golang.org/x/tools v0.49.0/go.mod h1:SJNXV9DBKT0UbdttsQjbfJlAE/q+y36++zo3uL3N0Oo= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/management/server/http/handlers/setup_keys/setupkeys_handler.go b/management/server/http/handlers/setup_keys/setupkeys_handler.go index d267b6eea..bb498f46b 100644 --- a/management/server/http/handlers/setup_keys/setupkeys_handler.go +++ b/management/server/http/handlers/setup_keys/setupkeys_handler.go @@ -64,6 +64,19 @@ func (h *handler) createSetupKey(w http.ResponseWriter, r *http.Request) { return } + // A one-off key can be used once, and GenerateSetupKey pins its usage limit + // at 1 whatever the request says. Silently overriding a caller that asked + // for a different number leaves them holding a key that does not do what + // they configured, and no way to find out except by using it. Only values + // above 1 are refused: usage_limit is a required field with no null, so 0 + // cannot be told apart from a caller that has nothing to say about it. + if types.SetupKeyType(req.Type) == types.SetupKeyOneOff && req.UsageLimit > 1 { + util.WriteError(r.Context(), status.Errorf(status.InvalidArgument, + "usage_limit %d is not valid for a one-off setup key, which can be used once; use type reusable for a key that can be used more than once", + req.UsageLimit), w) + return + } + expiresIn := time.Duration(req.ExpiresIn) * time.Second if expiresIn < 0 { diff --git a/management/server/http/handlers/setup_keys/setupkeys_handler_test.go b/management/server/http/handlers/setup_keys/setupkeys_handler_test.go index b137b6dd1..a9cfd4bd3 100644 --- a/management/server/http/handlers/setup_keys/setupkeys_handler_test.go +++ b/management/server/http/handlers/setup_keys/setupkeys_handler_test.go @@ -134,6 +134,40 @@ func TestSetupKeysHandlers(t *testing.T) { expectedBody: true, expectedSetupKey: expectedNewKey, }, + { + // A one-off key is used once. Asking for more used to be accepted + // and then quietly reduced to 1. + name: "Create One-Off Setup Key With Conflicting Usage Limit", + requestType: http.MethodPost, + requestPath: "/api/setup-keys", + requestBody: bytes.NewBuffer( + []byte(fmt.Sprintf("{\"name\":\"%s\",\"type\":\"one-off\",\"expires_in\":86400,\"usage_limit\":5}", newSetupKeyName))), + expectedStatus: http.StatusUnprocessableEntity, + expectedBody: false, + }, + { + // 0 is what a caller sends when it has nothing to say about the + // usage limit, since the field is required and has no null, so it + // has to keep working. + name: "Create One-Off Setup Key Without Usage Limit", + requestType: http.MethodPost, + requestPath: "/api/setup-keys", + requestBody: bytes.NewBuffer( + []byte(fmt.Sprintf("{\"name\":\"%s\",\"type\":\"one-off\",\"expires_in\":86400,\"usage_limit\":0}", newSetupKeyName))), + expectedStatus: http.StatusOK, + expectedBody: false, + }, + { + // Only one-off keys are constrained; a reusable key means what it + // says. + name: "Create Reusable Setup Key With Usage Limit", + requestType: http.MethodPost, + requestPath: "/api/setup-keys", + requestBody: bytes.NewBuffer( + []byte(fmt.Sprintf("{\"name\":\"%s\",\"type\":\"reusable\",\"expires_in\":86400,\"usage_limit\":5}", newSetupKeyName))), + expectedStatus: http.StatusOK, + expectedBody: false, + }, { name: "Update Setup Key", requestType: http.MethodPut, diff --git a/management/server/http/testing/integration/setupkeys_handler_integration_test.go b/management/server/http/testing/integration/setupkeys_handler_integration_test.go index 0d3aaac82..21ad9d347 100644 --- a/management/server/http/testing/integration/setupkeys_handler_integration_test.go +++ b/management/server/http/testing/integration/setupkeys_handler_integration_test.go @@ -136,7 +136,10 @@ func Test_SetupKeys_Create(t *testing.T) { }, }, { - name: "Create Setup Key as on-off with more than one usage", + // The key used to be created anyway, with its usage limit quietly + // reduced to 1, so the caller was told a key they had not asked for + // was what they asked for. + name: "Create Setup Key as one-off with more than one usage", requestType: http.MethodPost, requestPath: "/api/setup-keys", requestBody: &api.CreateSetupKeyRequest{ @@ -146,23 +149,7 @@ func Test_SetupKeys_Create(t *testing.T) { Type: "one-off", UsageLimit: 3, }, - expectedStatus: http.StatusOK, - expectedResponse: &api.SetupKey{ - AutoGroups: []string{}, - Ephemeral: false, - Expires: time.Time{}, - Id: "", - Key: "", - LastUsed: time.Time{}, - Name: testing_tools.NewKeyName, - Revoked: false, - State: "valid", - Type: "one-off", - UpdatedAt: time.Now(), - UsageLimit: 1, - UsedTimes: 0, - Valid: true, - }, + expectedStatus: http.StatusUnprocessableEntity, }, { name: "Create Setup Key with expiration in the past",