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 3f367b97f..24c911eb5 100644 --- a/client/android/login.go +++ b/client/android/login.go @@ -191,39 +191,49 @@ func (a *Auth) login(urlOpener URLOpener, isAndroidTV bool) error { return nil } -// loginHintSetter is implemented by both concrete flows (PKCE and device code) -// but absent from the OAuthFlow interface, hence the assertion below — the same -// way internal/auth wires it in authenticateWithPKCEFlow. -type loginHintSetter interface { - SetLoginHint(hint string) -} - func (a *Auth) foregroundGetTokenInfo(authClient *auth.Auth, urlOpener URLOpener, isAndroidTV bool) (*auth.TokenInfo, error) { - oAuthFlow, err := authClient.GetOAuthFlow(a.ctx, isAndroidTV) + oAuthFlow, err := authClient.GetOAuthFlow(a.ctx, isAndroidTV, profileLoginHint(a.cfgPath)) if err != nil { return nil, fmt.Errorf("failed to get OAuth flow: %v", err) } - // An empty hint is deliberate, not a fallback: a fresh or logged-out profile - // leaves the choice to the IdP, which is how accounts get switched. - if a.cfgPath != "" { - if hint := readProfileEmail(a.cfgPath); hint != "" { - if setter, ok := oAuthFlow.(loginHintSetter); ok { - setter.SetLoginHint(hint) - } - } + return runOAuthFlow(a.ctx, oAuthFlow, urlOpener, nil) +} + +// profileLoginHint returns the stored account email for the profile at cfgPath. +// 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 "" + } + return readProfileEmail(cfgPath) +} + +// runOAuthFlow drives an already acquired OAuth flow to a token: requests the +// flow info, presents the verification URL through the opener and waits for +// the browser round-trip. Open is called synchronously — it is what marks the +// surface as opened on the client side, and a fast token's OnLoginSuccess is +// a no-op until it has, so the dismissal would be dropped rather than +// delayed. Openers must therefore not block: they post their UI work and +// return. onWaiting, when set, runs after the URL is shown, right before the +// blocking wait. +func runOAuthFlow(ctx context.Context, flow auth.OAuthFlow, urlOpener URLOpener, onWaiting func()) (*auth.TokenInfo, error) { + flowInfo, err := flow.RequestAuthInfo(ctx) + if err != nil { + return nil, fmt.Errorf("request auth info: %w", err) } - flowInfo, err := oAuthFlow.RequestAuthInfo(context.TODO()) - if err != nil { - return nil, fmt.Errorf("getting a request OAuth flow info failed: %v", err) + urlOpener.Open(flowInfo.VerificationURIComplete, flowInfo.UserCode) + + if onWaiting != nil { + onWaiting() } - go urlOpener.Open(flowInfo.VerificationURIComplete, flowInfo.UserCode) - - tokenInfo, err := oAuthFlow.WaitToken(a.ctx, flowInfo) + tokenInfo, err := flow.WaitToken(ctx, flowInfo) if err != nil { - return nil, fmt.Errorf("waiting for browser login failed: %v", err) + return nil, fmt.Errorf("wait for token: %w", err) } return &tokenInfo, nil 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_prefs.go b/client/android/profile_prefs.go new file mode 100644 index 000000000..9c1fd307b --- /dev/null +++ b/client/android/profile_prefs.go @@ -0,0 +1,38 @@ +//go:build android + +package android + +import ( + "fmt" + + "github.com/netbirdio/netbird/client/internal/profilemanager" +) + +type prefsStore interface { + Get(namespace string, v any) (bool, error) + Put(namespace string, v any) error +} + +type profilePrefs struct { + prefs *profilemanager.Prefs +} + +func newProfilePrefs(configDir, profileID string) (*profilePrefs, error) { + if configDir == "" || profileID == "" { + return nil, fmt.Errorf("profile prefs require a config dir and profile ID") + } + pm := NewProfileManager(configDir) + prefs, err := pm.serviceMgr.ProfilePrefs(profilemanager.ID(profileID), androidUsername) + if err != nil { + return nil, fmt.Errorf("resolve profile prefs: %w", err) + } + return &profilePrefs{prefs: prefs}, nil +} + +func (p *profilePrefs) Get(namespace string, v any) (bool, error) { + return p.prefs.Get(namespace, v) +} + +func (p *profilePrefs) Put(namespace string, v any) error { + return p.prefs.Put(namespace, v) +} 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/android/ssh_client.go b/client/android/ssh_client.go new file mode 100644 index 000000000..2822b6539 --- /dev/null +++ b/client/android/ssh_client.go @@ -0,0 +1,649 @@ +//go:build android + +package android + +import ( + "context" + "errors" + "fmt" + "io" + "net" + "strconv" + "strings" + "sync" + "time" + + log "github.com/sirupsen/logrus" + gossh "golang.org/x/crypto/ssh" + + "github.com/netbirdio/netbird/client/internal" + "github.com/netbirdio/netbird/client/internal/auth" + "github.com/netbirdio/netbird/client/internal/profilemanager" + nbssh "github.com/netbirdio/netbird/client/ssh" + "github.com/netbirdio/netbird/client/ssh/detection" +) + +const ( + sshDialTimeout = 30 * time.Second + sshDetectionTimeout = 5 * time.Second +) + +// PasswordRequiredMarker tells Java to prompt for a password and retry. It is +// a string because gomobile flattens errors to their message, so a sentinel +// value would not survive the binding. +const PasswordRequiredMarker = "netbird-ssh-password-required" + +// HostKeyUnknownMarker tells Java to show the fingerprint and, on confirmation, +// retry with TrustHostKey set. The presented fingerprint is appended after the +// marker so the prompt can display it and the retry can guard against a key +// that changed between the two connects. Only regular (non-NetBird) servers +// reach this: NetBird peers verify against the registry. +const HostKeyUnknownMarker = "netbird-ssh-hostkey-unknown" + +var ( + errPasswordRequired = errors.New(PasswordRequiredMarker) + errClientClosed = errors.New("ssh client closed") +) + +// errHostKeyUnknown carries the presented fingerprint so Connect can build the +// marker message the Java side parses. +type errHostKeyUnknown struct { + fingerprint string +} + +func (e *errHostKeyUnknown) Error() string { + return HostKeyUnknownMarker + ":" + e.fingerprint +} + +// SSHTerminalListener receives SSH session events. It is implemented in Java. +// +// All callbacks are invoked from goroutines and may run concurrently with each +// other; the implementation must be safe to call from any thread. +type SSHTerminalListener interface { + OnConnected() + OnData(data []byte) + OnClose(reason string) + OnError(message string) +} + +// SSHClient is a NetBird-aware SSH client exposed to Java via gomobile. +// +// It dials through the running NetBird tunnel and runs a standard SSH session +// on top with PTY enabled. Host-key verification uses the NetBird-provided +// peer SSH host keys, identical to the desktop client. +type SSHClient struct { + nb *Client + mu sync.Mutex + listener SSHTerminalListener + urlOpener URLOpener + + sshClient *gossh.Client + session *gossh.Session + stdin io.WriteCloser + closed bool + + // gen identifies the current connection attempt. Connect and Close bump it, + // so an in-flight dial or a reader left over from a previous connection + // finds itself stale and stays silent instead of publishing OnConnected or + // OnClose for a connection the caller already abandoned. + gen uint64 + dialCancel context.CancelFunc + + // knownHostsConfigDir and knownHostsProfile locate the TOFU store for + // regular SSH servers in the profile's preferences. Java supplies them, + // since an overlay IP is a different host under a different profile. Empty + // until set: without them a regular server cannot be verified and Connect + // refuses one. + knownHostsConfigDir string + knownHostsProfile string + // trustHostKey carries the fingerprint the user confirmed on a previous + // attempt, so the retry accepts exactly that key and persists it. + trustHostKey string +} + +// NewSSHClient creates a new SSH client bound to the running NetBird Client. +func NewSSHClient(c *Client) *SSHClient { + return &SSHClient{nb: c} +} + +// SetListener registers the Java listener. Must be called before Connect to +// receive any events. +func (s *SSHClient) SetListener(l SSHTerminalListener) { + s.mu.Lock() + s.listener = l + s.mu.Unlock() +} + +// SetURLOpener registers the Java URL opener used to display the device-code +// authorization page in a Custom Tabs window when the target peer requires +// JWT authentication. Must be set before Connect to be effective. +func (s *SSHClient) SetURLOpener(opener URLOpener) { + s.mu.Lock() + s.urlOpener = opener + s.mu.Unlock() +} + +// SetKnownHostsStore points the TOFU host-key store at a profile's preferences. +// Must be set before connecting to a regular SSH server; without it such a +// server cannot be verified and Connect refuses one. +func (s *SSHClient) SetKnownHostsStore(configDir, profileID string) { + s.mu.Lock() + s.knownHostsConfigDir = configDir + s.knownHostsProfile = profileID + s.mu.Unlock() +} + +// TrustHostKey records the fingerprint the user confirmed for a regular server, +// so the next Connect accepts that exact key and adds it to the known-hosts +// store. Passing a fingerprint that no longer matches makes the connect fail +// rather than trust a key that changed since the prompt. +func (s *SSHClient) TrustHostKey(fingerprint string) { + s.mu.Lock() + s.trustHostKey = fingerprint + s.mu.Unlock() +} + +// Connect dials the SSH server through the NetBird tunnel and performs the +// SSH handshake. It auto-detects the server type via SSH banner inspection +// and selects the appropriate authentication path: +// +// - NetBird-SSH server requiring JWT: launches the OAuth 2.0 device-code +// flow, opens the verification URL through the registered URLOpener, and +// uses the resulting token as the SSH password. Host-key verification +// uses the NetBird peer registry. +// - NetBird-SSH server without JWT: authenticates with the NetBird SSH +// private key. Host-key verification uses the NetBird peer registry. +// - Regular SSH server (e.g. OpenSSH): authenticates with the NetBird key +// first (so a user-installed NetBird public key works), then falls back +// to the supplied password if non-empty. Host-key verification is +// trust-on-first-use against the per-profile known-hosts store. +// +// The password parameter is only consulted for regular SSH servers. +func (s *SSHClient) Connect(host string, port int, user, password string) error { + if port < 1 || port > 65535 { + return fmt.Errorf("invalid port: %d", port) + } + + cfg, cfgPath, cc := s.nb.authSnapshot() + if cc == nil { + return errors.New("netbird client not running") + } + if cfg == nil { + return errors.New("netbird config not loaded") + } + engine := cc.Engine() + if engine == nil { + return errors.New("netbird engine not available") + } + + s.mu.Lock() + s.gen++ + gen := s.gen + s.mu.Unlock() + + serverType := detectServerType(host, port) + log.Debugf("SSH server type: %s", serverType) + + authMethods, hostKeyCallback, err := s.buildAuth(cfg, cfgPath, engine, serverType, password) + if err != nil { + return err + } + + clientConfig := &gossh.ClientConfig{ + User: user, + Auth: authMethods, + HostKeyCallback: hostKeyCallback, + Timeout: sshDialTimeout, + } + err = s.dialAndHandshake(gen, host, port, clientConfig) + + // An unknown host key is a prompt, not a failure: return the marker intact + // (rootCause would unwrap it) so Java can show the fingerprint and retry. + var unknownHost *errHostKeyUnknown + if errors.As(err, &unknownHost) { + return errors.New(unknownHost.Error()) + } + + // A regular server may still accept a password, so let the caller ask for + // one instead of failing. NetBird servers never use a password, so a + // failure there is genuine. + if err != nil && serverType != detection.ServerTypeNetBirdJWT && + serverType != detection.ServerTypeNetBirdNoJWT && isAuthFailure(err) && + passwordCouldHelp(err, password != "") { + return errPasswordRequired + } + if err != nil { + return rootCause(err) + } + return nil +} + +// StartSession requests a PTY and starts an interactive shell. Output from +// the session is forwarded to the listener via OnData. +func (s *SSHClient) StartSession(cols, rows int) error { + err := s.startSession(cols, rows) + if err != nil { + log.Infof("SSH: start session failed: %v", err) + return rootCause(err) + } + return nil +} + +// Write sends data to the SSH session stdin. +func (s *SSHClient) Write(data []byte) error { + s.mu.Lock() + stdin := s.stdin + s.mu.Unlock() + if stdin == nil { + return errors.New("ssh session not started") + } + if _, err := stdin.Write(data); err != nil { + return fmt.Errorf("write stdin: %w", err) + } + return nil +} + +// Resize updates the PTY window size. +func (s *SSHClient) Resize(cols, rows int) error { + s.mu.Lock() + session := s.session + s.mu.Unlock() + if session == nil { + return errors.New("ssh session not started") + } + return session.WindowChange(rows, cols) +} + +// Reset makes a closed client usable for another Connect: Close leaves the +// one-shot guard set, and clearing it lets the same client back a reconnect. +func (s *SSHClient) Reset() { + s.mu.Lock() + defer s.mu.Unlock() + s.closed = false +} + +// Close terminates the SSH session and underlying connection. Safe to call +// multiple times. +func (s *SSHClient) Close() error { + s.mu.Lock() + s.gen++ + if s.dialCancel != nil { + s.dialCancel() + s.dialCancel = nil + } + sshClient := s.sshClient + session := s.session + stdin := s.stdin + s.sshClient = nil + s.session = nil + s.stdin = nil + notify := !s.closed + s.closed = true + listener := s.listener + s.mu.Unlock() + + if stdin != nil { + if err := stdin.Close(); err != nil { + log.Debugf("ssh: stdin close: %v", err) + } + } + if session != nil { + if err := session.Close(); err != nil && !errors.Is(err, io.EOF) { + log.Debugf("ssh: session close: %v", err) + } + } + var firstErr error + if sshClient != nil { + if err := sshClient.Close(); err != nil { + firstErr = err + } + } + if notify && listener != nil { + listener.OnClose("closed by client") + } + return firstErr +} + +func (s *SSHClient) startSession(cols, rows int) error { + log.Debugf("SSH: starting session %dx%d", cols, rows) + s.mu.Lock() + sshClient := s.sshClient + gen := s.gen + s.mu.Unlock() + + if sshClient == nil { + return errors.New("ssh client not connected") + } + + pty, err := nbssh.StartPTYSession(sshClient, cols, rows) + if err != nil { + return err + } + + s.mu.Lock() + if gen != s.gen { + s.mu.Unlock() + closeQuiet(pty.Session, "stale session") + return errClientClosed + } + s.session = pty.Session + s.stdin = pty.Stdin + s.mu.Unlock() + + readerDone := make(chan string, 2) + go func() { readerDone <- s.readLoop(pty.Stdout, "stdout") }() + go func() { readerDone <- s.readLoop(pty.Stderr, "stderr") }() + go func() { + reason := <-readerDone + if second := <-readerDone; reason == "" { + reason = second + } + s.notifyClose(gen, reason) + }() + log.Debug("SSH: session started, shell running") + return nil +} + +func (s *SSHClient) buildAuth(cfg *profilemanager.Config, cfgPath string, engine *internal.Engine, + serverType detection.ServerType, password string) ([]gossh.AuthMethod, gossh.HostKeyCallback, error) { + + switch serverType { + case detection.ServerTypeNetBirdJWT: + token, err := s.requestJWTToken(cfg, cfgPath) + if err != nil { + return nil, nil, fmt.Errorf("jwt: %w", err) + } + auths := []gossh.AuthMethod{gossh.Password(token)} + return auths, nbssh.CreateHostKeyCallback(nbssh.PeerKeyLookup(engine.GetPeerSSHKey)), nil + + case detection.ServerTypeNetBirdNoJWT: + if cfg.SSHKey == "" { + return nil, nil, errors.New("no NetBird SSH key available") + } + signer, err := gossh.ParsePrivateKey([]byte(cfg.SSHKey)) + if err != nil { + return nil, nil, fmt.Errorf("parse netbird ssh key: %w", err) + } + auths := []gossh.AuthMethod{gossh.PublicKeys(signer)} + return auths, nbssh.CreateHostKeyCallback(nbssh.PeerKeyLookup(engine.GetPeerSSHKey)), nil + + case detection.ServerTypeRegular: + var auths []gossh.AuthMethod + if cfg.SSHKey != "" { + if signer, err := gossh.ParsePrivateKey([]byte(cfg.SSHKey)); err == nil { + auths = append(auths, gossh.PublicKeys(signer)) + } else { + log.Debugf("ssh: parse netbird key for regular auth: %v", err) + } + } + if password != "" { + pw := password + auths = append(auths, gossh.Password(pw)) + auths = append(auths, gossh.KeyboardInteractive(func(_, _ string, questions []string, _ []bool) ([]string, error) { + answers := make([]string, len(questions)) + for i := range questions { + answers[i] = pw + } + return answers, nil + })) + } + if len(auths) == 0 { + // Nothing to offer at all: ask for a password rather than failing, + // so the caller can retry once the user supplies one. + return nil, nil, errPasswordRequired + } + callback, err := s.tofuHostKeyCallback() + if err != nil { + return nil, nil, err + } + return auths, callback, nil + + default: + return nil, nil, fmt.Errorf("unsupported SSH server type: %v", serverType) + } +} + +// tofuHostKeyCallback verifies a regular server's host key against the +// per-profile known-hosts store. An unknown host returns errHostKeyUnknown so +// Java can show the fingerprint and, once confirmed, retry with the key +// trusted; a changed key is rejected outright, as OpenSSH does. When the user +// has confirmed a fingerprint, the callback accepts exactly that key and +// appends it to the store. +func (s *SSHClient) tofuHostKeyCallback() (gossh.HostKeyCallback, error) { + s.mu.Lock() + configDir := s.knownHostsConfigDir + profileID := s.knownHostsProfile + trusted := s.trustHostKey + s.mu.Unlock() + + if configDir == "" || profileID == "" { + return nil, errors.New("no known-hosts store configured for regular SSH") + } + + store, err := openKnownHostsStore(configDir, profileID) + if err != nil { + return nil, fmt.Errorf("load known-hosts store: %w", err) + } + + return func(hostname string, remote net.Addr, key gossh.PublicKey) error { + verdict, err := store.verify(hostname, remote, key) + if err != nil { + return err + } + if verdict == hostKeyMatched { + return nil + } + if verdict == hostKeyChanged { + return fmt.Errorf("SSH host key changed for %s (possible attack)", hostname) + } + + fingerprint := gossh.FingerprintSHA256(key) + if trusted == "" { + return &errHostKeyUnknown{fingerprint: fingerprint} + } + if trusted != fingerprint { + return fmt.Errorf("SSH host key changed since it was confirmed for %s", hostname) + } + if err := store.append(hostname, remote, key); err != nil { + return fmt.Errorf("persist trusted host key: %w", err) + } + // The confirmation is spent: now that the key is stored, a later + // reconnect must verify against the file, not re-accept this fingerprint. + s.mu.Lock() + s.trustHostKey = "" + s.mu.Unlock() + return nil + }, nil +} + +func (s *SSHClient) requestJWTToken(cfg *profilemanager.Config, cfgPath string) (string, error) { + s.mu.Lock() + urlOpener := s.urlOpener + s.mu.Unlock() + if urlOpener == nil { + return "", errors.New("URL opener not configured for JWT auth") + } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + flow, err := auth.NewOAuthFlow(ctx, cfg, false, true, profileLoginHint(cfgPath)) + if err != nil { + return "", fmt.Errorf("create oauth flow: %w", err) + } + + // The status callback covers the browser round-trip, which would + // otherwise leave the terminal blank. + tokenInfo, err := runOAuthFlow(ctx, flow, urlOpener, func() { + s.notifyStatus("Waiting for browser authentication...") + }) + if err != nil { + return "", err + } + + token := tokenInfo.GetTokenToUse() + if token == "" { + return "", errors.New("empty token returned by IdP") + } + + // Tells the client the browser round-trip is over so it can dismiss the + // surface it opened, the same way the login and session-extend flows do. + // Without it the Custom Tab stays in front of the terminal even though the + // token has already been collected. + urlOpener.OnLoginSuccess() + + return token, nil +} + +func (s *SSHClient) dialAndHandshake(gen uint64, host string, port int, clientConfig *gossh.ClientConfig) error { + addr := net.JoinHostPort(host, strconv.Itoa(port)) + ctx, cancel := context.WithTimeout(context.Background(), sshDialTimeout) + defer cancel() + + s.mu.Lock() + if gen != s.gen { + s.mu.Unlock() + return errClientClosed + } + s.dialCancel = cancel + s.mu.Unlock() + + var dialer net.Dialer + conn, err := dialer.DialContext(ctx, "tcp", addr) + if err != nil { + return fmt.Errorf("dial %s: %w", addr, err) + } + + client, err := nbssh.Handshake(ctx, conn, addr, clientConfig) + if err != nil { + return err + } + + s.mu.Lock() + if gen != s.gen { + s.mu.Unlock() + closeQuiet(client, "stale ssh client") + return errClientClosed + } + s.sshClient = client + listener := s.listener + s.mu.Unlock() + + if listener != nil { + listener.OnConnected() + } + return nil +} + +func (s *SSHClient) readLoop(r io.Reader, name string) string { + buf := make([]byte, 4096) + for { + n, err := r.Read(buf) + if n > 0 { + s.mu.Lock() + listener := s.listener + s.mu.Unlock() + if listener != nil { + chunk := make([]byte, n) + copy(chunk, buf[:n]) + listener.OnData(chunk) + } + } + if err != nil { + // EOF is a normal shell exit, so report it without a reason. + if errors.Is(err, io.EOF) { + return "" + } + log.Debugf("ssh %s read: %v", name, err) + return rootCause(err).Error() + } + } +} + +// notifyStatus writes a progress line to the terminal through the normal +// output path, so long steps are visible while nothing else is arriving. +func (s *SSHClient) notifyStatus(text string) { + s.mu.Lock() + listener := s.listener + s.mu.Unlock() + if listener != nil { + listener.OnData([]byte("\r\n\x1b[33m" + text + "\x1b[0m\r\n")) + } +} + +func (s *SSHClient) notifyClose(gen uint64, reason string) { + s.mu.Lock() + if gen != s.gen || s.closed { + s.mu.Unlock() + return + } + s.closed = true + listener := s.listener + s.mu.Unlock() + if listener != nil { + listener.OnClose(reason) + } +} + +func closeQuiet(c io.Closer, label string) { + if c == nil { + return + } + if err := c.Close(); err != nil && !errors.Is(err, io.EOF) { + log.Debugf("ssh: close %s: %v", label, err) + } +} + +func detectServerType(host string, port int) detection.ServerType { + ctx, cancel := context.WithTimeout(context.Background(), sshDetectionTimeout) + defer cancel() + + dialer := &net.Dialer{} + serverType, err := detection.DetectSSHServerType(ctx, dialer, host, port) + if err != nil { + log.Debugf("ssh: server detection failed: %v (assuming regular SSH)", err) + return detection.ServerTypeRegular + } + return serverType +} + +// rootCause returns the innermost error of a %w chain, so the terminal shows +// "i/o timeout" rather than every layer that added context on the way up. +func rootCause(err error) error { + for { + // A joined error has no single root, so keep it as-is. + if _, ok := err.(interface{ Unwrap() []error }); ok { + return err + } + next := errors.Unwrap(err) + if next == nil { + return err + } + err = next + } +} + +// isAuthFailure distinguishes credential rejection from dial, timeout and +// host-key errors, which retrying with a password would not fix. +func isAuthFailure(err error) bool { + if errors.Is(err, errPasswordRequired) { + return true + } + var partial *gossh.PartialSuccessError + if errors.As(err, &partial) { + return true + } + return strings.Contains(err.Error(), "unable to authenticate") +} + +// passwordCouldHelp reports whether prompting for a password again can change +// the outcome. gossh lists a method under "attempted methods" only when the +// server offered it, so a supplied password that was never attempted means the +// server does not accept passwords and the real error should surface instead. +func passwordCouldHelp(err error, passwordOffered bool) bool { + if !passwordOffered { + return true + } + msg := err.Error() + return strings.Contains(msg, "password") || strings.Contains(msg, "keyboard-interactive") +} diff --git a/client/android/ssh_known_hosts.go b/client/android/ssh_known_hosts.go new file mode 100644 index 000000000..eea90fd32 --- /dev/null +++ b/client/android/ssh_known_hosts.go @@ -0,0 +1,168 @@ +//go:build android + +package android + +import ( + "bytes" + "net" + "strconv" + "strings" + "sync" + + gossh "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/knownhosts" +) + +const knownHostsNamespace = "ssh" + +const ( + hostKeyUnknown hostKeyVerdict = iota + hostKeyMatched + hostKeyChanged +) + +var knownHostsMu sync.Mutex + +type hostKeyVerdict uint8 + +type knownHostsSection struct { + KnownHosts []string `json:"knownHosts"` +} + +type knownHostsStore struct { + prefs prefsStore +} + +// RemoveKnownHost deletes every known-hosts entry for host:port from the +// profile's store, so a host trusted for a session that is being deleted does +// not linger. Java calls this only once no session targets that host, so a +// shared host stays trusted. A missing entry is not an error: the goal state +// is "absent". +func RemoveKnownHost(configDir, profileID, host string, port int) error { + store, err := openKnownHostsStore(configDir, profileID) + if err != nil { + return err + } + return store.removeHost(host, port) +} + +func openKnownHostsStore(configDir, profileID string) (*knownHostsStore, error) { + prefs, err := newProfilePrefs(configDir, profileID) + if err != nil { + return nil, err + } + return &knownHostsStore{prefs: prefs}, nil +} + +func (st *knownHostsStore) verify(hostname string, remote net.Addr, key gossh.PublicKey) (hostKeyVerdict, error) { + lines, err := st.lines() + if err != nil { + return hostKeyUnknown, err + } + targets := knownHostsTargets(hostname, remote) + + verdict := hostKeyUnknown + for _, line := range lines { + pubKey, ok := knownHostsLineKey(line, targets) + if !ok { + continue + } + if pubKey.Type() == key.Type() && bytes.Equal(pubKey.Marshal(), key.Marshal()) { + return hostKeyMatched, nil + } + verdict = hostKeyChanged + } + return verdict, nil +} + +func (st *knownHostsStore) append(hostname string, remote net.Addr, key gossh.PublicKey) error { + line := knownhosts.Line(knownHostsTargets(hostname, remote), key) + + knownHostsMu.Lock() + defer knownHostsMu.Unlock() + + lines, err := st.lines() + if err != nil { + return err + } + return st.prefs.Put(knownHostsNamespace, knownHostsSection{KnownHosts: append(lines, line)}) +} + +func (st *knownHostsStore) removeHost(host string, port int) error { + target := knownhosts.Normalize(net.JoinHostPort(host, strconv.Itoa(port))) + + knownHostsMu.Lock() + defer knownHostsMu.Unlock() + + lines, err := st.lines() + if err != nil { + return err + } + kept := make([]string, 0, len(lines)) + for _, line := range lines { + if knownHostsLineMatches(line, target) { + continue + } + kept = append(kept, line) + } + if len(kept) == len(lines) { + return nil + } + return st.prefs.Put(knownHostsNamespace, knownHostsSection{KnownHosts: kept}) +} + +func (st *knownHostsStore) lines() ([]string, error) { + var section knownHostsSection + if _, err := st.prefs.Get(knownHostsNamespace, §ion); err != nil { + return nil, err + } + return section.KnownHosts, nil +} + +func knownHostsTargets(hostname string, remote net.Addr) []string { + targets := []string{knownhosts.Normalize(hostname)} + if remote != nil { + if normalized := knownhosts.Normalize(remote.String()); normalized != targets[0] { + targets = append(targets, normalized) + } + } + return targets +} + +func knownHostsLineKey(line string, targets []string) (gossh.PublicKey, bool) { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + return nil, false + } + _, hosts, pubKey, _, _, err := gossh.ParseKnownHosts([]byte(trimmed)) + if err != nil { + return nil, false + } + for _, host := range hosts { + for _, target := range targets { + if host == target { + return pubKey, true + } + } + } + return nil, false +} + +// knownHostsLineMatches reports whether a known-hosts line's address list +// contains the normalized target. Comment and blank lines never match. +func knownHostsLineMatches(line, target string) bool { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + return false + } + fields := strings.Fields(trimmed) + if len(fields) == 0 { + return false + } + for _, addr := range strings.Split(fields[0], ",") { + if addr == target { + return true + } + } + return false +} diff --git a/client/android/ssh_sessions.go b/client/android/ssh_sessions.go new file mode 100644 index 000000000..44b5464e9 --- /dev/null +++ b/client/android/ssh_sessions.go @@ -0,0 +1,104 @@ +//go:build android + +package android + +const ( + sshSessionsNamespace = "ssh-sessions" + maxStoredSSHSessions = 50 +) + +type sshSessionRecord struct { + ID string `json:"id"` + Host string `json:"host"` + Port int `json:"port"` + User string `json:"user"` +} + +type sshSessionsSection struct { + Sessions []sshSessionRecord `json:"sessions"` +} + +// SSHSessionEntry is one stored SSH session, without any credential. +type SSHSessionEntry struct { + ID string + Host string + Port int + User string +} + +// SSHSessionArray wraps stored SSH sessions for gomobile compatibility. +type SSHSessionArray struct { + items []*SSHSessionEntry +} + +// NewSSHSessionArray creates an empty session array to fill via Add. +func NewSSHSessionArray() *SSHSessionArray { + return &SSHSessionArray{} +} + +// Add appends a session entry, oldest first. +func (a *SSHSessionArray) Add(id, host string, port int, user string) { + a.items = append(a.items, &SSHSessionEntry{ID: id, Host: host, Port: port, User: user}) +} + +// Length returns the number of entries. +func (a *SSHSessionArray) Length() int { + return len(a.items) +} + +// Get returns the entry at index i, or nil when out of range. +func (a *SSHSessionArray) Get(i int) *SSHSessionEntry { + if i < 0 || i >= len(a.items) { + return nil + } + return a.items[i] +} + +// SSHSessionStore reads and writes a profile's stored SSH sessions. +type SSHSessionStore struct { + prefs prefsStore +} + +// NewSSHSessionStore opens the session store of the given profile. +func NewSSHSessionStore(configDir, profileID string) (*SSHSessionStore, error) { + prefs, err := newProfilePrefs(configDir, profileID) + if err != nil { + return nil, err + } + return &SSHSessionStore{prefs: prefs}, nil +} + +// Load returns the stored sessions, oldest first. +func (s *SSHSessionStore) Load() (*SSHSessionArray, error) { + var section sshSessionsSection + if _, err := s.prefs.Get(sshSessionsNamespace, §ion); err != nil { + return nil, err + } + + out := NewSSHSessionArray() + for _, record := range section.Sessions { + if record.ID == "" || record.Host == "" { + continue + } + out.Add(record.ID, record.Host, record.Port, record.User) + } + return out, nil +} + +// Save replaces the stored sessions, keeping only the newest entries when the +// list exceeds the storage cap. +func (s *SSHSessionStore) Save(sessions *SSHSessionArray) error { + var items []*SSHSessionEntry + if sessions != nil { + items = sessions.items + } + if len(items) > maxStoredSSHSessions { + items = items[len(items)-maxStoredSSHSessions:] + } + + records := make([]sshSessionRecord, 0, len(items)) + for _, item := range items { + records = append(records, sshSessionRecord{ID: item.ID, Host: item.Host, Port: item.Port, User: item.User}) + } + return s.prefs.Put(sshSessionsNamespace, sshSessionsSection{Sessions: records}) +} 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/embed/embed.go b/client/embed/embed.go index 99a6b8229..1b2d84d7e 100644 --- a/client/embed/embed.go +++ b/client/embed/embed.go @@ -21,7 +21,7 @@ import ( "github.com/netbirdio/netbird/client/internal/auth" "github.com/netbirdio/netbird/client/internal/peer" "github.com/netbirdio/netbird/client/internal/profilemanager" - sshcommon "github.com/netbirdio/netbird/client/ssh" + nbssh "github.com/netbirdio/netbird/client/ssh" "github.com/netbirdio/netbird/client/system" "github.com/netbirdio/netbird/shared/management/domain" mgmProto "github.com/netbirdio/netbird/shared/management/proto" @@ -521,12 +521,7 @@ func (c *Client) VerifySSHHostKey(peerAddress string, key []byte) error { return err } - storedKey, found := engine.GetPeerSSHKey(peerAddress) - if !found { - return sshcommon.ErrPeerNotFound - } - - return sshcommon.VerifyHostKey(storedKey, key, peerAddress) + return nbssh.PeerKeyLookup(engine.GetPeerSSHKey).VerifySSHHostKey(peerAddress, key) } // SetPerformance retunes a running Client. Only PreallocatedBuffersPerPool 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/auth/auth.go b/client/internal/auth/auth.go index 153727a6c..b3a9e1158 100644 --- a/client/internal/auth/auth.go +++ b/client/internal/auth/auth.go @@ -138,26 +138,37 @@ func (a *Auth) IsSSOSupported(ctx context.Context) (bool, error) { // GetOAuthFlow returns an OAuth flow (PKCE or Device) using the existing management connection // This avoids creating a new connection to the management server -func (a *Auth) GetOAuthFlow(ctx context.Context, forceDeviceAuth bool) (OAuthFlow, error) { +func (a *Auth) GetOAuthFlow(ctx context.Context, forceDeviceAuth bool, hint string) (OAuthFlow, error) { var flow OAuthFlow - var err error - err = a.withRetry(ctx, func(client *mgm.GrpcClient) error { + err := a.withRetry(ctx, func(client *mgm.GrpcClient) error { if forceDeviceAuth { - flow, err = a.getDeviceFlow(client) - return err + deviceFlow, err := a.getDeviceFlow(client) + if err != nil { + return err + } + deviceFlow.SetLoginHint(hint) + flow = deviceFlow + return nil } // Try PKCE flow first - flow, err = a.getPKCEFlow(client) + pkceFlow, err := a.getPKCEFlow(client) if err != nil { // If PKCE not supported, try Device flow if s, ok := status.FromError(err); ok && (s.Code() == codes.NotFound || s.Code() == codes.Unimplemented) { - flow, err = a.getDeviceFlow(client) - return err + deviceFlow, err := a.getDeviceFlow(client) + if err != nil { + return err + } + deviceFlow.SetLoginHint(hint) + flow = deviceFlow + return nil } return err } + pkceFlow.SetLoginHint(hint) + flow = pkceFlow return nil }) diff --git a/client/internal/auth/oauth.go b/client/internal/auth/oauth.go index a50a2ce6f..91329c98b 100644 --- a/client/internal/auth/oauth.go +++ b/client/internal/auth/oauth.go @@ -97,9 +97,7 @@ func authenticateWithPKCEFlow(ctx context.Context, config *profilemanager.Config return nil, fmt.Errorf("getting pkce authorization flow info failed with error: %v", err) } - if hint != "" { - pkceFlowInfo.SetLoginHint(hint) - } + pkceFlowInfo.SetLoginHint(hint) return pkceFlowInfo, nil } @@ -127,9 +125,7 @@ func authenticateWithDeviceCodeFlow(ctx context.Context, config *profilemanager. } } - if hint != "" { - deviceFlowInfo.SetLoginHint(hint) - } + deviceFlowInfo.SetLoginHint(hint) return deviceFlowInfo, nil } 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/profilemanager/prefs.go b/client/internal/profilemanager/prefs.go new file mode 100644 index 000000000..5613b0be3 --- /dev/null +++ b/client/internal/profilemanager/prefs.go @@ -0,0 +1,130 @@ +package profilemanager + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "sync" + + "github.com/netbirdio/netbird/util" +) + +const prefsFileSuffix = ".prefs.json" + +var prefsMu sync.Mutex + +// Prefs is a namespaced per-profile preference store backed by a single JSON +// file next to the profile config; it is deleted together with the profile. +type Prefs struct { + path string +} + +// ProfilePrefs returns the preference store of the profile identified by id. +func (s *ServiceManager) ProfilePrefs(id ID, username string) (*Prefs, error) { + if !IsValidProfileFilenameStem(id) { + return nil, fmt.Errorf("invalid profile ID: %q", id) + } + if id == defaultProfileName { + return &Prefs{path: filepath.Join(filepath.Dir(DefaultConfigPath), id.String()+prefsFileSuffix)}, nil + } + configDir, err := s.getConfigDir(username) + if err != nil { + return nil, fmt.Errorf("get config directory for user %s: %w", username, err) + } + return &Prefs{path: filepath.Join(configDir, id.String()+prefsFileSuffix)}, nil +} + +// Get unmarshals the namespace section into v and reports whether it exists. +func (p *Prefs) Get(namespace string, v any) (bool, error) { + if namespace == "" { + return false, fmt.Errorf("empty prefs namespace") + } + + prefsMu.Lock() + defer prefsMu.Unlock() + + sections, err := readPrefsFile(p.path) + if err != nil { + return false, err + } + raw, ok := sections[namespace] + if !ok { + return false, nil + } + if err := json.Unmarshal(raw, v); err != nil { + return false, fmt.Errorf("decode prefs namespace %q: %w", namespace, err) + } + return true, nil +} + +// Put stores v as the namespace section, replacing any previous value. +func (p *Prefs) Put(namespace string, v any) error { + if namespace == "" { + return fmt.Errorf("empty prefs namespace") + } + raw, err := json.Marshal(v) + if err != nil { + return fmt.Errorf("encode prefs namespace %q: %w", namespace, err) + } + + prefsMu.Lock() + defer prefsMu.Unlock() + + sections, err := readPrefsFile(p.path) + if err != nil { + return err + } + sections[namespace] = raw + return writePrefsFile(p.path, sections) +} + +// Remove deletes the namespace section; a missing one is not an error. +func (p *Prefs) Remove(namespace string) error { + if namespace == "" { + return fmt.Errorf("empty prefs namespace") + } + + prefsMu.Lock() + defer prefsMu.Unlock() + + sections, err := readPrefsFile(p.path) + if err != nil { + return err + } + if _, ok := sections[namespace]; !ok { + return nil + } + delete(sections, namespace) + return writePrefsFile(p.path, sections) +} + +func removePrefsFile(path string) error { + prefsMu.Lock() + defer prefsMu.Unlock() + return os.Remove(path) +} + +func readPrefsFile(path string) (map[string]json.RawMessage, error) { + data, err := os.ReadFile(path) + if os.IsNotExist(err) { + return map[string]json.RawMessage{}, nil + } + if err != nil { + return nil, fmt.Errorf("read prefs: %w", err) + } + + sections := map[string]json.RawMessage{} + if err := json.Unmarshal(data, §ions); err != nil { + return nil, fmt.Errorf("decode prefs: %w", err) + } + return sections, nil +} + +func writePrefsFile(path string, sections map[string]json.RawMessage) error { + if err := util.WriteJsonWithRestrictedPermission(context.Background(), path, sections); err != nil { + return fmt.Errorf("write prefs: %w", err) + } + return nil +} diff --git a/client/internal/profilemanager/prefs_test.go b/client/internal/profilemanager/prefs_test.go new file mode 100644 index 000000000..692ade70f --- /dev/null +++ b/client/internal/profilemanager/prefs_test.go @@ -0,0 +1,138 @@ +package profilemanager + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type testPrefsSection struct { + Mode uint8 `json:"mode"` + Dest string `json:"dest"` +} + +func TestProfilePrefs_RoundTrip(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, username string) { + created, err := sm.AddProfile("work", username) + require.NoError(t, err) + + prefs, err := sm.ProfilePrefs(created.ID, username) + require.NoError(t, err) + + require.NoError(t, prefs.Put("filedrop", testPrefsSection{Mode: 2, Dest: "/tmp/x"})) + require.NoError(t, prefs.Put("other", map[string]int{"n": 1})) + + var got testPrefsSection + found, err := prefs.Get("filedrop", &got) + require.NoError(t, err) + assert.True(t, found) + assert.Equal(t, testPrefsSection{Mode: 2, Dest: "/tmp/x"}, got) + + var other map[string]int + found, err = prefs.Get("other", &other) + require.NoError(t, err) + assert.True(t, found) + assert.Equal(t, map[string]int{"n": 1}, other) + }) +} + +func TestProfilePrefs_GetMissingNamespace(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, username string) { + created, err := sm.AddProfile("work", username) + require.NoError(t, err) + + prefs, err := sm.ProfilePrefs(created.ID, username) + require.NoError(t, err) + + var got testPrefsSection + found, err := prefs.Get("filedrop", &got) + require.NoError(t, err) + assert.False(t, found) + }) +} + +func TestProfilePrefs_RemoveNamespace(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, username string) { + created, err := sm.AddProfile("work", username) + require.NoError(t, err) + + prefs, err := sm.ProfilePrefs(created.ID, username) + require.NoError(t, err) + + require.NoError(t, prefs.Put("filedrop", testPrefsSection{Mode: 1})) + require.NoError(t, prefs.Put("other", map[string]int{"n": 1})) + require.NoError(t, prefs.Remove("filedrop")) + require.NoError(t, prefs.Remove("missing")) + + var got testPrefsSection + found, err := prefs.Get("filedrop", &got) + require.NoError(t, err) + assert.False(t, found) + + var other map[string]int + found, err = prefs.Get("other", &other) + require.NoError(t, err) + assert.True(t, found) + assert.Equal(t, map[string]int{"n": 1}, other) + }) +} + +func TestProfilePrefs_RejectsInvalidID(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, username string) { + _, err := sm.ProfilePrefs("../escape", username) + assert.Error(t, err) + }) +} + +func TestProfilePrefs_RejectsEmptyNamespace(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, username string) { + created, err := sm.AddProfile("work", username) + require.NoError(t, err) + + prefs, err := sm.ProfilePrefs(created.ID, username) + require.NoError(t, err) + + _, err = prefs.Get("", &testPrefsSection{}) + assert.Error(t, err) + assert.Error(t, prefs.Put("", testPrefsSection{})) + assert.Error(t, prefs.Remove("")) + }) +} + +func TestProfilePrefs_DefaultProfile(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, username string) { + prefs, err := sm.ProfilePrefs(defaultProfileName, username) + require.NoError(t, err) + + require.NoError(t, prefs.Put("filedrop", testPrefsSection{Mode: 1})) + + expected := filepath.Join(filepath.Dir(DefaultConfigPath), "default"+prefsFileSuffix) + _, err = os.Stat(expected) + require.NoError(t, err) + }) +} + +func TestRemoveProfile_DeletesPrefsFile(t *testing.T) { + withTestSM(t, func(sm *ServiceManager, username string) { + created, err := sm.AddProfile("work", username) + require.NoError(t, err) + + prefs, err := sm.ProfilePrefs(created.ID, username) + require.NoError(t, err) + require.NoError(t, prefs.Put("filedrop", testPrefsSection{Mode: 2})) + + configDir, err := sm.getConfigDir(username) + require.NoError(t, err) + prefsPath := filepath.Join(configDir, created.ID.String()+prefsFileSuffix) + _, err = os.Stat(prefsPath) + require.NoError(t, err) + + require.NoError(t, sm.RemoveProfile(created.ID, username)) + _, err = os.Stat(prefsPath) + assert.True(t, errors.Is(err, os.ErrNotExist), "prefs file should be removed") + }) +} diff --git a/client/internal/profilemanager/service.go b/client/internal/profilemanager/service.go index 696a60310..ec287f01a 100644 --- a/client/internal/profilemanager/service.go +++ b/client/internal/profilemanager/service.go @@ -420,6 +420,11 @@ func (s *ServiceManager) RemoveProfile(id ID, username string) error { log.Warnf("failed to remove profile state file %s: %v", stateFile, err) } + prefsFile := filepath.Join(filepath.Dir(target.Path), id.String()+prefsFileSuffix) + if err := removePrefsFile(prefsFile); err != nil && !os.IsNotExist(err) { + log.Warnf("failed to remove profile prefs file %s: %v", prefsFile, err) + } + return nil } 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/ios/NetBirdSDK/login.go b/client/ios/NetBirdSDK/login.go index 6cba0c411..42a575359 100644 --- a/client/ios/NetBirdSDK/login.go +++ b/client/ios/NetBirdSDK/login.go @@ -323,7 +323,7 @@ func (a *Auth) login(urlOpener URLOpener, forceDeviceAuth bool, deviceName strin const authInfoRequestTimeout = 30 * time.Second func (a *Auth) foregroundGetTokenInfo(authClient *auth.Auth, urlOpener URLOpener, forceDeviceAuth bool) (*auth.TokenInfo, error) { - oAuthFlow, err := authClient.GetOAuthFlow(a.ctx, forceDeviceAuth) + oAuthFlow, err := authClient.GetOAuthFlow(a.ctx, forceDeviceAuth, "") if err != nil { return nil, fmt.Errorf("failed to get OAuth flow: %v", err) } 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/client/client.go b/client/ssh/client/client.go index 4180849cd..31143a4f4 100644 --- a/client/ssh/client/client.go +++ b/client/ssh/client/client.go @@ -313,21 +313,23 @@ func Dial(ctx context.Context, addr, user string, opts DialOptions) (*Client, er // dialSSH establishes an SSH connection without JWT authentication func dialSSH(ctx context.Context, network, addr string, config *ssh.ClientConfig) (*Client, error) { + if config.Timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, config.Timeout) + defer cancel() + } + dialer := &net.Dialer{} conn, err := dialer.DialContext(ctx, network, addr) if err != nil { return nil, fmt.Errorf("dial %s: %w", addr, err) } - clientConn, chans, reqs, err := ssh.NewClientConn(conn, addr, config) + client, err := nbssh.Handshake(ctx, conn, addr, config) if err != nil { - if closeErr := conn.Close(); closeErr != nil { - log.Debugf("connection close after handshake failure: %v", closeErr) - } - return nil, fmt.Errorf("ssh handshake: %w", err) + return nil, err } - client := ssh.NewClient(clientConn, chans, reqs) return &Client{ client: client, }, nil diff --git a/client/ssh/client/terminal_unix.go b/client/ssh/client/terminal_unix.go index aaa3418f9..a963dc8be 100644 --- a/client/ssh/client/terminal_unix.go +++ b/client/ssh/client/terminal_unix.go @@ -12,6 +12,8 @@ import ( log "github.com/sirupsen/logrus" "golang.org/x/crypto/ssh" "golang.org/x/term" + + nbssh "github.com/netbirdio/netbird/client/ssh" ) func (c *Client) setupTerminalMode(ctx context.Context, session *ssh.Session) error { @@ -82,37 +84,7 @@ func (c *Client) setupTerminal(session *ssh.Session, fd int) error { return fmt.Errorf("get terminal size: %w", err) } - modes := ssh.TerminalModes{ - ssh.ECHO: 1, - ssh.TTY_OP_ISPEED: 14400, - ssh.TTY_OP_OSPEED: 14400, - // Ctrl+C - ssh.VINTR: 3, - // Ctrl+\ - ssh.VQUIT: 28, - // Backspace - ssh.VERASE: 127, - // Ctrl+U - ssh.VKILL: 21, - // Ctrl+D - ssh.VEOF: 4, - ssh.VEOL: 0, - ssh.VEOL2: 0, - // Ctrl+Q - ssh.VSTART: 17, - // Ctrl+S - ssh.VSTOP: 19, - // Ctrl+Z - ssh.VSUSP: 26, - // Ctrl+O - ssh.VDISCARD: 15, - // Ctrl+R - ssh.VREPRINT: 18, - // Ctrl+W - ssh.VWERASE: 23, - // Ctrl+V - ssh.VLNEXT: 22, - } + modes := nbssh.DefaultTerminalModes terminal := os.Getenv("TERM") if terminal == "" { diff --git a/client/ssh/client/terminal_windows.go b/client/ssh/client/terminal_windows.go index 462438317..c6156fc26 100644 --- a/client/ssh/client/terminal_windows.go +++ b/client/ssh/client/terminal_windows.go @@ -10,6 +10,8 @@ import ( log "github.com/sirupsen/logrus" "golang.org/x/crypto/ssh" + + nbssh "github.com/netbirdio/netbird/client/ssh" ) const ( @@ -80,28 +82,14 @@ func (c *Client) setupTerminalMode(_ context.Context, session *ssh.Session) erro w, h := c.getWindowsConsoleSize() modes := ssh.TerminalModes{ - ssh.ECHO: 1, - ssh.TTY_OP_ISPEED: 14400, - ssh.TTY_OP_OSPEED: 14400, - ssh.ICRNL: 1, - ssh.OPOST: 1, - ssh.ONLCR: 1, - ssh.ISIG: 1, - ssh.ICANON: 1, - ssh.VINTR: 3, // Ctrl+C - ssh.VQUIT: 28, // Ctrl+\ - ssh.VERASE: 127, // Backspace - ssh.VKILL: 21, // Ctrl+U - ssh.VEOF: 4, // Ctrl+D - ssh.VEOL: 0, - ssh.VEOL2: 0, - ssh.VSTART: 17, // Ctrl+Q - ssh.VSTOP: 19, // Ctrl+S - ssh.VSUSP: 26, // Ctrl+Z - ssh.VDISCARD: 15, // Ctrl+O - ssh.VWERASE: 23, // Ctrl+W - ssh.VLNEXT: 22, // Ctrl+V - ssh.VREPRINT: 18, // Ctrl+R + ssh.ICRNL: 1, + ssh.OPOST: 1, + ssh.ONLCR: 1, + ssh.ISIG: 1, + ssh.ICANON: 1, + } + for mode, value := range nbssh.DefaultTerminalModes { + modes[mode] = value } if err := session.RequestPty("xterm-256color", h, w, modes); err != nil { diff --git a/client/ssh/common.go b/client/ssh/common.go index 92e647b7d..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 ( @@ -34,6 +35,19 @@ type HostKeyVerifier interface { VerifySSHHostKey(peerAddress string, key []byte) error } +// PeerKeyLookup returns the stored SSH host key for a peer address. +type PeerKeyLookup func(peerAddress string) ([]byte, bool) + +// VerifySSHHostKey implements HostKeyVerifier by looking up the stored key +// and comparing it against the presented key. +func (l PeerKeyLookup) VerifySSHHostKey(peerAddress string, presentedKey []byte) error { + storedKey, found := l(peerAddress) + if !found { + return ErrPeerNotFound + } + return VerifyHostKey(storedKey, presentedKey, peerAddress) +} + // DaemonHostKeyVerifier implements HostKeyVerifier using the NetBird daemon type DaemonHostKeyVerifier struct { client proto.DaemonServiceClient @@ -92,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 } @@ -193,4 +208,3 @@ func buildAddressList(hostname string, remote net.Addr) []string { } return addresses } - diff --git a/client/ssh/handshake.go b/client/ssh/handshake.go new file mode 100644 index 000000000..e78a806be --- /dev/null +++ b/client/ssh/handshake.go @@ -0,0 +1,45 @@ +package ssh + +import ( + "context" + "fmt" + "io" + "net" + "time" + + log "github.com/sirupsen/logrus" + "golang.org/x/crypto/ssh" +) + +// Handshake runs the SSH client handshake on an already dialed conn and +// returns the resulting client. Dialing bounds only the TCP establishment; +// without a deadline on the socket a peer that accepts and then goes silent +// blocks the handshake forever, so the context deadline is applied to conn +// for the duration of the handshake. conn is closed on any error. +func Handshake(ctx context.Context, conn net.Conn, addr string, config *ssh.ClientConfig) (*ssh.Client, error) { + if deadline, ok := ctx.Deadline(); ok { + if err := conn.SetDeadline(deadline); err != nil { + closeHandshake(conn, "conn after deadline error") + return nil, fmt.Errorf("set handshake deadline: %w", err) + } + } + + sshConn, chans, reqs, err := ssh.NewClientConn(conn, addr, config) + if err != nil { + closeHandshake(conn, "conn after handshake error") + return nil, fmt.Errorf("ssh handshake: %w", err) + } + + if err := conn.SetDeadline(time.Time{}); err != nil { + closeHandshake(sshConn, "ssh conn after deadline clear error") + return nil, fmt.Errorf("clear handshake deadline: %w", err) + } + + return ssh.NewClient(sshConn, chans, reqs), nil +} + +func closeHandshake(c io.Closer, label string) { + if err := c.Close(); err != nil { + log.Debugf("ssh: close %s: %v", label, err) + } +} diff --git a/client/ssh/proxy/proxy.go b/client/ssh/proxy/proxy.go index 721810edb..070515b57 100644 --- a/client/ssh/proxy/proxy.go +++ b/client/ssh/proxy/proxy.go @@ -610,13 +610,10 @@ func (p *SSHProxy) dialBackend(ctx context.Context, addr, user, jwtToken string) return nil, fmt.Errorf("connect to server: %w", err) } - clientConn, chans, reqs, err := cryptossh.NewClientConn(conn, addr, config) - if err != nil { - _ = conn.Close() - return nil, fmt.Errorf("SSH handshake: %w", err) - } + handshakeCtx, cancel := context.WithTimeout(ctx, sshHandshakeTimeout) + defer cancel() - return cryptossh.NewClient(clientConn, chans, reqs), nil + return nbssh.Handshake(handshakeCtx, conn, addr, config) } func (p *SSHProxy) verifyHostKey(hostname string, remote net.Addr, key cryptossh.PublicKey) error { diff --git a/client/ssh/session.go b/client/ssh/session.go new file mode 100644 index 000000000..999b6f251 --- /dev/null +++ b/client/ssh/session.go @@ -0,0 +1,84 @@ +package ssh + +import ( + "fmt" + "io" + + log "github.com/sirupsen/logrus" + "golang.org/x/crypto/ssh" +) + +// DefaultTerminalModes are the PTY modes used by the interactive terminal clients. +var DefaultTerminalModes = ssh.TerminalModes{ + ssh.ECHO: 1, + ssh.TTY_OP_ISPEED: 14400, + ssh.TTY_OP_OSPEED: 14400, + ssh.VINTR: 3, // Ctrl+C + ssh.VQUIT: 28, // Ctrl+\ + ssh.VERASE: 127, // Backspace + ssh.VKILL: 21, // Ctrl+U + ssh.VEOF: 4, // Ctrl+D + ssh.VEOL: 0, + ssh.VEOL2: 0, + ssh.VSTART: 17, // Ctrl+Q + ssh.VSTOP: 19, // Ctrl+S + ssh.VSUSP: 26, // Ctrl+Z + ssh.VDISCARD: 15, // Ctrl+O + ssh.VREPRINT: 18, // Ctrl+R + ssh.VWERASE: 23, // Ctrl+W + ssh.VLNEXT: 22, // Ctrl+V +} + +// PTYSession is an interactive shell session with a PTY and its I/O pipes. +type PTYSession struct { + Session *ssh.Session + Stdin io.WriteCloser + Stdout io.Reader + Stderr io.Reader +} + +// StartPTYSession opens a session on the client, requests an xterm-256color PTY +// with the default terminal modes, wires up the I/O pipes and starts a shell. +// The session is closed on any error. +func StartPTYSession(client *ssh.Client, cols, rows int) (*PTYSession, error) { + session, err := client.NewSession() + if err != nil { + return nil, fmt.Errorf("new session: %w", err) + } + + pty, err := setupPTYSession(session, cols, rows) + if err != nil { + if closeErr := session.Close(); closeErr != nil { + log.Debugf("ssh: session close after setup error: %v", closeErr) + } + return nil, err + } + return pty, nil +} + +// setupPTYSession requests the PTY, opens the pipes and starts the shell on an +// already created session. +func setupPTYSession(session *ssh.Session, cols, rows int) (*PTYSession, error) { + if err := session.RequestPty("xterm-256color", rows, cols, DefaultTerminalModes); err != nil { + return nil, fmt.Errorf("request pty: %w", err) + } + + stdin, err := session.StdinPipe() + if err != nil { + return nil, fmt.Errorf("stdin pipe: %w", err) + } + stdout, err := session.StdoutPipe() + if err != nil { + return nil, fmt.Errorf("stdout pipe: %w", err) + } + stderr, err := session.StderrPipe() + if err != nil { + return nil, fmt.Errorf("stderr pipe: %w", err) + } + + if err := session.Shell(); err != nil { + return nil, fmt.Errorf("start shell: %w", err) + } + + return &PTYSession{Session: session, Stdin: stdin, Stdout: stdout, Stderr: stderr}, nil +} 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/client/wasm/internal/ssh/client.go b/client/wasm/internal/ssh/client.go index 9cfe65266..28ae95ec0 100644 --- a/client/wasm/internal/ssh/client.go +++ b/client/wasm/internal/ssh/client.go @@ -80,13 +80,12 @@ func (c *Client) Connect(host string, port int, username, jwtToken string, ipVer return fmt.Errorf("dial %s: %w", addr, err) } - sshConn, chans, reqs, err := ssh.NewClientConn(conn, addr, config) + sshClient, err := nbssh.Handshake(ctx, conn, addr, config) if err != nil { - closeWithLog(conn, "connection after handshake error") - return fmt.Errorf("SSH handshake: %w", err) + return err } - c.sshClient = ssh.NewClient(sshConn, chans, reqs) + c.sshClient = sshClient logrus.Infof("SSH: Connected to %s", addr) return nil @@ -119,57 +118,26 @@ func (c *Client) getAuthMethods(jwtToken string) ([]ssh.AuthMethod, error) { return []ssh.AuthMethod{ssh.PublicKeys(signer)}, nil } -// StartSession starts an SSH session with PTY +// StartSession starts an SSH session with PTY. It holds the client lock for +// the whole startup so Close cannot tear the client down mid-setup and the +// new session cannot be installed into an already closed client. func (c *Client) StartSession(cols, rows int) error { + c.mu.Lock() + defer c.mu.Unlock() + if c.sshClient == nil { return fmt.Errorf("SSH client not connected") } - session, err := c.sshClient.NewSession() + pty, err := nbssh.StartPTYSession(c.sshClient, cols, rows) if err != nil { - return fmt.Errorf("create session: %w", err) + return err } - c.mu.Lock() - defer c.mu.Unlock() - c.session = session - - modes := ssh.TerminalModes{ - ssh.ECHO: 1, - ssh.TTY_OP_ISPEED: 14400, - ssh.TTY_OP_OSPEED: 14400, - ssh.VINTR: 3, - ssh.VQUIT: 28, - ssh.VERASE: 127, - } - - if err := session.RequestPty("xterm-256color", rows, cols, modes); err != nil { - closeWithLog(session, "session after PTY error") - return fmt.Errorf("PTY request: %w", err) - } - - c.stdin, err = session.StdinPipe() - if err != nil { - closeWithLog(session, "session after stdin error") - return fmt.Errorf("get stdin: %w", err) - } - - c.stdout, err = session.StdoutPipe() - if err != nil { - closeWithLog(session, "session after stdout error") - return fmt.Errorf("get stdout: %w", err) - } - - c.stderr, err = session.StderrPipe() - if err != nil { - closeWithLog(session, "session after stderr error") - return fmt.Errorf("get stderr: %w", err) - } - - if err := session.Shell(); err != nil { - closeWithLog(session, "session after shell error") - return fmt.Errorf("start shell: %w", err) - } + c.session = pty.Session + c.stdin = pty.Stdin + c.stdout = pty.Stdout + c.stderr = pty.Stderr logrus.Info("SSH: Session started with PTY") return nil 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) + }) + } +}