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/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/.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 diff --git a/client/android/login.go b/client/android/login.go index c099ef759..24c911eb5 100644 --- a/client/android/login.go +++ b/client/android/login.go @@ -201,8 +201,9 @@ func (a *Auth) foregroundGetTokenInfo(authClient *auth.Auth, urlOpener URLOpener } // profileLoginHint returns the stored account email for the profile at cfgPath. -// 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. func profileLoginHint(cfgPath string) string { if cfgPath == "" { return "" 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) } 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/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/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/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/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...) +} 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/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" 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_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/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_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{}), 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/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 + } +} 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 934bcbba6..4ebf8842a 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 ( @@ -105,7 +106,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 } 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/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": "サインインに失敗しました: このデバイスの時計がサーバーと同期していません。システムの時計を同期してからもう一度お試しください。" }, 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/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 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 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"` 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) + }) + } +}