From f5ce0bc65a172f81702c9f8043e141171dc561ba Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:25:12 +0900 Subject: [PATCH 01/11] [client] Fix macOS DNS panic on malformed scutil output (#7180) --- client/internal/dns/host_darwin.go | 87 ++++++++++++------ client/internal/dns/host_darwin_test.go | 114 ++++++++++++++++++++++++ 2 files changed, 174 insertions(+), 27 deletions(-) diff --git a/client/internal/dns/host_darwin.go b/client/internal/dns/host_darwin.go index 0f4eb6bf8..81029752e 100644 --- a/client/internal/dns/host_darwin.go +++ b/client/internal/dns/host_darwin.go @@ -267,18 +267,38 @@ func (s *systemConfigurator) getSystemDNSSettings() (SystemDNSSettings, error) { return SystemDNSSettings{}, fmt.Errorf("sending the command: %w", err) } - var dnsSettings SystemDNSSettings + dnsSettings, serverAddresses, err := parseSystemDNSSettings(b) + if err != nil { + return dnsSettings, err + } + + s.mu.Lock() + s.origNameservers = serverAddresses + s.mu.Unlock() + + return dnsSettings, nil +} + +// parseSystemDNSSettings parses the output of `scutil show State:/Network/Service//DNS`. +// Lines that don't match the expected "index : value" shape are skipped: hosts with unusual +// network services (e.g. orphaned hardware ports) can produce entries without a value. +func parseSystemDNSSettings(out []byte) (SystemDNSSettings, []netip.Addr, error) { + // port is not exposed by scutil, default to 53 + dnsSettings := SystemDNSSettings{ServerPort: DefaultPort} var serverAddresses []netip.Addr inSearchDomainsArray := false inServerAddressesArray := false - scanner := bufio.NewScanner(bytes.NewReader(b)) + scanner := bufio.NewScanner(bytes.NewReader(out)) for scanner.Scan() { line := strings.TrimSpace(scanner.Text()) switch { case strings.HasPrefix(line, "DomainName :"): - domainName := strings.TrimSpace(strings.Split(line, ":")[1]) - dnsSettings.Domains = append(dnsSettings.Domains, domainName) + domainName := strings.TrimSpace(strings.TrimPrefix(line, "DomainName :")) + if domainName != "" { + dnsSettings.Domains = append(dnsSettings.Domains, domainName) + } + continue case line == "SearchDomains : {": inSearchDomainsArray = true continue @@ -288,36 +308,45 @@ func (s *systemConfigurator) getSystemDNSSettings() (SystemDNSSettings, error) { case line == "}": inSearchDomainsArray = false inServerAddressesArray = false + continue + } + + if !inSearchDomainsArray && !inServerAddressesArray { + continue + } + + parts := strings.SplitN(line, " : ", 2) + if len(parts) != 2 { + log.Debugf("skipping unexpected scutil DNS line %q", line) + continue + } + value := strings.TrimSpace(parts[1]) + if value == "" { + continue } if inSearchDomainsArray { - searchDomain := strings.Split(line, " : ")[1] - dnsSettings.Domains = append(dnsSettings.Domains, searchDomain) - } else if inServerAddressesArray { - address := strings.Split(line, " : ")[1] - if ip, err := netip.ParseAddr(address); err == nil && !ip.IsUnspecified() { - ip = ip.Unmap() - serverAddresses = append(serverAddresses, ip) - // Prefer the first IPv4 server as ServerIP since our DNS listener is IPv4. - if !dnsSettings.ServerIP.IsValid() && ip.Is4() { - dnsSettings.ServerIP = ip - } - } + dnsSettings.Domains = append(dnsSettings.Domains, value) + continue + } + + ip, err := netip.ParseAddr(value) + if err != nil || ip.IsUnspecified() { + continue + } + ip = ip.Unmap() + serverAddresses = append(serverAddresses, ip) + // Prefer the first IPv4 server as ServerIP since our DNS listener is IPv4. + if !dnsSettings.ServerIP.IsValid() && ip.Is4() { + dnsSettings.ServerIP = ip } } if err := scanner.Err(); err != nil { - return dnsSettings, err + return dnsSettings, serverAddresses, err } - // default to 53 port - dnsSettings.ServerPort = DefaultPort - - s.mu.Lock() - s.origNameservers = serverAddresses - s.mu.Unlock() - - return dnsSettings, nil + return dnsSettings, serverAddresses, nil } func (s *systemConfigurator) getOriginalNameservers() []netip.Addr { @@ -435,11 +464,15 @@ func (s *systemConfigurator) getPrimaryService() (string, string, error) { router := "" for scanner.Scan() { text := scanner.Text() + parts := strings.SplitN(text, ":", 2) + if len(parts) != 2 { + continue + } if strings.Contains(text, "PrimaryService") { - primaryService = strings.TrimSpace(strings.Split(text, ":")[1]) + primaryService = strings.TrimSpace(parts[1]) } if strings.Contains(text, "Router") { - router = strings.TrimSpace(strings.Split(text, ":")[1]) + router = strings.TrimSpace(parts[1]) } } if err := scanner.Err(); err != nil && err != io.EOF { diff --git a/client/internal/dns/host_darwin_test.go b/client/internal/dns/host_darwin_test.go index 94d020c39..bee691c71 100644 --- a/client/internal/dns/host_darwin_test.go +++ b/client/internal/dns/host_darwin_test.go @@ -328,6 +328,120 @@ func removeTestDNSKey(key string) error { return err } +func TestParseSystemDNSSettings(t *testing.T) { + tests := []struct { + name string + output string + expectedDomains []string + expectedServers []netip.Addr + expectedIP netip.Addr + }{ + { + name: "well_formed", + output: ` { + DomainName : example.com + SearchDomains : { + 0 : example.com + 1 : corp.example.com + } + ServerAddresses : { + 0 : 192.168.1.1 + 1 : fd00::53 + } +} +`, + expectedDomains: []string{"example.com", "example.com", "corp.example.com"}, + expectedServers: []netip.Addr{netip.MustParseAddr("192.168.1.1"), netip.MustParseAddr("fd00::53")}, + expectedIP: netip.MustParseAddr("192.168.1.1"), + }, + { + // entries without a value after the separator used to panic with + // "index out of range [1] with length 1" + name: "malformed_array_entries_skipped", + output: ` { + SearchDomains : { + 0 : + (null) + + 1 : corp.example.com + } + ServerAddresses : { + 0 : + 1 : 192.168.1.1 + } +} +`, + expectedDomains: []string{"corp.example.com"}, + expectedServers: []netip.Addr{netip.MustParseAddr("192.168.1.1")}, + expectedIP: netip.MustParseAddr("192.168.1.1"), + }, + { + name: "domain_name_without_value_skipped", + output: ` { + DomainName : + ServerAddresses : { + 0 : 192.168.1.1 + } +} +`, + expectedServers: []netip.Addr{netip.MustParseAddr("192.168.1.1")}, + expectedIP: netip.MustParseAddr("192.168.1.1"), + }, + { + name: "ipv6_first_prefers_ipv4_server_ip", + output: ` { + ServerAddresses : { + 0 : fd00::53 + 1 : 192.168.1.1 + } +} +`, + expectedServers: []netip.Addr{netip.MustParseAddr("fd00::53"), netip.MustParseAddr("192.168.1.1")}, + expectedIP: netip.MustParseAddr("192.168.1.1"), + }, + { + name: "invalid_and_unspecified_addresses_skipped", + output: ` { + ServerAddresses : { + 0 : (null) + 1 : 0.0.0.0 + 2 : 192.168.1.1 + } +} +`, + expectedServers: []netip.Addr{netip.MustParseAddr("192.168.1.1")}, + expectedIP: netip.MustParseAddr("192.168.1.1"), + }, + { + name: "v4_mapped_address_unmapped", + output: ` { + ServerAddresses : { + 0 : ::ffff:192.168.1.1 + } +} +`, + expectedServers: []netip.Addr{netip.MustParseAddr("192.168.1.1")}, + expectedIP: netip.MustParseAddr("192.168.1.1"), + }, + { + name: "empty_output", + output: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + settings, servers, err := parseSystemDNSSettings([]byte(tc.output)) + require.NoError(t, err, "parsing should not fail") + + assert.Equal(t, tc.expectedDomains, settings.Domains, "domains should match") + assert.Equal(t, tc.expectedServers, servers, "server addresses should match") + assert.Equal(t, tc.expectedIP, settings.ServerIP, "server IP should match") + assert.Equal(t, DefaultPort, settings.ServerPort, "server port should default to 53") + }) + } +} + func TestGetOriginalNameservers(t *testing.T) { configurator := &systemConfigurator{ createdKeys: make(map[string]struct{}), From 52faa202b2d66fcefbaedd00df3cd4d60a817a7e Mon Sep 17 00:00:00 2001 From: Lamera Date: Wed, 12 Aug 2026 14:37:48 +0200 Subject: [PATCH 02/11] [client] fall back to per-IP ACL rules when ipset is unavailable (#6332) --- client/firewall/iptables/acl_linux.go | 43 +++++++++++++++++++ .../firewall/iptables/manager_linux_test.go | 37 ++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/client/firewall/iptables/acl_linux.go b/client/firewall/iptables/acl_linux.go index 4b4cebf9c..89d1ebf7c 100644 --- a/client/firewall/iptables/acl_linux.go +++ b/client/firewall/iptables/acl_linux.go @@ -42,6 +42,7 @@ type aclManager struct { optionalEntries map[string][]entry ipsetStore *ipsetStore v6 bool + ipsetSupported bool stateManager *statemanager.Manager } @@ -60,6 +61,8 @@ func newAclManager(iptablesClient *iptables.IPTables, wgIface iFaceMapper) (*acl func (m *aclManager) init(stateManager *statemanager.Manager) error { m.stateManager = stateManager + m.ipsetSupported = m.probeIPSetSupport() + m.seedInitialEntries() m.seedInitialOptionalEntries() @@ -91,6 +94,12 @@ func (m *aclManager) AddPeerFiltering( if m.v6 && ipsetName != "" { ipsetName += "-v6" } + // When the kernel lacks the required ipset hash module, fall back to + // per-IP iptables rules (pre-0.68 behavior) so ACLs keep working instead + // of silently leaving the chain empty. + if ipsetName != "" && !m.ipsetSupported { + ipsetName = "" + } proto := protoForFamily(protocol, m.v6) specs := filterRuleSpecs(ip, proto, sPort, dPort, action, ipsetName) @@ -498,6 +507,40 @@ func transformIPsetName(ipsetName string, sPort, dPort *firewall.Port, action fi } } +// probeIPSetSupport checks whether the kernel can create the ipset type used for +// ACL rules. On kernels lacking the required ipset hash module, ipset creation +// fails (e.g. "invalid argument"), which would otherwise leave the ACL chain +// empty and silently drop all policy-permitted inbound traffic. When unsupported, +// the manager falls back to per-IP iptables rules. +func (m *aclManager) probeIPSetSupport() bool { + // Use a unique name so concurrent processes don't collide and we only ever + // destroy the set we created ourselves. ipset names are limited to 31 chars, + // so use a short random suffix. + probeName := "nb-probe-" + uuid.New().String()[:8] + + opts := ipset.CreateOptions{ + Replace: true, + } + if m.v6 { + opts.Family = ipset.FamilyIPV6 + } + + if err := ipset.Create(probeName, ipset.TypeHashNet, opts); err != nil { + log.Warnf("ipset is not available (failed to create probe set: %v); "+ + "falling back to per-IP iptables ACL rules. Ensure the kernel provides "+ + "the ipset hash:net module (ip_set_hash_net) for better performance with large rule sets", err) + return false + } + + defer func() { + if err := ipset.Destroy(probeName); err != nil { + log.Debugf("destroy ipset probe set %q: %v", probeName, err) + } + }() + + return true +} + func (m *aclManager) createIPSet(name string) error { opts := ipset.CreateOptions{ Replace: true, diff --git a/client/firewall/iptables/manager_linux_test.go b/client/firewall/iptables/manager_linux_test.go index 7b0989f6c..2c3c1a08e 100644 --- a/client/firewall/iptables/manager_linux_test.go +++ b/client/firewall/iptables/manager_linux_test.go @@ -291,3 +291,40 @@ func TestIptablesCreatePerformance(t *testing.T) { }) } } + +// TestIptablesACLIPSetFallback verifies that when the kernel lacks ipset support, +// the ACL manager falls back to per-IP iptables rules (-s ) instead of +// silently leaving the chain empty. See discussion #6125. +func TestIptablesACLIPSetFallback(t *testing.T) { + ipv4Client, err := iptables.NewWithProtocol(iptables.ProtocolIPv4) + require.NoError(t, err) + + // Use Create()/Init() so the router-owned chains (chainRTFWDIN/OUT) are + // created before the ACL manager's createDefaultChains() references them. + manager, err := Create(ifaceMock, iface.DefaultMTU) + require.NoError(t, err) + require.NoError(t, manager.Init(nil)) + + aclMgr := manager.aclMgr + // Simulate a kernel without the ipset hash module. + aclMgr.ipsetSupported = false + + defer func() { + require.NoError(t, manager.Close(nil)) + }() + + ip := netip.MustParseAddr("10.20.0.42") + port := &fw.Port{Values: []uint16{22}} + + rules, err := aclMgr.AddPeerFiltering(nil, ip.AsSlice(), "tcp", nil, port, fw.ActionAccept, "nb0000001") + require.NoError(t, err, "AddPeerFiltering should succeed via fallback") + require.NotEmpty(t, rules) + + rule := rules[0].(*Rule) + require.Empty(t, rule.ipsetName, "fallback rule must not reference an ipset") + require.Contains(t, strings.Join(rule.specs, " "), "-s 10.20.0.42", "fallback rule must match by source IP") + require.NotContains(t, strings.Join(rule.specs, " "), "--match-set", "fallback rule must not use ipset matching") + + // The rule must actually be present in the ACL chain (not silently dropped). + checkRuleSpecs(t, ipv4Client, rule.chain, true, rule.specs...) +} From db9fcf39ef82f000fb5ba6a316108742bd10745e Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:07:00 +0900 Subject: [PATCH 03/11] [client] Gate IPv6 forwarding on overlay v6 and preserve host RA acceptance (#6221) --- .../iptables/dnat_refcount_linux_test.go | 240 +++++++++++++++++ client/firewall/iptables/manager_linux.go | 13 +- client/firewall/iptables/router_linux.go | 57 ++-- .../nftables/dnat_refcount_linux_test.go | 249 ++++++++++++++++++ client/firewall/nftables/manager_linux.go | 15 +- client/firewall/nftables/router_linux.go | 39 ++- client/internal/debug/debug_linux.go | 4 + .../routemanager/ipfwdstate/ipfwdstate.go | 189 ++++++++++--- .../ipfwdstate_privileged_linux_test.go | 39 +++ .../routemanager/sysctl/sysctl_linux.go | 13 +- .../systemops/systemops_android.go | 13 +- .../routemanager/systemops/systemops_ios.go | 13 +- .../routemanager/systemops/systemops_linux.go | 5 +- .../systemops/systemops_nonlinux.go | 13 +- .../systemops/v6forwarding_linux.go | 92 +++++++ 15 files changed, 906 insertions(+), 88 deletions(-) create mode 100644 client/firewall/iptables/dnat_refcount_linux_test.go create mode 100644 client/firewall/nftables/dnat_refcount_linux_test.go create mode 100644 client/internal/routemanager/ipfwdstate/ipfwdstate_privileged_linux_test.go create mode 100644 client/internal/routemanager/systemops/v6forwarding_linux.go diff --git a/client/firewall/iptables/dnat_refcount_linux_test.go b/client/firewall/iptables/dnat_refcount_linux_test.go new file mode 100644 index 000000000..681bc0b99 --- /dev/null +++ b/client/firewall/iptables/dnat_refcount_linux_test.go @@ -0,0 +1,240 @@ +//go:build privileged + +package iptables + +import ( + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + fw "github.com/netbirdio/netbird/client/firewall/manager" + "github.com/netbirdio/netbird/client/iface" + "github.com/netbirdio/netbird/client/iface/wgaddr" +) + +func iptRefcountIfaceV4() *iFaceMock { + return &iFaceMock{ + NameFunc: func() string { return "wt-refcount" }, + AddressFunc: func() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("10.20.0.1"), + Network: netip.MustParsePrefix("10.20.0.0/24"), + } + }, + } +} + +func iptRefcountIfaceDual() *iFaceMock { + return &iFaceMock{ + NameFunc: func() string { return "wt-refcount" }, + AddressFunc: func() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("10.20.0.1"), + Network: netip.MustParsePrefix("10.20.0.0/24"), + IPv6: netip.MustParseAddr("fd00::1"), + IPv6Net: netip.MustParsePrefix("fd00::/64"), + } + }, + } +} + +func newIptRefcountManager(t *testing.T, dual bool) *Manager { + t.Helper() + var ifMock *iFaceMock + if dual { + ifMock = iptRefcountIfaceDual() + } else { + ifMock = iptRefcountIfaceV4() + } + m, err := Create(ifMock, iface.DefaultMTU) + require.NoError(t, err, "create manager") + require.NoError(t, m.Init(nil), "init manager") + t.Cleanup(func() { + require.NoError(t, m.Close(nil), "close manager") + }) + return m +} + +func iptDnatV4(port uint16) fw.ForwardRule { + return fw.ForwardRule{ + Protocol: fw.ProtocolTCP, + DestinationPort: fw.Port{Values: []uint16{port}}, + TranslatedAddress: netip.MustParseAddr("10.20.0.2"), + TranslatedPort: fw.Port{Values: []uint16{80}}, + } +} + +func iptDnatV6(port uint16) fw.ForwardRule { + return fw.ForwardRule{ + Protocol: fw.ProtocolTCP, + DestinationPort: fw.Port{Values: []uint16{port}}, + TranslatedAddress: netip.MustParseAddr("fd00::2"), + TranslatedPort: fw.Port{Values: []uint16{80}}, + } +} + +// TestIptablesRouting_RepeatedEnableSingleReference verifies that EnableRouting +// (called on every network-map update) holds at most one reference per family +// and a single DisableRouting drops both back to zero. +func TestIptablesRouting_RepeatedEnableSingleReference(t *testing.T) { + m := newIptRefcountManager(t, true) + state := m.router.ipFwdState + + require.NoError(t, m.EnableRouting(), "first enable") + require.NoError(t, m.EnableRouting(), "second enable") + require.NoError(t, m.EnableRouting(), "third enable") + v4, v6 := state.Counts() + assert.Equal(t, 1, v4, "repeated enable holds a single v4 reference") + assert.Equal(t, 1, v6, "repeated enable holds a single v6 reference") + + require.NoError(t, m.DisableRouting(), "disable") + v4, v6 = state.Counts() + assert.Equal(t, 0, v4, "single disable releases the v4 reference") + assert.Equal(t, 0, v6, "single disable releases the v6 reference") +} + +// TestIptablesRouting_DisableKeepsDNATReference verifies that an unpaired +// DisableRouting does not release references held by active DNAT rules. +func TestIptablesRouting_DisableKeepsDNATReference(t *testing.T) { + m := newIptRefcountManager(t, true) + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(iptDnatV6(9095)) + require.NoError(t, err, "add v6 dnat") + + require.NoError(t, m.DisableRouting(), "unpaired disable") + _, v6 := state.Counts() + assert.Equal(t, 1, v6, "DNAT-held reference survives unpaired DisableRouting") + + require.NoError(t, m.DeleteDNATRule(r1), "delete v6 dnat") + _, v6 = state.Counts() + assert.Equal(t, 0, v6, "delete releases the DNAT reference") +} + +// TestIptablesDNAT_RefcountBalancedV4 covers a Balanced Add/Delete pair on v4. +func TestIptablesDNAT_RefcountBalancedV4(t *testing.T) { + m := newIptRefcountManager(t, false) + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(iptDnatV4(7081)) + require.NoError(t, err, "add v4 dnat 1") + v4, v6 := state.Counts() + assert.Equal(t, 1, v4, "v4 refcount after first add") + assert.Equal(t, 0, v6, "v6 refcount unchanged") + + r2, err := m.AddDNATRule(iptDnatV4(7082)) + require.NoError(t, err, "add v4 dnat 2") + v4, v6 = state.Counts() + assert.Equal(t, 2, v4, "v4 refcount after second add") + assert.Equal(t, 0, v6, "v6 refcount unchanged") + + require.NoError(t, m.DeleteDNATRule(r1)) + v4, v6 = state.Counts() + assert.Equal(t, 1, v4, "v4 refcount after first delete") + assert.Equal(t, 0, v6, "v6 refcount unchanged") + + require.NoError(t, m.DeleteDNATRule(r2)) + v4, v6 = state.Counts() + assert.Equal(t, 0, v4, "v4 refcount after second delete") + assert.Equal(t, 0, v6, "v6 refcount unchanged") +} + +// TestIptablesDNAT_RefcountBalancedV6 checks the v6 path increments v6 only and +// decrements back to zero. +func TestIptablesDNAT_RefcountBalancedV6(t *testing.T) { + m := newIptRefcountManager(t, true) + require.NotNil(t, m.router6, "v6 router") + require.Same(t, m.router.ipFwdState, m.router6.ipFwdState, "shared state") + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(iptDnatV6(9081)) + require.NoError(t, err, "add v6 dnat 1") + v4, v6 := state.Counts() + assert.Equal(t, 0, v4) + assert.Equal(t, 1, v6, "v6 refcount after first add") + + r2, err := m.AddDNATRule(iptDnatV6(9082)) + require.NoError(t, err, "add v6 dnat 2") + v4, v6 = state.Counts() + assert.Equal(t, 0, v4, "v4 refcount unchanged") + assert.Equal(t, 2, v6, "v6 refcount after second add") + + require.NoError(t, m.DeleteDNATRule(r1)) + v4, v6 = state.Counts() + assert.Equal(t, 0, v4, "v4 refcount unchanged") + assert.Equal(t, 1, v6, "v6 refcount after first delete") + + require.NoError(t, m.DeleteDNATRule(r2)) + v4, v6 = state.Counts() + assert.Equal(t, 0, v4) + assert.Equal(t, 0, v6, "v6 refcount after second delete") +} + +// TestIptablesDNAT_DuplicateAddNoLeak verifies the duplicate-rule path returns +// without bumping the refcount. +func TestIptablesDNAT_DuplicateAddNoLeak(t *testing.T) { + m := newIptRefcountManager(t, true) + state := m.router.ipFwdState + + rule := iptDnatV4(7083) + r1, err := m.AddDNATRule(rule) + require.NoError(t, err) + v4, _ := state.Counts() + assert.Equal(t, 1, v4) + + _, err = m.AddDNATRule(rule) + require.NoError(t, err, "duplicate add") + v4, _ = state.Counts() + assert.Equal(t, 1, v4, "duplicate add must not increment") + + require.NoError(t, m.DeleteDNATRule(r1)) + v4, _ = state.Counts() + assert.Equal(t, 0, v4, "single delete must drop to zero") +} + +// TestIptablesDNAT_DeleteMissingNoUnderflow verifies Delete on an unknown rule +// neither errors nor releases the refcount. +func TestIptablesDNAT_DeleteMissingNoUnderflow(t *testing.T) { + m := newIptRefcountManager(t, true) + state := m.router.ipFwdState + + phantom := iptDnatV4(7099) + require.NoError(t, m.DeleteDNATRule(&phantom), "delete missing v4") + v4, v6 := state.Counts() + assert.Equal(t, 0, v4) + assert.Equal(t, 0, v6) + + phantom6 := iptDnatV6(9099) + require.NoError(t, m.DeleteDNATRule(&phantom6), "delete missing v6") + v4, v6 = state.Counts() + assert.Equal(t, 0, v4) + assert.Equal(t, 0, v6) + + r1, err := m.AddDNATRule(iptDnatV4(7100)) + require.NoError(t, err) + v4, _ = state.Counts() + assert.Equal(t, 1, v4, "real add still increments after phantom delete") + require.NoError(t, m.DeleteDNATRule(r1)) +} + +// TestIptablesDNAT_DoubleDeleteNoUnderflow verifies a second Delete on the same +// rule is a no-op. +func TestIptablesDNAT_DoubleDeleteNoUnderflow(t *testing.T) { + m := newIptRefcountManager(t, true) + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(iptDnatV6(9083)) + require.NoError(t, err) + _, v6 := state.Counts() + assert.Equal(t, 1, v6) + + require.NoError(t, m.DeleteDNATRule(r1), "first delete") + _, v6 = state.Counts() + assert.Equal(t, 0, v6) + + require.NoError(t, m.DeleteDNATRule(r1), "second delete must be no-op") + _, v6 = state.Counts() + assert.Equal(t, 0, v6, "double delete must not underflow") +} diff --git a/client/firewall/iptables/manager_linux.go b/client/firewall/iptables/manager_linux.go index 696537dd8..aa052d933 100644 --- a/client/firewall/iptables/manager_linux.go +++ b/client/firewall/iptables/manager_linux.go @@ -89,7 +89,7 @@ func (m *Manager) createIPv6Components(wgIface iFaceMapper, mtu uint16) error { } // Share the same IP forwarding state with the v4 router, since - // EnableIPForwarding controls both v4 and v6 sysctls. + // Forwarding refcounter is per-family but shared between v4 and v6 routers. m.router6.ipFwdState = m.router.ipFwdState m.aclMgr6, err = newAclManager(ip6Client, wgIface) @@ -402,17 +402,12 @@ func (m *Manager) SetLogLevel(log.Level) { } func (m *Manager) EnableRouting() error { - if err := m.router.ipFwdState.RequestForwarding(); err != nil { - return fmt.Errorf("enable IP forwarding: %w", err) - } - return nil + // v6 only when the overlay actually has v6. + return m.router.ipFwdState.RequestRouting(m.router6 != nil) } func (m *Manager) DisableRouting() error { - if err := m.router.ipFwdState.ReleaseForwarding(); err != nil { - return fmt.Errorf("disable IP forwarding: %w", err) - } - return nil + return m.router.ipFwdState.ReleaseRouting() } // AddDNATRule adds a DNAT rule diff --git a/client/firewall/iptables/router_linux.go b/client/firewall/iptables/router_linux.go index 42d305f5c..01b18570c 100644 --- a/client/firewall/iptables/router_linux.go +++ b/client/firewall/iptables/router_linux.go @@ -102,7 +102,7 @@ func newRouter(iptablesClient *iptables.IPTables, wgIface iFaceMapper, mtu uint1 wgIface: wgIface, mtu: mtu, v6: iptablesClient.Proto() == iptables.ProtocolIPv6, - ipFwdState: ipfwdstate.NewIPForwardingState(), + ipFwdState: ipfwdstate.NewIPForwardingState(wgIface.Name()), } r.ipsetCounter = refcounter.New( @@ -770,10 +770,6 @@ func (r *router) updateState() { } func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { - if err := r.ipFwdState.RequestForwarding(); err != nil { - return nil, err - } - ruleKey := rule.ID() if _, exists := r.rules[ruleKey+dnatSuffix]; exists { return rule, nil @@ -840,18 +836,34 @@ func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { for key, ruleInfo := range rules { if err := r.iptablesClient.Append(ruleInfo.table, ruleInfo.chain, ruleInfo.rule...); err != nil { - if rollbackErr := r.rollbackRules(rules); rollbackErr != nil { - log.Errorf("rollback failed: %v", rollbackErr) - } + r.cleanupFailedDNATAdd(rules) return nil, fmt.Errorf("add rule %s: %w", key, err) } r.rules[key] = ruleInfo.rule } + if err := r.ipFwdState.RequestForwarding(r.v6); err != nil { + r.cleanupFailedDNATAdd(rules) + return nil, fmt.Errorf("enable forwarding: %w", err) + } + r.updateState() return rule, nil } +// cleanupFailedDNATAdd removes the bookkeeping written by a partially applied +// AddDNATRule before rolling back the kernel rules, so no entries remain that +// never got a forwarding refcount. rollbackRules re-adds entries it failed to +// remove from the kernel. +func (r *router) cleanupFailedDNATAdd(rules map[string]ruleInfo) { + for key := range rules { + delete(r.rules, key) + } + if err := r.rollbackRules(rules); err != nil { + log.Errorf("rollback failed: %v", err) + } +} + func (r *router) rollbackRules(rules map[string]ruleInfo) error { var merr *multierror.Error for key, ruleInfo := range rules { @@ -868,32 +880,47 @@ func (r *router) rollbackRules(rules map[string]ruleInfo) error { } func (r *router) DeleteDNATRule(rule firewall.Rule) error { - if err := r.ipFwdState.ReleaseForwarding(); err != nil { - log.Errorf("%v", err) - } - ruleKey := rule.ID() + _, hadDNAT := r.rules[ruleKey+dnatSuffix] + _, hadSNAT := r.rules[ruleKey+snatSuffix] + _, hadFWD := r.rules[ruleKey+fwdSuffix] + if !hadDNAT && !hadSNAT && !hadFWD { + return nil + } + var merr *multierror.Error if dnatRule, exists := r.rules[ruleKey+dnatSuffix]; exists { if err := r.iptablesClient.Delete(tableNat, chainRTRDR, dnatRule...); err != nil { merr = multierror.Append(merr, fmt.Errorf("delete DNAT rule: %w", err)) + } else { + delete(r.rules, ruleKey+dnatSuffix) } - delete(r.rules, ruleKey+dnatSuffix) } if snatRule, exists := r.rules[ruleKey+snatSuffix]; exists { if err := r.iptablesClient.Delete(tableNat, chainRTNAT, snatRule...); err != nil { merr = multierror.Append(merr, fmt.Errorf("delete SNAT rule: %w", err)) + } else { + delete(r.rules, ruleKey+snatSuffix) } - delete(r.rules, ruleKey+snatSuffix) } if fwdRule, exists := r.rules[ruleKey+fwdSuffix]; exists { if err := r.iptablesClient.Delete(tableFilter, chainRTFWDOUT, fwdRule...); err != nil { merr = multierror.Append(merr, fmt.Errorf("delete forward rule: %w", err)) + } else { + delete(r.rules, ruleKey+fwdSuffix) + } + } + + // Release the refcount only once all rules are gone from the kernel. On + // partial failure the failed entries stay in r.rules so a retry can remove + // them and release then. + if merr == nil { + if err := r.ipFwdState.ReleaseForwarding(r.v6); err != nil { + log.Errorf("%v", err) } - delete(r.rules, ruleKey+fwdSuffix) } r.updateState() diff --git a/client/firewall/nftables/dnat_refcount_linux_test.go b/client/firewall/nftables/dnat_refcount_linux_test.go new file mode 100644 index 000000000..86079676f --- /dev/null +++ b/client/firewall/nftables/dnat_refcount_linux_test.go @@ -0,0 +1,249 @@ +//go:build privileged + +package nftables + +import ( + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + fw "github.com/netbirdio/netbird/client/firewall/manager" + "github.com/netbirdio/netbird/client/iface" + "github.com/netbirdio/netbird/client/iface/wgaddr" +) + +func nftRefcountIfaceV4() *iFaceMock { + return &iFaceMock{ + NameFunc: func() string { return "wt-refcount" }, + AddressFunc: func() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("100.96.0.1"), + Network: netip.MustParsePrefix("100.96.0.0/16"), + } + }, + } +} + +func nftRefcountIfaceDual() *iFaceMock { + return &iFaceMock{ + NameFunc: func() string { return "wt-refcount" }, + AddressFunc: func() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("100.96.0.1"), + Network: netip.MustParsePrefix("100.96.0.0/16"), + IPv6: netip.MustParseAddr("fd00::1"), + IPv6Net: netip.MustParsePrefix("fd00::/64"), + } + }, + } +} + +func newNftRefcountManager(t *testing.T, dual bool) *Manager { + t.Helper() + if check() != NFTABLES { + t.Skip("nftables not supported on this system") + } + var ifMock *iFaceMock + if dual { + ifMock = nftRefcountIfaceDual() + } else { + ifMock = nftRefcountIfaceV4() + } + m, err := Create(ifMock, iface.DefaultMTU) + require.NoError(t, err, "create manager") + require.NoError(t, m.Init(nil), "init manager") + t.Cleanup(func() { + require.NoError(t, m.Close(nil), "close manager") + }) + return m +} + +func dnatV4(port uint16) fw.ForwardRule { + return fw.ForwardRule{ + Protocol: fw.ProtocolTCP, + DestinationPort: fw.Port{Values: []uint16{port}}, + TranslatedAddress: netip.MustParseAddr("100.96.0.2"), + TranslatedPort: fw.Port{Values: []uint16{80}}, + } +} + +func dnatV6(port uint16) fw.ForwardRule { + return fw.ForwardRule{ + Protocol: fw.ProtocolTCP, + DestinationPort: fw.Port{Values: []uint16{port}}, + TranslatedAddress: netip.MustParseAddr("fd00::2"), + TranslatedPort: fw.Port{Values: []uint16{80}}, + } +} + +// TestNftablesDNAT_RefcountBalancedV4 verifies that Add/Delete pairs leave the +// v4 refcount at zero. +func TestNftablesDNAT_RefcountBalancedV4(t *testing.T) { + m := newNftRefcountManager(t, false) + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(dnatV4(8081)) + require.NoError(t, err, "add v4 dnat 1") + v4, v6 := state.Counts() + assert.Equal(t, 1, v4, "v4 refcount after first add") + assert.Equal(t, 0, v6, "v6 refcount unchanged") + + r2, err := m.AddDNATRule(dnatV4(8082)) + require.NoError(t, err, "add v4 dnat 2") + v4, v6 = state.Counts() + assert.Equal(t, 2, v4, "v4 refcount after second add") + assert.Equal(t, 0, v6, "v6 refcount unchanged") + + require.NoError(t, m.DeleteDNATRule(r1), "delete v4 dnat 1") + v4, v6 = state.Counts() + assert.Equal(t, 1, v4, "v4 refcount after first delete") + assert.Equal(t, 0, v6, "v6 refcount unchanged") + + require.NoError(t, m.DeleteDNATRule(r2), "delete v4 dnat 2") + v4, v6 = state.Counts() + assert.Equal(t, 0, v4, "v4 refcount after second delete") + assert.Equal(t, 0, v6, "v6 refcount unchanged") +} + +// TestNftablesDNAT_RefcountBalancedV6 verifies the v6 path increments v6 only +// and decrements back to zero on Delete. +func TestNftablesDNAT_RefcountBalancedV6(t *testing.T) { + m := newNftRefcountManager(t, true) + require.NotNil(t, m.router6, "v6 router") + require.Same(t, m.router.ipFwdState, m.router6.ipFwdState, "shared state") + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(dnatV6(9091)) + require.NoError(t, err, "add v6 dnat 1") + v4, v6 := state.Counts() + assert.Equal(t, 0, v4, "v4 refcount unchanged") + assert.Equal(t, 1, v6, "v6 refcount after first add") + + r2, err := m.AddDNATRule(dnatV6(9092)) + require.NoError(t, err, "add v6 dnat 2") + v4, v6 = state.Counts() + assert.Equal(t, 0, v4) + assert.Equal(t, 2, v6, "v6 refcount after second add") + + require.NoError(t, m.DeleteDNATRule(r1), "delete v6 dnat 1") + v4, v6 = state.Counts() + assert.Equal(t, 0, v4, "v4 refcount unchanged") + assert.Equal(t, 1, v6, "v6 refcount after first delete") + + require.NoError(t, m.DeleteDNATRule(r2), "delete v6 dnat 2") + v4, v6 = state.Counts() + assert.Equal(t, 0, v4) + assert.Equal(t, 0, v6, "v6 refcount after second delete") +} + +// TestNftablesDNAT_DuplicateAddNoLeak verifies that a duplicate Add (same +// ForwardRule) does not double-increment the refcount. +func TestNftablesDNAT_DuplicateAddNoLeak(t *testing.T) { + m := newNftRefcountManager(t, true) + state := m.router.ipFwdState + + rule := dnatV4(8083) + r1, err := m.AddDNATRule(rule) + require.NoError(t, err, "add v4 dnat") + v4, _ := state.Counts() + assert.Equal(t, 1, v4) + + // duplicate add: same rule ID, must be a no-op for the refcount. + _, err = m.AddDNATRule(rule) + require.NoError(t, err, "duplicate add") + v4, _ = state.Counts() + assert.Equal(t, 1, v4, "duplicate add must not increment") + + require.NoError(t, m.DeleteDNATRule(r1), "delete v4 dnat") + v4, _ = state.Counts() + assert.Equal(t, 0, v4, "single delete must drop to zero") +} + +// TestNftablesDNAT_DeleteMissingNoUnderflow verifies deleting a rule that was +// never added does not underflow the refcount. +func TestNftablesDNAT_DeleteMissingNoUnderflow(t *testing.T) { + m := newNftRefcountManager(t, true) + state := m.router.ipFwdState + + // Construct a Rule reference for something never added. The router stores + // rules by ID(), and DeleteDNATRule looks them up in r.rules; a missing + // entry must be a no-op rather than calling Release. + phantom := dnatV4(8099) + require.NoError(t, m.DeleteDNATRule(&phantom), "delete missing v4 dnat") + v4, v6 := state.Counts() + assert.Equal(t, 0, v4, "v4 refcount unaffected by missing delete") + assert.Equal(t, 0, v6, "v6 refcount unaffected") + + phantom6 := dnatV6(9099) + require.NoError(t, m.DeleteDNATRule(&phantom6), "delete missing v6 dnat") + v4, v6 = state.Counts() + assert.Equal(t, 0, v4) + assert.Equal(t, 0, v6, "v6 refcount unaffected by missing delete") + + // And after a phantom delete, a real add still results in count=1. + r1, err := m.AddDNATRule(dnatV4(8100)) + require.NoError(t, err, "add v4 dnat after phantom delete") + v4, _ = state.Counts() + assert.Equal(t, 1, v4, "real add still increments after phantom delete") + require.NoError(t, m.DeleteDNATRule(r1)) +} + +// TestNftablesRouting_RepeatedEnableSingleReference verifies that EnableRouting +// (called on every network-map update) holds at most one reference per family +// and a single DisableRouting drops both back to zero. +func TestNftablesRouting_RepeatedEnableSingleReference(t *testing.T) { + m := newNftRefcountManager(t, true) + state := m.router.ipFwdState + + require.NoError(t, m.EnableRouting(), "first enable") + require.NoError(t, m.EnableRouting(), "second enable") + require.NoError(t, m.EnableRouting(), "third enable") + v4, v6 := state.Counts() + assert.Equal(t, 1, v4, "repeated enable holds a single v4 reference") + assert.Equal(t, 1, v6, "repeated enable holds a single v6 reference") + + require.NoError(t, m.DisableRouting(), "disable") + v4, v6 = state.Counts() + assert.Equal(t, 0, v4, "single disable releases the v4 reference") + assert.Equal(t, 0, v6, "single disable releases the v6 reference") +} + +// TestNftablesRouting_DisableKeepsDNATReference verifies that an unpaired +// DisableRouting does not release references held by active DNAT rules. +func TestNftablesRouting_DisableKeepsDNATReference(t *testing.T) { + m := newNftRefcountManager(t, true) + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(dnatV6(9095)) + require.NoError(t, err, "add v6 dnat") + + require.NoError(t, m.DisableRouting(), "unpaired disable") + _, v6 := state.Counts() + assert.Equal(t, 1, v6, "DNAT-held reference survives unpaired DisableRouting") + + require.NoError(t, m.DeleteDNATRule(r1), "delete v6 dnat") + _, v6 = state.Counts() + assert.Equal(t, 0, v6, "delete releases the DNAT reference") +} + +// TestNftablesDNAT_DoubleDeleteNoUnderflow verifies that deleting the same rule +// twice does not underflow the refcount (the second delete is a no-op). +func TestNftablesDNAT_DoubleDeleteNoUnderflow(t *testing.T) { + m := newNftRefcountManager(t, true) + state := m.router.ipFwdState + + r1, err := m.AddDNATRule(dnatV6(9093)) + require.NoError(t, err) + _, v6 := state.Counts() + assert.Equal(t, 1, v6) + + require.NoError(t, m.DeleteDNATRule(r1), "first delete") + _, v6 = state.Counts() + assert.Equal(t, 0, v6) + + require.NoError(t, m.DeleteDNATRule(r1), "second delete must be no-op") + _, v6 = state.Counts() + assert.Equal(t, 0, v6, "double delete must not underflow") +} diff --git a/client/firewall/nftables/manager_linux.go b/client/firewall/nftables/manager_linux.go index fdc7c2f3c..984b1c3ba 100644 --- a/client/firewall/nftables/manager_linux.go +++ b/client/firewall/nftables/manager_linux.go @@ -105,8 +105,8 @@ func (m *Manager) createIPv6Components(tableName string, wgIface iFaceMapper, mt return fmt.Errorf("create v6 router: %w", err) } - // Share the same IP forwarding state with the v4 router, since - // EnableIPForwarding controls both v4 and v6 sysctls. + // Share the per-family forwarding refcounter with the v4 router so a v4 + // rule and a v6 rule against the same state machine cooperate cleanly. m.router6.ipFwdState = m.router.ipFwdState m.aclManager6, err = newAclManager(workTable6, wgIface, chainNameRoutingFw) @@ -530,17 +530,12 @@ func (m *Manager) SetLogLevel(log.Level) { } func (m *Manager) EnableRouting() error { - if err := m.router.ipFwdState.RequestForwarding(); err != nil { - return fmt.Errorf("enable IP forwarding: %w", err) - } - return nil + // v6 only when the overlay actually has v6. + return m.router.ipFwdState.RequestRouting(m.router6 != nil) } func (m *Manager) DisableRouting() error { - if err := m.router.ipFwdState.ReleaseForwarding(); err != nil { - return fmt.Errorf("disable IP forwarding: %w", err) - } - return nil + return m.router.ipFwdState.ReleaseRouting() } // Flush rule/chain/set operations from the buffer diff --git a/client/firewall/nftables/router_linux.go b/client/firewall/nftables/router_linux.go index dfb94c514..d3e031c5f 100644 --- a/client/firewall/nftables/router_linux.go +++ b/client/firewall/nftables/router_linux.go @@ -93,7 +93,7 @@ func newRouter(workTable *nftables.Table, wgIface iFaceMapper, mtu uint16) (*rou rules: make(map[string]*nftables.Rule), af: familyForAddr(workTable.Family == nftables.TableFamilyIPv4), wgIface: wgIface, - ipFwdState: ipfwdstate.NewIPForwardingState(), + ipFwdState: ipfwdstate.NewIPForwardingState(wgIface.Name()), mtu: mtu, } @@ -1553,10 +1553,6 @@ func (r *router) refreshRulesMap() error { } func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { - if err := r.ipFwdState.RequestForwarding(); err != nil { - return nil, err - } - ruleKey := rule.ID() if _, exists := r.rules[ruleKey+dnatSuffix]; exists { return rule, nil @@ -1567,7 +1563,18 @@ func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { return nil, fmt.Errorf("convert protocol to number: %w", err) } + // Request forwarding before queueing rules: addDnatRedirect/addDnatMasq + // buffer netlink messages on r.conn that the next caller's Flush would + // commit if we returned without flushing them ourselves. + v6 := r.af.tableFamily == nftables.TableFamilyIPv6 + if err := r.ipFwdState.RequestForwarding(v6); err != nil { + return nil, fmt.Errorf("enable forwarding: %w", err) + } + if err := r.addDnatRedirect(rule, protoNum, ruleKey); err != nil { + if rerr := r.ipFwdState.ReleaseForwarding(v6); rerr != nil { + log.Warnf("rollback forwarding refcount: %v", rerr) + } return nil, err } @@ -1579,6 +1586,11 @@ func (r *router) AddDNATRule(rule firewall.ForwardRule) (firewall.Rule, error) { // TODO: find chains with drop policies and add rules there if err := r.conn.Flush(); err != nil { + if rerr := r.ipFwdState.ReleaseForwarding(v6); rerr != nil { + log.Warnf("rollback forwarding refcount: %v", rerr) + } + delete(r.rules, ruleKey+dnatSuffix) + delete(r.rules, ruleKey+snatSuffix) return nil, fmt.Errorf("flush rules: %w", err) } @@ -1781,16 +1793,18 @@ func (r *router) addDnatMasq(rule firewall.ForwardRule, protoNum uint8, ruleKey } func (r *router) DeleteDNATRule(rule firewall.Rule) error { - if err := r.ipFwdState.ReleaseForwarding(); err != nil { - log.Errorf("%v", err) - } - ruleKey := rule.ID() if err := r.refreshRulesMap(); err != nil { return fmt.Errorf(refreshRulesMapError, err) } + _, hadDNAT := r.rules[ruleKey+dnatSuffix] + _, hadSNAT := r.rules[ruleKey+snatSuffix] + if !hadDNAT && !hadSNAT { + return nil + } + var merr *multierror.Error var needsFlush bool @@ -1822,9 +1836,16 @@ func (r *router) DeleteDNATRule(rule firewall.Rule) error { } } + // Release the refcount only once the rules are gone from the kernel. On + // failure (including the refreshRulesMap error above) the rules and their + // map entries remain, keeping forwarding on until a retry removes them. if merr == nil { delete(r.rules, ruleKey+dnatSuffix) delete(r.rules, ruleKey+snatSuffix) + + if err := r.ipFwdState.ReleaseForwarding(r.af.tableFamily == nftables.TableFamilyIPv6); err != nil { + log.Errorf("%v", err) + } } return nberrors.FormatErrorOrNil(merr) diff --git a/client/internal/debug/debug_linux.go b/client/internal/debug/debug_linux.go index 40d864eda..a36c0c0e7 100644 --- a/client/internal/debug/debug_linux.go +++ b/client/internal/debug/debug_linux.go @@ -844,6 +844,10 @@ func collectSysctls() string { []string{"net.ipv4.conf.all.src_valid_mark", "net.ipv4.conf.default.src_valid_mark"}, listInterfaceSysctls("ipv4", "src_valid_mark")..., )) + writeSysctlGroup(&builder, "accept_ra", append( + []string{"net.ipv6.conf.all.accept_ra", "net.ipv6.conf.default.accept_ra"}, + listInterfaceSysctls("ipv6", "accept_ra")..., + )) writeSysctlGroup(&builder, "conntrack", []string{ "net.netfilter.nf_conntrack_acct", "net.netfilter.nf_conntrack_tcp_loose", diff --git a/client/internal/routemanager/ipfwdstate/ipfwdstate.go b/client/internal/routemanager/ipfwdstate/ipfwdstate.go index 2be1c2ae7..3d571e16b 100644 --- a/client/internal/routemanager/ipfwdstate/ipfwdstate.go +++ b/client/internal/routemanager/ipfwdstate/ipfwdstate.go @@ -2,54 +2,183 @@ package ipfwdstate import ( "fmt" + "sync" log "github.com/sirupsen/logrus" "github.com/netbirdio/netbird/client/internal/routemanager/systemops" ) -// IPForwardingState is a struct that keeps track of the IP forwarding state. -// todo: read initial state of the IP forwarding from the system and reset the state based on it. -// todo: separate v4/v6 forwarding state, since the sysctls are independent -// (net.ipv4.ip_forward vs net.ipv6.conf.all.forwarding). Currently the nftables -// manager shares one instance between both routers, which works only because -// EnableIPForwarding enables both sysctls in a single call. +// IPForwardingState tracks v4 and v6 IP-forwarding sysctl enables with +// independent refcounts so a v4-only routing setup doesn't flip v6 sysctls. type IPForwardingState struct { - enabledCounter int + mu sync.Mutex + + v4Count int + v6Count int + + // routingV4/routingV6 track whether the routing path currently holds a + // reference, so repeated EnableRouting calls (one per network-map update) + // hold at most one reference per family and an unpaired DisableRouting + // can't release references held by DNAT rules. + routingV4 bool + routingV6 bool + + wgIfaceName string + v6Saved map[string]int } -func NewIPForwardingState() *IPForwardingState { - return &IPForwardingState{} +// NewIPForwardingState returns a state tracker for the IP-forwarding sysctls. +// wgIfaceName is excluded from the per-interface accept_ra handling. +func NewIPForwardingState(wgIfaceName string) *IPForwardingState { + return &IPForwardingState{wgIfaceName: wgIfaceName} } -func (f *IPForwardingState) RequestForwarding() error { - if f.enabledCounter != 0 { - f.enabledCounter++ +// Counts returns the current v4 and v6 refcounts. Intended for diagnostics +// and tests. +func (f *IPForwardingState) Counts() (v4, v6 int) { + f.mu.Lock() + defer f.mu.Unlock() + return f.v4Count, f.v6Count +} + +// RequestRouting takes the forwarding references for the routing path. It is +// idempotent: while routing already holds a reference, further calls don't +// increment the refcounts, and a v4-only request releases a previously held v6 +// reference. A v6 sysctl failure is logged and not returned so it can't take +// down v4 routing (the sysctl may be unwritable, e.g. read-only /proc/sys or +// IPv6 disabled on the kernel command line); v6 is retried on the next call. +func (f *IPForwardingState) RequestRouting(v6 bool) error { + f.mu.Lock() + defer f.mu.Unlock() + + if !f.routingV4 { + if err := f.requestV4(); err != nil { + return err + } + f.routingV4 = true + } + + if !v6 { + if !f.routingV6 { + return nil + } + f.routingV6 = false + return f.releaseV6() + } + + if f.routingV6 { return nil } - - if err := systemops.EnableIPForwarding(); err != nil { - return fmt.Errorf("failed to enable IP forwarding with sysctl: %w", err) + if err := f.requestV6(); err != nil { + log.Warnf("enable IPv6 forwarding for routing: %v", err) + return nil } - f.enabledCounter = 1 - log.Info("IP forwarding enabled") - + f.routingV6 = true return nil } -func (f *IPForwardingState) ReleaseForwarding() error { - if f.enabledCounter == 0 { - return nil +// ReleaseRouting releases the references RequestRouting holds. Calls without a +// held reference are no-ops. +func (f *IPForwardingState) ReleaseRouting() error { + f.mu.Lock() + defer f.mu.Unlock() + + if f.routingV4 { + f.routingV4 = false + f.releaseV4() } - - if f.enabledCounter > 1 { - f.enabledCounter-- - return nil + if f.routingV6 { + f.routingV6 = false + return f.releaseV6() } - - // if failed to disable IP forwarding we anyway decrement the counter - f.enabledCounter = 0 - - // todo call systemops.DisableIPForwarding() + return nil +} + +// RequestForwarding enables the family's forwarding sysctl on first request. +func (f *IPForwardingState) RequestForwarding(v6 bool) error { + f.mu.Lock() + defer f.mu.Unlock() + + if v6 { + return f.requestV6() + } + return f.requestV4() +} + +// ReleaseForwarding decrements the family counter. The last v6 release restores +// what enable captured. v4 stays on: net.ipv4.ip_forward is co-owned by other +// tooling (docker, k8s, libvirt). +func (f *IPForwardingState) ReleaseForwarding(v6 bool) error { + f.mu.Lock() + defer f.mu.Unlock() + + if v6 { + return f.releaseV6() + } + f.releaseV4() + return nil +} + +func (f *IPForwardingState) requestV4() error { + if f.v4Count == 0 { + if err := systemops.EnableV4IPForwarding(); err != nil { + return fmt.Errorf("enable IPv4 forwarding: %w", err) + } + log.Info("IPv4 forwarding enabled") + } + f.v4Count++ + return nil +} + +func (f *IPForwardingState) releaseV4() { + if f.v4Count > 0 { + f.v4Count-- + } +} + +func (f *IPForwardingState) requestV6() error { + if f.v6Count == 0 { + saved, err := systemops.EnableV6IPForwarding(f.wgIfaceName) + if err != nil { + if rerr := systemops.DisableV6IPForwarding(saved); rerr != nil { + log.Warnf("rollback partial v6 sysctls: %v", rerr) + } + return fmt.Errorf("enable IPv6 forwarding: %w", err) + } + // A failed restore on a previous release keeps its saved values; those + // are the true originals, so keep them over what this enable captured. + if f.v6Saved == nil { + f.v6Saved = saved + } else { + for k, v := range saved { + if _, ok := f.v6Saved[k]; !ok { + f.v6Saved[k] = v + } + } + } + log.Info("IPv6 forwarding enabled") + } + f.v6Count++ + return nil +} + +func (f *IPForwardingState) releaseV6() error { + if f.v6Count == 0 { + return nil + } + f.v6Count-- + if f.v6Count > 0 { + return nil + } + + // Keep the saved values on failure so a later release or enable/release + // cycle can still restore them; re-restoring an already-restored key is a + // no-op since the sysctl already holds the desired value. + if err := systemops.DisableV6IPForwarding(f.v6Saved); err != nil { + return fmt.Errorf("disable IPv6 forwarding: %w", err) + } + f.v6Saved = nil + log.Info("IPv6 forwarding disabled") return nil } diff --git a/client/internal/routemanager/ipfwdstate/ipfwdstate_privileged_linux_test.go b/client/internal/routemanager/ipfwdstate/ipfwdstate_privileged_linux_test.go new file mode 100644 index 000000000..b4615ff02 --- /dev/null +++ b/client/internal/routemanager/ipfwdstate/ipfwdstate_privileged_linux_test.go @@ -0,0 +1,39 @@ +//go:build privileged + +package ipfwdstate + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestRequestRoutingV6ToV4Transition verifies that a v4-only routing request +// releases a previously held routing-owned v6 reference without touching +// references held by DNAT rules. +func TestRequestRoutingV6ToV4Transition(t *testing.T) { + f := NewIPForwardingState("wt-fwd-test") + + require.NoError(t, f.RequestRouting(true), "request routing with v6") + v4, v6 := f.Counts() + assert.Equal(t, 1, v4, "v4 reference held") + assert.Equal(t, 1, v6, "v6 reference held") + + require.NoError(t, f.RequestRouting(false), "request routing v4-only") + v4, v6 = f.Counts() + assert.Equal(t, 1, v4, "v4 reference kept") + assert.Equal(t, 0, v6, "routing-owned v6 reference released") + + // A DNAT-held reference survives a v4-only routing request. + require.NoError(t, f.RequestForwarding(true), "dnat v6 reference") + require.NoError(t, f.RequestRouting(false), "repeat v4-only request") + _, v6 = f.Counts() + assert.Equal(t, 1, v6, "dnat-held v6 reference survives") + require.NoError(t, f.ReleaseForwarding(true), "release dnat v6 reference") + + require.NoError(t, f.ReleaseRouting(), "release routing") + v4, v6 = f.Counts() + assert.Equal(t, 0, v4, "all v4 references released") + assert.Equal(t, 0, v6, "all v6 references released") +} diff --git a/client/internal/routemanager/sysctl/sysctl_linux.go b/client/internal/routemanager/sysctl/sysctl_linux.go index 46b7c9fb7..bb131c691 100644 --- a/client/internal/routemanager/sysctl/sysctl_linux.go +++ b/client/internal/routemanager/sysctl/sysctl_linux.go @@ -58,11 +58,7 @@ func Setup(wgIface iface) (map[string]int, error) { continue } - // Escape '%' and '.' so they survive the dot-to-slash conversion in Set() - safeName := strings.ReplaceAll(intf.Name, "%", percentEscape) - safeName = strings.ReplaceAll(safeName, ".", dotEscape) - - i := fmt.Sprintf(rpFilterInterfacePath, safeName) + i := fmt.Sprintf(rpFilterInterfacePath, EscapeInterfaceName(intf.Name)) oldVal, err := Set(i, 2, true) if err != nil { result = multierror.Append(result, err) @@ -74,6 +70,13 @@ func Setup(wgIface iface) (map[string]int, error) { return keys, nberrors.FormatErrorOrNil(result) } +// EscapeInterfaceName escapes '%' and '.' in an interface name (e.g. VLANs +// like eth0.100) so the name survives the dot-to-slash conversion in Set. +func EscapeInterfaceName(name string) string { + safe := strings.ReplaceAll(name, "%", percentEscape) + return strings.ReplaceAll(safe, ".", dotEscape) +} + // Set sets a sysctl configuration, if onlyIfOne is true it will only set the new value if it's set to 1 func Set(key string, desiredValue int, onlyIfOne bool) (int, error) { path := strings.ReplaceAll(key, ".", "/") diff --git a/client/internal/routemanager/systemops/systemops_android.go b/client/internal/routemanager/systemops/systemops_android.go index 7cb8dae93..97b4ed8ec 100644 --- a/client/internal/routemanager/systemops/systemops_android.go +++ b/client/internal/routemanager/systemops/systemops_android.go @@ -32,8 +32,17 @@ func (r *SysOps) removeFromRouteTable(netip.Prefix, Nexthop) error { return nil } -func EnableIPForwarding() error { - log.Infof("Enable IP forwarding is not implemented on %s", runtime.GOOS) +func EnableV4IPForwarding() error { + log.Infof("Enable IPv4 forwarding is not implemented on %s", runtime.GOOS) + return nil +} + +func EnableV6IPForwarding(string) (map[string]int, error) { + log.Infof("Enable IPv6 forwarding is not implemented on %s", runtime.GOOS) + return map[string]int{}, nil +} + +func DisableV6IPForwarding(map[string]int) error { return nil } diff --git a/client/internal/routemanager/systemops/systemops_ios.go b/client/internal/routemanager/systemops/systemops_ios.go index 99a363371..0cccd4962 100644 --- a/client/internal/routemanager/systemops/systemops_ios.go +++ b/client/internal/routemanager/systemops/systemops_ios.go @@ -58,8 +58,17 @@ func (r *SysOps) removeFromRouteTable(netip.Prefix, Nexthop) error { return nil } -func EnableIPForwarding() error { - log.Infof("Enable IP forwarding is not implemented on %s", runtime.GOOS) +func EnableV4IPForwarding() error { + log.Infof("Enable IPv4 forwarding is not implemented on %s", runtime.GOOS) + return nil +} + +func EnableV6IPForwarding(string) (map[string]int, error) { + log.Infof("Enable IPv6 forwarding is not implemented on %s", runtime.GOOS) + return map[string]int{}, nil +} + +func DisableV6IPForwarding(map[string]int) error { return nil } diff --git a/client/internal/routemanager/systemops/systemops_linux.go b/client/internal/routemanager/systemops/systemops_linux.go index 8c6b7d9a9..7d608d886 100644 --- a/client/internal/routemanager/systemops/systemops_linux.go +++ b/client/internal/routemanager/systemops/systemops_linux.go @@ -763,13 +763,10 @@ func flushRoutes(tableID, family int) error { return nberrors.FormatErrorOrNil(result) } -func EnableIPForwarding() error { +func EnableV4IPForwarding() error { if _, err := sysctl.Set(ipv4ForwardingPath, 1, false); err != nil { return err } - if _, err := sysctl.Set(ipv6ForwardingPath, 1, false); err != nil { - log.Warnf("failed to enable IPv6 forwarding: %v", err) - } return nil } diff --git a/client/internal/routemanager/systemops/systemops_nonlinux.go b/client/internal/routemanager/systemops/systemops_nonlinux.go index 016a62ebd..837ac0cd2 100644 --- a/client/internal/routemanager/systemops/systemops_nonlinux.go +++ b/client/internal/routemanager/systemops/systemops_nonlinux.go @@ -43,8 +43,17 @@ func (r *SysOps) RemoveVPNRoute(prefix netip.Prefix, intf *net.Interface) error return r.genericRemoveVPNRoute(prefix, intf) } -func EnableIPForwarding() error { - log.Infof("Enable IP forwarding is not implemented on %s", runtime.GOOS) +func EnableV4IPForwarding() error { + log.Infof("Enable IPv4 forwarding is not implemented on %s", runtime.GOOS) + return nil +} + +func EnableV6IPForwarding(string) (map[string]int, error) { + log.Infof("Enable IPv6 forwarding is not implemented on %s", runtime.GOOS) + return map[string]int{}, nil +} + +func DisableV6IPForwarding(map[string]int) error { return nil } diff --git a/client/internal/routemanager/systemops/v6forwarding_linux.go b/client/internal/routemanager/systemops/v6forwarding_linux.go new file mode 100644 index 000000000..c1e0d4588 --- /dev/null +++ b/client/internal/routemanager/systemops/v6forwarding_linux.go @@ -0,0 +1,92 @@ +//go:build !android + +package systemops + +import ( + "fmt" + "net" + "os" + + "github.com/hashicorp/go-multierror" + log "github.com/sirupsen/logrus" + + nberrors "github.com/netbirdio/netbird/client/errors" + "github.com/netbirdio/netbird/client/internal/routemanager/sysctl" +) + +const ( + // 1 (default) accepts RAs only while forwarding is off; 2 keeps RA + // acceptance on regardless, so RA-installed host defaults survive our + // v6 forwarding flip. + acceptRAInterfacePath = "net.ipv6.conf.%s.accept_ra" + acceptRADefaultPath = "net.ipv6.conf.default.accept_ra" + acceptRAProcPathFormat = "/proc/sys/net/ipv6/conf/%s/accept_ra" +) + +// EnableV6IPForwarding bumps accept_ra=2 on host v6 interfaces before flipping +// forwarding=1, so RA-installed host defaults survive. Returns the prior values +// of sysctls we actually changed; entries already at the target are omitted. +func EnableV6IPForwarding(wgIfaceName string) (map[string]int, error) { + saved := map[string]int{} + bumpAcceptRA(saved, wgIfaceName) + + oldVal, err := sysctl.Set(ipv6ForwardingPath, 1, false) + if err != nil { + return saved, err + } + if oldVal != 1 { + saved[ipv6ForwardingPath] = oldVal + } + return saved, nil +} + +// DisableV6IPForwarding restores what EnableV6IPForwarding captured. +func DisableV6IPForwarding(saved map[string]int) error { + var result *multierror.Error + for key, value := range saved { + if _, err := sysctl.Set(key, value, false); err != nil { + result = multierror.Append(result, fmt.Errorf("restore %s: %w", key, err)) + } + } + return nberrors.FormatErrorOrNil(result) +} + +func bumpAcceptRA(saved map[string]int, wgIfaceName string) { + // Also bump conf.default so interfaces created while forwarding is on + // (hotplug, new Wi-Fi/dock) inherit accept_ra=2 and keep accepting RAs. + bumpAcceptRAKey(saved, acceptRADefaultPath) + + interfaces, err := net.Interfaces() + if err != nil { + log.Warnf("list interfaces for accept_ra: %v", err) + return + } + for _, intf := range interfaces { + if intf.Name == "lo" || intf.Name == wgIfaceName { + continue + } + bumpAcceptRAForInterface(saved, intf.Name) + } +} + +func bumpAcceptRAForInterface(saved map[string]int, name string) { + // Build procfs path from name, not the dotted key: VLAN names like eth0.100. + if _, err := os.Stat(fmt.Sprintf(acceptRAProcPathFormat, name)); err != nil { + return + } + bumpAcceptRAKey(saved, fmt.Sprintf(acceptRAInterfacePath, sysctl.EscapeInterfaceName(name))) +} + +func bumpAcceptRAKey(saved map[string]int, key string) { + // onlyIfOne=true: leave admin overrides (0, 2) alone. + oldVal, err := sysctl.Set(key, 2, true) + if err != nil { + log.Warnf("bump %s: %v", key, err) + return + } + // With onlyIfOne, a write only happened when the old value was 1; values + // left untouched (0, 2) must not be recorded for restore. + if oldVal == 1 { + saved[key] = oldVal + } +} From 6b69f5c05d24666aa8f1e5665c0689bd449ad40f Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Wed, 12 Aug 2026 15:35:01 +0000 Subject: [PATCH 04/11] [client] Remove installer registry handlers for autostart Run keys (#7183) The NSIS installer deleted HKLM/HKCU CurrentVersion\Run values it never writes, which matches common AV heuristics for unwanted Run-key manipulation and is suspected to contribute to Windows Defender and third-party antivirus false positives on the installer. Drop all autostart registry deletions from both the install and uninstall sections so the installer only touches keys it creates itself. Cleanup of the legacy machine-wide entry written by old installers is left to documentation. Extends the approach of the closed PR #6735, which only removed the per-user deletion on uninstall. --- client/installer.nsis | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/client/installer.nsis b/client/installer.nsis index 71699071b..eb2d7d5bd 100644 --- a/client/installer.nsis +++ b/client/installer.nsis @@ -22,8 +22,6 @@ !define UI_REG_APP_PATH "Software\Microsoft\Windows\CurrentVersion\App Paths\${UI_APP_EXE}" !define UI_UNINSTALL_PATH "Software\Microsoft\Windows\CurrentVersion\Uninstall\${UI_APP_NAME}" -!define AUTOSTART_REG_KEY "Software\Microsoft\Windows\CurrentVersion\Run" - !define NETBIRD_DATA_DIR "$COMMONPROGRAMDATA\Netbird" Unicode True @@ -228,13 +226,6 @@ WriteRegStr ${REG_ROOT} "${UNINSTALL_PATH}" "Publisher" "${COMP_NAME}" WriteRegStr ${REG_ROOT} "${UI_REG_APP_PATH}" "" "$INSTDIR\${UI_APP_EXE}" -; Autostart is owned by the UI's per-user setting (HKCU\...\Run via Wails), -; not the installer. Drop the machine-wide entry older installers wrote so the -; toggle is the single source of truth. HKCU is left untouched -- it may hold -; the user's own toggle state, which must survive upgrades. -DetailPrint "Removing installer-managed autostart registry entry if present..." -DeleteRegValue HKLM "${AUTOSTART_REG_KEY}" "${APP_NAME}" - EnVar::SetHKLM EnVar::AddValueEx "path" "$INSTDIR" @@ -299,15 +290,6 @@ ExecWait '"$INSTDIR\${MAIN_APP_EXE}" service uninstall' DetailPrint "Terminating Netbird UI process..." ExecWait `taskkill /im ${UI_APP_EXE}.exe /f` -; Remove autostart registry entries -DetailPrint "Removing autostart registry entries if they exist..." -; Legacy machine-wide entry written by older installers. -DeleteRegValue HKLM "${AUTOSTART_REG_KEY}" "${APP_NAME}" -; Per-user entry the UI toggle writes via Wails (value name is the lowercase -; app-name slug). Uninstall removes the app, so drop it too. -DeleteRegValue HKCU "${AUTOSTART_REG_KEY}" "${APP_NAME}" -DeleteRegValue HKCU "${AUTOSTART_REG_KEY}" "netbird" - ; Handle data deletion based on checkbox DetailPrint "Checking if user requested data deletion..." ${If} $DeleteDataEnabled == "1" From c5503fdc7f93ae6844a39caecf2970b43618c9b2 Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Wed, 12 Aug 2026 18:04:36 +0200 Subject: [PATCH 05/11] [misc] Build release branches, and don't mark releases latest before signing (#7171) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prepares the repository for the release-branch process agreed internally: one long-lived release-0.N branch per minor, with fixes backported by cherry-pick and patch releases tagged from the branch. Pushes to release-* branches now run the Release workflow and publish immutable sha-* container images, the way pushes to main already do, so a release branch can be tested before it is tagged. Release branches never publish the floating "main" image tag. The push-to-main CI workflows (Go tests on all platforms, frontend UI, install script, mobile/wasm validation, infrastructure files, license check) also run on release-* pushes; pull request triggers were already unfiltered, so backport PRs were covered — this closes the post-merge gap. Releases are no longer marked latest before signing: make_latest is now false in all four goreleaser configs, so a release stays published but not latest until the signing pipeline uploads the signed Windows and macOS artifacts and marks it latest itself. Previously the release became GitHub's "Latest release" at publish time, and the download endpoints that resolve through the latest-release API could serve a release whose signed installers did not exist yet. prerelease: auto additionally labels rc tags as prereleases, so a release candidate can never take the latest slot. The trigger_sync_tag job is removed: it dispatched a downstream image build on every v* tag (release candidates included), which would race the deliberate release-branch build on every release. The android and ios submodule bumps are unchanged. Also sets perennial-regex = "^release-" so git-town never syncs or ships a release branch into main. --- .git-branches.toml | 2 +- .github/workflows/check-license-dependencies.yml | 2 +- .github/workflows/frontend-ui.yml | 1 + .github/workflows/golang-test-darwin.yml | 1 + .github/workflows/golang-test-freebsd.yml | 1 + .github/workflows/golang-test-linux.yml | 1 + .github/workflows/golang-test-windows.yml | 1 + .github/workflows/install-script-test.yml | 1 + .github/workflows/mobile-build-validation.yml | 1 + .github/workflows/release.yml | 15 ++++++++++++--- .github/workflows/sync-tag.yml | 16 ++-------------- .github/workflows/test-infrastructure-files.yml | 1 + .github/workflows/wasm-build-validation.yml | 1 + .goreleaser.yaml | 7 +++++++ .goreleaser_ui.yaml | 8 ++++++++ .goreleaser_ui_darwin.yaml | 8 ++++++++ .goreleaser_ui_gtk3.yaml | 8 ++++++++ 17 files changed, 56 insertions(+), 19 deletions(-) diff --git a/.git-branches.toml b/.git-branches.toml index d1818090f..4c34d7928 100644 --- a/.git-branches.toml +++ b/.git-branches.toml @@ -3,7 +3,7 @@ [branches] main = "main" perennials = [] -perennial-regex = "" +perennial-regex = "^release-" [create] new-branch-type = "feature" diff --git a/.github/workflows/check-license-dependencies.yml b/.github/workflows/check-license-dependencies.yml index 17c9fdc8d..81d293e4f 100644 --- a/.github/workflows/check-license-dependencies.yml +++ b/.github/workflows/check-license-dependencies.yml @@ -2,7 +2,7 @@ name: Check License Dependencies on: push: - branches: [main] + branches: [main, "release-*"] paths: - "go.mod" - "go.sum" diff --git a/.github/workflows/frontend-ui.yml b/.github/workflows/frontend-ui.yml index 552ccef29..014c5c2ae 100644 --- a/.github/workflows/frontend-ui.yml +++ b/.github/workflows/frontend-ui.yml @@ -10,6 +10,7 @@ on: push: branches: - main + - "release-*" paths: - "client/ui/frontend/**" - "client/ui/i18n/**" diff --git a/.github/workflows/golang-test-darwin.yml b/.github/workflows/golang-test-darwin.yml index 420749a0e..c17d8e775 100644 --- a/.github/workflows/golang-test-darwin.yml +++ b/.github/workflows/golang-test-darwin.yml @@ -4,6 +4,7 @@ on: push: branches: - main + - "release-*" pull_request: concurrency: diff --git a/.github/workflows/golang-test-freebsd.yml b/.github/workflows/golang-test-freebsd.yml index 9c795e783..65c39147a 100644 --- a/.github/workflows/golang-test-freebsd.yml +++ b/.github/workflows/golang-test-freebsd.yml @@ -4,6 +4,7 @@ on: push: branches: - main + - "release-*" pull_request: concurrency: diff --git a/.github/workflows/golang-test-linux.yml b/.github/workflows/golang-test-linux.yml index 0af506bba..004b78b3e 100644 --- a/.github/workflows/golang-test-linux.yml +++ b/.github/workflows/golang-test-linux.yml @@ -4,6 +4,7 @@ on: push: branches: - main + - "release-*" pull_request: concurrency: diff --git a/.github/workflows/golang-test-windows.yml b/.github/workflows/golang-test-windows.yml index 50a5ba4d6..fb7b745d2 100644 --- a/.github/workflows/golang-test-windows.yml +++ b/.github/workflows/golang-test-windows.yml @@ -4,6 +4,7 @@ on: push: branches: - main + - "release-*" pull_request: env: diff --git a/.github/workflows/install-script-test.yml b/.github/workflows/install-script-test.yml index 1514caedc..61709501c 100644 --- a/.github/workflows/install-script-test.yml +++ b/.github/workflows/install-script-test.yml @@ -4,6 +4,7 @@ on: push: branches: - main + - "release-*" pull_request: paths: - "release_files/install.sh" diff --git a/.github/workflows/mobile-build-validation.yml b/.github/workflows/mobile-build-validation.yml index 44e912c73..322f129c9 100644 --- a/.github/workflows/mobile-build-validation.yml +++ b/.github/workflows/mobile-build-validation.yml @@ -4,6 +4,7 @@ on: push: branches: - main + - "release-*" pull_request: concurrency: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 727bff45a..4d1945451 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -6,6 +6,7 @@ on: - "v*" branches: - main + - "release-*" pull_request: env: @@ -254,15 +255,23 @@ jobs: id: tag_and_push_images if: | (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) || - (github.event_name == 'push' && github.ref == 'refs/heads/main') + (github.event_name == 'push' && (github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/heads/release-'))) run: | set -euo pipefail + # $GITHUB_REF / $GITHUB_EVENT_NAME are read from the runner + # environment rather than substituted into this script with the + # workflow expression syntax: branch names may legally contain + # $(…), and interpolating github.ref would execute it. resolve_tags() { - if [[ "${{ github.event_name }}" == "pull_request" ]]; then + if [[ "$GITHUB_EVENT_NAME" == "pull_request" ]]; then echo "pr-${{ github.event.pull_request.number }}" - else + elif [[ "$GITHUB_REF" == "refs/heads/main" ]]; then echo "main sha-$(git rev-parse --short HEAD)" + else + # Release branches get an immutable sha-* tag only — the floating + # "main" tag must never move from a release branch. + echo "sha-$(git rev-parse --short HEAD)" fi } diff --git a/.github/workflows/sync-tag.yml b/.github/workflows/sync-tag.yml index d99f88b54..088e538d5 100644 --- a/.github/workflows/sync-tag.yml +++ b/.github/workflows/sync-tag.yml @@ -9,21 +9,9 @@ concurrency: group: ${{ github.workflow }}-${{ github.ref }}-${{ github.head_ref || github.actor_id }} cancel-in-progress: true -# Receiving workflows (cloud sync-tag, mobile bump-netbird) expect the short -# tag form (e.g. v0.30.0), not refs/tags/v0.30.0 — github.ref_name, not github.ref. +# The receiving bump-netbird workflows expect the short tag form +# (e.g. v0.30.0), not refs/tags/v0.30.0 — github.ref_name, not github.ref. jobs: - trigger_sync_tag: - runs-on: ubuntu-latest - steps: - - name: Trigger release tag sync - uses: benc-uk/workflow-dispatch@31e2b3319479a63f0ab15bf800eff9e913504e26 # v1.3.2 - with: - workflow: sync-tag.yml - ref: main - repo: ${{ secrets.UPSTREAM_REPO }} - token: ${{ secrets.NC_GITHUB_TOKEN }} - inputs: '{ "tag": "${{ github.ref_name }}" }' - trigger_android_bump: runs-on: ubuntu-latest if: github.event.created && !github.event.deleted && startsWith(github.ref, 'refs/tags/v') && !contains(github.ref_name, '-') diff --git a/.github/workflows/test-infrastructure-files.yml b/.github/workflows/test-infrastructure-files.yml index 729214d9e..1313379ee 100644 --- a/.github/workflows/test-infrastructure-files.yml +++ b/.github/workflows/test-infrastructure-files.yml @@ -4,6 +4,7 @@ on: push: branches: - main + - "release-*" pull_request: paths: - "infrastructure_files/**" diff --git a/.github/workflows/wasm-build-validation.yml b/.github/workflows/wasm-build-validation.yml index e8a12cdaf..5f21472e5 100644 --- a/.github/workflows/wasm-build-validation.yml +++ b/.github/workflows/wasm-build-validation.yml @@ -4,6 +4,7 @@ on: push: branches: - main + - "release-*" pull_request: concurrency: diff --git a/.goreleaser.yaml b/.goreleaser.yaml index 8dd05a192..c5d260376 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -468,6 +468,13 @@ checksum: - glob: ./infrastructure_files/migrate-to-enterprise.sh release: + # The signing pipeline (netbirdio/sign-pipelines, dispatched by + # trigger_signer) marks the release latest once the Windows and macOS + # artifacts are signed. Without this override goreleaser marks it latest + # at publish time, while those artifacts are still unsigned. + make_latest: false + # Mark x.y.z-rc.* and other prerelease tags as prereleases on GitHub. + prerelease: auto extra_files: - glob: ./infrastructure_files/getting-started-with-zitadel.sh - glob: ./release_files/install.sh diff --git a/.goreleaser_ui.yaml b/.goreleaser_ui.yaml index c61b8c474..1c5bc41ac 100644 --- a/.goreleaser_ui.yaml +++ b/.goreleaser_ui.yaml @@ -144,3 +144,11 @@ uploads: target: https://pkgs.wiretrustee.com/yum/{{ .Arch }}{{ if .Arm }}{{ .Arm }}{{ end }} username: dev@wiretrustee.com method: PUT + +release: + # Uploads into the release created by the main .goreleaser.yaml run. + # make_latest stays false everywhere: the signing pipeline + # (netbirdio/sign-pipelines) marks the release latest after the Windows + # and macOS artifacts are signed. + make_latest: false + prerelease: auto diff --git a/.goreleaser_ui_darwin.yaml b/.goreleaser_ui_darwin.yaml index 47b991344..8ca0e8da6 100644 --- a/.goreleaser_ui_darwin.yaml +++ b/.goreleaser_ui_darwin.yaml @@ -43,3 +43,11 @@ checksum: name_template: "{{ .ProjectName }}_darwin_checksums.txt" changelog: disable: true + +release: + # Uploads into the release created by the main .goreleaser.yaml run. + # make_latest stays false everywhere: the signing pipeline + # (netbirdio/sign-pipelines) marks the release latest after the Windows + # and macOS artifacts are signed. + make_latest: false + prerelease: auto diff --git a/.goreleaser_ui_gtk3.yaml b/.goreleaser_ui_gtk3.yaml index a6cfd199e..a9b2ca650 100644 --- a/.goreleaser_ui_gtk3.yaml +++ b/.goreleaser_ui_gtk3.yaml @@ -134,3 +134,11 @@ uploads: target: https://pkgs.wiretrustee.com/yum/{{ .Arch }}{{ if .Arm }}{{ .Arm }}{{ end }} username: dev@wiretrustee.com method: PUT + +release: + # Uploads into the release created by the main .goreleaser.yaml run. + # make_latest stays false everywhere: the signing pipeline + # (netbirdio/sign-pipelines) marks the release latest after the Windows + # and macOS artifacts are signed. + make_latest: false + prerelease: auto From 58c09ead211d2283f78b24bd9a4ccaa5a1a517e8 Mon Sep 17 00:00:00 2001 From: Jack Carter <128555021+SunsetDrifter@users.noreply.github.com> Date: Wed, 12 Aug 2026 20:39:06 +0200 Subject: [PATCH 06/11] [management] Document mutual exclusivity of policy rule ports and port_ranges (#7158) --- shared/management/http/api/openapi.yml | 7 ++++--- shared/management/http/api/types.gen.go | 14 +++++++------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index 7728961a2..3622ee1ef 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -1433,13 +1433,14 @@ components: enum: [ "all", "tcp", "udp", "icmp", "netbird-ssh" ] example: "tcp" ports: - description: Policy rule affected ports + description: Policy rule affected ports. Mutually exclusive with `port_ranges`. A rule accepts either individual ports or port ranges, not both. + x-omit-from-example: true type: array items: type: string example: "80" port_ranges: - description: Policy rule affected ports ranges list + description: Policy rule affected ports ranges list. Mutually exclusive with `ports`. To mix individual ports with ranges in one rule, express each single port as a range with identical start and end values (for example, start 443, end 443). type: array items: $ref: '#/components/schemas/RulePortRange' @@ -1459,7 +1460,7 @@ components: - action RulePortRange: - description: Policy rule affected ports range + description: Policy rule affected ports range. A range with identical start and end values represents a single port. type: object properties: start: diff --git a/shared/management/http/api/types.gen.go b/shared/management/http/api/types.gen.go index e5d32bfc4..825caad13 100644 --- a/shared/management/http/api/types.gen.go +++ b/shared/management/http/api/types.gen.go @@ -4468,10 +4468,10 @@ type PolicyRule struct { // Name Policy rule name identifier Name string `json:"name"` - // PortRanges Policy rule affected ports ranges list + // PortRanges Policy rule affected ports ranges list. Mutually exclusive with `ports`. To mix individual ports with ranges in one rule, express each single port as a range with identical start and end values (for example, start 443, end 443). PortRanges *[]RulePortRange `json:"port_ranges,omitempty"` - // Ports Policy rule affected ports + // Ports Policy rule affected ports. Mutually exclusive with `port_ranges`. A rule accepts either individual ports or port ranges, not both. Ports *[]string `json:"ports,omitempty"` // Protocol Policy rule type of the traffic @@ -4508,10 +4508,10 @@ type PolicyRuleMinimum struct { // Name Policy rule name identifier Name string `json:"name"` - // PortRanges Policy rule affected ports ranges list + // PortRanges Policy rule affected ports ranges list. Mutually exclusive with `ports`. To mix individual ports with ranges in one rule, express each single port as a range with identical start and end values (for example, start 443, end 443). PortRanges *[]RulePortRange `json:"port_ranges,omitempty"` - // Ports Policy rule affected ports + // Ports Policy rule affected ports. Mutually exclusive with `port_ranges`. A rule accepts either individual ports or port ranges, not both. Ports *[]string `json:"ports,omitempty"` // Protocol Policy rule type of the traffic @@ -4551,10 +4551,10 @@ type PolicyRuleUpdate struct { // Name Policy rule name identifier Name string `json:"name"` - // PortRanges Policy rule affected ports ranges list + // PortRanges Policy rule affected ports ranges list. Mutually exclusive with `ports`. To mix individual ports with ranges in one rule, express each single port as a range with identical start and end values (for example, start 443, end 443). PortRanges *[]RulePortRange `json:"port_ranges,omitempty"` - // Ports Policy rule affected ports + // Ports Policy rule affected ports. Mutually exclusive with `port_ranges`. A rule accepts either individual ports or port ranges, not both. Ports *[]string `json:"ports,omitempty"` // Protocol Policy rule type of the traffic @@ -4962,7 +4962,7 @@ type RouteRequest struct { SkipAutoApply *bool `json:"skip_auto_apply,omitempty"` } -// RulePortRange Policy rule affected ports range +// RulePortRange Policy rule affected ports range. A range with identical start and end values represents a single port. type RulePortRange struct { // End The ending port of the range End int `json:"end"` From e290769df10ceb7fd0176c9c8c2cca2d2d545c86 Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:28:34 +0900 Subject: [PATCH 07/11] [client] Take the graphical session answer from the caller instead of the daemon environment (#7187) --- client/cmd/login.go | 16 +++------- client/cmd/up.go | 4 +-- client/proto/daemon.pb.go | 44 ++++++++++++++++++++------ client/proto/daemon.proto | 8 +++++ client/server/server.go | 15 +++------ client/ssh/common.go | 5 +-- client/ui/authsession/service.go | 3 +- client/ui/services/connection.go | 9 +++--- util/common.go | 53 +++++++++++++++++++++++++++++++- util/session_test.go | 50 ++++++++++++++++++++++++++++++ 10 files changed, 164 insertions(+), 43 deletions(-) create mode 100644 util/session_test.go diff --git a/client/cmd/login.go b/client/cmd/login.go index a53cb6d5f..6aa019896 100644 --- a/client/cmd/login.go +++ b/client/cmd/login.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "os/user" - "runtime" "strings" log "github.com/sirupsen/logrus" @@ -121,7 +120,7 @@ func doDaemonLogin(ctx context.Context, cmd *cobra.Command, providedSetupKey str loginRequest := proto.LoginRequest{ SetupKey: providedSetupKey, ManagementUrl: managementURL, - IsUnixDesktopClient: isUnixRunningDesktop(), + IsUnixDesktopClient: util.HasGraphicalSession(), Hostname: hostName, DnsLabels: dnsLabelsReq, ProfileName: &handle, @@ -189,7 +188,8 @@ func doExtendSession(ctx context.Context, cmd *cobra.Command) error { client := proto.NewDaemonServiceClient(conn) - req := &proto.RequestExtendAuthSessionRequest{} + // the CLI runs in the user's session, the daemon does not: tell it what we can see + req := &proto.RequestExtendAuthSessionRequest{HasGraphicalSession: util.HasGraphicalSession()} // Pre-fill the IdP login hint from the active profile so the user // doesn't have to retype their email. Best-effort: we still proceed // without a hint if the lookup fails. @@ -408,7 +408,7 @@ func foregroundGetTokenInfo(ctx context.Context, cmd *cobra.Command, config *pro hint = profileState.Email } - oAuthFlow, err := auth.NewOAuthFlow(ctx, config, isUnixRunningDesktop(), false, hint) + oAuthFlow, err := auth.NewOAuthFlow(ctx, config, util.HasGraphicalSession(), false, hint) if err != nil { return nil, err } @@ -458,14 +458,6 @@ func openURL(cmd *cobra.Command, verificationURIComplete, userCode string, noBro } } -// isUnixRunningDesktop checks if a Linux OS is running desktop environment -func isUnixRunningDesktop() bool { - if runtime.GOOS != "linux" && runtime.GOOS != "freebsd" { - return false - } - return os.Getenv("DESKTOP_SESSION") != "" || os.Getenv("XDG_CURRENT_DESKTOP") != "" -} - func setEnvAndFlags(cmd *cobra.Command) error { SetFlagsFromEnvVars(rootCmd) diff --git a/client/cmd/up.go b/client/cmd/up.go index 142bcf6bd..9f4fa8c33 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -21,8 +21,8 @@ import ( "github.com/netbirdio/netbird/client/internal" "github.com/netbirdio/netbird/client/internal/peer" "github.com/netbirdio/netbird/client/internal/profilemanager" - "github.com/netbirdio/netbird/client/proto" nbnet "github.com/netbirdio/netbird/client/net" + "github.com/netbirdio/netbird/client/proto" "github.com/netbirdio/netbird/client/server" "github.com/netbirdio/netbird/client/system" "github.com/netbirdio/netbird/shared/management/domain" @@ -626,7 +626,7 @@ func setupLoginRequest(providedSetupKey string, customDNSAddressConverted []byte NatExternalIPs: natExternalIPs, CleanNATExternalIPs: natExternalIPs != nil && len(natExternalIPs) == 0, CustomDNSAddress: customDNSAddressConverted, - IsUnixDesktopClient: isUnixRunningDesktop(), + IsUnixDesktopClient: util.HasGraphicalSession(), Hostname: hostName, ExtraIFaceBlacklist: extraIFaceBlackList, DnsLabels: dnsLabels, diff --git a/client/proto/daemon.pb.go b/client/proto/daemon.pb.go index 83243be49..b438a310a 100644 --- a/client/proto/daemon.pb.go +++ b/client/proto/daemon.pb.go @@ -5628,9 +5628,13 @@ func (x *GetPeerSSHHostKeyResponse) GetFound() bool { type RequestJWTAuthRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // hint for OIDC login_hint parameter (typically email address) - Hint *string `protobuf:"bytes,1,opt,name=hint,proto3,oneof" json:"hint,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + Hint *string `protobuf:"bytes,1,opt,name=hint,proto3,oneof" json:"hint,omitempty"` + // hasGraphicalSession tells the daemon that the caller has a graphical session, + // which decides whether PKCE or the device code flow is preferred. The daemon + // cannot detect this itself: it does not inherit the session environment. + HasGraphicalSession bool `protobuf:"varint,2,opt,name=hasGraphicalSession,proto3" json:"hasGraphicalSession,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *RequestJWTAuthRequest) Reset() { @@ -5670,6 +5674,13 @@ func (x *RequestJWTAuthRequest) GetHint() string { return "" } +func (x *RequestJWTAuthRequest) GetHasGraphicalSession() bool { + if x != nil { + return x.HasGraphicalSession + } + return false +} + // RequestJWTAuthResponse contains authentication flow information type RequestJWTAuthResponse struct { state protoimpl.MessageState `protogen:"open.v1"` @@ -5894,9 +5905,13 @@ type RequestExtendAuthSessionRequest struct { state protoimpl.MessageState `protogen:"open.v1"` // Optional OIDC login_hint (typically the user's email) to pre-fill the // IdP login form. - Hint *string `protobuf:"bytes,1,opt,name=hint,proto3,oneof" json:"hint,omitempty"` - unknownFields protoimpl.UnknownFields - sizeCache protoimpl.SizeCache + Hint *string `protobuf:"bytes,1,opt,name=hint,proto3,oneof" json:"hint,omitempty"` + // hasGraphicalSession tells the daemon that the caller has a graphical session, + // which decides whether PKCE or the device code flow is preferred. The daemon + // cannot detect this itself: it does not inherit the session environment. + HasGraphicalSession bool `protobuf:"varint,2,opt,name=hasGraphicalSession,proto3" json:"hasGraphicalSession,omitempty"` + unknownFields protoimpl.UnknownFields + sizeCache protoimpl.SizeCache } func (x *RequestExtendAuthSessionRequest) Reset() { @@ -5936,6 +5951,13 @@ func (x *RequestExtendAuthSessionRequest) GetHint() string { return "" } +func (x *RequestExtendAuthSessionRequest) GetHasGraphicalSession() bool { + if x != nil { + return x.HasGraphicalSession + } + return false +} + // RequestExtendAuthSessionResponse carries the verification URI the UI // should open in a browser. The daemon retains the flow state and resolves // it via WaitExtendAuthSession. @@ -7503,9 +7525,10 @@ const file_daemon_proto_rawDesc = "" + "sshHostKey\x12\x16\n" + "\x06peerIP\x18\x02 \x01(\tR\x06peerIP\x12\x1a\n" + "\bpeerFQDN\x18\x03 \x01(\tR\bpeerFQDN\x12\x14\n" + - "\x05found\x18\x04 \x01(\bR\x05found\"9\n" + + "\x05found\x18\x04 \x01(\bR\x05found\"k\n" + "\x15RequestJWTAuthRequest\x12\x17\n" + - "\x04hint\x18\x01 \x01(\tH\x00R\x04hint\x88\x01\x01B\a\n" + + "\x04hint\x18\x01 \x01(\tH\x00R\x04hint\x88\x01\x01\x120\n" + + "\x13hasGraphicalSession\x18\x02 \x01(\bR\x13hasGraphicalSessionB\a\n" + "\x05_hint\"\x9a\x02\n" + "\x16RequestJWTAuthResponse\x12(\n" + "\x0fverificationURI\x18\x01 \x01(\tR\x0fverificationURI\x128\n" + @@ -7525,9 +7548,10 @@ const file_daemon_proto_rawDesc = "" + "\x14WaitJWTTokenResponse\x12\x14\n" + "\x05token\x18\x01 \x01(\tR\x05token\x12\x1c\n" + "\ttokenType\x18\x02 \x01(\tR\ttokenType\x12\x1c\n" + - "\texpiresIn\x18\x03 \x01(\x03R\texpiresIn\"C\n" + + "\texpiresIn\x18\x03 \x01(\x03R\texpiresIn\"u\n" + "\x1fRequestExtendAuthSessionRequest\x12\x17\n" + - "\x04hint\x18\x01 \x01(\tH\x00R\x04hint\x88\x01\x01B\a\n" + + "\x04hint\x18\x01 \x01(\tH\x00R\x04hint\x88\x01\x01\x120\n" + + "\x13hasGraphicalSession\x18\x02 \x01(\bR\x13hasGraphicalSessionB\a\n" + "\x05_hint\"\xe0\x01\n" + " RequestExtendAuthSessionResponse\x12(\n" + "\x0fverificationURI\x18\x01 \x01(\tR\x0fverificationURI\x128\n" + diff --git a/client/proto/daemon.proto b/client/proto/daemon.proto index 18ce0e79c..a3e3f4500 100644 --- a/client/proto/daemon.proto +++ b/client/proto/daemon.proto @@ -894,6 +894,10 @@ message GetPeerSSHHostKeyResponse { message RequestJWTAuthRequest { // hint for OIDC login_hint parameter (typically email address) optional string hint = 1; + // hasGraphicalSession tells the daemon that the caller has a graphical session, + // which decides whether PKCE or the device code flow is preferred. The daemon + // cannot detect this itself: it does not inherit the session environment. + bool hasGraphicalSession = 2; } // RequestJWTAuthResponse contains authentication flow information @@ -937,6 +941,10 @@ message RequestExtendAuthSessionRequest { // Optional OIDC login_hint (typically the user's email) to pre-fill the // IdP login form. optional string hint = 1; + // hasGraphicalSession tells the daemon that the caller has a graphical session, + // which decides whether PKCE or the device code flow is preferred. The daemon + // cannot detect this itself: it does not inherit the session environment. + bool hasGraphicalSession = 2; } // RequestExtendAuthSessionResponse carries the verification URI the UI diff --git a/client/server/server.go b/client/server/server.go index 01778b8e0..f33e19075 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -1723,8 +1723,8 @@ func (s *Server) RequestJWTAuth( hint = profilemanager.GetLoginHint() } - isDesktop := isUnixRunningDesktop() - oAuthFlow, err := auth.NewOAuthFlow(ctx, config, isDesktop, false, hint) + // the daemon has no graphical session of its own, only the caller can answer this + oAuthFlow, err := auth.NewOAuthFlow(ctx, config, msg.GetHasGraphicalSession(), false, hint) if err != nil { return nil, gstatus.Errorf(codes.Internal, "failed to create OAuth flow: %v", err) } @@ -1827,8 +1827,8 @@ func (s *Server) RequestExtendAuthSession( hint = profilemanager.GetLoginHint() } - isDesktop := isUnixRunningDesktop() - oAuthFlow, err := auth.NewOAuthFlow(ctx, config, isDesktop, false, hint) + // the daemon has no graphical session of its own, only the caller can answer this + oAuthFlow, err := auth.NewOAuthFlow(ctx, config, msg.GetHasGraphicalSession(), false, hint) if err != nil { return nil, gstatus.Errorf(codes.Internal, "failed to create OAuth flow: %v", err) } @@ -2000,13 +2000,6 @@ func (s *Server) ExposeService(req *proto.ExposeServiceRequest, srv proto.Daemon return nil } -func isUnixRunningDesktop() bool { - if runtime.GOOS != "linux" && runtime.GOOS != "freebsd" { - return false - } - return os.Getenv("DESKTOP_SESSION") != "" || os.Getenv("XDG_CURRENT_DESKTOP") != "" -} - func (s *Server) runProbes(ctx context.Context, waitForProbeResult bool) { if s.connectClient == nil { return diff --git a/client/ssh/common.go b/client/ssh/common.go index 92e647b7d..3f4f3e9d1 100644 --- a/client/ssh/common.go +++ b/client/ssh/common.go @@ -13,6 +13,7 @@ import ( "golang.org/x/crypto/ssh" "github.com/netbirdio/netbird/client/proto" + "github.com/netbirdio/netbird/util" ) const ( @@ -92,7 +93,8 @@ func printAuthInstructions(stderr io.Writer, authResponse *proto.RequestJWTAuthR // RequestJWTToken requests or retrieves a JWT token for SSH authentication func RequestJWTToken(ctx context.Context, client proto.DaemonServiceClient, stdout, stderr io.Writer, useCache bool, hint string, openBrowser func(string) error) (string, error) { - req := &proto.RequestJWTAuthRequest{} + // the ssh client runs in the user's session, the daemon does not: tell it what we can see + req := &proto.RequestJWTAuthRequest{HasGraphicalSession: util.HasGraphicalSession()} if hint != "" { req.Hint = &hint } @@ -193,4 +195,3 @@ func buildAddressList(hostname string, remote net.Addr) []string { } return addresses } - diff --git a/client/ui/authsession/service.go b/client/ui/authsession/service.go index 28efe7cfd..d94cef696 100644 --- a/client/ui/authsession/service.go +++ b/client/ui/authsession/service.go @@ -58,7 +58,8 @@ func (s *Session) RequestExtend(ctx context.Context, p ExtendStartParams) (Exten return ExtendStartResult{}, err } - req := &proto.RequestExtendAuthSessionRequest{} + // a request from the UI implies a graphical session, which the daemon cannot detect itself + req := &proto.RequestExtendAuthSessionRequest{HasGraphicalSession: true} if p.Hint != "" { h := p.Hint req.Hint = &h diff --git a/client/ui/services/connection.go b/client/ui/services/connection.go index 1069f8754..aa649bb6d 100644 --- a/client/ui/services/connection.go +++ b/client/ui/services/connection.go @@ -108,10 +108,11 @@ func (s *Connection) Login(ctx context.Context, p LoginParams) (LoginResult, err } req := &proto.LoginRequest{ - ManagementUrl: p.ManagementURL, - SetupKey: p.SetupKey, - Hostname: p.Hostname, - IsUnixDesktopClient: runtime.GOOS == "linux", + ManagementUrl: p.ManagementURL, + SetupKey: p.SetupKey, + Hostname: p.Hostname, + // a login driven by the UI always has a graphical session available + IsUnixDesktopClient: true, } if profileName != "" { req.ProfileName = ptrStr(profileName) diff --git a/util/common.go b/util/common.go index 89903b609..c08be3617 100644 --- a/util/common.go +++ b/util/common.go @@ -3,18 +3,69 @@ package util import ( "os" "os/exec" + "runtime" + "slices" "github.com/skratchdot/open-golang/open" ) +const ( + // envBrowser overrides the browser OpenBrowser launches + envBrowser = "BROWSER" + // envDesktopSession and envXDGCurrentDesktop are what xdg-open uses to pick a handler + envDesktopSession = "DESKTOP_SESSION" + envXDGCurrentDesktop = "XDG_CURRENT_DESKTOP" + // envDisplay and envWaylandDisplay are what a graphical browser needs to reach a display + envDisplay = "DISPLAY" + envWaylandDisplay = "WAYLAND_DISPLAY" + // envXDGSessionType names the session kind, e.g. tty, x11 or wayland + envXDGSessionType = "XDG_SESSION_TYPE" +) + // OpenBrowser opens the URL in a browser, respecting the BROWSER environment variable. func OpenBrowser(url string) error { - if browser := os.Getenv("BROWSER"); browser != "" { + if browser := os.Getenv(envBrowser); browser != "" { return exec.Command(browser, url).Start() } return open.Run(url) } +// browserSessionEnvVars returns the variables that decide whether OpenBrowser can open a URL. +// DISPLAY and WAYLAND_DISPLAY are exactly what xdg-open's own has_display() checks, and without +// them it degrades to terminal browsers. BROWSER is the explicit override both xdg-open and +// OpenBrowser honor first. DESKTOP_SESSION and XDG_CURRENT_DESKTOP only tell xdg-open which +// desktop-specific opener to prefer, so they are weaker evidence, kept because the previous +// detection relied on them alone and dropping them would demote sessions that work today. +func browserSessionEnvVars() []string { + return []string{envDisplay, envWaylandDisplay, envBrowser, envDesktopSession, envXDGCurrentDesktop} +} + +// graphicalXDGSessionTypes are the systemd-logind session types that come with a display. The +// other documented values are "tty" and "unspecified"; anything unrecognized is treated as no +// display, so an unknown value picks the device code flow, which works without a browser. +func graphicalXDGSessionTypes() []string { + return []string{"x11", "wayland", "mir"} +} + +// HasGraphicalSession reports whether this process can open a browser and serve a loopback +// redirect back to it. Windows and macOS always can. On Linux and FreeBSD the answer is env +// based, so it only holds for a process started from the graphical session itself: a service +// does not inherit those variables and always reports false, which is why callers running in +// the user's session pass their own answer to the daemon. +func HasGraphicalSession() bool { + if runtime.GOOS != "linux" && runtime.GOOS != "freebsd" { + return true + } + + for _, env := range browserSessionEnvVars() { + if os.Getenv(env) != "" { + return true + } + } + + return slices.Contains(graphicalXDGSessionTypes(), os.Getenv(envXDGSessionType)) +} + // SliceDiff returns the elements in slice `x` that are not in slice `y` func SliceDiff(x, y []string) []string { mapY := make(map[string]struct{}, len(y)) diff --git a/util/session_test.go b/util/session_test.go new file mode 100644 index 000000000..f301ff8f7 --- /dev/null +++ b/util/session_test.go @@ -0,0 +1,50 @@ +package util + +import ( + "os" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestHasGraphicalSession(t *testing.T) { + if runtime.GOOS != "linux" && runtime.GOOS != "freebsd" { + assert.True(t, HasGraphicalSession(), "%s always has a graphical session", runtime.GOOS) + return + } + + // clear anything inherited from the session running the test, restored on cleanup + for _, env := range append(browserSessionEnvVars(), envXDGSessionType) { + t.Setenv(env, "") + os.Unsetenv(env) + } + + assert.False(t, HasGraphicalSession(), "no session variables means no graphical session") + + tests := []struct { + env string + value string + expected bool + }{ + {env: envDisplay, value: ":0", expected: true}, + {env: envWaylandDisplay, value: "wayland-0", expected: true}, + {env: envDesktopSession, value: "gnome", expected: true}, + {env: envXDGCurrentDesktop, value: "KDE", expected: true}, + {env: envBrowser, value: "firefox", expected: true}, + {env: envXDGSessionType, value: "wayland", expected: true}, + {env: envXDGSessionType, value: "x11", expected: true}, + {env: envXDGSessionType, value: "mir", expected: true}, + {env: envXDGSessionType, value: "tty", expected: false}, + {env: envXDGSessionType, value: "unspecified", expected: false}, + // an unrecognized type must not be read as a display: the device code flow works anyway + {env: envXDGSessionType, value: "something-new", expected: false}, + } + + for _, tt := range tests { + t.Run(tt.env+"="+tt.value, func(t *testing.T) { + t.Setenv(tt.env, tt.value) + assert.Equal(t, tt.expected, HasGraphicalSession(), "%s=%s", tt.env, tt.value) + }) + } +} From 1d372bb6348f2e7073c9a3657b15bc6c3b895af9 Mon Sep 17 00:00:00 2001 From: Kim Harre <99537307+znel2002@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:58:03 +0200 Subject: [PATCH 08/11] [infrastructure] Support non-interactive installation in getting-started.sh (#7168) --- infrastructure_files/getting-started.sh | 99 ++++++++++++++++++++----- 1 file changed, 80 insertions(+), 19 deletions(-) diff --git a/infrastructure_files/getting-started.sh b/infrastructure_files/getting-started.sh index 4f2c1d82e..0fc5b23c5 100755 --- a/infrastructure_files/getting-started.sh +++ b/infrastructure_files/getting-started.sh @@ -111,6 +111,59 @@ check_nb_domain() { return 0 } +# Non-interactive configuration +# ------------------------------ +# Every prompt below can be pre-answered with an environment variable, so the +# script runs unattended (cloud-init, CI, Terraform, curl | bash). resolve() +# is the single place that decides env var vs prompt vs default; the read_* +# helpers stay pure prompts. +# +# Supported env vars: +# NETBIRD_DOMAIN domain/FQDN (required) +# NETBIRD_LETSENCRYPT_EMAIL ACME email (required for built-in Traefik) +# NETBIRD_AGENT_NETWORK true enables the agent-network preset +# NETBIRD_REVERSE_PROXY_TYPE 0-5 (default 0 = built-in Traefik) +# NETBIRD_ENABLE_PROXY true/false (default false) +# NETBIRD_ENABLE_CROWDSEC true/false (default false) +# NETBIRD_TRAEFIK_EXTERNAL_NETWORK external-Traefik network (type 1) +# NETBIRD_TRAEFIK_ENTRYPOINT external-Traefik entrypoint (type 1, default websecure) +# NETBIRD_TRAEFIK_CERTRESOLVER external-Traefik cert resolver (type 1) +# NETBIRD_BIND_LOCALHOST_ONLY true/false (default true, types 2-5) +# NETBIRD_EXTERNAL_PROXY_NETWORK docker network to join (types 2-4) +# NETBIRD_NON_INTERACTIVE true forces unattended mode even with a TTY + +# tty_available succeeds only when we may prompt: never when the operator has +# set NETBIRD_NON_INTERACTIVE=true, otherwise only when /dev/tty can actually +# be opened. A PTY can be attached in automation (CI runners, some +# provisioners), so the env override is the authoritative signal and the +# /dev/tty probe is the fallback. /dev/tty is a world-rw device node even with +# no terminal, so a permission test ([ -r ]) is not enough - we must open it. +tty_available() { + [[ "${NETBIRD_NON_INTERACTIVE:-}" == "true" ]] && return 1 + { true < /dev/tty; } 2>/dev/null +} + +# resolve ENV_VAR_NAME DEFAULT PROMPT_FN [prompt args...] +# env var set and non-empty -> its value +# interactive -> PROMPT_FN "$@" (prompt behavior unchanged) +# otherwise -> DEFAULT, or abort when DEFAULT is "required" +resolve() { + local env_name="$1" default="$2" prompt_fn="$3" + shift 3 + local env_value="${!env_name:-}" + if [[ -n "$env_value" ]]; then + echo "$env_value" + elif tty_available; then + "$prompt_fn" "$@" + elif [[ "$default" == "required" ]]; then + echo "$env_name is required for a non-interactive install." > /dev/stderr + exit 1 + else + echo "$default" + fi + return 0 +} + read_nb_domain() { READ_NETBIRD_DOMAIN="" echo -n "Enter the domain you want to use for NetBird (e.g. netbird.my-domain.com): " > /dev/stderr @@ -383,7 +436,14 @@ initialize_default_values() { } configure_domain() { + # Domain is validated (not a free-form value), so it keeps its own guard + # rather than going through resolve(): a valid NETBIRD_DOMAIN is used as-is, + # otherwise we prompt, or abort when there is no terminal to prompt on. if ! check_nb_domain "$NETBIRD_DOMAIN"; then + if ! tty_available; then + echo "NETBIRD_DOMAIN is required for a non-interactive install." > /dev/stderr + exit 1 + fi NETBIRD_DOMAIN=$(read_nb_domain) fi @@ -411,11 +471,7 @@ apply_agent_network_preset() { ENABLE_PROXY="true" ENABLE_CROWDSEC="false" - if [[ -n "${NETBIRD_LETSENCRYPT_EMAIL}" ]]; then - TRAEFIK_ACME_EMAIL="${NETBIRD_LETSENCRYPT_EMAIL}" - else - TRAEFIK_ACME_EMAIL=$(read_traefik_acme_email) - fi + TRAEFIK_ACME_EMAIL=$(resolve NETBIRD_LETSENCRYPT_EMAIL required read_traefik_acme_email) echo "" > /dev/stderr echo "Agent-network preset enabled (NETBIRD_AGENT_NETWORK=true):" > /dev/stderr @@ -437,35 +493,35 @@ configure_reverse_proxy() { return 0 fi - # Prompt for reverse proxy type - REVERSE_PROXY_TYPE=$(read_reverse_proxy_type) + # Reverse proxy type (env NETBIRD_REVERSE_PROXY_TYPE, else prompt, else 0) + REVERSE_PROXY_TYPE=$(resolve NETBIRD_REVERSE_PROXY_TYPE 0 read_reverse_proxy_type) # Handle built-in Traefik prompts (option 0) if [[ "$REVERSE_PROXY_TYPE" == "0" ]]; then - TRAEFIK_ACME_EMAIL=$(read_traefik_acme_email) - ENABLE_PROXY=$(read_enable_proxy) + TRAEFIK_ACME_EMAIL=$(resolve NETBIRD_LETSENCRYPT_EMAIL required read_traefik_acme_email) + ENABLE_PROXY=$(resolve NETBIRD_ENABLE_PROXY false read_enable_proxy) if [[ "$ENABLE_PROXY" == "true" ]]; then - ENABLE_CROWDSEC=$(read_enable_crowdsec) + ENABLE_CROWDSEC=$(resolve NETBIRD_ENABLE_CROWDSEC false read_enable_crowdsec) fi fi # Handle external Traefik-specific prompts (option 1) if [[ "$REVERSE_PROXY_TYPE" == "1" ]]; then - TRAEFIK_EXTERNAL_NETWORK=$(read_traefik_network) - TRAEFIK_ENTRYPOINT=$(read_traefik_entrypoint) - TRAEFIK_CERTRESOLVER=$(read_traefik_certresolver) + TRAEFIK_EXTERNAL_NETWORK=$(resolve NETBIRD_TRAEFIK_EXTERNAL_NETWORK "" read_traefik_network) + TRAEFIK_ENTRYPOINT=$(resolve NETBIRD_TRAEFIK_ENTRYPOINT websecure read_traefik_entrypoint) + TRAEFIK_CERTRESOLVER=$(resolve NETBIRD_TRAEFIK_CERTRESOLVER "" read_traefik_certresolver) fi # Handle port binding for external proxy options (2-5) if [[ "$REVERSE_PROXY_TYPE" -ge 2 ]]; then - BIND_LOCALHOST_ONLY=$(read_port_binding_preference) + BIND_LOCALHOST_ONLY=$(resolve NETBIRD_BIND_LOCALHOST_ONLY true read_port_binding_preference) fi # Handle Docker network prompts for external proxies (options 2-4) case "$REVERSE_PROXY_TYPE" in - 2) EXTERNAL_PROXY_NETWORK=$(read_proxy_docker_network "Nginx") ;; - 3) EXTERNAL_PROXY_NETWORK=$(read_proxy_docker_network "Nginx Proxy Manager") ;; - 4) EXTERNAL_PROXY_NETWORK=$(read_proxy_docker_network "Caddy") ;; + 2) EXTERNAL_PROXY_NETWORK=$(resolve NETBIRD_EXTERNAL_PROXY_NETWORK "" read_proxy_docker_network "Nginx") ;; + 3) EXTERNAL_PROXY_NETWORK=$(resolve NETBIRD_EXTERNAL_PROXY_NETWORK "" read_proxy_docker_network "Nginx Proxy Manager") ;; + 4) EXTERNAL_PROXY_NETWORK=$(resolve NETBIRD_EXTERNAL_PROXY_NETWORK "" read_proxy_docker_network "Caddy") ;; *) ;; # No network prompt for other options esac return 0 @@ -643,8 +699,13 @@ start_services_and_show_instructions() { print_post_setup_instructions echo "" - echo -n "Press Enter when your reverse proxy is configured (or Ctrl+C to exit)... " - read -r < /dev/tty + if tty_available; then + echo -n "Press Enter when your reverse proxy is configured (or Ctrl+C to exit)... " + read -r < /dev/tty + else + echo "Non-interactive mode: starting NetBird containers now. Finish configuring" + echo "your reverse proxy using the instructions above so it can reach them." + fi echo -e "$MSG_STARTING_SERVICES" $DOCKER_COMPOSE_COMMAND up -d From 5544761b4780626b092af715cc7572baf30e8f9c Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Fri, 14 Aug 2026 03:07:37 +0900 Subject: [PATCH 09/11] [client] Add Windows DNS configuration to the debug bundle (#7196) --- client/anonymize/anonymize.go | 12 +- client/anonymize/reverse_zone.go | 174 ++++++++ client/anonymize/reverse_zone_test.go | 171 ++++++++ client/internal/debug/debug.go | 8 + client/internal/debug/debug_nonunix.go | 2 +- client/internal/debug/debug_windows.go | 443 ++++++++++++++++++++ client/internal/debug/debug_windows_test.go | 146 +++++++ client/internal/debug/nrpt_windows.go | 317 ++++++++++++++ client/internal/dns/host_windows.go | 32 +- go.mod | 2 +- 10 files changed, 1296 insertions(+), 11 deletions(-) create mode 100644 client/anonymize/reverse_zone.go create mode 100644 client/anonymize/reverse_zone_test.go create mode 100644 client/internal/debug/debug_windows.go create mode 100644 client/internal/debug/debug_windows_test.go create mode 100644 client/internal/debug/nrpt_windows.go diff --git a/client/anonymize/anonymize.go b/client/anonymize/anonymize.go index acadb717b..c5d43ed55 100644 --- a/client/anonymize/anonymize.go +++ b/client/anonymize/anonymize.go @@ -305,6 +305,12 @@ func (a *Anonymizer) AnonymizeDomain(domain string) string { return domain } + // A reverse zone names an address prefix, so it follows the address rules, + // which also keeps its digit labels intact. + if zone, ok := a.anonymizeReverseZone(baseDomain); ok { + return withTrailingDot(zone, hasDot) + } + if suffix := protectedSuffix(baseDomain); suffix != "" { if a.level < LevelStrict || baseDomain == suffix || suffix == infraDomain { return domain @@ -405,6 +411,10 @@ func (a *Anonymizer) AnonymizeString(str string) string { ipv4Regex := regexp.MustCompile(`\b(?:[0-9]{1,3}\.){3}[0-9]{1,3}\b`) ipv6Regex := regexp.MustCompile(`\b([0-9a-fA-F:]+:+[0-9a-fA-F]{0,4})(?:%[0-9a-zA-Z]+)?(?:\/[0-9]{1,3})?(?::[0-9]{1,5})?\b`) + // Reverse zones go first and are then held out of the passes below: their + // labels are digits, which the address patterns would otherwise consume. + str, restoreZones := a.replaceReverseZones(str) + str = ipv4Regex.ReplaceAllStringFunc(str, a.AnonymizeIPString) str = ipv6Regex.ReplaceAllStringFunc(str, a.AnonymizeIPString) @@ -425,7 +435,7 @@ func (a *Anonymizer) AnonymizeString(str string) string { str = wgKeyRegex.ReplaceAllStringFunc(str, a.AnonymizeWGKey) } - return str + return restoreZones(str) } // sortedDomains returns the domain mappings longest-first, so a full-FQDN diff --git a/client/anonymize/reverse_zone.go b/client/anonymize/reverse_zone.go new file mode 100644 index 000000000..b521b71b7 --- /dev/null +++ b/client/anonymize/reverse_zone.go @@ -0,0 +1,174 @@ +package anonymize + +import ( + "encoding/hex" + "net/netip" + "regexp" + "strconv" + "strings" +) + +const ( + reverseZoneSuffixV4 = ".in-addr.arpa" + reverseZoneSuffixV6 = ".ip6.arpa" + + v6Nibbles = 32 + v4Octets = 4 +) + +// reverseZoneRegexes match a reverse zone or a full reverse name in free text. +// They are applied before the address passes of AnonymizeString, whose IPv4 +// pattern would otherwise consume the digit labels of a zone and replace parts +// of it with unrelated addresses. +var reverseZoneRegexes = []*regexp.Regexp{ + regexp.MustCompile(`(?:[0-9]{1,3}\.){1,4}in-addr\.arpa\b`), + regexp.MustCompile(`(?:[0-9a-fA-F]\.){1,32}ip6\.arpa\b`), +} + +// anonymizeReverseZone maps a reverse zone to the zone of the anonymized form +// of the prefix it encodes, so it follows the address rules rather than the +// domain ones: the zone of an address that is preserved is preserved too, and +// the zone of one that is replaced names the replacement. This keeps a reverse +// zone recognizable as such, and consistent with the addresses it belongs to +// elsewhere in the same output. It reports false for anything that is not a +// reverse zone. +func (a *Anonymizer) anonymizeReverseZone(domain string) (string, bool) { + prefix, labelCount, suffix, ok := parseReverseZone(domain) + if !ok { + return "", false + } + + anonymized := a.AnonymizeIP(prefix) + if anonymized == prefix { + return domain, true + } + + return reverseZoneName(anonymized, labelCount) + suffix, true +} + +// replaceReverseZones anonymizes every reverse zone in str and swaps each one +// for a placeholder, returning a function that puts the anonymized zones back. +// The placeholders carry no dots, digits or colons, so no later pass matches +// them. +func (a *Anonymizer) replaceReverseZones(str string) (string, func(string) string) { + var zones []string + + for _, re := range reverseZoneRegexes { + str = re.ReplaceAllStringFunc(str, func(match string) string { + zone, ok := a.anonymizeReverseZone(match) + if !ok { + return match + } + + zones = append(zones, zone) + return reverseZonePlaceholder(len(zones) - 1) + }) + } + + if len(zones) == 0 { + return str, func(s string) string { return s } + } + + return str, func(s string) string { + for i, zone := range zones { + s = strings.ReplaceAll(s, reverseZonePlaceholder(i), zone) + } + return s + } +} + +func reverseZonePlaceholder(index int) string { + return "\x00reversezone" + strconv.Itoa(index) + "\x00" +} + +// parseReverseZone turns a reverse zone into the address of the prefix its +// labels spell backwards, padding the absent low-order part with zeroes, and +// returns the label count and zone suffix so the name can be rebuilt. +func parseReverseZone(domain string) (netip.Addr, int, string, bool) { + lower := strings.ToLower(domain) + + switch { + case strings.HasSuffix(lower, reverseZoneSuffixV4): + labels := strings.Split(strings.TrimSuffix(lower, reverseZoneSuffixV4), ".") + addr, ok := reverseZoneAddrV4(labels) + return addr, len(labels), reverseZoneSuffixV4, ok + case strings.HasSuffix(lower, reverseZoneSuffixV6): + labels := strings.Split(strings.TrimSuffix(lower, reverseZoneSuffixV6), ".") + addr, ok := reverseZoneAddrV6(labels) + return addr, len(labels), reverseZoneSuffixV6, ok + default: + return netip.Addr{}, 0, "", false + } +} + +func reverseZoneAddrV4(labels []string) (netip.Addr, bool) { + if len(labels) == 0 || len(labels) > v4Octets { + return netip.Addr{}, false + } + + var octets [v4Octets]byte + for i, label := range labels { + octet, err := strconv.ParseUint(label, 10, 8) + if err != nil { + return netip.Addr{}, false + } + octets[len(labels)-1-i] = byte(octet) + } + + return netip.AddrFrom4(octets), true +} + +func reverseZoneAddrV6(labels []string) (netip.Addr, bool) { + if len(labels) == 0 || len(labels) > v6Nibbles { + return netip.Addr{}, false + } + + nibbles := make([]byte, 0, v6Nibbles) + for i := len(labels) - 1; i >= 0; i-- { + if len(labels[i]) != 1 || !isHexDigit(labels[i][0]) { + return netip.Addr{}, false + } + nibbles = append(nibbles, labels[i][0]) + } + for len(nibbles) < v6Nibbles { + nibbles = append(nibbles, '0') + } + + var groups []string + for i := 0; i < len(nibbles); i += 4 { + groups = append(groups, string(nibbles[i:i+4])) + } + + addr, err := netip.ParseAddr(strings.Join(groups, ":")) + if err != nil { + return netip.Addr{}, false + } + + return addr, true +} + +// reverseZoneName spells the first labelCount labels of addr backwards, the +// inverse of parseReverseZone, without the zone suffix. +func reverseZoneName(addr netip.Addr, labelCount int) string { + labels := make([]string, 0, labelCount) + + if addr.Is4() { + octets := addr.As4() + for i := labelCount - 1; i >= 0; i-- { + labels = append(labels, strconv.Itoa(int(octets[i]))) + } + return strings.Join(labels, ".") + } + + address := addr.As16() + nibbles := hex.EncodeToString(address[:]) + for i := labelCount - 1; i >= 0; i-- { + labels = append(labels, string(nibbles[i])) + } + + return strings.Join(labels, ".") +} + +func isHexDigit(c byte) bool { + return c >= '0' && c <= '9' || c >= 'a' && c <= 'f' || c >= 'A' && c <= 'F' +} diff --git a/client/anonymize/reverse_zone_test.go b/client/anonymize/reverse_zone_test.go new file mode 100644 index 000000000..8c3b8954a --- /dev/null +++ b/client/anonymize/reverse_zone_test.go @@ -0,0 +1,171 @@ +package anonymize + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newLeveledAnonymizer(level Level) *Anonymizer { + a := NewAnonymizer(DefaultAddresses()) + a.SetLevel(level) + return a +} + +// TestAnonymizeDomainReverseZone covers reverse zones going through the address +// rules instead of the domain ones, so a zone stays a zone and an address that +// is preserved keeps the zone that names it. +func TestAnonymizeDomainReverseZone(t *testing.T) { + // 100.64.0.0/10 is the overlay range, which is CGNAT: preserved at the + // default level and replaced from the internal pool at the strict one + const overlayZone = "64.100.in-addr.arpa" + + t.Run("overlay zone preserved at the default level", func(t *testing.T) { + a := newLeveledAnonymizer(LevelDefault) + assert.Equal(t, overlayZone, a.AnonymizeDomain(overlayZone), "should keep the zone of a preserved address") + }) + + t.Run("private zone preserved at the default level", func(t *testing.T) { + a := newLeveledAnonymizer(LevelDefault) + assert.Equal(t, "168.192.in-addr.arpa", a.AnonymizeDomain("168.192.in-addr.arpa"), "should keep the zone of a private address") + }) + + t.Run("overlay zone replaced at the strict level", func(t *testing.T) { + a := newLeveledAnonymizer(LevelStrict) + + got := a.AnonymizeDomain(overlayZone) + require.True(t, strings.HasSuffix(got, reverseZoneSuffixV4), "should stay a reverse zone, got %q", got) + assert.NotEqual(t, overlayZone, got, "should replace the encoded prefix") + assert.Len(t, strings.Split(strings.TrimSuffix(got, reverseZoneSuffixV4), "."), 2, + "should keep the label count, got %q", got) + }) + + t.Run("public zone replaced at the default level", func(t *testing.T) { + a := newLeveledAnonymizer(LevelDefault) + + got := a.AnonymizeDomain("113.0.203.in-addr.arpa") + require.True(t, strings.HasSuffix(got, reverseZoneSuffixV4), "should stay a reverse zone, got %q", got) + assert.NotEqual(t, "113.0.203.in-addr.arpa", got, "should replace a public prefix") + }) + + t.Run("zone of an address keeps that address mapping", func(t *testing.T) { + a := newLeveledAnonymizer(LevelDefault) + + anonymizedAddr := a.AnonymizeIPString("203.0.113.7") + got := a.AnonymizeDomain("7.113.0.203.in-addr.arpa") + + octets := strings.Split(anonymizedAddr, ".") + want := octets[3] + "." + octets[2] + "." + octets[1] + "." + octets[0] + reverseZoneSuffixV4 + assert.Equal(t, want, got, "should name the same replacement as the address itself") + }) + + t.Run("ipv6 nibble labels stay single digits", func(t *testing.T) { + a := newLeveledAnonymizer(LevelDefault) + + zone := "0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.0" + reverseZoneSuffixV6 + got := a.AnonymizeDomain(zone) + + require.True(t, strings.HasSuffix(got, reverseZoneSuffixV6), "should stay a reverse zone, got %q", got) + labels := strings.Split(strings.TrimSuffix(got, reverseZoneSuffixV6), ".") + assert.Len(t, labels, 28, "should keep every nibble label, got %q", got) + for _, label := range labels { + assert.Len(t, label, 1, "nibble label %q should stay a single digit", label) + } + }) + + t.Run("trailing dot is kept", func(t *testing.T) { + a := newLeveledAnonymizer(LevelDefault) + assert.Equal(t, "64.100.in-addr.arpa.", a.AnonymizeDomain("64.100.in-addr.arpa."), "should keep the trailing dot") + }) + + t.Run("a domain that only looks like a zone is anonymized as a domain", func(t *testing.T) { + a := newLeveledAnonymizer(LevelDefault) + + got := a.AnonymizeDomain("not-a-zone.in-addr.arpa") + assert.NotContains(t, got, "in-addr.arpa", "should fall back to domain anonymization") + }) +} + +// TestAnonymizeStringReverseZone verifies that a zone inside free text, such as +// a DNS log line, is not chewed up by the address passes. The IPv4 pattern +// matches any run of dotted digits, which a reverse zone is made of. +func TestAnonymizeStringReverseZone(t *testing.T) { + t.Run("ipv6 zone survives the address passes", func(t *testing.T) { + a := newLeveledAnonymizer(LevelDefault) + + zone := "0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.0" + reverseZoneSuffixV6 + got := a.AnonymizeString("question: domain=" + zone + " type=PTR") + + assert.Contains(t, got, "type=PTR", "should keep the rest of the line") + assert.NotContains(t, got, "198.51.100", "should not rewrite nibble labels as an address") + + labels := strings.Split(strings.TrimSuffix(strings.TrimPrefix(got, "question: domain="), reverseZoneSuffixV6+" type=PTR"), ".") + assert.Len(t, labels, 28, "should keep every nibble label, got %q", got) + }) + + t.Run("preserved ipv4 zone is untouched", func(t *testing.T) { + a := newLeveledAnonymizer(LevelDefault) + + line := "reverse zone 64.100.in-addr.arpa registered" + assert.Equal(t, line, a.AnonymizeString(line), "should keep the zone of a preserved address") + }) + + t.Run("public ipv4 zone is replaced consistently", func(t *testing.T) { + a := newLeveledAnonymizer(LevelDefault) + + got := a.AnonymizeString("zone 113.0.203.in-addr.arpa and address 203.0.113.7") + assert.NotContains(t, got, "113.0.203.in-addr.arpa", "should replace the zone") + assert.NotContains(t, got, "203.0.113.7", "should replace the address") + assert.Contains(t, got, reverseZoneSuffixV4, "should keep the zone suffix") + }) +} + +func TestParseReverseZone(t *testing.T) { + tests := []struct { + name string + zone string + addr string + labels int + }{ + {name: "v4 two labels", zone: "0.100" + reverseZoneSuffixV4, addr: "100.0.0.0", labels: 2}, + {name: "v4 three labels", zone: "1.168.192" + reverseZoneSuffixV4, addr: "192.168.1.0", labels: 3}, + {name: "v4 full address", zone: "7.113.0.203" + reverseZoneSuffixV4, addr: "203.0.113.7", labels: 4}, + { + name: "v6 prefix", + zone: "0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.0" + reverseZoneSuffixV6, + addr: "2::", + labels: 28, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + addr, labels, suffix, ok := parseReverseZone(tc.zone) + require.True(t, ok, "should decode the reverse zone") + assert.Equal(t, tc.addr, addr.String(), "should decode to the encoded prefix") + assert.Equal(t, tc.labels, labels, "should count the labels") + assert.Equal(t, tc.zone, reverseZoneName(addr, labels)+suffix, "should re-encode to the original zone") + }) + } +} + +func TestParseReverseZoneRejectsNonZones(t *testing.T) { + tests := []string{ + "example.com", + "in-addr.arpa", + "x.100" + reverseZoneSuffixV4, + "256" + reverseZoneSuffixV4, + "1.2.3.4.5" + reverseZoneSuffixV4, + "ab" + reverseZoneSuffixV6, + "g" + reverseZoneSuffixV6, + } + + for _, zone := range tests { + t.Run(zone, func(t *testing.T) { + _, _, _, ok := parseReverseZone(zone) + assert.False(t, ok, "should reject %q", zone) + }) + } +} diff --git a/client/internal/debug/debug.go b/client/internal/debug/debug.go index dbe22139a..1d31c75ca 100644 --- a/client/internal/debug/debug.go +++ b/client/internal/debug/debug.go @@ -51,6 +51,7 @@ nftables.txt: Anonymized nftables rules with packet counters across all families sysctls.txt: Forwarding, reverse-path filter, source-validation, and conntrack accounting sysctl values that the NetBird client may read or modify, if --system-info flag was provided (Linux only). resolv.conf: DNS resolver configuration from /etc/resolv.conf (Unix systems only), if --system-info flag was provided. scutil_dns.txt: DNS configuration from scutil --dns (macOS only), if --system-info flag was provided. +dns_windows.txt: Anonymized NRPT rules and policy table in effect, DNS client policy, and per-interface and per-adapter DNS configuration (Windows only), if --system-info flag was provided. resolved_domains.txt: Anonymized resolved domain IP addresses from the status recorder. config.txt: Anonymized configuration information of the NetBird client. network_map.json: Anonymized sync response containing peer configurations, routes, DNS settings, and firewall rules. @@ -237,6 +238,13 @@ scutil_dns.txt (macOS only): - Shows DNS configuration for all network interfaces - Includes search domains, nameservers, and DNS resolver settings - All IP addresses and domain names are anonymized + +dns_windows.txt (Windows only): +- Lists the NRPT rules of both policy stores, the local one and the group policy one, marking the rules the client created +- Follows them with the policy table the resolver has loaded, which differs from the rules while a change has not been picked up yet +- Includes the DNS client group policy, the global TCP/IP and Dnscache parameters, and the DNS values of every interface that has any +- Ends with the resolver configuration in effect per adapter, from GetAdaptersAddresses +- All IP addresses and domain names are anonymized ` const ( diff --git a/client/internal/debug/debug_nonunix.go b/client/internal/debug/debug_nonunix.go index 18d017050..adc9b9649 100644 --- a/client/internal/debug/debug_nonunix.go +++ b/client/internal/debug/debug_nonunix.go @@ -1,4 +1,4 @@ -//go:build !unix +//go:build !unix && !windows package debug diff --git a/client/internal/debug/debug_windows.go b/client/internal/debug/debug_windows.go new file mode 100644 index 000000000..e88940fd3 --- /dev/null +++ b/client/internal/debug/debug_windows.go @@ -0,0 +1,443 @@ +//go:build windows + +package debug + +import ( + "encoding/hex" + "errors" + "fmt" + "net/netip" + "strings" + "unsafe" + + log "github.com/sirupsen/logrus" + "golang.org/x/sys/windows" + "golang.org/x/sys/windows/registry" + + nbdns "github.com/netbirdio/netbird/client/internal/dns" +) + +const dnsInfoFileName = "dns_windows.txt" + +const ( + gpoDNSClientRoot = `SOFTWARE\Policies\Microsoft\Windows NT\DNSClient` + tcpipParamsPath = `SYSTEM\CurrentControlSet\Services\Tcpip\Parameters` + dnscacheParams = `SYSTEM\CurrentControlSet\Services\Dnscache\Parameters` +) + +// interfaceDNSValues are the per-interface values that decide how a name is +// resolved and registered. Everything the DNS host manager writes is in here, +// so a bundle shows both what we set and what it replaced. +var interfaceDNSValues = []string{ + "NameServer", + "DhcpNameServer", + "Domain", + "DhcpDomain", + "SearchList", + "RegistrationEnabled", + "DisableDynamicUpdate", + "MaxNumberOfAddressesToRegister", + "EnableDHCP", +} + +// addDNSInfo collects and adds DNS configuration information to the archive +func (g *BundleGenerator) addDNSInfo() error { + if err := g.addFileToZip(strings.NewReader(g.collectDNSInfo()), dnsInfoFileName); err != nil { + return fmt.Errorf("add DNS info to zip: %w", err) + } + + return nil +} + +// collectDNSInfo renders the report. Everything below it reaches the platform +// through COM and through lazily resolved procedures, which panic when a +// procedure is missing rather than returning an error, and a debug bundle is not +// allowed to take the daemon down. The panic is contained here, and whatever was +// collected before it is kept and reported with it. +func (g *BundleGenerator) collectDNSInfo() (content string) { + var sb strings.Builder + + defer func() { + if r := recover(); r != nil { + log.Errorf("collecting Windows DNS configuration panicked: %v", r) + fmt.Fprintf(&sb, "\nerror: collection stopped: %v\n", r) + } + content = sb.String() + }() + + sb.WriteString("Windows DNS configuration\n") + sb.WriteString("=========================\n") + + adapters, adaptersErr := adapterAddresses() + + g.writeNRPTRules(&sb, "NRPT rules, local policy store", nbdns.DNSPolicyConfigRoot) + g.writeNRPTRules(&sb, "NRPT rules, group policy store", nbdns.GPODNSPolicyConfigRoot) + g.writeEffectiveNRPTPolicies(&sb) + g.writeRegistryKey(&sb, "DNS client group policy", gpoDNSClientRoot) + g.writeRegistryKey(&sb, "Global TCP/IP parameters", tcpipParamsPath) + g.writeRegistryKey(&sb, "Dnscache parameters", dnscacheParams) + g.writeInterfaceDNS(&sb, "Per-interface DNS, IPv4", nbdns.InterfaceConfigPath, adapterNames(adapters)) + g.writeInterfaceDNS(&sb, "Per-interface DNS, IPv6", nbdns.InterfaceConfigPathV6, adapterNames(adapters)) + g.writeAdapterDNS(&sb, adapters, adaptersErr) + + return sb.String() +} + +// writeNRPTRules lists every rule in a policy store, ours and any other +// product's, since a foreign rule for the same namespace decides resolution +// just as ours does. Rules the client wrote are marked. +func (g *BundleGenerator) writeNRPTRules(sb *strings.Builder, title, root string) { + writeSection(sb, title, root) + + names, err := subKeyNames(root) + if err != nil { + fmt.Fprintf(sb, "error: %v\n", err) + return + } + + if len(names) == 0 { + sb.WriteString("no rules\n") + return + } + + for _, name := range names { + owner := "" + if strings.HasPrefix(strings.ToLower(name), strings.ToLower(nbdns.NRPTKeyPrefix)) { + owner = " (netbird)" + } + fmt.Fprintf(sb, "%s%s\n", name, owner) + g.writeValues(sb, root+`\`+name, nil, " ") + } +} + +// writeEffectiveNRPTPolicies reports the table the resolver answers from, which +// the registry cannot show: a rule is written before it is loaded, and it keeps +// being enforced after its key is gone until the resolver reloads its policy. +func (g *BundleGenerator) writeEffectiveNRPTPolicies(sb *strings.Builder) { + writeSection(sb, "NRPT policy table in effect", nrptPolicyClass+"."+nrptPolicyMethod+" in "+nrptPolicyNamespace) + + entries, err := effectiveNRPTPolicies() + if err != nil { + fmt.Fprintf(sb, "error: %v\n", err) + return + } + + if len(entries) == 0 { + sb.WriteString("no policies\n") + return + } + + for _, entry := range entries { + fmt.Fprintf(sb, "%s\n", g.anonymizeValue("Namespace", entry.namespace)) + for _, value := range entry.values { + fmt.Fprintf(sb, " %s: %s\n", value.name, g.anonymizeValue(value.name, value.value)) + } + } +} + +// writeInterfaceDNS reports the DNS values of every interface that has any, so +// the netbird interface can be compared against the physical ones. The registry +// keys the values by GUID, so each is named from the adapter list; a GUID with +// no adapter is a leftover key of an interface that no longer exists. +func (g *BundleGenerator) writeInterfaceDNS(sb *strings.Builder, title, root string, names map[string]string) { + writeSection(sb, title, root) + + guids, err := subKeyNames(root) + if err != nil { + fmt.Fprintf(sb, "error: %v\n", err) + return + } + + var reported int + for _, guid := range guids { + var iface strings.Builder + g.writeValues(&iface, root+`\`+guid, interfaceDNSValues, " ") + if iface.Len() == 0 { + continue + } + + name, ok := names[strings.ToLower(guid)] + if !ok { + name = "no adapter with this GUID" + } + + reported++ + fmt.Fprintf(sb, "%s (%s)\n%s", guid, name, iface.String()) + } + + if reported == 0 { + sb.WriteString("no interface holds DNS values\n") + } +} + +// writeRegistryKey reports the values of a single key, without its subkeys. +func (g *BundleGenerator) writeRegistryKey(sb *strings.Builder, title, path string) { + writeSection(sb, title, path) + + var values strings.Builder + g.writeValues(&values, path, nil, "") + if values.Len() == 0 { + sb.WriteString("no values\n") + return + } + + sb.WriteString(values.String()) +} + +// writeValues renders the values of a key. A nil names list reports every +// value, otherwise only those named and present. +func (g *BundleGenerator) writeValues(sb *strings.Builder, path string, names []string, indent string) { + k, err := registry.OpenKey(registry.LOCAL_MACHINE, path, registry.QUERY_VALUE) + switch { + case errors.Is(err, registry.ErrNotExist), errors.Is(err, windows.ERROR_PATH_NOT_FOUND): + // an absent key is the normal state for the GPO store and for + // interfaces without DNS settings + log.Debugf("HKEY_LOCAL_MACHINE\\%s does not exist", path) + return + case err != nil: + fmt.Fprintf(sb, "%serror: open HKEY_LOCAL_MACHINE\\%s: %v\n", indent, path, err) + return + } + defer closeKey(k) + + if names == nil { + names, err = k.ReadValueNames(-1) + if err != nil { + fmt.Fprintf(sb, "%serror: read value names: %v\n", indent, err) + return + } + } + + for _, name := range names { + value, err := readRegistryValue(k, name) + switch { + case errors.Is(err, registry.ErrNotExist): + // the caller asks for a fixed set of values, most of which a + // given interface does not carry + continue + case err != nil: + // report rather than omit: a value that is there but cannot be + // read reads as unset otherwise + fmt.Fprintf(sb, "%s%s: error: %v\n", indent, name, err) + continue + } + + fmt.Fprintf(sb, "%s%s: %s\n", indent, name, g.anonymizeValue(name, value)) + } +} + +// anonymizeValue redacts a registry value according to what its name says it +// holds. Domains and addresses are handled per entry rather than by the string +// pass: the pass only replaces domains something else in the bundle already +// seeded, and its address regex would eat the digit labels of a reverse zone. +func (g *BundleGenerator) anonymizeValue(name, value string) string { + if !g.anonymize || value == "" { + return value + } + + switch { + case holdsDomains(name): + return joinValueEntries(splitValueEntries(value), g.anonymizeDomain) + case holdsAddresses(name): + return joinValueEntries(splitValueEntries(value), g.anonymizer.AnonymizeIPString) + default: + return g.anonymizer.AnonymizeString(value) + } +} + +// holdsDomains reports whether a value name holds domains: the domain list of +// an NRPT rule (Name) or of the policy table (Namespace), a search list, the +// DNS suffix values of the TCP/IP and policy keys, which all end in "Domain" +// (Domain, DhcpDomain, NV Domain, ICSDomain), and a proxy host name. +func holdsDomains(name string) bool { + lower := strings.ToLower(name) + return lower == "name" || lower == "namespace" || lower == "searchlist" || + strings.HasSuffix(lower, "domain") || strings.HasSuffix(lower, "proxyname") +} + +// holdsAddresses reports whether a value name holds DNS server addresses +// (NameServer, DhcpNameServer, GenericDNSServers, NameServers). +func holdsAddresses(name string) bool { + lower := strings.ToLower(name) + return strings.Contains(lower, "nameserver") || strings.Contains(lower, "dnsserver") +} + +// adapterNames maps adapter GUIDs, as the registry keys the interfaces, to the +// names an operator sees. +func adapterNames(adapters []*windows.IpAdapterAddresses) map[string]string { + names := make(map[string]string, len(adapters)) + for _, adapter := range adapters { + guid := windows.BytePtrToString(adapter.AdapterName) + names[strings.ToLower(guid)] = windows.UTF16PtrToString(adapter.FriendlyName) + } + return names +} + +// writeAdapterDNS reports the resolver configuration in effect per adapter, +// which is what the resolver uses for a name no NRPT rule matches. +func (g *BundleGenerator) writeAdapterDNS(sb *strings.Builder, adapters []*windows.IpAdapterAddresses, err error) { + writeSection(sb, "Adapter DNS configuration", "GetAdaptersAddresses") + + if err != nil { + fmt.Fprintf(sb, "error: %v\n", err) + return + } + + for _, adapter := range adapters { + name := windows.UTF16PtrToString(adapter.FriendlyName) + suffix := g.anonymizeDomain(windows.UTF16PtrToString(adapter.DnsSuffix)) + + fmt.Fprintf(sb, "%s (index %d, oper status %d)\n", name, adapter.IfIndex, adapter.OperStatus) + fmt.Fprintf(sb, " DNS suffix: %s\n", suffix) + + var servers []string + for server := adapter.FirstDnsServerAddress; server != nil; server = server.Next { + addr, ok := netip.AddrFromSlice(server.Address.IP()) + if !ok { + continue + } + + addr = addr.Unmap() + if g.anonymize { + addr = g.anonymizer.AnonymizeIP(addr) + } + servers = append(servers, addr.String()) + } + + fmt.Fprintf(sb, " DNS servers: %s\n", strings.Join(servers, ", ")) + } +} + +// anonymizeDomain anonymizes a single domain, keeping the leading dot an NRPT +// match domain carries. +func (g *BundleGenerator) anonymizeDomain(entry string) string { + if !g.anonymize { + return entry + } + + domain, dot := strings.CutPrefix(entry, ".") + if domain == "" { + return entry + } + + anonymized := g.anonymizer.AnonymizeDomain(domain) + if dot { + anonymized = "." + anonymized + } + return anonymized +} + +// splitValueEntries splits a registry value that holds a list. The separator +// differs per value: a REG_MULTI_SZ arrives joined with ", ", a SearchList is +// comma separated and a NameServer may use commas or spaces. +func splitValueEntries(value string) []string { + return strings.FieldsFunc(value, func(r rune) bool { + return r == ',' || r == ';' || r == ' ' || r == '\t' + }) +} + +func joinValueEntries(entries []string, anonymize func(string) string) string { + for i, entry := range entries { + entries[i] = anonymize(entry) + } + return strings.Join(entries, ", ") +} + +func writeSection(sb *strings.Builder, title, source string) { + fmt.Fprintf(sb, "\n%s\n%s\n%s\n", title, strings.Repeat("-", len(title)), source) +} + +func subKeyNames(root string) ([]string, error) { + k, err := registry.OpenKey(registry.LOCAL_MACHINE, root, registry.ENUMERATE_SUB_KEYS) + if err != nil { + return nil, fmt.Errorf("open HKEY_LOCAL_MACHINE\\%s: %w", root, err) + } + defer closeKey(k) + + names, err := k.ReadSubKeyNames(-1) + if err != nil { + return nil, fmt.Errorf("read subkey names: %w", err) + } + + return names, nil +} + +// readRegistryValue renders a value as text regardless of its type, so an +// unexpected type in a policy key still shows up instead of being dropped. +func readRegistryValue(k registry.Key, name string) (string, error) { + _, valueType, err := k.GetValue(name, nil) + if err != nil { + return "", fmt.Errorf("get value %s: %w", name, err) + } + + switch valueType { + case registry.SZ, registry.EXPAND_SZ: + value, _, err := k.GetStringValue(name) + if err != nil { + return "", fmt.Errorf("get string value %s: %w", name, err) + } + return value, nil + case registry.MULTI_SZ: + values, _, err := k.GetStringsValue(name) + if err != nil { + return "", fmt.Errorf("get strings value %s: %w", name, err) + } + return strings.Join(values, ", "), nil + case registry.DWORD, registry.QWORD: + value, _, err := k.GetIntegerValue(name) + if err != nil { + return "", fmt.Errorf("get integer value %s: %w", name, err) + } + return fmt.Sprintf("%d (0x%x)", value, value), nil + case registry.BINARY: + value, _, err := k.GetBinaryValue(name) + if err != nil { + return "", fmt.Errorf("get binary value %s: %w", name, err) + } + return hex.EncodeToString(value), nil + default: + return fmt.Sprintf("", valueType), nil + } +} + +// adapterAddresses returns the adapter list including DNS servers. The call +// reports the size it needs, so grow the buffer and retry until it fits. +func adapterAddresses() (adapters []*windows.IpAdapterAddresses, err error) { + // GetAdaptersAddresses is resolved on first use and panics when it is + // missing, so this reports it as an error and leaves the rest of the + // report intact. + defer func() { + if r := recover(); r != nil { + adapters, err = nil, fmt.Errorf("GetAdaptersAddresses: %v", r) + } + }() + + const flags = windows.GAA_FLAG_SKIP_ANYCAST | windows.GAA_FLAG_SKIP_MULTICAST + + size := uint32(15000) + for range 3 { + buf := make([]byte, size) + first := (*windows.IpAdapterAddresses)(unsafe.Pointer(&buf[0])) + + err := windows.GetAdaptersAddresses(windows.AF_UNSPEC, flags, 0, first, &size) + if errors.Is(err, windows.ERROR_BUFFER_OVERFLOW) { + continue + } + if err != nil { + return nil, fmt.Errorf("GetAdaptersAddresses: %w", err) + } + + for adapter := first; adapter != nil; adapter = adapter.Next { + adapters = append(adapters, adapter) + } + return adapters, nil + } + + return nil, fmt.Errorf("GetAdaptersAddresses: buffer kept growing") +} + +func closeKey(k registry.Key) { + if err := k.Close(); err != nil { + log.Debugf("close registry key: %v", err) + } +} diff --git a/client/internal/debug/debug_windows_test.go b/client/internal/debug/debug_windows_test.go new file mode 100644 index 000000000..47df3f6f9 --- /dev/null +++ b/client/internal/debug/debug_windows_test.go @@ -0,0 +1,146 @@ +//go:build windows + +package debug + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/anonymize" +) + +func newDNSValueGenerator(level anonymize.Level) *BundleGenerator { + anonymizer := anonymize.NewAnonymizer(anonymize.DefaultAddresses()) + anonymizer.SetLevel(level) + + return &BundleGenerator{ + anonymize: true, + anonymizeLevel: level, + anonymizer: anonymizer, + } +} + +// TestAnonymizeValueByName covers the value kinds of the DNS registry keys. The +// names decide the treatment, because the string pass alone replaces only +// domains another part of the bundle already seeded. +func TestAnonymizeValueByName(t *testing.T) { + tests := []struct { + name string + valueName string + value string + assert func(t *testing.T, got string) + }{ + { + name: "NRPT match domains keep the leading dot", + valueName: "Name", + value: ".internal.example.com, .corp.example.org", + assert: func(t *testing.T, got string) { + t.Helper() + for _, entry := range strings.Split(got, ", ") { + assert.True(t, strings.HasPrefix(entry, "."), "entry %q should keep its leading dot", entry) + assert.NotContains(t, entry, "example", "entry %q should not keep the original domain", entry) + } + }, + }, + { + name: "any value name ending in Domain is treated as a domain", + valueName: "ICSDomain", + value: "mshome.net", + assert: func(t *testing.T, got string) { + t.Helper() + assert.NotContains(t, got, "mshome", "should anonymize a domain suffix value") + }, + }, + { + name: "search list is a comma separated domain list", + valueName: "SearchList", + value: "corp.example.com,branch.example.com", + assert: func(t *testing.T, got string) { + t.Helper() + assert.NotContains(t, got, "example", "should anonymize every search domain") + assert.Len(t, strings.Split(got, ", "), 2, "should keep both search domains") + }, + }, + { + name: "name servers are anonymized as addresses", + valueName: "DhcpNameServer", + value: "203.0.113.10 8.8.8.8", + assert: func(t *testing.T, got string) { + t.Helper() + assert.NotContains(t, got, "203.0.113.10", "should anonymize a public resolver address") + // well-known resolvers stay readable at every level + assert.Contains(t, got, "8.8.8.8", "should keep a well-known resolver address") + }, + }, + { + name: "opaque values are left to the string pass", + valueName: "DataBasePath", + value: `%SystemRoot%\System32\drivers\etc`, + assert: func(t *testing.T, got string) { + t.Helper() + assert.Equal(t, `%SystemRoot%\System32\drivers\etc`, got, "should not alter a path") + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := newDNSValueGenerator(anonymize.LevelDefault) + tc.assert(t, g.anonymizeValue(tc.valueName, tc.value)) + }) + } +} + +// TestParseNRPTPolicyTable parses the MOF text of the policy table out +// parameters, as the provider on a client with one NRPT rule renders it. +func TestParseNRPTPolicyTable(t *testing.T) { + const text = `[abstract] +class __PARAMETERS +{ + [Out, EmbeddedInstance("DnsClientPolicyConfiguration"): ToSubClass, ID(2): DisableOverride ToInstance] DnsClientPolicyConfiguration cmdletOutput[] = { +instance of DnsClientPolicyConfiguration +{ + DirectAccessProxyType = "NoProxy"; + DirectAccessQueryIPsecRequired = FALSE; + NameEncoding = "Utf8WithoutMapping"; + Namespace = ".0.100.in-addr.arpa"; +}, +instance of DnsClientPolicyConfiguration +{ + DirectAccessProxyType = "NoProxy"; + NameEncoding = "Utf8WithoutMapping"; + NameServers = {"100.0.255.254", "100.0.255.253"}; + Namespace = ".nb.internal"; +}}; + [in] boolean Effective; + [out] uint32 ReturnValue = 0; +}; +` + + entries := parseNRPTPolicyTable(text) + require.Len(t, entries, 2, "should parse both embedded instances") + + assert.Equal(t, ".0.100.in-addr.arpa", entries[0].namespace, "should read the namespace of the first instance") + assert.Equal(t, ".nb.internal", entries[1].namespace, "should read the namespace of the second instance") + + assert.Equal(t, []registryValue{ + {name: "DirectAccessProxyType", value: "NoProxy"}, + {name: "DirectAccessQueryIPsecRequired", value: "FALSE"}, + {name: "NameEncoding", value: "Utf8WithoutMapping"}, + }, entries[0].values, "should keep the remaining values in order") + + assert.Contains(t, entries[1].values, registryValue{name: "NameServers", value: "100.0.255.254, 100.0.255.253"}, + "should flatten a MOF array") + + for _, value := range entries[1].values { + assert.NotContains(t, value.name, "ReturnValue", "should not read the class level parameters as values") + } +} + +func TestParseNRPTPolicyTableEmpty(t *testing.T) { + assert.Empty(t, parseNRPTPolicyTable(""), "should parse no entries from empty text") + assert.Empty(t, parseNRPTPolicyTable("class __PARAMETERS\n{\n};\n"), "should parse no entries from a table with no instances") +} diff --git a/client/internal/debug/nrpt_windows.go b/client/internal/debug/nrpt_windows.go new file mode 100644 index 000000000..6b6e0e29a --- /dev/null +++ b/client/internal/debug/nrpt_windows.go @@ -0,0 +1,317 @@ +//go:build windows + +package debug + +import ( + "errors" + "fmt" + "runtime" + "strings" + "time" + + "github.com/go-ole/go-ole" + "github.com/go-ole/go-ole/oleutil" + log "github.com/sirupsen/logrus" +) + +const ( + // The NRPT policy table is reachable through the CIM class that backs + // Get-DnsClientNrptPolicy. Unlike the rules in the registry, the table is + // what the resolver currently has loaded, which is the only way to tell an + // applied rule from one that is merely written, in either direction. + nrptPolicyNamespace = `root\Microsoft\Windows\DNS` + nrptPolicyClass = "PS_DnsClientNrptPolicy" + nrptPolicyMethod = "Get" + + // The class has no instances, so the table comes from the out parameters + // of a static method call, rendered as MOF text: the embedded instances + // arrive as a safe array of objects, which cannot be read back through the + // COM bindings, and the text form carries all of them. + nrptPolicyInstanceKeyword = "instance of DnsClientPolicyConfiguration" + + nrptPolicyTimeout = 15 * time.Second +) + +// COM initialization results that leave the calling thread usable: S_FALSE for +// a thread this process already initialized, RPC_E_CHANGED_MODE for one that +// belongs to another apartment. +const ( + sFalse = 0x00000001 + rpcEChangedMode = 0x80010106 +) + +// nrptQueryInFlight admits one read of the policy table at a time. A provider +// that stops answering keeps its goroutine and the OS thread that goroutine +// pinned, so a later bundle reports that instead of pinning another one. +var nrptQueryInFlight = make(chan struct{}, 1) + +// nrptPolicyEntry is one namespace of the effective policy table, holding the +// values of an embedded DnsClientPolicyConfiguration instance in the order the +// provider reported them. +type nrptPolicyEntry struct { + namespace string + values []registryValue +} + +// registryValue is a name and its rendered value, shared by the registry and +// policy table readers so both anonymize by value name the same way. +type registryValue struct { + name string + value string +} + +// effectiveNRPTPolicies reads the effective NRPT table. The call is bounded +// because a WMI provider can block indefinitely and a debug bundle must not. +func effectiveNRPTPolicies() ([]nrptPolicyEntry, error) { + type result struct { + text string + err error + } + + select { + case nrptQueryInFlight <- struct{}{}: + default: + return nil, errors.New("an earlier read of the policy table has not returned") + } + + done := make(chan result, 1) + go func() { + // the slot is released here rather than by the caller, so a read that + // outlives the timeout holds it until the provider answers + defer func() { <-nrptQueryInFlight }() + + text, err := nrptPolicyTableText() + done <- result{text: text, err: err} + }() + + select { + case res := <-done: + if res.err != nil { + return nil, res.err + } + return parseNRPTPolicyTable(res.text), nil + case <-time.After(nrptPolicyTimeout): + return nil, errors.New("read of the policy table timed out") + } +} + +// nrptPolicyTableText calls the policy table method and returns the MOF text of +// its out parameters. +func nrptPolicyTableText() (text string, err error) { + // COM is per thread, and the collection is short lived, so the thread is + // pinned for the duration rather than initialized for the process. + runtime.LockOSThread() + defer runtime.UnlockOSThread() + + defer func() { + // The COM call chain is dynamically typed, so a provider that answers + // with an unexpected shape must not take the daemon down with it. + if r := recover(); r != nil { + err = fmt.Errorf("read NRPT policy table: %v", r) + } + }() + + owns, err := coInitialize() + if err != nil { + return "", err + } + if owns { + defer ole.CoUninitialize() + } + + locator, err := oleutil.CreateObject("WbemScripting.SWbemLocator") + if err != nil { + return "", fmt.Errorf("create WMI locator: %w", err) + } + defer locator.Release() + + dispatch, err := locator.QueryInterface(ole.IID_IDispatch) + if err != nil { + return "", fmt.Errorf("query WMI locator interface: %w", err) + } + defer dispatch.Release() + + service, err := dispatchCall(dispatch, "ConnectServer", nil, nrptPolicyNamespace) + if err != nil { + return "", fmt.Errorf("connect to %s: %w", nrptPolicyNamespace, err) + } + defer service.Release() + + inParams, err := spawnMethodInParams(service) + if err != nil { + return "", err + } + defer inParams.Release() + + // The effective table is the merge of the local and the group policy + // store, which is what the resolver answers from. + if _, err := oleutil.PutProperty(inParams, "Effective", true); err != nil { + return "", fmt.Errorf("set Effective parameter: %w", err) + } + + outParams, err := dispatchCall(service, "ExecMethod", nrptPolicyClass, nrptPolicyMethod, inParams) + if err != nil { + return "", fmt.Errorf("call %s.%s: %w", nrptPolicyClass, nrptPolicyMethod, err) + } + defer outParams.Release() + + textVariant, err := oleutil.CallMethod(outParams, "GetObjectText_") + if err != nil { + return "", fmt.Errorf("render policy table: %w", err) + } + defer func() { + if err := textVariant.Clear(); err != nil { + log.Debugf("clear policy table variant: %v", err) + } + }() + + return textVariant.ToString(), nil +} + +// spawnMethodInParams builds the in parameters instance the method needs. The +// provider rejects the call without one, even when every parameter is optional. +func spawnMethodInParams(service *ole.IDispatch) (*ole.IDispatch, error) { + class, err := dispatchCall(service, "Get", nrptPolicyClass) + if err != nil { + return nil, fmt.Errorf("get class %s: %w", nrptPolicyClass, err) + } + defer class.Release() + + methods, err := dispatchProperty(class, "Methods_") + if err != nil { + return nil, fmt.Errorf("get class methods: %w", err) + } + defer methods.Release() + + method, err := dispatchCall(methods, "Item", nrptPolicyMethod) + if err != nil { + return nil, fmt.Errorf("get method %s: %w", nrptPolicyMethod, err) + } + defer method.Release() + + params, err := dispatchProperty(method, "InParameters") + if err != nil { + return nil, fmt.Errorf("get method parameters: %w", err) + } + defer params.Release() + + inParams, err := dispatchCall(params, "SpawnInstance_") + if err != nil { + return nil, fmt.Errorf("spawn parameter instance: %w", err) + } + + return inParams, nil +} + +// parseNRPTPolicyTable pulls the embedded instances out of the MOF text. Each +// instance is a namespace of the table, with one name and value per line. +func parseNRPTPolicyTable(text string) []nrptPolicyEntry { + var entries []nrptPolicyEntry + var current *nrptPolicyEntry + + for _, line := range strings.Split(text, "\n") { + line = strings.TrimSpace(strings.TrimSuffix(strings.TrimSpace(line), ";")) + + switch { + case strings.HasPrefix(line, nrptPolicyInstanceKeyword): + entries = append(entries, nrptPolicyEntry{}) + current = &entries[len(entries)-1] + continue + case strings.HasPrefix(line, "}"): + // closes an instance, and the array with the last one, so the + // class level parameters that follow are not read as values + current = nil + continue + case current == nil, line == "{": + continue + } + + name, value, ok := strings.Cut(line, " = ") + if !ok { + continue + } + + value = unquoteMOFValue(value) + if name == "Namespace" { + current.namespace = value + continue + } + + current.values = append(current.values, registryValue{name: name, value: value}) + } + + return entries +} + +// unquoteMOFValue renders a MOF scalar or array as plain text: "a" becomes a, +// and {"a", "b"} becomes a, b. +func unquoteMOFValue(value string) string { + value = strings.TrimSpace(value) + + if inner, ok := strings.CutPrefix(value, "{"); ok { + value = strings.TrimSuffix(inner, "}") + + entries := strings.Split(value, ",") + for i, entry := range entries { + entries[i] = strings.Trim(strings.TrimSpace(entry), `"`) + } + return strings.Join(entries, ", ") + } + + return strings.Trim(value, `"`) +} + +// coInitialize prepares the calling thread for COM and reports whether this +// call owns the initialization, which decides whether it may be balanced with +// CoUninitialize. S_FALSE took a reference on a thread this process had already +// initialized and so has to be released, while RPC_E_CHANGED_MODE took none: +// the thread belongs to another apartment, which is usable but is not ours to +// uninitialize. +func coInitialize() (bool, error) { + err := ole.CoInitializeEx(0, ole.COINIT_MULTITHREADED) + if err == nil { + return true, nil + } + + var oleErr *ole.OleError + if errors.As(err, &oleErr) { + switch oleErr.Code() { + case sFalse: + return true, nil + case rpcEChangedMode: + return false, nil + } + } + + return false, fmt.Errorf("initialize COM: %w", err) +} + +// dispatchCall calls a COM method that returns an object. +func dispatchCall(dispatch *ole.IDispatch, method string, params ...any) (*ole.IDispatch, error) { + variant, err := oleutil.CallMethod(dispatch, method, params...) + if err != nil { + return nil, err + } + + object := variant.ToIDispatch() + if object == nil { + return nil, fmt.Errorf("%s returned no object", method) + } + + return object, nil +} + +// dispatchProperty reads a COM property that holds an object. +func dispatchProperty(dispatch *ole.IDispatch, property string) (*ole.IDispatch, error) { + variant, err := oleutil.GetProperty(dispatch, property) + if err != nil { + return nil, err + } + + object := variant.ToIDispatch() + if object == nil { + return nil, fmt.Errorf("property %s holds no object", property) + } + + return object, nil +} diff --git a/client/internal/dns/host_windows.go b/client/internal/dns/host_windows.go index 4f6ece532..d20fdd1d6 100644 --- a/client/internal/dns/host_windows.go +++ b/client/internal/dns/host_windows.go @@ -31,10 +31,28 @@ var ( dnsFlushResolverCacheFn = dnsapi.NewProc("DnsFlushResolverCache") ) +// Registry locations of the host DNS configuration this package programs, +// exported so a diagnostic reader reports the same locations that are written. const ( - dnsPolicyConfigMatchPath = `SYSTEM\CurrentControlSet\Services\Dnscache\Parameters\DnsPolicyConfig\NetBird-Match` - gpoDnsPolicyRoot = `SOFTWARE\Policies\Microsoft\Windows NT\DNSClient\DnsPolicyConfig` - gpoDnsPolicyConfigMatchPath = gpoDnsPolicyRoot + `\NetBird-Match` + // NRPTKeyPrefix starts the name of every NRPT rule key this client creates. + NRPTKeyPrefix = "NetBird-Match" + + // DNSPolicyConfigRoot holds the NRPT rules of the local policy store. + DNSPolicyConfigRoot = `SYSTEM\CurrentControlSet\Services\Dnscache\Parameters\DnsPolicyConfig` + + // GPODNSPolicyConfigRoot holds the NRPT rules of the group policy store, + // which takes precedence over the local one when it is present. + GPODNSPolicyConfigRoot = `SOFTWARE\Policies\Microsoft\Windows NT\DNSClient\DnsPolicyConfig` + + // InterfaceConfigPath and InterfaceConfigPathV6 hold the per-interface DNS + // settings, keyed by interface GUID, in separate hives per address family. + InterfaceConfigPath = `SYSTEM\CurrentControlSet\Services\Tcpip\Parameters\Interfaces` + InterfaceConfigPathV6 = `SYSTEM\CurrentControlSet\Services\Tcpip6\Parameters\Interfaces` +) + +const ( + dnsPolicyConfigMatchPath = DNSPolicyConfigRoot + `\` + NRPTKeyPrefix + gpoDnsPolicyConfigMatchPath = GPODNSPolicyConfigRoot + `\` + NRPTKeyPrefix dnsPolicyConfigVersionKey = "Version" dnsPolicyConfigVersionValue = 2 @@ -45,8 +63,6 @@ const ( nrptMaxDomainsPerRule = 50 - interfaceConfigPath = `SYSTEM\CurrentControlSet\Services\Tcpip\Parameters\Interfaces` - interfaceConfigPathV6 = `SYSTEM\CurrentControlSet\Services\Tcpip6\Parameters\Interfaces` interfaceConfigNameServerKey = "NameServer" interfaceConfigDhcpNameSrvKey = "DhcpNameServer" interfaceConfigSearchListKey = "SearchList" @@ -84,7 +100,7 @@ func newHostManager(wgInterface WGIface) (*registryConfigurator, error) { } var useGPO bool - k, err := registry.OpenKey(registry.LOCAL_MACHINE, gpoDnsPolicyRoot, registry.QUERY_VALUE) + k, err := registry.OpenKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.QUERY_VALUE) if err != nil { log.Debugf("failed to open GPO DNS policy root: %v", err) } else { @@ -123,7 +139,7 @@ func (r *registryConfigurator) captureOriginalNameservers() ([]netip.Addr, error seen := make(map[netip.Addr]struct{}) var out []netip.Addr var merr *multierror.Error - for _, root := range []string{interfaceConfigPath, interfaceConfigPathV6} { + for _, root := range []string{InterfaceConfigPath, InterfaceConfigPathV6} { addrs, err := r.captureFromTcpipRoot(root) if err != nil { merr = multierror.Append(merr, fmt.Errorf("%s: %w", root, err)) @@ -496,7 +512,7 @@ func (r *registryConfigurator) deleteInterfaceRegistryKeyProperty(propertyKey st } func (r *registryConfigurator) getInterfaceRegistryKey() (registry.Key, error) { - regKeyPath := interfaceConfigPath + "\\" + r.guid + regKeyPath := InterfaceConfigPath + "\\" + r.guid regKey, err := registry.OpenKey(registry.LOCAL_MACHINE, regKeyPath, registry.SET_VALUE) if err != nil { return regKey, fmt.Errorf("open HKEY_LOCAL_MACHINE\\%s: %w", regKeyPath, err) diff --git a/go.mod b/go.mod index f98073417..f119d4a92 100644 --- a/go.mod +++ b/go.mod @@ -57,6 +57,7 @@ require ( github.com/fsnotify/fsnotify v1.9.0 github.com/gliderlabs/ssh v0.3.8 github.com/go-jose/go-jose/v4 v4.1.4 + github.com/go-ole/go-ole v1.3.0 github.com/gobwas/ws v1.4.0 github.com/goccy/go-yaml v1.18.0 github.com/godbus/dbus/v5 v5.2.2 @@ -199,7 +200,6 @@ require ( github.com/go-ldap/ldap/v3 v3.4.13 // indirect github.com/go-logr/logr v1.4.3 // indirect github.com/go-logr/stdr v1.2.2 // indirect - github.com/go-ole/go-ole v1.3.0 // indirect github.com/go-openapi/analysis v0.23.0 // indirect github.com/go-openapi/errors v0.22.2 // indirect github.com/go-openapi/jsonpointer v0.21.1 // indirect From 85dd335836efb7d170f318222d96199d65529d56 Mon Sep 17 00:00:00 2001 From: Eduard Gert Date: Fri, 14 Aug 2026 10:57:11 +0200 Subject: [PATCH 10/11] [client] Add CI check for translation key parity (#6852) English (en) is the source of truth for UI translation keys; the other nine locales rely on runtime English fallback for any missing key, so a gap never surfaces in CI. Add a dependency-free Node check that fails when any locale declared in _index.json does not carry the exact same key set as en (missing or orphaned keys), wired into a dedicated UI Translations workflow that runs on locale changes. Also close the one existing gap the check found: ja was missing daemon.outdated.download ("Download Latest"). Co-authored-by: Claude Opus 4.8 (1M context) --- .github/workflows/ui-translations.yml | 42 +++++++++++ client/ui/frontend/package.json | 3 +- client/ui/i18n/check-translations.mjs | 104 ++++++++++++++++++++++++++ client/ui/i18n/locales/ja/common.json | 3 + 4 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/ui-translations.yml create mode 100644 client/ui/i18n/check-translations.mjs diff --git a/.github/workflows/ui-translations.yml b/.github/workflows/ui-translations.yml new file mode 100644 index 000000000..7d3b12f2d --- /dev/null +++ b/.github/workflows/ui-translations.yml @@ -0,0 +1,42 @@ +name: UI Translations + +on: + pull_request: + paths: + - "client/ui/i18n/locales/**" + - "client/ui/i18n/check-translations.mjs" + - ".github/workflows/ui-translations.yml" + push: + branches: + - main + paths: + - "client/ui/i18n/locales/**" + - "client/ui/i18n/check-translations.mjs" + +permissions: + contents: read + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }}-${{ github.head_ref || github.actor_id }} + cancel-in-progress: true + +jobs: + check-translations: + name: Check translation key parity + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Checkout repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + persist-credentials: false + + - name: Set up Node.js + uses: actions/setup-node@v4 + with: + node-version: "22" + + # English (en) is the source of truth for translation keys; every other + # locale declared in _index.json must carry the exact same key set. + - name: Check translation key parity + run: node client/ui/i18n/check-translations.mjs diff --git a/client/ui/frontend/package.json b/client/ui/frontend/package.json index 3131b36cd..dcef99ad3 100644 --- a/client/ui/frontend/package.json +++ b/client/ui/frontend/package.json @@ -15,7 +15,8 @@ "lint": "eslint \"src/**/*.{ts,tsx}\"", "lint:fix": "eslint \"src/**/*.{ts,tsx}\" --fix", "check": "pnpm lint && pnpm typecheck && pnpm format:check", - "check:fix": "pnpm lint:fix && pnpm format && pnpm typecheck" + "check:fix": "pnpm lint:fix && pnpm format && pnpm typecheck", + "i18n:check": "node ../i18n/check-translations.mjs" }, "dependencies": { "@radix-ui/react-dialog": "^1.1.15", diff --git a/client/ui/i18n/check-translations.mjs b/client/ui/i18n/check-translations.mjs new file mode 100644 index 000000000..bd076e0e0 --- /dev/null +++ b/client/ui/i18n/check-translations.mjs @@ -0,0 +1,104 @@ +#!/usr/bin/env node +// Validates that every shipped translation bundle carries exactly the same set +// of keys as the English source of truth. English (en) defines the keys; every +// other locale declared in _index.json must match it 1:1: +// +// - no missing keys — a missing key silently falls back to English at runtime +// (see i18n bundle fallback), so the gap never surfaces to users or CI +// without this check; +// - no orphaned keys — keys left behind after an English key is renamed or +// removed are dead weight and a sign the locale is drifting. +// +// Pure Node, no dependencies, so it runs without installing the frontend +// toolchain. +// +// Local: node client/ui/i18n/check-translations.mjs (or: pnpm i18n:check) +// CI: .github/workflows/ui-translations.yml + +import { readdirSync, readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; + +const SOURCE = "en"; +const localesDir = join(dirname(fileURLToPath(import.meta.url)), "locales"); +const isCI = Boolean(process.env.GITHUB_ACTIONS); + +function readJSON(path) { + return JSON.parse(readFileSync(path, "utf8")); +} + +function keysOf(langCode) { + return Object.keys(readJSON(join(localesDir, langCode, "common.json"))); +} + +// Emit a GitHub Actions annotation so failures render inline on the PR diff. +function annotate(file, message) { + if (isCI) console.log(`::error file=${file}::${message}`); +} + +const index = readJSON(join(localesDir, "_index.json")); +const declared = index.languages.map((l) => l.code); + +if (!declared.includes(SOURCE)) { + console.error(`FATAL: source language "${SOURCE}" is not declared in _index.json`); + process.exit(1); +} + +const sourceKeys = keysOf(SOURCE); +const sourceSet = new Set(sourceKeys); +console.log(`Source of truth: ${SOURCE}/common.json — ${sourceKeys.length} keys\n`); + +let failed = false; + +for (const code of declared) { + if (code === SOURCE) continue; + const file = `client/ui/i18n/locales/${code}/common.json`; + + let keys; + try { + keys = keysOf(code); + } catch (e) { + failed = true; + const msg = `bundle is declared in _index.json but common.json is missing or invalid (${e.message})`; + console.error(`✗ ${code}: ${msg}`); + annotate("client/ui/i18n/locales/_index.json", `${code}: ${msg}`); + continue; + } + + const set = new Set(keys); + const missing = sourceKeys.filter((k) => !set.has(k)); + const extra = keys.filter((k) => !sourceSet.has(k)); + + if (missing.length === 0 && extra.length === 0) { + console.log(`✓ ${code}: ${keys.length} keys`); + continue; + } + + failed = true; + console.error(`✗ ${code}: ${keys.length} keys (expected ${sourceKeys.length})`); + if (missing.length) { + console.error(` missing ${missing.length}: ${missing.join(", ")}`); + annotate(file, `Missing ${missing.length} key(s) present in ${SOURCE}: ${missing.join(", ")}`); + } + if (extra.length) { + console.error(` extra ${extra.length}: ${extra.join(", ")}`); + annotate(file, `Has ${extra.length} key(s) not present in ${SOURCE}: ${extra.join(", ")}`); + } +} + +// Locale directories present on disk but not declared in _index.json are never +// loaded by the app — surface them so dead translation files don't rot silently. +const onDisk = readdirSync(localesDir, { withFileTypes: true }) + .filter((e) => e.isDirectory()) + .map((e) => e.name); +const undeclared = onDisk.filter((d) => !declared.includes(d)); +if (undeclared.length) { + console.warn(`\n⚠ locale directories not declared in _index.json (not shipped): ${undeclared.join(", ")}`); +} + +console.log(); +if (failed) { + console.error("Translation check FAILED — every locale must match the English key set."); + process.exit(1); +} +console.log("Translation check passed — all locales match the English key set."); diff --git a/client/ui/i18n/locales/ja/common.json b/client/ui/i18n/locales/ja/common.json index 10cf7598d..ec69de9a5 100644 --- a/client/ui/i18n/locales/ja/common.json +++ b/client/ui/i18n/locales/ja/common.json @@ -1312,6 +1312,9 @@ "daemon.outdated.description": { "message": "このアプリを使用するには NetBird サービスを更新してください。" }, + "daemon.outdated.download": { + "message": "最新版をダウンロード" + }, "error.jwt_clock_skew": { "message": "サインインに失敗しました: このデバイスの時計がサーバーと同期していません。システムの時計を同期してからもう一度お試しください。" }, From 2cfe14d7ec6358f2f76e0f9888fcb2ed53a5c35d Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Fri, 14 Aug 2026 16:13:52 +0000 Subject: [PATCH 11/11] [client] Keep account email on Android logout, drop it on profile removal (#7200) Align Android logout semantics with the desktop UI and CLI: logging out no longer deletes the stored account email, so the next login passes it as the OIDC login_hint and the IdP preselects the account. Removing the profile is now the operation that deletes the email; previously RemoveProfile left the account file behind, which the fixed-name default profile would have inherited on recreation. --- client/android/login.go | 5 +++-- client/android/profile_manager.go | 24 ++++++++++++++++++------ client/android/profile_state.go | 8 ++++---- client/android/profile_state_test.go | 4 ++-- 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/client/android/login.go b/client/android/login.go index 3f367b97f..897b1561e 100644 --- a/client/android/login.go +++ b/client/android/login.go @@ -204,8 +204,9 @@ func (a *Auth) foregroundGetTokenInfo(authClient *auth.Auth, urlOpener URLOpener return nil, fmt.Errorf("failed to get OAuth flow: %v", err) } - // An empty hint is deliberate, not a fallback: a fresh or logged-out profile - // leaves the choice to the IdP, which is how accounts get switched. + // An empty hint is deliberate, not a fallback: a fresh profile leaves the + // choice to the IdP. Switching accounts is done by switching or removing + // profiles, not by logging out — logout keeps the email. if a.cfgPath != "" { if hint := readProfileEmail(a.cfgPath); hint != "" { if setter, ok := oAuthFlow.(loginHintSetter); ok { diff --git a/client/android/profile_manager.go b/client/android/profile_manager.go index 3197124d7..20d585d6a 100644 --- a/client/android/profile_manager.go +++ b/client/android/profile_manager.go @@ -22,7 +22,8 @@ type Profile struct { ID string Name string // Email is the account this profile last logged in with, "" if it never - // completed an SSO login or was logged out. See profile_state.go. + // completed an SSO login. Kept across logouts; cleared when the profile is + // removed. See profile_state.go. Email string IsActive bool } @@ -200,11 +201,9 @@ func (pm *ProfileManager) LogoutProfile(id string) error { return fmt.Errorf("failed to save config: %w", err) } - // Not fatal: a stale hint costs an account switch, not the logout itself. - if err := removeProfileEmail(configPath); err != nil { - log.Warnf("failed to clear stored account email for profile %s: %v", id, err) - } - + // The stored account email is kept on purpose, matching the desktop and CLI + // logout semantics: the next login passes it as the login_hint so the IdP + // preselects the account. Removing the profile is what deletes it. log.Infof("logged out from profile: %s", id) return nil } @@ -224,11 +223,24 @@ func (pm *ProfileManager) RenameProfile(id string, newName string) error { // RemoveProfile deletes a profile func (pm *ProfileManager) RemoveProfile(id string) error { + configPath, err := pm.getProfileConfigPath(id) + if err != nil { + return err + } + // Use ServiceManager (removes profile from profiles/ directory) if err := pm.serviceMgr.RemoveProfile(profilemanager.ID(id), androidUsername); err != nil { return fmt.Errorf("failed to remove profile: %w", err) } + // The account file is this package's, not the ServiceManager's, so it must + // go here. The default profile has a fixed filename, so a recreated one + // would otherwise inherit the deleted profile's email as its login_hint. + // Not fatal: the profile itself is gone. + if err := removeProfileEmail(configPath); err != nil { + log.Warnf("failed to remove stored account email for profile %s: %v", id, err) + } + log.Infof("removed profile: %s", id) return nil } diff --git a/client/android/profile_state.go b/client/android/profile_state.go index 3f0a09701..0063b587f 100644 --- a/client/android/profile_state.go +++ b/client/android/profile_state.go @@ -90,10 +90,10 @@ func writeProfileEmail(configPath string, email string) error { return nil } -// removeProfileEmail drops the stored account email. Called on logout: while the -// email is on disk it goes out as a login_hint, which would steer the next login -// straight back into the account just logged out of. Mirrors the desktop UI's -// RemoveProfileState call. +// removeProfileEmail drops the stored account email. Called on profile removal, +// not on logout: a logged-out profile keeps its email so the next login passes +// it as the login_hint, matching the desktop and CLI semantics. Mirrors the +// desktop UI's RemoveProfileState call. func removeProfileEmail(configPath string) error { accountPath, err := profileAccountPathFor(configPath) if err != nil { diff --git a/client/android/profile_state_test.go b/client/android/profile_state_test.go index 623e16c3b..82a1c2a87 100644 --- a/client/android/profile_state_test.go +++ b/client/android/profile_state_test.go @@ -127,10 +127,10 @@ func TestWriteThenReadProfileEmail(t *testing.T) { t.Fatalf("remove: %v", err) } if got := readProfileEmail(configPath); got != "" { - t.Errorf("expected no email after logout, got %q", got) + t.Errorf("expected no email after removal, got %q", got) } - // Logout may run on a never-logged-in profile, so a second remove must pass. + // Removal may run on a never-logged-in profile, so a second remove must pass. if err := removeProfileEmail(configPath); err != nil { t.Fatalf("second remove should be a no-op: %v", err) }