From 2299e3d5a742ac5c8d28cf3fa36a8fcaacbb771d Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Wed, 14 Oct 2026 13:46:35 +0200 Subject: [PATCH 01/10] [management] move is child account (#8195) --- management/internals/server/boot.go | 2 +- management/internals/server/modules.go | 4 ---- management/server/account.go | 18 ++---------------- management/server/auth/manager.go | 4 ++-- .../server/http/middleware/auth_middleware.go | 19 ------------------- .../http/middleware/auth_middleware_test.go | 12 +----------- .../http/middleware/middleware_factory.go | 3 +-- .../testing/testing_tools/channel/channel.go | 4 ++-- 8 files changed, 9 insertions(+), 57 deletions(-) diff --git a/management/internals/server/boot.go b/management/internals/server/boot.go index 4ced64a16..13bbaa6fa 100644 --- a/management/internals/server/boot.go +++ b/management/internals/server/boot.go @@ -204,7 +204,7 @@ func (s *BaseServer) IDPHandler() http.Handler { func (s *BaseServer) Router() *mux.Router { return Create(s, func() *mux.Router { router := mux.NewRouter() - router.Use(middleware.BuildMiddleware(s.RateLimiter(), s.AuthManager(), s.AccountManager(), s.Metrics(), s.IsValidChildAccount)...) + router.Use(middleware.BuildMiddleware(s.RateLimiter(), s.AuthManager(), s.AccountManager(), s.Metrics())...) return router }) } diff --git a/management/internals/server/modules.go b/management/internals/server/modules.go index f548e9238..3379becf6 100644 --- a/management/internals/server/modules.go +++ b/management/internals/server/modules.go @@ -248,7 +248,3 @@ func (s *BaseServer) ReverseProxyDomainManager() *manager.Manager { return &m }) } - -func (s *BaseServer) IsValidChildAccount(_ context.Context, _, _, _ string) bool { - return false -} diff --git a/management/server/account.go b/management/server/account.go index 0a8ec9a59..90e384ac0 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -1637,10 +1637,6 @@ func (am *DefaultAccountManager) GetAccountIDFromUserAuth(ctx context.Context, u return "", "", status.Errorf(status.NotFound, "user %s not found", userAuth.UserId) } - if userAuth.IsChild { - return accountID, user.Id, nil - } - ctx, err = am.permissionsManager.ValidateAccountAccess(ctx, accountID, user, false) if err != nil { return "", "", err @@ -1692,10 +1688,10 @@ func (am *DefaultAccountManager) updateUserAuthWithSingleMode(ctx context.Contex // and propagates changes to peers if group propagation is enabled. // requires userAuth to have been ValidateAndParseToken and EnsureUserAccessByJWTGroups by the AuthManager func (am *DefaultAccountManager) SyncUserJWTGroups(ctx context.Context, userAuth auth.UserAuth) error { - // Child accounts and PAT-authenticated requests do not sync JWT groups. + // PAT-authenticated requests do not sync JWT groups. // Embedded-Dex local users also skip sync because local password authentication // does not provide external IdP group claims. - if userAuth.IsChild || userAuth.IsPAT || dex.IsLocalUserID(userAuth.UserId) { + if userAuth.IsPAT || dex.IsLocalUserID(userAuth.UserId) { return nil } @@ -1874,8 +1870,6 @@ func (am *DefaultAccountManager) SyncUserJWTGroups(ctx context.Context, userAuth // Existing user + Existing account + Existing Indexed Domain -> Nothing changes // // Existing user + Existing account + Existing domain reclassified Domain as private -> Nothing changes (index domain) -// -// UserAuth IsChild -> checks that account exists func (am *DefaultAccountManager) getAccountIDWithAuthorizationClaims(ctx context.Context, userAuth auth.UserAuth) (string, error) { log.WithContext(ctx).Tracef("getting account with authorization claims. User ID: \"%s\", Account ID: \"%s\", Domain: \"%s\", Domain Category: \"%s\"", userAuth.UserId, userAuth.AccountId, userAuth.Domain, userAuth.DomainCategory) @@ -1884,14 +1878,6 @@ func (am *DefaultAccountManager) getAccountIDWithAuthorizationClaims(ctx context return "", errors.New(emptyUserID) } - if userAuth.IsChild { - exists, err := am.Store.AccountExists(ctx, store.LockingStrengthNone, userAuth.AccountId) - if err != nil || !exists { - return "", err - } - return userAuth.AccountId, nil - } - if userAuth.DomainCategory != types.PrivateCategory || !isDomainValid(userAuth.Domain) { return am.GetAccountIDByUserID(ctx, userAuth) } diff --git a/management/server/auth/manager.go b/management/server/auth/manager.go index 9498789f2..7064c094f 100644 --- a/management/server/auth/manager.go +++ b/management/server/auth/manager.go @@ -75,10 +75,10 @@ func (m *manager) ValidateAndParseToken(ctx context.Context, value string) (auth } func (m *manager) EnsureUserAccessByJWTGroups(ctx context.Context, userAuth auth.UserAuth, token *jwt.Token) (auth.UserAuth, error) { - // Child accounts and PAT-authenticated requests do not use JWT group access checks. + // PAT-authenticated requests do not use JWT group access checks. // Embedded-Dex local users also skip them because local password authentication // does not provide external IdP group claims. - if userAuth.IsChild || userAuth.IsPAT || dex.IsLocalUserID(userAuth.UserId) { + if userAuth.IsPAT || dex.IsLocalUserID(userAuth.UserId) { return userAuth, nil } diff --git a/management/server/http/middleware/auth_middleware.go b/management/server/http/middleware/auth_middleware.go index 1e831e6c3..7c21b6a55 100644 --- a/management/server/http/middleware/auth_middleware.go +++ b/management/server/http/middleware/auth_middleware.go @@ -26,8 +26,6 @@ type SyncUserJWTGroupsFunc func(ctx context.Context, userAuth auth.UserAuth) err type GetUserFromUserAuthFunc func(ctx context.Context, userAuth auth.UserAuth) (*types.User, error) -type IsValidChildAccountFunc func(ctx context.Context, userID, accountID, childAccountID string) bool - // AuthMiddleware middleware to verify personal access tokens (PAT) and JWT tokens type AuthMiddleware struct { authManager serverauth.Manager @@ -36,7 +34,6 @@ type AuthMiddleware struct { syncUserJWTGroups SyncUserJWTGroupsFunc rateLimiter *ratelimit.APIRateLimiter patUsageTracker *PATUsageTracker - isValidChildAccount IsValidChildAccountFunc } // NewAuthMiddleware instance constructor @@ -47,7 +44,6 @@ func NewAuthMiddleware( getUserFromUserAuth GetUserFromUserAuthFunc, rateLimiter *ratelimit.APIRateLimiter, meter metric.Meter, - isValidChildAccount IsValidChildAccountFunc, ) *AuthMiddleware { var patUsageTracker *PATUsageTracker if meter != nil { @@ -65,7 +61,6 @@ func NewAuthMiddleware( getUserFromUserAuth: getUserFromUserAuth, rateLimiter: rateLimiter, patUsageTracker: patUsageTracker, - isValidChildAccount: isValidChildAccount, } } @@ -127,13 +122,6 @@ func (m *AuthMiddleware) checkJWTFromRequest(r *http.Request, authHeaderParts [] return err } - if impersonate, ok := r.URL.Query()["account"]; ok && len(impersonate) == 1 { - if m.isValidChildAccount(ctx, userAuth.UserId, userAuth.AccountId, impersonate[0]) { - userAuth.AccountId = impersonate[0] - userAuth.IsChild = true - } - } - // Email is now extracted in ToUserAuth (from claims or userinfo endpoint) // Available as userAuth.Email @@ -210,13 +198,6 @@ func (m *AuthMiddleware) checkPATFromRequest(r *http.Request, authHeaderParts [] IsPAT: true, } - if impersonate, ok := r.URL.Query()["account"]; ok && len(impersonate) == 1 { - if m.isValidChildAccount(r.Context(), userAuth.UserId, userAuth.AccountId, impersonate[0]) { - userAuth.AccountId = impersonate[0] - userAuth.IsChild = true - } - } - // propagates ctx change to upstream middleware *r = *nbcontext.SetUserAuthInRequest(r, userAuth) return nil diff --git a/management/server/http/middleware/auth_middleware_test.go b/management/server/http/middleware/auth_middleware_test.go index ceefc7c45..9e60af1fa 100644 --- a/management/server/http/middleware/auth_middleware_test.go +++ b/management/server/http/middleware/auth_middleware_test.go @@ -104,7 +104,7 @@ func mockMarkPATUsed(_ context.Context, token string) error { } func mockEnsureUserAccessByJWTGroups(_ context.Context, userAuth nbauth.UserAuth, token *jwt.Token) (nbauth.UserAuth, error) { - if userAuth.IsChild || userAuth.IsPAT { + if userAuth.IsPAT { return userAuth, nil } @@ -212,7 +212,6 @@ func TestAuthMiddleware_Handler(t *testing.T) { }, disabledLimiter, nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handlerToTest := authMiddleware.Handler(nextHandler) @@ -279,7 +278,6 @@ func TestAuthMiddleware_SyncUserJWTGroupsDetachedFromRequestCancellation(t *test }, disabledLimiter, nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handlerToTest := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) @@ -332,7 +330,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -385,7 +382,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -429,7 +425,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -474,7 +469,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -539,7 +533,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -599,7 +592,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -655,7 +647,6 @@ func TestAuthMiddleware_RateLimiting(t *testing.T) { }, ratelimit.NewAPIRateLimiter(rateLimitConfig), nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) handler := authMiddleware.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -756,7 +747,6 @@ func TestAuthMiddleware_Handler_Child(t *testing.T) { }, disabledLimiter, nil, - func(_ context.Context, _, _, _ string) bool { return false }, ) for _, tc := range tt { diff --git a/management/server/http/middleware/middleware_factory.go b/management/server/http/middleware/middleware_factory.go index 3ae617774..5346301ea 100644 --- a/management/server/http/middleware/middleware_factory.go +++ b/management/server/http/middleware/middleware_factory.go @@ -10,7 +10,7 @@ import ( log "github.com/sirupsen/logrus" ) -func BuildMiddleware(rateLimiter *ratelimit.APIRateLimiter, authManager auth.Manager, accountManager account.Manager, metrics telemetry.AppMetrics, isValidChildAcctFunc IsValidChildAccountFunc) []mux.MiddlewareFunc { +func BuildMiddleware(rateLimiter *ratelimit.APIRateLimiter, authManager auth.Manager, accountManager account.Manager, metrics telemetry.AppMetrics) []mux.MiddlewareFunc { toret := make([]mux.MiddlewareFunc, 0) toret = append(toret, metrics.HTTPMiddleware().Handler) @@ -28,7 +28,6 @@ func BuildMiddleware(rateLimiter *ratelimit.APIRateLimiter, authManager auth.Man accountManager.GetUserFromUserAuth, rateLimiter, metrics.GetMeter(), - isValidChildAcctFunc, ).Handler) return toret diff --git a/management/server/http/testing/testing_tools/channel/channel.go b/management/server/http/testing/testing_tools/channel/channel.go index 1b15ba780..88ab70b9c 100644 --- a/management/server/http/testing/testing_tools/channel/channel.go +++ b/management/server/http/testing/testing_tools/channel/channel.go @@ -146,7 +146,7 @@ func BuildApiBlackBoxWithDBState(t testing_tools.TB, sqlFile string, expectedPee zoneRecordsManager := recordsManager.NewManager(store, am, permissionsManager) apiRouter := mux.NewRouter().PathPrefix("/api").Subrouter() - apiRouter.Use(middleware.BuildMiddleware(nil, authManagerMock, am, metrics, nil)...) + apiRouter.Use(middleware.BuildMiddleware(nil, authManagerMock, am, metrics)...) apiHandler, err := http2.NewAPIHandler(ctx, apiRouter, am, networksManager, resourcesManager, routersManager, groupsManager, geoMock, permissionsManager, settingsManager, customZonesManager, zoneRecordsManager, networkMapController, nil, serviceManager, nil, nil, nil, nil, nil, nil) @@ -288,7 +288,7 @@ func BuildApiBlackBoxWithDBStateAndPeerChannel(t testing_tools.TB, sqlFile strin zoneRecordsManager := recordsManager.NewManager(store, am, permissionsManager) apiRouter := mux.NewRouter().PathPrefix("/api").Subrouter() - apiRouter.Use(middleware.BuildMiddleware(nil, authManagerMock, am, metrics, nil)...) + apiRouter.Use(middleware.BuildMiddleware(nil, authManagerMock, am, metrics)...) apiHandler, err := http2.NewAPIHandler(ctx, apiRouter, am, networksManager, resourcesManager, routersManager, groupsManager, geoMock, permissionsManager, settingsManager, customZonesManager, zoneRecordsManager, networkMapController, nil, serviceManager, nil, nil, nil, nil, nil, nil) From 405bbb8997f3d58d34cf6138bb05f066c1686fd2 Mon Sep 17 00:00:00 2001 From: Maycon Santos Date: Wed, 14 Oct 2026 15:50:16 +0200 Subject: [PATCH 02/10] [infrastructure] Restrict the FreeBSD port job to stable releases (#8204) * [infrastructure] Restrict the FreeBSD port job to stable releases The port scripts picked the target version by scraping the GitHub tags page and only dropped tags containing "rc". A canary tag such as v0.81.0-canary.pr-8098.1 sorts above the latest stable release, so the job tried to package a prerelease, and the page only lists ten tags, so a run of prereleases could leave no candidate at all. Both scripts now share one helper that reads the tags API, keeps MAJOR.MINOR.PATCH versions only, and rejects an explicitly requested prerelease. Go proxy downloads fail instead of hashing an error page. The job moves out of the release workflow into its own, so it runs on pushes to main, on release tags, and on pull requests that touch the scripts or the workflow itself. Prerelease tags skip the job, and a stable tag is packaged directly rather than whatever happens to be the latest release at the time. * [infrastructure] Keep the FreeBSD port job in the release workflow A separate workflow would rename the check and detach the job from the release run, so the job stays in release.yml behind a small trigger job that decides when it runs: pushes to main, stable release tags, and pull requests that touch the port scripts or the workflow. Prerelease tags and other branches skip it. Tags are now listed with git ls-remote, which returns every tag in one call and needs neither a token nor pagination, and the Go module proxy downloads are limited to HTTPS like the other fetches. * [infrastructure] Address Sonar findings in the FreeBSD port helper Read the version into a local and return explicitly from is_stable_version. * [infrastructure] Address review findings in the FreeBSD port job Only a full vMAJOR.MINOR.PATCH tag starts the port job, so a tag like v1.2 skips it instead of failing in the resolver. The diff detection steps use a glob array rather than parsing ls, and quote GITHUB_OUTPUT. A failed tag listing now stops the issue body script instead of writing an incomplete changelog, and a bare "v" argument is rejected rather than treated as a request for the latest release. --- .github/workflows/release.yml | 71 ++++++++++++++--- release_files/freebsd-port-diff.sh | 86 ++++----------------- release_files/freebsd-port-issue-body.sh | 88 +++++---------------- release_files/freebsd-port-lib.sh | 98 ++++++++++++++++++++++++ 4 files changed, 197 insertions(+), 146 deletions(-) create mode 100644 release_files/freebsd-port-lib.sh diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index d133d87f1..b3ed4763c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -23,8 +23,50 @@ concurrency: cancel-in-progress: true jobs: + freebsd_port_trigger: + name: "FreeBSD Port / Trigger" + runs-on: ubuntu-22.04 + outputs: + run: ${{ steps.decide.outputs.run }} + version: ${{ steps.decide.outputs.version }} + steps: + - name: Checkout + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + + - name: Decide whether the port job runs + id: decide + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: | + set -euo pipefail + run=false + case "$GITHUB_EVENT_NAME" in + push) + if [[ "$GITHUB_REF" == "refs/heads/main" ]]; then + run=true + elif [[ "$GITHUB_REF" == refs/tags/* && "$GITHUB_REF_NAME" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then + # The port tracks stable releases only, so a canary or rc tag has nothing to submit. + run=true + echo "version=${GITHUB_REF_NAME#v}" >> "$GITHUB_OUTPUT" + fi + ;; + pull_request) + git fetch --depth=1 origin "$BASE_SHA" + if git diff --name-only "$BASE_SHA" HEAD | \ + grep -qE '^(release_files/freebsd-port-.*\.sh|\.github/workflows/release\.yml)$'; then + run=true + fi + ;; + esac + echo "run=$run" >> "$GITHUB_OUTPUT" + echo "FreeBSD port job runs: $run" + release_freebsd_port: name: "FreeBSD Port / Build & Test" + needs: freebsd_port_trigger + if: needs.freebsd_port_trigger.outputs.run == 'true' runs-on: ubuntu-22.04 steps: - name: Checkout @@ -33,18 +75,19 @@ jobs: persist-credentials: false - name: Generate FreeBSD port diff - run: bash -x release_files/freebsd-port-diff.sh - - - name: Generate FreeBSD port issue body - run: bash -x release_files/freebsd-port-issue-body.sh + env: + TARGET_VERSION: ${{ needs.freebsd_port_trigger.outputs.version }} + run: bash -x release_files/freebsd-port-diff.sh "$TARGET_VERSION" - name: Check if diff was generated id: check_diff run: | - if ls netbird-*.diff 1> /dev/null 2>&1; then - echo "diff_exists=true" >> $GITHUB_OUTPUT + shopt -s nullglob + diffs=(netbird-*.diff) + if [[ ${#diffs[@]} -gt 0 ]]; then + echo "diff_exists=true" >> "$GITHUB_OUTPUT" else - echo "diff_exists=false" >> $GITHUB_OUTPUT + echo "diff_exists=false" >> "$GITHUB_OUTPUT" echo "No diff file generated (port may already be up to date)" fi @@ -52,10 +95,18 @@ jobs: if: steps.check_diff.outputs.diff_exists == 'true' id: version run: | - VERSION=$(ls netbird-*.diff | sed 's/netbird-\(.*\)\.diff/\1/') - echo "version=$VERSION" >> $GITHUB_OUTPUT + diffs=(netbird-*.diff) + VERSION="${diffs[0]#netbird-}" + VERSION="${VERSION%.diff}" + echo "version=$VERSION" >> "$GITHUB_OUTPUT" echo "Generated files for version: $VERSION" - cat netbird-*.diff + cat "${diffs[0]}" + + - name: Generate FreeBSD port issue body + if: steps.check_diff.outputs.diff_exists == 'true' + env: + TARGET_VERSION: ${{ steps.version.outputs.version }} + run: bash -x release_files/freebsd-port-issue-body.sh "" "$TARGET_VERSION" - name: Read Go version from go.mod id: goversion diff --git a/release_files/freebsd-port-diff.sh b/release_files/freebsd-port-diff.sh index 77ea55520..cd3c341b9 100755 --- a/release_files/freebsd-port-diff.sh +++ b/release_files/freebsd-port-diff.sh @@ -9,62 +9,19 @@ # Usage: ./freebsd-port-diff.sh [new_version] # Example: ./freebsd-port-diff.sh 0.60.7 # -# If no version is provided, it fetches the latest from GitHub. +# If no version is provided, it uses the latest stable release from GitHub. +# Prerelease versions (canary, rc, ...) are rejected: the port only tracks +# stable releases. set -e -GITHUB_REPO="netbirdio/netbird" -PORTS_MIRROR_BASE="https://raw.githubusercontent.com/freebsd/freebsd-ports/main/security/netbird" +# shellcheck source=release_files/freebsd-port-lib.sh +source "$(dirname "${BASH_SOURCE[0]}")/freebsd-port-lib.sh" + GO_PROXY="https://proxy.golang.org/github.com/netbirdio/netbird/@v" OUTPUT_DIR="${OUTPUT_DIR:-.}" AWK_FIRST_FIELD='{print $1}' -fetch_all_tags() { - curl -sL "https://github.com/${GITHUB_REPO}/tags" 2>/dev/null | \ - grep -oE '/releases/tag/v[0-9]+\.[0-9]+\.[0-9]+([^"]+)?' | \ - grep -iv 'rc' | \ - sed 's/.*\/v//' | \ - sort -u -V - return 0 -} - -fetch_current_ports_version() { - echo "Fetching current version from FreeBSD ports..." >&2 - local makefile version - makefile=$(fetch_ports_file "Makefile") || return 1 - version=$(echo "$makefile" | \ - grep -E "^DISTVERSION=" | \ - sed 's/DISTVERSION=[[:space:]]*//' | \ - tr -d '\t ') - if [[ -z "$version" ]]; then - echo "Error: Could not extract DISTVERSION from ports Makefile" >&2 - return 1 - fi - echo "$version" - return 0 -} - -fetch_latest_github_release() { - echo "Fetching latest release from GitHub..." >&2 - fetch_all_tags | tail -1 - return 0 -} - -fetch_ports_file() { - local filename="$1" - local content - if ! content=$(curl -fsL --proto '=https' --proto-redir '=https' --retry 3 "${PORTS_MIRROR_BASE}/${filename}" 2>/dev/null); then - echo "Error: Could not fetch ${filename} from ${PORTS_MIRROR_BASE}" >&2 - return 1 - fi - if [[ "$content" == \<* ]]; then - echo "Error: Received HTML instead of ${filename} from ${PORTS_MIRROR_BASE}" >&2 - return 1 - fi - printf '%s' "$content" - return 0 -} - compute_checksums() { local version="$1" local tmpdir @@ -77,11 +34,15 @@ compute_checksums() { local mod_file="${tmpdir}/v${version}.mod" local zip_file="${tmpdir}/v${version}.zip" - curl -sL "${GO_PROXY}/v${version}.mod" -o "$mod_file" 2>/dev/null - curl -sL "${GO_PROXY}/v${version}.zip" -o "$zip_file" 2>/dev/null + local -a curl_opts=(-fsL --proto '=https' --proto-redir '=https' --retry 3) + if ! curl "${curl_opts[@]}" "${GO_PROXY}/v${version}.mod" -o "$mod_file" 2>/dev/null || \ + ! curl "${curl_opts[@]}" "${GO_PROXY}/v${version}.zip" -o "$zip_file" 2>/dev/null; then + echo "Error: Could not download v${version} from the Go module proxy" >&2 + return 1 + fi if [[ ! -s "$mod_file" ]] || [[ ! -s "$zip_file" ]]; then - echo "Error: Could not download files from Go module proxy" >&2 + echo "Error: Go module proxy returned empty files for v${version}" >&2 return 1 fi @@ -133,25 +94,12 @@ generate_new_makefile() { } # Parse arguments -NEW_VERSION="${1:-}" - -# Auto-detect versions if not provided -OLD_VERSION=$(fetch_current_ports_version) -if [[ -z "$OLD_VERSION" ]]; then - echo "Error: Could not fetch current version from FreeBSD ports" >&2 - exit 1 -fi -echo "Current FreeBSD ports version: ${OLD_VERSION}" >&2 - -if [[ -z "$NEW_VERSION" ]]; then - NEW_VERSION=$(fetch_latest_github_release) - if [[ -z "$NEW_VERSION" ]]; then - echo "Error: Could not fetch latest release from GitHub" >&2 - exit 1 - fi -fi +NEW_VERSION=$(resolve_target_version "${1:-}") echo "Target version: ${NEW_VERSION}" >&2 +OLD_VERSION=$(fetch_current_ports_version) +echo "Current FreeBSD ports version: ${OLD_VERSION}" >&2 + if [[ "$OLD_VERSION" = "$NEW_VERSION" ]]; then echo "Port is already at version ${NEW_VERSION}. Nothing to do." >&2 exit 0 diff --git a/release_files/freebsd-port-issue-body.sh b/release_files/freebsd-port-issue-body.sh index 1f0c8a567..16fe0e2b6 100755 --- a/release_files/freebsd-port-issue-body.sh +++ b/release_files/freebsd-port-issue-body.sh @@ -10,81 +10,29 @@ # # If no versions are provided, the script will: # - Fetch OLD version from the FreeBSD ports GitHub mirror (current version in ports tree) -# - Fetch NEW version from latest NetBird GitHub release tag +# - Fetch NEW version from the latest stable NetBird release tag on GitHub +# +# Prerelease versions (canary, rc, ...) are rejected: the port only tracks +# stable releases. set -e -GITHUB_REPO="netbirdio/netbird" -PORTS_MAKEFILE_URL="https://raw.githubusercontent.com/freebsd/freebsd-ports/main/security/netbird/Makefile" - -fetch_current_ports_version() { - echo "Fetching current version from FreeBSD ports..." >&2 - local makefile_content - makefile_content=$(curl -fsL --proto '=https' --proto-redir '=https' --retry 3 "$PORTS_MAKEFILE_URL" 2>/dev/null) || makefile_content="" - if [[ "$makefile_content" == \<* ]]; then - echo "Error: Received HTML instead of Makefile from ${PORTS_MAKEFILE_URL}" >&2 - return 1 - fi - if [[ -z "$makefile_content" ]]; then - echo "Error: Could not fetch Makefile from FreeBSD ports" >&2 - return 1 - fi - echo "$makefile_content" | grep -E "^DISTVERSION=" | sed 's/DISTVERSION=[[:space:]]*//' | tr -d '\t ' - return 0 -} - -fetch_all_tags() { - # Fetch tags from GitHub tags page (no rate limiting, no auth needed) - curl -sL "https://github.com/${GITHUB_REPO}/tags" 2>/dev/null | \ - grep -oE '/releases/tag/v[0-9]+\.[0-9]+\.[0-9]+([^"]+)?' | \ - grep -iv 'rc' | \ - sed 's/.*\/v//' | \ - sort -u -V - return 0 -} - -fetch_latest_github_release() { - echo "Fetching latest release from GitHub..." >&2 - local latest - - # Fetch from GitHub tags page - latest=$(fetch_all_tags | tail -1) - - if [[ -z "$latest" ]]; then - # Fallback to GitHub API - latest=$(curl -sL "https://api.github.com/repos/${GITHUB_REPO}/releases/latest" 2>/dev/null | \ - grep '"tag_name"' | sed 's/.*"tag_name": *"v\([^"]*\)".*/\1/') - fi - - if [[ -z "$latest" ]]; then - echo "Error: Could not fetch latest release from GitHub" >&2 - return 1 - fi - echo "$latest" - return 0 -} +# shellcheck source=release_files/freebsd-port-lib.sh +source "$(dirname "${BASH_SOURCE[0]}")/freebsd-port-lib.sh" OLD_VERSION="${1:-}" NEW_VERSION="${2:-}" if [[ -z "$OLD_VERSION" ]]; then OLD_VERSION=$(fetch_current_ports_version) - if [[ -z "$OLD_VERSION" ]]; then - echo "Error: Could not determine old version. Please provide it manually." >&2 - echo "Usage: $0 " >&2 - exit 1 - fi echo "Detected OLD version from FreeBSD ports: $OLD_VERSION" >&2 fi if [[ -z "$NEW_VERSION" ]]; then - NEW_VERSION=$(fetch_latest_github_release) - if [[ -z "$NEW_VERSION" ]]; then - echo "Error: Could not determine new version. Please provide it manually." >&2 - echo "Usage: $0 " >&2 - exit 1 - fi + NEW_VERSION=$(resolve_target_version "") echo "Detected NEW version from GitHub: $NEW_VERSION" >&2 +else + NEW_VERSION=$(resolve_target_version "$NEW_VERSION") fi if [[ "$OLD_VERSION" = "$NEW_VERSION" ]]; then @@ -98,8 +46,11 @@ OUTPUT_DIR="${OUTPUT_DIR:-.}" fetch_releases_between_versions() { echo "Fetching release history from GitHub..." >&2 - # Fetch all tags and filter to those between OLD and NEW versions - fetch_all_tags | \ + local tags + tags=$(fetch_stable_tags) || return 1 + + # List the stable releases after OLD up to and including NEW + echo "$tags" | \ while read -r ver; do if [[ "$(printf '%s\n' "$OLD_VERSION" "$ver" | sort -V | head -n1)" = "$OLD_VERSION" ]] && \ [[ "$(printf '%s\n' "$ver" "$NEW_VERSION" | sort -V | head -n1)" = "$ver" ]] && \ @@ -112,7 +63,7 @@ fetch_releases_between_versions() { generate_changelog_section() { local releases - releases=$(fetch_releases_between_versions) + releases=$(fetch_releases_between_versions) || return 1 echo "Changelogs:" if [[ -n "$releases" ]]; then @@ -127,7 +78,10 @@ generate_changelog_section() { OUTPUT_FILE="${OUTPUT_DIR}/netbird-${NEW_VERSION}-issue.txt" -cat << EOF > "$OUTPUT_FILE" +# Built before the heredoc: a failing substitution inside it would not stop the script. +CHANGELOG=$(generate_changelog_section) + +cat << EOT > "$OUTPUT_FILE" BUGZILLA ISSUE DETAILS ====================== @@ -139,11 +93,11 @@ Description: ------------ security/netbird: Update ${OLD_VERSION} => ${NEW_VERSION} -$(generate_changelog_section) +${CHANGELOG} Commit log: https://github.com/${GITHUB_REPO}/compare/v${OLD_VERSION}...v${NEW_VERSION} -EOF +EOT echo "=========================================" echo "Issue body saved to: ${OUTPUT_FILE}" diff --git a/release_files/freebsd-port-lib.sh b/release_files/freebsd-port-lib.sh new file mode 100644 index 000000000..20102be54 --- /dev/null +++ b/release_files/freebsd-port-lib.sh @@ -0,0 +1,98 @@ +#!/bin/bash +# +# Shared helpers for the FreeBSD port scripts. Source this file, do not run it. +# +# The FreeBSD port tracks stable NetBird releases only, so every helper here +# works on plain MAJOR.MINOR.PATCH versions and ignores canary, release +# candidate and any other prerelease tag. + +GITHUB_REPO="netbirdio/netbird" +GITHUB_REPO_URL="https://github.com/${GITHUB_REPO}.git" +PORTS_MIRROR_BASE="https://raw.githubusercontent.com/freebsd/freebsd-ports/main/security/netbird" +STABLE_VERSION_RE='^[0-9]+\.[0-9]+\.[0-9]+$' + +# is_stable_version reports whether the version is a plain MAJOR.MINOR.PATCH release. +is_stable_version() { + local version="$1" + if [[ "$version" =~ $STABLE_VERSION_RE ]]; then + return 0 + fi + return 1 +} + +# fetch_stable_tags prints the stable release versions known to GitHub, oldest +# first. git ls-remote returns every tag in one round trip, with no API +# pagination or rate limit to work around. +fetch_stable_tags() { + local refs + if ! refs=$(git ls-remote --tags --refs "$GITHUB_REPO_URL" 2>/dev/null); then + echo "Error: Could not list tags of ${GITHUB_REPO_URL}" >&2 + return 1 + fi + + echo "$refs" | \ + sed -nE 's#.*refs/tags/v([0-9]+\.[0-9]+\.[0-9]+)$#\1#p' | \ + sort -u -V + return 0 +} + +# resolve_target_version validates an explicitly requested version, or picks the +# latest stable release from GitHub when none was given. A leading "v" is +# accepted so a git tag name can be passed as is. +resolve_target_version() { + local raw="$1" + local requested="${raw#v}" + + if [[ -n "$raw" ]]; then + if ! is_stable_version "$requested"; then + echo "Error: '${raw}' is not a stable release. The FreeBSD port only tracks MAJOR.MINOR.PATCH versions." >&2 + return 1 + fi + echo "$requested" + return 0 + fi + + echo "Fetching latest stable release from GitHub..." >&2 + local latest + latest=$(fetch_stable_tags | tail -1) + if [[ -z "$latest" ]]; then + echo "Error: Could not determine the latest stable release from GitHub" >&2 + return 1 + fi + echo "$latest" + return 0 +} + +# fetch_ports_file prints a file from the security/netbird port on the FreeBSD +# ports GitHub mirror. +fetch_ports_file() { + local filename="$1" + local content + if ! content=$(curl -fsL --proto '=https' --proto-redir '=https' --retry 3 "${PORTS_MIRROR_BASE}/${filename}" 2>/dev/null); then + echo "Error: Could not fetch ${filename} from ${PORTS_MIRROR_BASE}" >&2 + return 1 + fi + if [[ "$content" == \<* ]]; then + echo "Error: Received HTML instead of ${filename} from ${PORTS_MIRROR_BASE}" >&2 + return 1 + fi + printf '%s' "$content" + return 0 +} + +# fetch_current_ports_version prints the DISTVERSION currently in the ports tree. +fetch_current_ports_version() { + echo "Fetching current version from FreeBSD ports..." >&2 + local makefile version + makefile=$(fetch_ports_file "Makefile") || return 1 + version=$(echo "$makefile" | \ + grep -E "^DISTVERSION=" | \ + sed 's/DISTVERSION=[[:space:]]*//' | \ + tr -d '\t ') + if [[ -z "$version" ]]; then + echo "Error: Could not extract DISTVERSION from ports Makefile" >&2 + return 1 + fi + echo "$version" + return 0 +} From 85db35194ef437bc37e6f3cfe561378582c6c994 Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Thu, 15 Oct 2026 10:53:55 +0200 Subject: [PATCH 03/10] [client] Prolong HA routes on the current routing peer across relay and direct transitions (#8192) * [client] Do not move an HA route on latency before both peers have a sample A direct connection reports no latency until the first sample of its new ICE agent, and a drop to relay clears the previous one. Route selection ranked such a peer at unknownLatency, so a routing peer whose direct connection came back within the relayed switch delay lost the route to an equivalent peer on a "lower latency" switch right after recovering, resetting every masqueraded flow the delay was meant to keep alive. The sample usually lands before the evaluation, but nothing orders the two, and a pair with no round trip time yet postpones it to the next sampling tick. Latency-only switches now require a sample on both peers. Categorical criteria (connected, metric, direct versus relayed) are unaffected. * [client] Hold a never-direct HA routing peer before a direct peer takes over The peers of an HA group come up over relay first and turn direct one after the other, as when lazy connections wake the group or the client reconnects. A routing peer that was relayed when it carried the route gave way at once to the first equivalent peer that turned direct, so in a group of N peers the route moved right after the session started in roughly (N-1)/N of the cases, resetting every masqueraded flow, typically an SSH session opened on the wake-up traffic. Such a peer is now kept for the relayedSwitch delay, counted from the first evaluation that finds a direct contender. If it turns direct within the delay the route stays; otherwise it moves when the delay runs out, so the route does not stay on a relay for longer than the delay. A peer that dropped from direct keeps counting from the drop. * [client] Keep allowed IPs when only the chosen routing peer's status changes recalculateRoutes did not tell a status change of the chosen routing peer (idle to connected when traffic wakes it, connected to relayed and back) from a switch to another peer: it removed the allowed IPs and added them back on the same peer. In between the prefix had no owner and packets for it were dropped, and default routes published a spurious "disconnected due to high availability change" event followed by two "connected" ones. When the same route stays on the same peer with the same definition, only the recorded status is updated now. * [client] Limit the "never" relayed switch policy to peers that dropped from direct NB_HA_RELAYED_SWITCH_DELAY=never keeps a routing peer whose direct connection dropped to relay for as long as it stays connected. Once the relayed delay also covered peers that were never direct, "never" pinned those to the relay for good, although a direct equivalent was available and the peer showed no sign of turning direct. A peer that was never direct now uses the default delay under "never", which keeps the policy's original scope. * [client] Merge the unchanged-route checks of the HA route watcher shouldSkipRecalculation and the status-only fast path in recalculateRoutes both handled the same route staying on the same routing peer, one for an unchanged status and one for a changed one. They are now a single keepChosenRoute check. Pure refactor, no behavior change. * [client] Re-apply routed allowed IPs when a kept routing peer connects A peer update can recreate a routing peer: modifyPeers removes it from the interface, which drops its routed prefixes, and adds it back idle with its overlay addresses only. The remove and re-add that recalculateRoutes used to run on every status change of the chosen peer put the prefixes back as a side effect once the peer connected. Without it, a watcher that sees the same peer go idle and then connected leaves the routed subnet without an owner until a route update or an HA switch. When the chosen peer connects while keeping the route, the watcher now re-applies its routed allowed IPs through the route manager's reconcile, which is add-only and idempotent. Lazy connections already do the same when they arm an idle peer. * [client] Bound the HA route hold on a routing peer without a latency sample A latency-only switch waited for a sample on both peers. If the current peer never got one, because its candidate pair stats kept failing or reporting no round trip time, it kept the route indefinitely against a measured peer, however much faster that one was. The hold on an unsampled current peer now lasts unmeasuredLatencyHold (30s), long enough for a recovered direct connection to report its first sample. After that the peer is ranked at unknownLatency again and the usual switch margins apply. relayedHold is armed so the switch is reconsidered when the hold runs out. * [client] Restart the relayed delay of a never-direct peer when its contender goes For a routing peer that was never direct, the relayed delay runs from the first evaluation that finds a direct contender. It was only reset when the current peer changed or turned direct, so if that contender went away and another one showed up later, the delay counted from the first one and the new contender took over at once. The delay of such a peer now restarts once no direct contender is left. The delay of a peer that dropped from direct keeps running from the drop. * [client] Retrigger pull request synchronization * [client] Retry a failed routed allowed IPs reconcile on the next evaluation keepChosenRoute recorded the connected status before re-applying the routed allowed IPs and only logged a failed reconcile. Later evaluations then saw an unchanged status and never retried, so a transient AddAllowedIP failure left the recreated routing peer without its routed prefixes until another status transition or a route switch. The status is now recorded only once the reconcile succeeds. A failure leaves the previous status in place, so the next evaluation, at the latest the periodic re-evaluation, sees the change again and retries. --- client/internal/routemanager/client/client.go | 171 ++++++++++++--- .../routemanager/client/client_test.go | 82 +++++++ client/internal/routemanager/client/env.go | 9 +- .../client/relayed_switch_test.go | 204 +++++++++++++++--- client/internal/routemanager/manager.go | 26 +-- 5 files changed, 421 insertions(+), 71 deletions(-) diff --git a/client/internal/routemanager/client/client.go b/client/internal/routemanager/client/client.go index b882c4d4a..66c2ed491 100644 --- a/client/internal/routemanager/client/client.go +++ b/client/internal/routemanager/client/client.go @@ -65,6 +65,12 @@ const ( // well above any usable path so that slow paths keep their order. unknownLatency = 10 * time.Second + // unmeasuredLatencyHold is how long a current routing peer without a + // latency sample is kept against a measured one. A direct connection is + // sampled within seconds, so a peer still unsampled after this is ranked + // at unknownLatency rather than held indefinitely. + unmeasuredLatencyHold = 30 * time.Second + // holdReportInterval is how long a routing peer held back by a switch rule // is reported at debug level only, after being reported at info level once. holdReportInterval = 10 * time.Minute @@ -99,6 +105,7 @@ type holdKind int const ( holdLatencyMargin holdKind = iota holdLatencyDwell + holdLatencyUnmeasured holdRelayedDelay ) @@ -124,6 +131,9 @@ type routeCandidate struct { relayed bool latency time.Duration noise time.Duration + // measured is false while the peer has no latency sample, as right after a + // direct connection comes up: latency then holds unknownLatency + measured bool } func newRouteCandidate(r *route.Route, status routerPeerStatus) routeCandidate { @@ -140,6 +150,7 @@ func newRouteCandidate(r *route.Route, status routerPeerStatus) routeCandidate { relayed: status.relayed, latency: latency, noise: status.noise, + measured: status.latency > 0, } } @@ -230,6 +241,9 @@ type WatcherConfig struct { StatusRecorder *peer.Status Route *route.Route Handler RouteHandler + // ReconcileAllowedIPs re-applies the routed allowed IPs tracked for a + // routing peer onto the interface. Optional. + ReconcileAllowedIPs func(peerKey string) error } // Watcher watches route and peer changes and updates allowed IPs accordingly. @@ -271,11 +285,20 @@ type Watcher struct { trackedCurrent route.ID trackedPeer string trackedDirect bool - // degradedSince is when the current routing peer dropped from direct to - // relayed, zero if it is direct or was already relayed when chosen + // degradedSince is when the relayedSwitch delay of the current relayed + // routing peer started: its drop from direct, or the first direct contender + // if it was never direct. Zero while it is direct or uncontended. degradedSince time.Time - // relayedHold fires when a relayed switch being held back becomes due, so it - // is applied on time rather than on the next periodic re-evaluation + // unmeasuredRoute and unmeasuredSince track since when the current route's + // peer has had no latency sample, bounding the hold in shouldSwitchRoute + unmeasuredRoute route.ID + unmeasuredSince time.Time + // neverDirect marks a degradedSince started by a direct contender rather + // than by a drop from direct, which the "never" policy does not cover + neverDirect bool + // relayedHold fires when a switch held back by the relayed delay or the + // unmeasured latency hold becomes due, so it is applied on time rather than + // on the next periodic re-evaluation relayedHold *time.Timer // heldCandidate and heldKind identify the alternative last held back by the // switch rules and the rule holding it, and heldReported is when that was @@ -298,6 +321,9 @@ type Watcher struct { stopped bool handler RouteHandler updateSerial uint64 + // reconcileAllowedIPs re-applies a routing peer's routed allowed IPs when + // it connects while keeping the route, see keepChosenRoute. May be nil. + reconcileAllowedIPs func(peerKey string) error } func newPeerStateUpdate() chan struct{} { @@ -320,6 +346,7 @@ func NewWatcher(config WatcherConfig) *Watcher { handler: config.Handler, currentChosenStatus: nil, relayedSwitch: relayedSwitch(), + reconcileAllowedIPs: config.ReconcileAllowedIPs, } return client } @@ -393,6 +420,8 @@ func (w *Watcher) getBestRouteFromStatuses(routePeerStatuses map[route.ID]router } w.trackRelayed(current, haveCurrent) + w.trackUnmeasured(current, haveCurrent) + w.trackContender(current, best, haveCurrent && haveBest) if !haveBest { w.reportUnassigned() @@ -449,9 +478,8 @@ func (w *Watcher) reportUnassigned() { // trackRelayed records when the current routing peer dropped from a direct to a // relayed connection while carrying the route, which starts the relayedSwitch -// delay. A peer that was never direct, because it was relayed when chosen or -// came up over relay from idle, is not degraded: holding on to it would only -// keep the route on the slower path. +// delay at once. A peer that was never direct starts it later, when a direct +// peer first contends for the route, see relayedSwitchDue. func (w *Watcher) trackRelayed(current routeCandidate, haveCurrent bool) { if !haveCurrent { w.trackedCurrent, w.trackedPeer, w.trackedDirect, w.degradedSince = "", "", false, time.Time{} @@ -462,7 +490,7 @@ func (w *Watcher) trackRelayed(current routeCandidate, haveCurrent bool) { case current.id != w.trackedCurrent, current.peer != w.trackedPeer, !current.relayed: w.degradedSince = time.Time{} case w.trackedDirect: - w.degradedSince = time.Now() + w.degradedSince, w.neverDirect = time.Now(), false // a new degradation, reported as such even if an earlier one held // back the same candidate w.heldCandidate = "" @@ -494,6 +522,20 @@ func (w *Watcher) shouldSwitchRoute(current, candidate routeCandidate) bool { return true } + // Without a sample on both sides there is no gain to weigh. A direct + // connection reports no latency until its first sample, so comparing it as + // unknownLatency would move the route away from a peer that just recovered. + // The hold is bounded so a peer that never gets a sample does not keep the + // route for good. + if w.unmeasuredHoldActive(current, candidate) { + if level := w.holdLevel(candidate, holdLatencyUnmeasured); log.IsLevelEnabled(level) { + log.StandardLogger().Logf(level, + "keeping routing peer %s for [%v] over %s: latency not measured on both peers yet (%s)", + current.peer, w.handler, candidate.peer, candidate) + } + return false + } + gain := current.latency - candidate.latency if margin := switchMargin(current, candidate); gain < margin { if level := w.holdLevel(candidate, holdLatencyMargin); log.IsLevelEnabled(level) { @@ -518,22 +560,75 @@ func (w *Watcher) shouldSwitchRoute(current, candidate routeCandidate) bool { return true } +// trackContender restarts the relayed delay of a peer that was never direct +// once no direct contender is left. That delay runs from the first contender, +// so keeping it would let a later contender take over without waiting. The +// delay of a peer that dropped from direct keeps running from the drop. +func (w *Watcher) trackContender(current, best routeCandidate, have bool) { + if !w.neverDirect || w.degradedSince.IsZero() { + return + } + if have && best.winsOnlyOnRelay(current) { + return + } + w.degradedSince = time.Time{} +} + +// trackUnmeasured records since when the current route's peer has had no +// latency sample, and forgets it once the peer is measured or the route moves. +func (w *Watcher) trackUnmeasured(current routeCandidate, haveCurrent bool) { + if !haveCurrent || current.measured || current.id != w.unmeasuredRoute { + w.unmeasuredRoute, w.unmeasuredSince = "", time.Time{} + } + if haveCurrent && !current.measured && w.unmeasuredSince.IsZero() { + w.unmeasuredRoute, w.unmeasuredSince = current.id, time.Now() + } +} + +// unmeasuredHoldActive reports whether a latency comparison must wait because +// a side has no sample: always for an unsampled candidate, and for an unsampled +// current peer until unmeasuredLatencyHold runs out. A pending hold arms +// relayedHold so the switch is reconsidered when it runs out. +func (w *Watcher) unmeasuredHoldActive(current, candidate routeCandidate) bool { + if !candidate.measured { + return true + } + if current.measured { + return false + } + + remaining := unmeasuredLatencyHold - time.Since(w.unmeasuredSince) + if remaining <= 0 { + return false + } + w.armRelayedHold(remaining) + + return true +} + // relayedSwitchDue reports whether a relayed current routing peer should give // way to a candidate that is better only by being direct. // -// A peer that dropped from direct to relayed while carrying the route is kept -// for the relayedSwitch delay: the relay still carries the traffic, staying -// keeps every flow through this routing peer alive, and a direct connection that -// dropped often recovers within seconds. A peer that was already relayed when -// chosen gives way at once, so the route is not stuck on the slower path when a -// direct one becomes available. A switch that is not due yet arms relayedHold so -// it is applied as soon as it becomes due. +// A relayed peer is kept for the relayedSwitch delay: the relay still carries +// the traffic, and staying keeps every flow through this routing peer alive. +// For a peer that dropped from direct to relayed the delay runs from the drop, +// since a direct connection that dropped often recovers within seconds. For a +// peer that was never direct, as when the peers of a group come up over relay +// before their direct connections, it runs from the first evaluation that +// finds a direct contender: the current peer usually turns direct within +// seconds too. A switch that is not due yet arms relayedHold so it is applied +// as soon as it becomes due. func (w *Watcher) relayedSwitchDue(current, candidate routeCandidate) bool { if w.degradedSince.IsZero() { - return true + w.degradedSince, w.neverDirect = time.Now(), true } policy := w.relayedSwitch + // "never" keeps a peer whose direct connection dropped; one that was never + // direct still moves after the default delay instead of staying relayed + if policy.never && w.neverDirect { + policy = relayedSwitchPolicy{delay: defaultRelayedSwitchDelay} + } if !policy.never { remaining := policy.delay - time.Since(w.degradedSince) if remaining <= 0 { @@ -544,7 +639,7 @@ func (w *Watcher) relayedSwitchDue(current, candidate routeCandidate) bool { if level := w.holdLevel(candidate, holdRelayedDelay); log.IsLevelEnabled(level) { log.StandardLogger().Logf(level, - "keeping relayed routing peer %s for [%v] over direct %s: dropped to relay %v ago, switch delay %s", + "keeping relayed routing peer %s for [%v] over direct %s: held for %v, switch delay %s", current.peer, w.handler, candidate.peer, time.Since(w.degradedSince).Round(time.Second), policy) } @@ -677,21 +772,46 @@ func (w *Watcher) removeAllowedIPs(route *route.Route, rsn reason) error { return nil } -// shouldSkipRecalculation checks if we can skip route recalculation for the same route without status changes -func (w *Watcher) shouldSkipRecalculation(newChosenID route.ID, newStatus routerPeerStatus) bool { - if w.currentChosen == nil { +// keepChosenRoute reports whether the evaluation keeps the current route, with +// the same definition, on the same routing peer. A status change of that peer +// is recorded without removing and re-adding its allowed IPs, which would leave +// the prefix without an owner in between. When the peer connects, its routed +// allowed IPs are re-applied instead: a peer recreated by a peer update comes +// back with its overlay addresses only. +func (w *Watcher) keepChosenRoute(newChosenID route.ID, newStatus routerPeerStatus) bool { + if w.currentChosen == nil || w.currentChosen.ID != newChosenID { return false } - isSameRoute := w.currentChosen.ID == newChosenID && w.currentChosen.Equal(w.routes[newChosenID]) - if !isSameRoute { + newChosenRoute := w.routes[newChosenID] + if !w.currentChosen.Equal(newChosenRoute) { return false } - if w.currentChosenStatus != nil { - return w.currentChosenStatus.status == newStatus.status + if w.currentChosenStatus == nil || w.currentChosenStatus.status == newStatus.status { + return true } + // a failed reconcile leaves the previous status recorded, so the next + // evaluation sees the change again and retries + if newStatus.status == peer.StatusConnected && !w.reconcileRoutedAllowedIPs(newChosenRoute.Peer) { + return true + } + + w.currentChosen = newChosenRoute + w.currentChosenStatus = &newStatus + + return true +} + +func (w *Watcher) reconcileRoutedAllowedIPs(peerKey string) bool { + if w.reconcileAllowedIPs == nil { + return true + } + if err := w.reconcileAllowedIPs(peerKey); err != nil { + log.Warnf("failed to reconcile routed allowed IPs of routing peer %s for [%v], will retry: %v", peerKey, w.handler, err) + return false + } return true } @@ -719,8 +839,7 @@ func (w *Watcher) recalculateRoutes(rsn reason, routerPeerStatuses map[route.ID] return nil } - // If we can skip recalculation for the same route without changes, do nothing - if w.shouldSkipRecalculation(newChosenID, newStatus) { + if w.keepChosenRoute(newChosenID, newStatus) { return nil } diff --git a/client/internal/routemanager/client/client_test.go b/client/internal/routemanager/client/client_test.go index 764a2f74d..641383e90 100644 --- a/client/internal/routemanager/client/client_test.go +++ b/client/internal/routemanager/client/client_test.go @@ -2,6 +2,7 @@ package client import ( "context" + "errors" "fmt" "net/netip" "sync" @@ -1480,3 +1481,84 @@ func TestLatencyJitterDoesNotSwitchRoute(t *testing.T) { require.Equal(t, route.ID("route1"), chosen, "route changed on jitter at sample %d", i) } } + +// A routing peer that keeps the route while only its connection status changes, +// as when an idle peer is woken by traffic, must keep its allowed IPs in place: +// removing and re-adding them leaves the prefix without an owner in between. +func TestRecalculateRoutesStatusChangeKeepsAllowedIPs(t *testing.T) { + w, handler := newRecalcWatcher(t) + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusIdle}, + })) + require.Equal(t, []string{"peer1"}, handler.added) + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + })) + assert.Equal(t, []string{"peer1"}, handler.added, "a status change must not re-add the allowed IPs") + assert.Equal(t, 0, handler.removed, "a status change must not remove the allowed IPs") + require.NotNil(t, w.currentChosenStatus) + assert.Equal(t, peer.StatusConnected, w.currentChosenStatus.status, "the new status must be recorded") +} + +// A routing peer recreated by a peer update comes back idle with its overlay +// addresses only. When it connects while keeping the route, its routed allowed +// IPs must be re-applied, since nothing else puts them back on the new peer. +func TestRecalculateRoutesReconcilesAllowedIPsOnConnect(t *testing.T) { + w, handler := newRecalcWatcher(t) + var reconciled []string + w.reconcileAllowedIPs = func(peerKey string) error { + reconciled = append(reconciled, peerKey) + return nil + } + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected}, + })) + require.Equal(t, []string{"peer1"}, handler.added) + require.Empty(t, reconciled, "a fresh assignment adds the allowed IPs itself") + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusIdle}, + })) + assert.Empty(t, reconciled, "going idle must not reconcile") + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected}, + })) + assert.Equal(t, []string{"peer1"}, reconciled, "connecting while keeping the route must reconcile its allowed IPs") + assert.Equal(t, []string{"peer1"}, handler.added, "the reconcile must not go through the refcounted add") + assert.Equal(t, 0, handler.removed, "the allowed IPs must not be removed") +} + +// A failed reconcile must be retried: the new status is only recorded once the +// routed allowed IPs are back, so the next evaluation tries again. +func TestRecalculateRoutesRetriesFailedReconcile(t *testing.T) { + w, _ := newRecalcWatcher(t) + calls := 0 + w.reconcileAllowedIPs = func(string) error { + calls++ + if calls == 1 { + return errors.New("add allowed IP: transient failure") + } + return nil + } + + connected := map[route.ID]routerPeerStatus{"route1": {status: peer.StatusConnected}} + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, connected)) + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusIdle}, + })) + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, connected)) + require.Equal(t, 1, calls, "connecting must reconcile") + assert.Equal(t, peer.StatusIdle, w.currentChosenStatus.status, "a failed reconcile must not record the new status") + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, connected)) + assert.Equal(t, 2, calls, "the next evaluation must retry the reconcile") + assert.Equal(t, peer.StatusConnected, w.currentChosenStatus.status, "a successful reconcile records the status") + + require.NoError(t, w.recalculateRoutes(reasonPeerUpdate, connected)) + assert.Equal(t, 2, calls, "nothing to retry once the reconcile succeeded") +} diff --git a/client/internal/routemanager/client/env.go b/client/internal/routemanager/client/env.go index 398667cdf..6a7c08a1f 100644 --- a/client/internal/routemanager/client/env.go +++ b/client/internal/routemanager/client/env.go @@ -12,10 +12,11 @@ import ( const ( // envRelayedSwitchDelay sets how long a routing peer that dropped from a // direct to a relayed connection while carrying the route is kept before the - // route moves to an equivalent peer with a direct connection. Takes a - // duration ("30s", "0" to switch at once) or "never" to only leave such a - // peer when it disconnects. A routing peer that was already relayed when - // chosen always gives way to a direct one at once. + // route moves to an equivalent peer with a direct connection. A routing peer + // that was never direct is kept for the same delay from when a direct peer + // first contends for the route. Takes a duration ("30s", "0" to switch at + // once) or "never" to only leave a peer that dropped from direct when it + // disconnects; a peer that was never direct then uses the default delay. envRelayedSwitchDelay = "NB_HA_RELAYED_SWITCH_DELAY" relayedSwitchDelayNever = "never" diff --git a/client/internal/routemanager/client/relayed_switch_test.go b/client/internal/routemanager/client/relayed_switch_test.go index 8f4a67bba..f70dfff50 100644 --- a/client/internal/routemanager/client/relayed_switch_test.go +++ b/client/internal/routemanager/client/relayed_switch_test.go @@ -103,11 +103,11 @@ func TestDegradedRoutingPeerIsReplacedAfterDelay(t *testing.T) { assert.Equal(t, switchReasonDirect, w.switchReason, "the switch reason must name the relayed connection") } -// A routing peer that was relayed when it was chosen is not degraded: when an -// equivalent direct peer shows up the route must upgrade at once instead of -// staying on the slower path for the delay. -func TestRelayedRoutingPeerUpgradesAtOnce(t *testing.T) { - w := newRelayedTestWatcher(t, relayedSwitchPolicy{never: true}) +// A routing peer that was relayed when it was chosen is held for the delay when +// an equivalent direct peer shows up, from that moment on: it usually turns +// direct within seconds, and moving the route resets its masqueraded flows. +func TestRelayedRoutingPeerIsKeptDuringDelay(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) // route1 was chosen while relayed and route2 was still connecting chosen, _ := w.getBestRouteFromStatuses(map[route.ID]routerPeerStatus{ @@ -115,36 +115,57 @@ func TestRelayedRoutingPeerUpgradesAtOnce(t *testing.T) { "route2": {status: peer.StatusConnecting}, }) require.Equal(t, route.ID("route1"), chosen) - require.True(t, w.degradedSince.IsZero(), "a peer relayed from the start is not degraded") + require.True(t, w.degradedSince.IsZero(), "the delay must not run before a direct peer contends") chosen, _ = w.getBestRouteFromStatuses(currentRelayed) - assert.Equal(t, route.ID("route2"), chosen, "a direct peer must take over from one that was relayed all along") - assert.Equal(t, switchReasonDirect, w.switchReason) - assert.Nil(t, w.relayedHold, "an upgrade must not schedule anything") + assert.Equal(t, route.ID("route1"), chosen, "a relayed peer must be kept when a direct one shows up") + assert.False(t, w.degradedSince.IsZero(), "the direct contender must start the delay") + assert.NotNil(t, w.relayedHold, "the pending switch must arm a timer so it is applied when due") + + w.degradedSince = w.degradedSince.Add(-31 * time.Second) + chosen, _ = w.getBestRouteFromStatuses(currentRelayed) + assert.Equal(t, route.ID("route2"), chosen, "a peer still relayed after the delay must give way to a direct one") + assert.Equal(t, switchReasonDirect, w.switchReason, "the switch reason must name the relayed connection") } -// An idle routing peer woken by traffic whose relay connection comes up before -// the direct one was never direct: it is not degraded, and must give way to an -// equivalent direct peer at once. -func TestIdleRoutingPeerWokenOverRelayUpgradesAtOnce(t *testing.T) { - w := newRelayedTestWatcher(t, relayedSwitchPolicy{never: true}) +// The peers of an HA group woken from idle come up over relay first and turn +// direct one after the other. The route must stay on the peer it was on when +// the other one turns direct first and the current one follows within the +// delay, including before the current peer's first latency sample. +func TestIdleRoutingPeersWokenOverRelayKeepRoute(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) - chosen, _ := w.getBestRouteFromStatuses(map[route.ID]routerPeerStatus{ - "route1": {status: peer.StatusIdle}, - "route2": {status: peer.StatusIdle}, - }) - require.Equal(t, route.ID("route1"), chosen) + steps := []struct { + name string + statuses map[route.ID]routerPeerStatus + }{ + {"both idle", map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusIdle}, + "route2": {status: peer.StatusIdle}, + }}, + {"both relayed", map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + "route2": {status: peer.StatusConnected, relayed: true}, + }}, + {"other direct first", map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + "route2": {status: peer.StatusConnected, latency: 20 * time.Millisecond}, + }}, + {"current direct, not sampled", map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected}, + "route2": {status: peer.StatusConnected, latency: 20 * time.Millisecond}, + }}, + {"current direct, sampled", map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, latency: 38 * time.Millisecond}, + "route2": {status: peer.StatusConnected, latency: 20 * time.Millisecond}, + }}, + } - chosen, _ = w.getBestRouteFromStatuses(map[route.ID]routerPeerStatus{ - "route1": {status: peer.StatusConnected, relayed: true}, - "route2": {status: peer.StatusConnecting}, - }) - require.Equal(t, route.ID("route1"), chosen) - require.True(t, w.degradedSince.IsZero(), "waking up over relay is not a drop from direct") - - chosen, _ = w.getBestRouteFromStatuses(currentRelayed) - assert.Equal(t, route.ID("route2"), chosen, "a direct peer must take over from one that was never direct") - assert.Nil(t, w.relayedHold, "an upgrade must not schedule anything") + for _, step := range steps { + chosen, _ := w.getBestRouteFromStatuses(step.statuses) + require.Equal(t, route.ID("route1"), chosen, "step %q must keep the route on route1", step.name) + } + assert.True(t, w.degradedSince.IsZero(), "the delay must end once the current peer is direct") } // A routing peer that drops to relay again, after recovering from an earlier @@ -356,3 +377,128 @@ func TestRoutingPeerSwitchPublishesEvent(t *testing.T) { assert.Equal(t, "No routing peer available", events[1].Message) assert.Equal(t, "peer2", events[1].Metadata["previous_peer"]) } + +// The statuses below follow the status recorder: a drop to relay clears the +// latency, and a recovered direct connection reports no latency until the new +// ICE agent's first sample lands. +var ( + flapDirect = map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, latency: 50 * time.Millisecond}, + "route2": {status: peer.StatusConnected, latency: 60 * time.Millisecond}, + } + flapRelayed = map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + "route2": {status: peer.StatusConnected, latency: 60 * time.Millisecond}, + } + flapRecoveredUnsampled = map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected}, + "route2": {status: peer.StatusConnected, latency: 60 * time.Millisecond}, + } +) + +// A routing peer whose direct connection drops to relay and comes back within +// the delay must keep the route, including the window between the recovery and +// the first latency sample of the new connection. +func TestRecoveredRoutingPeerWithoutLatencySampleIsKept(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) + + chosen, _ := w.getBestRouteFromStatuses(flapDirect) + require.Equal(t, route.ID("route1"), chosen) + + chosen, _ = w.getBestRouteFromStatuses(flapRelayed) + require.Equal(t, route.ID("route1"), chosen, "a routing peer that just dropped to relay must be kept") + + chosen, _ = w.getBestRouteFromStatuses(flapRecoveredUnsampled) + assert.Equal(t, route.ID("route1"), chosen, "a recovered routing peer must not lose the route before its latency is sampled") +} + +// A routing peer that keeps dropping to relay for less than the delay, and +// staying direct for less than the delay in between, must keep the route on +// every cycle: each drop is a new degradation with its own delay. +func TestFlappingRoutingPeerIsKept(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) + + chosen, _ := w.getBestRouteFromStatuses(flapDirect) + require.Equal(t, route.ID("route1"), chosen) + + for cycle := 0; cycle < 3; cycle++ { + chosen, _ = w.getBestRouteFromStatuses(flapRelayed) + require.Equal(t, route.ID("route1"), chosen, "cycle %d: drop to relay", cycle) + + // relayed for 25s, still within the delay + w.degradedSince = w.degradedSince.Add(-25 * time.Second) + chosen, _ = w.getBestRouteFromStatuses(flapRelayed) + require.Equal(t, route.ID("route1"), chosen, "cycle %d: relayed within the delay", cycle) + + chosen, _ = w.getBestRouteFromStatuses(flapRecoveredUnsampled) + require.Equal(t, route.ID("route1"), chosen, "cycle %d: recovered, latency not sampled yet", cycle) + + chosen, _ = w.getBestRouteFromStatuses(flapDirect) + require.Equal(t, route.ID("route1"), chosen, "cycle %d: recovered and sampled", cycle) + } +} + +// "never" only keeps a routing peer that dropped from direct. A peer that was +// never direct must still move to a direct one after the default delay, so the +// route is not left on a relay for good. +func TestNeverPolicyDoesNotPinNeverDirectPeer(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{never: true}) + + chosen, _ := w.getBestRouteFromStatuses(map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + "route2": {status: peer.StatusConnecting}, + }) + require.Equal(t, route.ID("route1"), chosen) + + chosen, _ = w.getBestRouteFromStatuses(currentRelayed) + require.Equal(t, route.ID("route1"), chosen, "a never-direct peer must be held for the default delay") + require.NotNil(t, w.relayedHold, "the default delay must arm the timer") + + w.degradedSince = w.degradedSince.Add(-defaultRelayedSwitchDelay - time.Second) + chosen, _ = w.getBestRouteFromStatuses(currentRelayed) + assert.Equal(t, route.ID("route2"), chosen, "with never, a never-direct peer must still give way after the default delay") +} + +// A current peer that never gets a latency sample must not keep the route +// forever against a measured peer: once the hold runs out it is ranked as +// unknown latency again. +func TestUnmeasuredRoutingPeerHoldIsBounded(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) + + chosen, _ := w.getBestRouteFromStatuses(flapRecoveredUnsampled) + require.Equal(t, route.ID("route1"), chosen, "an unsampled current peer must be held at first") + require.False(t, w.unmeasuredSince.IsZero(), "the hold must record when it started") + + w.unmeasuredSince = w.unmeasuredSince.Add(-unmeasuredLatencyHold - time.Second) + chosen, _ = w.getBestRouteFromStatuses(flapRecoveredUnsampled) + assert.Equal(t, route.ID("route2"), chosen, "a peer still unsampled after the hold must give way to a measured one") + assert.Equal(t, switchReasonLatency, w.switchReason) +} + +// The delay of a peer that was never direct runs from the first direct +// contender. When that contender goes away the delay must start over, so a +// later contender is held for the full delay instead of taking over at once. +func TestNeverDirectDelayRestartsWhenContenderGoes(t *testing.T) { + w := newRelayedTestWatcher(t, relayedSwitchPolicy{delay: 30 * time.Second}) + + relayedAlone := map[route.ID]routerPeerStatus{ + "route1": {status: peer.StatusConnected, relayed: true}, + "route2": {status: peer.StatusConnecting}, + } + chosen, _ := w.getBestRouteFromStatuses(relayedAlone) + require.Equal(t, route.ID("route1"), chosen) + + chosen, _ = w.getBestRouteFromStatuses(currentRelayed) + require.Equal(t, route.ID("route1"), chosen, "the first direct contender must be held") + require.False(t, w.degradedSince.IsZero()) + + // the first contender was seen longer ago than the delay + w.degradedSince = w.degradedSince.Add(-31 * time.Second) + + chosen, _ = w.getBestRouteFromStatuses(relayedAlone) + require.Equal(t, route.ID("route1"), chosen) + assert.True(t, w.degradedSince.IsZero(), "the delay must start over once no direct contender is left") + + chosen, _ = w.getBestRouteFromStatuses(currentRelayed) + assert.Equal(t, route.ID("route1"), chosen, "a new direct contender must be held for the full delay") +} diff --git a/client/internal/routemanager/manager.go b/client/internal/routemanager/manager.go index 981b0c987..7d699fbb4 100644 --- a/client/internal/routemanager/manager.go +++ b/client/internal/routemanager/manager.go @@ -578,12 +578,13 @@ func (m *DefaultManager) TriggerSelection(networks route.HAMap) { } config := client.WatcherConfig{ - Context: m.ctx, - DNSRouteInterval: m.dnsRouteInterval, - WGInterface: m.wgInterface, - StatusRecorder: m.statusRecorder, - Route: routes[0], - Handler: handler, + Context: m.ctx, + DNSRouteInterval: m.dnsRouteInterval, + WGInterface: m.wgInterface, + StatusRecorder: m.statusRecorder, + Route: routes[0], + Handler: handler, + ReconcileAllowedIPs: m.ReconcilePeerAllowedIPs, } clientNetworkWatcher := client.NewWatcher(config) m.clientNetworks[id] = clientNetworkWatcher @@ -628,12 +629,13 @@ func (m *DefaultManager) updateClientNetworks(updateSerial uint64, networks rout } config := client.WatcherConfig{ - Context: m.ctx, - DNSRouteInterval: m.dnsRouteInterval, - WGInterface: m.wgInterface, - StatusRecorder: m.statusRecorder, - Route: routes[0], - Handler: handler, + Context: m.ctx, + DNSRouteInterval: m.dnsRouteInterval, + WGInterface: m.wgInterface, + StatusRecorder: m.statusRecorder, + Route: routes[0], + Handler: handler, + ReconcileAllowedIPs: m.ReconcilePeerAllowedIPs, } clientNetworkWatcher = client.NewWatcher(config) m.clientNetworks[id] = clientNetworkWatcher From 9d837c6ae672c93fd23d40dafb615541d4bd6b2e Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Thu, 15 Oct 2026 19:14:26 +0900 Subject: [PATCH 04/10] [client, management] Fix the edge cases left open in the certificate posture check (#8174) --- .github/workflows/release.yml | 2 + .goreleaser.yaml | 32 ++++- client/internal/certproof/collect.go | 14 ++- client/internal/certproof/collect_darwin.go | 8 +- client/internal/certproof/collect_test.go | 18 +-- client/internal/certproof/collect_windows.go | 6 +- .../internal/certproof/consoleuser_darwin.go | 4 +- .../certproof/consoleuser_darwin_test.go | 4 +- .../internal/certproof/desktopuser_windows.go | 12 +- .../certproof/desktopuser_windows_test.go | 8 +- client/internal/certproof/helper_spawn.go | 45 ------- .../internal/certproof/helper_spawn_test.go | 27 ---- client/internal/certproof/helper_test.go | 11 +- client/internal/certproof/keychain_darwin.go | 27 ++-- client/internal/certproof/keystore_test.go | 6 +- client/internal/certproof/merge.go | 50 ++++++++ client/internal/certproof/merge_test.go | 54 ++++++++ client/internal/certproof/pkcs11store.go | 51 ++++---- client/internal/certproof/pkcs11store_test.go | 33 +++-- client/internal/certproof/signattempts.go | 24 ++++ .../internal/certproof/signattempts_test.go | 41 +++++++ client/internal/certproof/store.go | 12 +- client/internal/certproof/store_other.go | 6 +- client/internal/certproof/store_other_test.go | 10 +- client/internal/certproof/store_tpm_test.go | 4 +- .../internal/certproof/storefile_unix_test.go | 2 +- .../internal/certproof/systemstore_windows.go | 22 ++-- client/internal/pkcs11/driver_linux.go | 69 ++++++++--- client/internal/pkcs11/driver_linux_test.go | 116 ++++++++++++++++++ client/internal/pkcs11/pkcs11.go | 1 + client/internal/pkcs11/publickey.go | 8 +- client/internal/pkcs11/publickey_test.go | 16 +++ client/internal/pkcs11/uri.go | 57 ++++++++- client/internal/pkcs11/uri_test.go | 56 +++++++++ .../network_map_db/shared_types_test.go | 22 ++++ .../certposture/certposture_test.go | 21 ++++ shared/management/client/grpc_test.go | 24 ++++ shared/management/http/api/openapi.yml | 1 + 38 files changed, 718 insertions(+), 206 deletions(-) create mode 100644 client/internal/certproof/merge.go create mode 100644 client/internal/certproof/merge_test.go create mode 100644 client/internal/certproof/signattempts.go create mode 100644 client/internal/certproof/signattempts_test.go diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index b3ed4763c..22ab7f566 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -417,6 +417,8 @@ jobs: path: | dist/netbird_linux** dist/netbird-pkcs11_linux** + dist/netbird-deb-32bit_linux** + dist/netbird-rpm-*_linux** retention-days: 7 - name: upload windows packages id: upload_windows_packages diff --git a/.goreleaser.yaml b/.goreleaser.yaml index de5f7419c..41c330a64 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -40,8 +40,9 @@ builds: tags: - load_wgnt_from_rsrc - # deb and rpm packages target glibc distributions, so they carry the PKCS#11 store, which - # links libc. Tarballs and the Alpine-based images keep the static build above. + # deb and rpm packages target glibc distributions, so on amd64 and arm64, the only + # architectures with a PKCS#11 driver, they carry the PKCS#11 store, which links libc. + # Tarballs, the Alpine-based images and the other architectures keep the static build. - id: netbird-pkcs11 dir: client binary: netbird @@ -49,10 +50,8 @@ builds: goos: - linux goarch: - - arm - amd64 - arm64 - - 386 ldflags: - -s -w -X github.com/netbirdio/netbird/version.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.CommitDate}} -X main.builtBy=goreleaser mod_timestamp: "{{ .CommitTimestamp }}" @@ -60,8 +59,24 @@ builds: - load_wgnt_from_rsrc - pkcs11 - # Single-arch builds: nfpm provides is not templated, so the RPM splits per arch. They - # carry the PKCS#11 store like the deb build above. + # The 32-bit deb architectures, which have no PKCS#11 driver. + - id: netbird-deb-32bit + dir: client + binary: netbird + env: [CGO_ENABLED=0] + goos: + - linux + goarch: + - arm + - 386 + ldflags: + - -s -w -X github.com/netbirdio/netbird/version.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.CommitDate}} -X main.builtBy=goreleaser + mod_timestamp: "{{ .CommitTimestamp }}" + tags: + - load_wgnt_from_rsrc + + # Single-arch builds: nfpm provides is not templated, so the RPM splits per arch. The + # ones with a PKCS#11 driver carry the store like the deb build above. - &netbird_rpm_build id: netbird-rpm-amd64 dir: client @@ -83,10 +98,14 @@ builds: - <<: *netbird_rpm_build id: netbird-rpm-arm goarch: [arm] + tags: + - load_wgnt_from_rsrc - <<: *netbird_rpm_build id: netbird-rpm-386 goarch: [386] + tags: + - load_wgnt_from_rsrc - id: netbird-static dir: client @@ -323,6 +342,7 @@ nfpms: bindir: /usr/bin builds: - netbird-pkcs11 + - netbird-deb-32bit formats: - deb scripts: diff --git a/client/internal/certproof/collect.go b/client/internal/certproof/collect.go index 65affca71..310fd2b19 100644 --- a/client/internal/certproof/collect.go +++ b/client/internal/certproof/collect.go @@ -149,9 +149,19 @@ func prove(signer crypto.Signer, chain []*x509.Certificate, nonce, peerKey []byt } func chainFingerprint(chain []*x509.Certificate) [sha256.Size]byte { - buf := make([]byte, 0, len(chain)*sha256.Size) + ders := make([][]byte, 0, len(chain)) for _, cert := range chain { - certHash := sha256.Sum256(cert.Raw) + ders = append(ders, cert.Raw) + } + return chainDERFingerprint(ders) +} + +// chainDERFingerprint identifies a chain of DER certificates by the hashes of all of them, +// in order. +func chainDERFingerprint(chain [][]byte) [sha256.Size]byte { + buf := make([]byte, 0, len(chain)*sha256.Size) + for _, der := range chain { + certHash := sha256.Sum256(der) buf = append(buf, certHash[:]...) } return sha256.Sum256(buf) diff --git a/client/internal/certproof/collect_darwin.go b/client/internal/certproof/collect_darwin.go index 92f0471e7..cc21563c1 100644 --- a/client/internal/certproof/collect_darwin.go +++ b/client/internal/certproof/collect_darwin.go @@ -60,7 +60,7 @@ func UserContext(cfg Config) string { if os.Geteuid() != 0 || cfg.OwnerUnknown { return "" } - user, ok := CurrentConsoleUser() + user, ok := currentConsoleUser() if !ok || !user.isOwner(cfg.ProfileOwner) { return "" } @@ -72,7 +72,7 @@ func UserContext(cfg Config) string { // per-session securityd, so the helper has to enter their Mach bootstrap namespace, // which is what launchctl asuser does. func collectAsConsoleUser(ctx context.Context, owner string, challenges []*proto.CertificateChallenge, peerKey []byte) ([]certposture.Proof, error) { - user, ok := CurrentConsoleUser() + user, ok := currentConsoleUser() if !ok { return nil, nil } @@ -126,12 +126,12 @@ func helperStore() Store { home, err := os.UserHomeDir() if err != nil { log.Debugf("certificate posture: no home directory, searching the default keychain list: %v", err) - return NewKeychainStore() + return newKeychainStore() } login := filepath.Join(home, "Library", "Keychains", "login.keychain-db") if _, err := os.Stat(login); err != nil { // Keychains created before macOS 10.12 keep the old file name. login = filepath.Join(home, "Library", "Keychains", "login.keychain") } - return NewKeychainStore(login) + return newKeychainStore(login) } diff --git a/client/internal/certproof/collect_test.go b/client/internal/certproof/collect_test.go index e16ee3668..aa033dea5 100644 --- a/client/internal/certproof/collect_test.go +++ b/client/internal/certproof/collect_test.go @@ -51,7 +51,7 @@ func TestCollect_ProvesOneMatchingCertificatePerChallenge(t *testing.T) { challenge("not a pem"), } - proofs := Collect(context.Background(), NewFileStore(dir), checks, peerKey) + proofs := Collect(context.Background(), newFileStore(dir), checks, peerKey) require.Len(t, proofs, 2) var subjects []string @@ -74,10 +74,10 @@ func TestCollect_NothingToProve(t *testing.T) { store Store checks []*proto.Checks }{ - {"no checks", NewFileStore(dir), nil}, - {"files only", NewFileStore(dir), []*proto.Checks{{Files: []string{"/bin/x"}}}}, - {"challenge without nonce", NewFileStore(dir), []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{CaCertificates: []string{ca.PEM}}}}}, - {"missing store dir", NewFileStore(filepath.Join(dir, "missing")), []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{Nonce: make([]byte, certposture.NonceSize), CaCertificates: []string{ca.PEM}}}}}, + {"no checks", newFileStore(dir), nil}, + {"files only", newFileStore(dir), []*proto.Checks{{Files: []string{"/bin/x"}}}}, + {"challenge without nonce", newFileStore(dir), []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{CaCertificates: []string{ca.PEM}}}}}, + {"missing store dir", newFileStore(filepath.Join(dir, "missing")), []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{Nonce: make([]byte, certposture.NonceSize), CaCertificates: []string{ca.PEM}}}}}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -95,7 +95,7 @@ func TestFileStore_ChainWithIntermediate(t *testing.T) { dir := storeDir(t) writeFile(t, dir, "device.pem", certtest.CertPEM(leaf)+certtest.CertPEM(intermediate.Cert)+certtest.KeyPEM(t, key)) - candidates, err := NewFileStore(dir).Candidates(context.Background()) + candidates, err := newFileStore(dir).Candidates(context.Background()) require.NoError(t, err) require.Len(t, candidates, 1) require.Len(t, candidates[0].Chain, 2) @@ -124,7 +124,7 @@ func TestCollectChallenges_RefusesMalformedInput(t *testing.T) { dir := storeDir(t) key := certtest.ECDSAKey(t) writeFile(t, dir, "device.pem", certtest.CertPEM(ca.Issue(t, key, "device"))+certtest.KeyPEM(t, key)) - store := NewFileStore(dir) + store := newFileStore(dir) nonce := certposture.NewChallenger([]byte("secret")).Nonce(peerKey, time.Now()) tests := []struct { @@ -159,7 +159,7 @@ func TestFileStore_SkipsKeyOfAnotherCertificate(t *testing.T) { good := ca.Issue(t, goodKey, "good") writeFile(t, dir, "b-good.pem", certtest.CertPEM(good)+certtest.KeyPEM(t, goodKey)) - candidates, err := NewFileStore(dir).Candidates(context.Background()) + candidates, err := newFileStore(dir).Candidates(context.Background()) require.NoError(t, err) require.Len(t, candidates, 1, "only the certificate whose key matches is a candidate") assert.True(t, good.Equal(candidates[0].Chain[0]), "the matching pair is kept") @@ -167,7 +167,7 @@ func TestFileStore_SkipsKeyOfAnotherCertificate(t *testing.T) { challenger := certposture.NewChallenger([]byte("secret")) now := time.Now() nonce := challenger.Nonce(peerKey, now) - proofs := CollectChallenges(context.Background(), NewFileStore(dir), []*proto.CertificateChallenge{{Nonce: nonce, CaCertificates: []string{ca.PEM}}}, peerKey) + proofs := CollectChallenges(context.Background(), newFileStore(dir), []*proto.CertificateChallenge{{Nonce: nonce, CaCertificates: []string{ca.PEM}}}, peerKey) require.Len(t, proofs, 1) _, err = challenger.Verify(proofs[0], peerKey, now) assert.NoError(t, err, "the proof sent is one management accepts") diff --git a/client/internal/certproof/collect_windows.go b/client/internal/certproof/collect_windows.go index 8e5695539..3b21ec147 100644 --- a/client/internal/certproof/collect_windows.go +++ b/client/internal/certproof/collect_windows.go @@ -51,7 +51,7 @@ func UserContext(cfg Config) string { if !runningAsLocalSystem() || cfg.OwnerUnknown { return "" } - user, ok := CurrentDesktopUser(cfg.ProfileOwner) + user, ok := currentDesktopUser(cfg.ProfileOwner) if !ok { return "" } @@ -62,14 +62,14 @@ func UserContext(cfg Config) string { // helperStore is the store the helper reads. It runs as the signed-in user, so it wants // that user's store rather than the machine store the service already read. func helperStore() Store { - return NewUserStore() + return newUserStore() } // collectAsDesktopUser runs the helper inside the interactive session of the signed-in // user. Unlike a keychain on macOS, a Windows service can assume a user identity // directly, so the session token goes straight into the child process. func collectAsDesktopUser(ctx context.Context, owner string, challenges []*proto.CertificateChallenge, peerKey []byte) ([]certposture.Proof, error) { - user, ok := CurrentDesktopUser(owner) + user, ok := currentDesktopUser(owner) if !ok { return nil, nil } diff --git a/client/internal/certproof/consoleuser_darwin.go b/client/internal/certproof/consoleuser_darwin.go index a5182c7fd..1045bc014 100644 --- a/client/internal/certproof/consoleuser_darwin.go +++ b/client/internal/certproof/consoleuser_darwin.go @@ -35,10 +35,10 @@ type ConsoleUser struct { GID uint32 } -// CurrentConsoleUser reports the user sitting at the desktop. The second return value is +// currentConsoleUser reports the user sitting at the desktop. The second return value is // false when nobody is: at the login window macOS either reports no console user at all // or attributes the session to root, and neither has a login keychain to offer. -func CurrentConsoleUser() (ConsoleUser, bool) { +func currentConsoleUser() (ConsoleUser, bool) { if err := loadConsoleUser(); err != nil { log.Debugf("console user lookup unavailable: %v", err) return ConsoleUser{}, false diff --git a/client/internal/certproof/consoleuser_darwin_test.go b/client/internal/certproof/consoleuser_darwin_test.go index 9c1aaa22c..051cc4ab0 100644 --- a/client/internal/certproof/consoleuser_darwin_test.go +++ b/client/internal/certproof/consoleuser_darwin_test.go @@ -27,10 +27,10 @@ func TestConsoleUser_OnlyADesktopSessionCanBeValidated(t *testing.T) { } } -// CurrentConsoleUser runs against the real SystemConfiguration framework. A machine with +// currentConsoleUser runs against the real SystemConfiguration framework. A machine with // a desktop open must report a non-root user; a headless runner must report none. func TestCurrentConsoleUser_AgreesWithItself(t *testing.T) { - user, ok := CurrentConsoleUser() + user, ok := currentConsoleUser() if !ok { t.Log("no console user, running headless") return diff --git a/client/internal/certproof/desktopuser_windows.go b/client/internal/certproof/desktopuser_windows.go index 6f50aae4f..0a9d63b36 100644 --- a/client/internal/certproof/desktopuser_windows.go +++ b/client/internal/certproof/desktopuser_windows.go @@ -13,12 +13,8 @@ const ( noActiveSession = 0xFFFFFFFF servicesSession = 0 - // wtsCurrentServer is WTS_CURRENT_SERVER_HANDLE; wtsActive and wtsDisconnected are - // WTSActive and WTSDisconnected of WTS_CONNECTSTATE_CLASS. None is exported by - // x/sys/windows. + // wtsCurrentServer is WTS_CURRENT_SERVER_HANDLE, which x/sys/windows does not export. wtsCurrentServer = windows.Handle(0) - wtsActive = 0 - wtsDisconnected = 4 ) // DesktopUser is an interactive session and the account signed into it. The user's @@ -37,7 +33,7 @@ func (u DesktopUser) Close() { } } -// CurrentDesktopUser returns a token for the session whose certificate store should be +// currentDesktopUser returns a token for the session whose certificate store should be // asked: a session of owner, the account the active profile belongs to, with the // physical console preferred over remote sessions. With no owner only the console user // counts. Picking any signed-in user instead would let whoever else is logged in to a @@ -46,7 +42,7 @@ func (u DesktopUser) Close() { // // Obtaining the token needs SE_TCB_NAME, which the LocalSystem service has and an // ordinary process does not. -func CurrentDesktopUser(owner string) (DesktopUser, bool) { +func currentDesktopUser(owner string) (DesktopUser, bool) { console := windows.WTSGetActiveConsoleSessionId() if owner == "" { return consoleUser(console) @@ -163,7 +159,7 @@ func userSessions(console uint32) ([]uint32, error) { if console != noActiveSession && console != servicesSession { sessions = append(sessions, console) } - for _, state := range []uint32{wtsActive, wtsDisconnected} { + for _, state := range []uint32{windows.WTSActive, windows.WTSDisconnected} { for _, session := range unsafe.Slice(info, count) { if session.SessionID == servicesSession || session.SessionID == console || session.State != state { continue diff --git a/client/internal/certproof/desktopuser_windows_test.go b/client/internal/certproof/desktopuser_windows_test.go index 95d29eb5f..c3c4022e5 100644 --- a/client/internal/certproof/desktopuser_windows_test.go +++ b/client/internal/certproof/desktopuser_windows_test.go @@ -26,10 +26,10 @@ func TestSameAccountName(t *testing.T) { } } -// CurrentDesktopUser against the real session manager: an owner no session belongs to +// currentDesktopUser against the real session manager: an owner no session belongs to // must never yield a session, whoever else is signed in. func TestCurrentDesktopUser_UnknownOwnerHasNoSession(t *testing.T) { - user, ok := CurrentDesktopUser(`NO-SUCH-DOMAIN\no-such-user-netbird`) + user, ok := currentDesktopUser(`NO-SUCH-DOMAIN\no-such-user-netbird`) if ok { user.Close() } @@ -44,14 +44,14 @@ func TestCurrentDesktopUser_FindsTheOwnersSession(t *testing.T) { if owner == "" { t.Skip("set NB_TEST_DESKTOP_OWNER to a signed-in account and run as LocalSystem") } - user, ok := CurrentDesktopUser(owner) + user, ok := currentDesktopUser(owner) require.True(t, ok, "the signed-in owner %s must have a session", owner) defer user.Close() t.Logf("owner %s resolved to session %d as %s", owner, user.Session, user.Name) assert.True(t, sameAccountName(user.Name, owner), "the session found belongs to %s, not %s", owner, user.Name) assert.NotZero(t, user.Session, "session 0 hosts services and never belongs to the owner") - console, consoleOK := CurrentDesktopUser("") + console, consoleOK := currentDesktopUser("") if consoleOK { defer console.Close() t.Logf("without an owner the console user counts: session %d as %s", console.Session, console.Name) diff --git a/client/internal/certproof/helper_spawn.go b/client/internal/certproof/helper_spawn.go index 968cc3679..54ed265ec 100644 --- a/client/internal/certproof/helper_spawn.go +++ b/client/internal/certproof/helper_spawn.go @@ -3,12 +3,6 @@ package certproof import ( - "crypto/sha256" - "crypto/x509" - - log "github.com/sirupsen/logrus" - - "github.com/netbirdio/netbird/shared/management/certposture" "github.com/netbirdio/netbird/shared/management/proto" ) @@ -22,42 +16,3 @@ func helperRequest(challenges []*proto.CertificateChallenge, peerKey []byte) Hel } return req } - -// mergeProofs appends the user session proofs to the device proofs, dropping a leaf that -// both stores hold so the same certificate is proven once. -func mergeProofs(device, user []certposture.Proof) []certposture.Proof { - if len(user) == 0 { - return device - } - - seen := make(map[[sha256.Size]byte]struct{}, len(device)) - for _, proof := range device { - if len(proof.Chain) > 0 { - seen[sha256.Sum256(proof.Chain[0])] = struct{}{} - } - } - - merged := device - for _, proof := range user { - if len(proof.Chain) == 0 { - continue - } - fingerprint := sha256.Sum256(proof.Chain[0]) - if _, done := seen[fingerprint]; done { - continue - } - seen[fingerprint] = struct{}{} - merged = append(merged, proof) - logUserProof(proof) - } - return merged -} - -func logUserProof(proof certposture.Proof) { - leaf, err := x509.ParseCertificate(proof.Chain[0]) - if err != nil { - log.Debugf("certificate posture: user proof carries an unparsable leaf: %v", err) - return - } - log.Debugf("certificate posture: signed-in user proved %q issued by %q", leaf.Subject, leaf.Issuer) -} diff --git a/client/internal/certproof/helper_spawn_test.go b/client/internal/certproof/helper_spawn_test.go index 338a5f66a..74a792a49 100644 --- a/client/internal/certproof/helper_spawn_test.go +++ b/client/internal/certproof/helper_spawn_test.go @@ -3,42 +3,15 @@ package certproof import ( - "crypto/sha256" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/netbirdio/netbird/shared/management/certposture" "github.com/netbirdio/netbird/shared/management/certposture/certtest" "github.com/netbirdio/netbird/shared/management/proto" ) -func TestMergeProofs_ProvesACertificateHeldByBothStoresOnce(t *testing.T) { - ca := certtest.NewCA(t, "corp-root") - shared := ca.Issue(t, certtest.ECDSAKey(t), "shared") - userOnly := ca.Issue(t, certtest.ECDSAKey(t), "user-only") - - device := []certposture.Proof{{Chain: [][]byte{shared.Raw}}} - user := []certposture.Proof{{Chain: [][]byte{shared.Raw}}, {Chain: [][]byte{userOnly.Raw}}, {}} - - merged := mergeProofs(device, user) - - require.Len(t, merged, 2, "the shared leaf is proven once and the empty chain is dropped") - assert.Equal(t, shared.Raw, merged[0].Chain[0], "the device proof keeps its place") - assert.Equal(t, userOnly.Raw, merged[1].Chain[0], "the user-only certificate is appended") -} - -func TestMergeProofs_KeepsDeviceProofsWhenNoUserSession(t *testing.T) { - ca := certtest.NewCA(t, "corp-root") - device := []certposture.Proof{{Chain: [][]byte{ca.Issue(t, certtest.ECDSAKey(t), "device").Raw}}} - - merged := mergeProofs(device, nil) - - require.Len(t, merged, 1, "a machine with nobody signed in still sends its device proof") - assert.Equal(t, sha256.Sum256(device[0].Chain[0]), sha256.Sum256(merged[0].Chain[0]), "the device proof is unchanged") -} - func TestHelperRequest_CarriesEveryChallenge(t *testing.T) { ca := certtest.NewCA(t, "corp-root") challenges := []*proto.CertificateChallenge{ diff --git a/client/internal/certproof/helper_test.go b/client/internal/certproof/helper_test.go index 37dadead2..61fcc8532 100644 --- a/client/internal/certproof/helper_test.go +++ b/client/internal/certproof/helper_test.go @@ -14,7 +14,10 @@ import ( "github.com/netbirdio/netbird/shared/management/certposture/certtest" ) -func TestRunHelper_ProofSurvivesTheProcessBoundary(t *testing.T) { +// TestRunHelper_AnswersAChallengeFromItsStore runs the helper's request and response +// handling in-process: the proof it writes verifies as the daemon will verify it. The +// process boundary itself is covered by the runHelperCmd tests. +func TestRunHelper_AnswersAChallengeFromItsStore(t *testing.T) { ca := certtest.NewCA(t, "corp-root") dir := storeDir(t) key := certtest.ECDSAKey(t) @@ -29,7 +32,7 @@ func TestRunHelper_ProofSurvivesTheProcessBoundary(t *testing.T) { require.NoError(t, err, "request must encode") var stdout bytes.Buffer - require.NoError(t, runHelper(context.Background(), NewFileStore(dir), bytes.NewReader(request), &stdout)) + require.NoError(t, runHelper(context.Background(), newFileStore(dir), bytes.NewReader(request), &stdout)) var resp HelperResponse require.NoError(t, json.Unmarshal(stdout.Bytes(), &resp), "helper must emit decodable JSON") @@ -47,7 +50,7 @@ func TestRunHelper_NoChallengesYieldsEmptyResponse(t *testing.T) { require.NoError(t, err) var stdout bytes.Buffer - require.NoError(t, runHelper(context.Background(), NewFileStore(storeDir(t)), bytes.NewReader(request), &stdout)) + require.NoError(t, runHelper(context.Background(), newFileStore(storeDir(t)), bytes.NewReader(request), &stdout)) var resp HelperResponse require.NoError(t, json.Unmarshal(stdout.Bytes(), &resp), "an empty request must still emit valid JSON") @@ -56,7 +59,7 @@ func TestRunHelper_NoChallengesYieldsEmptyResponse(t *testing.T) { func TestRunHelper_RejectsMalformedRequest(t *testing.T) { var stdout bytes.Buffer - err := runHelper(context.Background(), NewFileStore(storeDir(t)), bytes.NewReader([]byte("not json")), &stdout) + err := runHelper(context.Background(), newFileStore(storeDir(t)), bytes.NewReader([]byte("not json")), &stdout) require.Error(t, err, "a malformed request must fail rather than emit an empty proof set") assert.Empty(t, stdout.String(), "nothing should be written to stdout on a decode failure") diff --git a/client/internal/certproof/keychain_darwin.go b/client/internal/certproof/keychain_darwin.go index 2bffdc81e..5b1426997 100644 --- a/client/internal/certproof/keychain_darwin.go +++ b/client/internal/certproof/keychain_darwin.go @@ -71,25 +71,25 @@ const systemKeychain = "/Library/Keychains/System.keychain" // by an ordinary user, it is that user's keychain search list. func DefaultStore() Store { if os.Geteuid() == 0 { - return NewKeychainStore(systemKeychain) + return newKeychainStore(systemKeychain) } - return NewKeychainStore() + return newKeychainStore() } -// KeychainStore yields the identities of a set of keychains, or of the process's keychain +// keychainStore yields the identities of a set of keychains, or of the process's keychain // search list when none is named, reached through purego so the client keeps building // with CGO_ENABLED=0. -type KeychainStore struct { +type keychainStore struct { keychains []string } -// NewKeychainStore returns a store that searches the keychain files at paths, or the +// newKeychainStore returns a store that searches the keychain files at paths, or the // process's keychain search list when no path is given. -func NewKeychainStore(paths ...string) *KeychainStore { - return &KeychainStore{keychains: paths} +func newKeychainStore(paths ...string) *keychainStore { + return &keychainStore{keychains: paths} } -func (s *KeychainStore) Candidates(_ context.Context) ([]Candidate, error) { +func (s *keychainStore) Candidates(_ context.Context) ([]Candidate, error) { if err := loadKeychain(); err != nil { return nil, err } @@ -163,17 +163,24 @@ func (s *keychainSigner) Sign(_ io.Reader, digest []byte, opts crypto.SignerOpts algorithm := keychainAlgorithm(scheme) var signature []byte + var attempts signAttempts err = eachIdentity(searchList, func(identity uintptr, der []byte) (bool, error) { if !bytes.Equal(der, s.leaf.Raw) { return false, nil } - signature, err = signWithIdentity(identity, algorithm, digest) - return true, err + // The same certificate can sit in the keychain with more than one key, of which + // only one may be usable, so a failure moves on to the next match. + var ok bool + signature, ok = attempts.try(func() ([]byte, error) { return signWithIdentity(identity, algorithm, digest) }) + return ok, nil }) if err != nil { return nil, err } if signature == nil { + if err := attempts.err(); err != nil { + return nil, err + } return nil, errors.New("certificate is no longer in the keychain") } log.Debugf("keychain signed certificate posture challenge for %q, %d bytes", s.leaf.Subject, len(signature)) diff --git a/client/internal/certproof/keystore_test.go b/client/internal/certproof/keystore_test.go index 592d7414f..7d028c4fb 100644 --- a/client/internal/certproof/keystore_test.go +++ b/client/internal/certproof/keystore_test.go @@ -56,6 +56,8 @@ func TestSchemeFor(t *testing.T) { {"ecdsa sha384", ecKey.Public(), crypto.SHA384, schemeECDSASHA384}, {"rsa pss sha256", rsaKey.Public(), pss, schemeRSAPSSSHA256}, {"rsa pkcs1v15", rsaKey.Public(), crypto.SHA256, 0}, + {"ecdsa sha512", ecKey.Public(), crypto.SHA512, 0}, + {"rsa pss sha384", rsaKey.Public(), &rsa.PSSOptions{SaltLength: rsa.PSSSaltLengthEqualsHash, Hash: crypto.SHA384}, 0}, {"ed25519", certtest.Ed25519Key(t).Public(), crypto.Hash(0), 0}, } for _, tc := range tests { @@ -83,5 +85,7 @@ func TestECDSASignatureASN1(t *testing.T) { assert.True(t, ecdsa.VerifyASN1(&key.PublicKey, digest[:], der)) _, err = ecdsaSignatureASN1(raw[:63]) - assert.Error(t, err) + assert.Error(t, err, "an odd length cannot split into r and s") + _, err = ecdsaSignatureASN1(nil) + assert.Error(t, err, "an empty signature is malformed") } diff --git a/client/internal/certproof/merge.go b/client/internal/certproof/merge.go new file mode 100644 index 000000000..d539ce927 --- /dev/null +++ b/client/internal/certproof/merge.go @@ -0,0 +1,50 @@ +package certproof + +import ( + "crypto/sha256" + "crypto/x509" + + log "github.com/sirupsen/logrus" + + "github.com/netbirdio/netbird/shared/management/certposture" +) + +// mergeProofs appends the user session proofs to the device proofs, dropping one whose +// chain the device already proved. Management checks each chain against each check's CAs, +// so the same leaf proven through another chain is kept. +func mergeProofs(device, user []certposture.Proof) []certposture.Proof { + if len(user) == 0 { + return device + } + + seen := make(map[[sha256.Size]byte]struct{}, len(device)+len(user)) + for _, proof := range device { + if len(proof.Chain) > 0 { + seen[chainDERFingerprint(proof.Chain)] = struct{}{} + } + } + + merged := device + for _, proof := range user { + if len(proof.Chain) == 0 { + continue + } + fingerprint := chainDERFingerprint(proof.Chain) + if _, done := seen[fingerprint]; done { + continue + } + seen[fingerprint] = struct{}{} + merged = append(merged, proof) + logUserProof(proof) + } + return merged +} + +func logUserProof(proof certposture.Proof) { + leaf, err := x509.ParseCertificate(proof.Chain[0]) + if err != nil { + log.Debugf("certificate posture: user proof carries an unparsable leaf: %v", err) + return + } + log.Debugf("certificate posture: signed-in user proved %q issued by %q", leaf.Subject, leaf.Issuer) +} diff --git a/client/internal/certproof/merge_test.go b/client/internal/certproof/merge_test.go new file mode 100644 index 000000000..7db2dd421 --- /dev/null +++ b/client/internal/certproof/merge_test.go @@ -0,0 +1,54 @@ +package certproof + +import ( + "crypto/sha256" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/shared/management/certposture" + "github.com/netbirdio/netbird/shared/management/certposture/certtest" +) + +func TestMergeProofs_ProvesACertificateHeldByBothStoresOnce(t *testing.T) { + ca := certtest.NewCA(t, "corp-root") + shared := ca.Issue(t, certtest.ECDSAKey(t), "shared") + userOnly := ca.Issue(t, certtest.ECDSAKey(t), "user-only") + + device := []certposture.Proof{{Chain: [][]byte{shared.Raw}}} + user := []certposture.Proof{{Chain: [][]byte{shared.Raw}}, {Chain: [][]byte{userOnly.Raw}}, {}} + + merged := mergeProofs(device, user) + + require.Len(t, merged, 2, "the shared leaf is proven once and the empty chain is dropped") + assert.Equal(t, shared.Raw, merged[0].Chain[0], "the device proof keeps its place") + assert.Equal(t, userOnly.Raw, merged[1].Chain[0], "the user-only certificate is appended") +} + +func TestMergeProofs_KeepsDeviceProofsWhenNoUserSession(t *testing.T) { + ca := certtest.NewCA(t, "corp-root") + device := []certposture.Proof{{Chain: [][]byte{ca.Issue(t, certtest.ECDSAKey(t), "device").Raw}}} + + merged := mergeProofs(device, nil) + + require.Len(t, merged, 1, "a machine with nobody signed in still sends its device proof") + assert.Equal(t, sha256.Sum256(device[0].Chain[0]), sha256.Sum256(merged[0].Chain[0]), "the device proof is unchanged") +} + +func TestMergeProofs_KeepsAnotherChainOfTheSameLeaf(t *testing.T) { + root := certtest.NewCA(t, "corp-root") + interA := root.Issue(t, certtest.ECDSAKey(t), "inter-a") + interB := root.Issue(t, certtest.ECDSAKey(t), "inter-b") + leaf := root.Issue(t, certtest.ECDSAKey(t), "leaf") + + // Management checks each chain against each check's CAs, so the same leaf proven + // through another chain is a different proof, not a duplicate. + device := []certposture.Proof{{Chain: [][]byte{leaf.Raw, interA.Raw}}} + user := []certposture.Proof{{Chain: [][]byte{leaf.Raw, interB.Raw}}, {Chain: [][]byte{leaf.Raw, interA.Raw}}} + + merged := mergeProofs(device, user) + + require.Len(t, merged, 2, "the chain only the user store holds is kept, the one both hold is not repeated") + assert.Equal(t, interB.Raw, merged[1].Chain[1], "the user's chain through the other intermediate is appended") +} diff --git a/client/internal/certproof/pkcs11store.go b/client/internal/certproof/pkcs11store.go index 1f88e7600..381f55799 100644 --- a/client/internal/certproof/pkcs11store.go +++ b/client/internal/certproof/pkcs11store.go @@ -38,24 +38,24 @@ func PKCS11FromEnv() PKCS11Config { return PKCS11Config{URI: os.Getenv(PKCS11URIEnv), PIN: os.Getenv(PINEnv)} } -// PKCS11Store yields the identities of a PKCS#11 token, which is how tpm2-pkcs11 exposes +// pkcs11Store yields the identities of a PKCS#11 token, which is how tpm2-pkcs11 exposes // TPM-held keys on Linux. Certificates on the token are paired with keys by CKA_ID, the // convention tpm2_ptool addcert and pkcs11-tool follow; certificate files in the PEM // directory by public key. Every signature happens on the token. -type PKCS11Store struct { +type pkcs11Store struct { uri *pkcs11.URI pin string certDir string } -// NewPKCS11Store parses cfg.URI, standing in the bare defaults when it is empty. Files in +// newPKCS11Store parses cfg.URI, standing in the bare defaults when it is empty. Files in // certDir without a key of their own are paired with the token's keys by public key. // // A PIN is only accepted together with a token label: without one the PIN would go to // whichever token the module lists first, which on a machine with a smartcard plugged // in may be the card, and every wrong PIN counts towards that card's lockout. -func NewPKCS11Store(cfg PKCS11Config, certDir string) (*PKCS11Store, error) { - store := &PKCS11Store{uri: &pkcs11.URI{}, pin: cfg.PIN, certDir: certDir} +func newPKCS11Store(cfg PKCS11Config, certDir string) (*pkcs11Store, error) { + store := &pkcs11Store{uri: &pkcs11.URI{}, pin: cfg.PIN, certDir: certDir} if cfg.URI != "" { parsed, err := pkcs11.ParseURI(cfg.URI) if err != nil { @@ -69,7 +69,7 @@ func NewPKCS11Store(cfg PKCS11Config, certDir string) (*PKCS11Store, error) { return store, nil } -func (s *PKCS11Store) Candidates(_ context.Context) ([]Candidate, error) { +func (s *pkcs11Store) Candidates(_ context.Context) ([]Candidate, error) { session, err := s.open() if err != nil { return nil, err @@ -80,7 +80,7 @@ func (s *PKCS11Store) Candidates(_ context.Context) ([]Candidate, error) { if err != nil { return nil, err } - fileChains, err := s.fileChains() + fileChains, fileIntermediates, err := s.fileChains() if err != nil { return nil, err } @@ -93,6 +93,7 @@ func (s *PKCS11Store) Candidates(_ context.Context) ([]Candidate, error) { for _, chain := range fileChains { pool = append(pool, chain...) } + pool = append(pool, fileIntermediates...) var candidates []Candidate for _, cert := range certs { @@ -106,9 +107,12 @@ func (s *PKCS11Store) Candidates(_ context.Context) ([]Candidate, error) { return candidates, nil } + // A token that cannot list its public keys still answers for the certificates it + // holds, so the identities found above are kept. keys, err := tokenPublicKeys(session) if err != nil { - return nil, err + log.Warnf("%s: cannot read its public keys, certificate files are not paired with them: %v", s, err) + return candidates, nil } for _, chain := range fileChains { leaf := chain[0] @@ -122,41 +126,45 @@ func (s *PKCS11Store) Candidates(_ context.Context) ([]Candidate, error) { return candidates, nil } -func (s *PKCS11Store) candidate(leaf *x509.Certificate, id []byte, pool []*x509.Certificate) Candidate { +func (s *pkcs11Store) candidate(leaf *x509.Certificate, id []byte, pool []*x509.Certificate) Candidate { chain := buildChain(leaf, pool) log.Debugf("%s candidate %q issued by %q built a chain of %d certificates", s, leaf.Subject, leaf.Issuer, len(chain)) return Candidate{Chain: chain, Signer: &pkcs11Signer{store: s, leaf: leaf, id: id}, Intermediates: pool} } // fileChains reads the certificate files in the PEM directory that carry no key of their -// own; the file store answers for the ones that do. -func (s *PKCS11Store) fileChains() ([][]*x509.Certificate, error) { +// own; the file store answers for the ones that do. The certificates of a file that does +// carry a key are returned too, since they can complete the chain of a token certificate. +func (s *pkcs11Store) fileChains() (chains [][]*x509.Certificate, intermediates []*x509.Certificate, err error) { if s.certDir == "" { - return nil, nil + return nil, nil, nil } paths, err := certFiles(s.certDir) if err != nil { - return nil, err + return nil, nil, err } - var chains [][]*x509.Certificate for _, path := range paths { chain, signer, err := loadPEM(path) - if err != nil || signer != nil { + if err != nil { + continue + } + if signer != nil { + intermediates = append(intermediates, chain...) continue } chains = append(chains, chain) } - return chains, nil + return chains, intermediates, nil } -func (s *PKCS11Store) String() string { +func (s *pkcs11Store) String() string { if s.uri.Token == "" { return "PKCS#11 token" } return fmt.Sprintf("PKCS#11 token %q", s.uri.Token) } -func (s *PKCS11Store) open() (*pkcs11.Session, error) { +func (s *pkcs11Store) open() (*pkcs11.Session, error) { module, err := pkcs11.Load(s.uri.Module()) if err != nil { return nil, err @@ -185,7 +193,7 @@ func (s *PKCS11Store) open() (*pkcs11.Session, error) { return session, err } -func (s *PKCS11Store) userPIN() ([]byte, error) { +func (s *pkcs11Store) userPIN() ([]byte, error) { if s.pin != "" { return []byte(s.pin), nil } @@ -208,7 +216,8 @@ func tokenPublicKeys(session *pkcs11.Session) (tokenKeys, error) { for _, object := range objects { id, err := session.Attribute(object, pkcs11.AttrID) if err != nil { - return nil, err + log.Debugf("skipping public key on PKCS#11 token: %v", err) + continue } public, err := session.PublicKey(object) if err != nil { @@ -287,7 +296,7 @@ func privateKey(session *pkcs11.Session, id []byte) (pkcs11.Object, error) { // pkcs11Signer holds only the certificate and its CKA_ID; the key is looked up in a fresh // session at signing time so no token handle outlives a call. type pkcs11Signer struct { - store *PKCS11Store + store *pkcs11Store leaf *x509.Certificate id []byte } diff --git a/client/internal/certproof/pkcs11store_test.go b/client/internal/certproof/pkcs11store_test.go index 485c84aae..746b54918 100644 --- a/client/internal/certproof/pkcs11store_test.go +++ b/client/internal/certproof/pkcs11store_test.go @@ -44,7 +44,7 @@ func TestStores_KeepsFileCertificatesWhenTokenFails(t *testing.T) { dir := storeDir(t) writeFile(t, dir, "device.pem", certtest.CertPEM(ca.Issue(t, key, "device"))+certtest.KeyPEM(t, key)) - candidates, err := Stores{failingStore{}, NewFileStore(dir)}.Candidates(context.Background()) + candidates, err := Stores{failingStore{}, newFileStore(dir)}.Candidates(context.Background()) require.NoError(t, err) assert.Len(t, candidates, 1, "the directory's certificate must survive a failing token") } @@ -241,13 +241,13 @@ func attr(typ uint, value []byte) pkcs11.Attribute { // pkcs11TestStore builds the store for the token NB_TEST_PKCS11_URI names, skipping when // no token is configured or this build lacks PKCS#11 support. -func pkcs11TestStore(t *testing.T, certDir string) (*PKCS11Store, string) { +func pkcs11TestStore(t *testing.T, certDir string) (*pkcs11Store, string) { t.Helper() uri := os.Getenv(testPKCS11URIEnv) if uri == "" { t.Skipf("set %s to a PKCS#11 URI with a PIN to run", testPKCS11URIEnv) } - store, err := NewPKCS11Store(PKCS11Config{URI: uri}, certDir) + store, err := newPKCS11Store(PKCS11Config{URI: uri}, certDir) require.NoError(t, err) if _, err := pkcs11.Load(store.uri.Module()); errors.Is(err, pkcs11.ErrUnsupported) { t.Skip(err) @@ -297,17 +297,30 @@ func TestPKCS11Store_FileChains(t *testing.T) { writeFile(t, dir, "sibling.key", certtest.KeyPEM(t, keyed)) writeFile(t, dir, "notes.txt", "not a certificate") - chains, err := (&PKCS11Store{uri: &pkcs11.URI{}, certDir: dir}).fileChains() + // An intermediate that only a keyed file carries still has to reach the pool a token + // certificate's chain is built from. + root := certtest.NewCA(t, "corp-root") + intermediateCA := certtest.NewIntermediate(t, root, "corp-intermediate") + chained := certtest.ECDSAKey(t) + writeFile(t, dir, "chained.pem", certtest.CertPEM(intermediateCA.Issue(t, chained, "chained"))+certtest.CertPEM(intermediateCA.Cert)+certtest.KeyPEM(t, chained)) + + chains, extra, err := (&pkcs11Store{uri: &pkcs11.URI{}, certDir: dir}).fileChains() require.NoError(t, err) + require.NotEmpty(t, extra, "the keyed file's certificates are offered for chain building") + tokenLeaf := intermediateCA.Issue(t, certtest.ECDSAKey(t), "token-leaf") + chain := buildChain(tokenLeaf, extra) + require.Len(t, chain, 2, "a token certificate is completed through the intermediate only the keyed file holds") + assert.True(t, intermediateCA.Cert.Equal(chain[1]), "the second certificate is that intermediate") var subjects []string for _, chain := range chains { subjects = append(subjects, chain[0].Subject.CommonName) } assert.ElementsMatch(t, []string{"device", "corp"}, subjects, "only key-less certificate files are left to the token") - chains, err = (&PKCS11Store{uri: &pkcs11.URI{}}).fileChains() + chains, extra, err = (&pkcs11Store{uri: &pkcs11.URI{}}).fileChains() require.NoError(t, err) assert.Empty(t, chains, "no directory configured means no file certificates") + assert.Empty(t, extra, "and no extra intermediates") } func TestNewPKCS11Store_PIN(t *testing.T) { @@ -324,7 +337,7 @@ func TestNewPKCS11Store_PIN(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - store, err := NewPKCS11Store(tt.cfg, "") + store, err := newPKCS11Store(tt.cfg, "") require.NoError(t, err) pin, err := store.userPIN() require.NoError(t, err) @@ -333,7 +346,7 @@ func TestNewPKCS11Store_PIN(t *testing.T) { }) } - _, err := NewPKCS11Store(PKCS11Config{URI: "not-a-pkcs11-uri", PIN: "1234"}, "") + _, err := newPKCS11Store(PKCS11Config{URI: "not-a-pkcs11-uri", PIN: "1234"}, "") assert.Error(t, err, "a malformed URI must not be silently replaced by the defaults") for name, cfg := range map[string]PKCS11Config{ @@ -342,7 +355,7 @@ func TestNewPKCS11Store_PIN(t *testing.T) { "inline pin-value only": {URI: "pkcs11:?pin-value=0000"}, "pin-source only": {URI: "pkcs11:?pin-source=file:/etc/netbird/pkcs11.pin"}, } { - _, err := NewPKCS11Store(cfg, "") + _, err := newPKCS11Store(cfg, "") assert.ErrorIs(t, err, errPINNeedsToken, "%s: a PIN must not go to whichever token is listed first", name) } } @@ -360,7 +373,7 @@ func TestPKCS11Store_WrongPINIsTriedOnce(t *testing.T) { _, uri := pkcs11TestStore(t, "") wrongPIN := "wrong-pin-" + t.Name() - store, err := NewPKCS11Store(PKCS11Config{URI: uri, PIN: wrongPIN}, "") + store, err := newPKCS11Store(PKCS11Config{URI: uri, PIN: wrongPIN}, "") require.NoError(t, err) _, err = store.Candidates(context.Background()) @@ -370,7 +383,7 @@ func TestPKCS11Store_WrongPINIsTriedOnce(t *testing.T) { _, err = store.Candidates(context.Background()) assert.ErrorIs(t, err, errPINRejectedBefore, "the rejected PIN is not sent to the token again") - good, err := NewPKCS11Store(PKCS11Config{URI: uri}, "") + good, err := newPKCS11Store(PKCS11Config{URI: uri}, "") require.NoError(t, err) _, err = good.Candidates(context.Background()) assert.NoError(t, err, "the correct PIN for the same token is unaffected") diff --git a/client/internal/certproof/signattempts.go b/client/internal/certproof/signattempts.go new file mode 100644 index 000000000..b056991bf --- /dev/null +++ b/client/internal/certproof/signattempts.go @@ -0,0 +1,24 @@ +package certproof + +import "errors" + +// signAttempts tracks the identities tried to sign with, for a certificate that can match +// more than one of them. +type signAttempts struct { + errs []error +} + +// try runs sign and reports its signature and whether it succeeded. A failure is kept. +func (a *signAttempts) try(sign func() ([]byte, error)) ([]byte, bool) { + signature, err := sign() + if err != nil { + a.errs = append(a.errs, err) + return nil, false + } + return signature, true +} + +// err joins the failures of the attempts, nil when there were none. +func (a *signAttempts) err() error { + return errors.Join(a.errs...) +} diff --git a/client/internal/certproof/signattempts_test.go b/client/internal/certproof/signattempts_test.go new file mode 100644 index 000000000..f5a2fef4f --- /dev/null +++ b/client/internal/certproof/signattempts_test.go @@ -0,0 +1,41 @@ +package certproof + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var errNeedsApprovalForTest = errors.New("key needs approval") + +// TestSignAttempts_FallsThroughToTheNextIdentity: a certificate can sit in a keychain with +// more than one key, of which only some can sign. A failure moves on, and an identity that +// signs ends the search without reporting the earlier failure. +func TestSignAttempts_FallsThroughToTheNextIdentity(t *testing.T) { + var attempts signAttempts + + signature, ok := attempts.try(func() ([]byte, error) { return nil, errNeedsApprovalForTest }) + assert.False(t, ok, "a failing identity does not end the search") + assert.Nil(t, signature) + + signature, ok = attempts.try(func() ([]byte, error) { return []byte("sig"), nil }) + require.True(t, ok, "the next identity signs") + assert.Equal(t, []byte("sig"), signature) +} + +// TestSignAttempts_KeepsEveryFailure: when no identity signs, the approval a user has to +// give must not be hidden behind the failure of a later identity. +func TestSignAttempts_KeepsEveryFailure(t *testing.T) { + var attempts signAttempts + assert.NoError(t, attempts.err(), "no attempts, no error") + + attempts.try(func() ([]byte, error) { return nil, errNeedsApprovalForTest }) + attempts.try(func() ([]byte, error) { return nil, errors.New("key is gone") }) + + err := attempts.err() + require.Error(t, err) + assert.ErrorIs(t, err, errNeedsApprovalForTest, "the earlier approval error survives the later failure") + assert.ErrorContains(t, err, "key is gone") +} diff --git a/client/internal/certproof/store.go b/client/internal/certproof/store.go index 295d6caa4..85ab7cda7 100644 --- a/client/internal/certproof/store.go +++ b/client/internal/certproof/store.go @@ -46,7 +46,7 @@ func (c Candidate) issuers() []*x509.Certificate { return append(slices.Clip(c.Chain[1:]), c.Intermediates...) } -// Store yields the certificates a peer may prove possession of. FileStore is the PEM +// Store yields the certificates a peer may prove possession of. fileStore is the PEM // directory implementation; OS keystores (CNG, Keychain, PKCS#11) slot in here. type Store interface { Candidates(ctx context.Context) ([]Candidate, error) @@ -68,15 +68,15 @@ type Config struct { OwnerUnknown bool } -// FileStore reads PEM files from a directory. A file holds the chain (leaf first) and +// fileStore reads PEM files from a directory. A file holds the chain (leaf first) and // either its private key or a sibling ".key" file holds it. The key is a plain // PKCS#8, EC or RSA key, or a TSS2 key the TPM signs with. -type FileStore struct { +type fileStore struct { dir string } -func NewFileStore(dir string) *FileStore { - return &FileStore{dir: dir} +func newFileStore(dir string) *fileStore { + return &fileStore{dir: dir} } func StoreDir() string { @@ -86,7 +86,7 @@ func StoreDir() string { return defaultStoreDir } -func (s *FileStore) Candidates(_ context.Context) ([]Candidate, error) { +func (s *fileStore) Candidates(_ context.Context) ([]Candidate, error) { paths, err := certFiles(s.dir) if err != nil { return nil, err diff --git a/client/internal/certproof/store_other.go b/client/internal/certproof/store_other.go index e6646732b..eeecf3c39 100644 --- a/client/internal/certproof/store_other.go +++ b/client/internal/certproof/store_other.go @@ -14,13 +14,13 @@ var unsupportedTokenOnce sync.Once // DefaultStore is the PEM directory named by NB_CERT_STORE_DIR, or /etc/netbird/certs. func DefaultStore() Store { - return NewFileStore(StoreDir()) + return newFileStore(StoreDir()) } // storeWithToken reads the PEM directory cfg names, joined by the PKCS#11 token when cfg // names one. The token pairs the directory's key-less certificates with its own keys. func storeWithToken(cfg Config) Store { - files := NewFileStore(StoreDir()) + files := newFileStore(StoreDir()) if cfg.PKCS11.URI == "" && cfg.PKCS11.PIN == "" { return files } @@ -30,7 +30,7 @@ func storeWithToken(cfg Config) Store { }) return files } - token, err := NewPKCS11Store(cfg.PKCS11, StoreDir()) + token, err := newPKCS11Store(cfg.PKCS11, StoreDir()) if err != nil { log.Warnf("ignoring PKCS#11 URI: %v", err) return files diff --git a/client/internal/certproof/store_other_test.go b/client/internal/certproof/store_other_test.go index 47b05f058..68b4df2a6 100644 --- a/client/internal/certproof/store_other_test.go +++ b/client/internal/certproof/store_other_test.go @@ -15,12 +15,12 @@ func TestStoreWithToken(t *testing.T) { dir := storeDir(t) t.Setenv(StoreDirEnv, dir) - files, ok := storeWithToken(Config{}).(*FileStore) + files, ok := storeWithToken(Config{}).(*fileStore) require.True(t, ok, "no token configured reads the PEM directory alone") assert.Equal(t, dir, files.dir, "the directory comes from NB_CERT_STORE_DIR") - assert.IsType(t, &FileStore{}, storeWithToken(Config{PKCS11: PKCS11Config{URI: "not-a-pkcs11-uri"}}), "an invalid URI must not hide the PEM directory") - assert.IsType(t, &FileStore{}, storeWithToken(Config{PKCS11: PKCS11Config{PIN: "1234"}}), "a PIN naming no token is refused and leaves the PEM directory") + assert.IsType(t, &fileStore{}, storeWithToken(Config{PKCS11: PKCS11Config{URI: "not-a-pkcs11-uri"}}), "an invalid URI must not hide the PEM directory") + assert.IsType(t, &fileStore{}, storeWithToken(Config{PKCS11: PKCS11Config{PIN: "1234"}}), "a PIN naming no token is refused and leaves the PEM directory") configured := map[string]PKCS11Config{ "env pin with token uri": {URI: "pkcs11:token=netbird", PIN: "1234"}, @@ -28,7 +28,7 @@ func TestStoreWithToken(t *testing.T) { } if !pkcs11.Supported() { for name, cfg := range configured { - assert.IsType(t, &FileStore{}, storeWithToken(Config{PKCS11: cfg}), + assert.IsType(t, &fileStore{}, storeWithToken(Config{PKCS11: cfg}), "%s: a build without PKCS#11 support reads the PEM directory alone", name) } return @@ -37,7 +37,7 @@ func TestStoreWithToken(t *testing.T) { store, ok := storeWithToken(Config{PKCS11: cfg}).(Stores) require.True(t, ok, "%s joins the token to the PEM directory", name) require.Len(t, store, 2, name) - token, ok := store[1].(*PKCS11Store) + token, ok := store[1].(*pkcs11Store) require.True(t, ok, name) assert.Equal(t, dir, token.certDir, "%s: the token pairs certificates from the same directory", name) } diff --git a/client/internal/certproof/store_tpm_test.go b/client/internal/certproof/store_tpm_test.go index f8a7a0960..cce570891 100644 --- a/client/internal/certproof/store_tpm_test.go +++ b/client/internal/certproof/store_tpm_test.go @@ -36,7 +36,7 @@ func TestFileStore_TPMKeyFile(t *testing.T) { writeFile(t, dir, "locked.pem", certtest.CertPEM(ca.Issue(t, locked, "locked"))) writeFile(t, dir, "locked.key", tpmtest.KeyPEM(t, locked.Public().(*ecdsa.PublicKey), tpmtest.WithAuth())) - candidates, err := NewFileStore(dir).Candidates(context.Background()) + candidates, err := newFileStore(dir).Candidates(context.Background()) require.NoError(t, err) require.Len(t, candidates, 1, "only the key without an authorization value is usable") assert.True(t, leaf.Equal(candidates[0].Chain[0]), "candidate must carry the device certificate") @@ -73,7 +73,7 @@ func TestCollect_TPMKeyEndToEnd(t *testing.T) { nonce := challenger.Nonce(peerKey, now) checks := []*proto.Checks{{CertificateChallenge: &proto.CertificateChallenge{Nonce: nonce, CaCertificates: []string{ca.PEM}}}} - proofs := Collect(context.Background(), NewFileStore(dir), checks, peerKey) + proofs := Collect(context.Background(), newFileStore(dir), checks, peerKey) require.Len(t, proofs, 1, "the TPM-held key must prove the certificate") chain, err := challenger.Verify(proofs[0], peerKey, now) require.NoError(t, err) diff --git a/client/internal/certproof/storefile_unix_test.go b/client/internal/certproof/storefile_unix_test.go index d35a4e759..9932388b6 100644 --- a/client/internal/certproof/storefile_unix_test.go +++ b/client/internal/certproof/storefile_unix_test.go @@ -26,7 +26,7 @@ func devicePair(t *testing.T, dir string) { func candidates(t *testing.T, dir string) ([]Candidate, error) { t.Helper() - return NewFileStore(dir).Candidates(context.Background()) + return newFileStore(dir).Candidates(context.Background()) } func TestFileStore_AcceptsAPrivatePair(t *testing.T) { diff --git a/client/internal/certproof/systemstore_windows.go b/client/internal/certproof/systemstore_windows.go index 92aa03d4b..908899107 100644 --- a/client/internal/certproof/systemstore_windows.go +++ b/client/internal/certproof/systemstore_windows.go @@ -49,10 +49,10 @@ type bcryptPSSPaddingInfo struct { // DefaultStore is the local machine's personal certificate store, where device // certificates enrolled through AD or Intune are kept. func DefaultStore() Store { - return NewSystemStore() + return newSystemStore() } -// SystemStore yields the identities of a personal certificate store, completing their +// systemStore yields the identities of a personal certificate store, completing their // chains from the matching intermediate CA store. Keys are used through CNG and never // exported. // @@ -60,22 +60,22 @@ func DefaultStore() Store { // a service reads; the current user store lives in the signed-in user's registry hive // with keys protected against their profile, so it is only readable while running as // that user. -type SystemStore struct { +type systemStore struct { location uint32 } -// NewSystemStore reads the local machine store, which is what the daemon uses. -func NewSystemStore() *SystemStore { - return &SystemStore{location: windows.CERT_SYSTEM_STORE_LOCAL_MACHINE} +// newSystemStore reads the local machine store, which is what the daemon uses. +func newSystemStore() *systemStore { + return &systemStore{location: windows.CERT_SYSTEM_STORE_LOCAL_MACHINE} } -// NewUserStore reads the calling user's personal store. It is only useful in a process +// newUserStore reads the calling user's personal store. It is only useful in a process // already running as that user, which is what the posture helper is. -func NewUserStore() *SystemStore { - return &SystemStore{location: windows.CERT_SYSTEM_STORE_CURRENT_USER} +func newUserStore() *systemStore { + return &systemStore{location: windows.CERT_SYSTEM_STORE_CURRENT_USER} } -func (s *SystemStore) Candidates(_ context.Context) ([]Candidate, error) { +func (s *systemStore) Candidates(_ context.Context) ([]Candidate, error) { leaves, err := storeCertificates(s.location, personalStore) if err != nil { return nil, err @@ -100,7 +100,7 @@ func (s *SystemStore) Candidates(_ context.Context) ([]Candidate, error) { } // String names the store location the way the Windows documentation does. -func (s *SystemStore) String() string { +func (s *systemStore) String() string { if s.location == windows.CERT_SYSTEM_STORE_CURRENT_USER { return "CurrentUser" } diff --git a/client/internal/pkcs11/driver_linux.go b/client/internal/pkcs11/driver_linux.go index 93bf4a5ba..54a3863cb 100644 --- a/client/internal/pkcs11/driver_linux.go +++ b/client/internal/pkcs11/driver_linux.go @@ -32,6 +32,7 @@ const ( // signature is 2 KiB. maxAttributeLen = 1 << 20 maxSignatureLen = 1 << 16 + maxFoundObjects = 1 << 16 ) type version struct { @@ -103,6 +104,15 @@ const ( fnSign = 43 ) +// checkFunctionListVersion refuses a module whose function list is older than the layout +// functionList mirrors, which would otherwise be read past its end. +func checkFunctionListVersion(v version) error { + if v.major < 2 { + return fmt.Errorf("PKCS#11 function list version %d.%d is older than 2.0", v.major, v.minor) + } + return nil +} + // module holds the entry points of one loaded library, bound straight from its // CK_FUNCTION_LIST. type module struct { @@ -152,6 +162,10 @@ func load(path string) (_ driver, err error) { return nil, Error{Op: "C_GetFunctionList", Code: uint(rv)} } + if err := checkFunctionListVersion(list.version); err != nil { + return nil, fmt.Errorf("%s: %w", path, err) + } + m := &module{} for _, entry := range []struct { fn any @@ -185,21 +199,43 @@ func load(path string) (_ driver, err error) { return m, nil } +// slotListAttempts bounds how often the slot list is asked for again after it grew between +// the size query and the read. +const slotListAttempts = 3 + +// slotList returns the slots with a token present. A token plugged in between the size +// query and the read makes the module answer CKR_BUFFER_TOO_SMALL, in which case the list +// is asked for again. +func (m *module) slotList() ([]ulong, error) { + for range slotListAttempts { + var count ulong + if rv := m.cGetSlotList(1, nil, &count); rv != rvOK { + return nil, Error{Op: "C_GetSlotList", Code: uint(rv)} + } + if count == 0 { + return nil, nil + } + slots := make([]ulong, count) + rv := m.cGetSlotList(1, &slots[0], &count) + if rv == rvBufferTooSmall { + continue + } + if rv != rvOK { + return nil, Error{Op: "C_GetSlotList", Code: uint(rv)} + } + return slots[:count], nil + } + return nil, Error{Op: "C_GetSlotList", Code: rvBufferTooSmall} +} + func (m *module) tokens() ([]Token, error) { - var count ulong - if rv := m.cGetSlotList(1, nil, &count); rv != rvOK { - return nil, Error{Op: "C_GetSlotList", Code: uint(rv)} - } - if count == 0 { - return nil, nil - } - slots := make([]ulong, count) - if rv := m.cGetSlotList(1, &slots[0], &count); rv != rvOK { - return nil, Error{Op: "C_GetSlotList", Code: uint(rv)} + slots, err := m.slotList() + if err != nil { + return nil, err } - tokens := make([]Token, 0, count) - for _, slot := range slots[:count] { + tokens := make([]Token, 0, len(slots)) + for _, slot := range slots { var info tokenInfo if rv := m.cGetTokenInfo(slot, &info); rv != rvOK { continue @@ -262,11 +298,16 @@ func (m *module) findObjects(session uint, template []Attribute) ([]Object, erro if count > findBatch { return nil, fmt.Errorf("C_FindObjects reported %d handles for a batch of %d", count, findBatch) } + // Only an empty batch ends the search: a module may return fewer handles than + // asked for before it has returned them all. + if count == 0 { + return objects, nil + } for _, handle := range batch[:count] { objects = append(objects, Object(handle)) } - if count < findBatch { - return objects, nil + if len(objects) > maxFoundObjects { + return nil, fmt.Errorf("C_FindObjects returned more than %d objects", maxFoundObjects) } } } diff --git a/client/internal/pkcs11/driver_linux_test.go b/client/internal/pkcs11/driver_linux_test.go index 408a3fabc..2362ee18f 100644 --- a/client/internal/pkcs11/driver_linux_test.go +++ b/client/internal/pkcs11/driver_linux_test.go @@ -70,3 +70,119 @@ func loadFirst(t *testing.T, paths ...string) *Module { t.Skip("p11-kit trust module not installed") return nil } + +// TestSlotList_RetriesWhenTheListGrows: a token plugged in between the size query and the +// fill makes the module answer CKR_BUFFER_TOO_SMALL; the list has to be asked for again. +func TestSlotList_RetriesWhenTheListGrows(t *testing.T) { + calls := 0 + m := &module{cGetSlotList: func(_ byte, slots *ulong, count *ulong) ulong { + calls++ + switch { + case slots == nil && calls == 1: + *count = 1 + case slots != nil && calls == 2: + *count = 2 + return rvBufferTooSmall + case slots == nil: + *count = 2 + default: + *count = 2 + backing := unsafe.Slice(slots, 2) + backing[0], backing[1] = 7, 9 + } + return rvOK + }} + + slots, err := m.slotList() + + require.NoError(t, err) + assert.Equal(t, []ulong{7, 9}, slots, "the list is read again at its new size") +} + +func TestSlotList_GivesUpOnAListThatKeepsGrowing(t *testing.T) { + m := &module{cGetSlotList: func(_ byte, slots *ulong, count *ulong) ulong { + if slots == nil { + *count = 1 + return rvOK + } + return rvBufferTooSmall + }} + + _, err := m.slotList() + + assert.Error(t, err, "a module that never settles on a size is an error, not a loop") +} + +func TestCheckFunctionListVersion(t *testing.T) { + assert.NoError(t, checkFunctionListVersion(version{major: 2, minor: 40})) + assert.NoError(t, checkFunctionListVersion(version{major: 3, minor: 0})) + assert.Error(t, checkFunctionListVersion(version{major: 1, minor: 0}), "a pre-2.0 list has no entry points to index") + assert.Error(t, checkFunctionListVersion(version{}), "an empty version is not a function list") +} + +// fakeFindModule answers C_FindObjects from batches, a handle per entry, and reports how +// the search was torn down. +func fakeFindModule(batches [][]ulong, finalized *int) *module { + next := 0 + return &module{ + cFindObjectsInit: func(ulong, *attribute, ulong) ulong { return rvOK }, + cFindObjectsFinal: func(ulong) ulong { *finalized++; return rvOK }, + cFindObjects: func(_ ulong, objects *ulong, max ulong, count *ulong) ulong { + if next >= len(batches) { + *count = 0 + return rvOK + } + batch := batches[next] + next++ + copy(unsafe.Slice(objects, max), batch) + *count = ulong(len(batch)) + return rvOK + }, + } +} + +var findTemplate = []Attribute{{Type: AttrClass, Value: ULong(ClassPrivateKey)}} + +// TestFindObjects_ReadsPastAShortBatch: PKCS#11 ends a search with an empty batch, not with +// one shorter than asked for, so a module that hands out its handles in small batches must +// still have all of them returned. +func TestFindObjects_ReadsPastAShortBatch(t *testing.T) { + finalized := 0 + m := fakeFindModule([][]ulong{{1, 2}, {3}, {4, 5, 6}}, &finalized) + + objects, err := m.findObjects(1, findTemplate) + + require.NoError(t, err) + assert.Equal(t, []Object{1, 2, 3, 4, 5, 6}, objects, "every batch is read until the module returns an empty one") + assert.Equal(t, 1, finalized, "the search is finished once") +} + +func TestFindObjects_StopsAtAnEmptyBatch(t *testing.T) { + finalized := 0 + m := fakeFindModule(nil, &finalized) + + objects, err := m.findObjects(1, findTemplate) + + require.NoError(t, err) + assert.Empty(t, objects, "a search that finds nothing returns nothing") + assert.Equal(t, 1, finalized) +} + +// TestFindObjects_BoundsAModuleThatNeverEnds: a module that keeps returning handles must +// not make the daemon allocate without limit. +func TestFindObjects_BoundsAModuleThatNeverEnds(t *testing.T) { + finalized := 0 + endless := &module{ + cFindObjectsInit: func(ulong, *attribute, ulong) ulong { return rvOK }, + cFindObjectsFinal: func(ulong) ulong { finalized++; return rvOK }, + cFindObjects: func(_ ulong, _ *ulong, _ ulong, count *ulong) ulong { + *count = findBatch + return rvOK + }, + } + + _, err := endless.findObjects(1, findTemplate) + + assert.Error(t, err, "an endless search is cut off") + assert.Equal(t, 1, finalized, "and still finished") +} diff --git a/client/internal/pkcs11/pkcs11.go b/client/internal/pkcs11/pkcs11.go index da874fb62..4a34848d9 100644 --- a/client/internal/pkcs11/pkcs11.go +++ b/client/internal/pkcs11/pkcs11.go @@ -50,6 +50,7 @@ const ( rvPINExpired = 0xa3 rvPINLocked = 0xa4 rvUserAlreadyLoggedIn = 0x100 + rvBufferTooSmall = 0x150 rvAlreadyInitialized = 0x191 ) diff --git a/client/internal/pkcs11/publickey.go b/client/internal/pkcs11/publickey.go index 4d59598e6..aac78524a 100644 --- a/client/internal/pkcs11/publickey.go +++ b/client/internal/pkcs11/publickey.go @@ -70,8 +70,10 @@ func rsaPublicKey(modulus, exponent []byte) (*rsa.PublicKey, error) { // uncompressed point wrapped in a DER OCTET STRING, which some modules hand out bare. func ecPublicKey(params, point []byte) (*ecdsa.PublicKey, error) { var oid asn1.ObjectIdentifier - if _, err := asn1.Unmarshal(params, &oid); err != nil { + if rest, err := asn1.Unmarshal(params, &oid); err != nil { return nil, fmt.Errorf("CKA_EC_PARAMS: %w", err) + } else if len(rest) != 0 { + return nil, errors.New("CKA_EC_PARAMS has trailing data") } curve, ok := curvesByOID[oid.String()] if !ok { @@ -80,8 +82,10 @@ func ecPublicKey(params, point []byte) (*ecdsa.PublicKey, error) { size := (curve.Params().BitSize + 7) / 8 raw := point if len(raw) != 1+2*size { - if _, err := asn1.Unmarshal(point, &raw); err != nil { + if rest, err := asn1.Unmarshal(point, &raw); err != nil { return nil, fmt.Errorf("CKA_EC_POINT: %w", err) + } else if len(rest) != 0 { + return nil, errors.New("CKA_EC_POINT has trailing data") } } if len(raw) != 1+2*size || raw[0] != 4 { diff --git a/client/internal/pkcs11/publickey_test.go b/client/internal/pkcs11/publickey_test.go index 527d6db46..70c1c9a15 100644 --- a/client/internal/pkcs11/publickey_test.go +++ b/client/internal/pkcs11/publickey_test.go @@ -6,6 +6,7 @@ import ( "crypto/rand" "crypto/rsa" "encoding/asn1" + "math" "math/big" "testing" @@ -21,6 +22,8 @@ func TestECPublicKey(t *testing.T) { }{ {"P-256", elliptic.P256(), asn1.ObjectIdentifier{1, 2, 840, 10045, 3, 1, 7}}, {"P-384", elliptic.P384(), asn1.ObjectIdentifier{1, 3, 132, 0, 34}}, + // The wrapped P-521 point is 133 bytes, which needs the long form of the DER length. + {"P-521", elliptic.P521(), asn1.ObjectIdentifier{1, 3, 132, 0, 35}}, } for _, tt := range curves { t.Run(tt.name, func(t *testing.T) { @@ -57,6 +60,13 @@ func TestECPublicKey_Rejections(t *testing.T) { assert.Error(t, err, "a truncated point must be rejected") _, err = ecPublicKey([]byte("junk"), point) assert.Error(t, err, "malformed parameters must be rejected") + + wrapped, err := asn1.Marshal(point) + require.NoError(t, err) + _, err = ecPublicKey(append(append([]byte{}, p256...), 0), point) + assert.Error(t, err, "bytes after the curve parameters must be rejected") + _, err = ecPublicKey(p256, append(append([]byte{}, wrapped...), 0)) + assert.Error(t, err, "bytes after the wrapped point must be rejected") } func TestRSAPublicKey(t *testing.T) { @@ -69,6 +79,12 @@ func TestRSAPublicKey(t *testing.T) { _, err = rsaPublicKey(key.N.Bytes(), nil) assert.Error(t, err, "a missing exponent must be rejected") + _, err = rsaPublicKey(key.N.Bytes(), big.NewInt(math.MaxInt32).Bytes()) + assert.NoError(t, err, "the largest exponent an int holds is accepted") + _, err = rsaPublicKey(key.N.Bytes(), big.NewInt(math.MaxInt32+1).Bytes()) + assert.Error(t, err, "an exponent past int32 must be rejected") + _, err = rsaPublicKey(key.N.Bytes(), []byte{0}) + assert.Error(t, err, "a zero exponent must be rejected") } func TestULongRoundTrip(t *testing.T) { diff --git a/client/internal/pkcs11/uri.go b/client/internal/pkcs11/uri.go index c7248c10e..fb5dc2765 100644 --- a/client/internal/pkcs11/uri.go +++ b/client/internal/pkcs11/uri.go @@ -29,7 +29,7 @@ type URI struct { // module-path must be absolute and a module-name must be a bare name. Unknown query // attributes are ignored, as the RFC asks. func ParseURI(raw string) (*URI, error) { - rest, ok := strings.CutPrefix(raw, "pkcs11:") + rest, ok := cutPrefixFold(raw, "pkcs11:") if !ok { return nil, errors.New("PKCS#11 URI does not start with the pkcs11 scheme") } @@ -72,11 +72,61 @@ func (u *URI) setQueryAttribute(name, value string) error { case "pin-value": u.pinValue = &value case "pin-source": - u.pinSource = value + path, err := pinSourcePath(value) + if err != nil { + return err + } + u.pinSource = path } return nil } +// pinSourcePath turns a pin-source value into the file to read. A plain path is used as +// it is; a file URI must name a local absolute path, with no host or localhost (RFC 8089 +// section 2), so a PIN is never read from somewhere the URI only appears to point to. +func pinSourcePath(value string) (string, error) { + if value == "" { + return "", errors.New("PKCS#11 URI pin-source is empty") + } + rest, isFile := cutPrefixFold(value, "file:") + if !isFile { + return value, nil + } + authorityAndPath, hasAuthority := strings.CutPrefix(rest, "//") + if !hasAuthority { + if !isAbsolutePath(rest) { + return "", fmt.Errorf("PKCS#11 URI pin-source %q must be an absolute path", value) + } + return rest, nil + } + if isAbsolutePath(authorityAndPath) { + return authorityAndPath, nil + } + host, path, _ := strings.Cut(authorityAndPath, "/") + if host != "" && !strings.EqualFold(host, "localhost") { + return "", fmt.Errorf("PKCS#11 URI pin-source host %q is not the local machine", host) + } + if path == "" { + return "", fmt.Errorf("PKCS#11 URI pin-source %q names no file", value) + } + return "/" + path, nil +} + +// isAbsolutePath reports whether p is absolute in a file URI. A path starting with a slash +// is absolute there on every platform, including Windows, where filepath.IsAbs wants a +// drive letter. +func isAbsolutePath(p string) bool { + return strings.HasPrefix(p, "/") || filepath.IsAbs(p) +} + +// cutPrefixFold is strings.CutPrefix with the prefix matched without regard to case. +func cutPrefixFold(s, prefix string) (string, bool) { + if len(s) < len(prefix) || !strings.EqualFold(s[:len(prefix)], prefix) { + return s, false + } + return s[len(prefix):], true +} + // Module is the library to load, DefaultModule when the URI names none. func (u *URI) Module() string { if u.ModulePath == "" { @@ -99,8 +149,7 @@ func (u *URI) PIN() ([]byte, error) { if u.pinSource == "" { return nil, nil } - path := strings.TrimPrefix(strings.TrimPrefix(u.pinSource, "file://"), "file:") - pin, err := os.ReadFile(path) + pin, err := os.ReadFile(u.pinSource) if err != nil { return nil, fmt.Errorf("read PIN: %w", err) } diff --git a/client/internal/pkcs11/uri_test.go b/client/internal/pkcs11/uri_test.go index 54e8a0d91..09aede6a7 100644 --- a/client/internal/pkcs11/uri_test.go +++ b/client/internal/pkcs11/uri_test.go @@ -84,6 +84,10 @@ func TestParseURI_Rejections(t *testing.T) { "module-name with dotdot": "pkcs11:token=a?module-name=..", "empty module-name": "pkcs11:token=a?module-name=", "pin-value and pin-source": "pkcs11:token=a?pin-value=1&pin-source=file:/etc/pin", + // Without a value the URI would silently mean "no PIN" and skip the login. + "empty pin-source": "pkcs11:token=a?pin-source=", + "remote pin-source": "pkcs11:token=a?pin-source=file://example.com/etc/pin", + "relative pin-source": "pkcs11:token=a?pin-source=file:pin", } for name, raw := range tests { _, err := ParseURI(raw) @@ -91,6 +95,31 @@ func TestParseURI_Rejections(t *testing.T) { } } +// TestParseURI_SchemeIsCaseInsensitive: RFC 3986 section 3.1 makes schemes case-insensitive. +func TestParseURI_SchemeIsCaseInsensitive(t *testing.T) { + for _, raw := range []string{"PKCS11:token=netbird", "Pkcs11:token=netbird"} { + uri, err := ParseURI(raw) + require.NoError(t, err, raw) + assert.Equal(t, "netbird", uri.Token, raw) + } +} + +func TestURI_PINFromLocalhostFileURI(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file URIs with a host name are written differently on Windows") + } + path := filepath.Join(t.TempDir(), "pin") + require.NoError(t, os.WriteFile(path, []byte("secret\n"), 0o600)) + + for _, host := range []string{"localhost", "LocalHost"} { + uri, err := ParseURI("pkcs11:token=netbird?pin-source=file://" + host + path) + require.NoError(t, err, host) + pin, err := uri.PIN() + require.NoError(t, err, host) + assert.Equal(t, []byte("secret"), pin, "RFC 8089 names the local machine as localhost") + } +} + func TestURI_PINFromFile(t *testing.T) { path := filepath.Join(t.TempDir(), "pin") require.NoError(t, os.WriteFile(path, []byte("secret\n"), 0o600)) @@ -117,3 +146,30 @@ func absModule(name string) string { } return "/lib/" + name } + +// TestPinSourcePath covers how a pin-source value becomes a file path, without touching the +// file system, so the same cases run on every platform. A path starting with a slash is +// absolute in a file URI everywhere, including Windows, where filepath.IsAbs would refuse it. +func TestPinSourcePath(t *testing.T) { + tests := []struct { + value string + want string + }{ + {"/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + {"file:/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + {"file:///etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + {"FILE:///etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + {"file://localhost/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + {"file://LocalHost/etc/netbird/pkcs11.pin", "/etc/netbird/pkcs11.pin"}, + } + for _, tt := range tests { + got, err := pinSourcePath(tt.value) + require.NoError(t, err, tt.value) + assert.Equal(t, tt.want, got, tt.value) + } + + for _, value := range []string{"", "file:pin", "file://example.com/etc/pin", "file://localhost", "file://localhost/"} { + _, err := pinSourcePath(value) + assert.Error(t, err, "%q must be refused", value) + } +} diff --git a/management/internals/network_map_db/shared_types_test.go b/management/internals/network_map_db/shared_types_test.go index 8a1239e26..34b69d265 100644 --- a/management/internals/network_map_db/shared_types_test.go +++ b/management/internals/network_map_db/shared_types_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRecordTypeAndRdata(t *testing.T) { @@ -36,3 +37,24 @@ func TestRecordTypeAndRdata(t *testing.T) { }) } } + +// TestConvertToNmdataPeers_CarriesTheVerifiedCertificates: the certificate posture check is +// evaluated on the chains stored in the peer meta, so a peer loaded through this path with +// its certificates dropped would silently fail every certificate check. +func TestConvertToNmdataPeers_CarriesTheVerifiedCertificates(t *testing.T) { + peers, _, err := ConvertToNmdataPeers([]Peer{ + {MetaCertificates: []byte(`["-----BEGIN CERTIFICATE-----\nleaf\n-----END CERTIFICATE-----\n","chain-2"]`)}, + {}, + }) + + require.NoError(t, err) + require.Len(t, peers, 2) + assert.Equal(t, []string{"-----BEGIN CERTIFICATE-----\nleaf\n-----END CERTIFICATE-----\n", "chain-2"}, peers[0].Meta.Certificates) + assert.Empty(t, peers[1].Meta.Certificates, "a peer that proved nothing has no certificates") +} + +func TestConvertToNmdataPeers_RejectsMalformedCertificates(t *testing.T) { + _, _, err := ConvertToNmdataPeers([]Peer{{MetaCertificates: []byte(`{"not":"a list"}`)}}) + + assert.Error(t, err, "a stored value that is not a list of chains must not load as no certificates") +} diff --git a/shared/management/certposture/certposture_test.go b/shared/management/certposture/certposture_test.go index ae31a2203..e8f3d20c1 100644 --- a/shared/management/certposture/certposture_test.go +++ b/shared/management/certposture/certposture_test.go @@ -2,6 +2,8 @@ package certposture import ( "crypto" + "crypto/ecdsa" + "crypto/elliptic" "crypto/rand" "crypto/rsa" "crypto/sha256" @@ -86,6 +88,7 @@ func TestSignVerify_RoundTripPerKeyType(t *testing.T) { ca := certtest.NewCA(t, "root") keys := map[string]crypto.Signer{ SigAlgECDSASHA256: certtest.ECDSAKey(t), + SigAlgECDSASHA384: p384Key(t), SigAlgRSAPSSSHA256: certtest.RSAKey(t), SigAlgEd25519: certtest.Ed25519Key(t), } @@ -103,6 +106,24 @@ func TestSignVerify_RoundTripPerKeyType(t *testing.T) { } } +func p384Key(t *testing.T) crypto.Signer { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P384(), rand.Reader) + require.NoError(t, err) + return key +} + +// TestSign_RefusesCurvesTheProofDoesNotName: a proof names its algorithm from the key, and +// the verifier only knows P-256 and P-384, so P-521 is refused before anything is signed. +func TestSign_RefusesCurvesTheProofDoesNotName(t *testing.T) { + key, err := ecdsa.GenerateKey(elliptic.P521(), rand.Reader) + require.NoError(t, err) + + _, _, err = Sign(key, NewChallenger(secret).Nonce(peerKey, now), peerKey) + + assert.ErrorIs(t, err, ErrUnsupportedKey) +} + func TestVerify_Rejections(t *testing.T) { ca := certtest.NewCA(t, "root") c := NewChallenger(secret) diff --git a/shared/management/client/grpc_test.go b/shared/management/client/grpc_test.go index c947130fd..8c69cea1c 100644 --- a/shared/management/client/grpc_test.go +++ b/shared/management/client/grpc_test.go @@ -12,6 +12,8 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" + "github.com/netbirdio/netbird/client/system" + "github.com/netbirdio/netbird/shared/management/certposture" mgmtProto "github.com/netbirdio/netbird/shared/management/proto" ) @@ -93,3 +95,25 @@ func TestMaxRecvMsgSizeIntegration(t *testing.T) { assert.Len(t, resp.Key, payloadSize) }) } + +// TestInfoToMetaData_CarriesCertificateProofs: management verifies the proofs it receives +// in the peer meta, so a field dropped by the mapping fails the certificate check without +// any error on either side. +func TestInfoToMetaData_CarriesCertificateProofs(t *testing.T) { + info := &system.Info{CertificateProofs: []certposture.Proof{ + {Nonce: []byte("nonce-1"), Chain: [][]byte{[]byte("leaf"), []byte("intermediate")}, SigAlg: "ecdsa-sha256", Signature: []byte("sig-1")}, + {Nonce: []byte("nonce-2"), Chain: [][]byte{[]byte("other")}, SigAlg: "rsa-pss-sha256", Signature: []byte("sig-2")}, + }} + + meta := infoToMetaData(info) + + require.Len(t, meta.GetCertificateProofs(), 2, "every proof reaches the meta") + first := meta.GetCertificateProofs()[0] + assert.Equal(t, []byte("nonce-1"), first.GetNonce()) + assert.Equal(t, [][]byte{[]byte("leaf"), []byte("intermediate")}, first.GetChain(), "the whole chain is carried, in order") + assert.Equal(t, "ecdsa-sha256", first.GetSigAlg()) + assert.Equal(t, []byte("sig-1"), first.GetSignature()) + assert.Equal(t, "rsa-pss-sha256", meta.GetCertificateProofs()[1].GetSigAlg()) + + assert.Empty(t, infoToMetaData(&system.Info{}).GetCertificateProofs(), "a device without proofs sends none") +} diff --git a/shared/management/http/api/openapi.yml b/shared/management/http/api/openapi.yml index 8b97d5f47..fbf8bf5cd 100644 --- a/shared/management/http/api/openapi.yml +++ b/shared/management/http/api/openapi.yml @@ -1821,6 +1821,7 @@ components: ca_certificates: description: PEM encoded CA certificates the peer's certificate must chain to type: array + minItems: 1 items: type: string example: ["-----BEGIN CERTIFICATE-----\nMIIB...\n-----END CERTIFICATE-----"] From 547dd1f177bc1190558a680488f085550b1f39a4 Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Thu, 15 Oct 2026 12:19:57 +0200 Subject: [PATCH 05/10] [client] Detect default route changes in the Linux network monitor (#8216) The Linux network monitor skipped every route update whose Dst was not nil, treating a nil Dst as the default route. Since the bump to vishvananda/netlink v1.3 (v0.43.0) the default route arrives as an explicit 0.0.0.0/0 or ::/0, so every default route change was filtered out and the monitor never fired on Linux, no matter how the underlay moved. Treat both a nil Dst and a zero-length prefix as the default route. --- .../networkmonitor/check_change_linux.go | 13 +++++- .../networkmonitor/check_change_linux_test.go | 43 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 client/internal/networkmonitor/check_change_linux_test.go diff --git a/client/internal/networkmonitor/check_change_linux.go b/client/internal/networkmonitor/check_change_linux.go index efd8b5884..4fbff4448 100644 --- a/client/internal/networkmonitor/check_change_linux.go +++ b/client/internal/networkmonitor/check_change_linux.go @@ -6,6 +6,7 @@ import ( "context" "errors" "fmt" + "net" "syscall" log "github.com/sirupsen/logrus" @@ -35,7 +36,7 @@ func checkChange(ctx context.Context, nexthopv4, nexthopv6 systemops.Nexthop) er // handle route changes case route := <-routeChan: // default route and main table - if route.Dst != nil || route.Table != syscall.RT_TABLE_MAIN { + if !isDefaultRoute(route.Dst) || route.Table != syscall.RT_TABLE_MAIN { continue } switch route.Type { @@ -52,3 +53,13 @@ func checkChange(ctx context.Context, nexthopv4, nexthopv6 systemops.Nexthop) er } } } + +// isDefaultRoute reports whether dst is a default route. netlink v1.3 reports +// the default as 0.0.0.0/0 or ::/0, older versions as a nil Dst. +func isDefaultRoute(dst *net.IPNet) bool { + if dst == nil { + return true + } + ones, _ := dst.Mask.Size() + return ones == 0 +} diff --git a/client/internal/networkmonitor/check_change_linux_test.go b/client/internal/networkmonitor/check_change_linux_test.go new file mode 100644 index 000000000..13e1c9e6d --- /dev/null +++ b/client/internal/networkmonitor/check_change_linux_test.go @@ -0,0 +1,43 @@ +//go:build !android + +package networkmonitor + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIsDefaultRoute(t *testing.T) { + mustCIDR := func(s string) *net.IPNet { + t.Helper() + _, n, err := net.ParseCIDR(s) + if err != nil { + t.Fatalf("parse %s: %v", s, err) + } + return n + } + + tests := []struct { + name string + dst *net.IPNet + want bool + }{ + // netlink before v1.3 left Dst nil for the default route. + {name: "nil dst", dst: nil, want: true}, + // netlink v1.3 reports the default route with an explicit zero-length prefix. + {name: "v4 default", dst: mustCIDR("0.0.0.0/0"), want: true}, + {name: "v6 default", dst: mustCIDR("::/0"), want: true}, + // The exit node split routes and ordinary prefixes are not defaults. + {name: "v4 split half", dst: mustCIDR("0.0.0.0/1"), want: false}, + {name: "v4 subnet", dst: mustCIDR("10.0.0.0/8"), want: false}, + {name: "v4 host", dst: mustCIDR("1.1.1.1/32"), want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isDefaultRoute(tt.dst), "default route detection") + }) + } +} From 357d8a2a7c565605e0e10cd79d1b46c5ea958555 Mon Sep 17 00:00:00 2001 From: Nicolas Frati Date: Thu, 15 Oct 2026 14:26:50 +0200 Subject: [PATCH 06/10] [proxy,client] Add --external-ip-map flag for the embedded client (#8185) * [proxy] Add --external-ip-map flag for the embedded client The proxy behind a NAT with a forwarded fixed --wg-port could not advertise its public address as an ICE candidate, so peers fell back to relay. Expose the agent's external IP map through embed.Options and a proxy flag (NB_PROXY_EXTERNAL_IP_MAP). * [proxy] Validate --external-ip-map at startup The engine drops every external IP mapping when one fails to parse, so a typo or a space after a comma silently disabled the feature. Move the agent's validator to shared/externalipmap, run it from the proxy at startup, and trim entries first. * [client,proxy] Require an external IP first in --external-ip-map The engine reads the first component as the external IP, so a mapping like iface/10.0.0.5 passed validation and then dropped every mapping. --- client/cmd/up.go | 73 +-------------------------- client/embed/embed.go | 4 ++ proxy/cmd/proxy/cmd/root.go | 20 ++++++++ proxy/internal/roundtrip/netbird.go | 4 ++ proxy/lifecycle.go | 4 ++ proxy/server.go | 12 +++-- shared/externalipmap/validate.go | 70 +++++++++++++++++++++++++ shared/externalipmap/validate_test.go | 48 ++++++++++++++++++ 8 files changed, 160 insertions(+), 75 deletions(-) create mode 100644 shared/externalipmap/validate.go create mode 100644 shared/externalipmap/validate_test.go diff --git a/client/cmd/up.go b/client/cmd/up.go index 120a25595..e336ba257 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "net" "net/netip" "runtime" "strings" @@ -26,16 +25,11 @@ import ( "github.com/netbirdio/netbird/client/proto" "github.com/netbirdio/netbird/client/server" "github.com/netbirdio/netbird/client/system" + "github.com/netbirdio/netbird/shared/externalipmap" "github.com/netbirdio/netbird/shared/management/domain" "github.com/netbirdio/netbird/util" ) -const ( - invalidInputType int = iota - ipInputType - interfaceInputType -) - const ( dnsLabelsFlag = "extra-dns-labels" @@ -106,7 +100,7 @@ func upFunc(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed initializing log %v", err) } - err = validateNATExternalIPs(natExternalIPs) + err = externalipmap.Validate(natExternalIPs) if err != nil { return err } @@ -836,36 +830,6 @@ func setupLoginRequest(providedSetupKey string, customDNSAddressConverted []byte return &loginRequest, nil } -func validateNATExternalIPs(list []string) error { - for _, element := range list { - if element == "" { - return fmt.Errorf("empty string is not a valid input for %s", externalIPMapFlag) - } - - subElements := strings.Split(element, "/") - if len(subElements) > 2 { - return fmt.Errorf("%s is not a valid input for %s. it should be formatted as \"String\" or \"String/String\"", element, externalIPMapFlag) - } - - if len(subElements) == 1 && !isValidIP(subElements[0]) { - return fmt.Errorf("%s is not a valid input for %s. it should be formatted as \"IP\" or \"IP/IP\", or \"IP/Interface Name\"", element, externalIPMapFlag) - } - - last := 0 - for _, singleElement := range subElements { - inputType, err := validateElement(singleElement) - if err != nil { - return fmt.Errorf("%s is not a valid input for %s. it should be an IP string or a network name", singleElement, externalIPMapFlag) - } - if last == interfaceInputType && inputType == interfaceInputType { - return fmt.Errorf("%s is not a valid input for %s. it should not contain two interface names", element, externalIPMapFlag) - } - last = inputType - } - } - return nil -} - func parseInterfaceName(name string) error { if runtime.GOOS != "darwin" { return nil @@ -878,39 +842,6 @@ func parseInterfaceName(name string) error { return fmt.Errorf("invalid interface name %s. Please use the prefix utun followed by a number on MacOS. e.g., utun1 or utun199", name) } -func validateElement(element string) (int, error) { - if isValidIP(element) { - return ipInputType, nil - } - validIface, err := isValidInterface(element) - if err != nil { - return invalidInputType, fmt.Errorf("unable to validate the network interface name, error: %s", err) - } - - if validIface { - return interfaceInputType, nil - } - - return interfaceInputType, fmt.Errorf("invalid IP or network interface name not found") -} - -func isValidIP(ip string) bool { - return net.ParseIP(ip) != nil -} - -func isValidInterface(name string) (bool, error) { - netInterfaces, err := net.Interfaces() - if err != nil { - return false, err - } - for _, iface := range netInterfaces { - if iface.Name == name { - return true, nil - } - } - return false, nil -} - func parseCustomDNSAddress(modified bool) ([]byte, error) { var parsed []byte if modified { diff --git a/client/embed/embed.go b/client/embed/embed.go index 6fdddab48..208e71817 100644 --- a/client/embed/embed.go +++ b/client/embed/embed.go @@ -118,6 +118,9 @@ type Options struct { MTU *uint16 // DNSLabels defines additional DNS labels configured in the peer. DNSLabels []string + // NATExternalIPs maps external IPs to internal IPs or interfaces for ICE + // candidates, same format as the agent's --external-ip-map flag. + NATExternalIPs []string // Performance configures the tunnel's buffer pool cap and batch size. Performance Performance } @@ -224,6 +227,7 @@ func New(opts Options) (*Client, error) { WireguardPort: opts.WireguardPort, MTU: opts.MTU, DNSLabels: parsedLabels, + NATExternalIPs: opts.NATExternalIPs, } if opts.ConfigPath != "" { config, err = profilemanager.UpdateOrCreateConfig(input) diff --git a/proxy/cmd/proxy/cmd/root.go b/proxy/cmd/proxy/cmd/root.go index 765d5c05a..5734efe4e 100644 --- a/proxy/cmd/proxy/cmd/root.go +++ b/proxy/cmd/proxy/cmd/root.go @@ -6,6 +6,7 @@ import ( "os" "os/signal" "strconv" + "strings" "syscall" "time" @@ -13,6 +14,7 @@ import ( "github.com/spf13/cobra" "golang.org/x/crypto/acme" + "github.com/netbirdio/netbird/shared/externalipmap" "github.com/netbirdio/netbird/shared/management/domain" "github.com/netbirdio/netbird/shared/profiling" @@ -74,6 +76,7 @@ var ( certLockMethod string wildcardCertDir string wgPort uint16 + externalIPMap []string proxyProtocol bool preSharedKey string supportsCustomPorts bool @@ -117,6 +120,7 @@ func init() { rootCmd.Flags().StringVar(&certLockMethod, "cert-lock-method", envStringOrDefault("NB_PROXY_CERT_LOCK_METHOD", "auto"), "Certificate lock method for cross-replica coordination: auto, flock, or k8s-lease") rootCmd.Flags().StringVar(&wildcardCertDir, "wildcard-cert-dir", envStringOrDefault("NB_PROXY_WILDCARD_CERT_DIR", ""), "Directory containing wildcard certificate pairs (.crt/.key). Wildcard patterns are extracted from SANs automatically") rootCmd.Flags().Uint16Var(&wgPort, "wg-port", envUint16OrDefault("NB_PROXY_WG_PORT", 0), "WireGuard listen port (0 = random). Fixed port only works with single-account deployments") + rootCmd.Flags().StringSliceVar(&externalIPMap, "external-ip-map", envStringSliceOrDefault("NB_PROXY_EXTERNAL_IP_MAP", nil), "Comma-separated external IPs to advertise as ICE candidates, as IP or IP/internal-IP-or-interface (e.g. '203.0.113.5/10.0.0.5'). Requires a fixed --wg-port forwarded on the NAT") rootCmd.Flags().BoolVar(&proxyProtocol, "proxy-protocol", envBoolOrDefault("NB_PROXY_PROXY_PROTOCOL", false), "Enable PROXY protocol on TCP listeners to preserve client IPs behind L4 proxies") rootCmd.Flags().StringVar(&preSharedKey, "preshared-key", envStringOrDefault("NB_PROXY_PRESHARED_KEY", ""), "Define a pre-shared key for the tunnel between proxy and peers") rootCmd.Flags().BoolVar(&supportsCustomPorts, "supports-custom-ports", envBoolOrDefault("NB_PROXY_SUPPORTS_CUSTOM_PORTS", true), "Whether the proxy can bind arbitrary ports for UDP/TCP passthrough") @@ -221,6 +225,13 @@ func runServer(cmd *cobra.Command, args []string) error { return fmt.Errorf("invalid --trusted-proxies: %w", err) } + for i := range externalIPMap { + externalIPMap[i] = strings.TrimSpace(externalIPMap[i]) + } + if err := externalipmap.Validate(externalIPMap); err != nil { + return fmt.Errorf("invalid --external-ip-map: %w", err) + } + ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGTERM, syscall.SIGINT) defer stop() @@ -248,6 +259,7 @@ func runServer(cmd *cobra.Command, args []string) error { CertLockMethod: nbacme.CertLockMethod(certLockMethod), WildcardCertDir: wildcardCertDir, WireguardPort: wgPort, + NATExternalIPs: externalIPMap, Performance: perf, ProxyProtocol: proxyProtocol, PreSharedKey: preSharedKey, @@ -286,6 +298,14 @@ func envStringOrDefault(key string, def string) string { return v } +func envStringSliceOrDefault(key string, def []string) []string { + v, exists := os.LookupEnv(key) + if !exists || v == "" { + return def + } + return strings.Split(v, ",") +} + func envUint16OrDefault(key string, def uint16) uint16 { v, exists := os.LookupEnv(key) if !exists { diff --git a/proxy/internal/roundtrip/netbird.go b/proxy/internal/roundtrip/netbird.go index 07b497c46..21feb59b7 100644 --- a/proxy/internal/roundtrip/netbird.go +++ b/proxy/internal/roundtrip/netbird.go @@ -144,6 +144,8 @@ type ClientConfig struct { WGPort uint16 PreSharedKey string Performance embed.Performance + // NATExternalIPs mirrors embed.Options.NATExternalIPs. + NATExternalIPs []string // BlockInbound mirrors embed.Options.BlockInbound. Set to true on the // standalone proxy where the embedded client never accepts inbound; // set to false on the private/embedded proxy so the engine creates @@ -402,6 +404,7 @@ func (n *NetBird) createClientEntry(ctx context.Context, accountID types.Account WireguardPort: &wgPort, PreSharedKey: n.clientCfg.PreSharedKey, Performance: n.clientCfg.Performance, + NATExternalIPs: n.clientCfg.NATExternalIPs, } logEmbedOptions(n.logger, accountID, serviceID, publicKey.String(), embedOpts) client, err := embed.New(embedOpts) @@ -914,6 +917,7 @@ func logEmbedOptions(logger *log.Logger, accountID types.AccountID, serviceID ty "management_url": opts.ManagementURL, "log_level": opts.LogLevel, "wg_port": wgPort, + "nat_external_ips": opts.NATExternalIPs, "mtu": mtu, "block_inbound": opts.BlockInbound, "block_lan_access": opts.BlockLANAccess, diff --git a/proxy/lifecycle.go b/proxy/lifecycle.go index f8c74d8b5..206c296ac 100644 --- a/proxy/lifecycle.go +++ b/proxy/lifecycle.go @@ -89,6 +89,9 @@ type Config struct { // WireguardPort is the UDP port for the embedded NetBird tunnel. // Zero asks the OS for a random port. WireguardPort uint16 + // NATExternalIPs are external IPs advertised as ICE candidates by the + // embedded tunnel. Requires a fixed WireguardPort forwarded on the NAT. + NATExternalIPs []string // ProxyProtocol enables PROXY protocol (v1/v2) on TCP listeners. ProxyProtocol bool // PreSharedKey is the WireGuard pre-shared key used between the @@ -160,6 +163,7 @@ func New(ctx context.Context, cfg Config) *Server { ForwardedProto: cfg.ForwardedProto, TrustedProxies: cfg.TrustedProxies, WireguardPort: cfg.WireguardPort, + NATExternalIPs: cfg.NATExternalIPs, ProxyProtocol: cfg.ProxyProtocol, PreSharedKey: cfg.PreSharedKey, Performance: cfg.Performance, diff --git a/proxy/server.go b/proxy/server.go index 083449962..9ad81b50f 100644 --- a/proxy/server.go +++ b/proxy/server.go @@ -203,6 +203,9 @@ type Server struct { // single-account deployments; multiple accounts will fail to bind // the same port. WireguardPort uint16 + // NATExternalIPs maps external IPs advertised as ICE candidates by every + // embedded client. Requires a fixed WireguardPort forwarded on the NAT. + NATExternalIPs []string // Performance configures the tunnel pool/batch sizes for every // embedded client this proxy spawns. Performance embed.Performance @@ -581,10 +584,11 @@ func (s *Server) initManagementClient() error { // inbound listeners. func (s *Server) initNetBirdClient() error { s.netbird = roundtrip.NewNetBird(s.ctx, s.ID, s.ProxyURL, roundtrip.ClientConfig{ - MgmtAddr: s.ManagementAddress, - WGPort: s.WireguardPort, - PreSharedKey: s.PreSharedKey, - Performance: s.Performance, + MgmtAddr: s.ManagementAddress, + WGPort: s.WireguardPort, + PreSharedKey: s.PreSharedKey, + Performance: s.Performance, + NATExternalIPs: s.NATExternalIPs, // On --private the embedded client serves per-account inbound // listeners and must apply management's ACL: keep BlockInbound off // so the engine creates the ACL manager. On the standalone proxy diff --git a/shared/externalipmap/validate.go b/shared/externalipmap/validate.go new file mode 100644 index 000000000..30517007c --- /dev/null +++ b/shared/externalipmap/validate.go @@ -0,0 +1,70 @@ +// Package externalipmap validates the --external-ip-map flag shared by the +// agent and the proxy. +package externalipmap + +import ( + "fmt" + "net" + "strings" +) + +const flagName = "external-ip-map" + +// Validate checks each element is formatted as "IP", "IP/IP" or +// "IP/interface". The engine drops every mapping when one element fails to +// parse, so callers should reject bad input up front. +func Validate(list []string) error { + for _, element := range list { + if element == "" { + return fmt.Errorf("empty string is not a valid input for %s", flagName) + } + + subElements := strings.Split(element, "/") + if len(subElements) > 2 { + return fmt.Errorf("%s is not a valid input for %s. it should be formatted as \"String\" or \"String/String\"", element, flagName) + } + + if !isValidIP(subElements[0]) { + return fmt.Errorf("%s is not a valid input for %s. it should be formatted as \"IP\" or \"IP/IP\", or \"IP/Interface Name\"", element, flagName) + } + + if len(subElements) == 2 { + if err := validateInternal(subElements[1]); err != nil { + return fmt.Errorf("%s is not a valid input for %s. it should be an IP string or a network name: %w", subElements[1], flagName, err) + } + } + } + return nil +} + +func validateInternal(element string) error { + if isValidIP(element) { + return nil + } + validIface, err := isValidInterface(element) + if err != nil { + return fmt.Errorf("validate network interface name: %w", err) + } + if !validIface { + return fmt.Errorf("invalid IP or network interface name not found") + } + return nil +} + +// isValidIP uses net.ParseIP to match the engine's parser exactly. +func isValidIP(ip string) bool { + return net.ParseIP(ip) != nil +} + +func isValidInterface(name string) (bool, error) { + netInterfaces, err := net.Interfaces() + if err != nil { + return false, err + } + for _, iface := range netInterfaces { + if iface.Name == name { + return true, nil + } + } + return false, nil +} diff --git a/shared/externalipmap/validate_test.go b/shared/externalipmap/validate_test.go new file mode 100644 index 000000000..baf3cf787 --- /dev/null +++ b/shared/externalipmap/validate_test.go @@ -0,0 +1,48 @@ +package externalipmap + +import ( + "net" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidate(t *testing.T) { + ifaces, err := net.Interfaces() + require.NoError(t, err) + require.NotEmpty(t, ifaces, "host must have at least one interface") + iface := ifaces[0].Name + + tests := []struct { + name string + input []string + wantErr bool + }{ + {name: "nil", input: nil}, + {name: "single IP", input: []string{"203.0.113.5"}}, + {name: "IP to IP", input: []string{"203.0.113.5/10.0.0.5"}}, + {name: "IP to interface", input: []string{"203.0.113.5/" + iface}}, + {name: "several", input: []string{"203.0.113.5", "203.0.113.6/10.0.0.6"}}, + {name: "empty element", input: []string{""}, wantErr: true}, + // The engine rejects padded values, so validation must too. + {name: "space padded", input: []string{"203.0.113.5", " 203.0.113.6"}, wantErr: true}, + {name: "not an IP", input: []string{"not-an-ip"}, wantErr: true}, + {name: "too many slashes", input: []string{"203.0.113.5/10.0.0.5/10.0.0.6"}, wantErr: true}, + {name: "unknown interface", input: []string{"203.0.113.5/nb-no-such-iface0"}, wantErr: true}, + {name: "external interface", input: []string{iface}, wantErr: true}, + {name: "interface to IP", input: []string{iface + "/10.0.0.5"}, wantErr: true}, + {name: "two interfaces", input: []string{iface + "/" + iface}, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := Validate(tt.input) + if tt.wantErr { + assert.Error(t, err, "input %q should be rejected", tt.input) + return + } + assert.NoError(t, err, "input %q should be accepted", tt.input) + }) + } +} From 89c1b47e9ca73d7bc037f431760674ff919435eb Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:23:29 +0200 Subject: [PATCH 07/10] [management] cancel peer expiration for account if account is gone (#8218) --- management/server/account.go | 21 +++++++++++++++++++-- management/server/account_test.go | 24 ++++++++++++++++++++++++ management/server/peer.go | 6 ++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/management/server/account.go b/management/server/account.go index 90e384ac0..59991a4e4 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -770,7 +770,12 @@ func (am *DefaultAccountManager) peerLoginExpirationJob(ctx context.Context, acc ctx = context.WithValue(ctx, hook.ExecutionContextKey, fmt.Sprintf("%s-PEER-EXPIRATION", hook.SystemSource)) expiredPeers, err := am.getExpiredPeers(ctx, accountID) + if isNotFound(err) { + log.WithContext(ctx).Debugf("account %s not found, stopping peer login expiration job", accountID) + return 0, false + } if err != nil { + log.WithContext(ctx).Errorf("failed getting expired peers for account %s: %v", accountID, err) return peerSchedulerRetryInterval, true } @@ -806,8 +811,12 @@ func (am *DefaultAccountManager) schedulePeerLoginExpiration(ctx context.Context func (am *DefaultAccountManager) peerInactivityExpirationJob(ctx context.Context, accountID string) func() (time.Duration, bool) { return func() (time.Duration, bool) { inactivePeers, err := am.getInactivePeers(ctx, accountID) + if isNotFound(err) { + log.WithContext(ctx).Debugf("account %s not found, stopping peer inactivity expiration job", accountID) + return 0, false + } if err != nil { - log.WithContext(ctx).Errorf("failed getting inactive peers for account %s", accountID) + log.WithContext(ctx).Errorf("failed getting inactive peers for account %s: %v", accountID, err) return peerSchedulerRetryInterval, true } @@ -827,6 +836,14 @@ func (am *DefaultAccountManager) peerInactivityExpirationJob(ctx context.Context } } +func isNotFound(err error) bool { + if err == nil { + return false + } + sErr, ok := status.FromError(err) + return ok && sErr.Type() == status.NotFound +} + // checkAndSchedulePeerInactivityExpiration periodically checks for inactive peers to end their sessions func (am *DefaultAccountManager) checkAndSchedulePeerInactivityExpiration(ctx context.Context, accountID string) { am.peerInactivityExpiry.Cancel(ctx, []string{accountID}) @@ -978,8 +995,8 @@ func (am *DefaultAccountManager) DeleteAccount(ctx context.Context, accountID, u log.WithContext(ctx).Errorf("failed deleting account %s. error: %s", accountID, err) return err } - // cancel peer login expiry job am.peerLoginExpiry.Cancel(ctx, []string{account.Id}) + am.peerInactivityExpiry.Cancel(ctx, []string{account.Id}) am.certChallenges.Forget(account.Id) meta := map[string]any{"account_id": account.Id, "domain": account.Domain, "created_at": account.CreatedAt} diff --git a/management/server/account_test.go b/management/server/account_test.go index 7aa88ee64..ef7525961 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -4990,3 +4990,27 @@ func TestUpdateUserAuthWithSingleMode(t *testing.T) { assert.Contains(t, err.Error(), "query failed") }) } + +func TestDefaultAccountManager_ExpirationJobsStopForDeletedAccount(t *testing.T) { + manager, _, err := createManager(t) + require.NoError(t, err, "unable to create account manager") + + ctx := context.Background() + accountID, err := manager.GetAccountIDByUserID(ctx, auth.UserAuth{UserId: userID}) + require.NoError(t, err, "unable to create an account") + + account, err := manager.Store.GetAccount(ctx, accountID) + require.NoError(t, err, "unable to get account") + require.NoError(t, manager.Store.DeleteAccount(ctx, account), "unable to delete account") + + jobs := map[string]func() (time.Duration, bool){ + "login": manager.peerLoginExpirationJob(ctx, accountID), + "inactivity": manager.peerInactivityExpirationJob(ctx, accountID), + } + for name, job := range jobs { + t.Run(name, func(t *testing.T) { + _, reschedule := job() + assert.False(t, reschedule, "job must not reschedule for a deleted account") + }) + } +} diff --git a/management/server/peer.go b/management/server/peer.go index 1e3f72a3c..f21e5ebc9 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -1702,6 +1702,9 @@ func (am *DefaultAccountManager) getNextPeerExpiration(ctx context.Context, acco } settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthNone, accountID) + if isNotFound(err) { + return 0, false + } if err != nil { log.WithContext(ctx).Errorf("failed to get account settings: %v", err) return peerSchedulerRetryInterval, true @@ -1745,6 +1748,9 @@ func (am *DefaultAccountManager) getNextInactivePeerExpiration(ctx context.Conte } settings, err := am.Store.GetAccountSettings(ctx, store.LockingStrengthNone, accountID) + if isNotFound(err) { + return 0, false + } if err != nil { log.WithContext(ctx).Errorf("failed to get account settings: %v", err) return peerSchedulerRetryInterval, true From dd830f089086790e988b345e5a5f9415ce8e3e15 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:26:04 +0200 Subject: [PATCH 08/10] [management] clean nul byte strings in peer meta (#8219) --- .../internals/shared/grpc/peer_meta_test.go | 45 +++++++++++++++++++ management/internals/shared/grpc/server.go | 41 +++++++++-------- 2 files changed, 68 insertions(+), 18 deletions(-) create mode 100644 management/internals/shared/grpc/peer_meta_test.go diff --git a/management/internals/shared/grpc/peer_meta_test.go b/management/internals/shared/grpc/peer_meta_test.go new file mode 100644 index 000000000..c6336ff1b --- /dev/null +++ b/management/internals/shared/grpc/peer_meta_test.go @@ -0,0 +1,45 @@ +package grpc + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/netbirdio/netbird/shared/management/proto" +) + +func TestExtractPeerMeta_RemovesNulAndInvalidUTF8(t *testing.T) { + meta := extractPeerMeta(context.Background(), &proto.PeerSystemMeta{ + Hostname: "host\x00name", + OS: "\x00\x00\x00\x00\x00\x00s", + OSVersion: "11\xff", + SysSerialNumber: "SN\x00\x00", + Environment: &proto.Environment{Cloud: "\x00aws"}, + NetworkAddresses: []*proto.NetworkAddress{ + {NetIP: "192.168.0.1/24", Mac: "e0:d5\x00"}, + }, + Files: []*proto.File{{Path: "C:\\x\x00.exe"}}, + }) + + assert.Equal(t, "hostname", meta.Hostname) + assert.Equal(t, "s", meta.OS) + assert.Equal(t, "11", meta.OSVersion) + assert.Equal(t, "SN", meta.SystemSerialNumber) + assert.Equal(t, "aws", meta.Environment.Cloud) + assert.Equal(t, "e0:d5", meta.NetworkAddresses[0].Mac) + assert.Equal(t, "C:\\x.exe", meta.Files[0].Path) +} + +func TestCleanClientString(t *testing.T) { + tests := map[string]string{ + "": "", + "\x00\x00": "", + "Wïndows": "Wïndows", + "ssh-ed25519 AAA\x00": "ssh-ed25519 AAA", + "bad\xc3\x28utf8": "bad(utf8", + } + for in, want := range tests { + assert.Equal(t, want, cleanClientString(in), "input %q", in) + } +} diff --git a/management/internals/shared/grpc/server.go b/management/internals/shared/grpc/server.go index 51023c34d..b48a86d55 100644 --- a/management/internals/shared/grpc/server.go +++ b/management/internals/shared/grpc/server.go @@ -553,9 +553,9 @@ func extractPeerMeta(ctx context.Context, meta *proto.PeerSystemMeta) nbpeer.Pee return nbpeer.PeerSystemMeta{} } - osVersion := meta.GetOSVersion() + osVersion := cleanClientString(meta.GetOSVersion()) if osVersion == "" { - osVersion = meta.GetCore() + osVersion = cleanClientString(meta.GetCore()) } networkAddresses := make([]nbpeer.NetworkAddress, 0, len(meta.GetNetworkAddresses())) @@ -567,36 +567,36 @@ func extractPeerMeta(ctx context.Context, meta *proto.PeerSystemMeta) nbpeer.Pee } networkAddresses = append(networkAddresses, nbpeer.NetworkAddress{ NetIP: netAddr, - Mac: addr.GetMac(), + Mac: cleanClientString(addr.GetMac()), }) } files := make([]nbpeer.File, 0, len(meta.GetFiles())) for _, file := range meta.GetFiles() { files = append(files, nbpeer.File{ - Path: file.GetPath(), + Path: cleanClientString(file.GetPath()), Exist: file.GetExist(), ProcessIsRunning: file.GetProcessIsRunning(), }) } return nbpeer.PeerSystemMeta{ - Hostname: meta.GetHostname(), - GoOS: meta.GetGoOS(), - Kernel: meta.GetKernel(), - Platform: meta.GetPlatform(), - OS: meta.GetOS(), + Hostname: cleanClientString(meta.GetHostname()), + GoOS: cleanClientString(meta.GetGoOS()), + Kernel: cleanClientString(meta.GetKernel()), + Platform: cleanClientString(meta.GetPlatform()), + OS: cleanClientString(meta.GetOS()), OSVersion: osVersion, - WtVersion: meta.GetNetbirdVersion(), - UIVersion: meta.GetUiVersion(), - KernelVersion: meta.GetKernelVersion(), + WtVersion: cleanClientString(meta.GetNetbirdVersion()), + UIVersion: cleanClientString(meta.GetUiVersion()), + KernelVersion: cleanClientString(meta.GetKernelVersion()), NetworkAddresses: networkAddresses, - SystemSerialNumber: meta.GetSysSerialNumber(), - SystemProductName: meta.GetSysProductName(), - SystemManufacturer: meta.GetSysManufacturer(), + SystemSerialNumber: cleanClientString(meta.GetSysSerialNumber()), + SystemProductName: cleanClientString(meta.GetSysProductName()), + SystemManufacturer: cleanClientString(meta.GetSysManufacturer()), Environment: nbpeer.Environment{ - Cloud: meta.GetEnvironment().GetCloud(), - Platform: meta.GetEnvironment().GetPlatform(), + Cloud: cleanClientString(meta.GetEnvironment().GetCloud()), + Platform: cleanClientString(meta.GetEnvironment().GetPlatform()), }, Flags: nbpeer.Flags{ RosenpassEnabled: meta.GetFlags().GetRosenpassEnabled(), @@ -618,6 +618,11 @@ func extractPeerMeta(ctx context.Context, meta *proto.PeerSystemMeta) nbpeer.Pee } } +// cleanClientString removes NUL bytes and invalid UTF-8 from a client-reported string, which Postgres rejects in text columns. +func cleanClientString(s string) string { + return strings.ToValidUTF8(strings.ReplaceAll(s, "\x00", ""), "") +} + func capabilitiesToInt32(caps []proto.PeerCapability) []int32 { result := make([]int32, len(caps)) for i, c := range caps { @@ -711,7 +716,7 @@ func (s *Server) Login(ctx context.Context, req *proto.EncryptedMessage) (*proto peer, network, postureChecks, enableSSH, err := s.accountManager.LoginPeer(ctx, types.PeerLogin{ WireGuardPubKey: peerKey.String(), - SSHKey: string(sshKey), + SSHKey: cleanClientString(string(sshKey)), Meta: peerMeta, UserID: userID, SetupKey: loginReq.GetSetupKey(), From afd5c6df6a26eec3c0a81df2374dfe1ce9983ea8 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:27:36 +0200 Subject: [PATCH 09/10] [management] fix proxy conn lifecycle during acks (#8220) --- management/internals/shared/grpc/proxy.go | 57 ++++++++++++--- .../shared/grpc/sync_mappings_test.go | 72 +++++++++++++++++++ proxy/server.go | 8 +++ 3 files changed, 127 insertions(+), 10 deletions(-) diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index 8e93f77b0..439ff1dba 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -152,6 +152,10 @@ type ProxyServiceServer struct { // initial snapshot delivery. Configurable via NB_PROXY_SNAPSHOT_BATCH_SIZE. snapshotBatchSize int + // snapshotAckTimeout bounds the wait for each snapshot batch ack. + // Defaults to defaultSnapshotAckTimeout when zero. + snapshotAckTimeout time.Duration + cancel context.CancelFunc } @@ -168,6 +172,8 @@ const defaultProxyTokenTTL = 5 * time.Minute const defaultSnapshotBatchSize = 500 +const defaultSnapshotAckTimeout = 5 * time.Minute + func snapshotBatchSizeFromEnv() int { if v := os.Getenv("NB_PROXY_SNAPSHOT_BATCH_SIZE"); v != "" { if n, err := strconv.Atoi(v); err == nil && n > 0 { @@ -185,6 +191,14 @@ func (s *ProxyServiceServer) proxyTokenTTL() time.Duration { return defaultProxyTokenTTL } +// snapshotAckWait returns the configured snapshot ack timeout or the default when unset. +func (s *ProxyServiceServer) snapshotAckWait() time.Duration { + if s.snapshotAckTimeout > 0 { + return s.snapshotAckTimeout + } + return defaultSnapshotAckTimeout +} + // proxyConnection represents a connected proxy type proxyConnection struct { proxyID string @@ -484,7 +498,7 @@ func (s *ProxyServiceServer) SyncMappings(stream proto.ProxyService_SyncMappings return err } - if err := s.sendSnapshotSync(stream.Context(), conn, stream); err != nil { + if err := s.sendSnapshotSync(conn.ctx, conn, stream); err != nil { s.cleanupFailedSnapshot(stream.Context(), conn) return fmt.Errorf("send snapshot to proxy %s: %w", params.proxyID, err) } @@ -728,7 +742,7 @@ func (s *ProxyServiceServer) sendSnapshotSync(ctx context.Context, conn *proxyCo return fmt.Errorf("send snapshot batch: %w", err) } - if err := waitForAck(stream); err != nil { + if err := waitForAck(ctx, stream, s.snapshotAckWait()); err != nil { return err } } @@ -740,7 +754,7 @@ func (s *ProxyServiceServer) sendSnapshotSync(ctx context.Context, conn *proxyCo return fmt.Errorf("send snapshot completion: %w", err) } - if err := waitForAck(stream); err != nil { + if err := waitForAck(ctx, stream, s.snapshotAckWait()); err != nil { return err } } @@ -748,15 +762,38 @@ func (s *ProxyServiceServer) sendSnapshotSync(ctx context.Context, conn *proxyCo return nil } -func waitForAck(stream proto.ProxyService_SyncMappingsServer) error { - msg, err := stream.Recv() - if err != nil { - return fmt.Errorf("receive ack: %w", err) +// waitForAck waits for the proxy to ack a snapshot batch. It gives up when ctx +// is canceled or no ack arrives within timeout, since a proxy that abandoned the +// stream without closing it would otherwise block the handler indefinitely. +func waitForAck(ctx context.Context, stream proto.ProxyService_SyncMappingsServer, timeout time.Duration) error { + type recvResult struct { + msg *proto.SyncMappingsRequest + err error } - if msg.GetAck() == nil { - return fmt.Errorf("expected ack, got %T", msg.GetMsg()) + // Recv returns once the handler exits and gRPC tears the stream down. + result := make(chan recvResult, 1) + go func() { + msg, err := stream.Recv() + result <- recvResult{msg: msg, err: err} + }() + + timer := time.NewTimer(timeout) + defer timer.Stop() + + select { + case r := <-result: + if r.err != nil { + return fmt.Errorf("receive ack: %w", r.err) + } + if r.msg.GetAck() == nil { + return fmt.Errorf("expected ack, got %T", r.msg.GetMsg()) + } + return nil + case <-ctx.Done(): + return fmt.Errorf("wait for ack: %w", ctx.Err()) + case <-timer.C: + return fmt.Errorf("no ack within %s", timeout) } - return nil } // heartbeat updates the proxy's last_seen timestamp every minute and diff --git a/management/internals/shared/grpc/sync_mappings_test.go b/management/internals/shared/grpc/sync_mappings_test.go index 6949b79e9..753142ea0 100644 --- a/management/internals/shared/grpc/sync_mappings_test.go +++ b/management/internals/shared/grpc/sync_mappings_test.go @@ -409,3 +409,75 @@ func TestConnectionSendResponse_RoutesToLegacyStream(t *testing.T) { require.Len(t, stream.messages, 1) assert.Equal(t, "svc-2", stream.messages[0].Mapping[0].Id) } + +// blockingSyncStream is a ProxyService_SyncMappingsServer whose Recv blocks +// until the test ends, like a proxy that stopped acking without closing the stream. +type blockingSyncStream struct { + grpc.ServerStream + done chan struct{} +} + +func (s *blockingSyncStream) Recv() (*proto.SyncMappingsRequest, error) { + <-s.done + return nil, context.Canceled +} + +func (s *blockingSyncStream) Send(*proto.SyncMappingsResponse) error { return nil } + +func newBlockingSyncStream(t *testing.T) *blockingSyncStream { + t.Helper() + s := &blockingSyncStream{done: make(chan struct{})} + t.Cleanup(func() { close(s.done) }) + return s +} + +func TestWaitForAck_Ack(t *testing.T) { + stream := &syncRecordingStream{recvMsgs: []*proto.SyncMappingsRequest{ackMsg()}} + + require.NoError(t, waitForAck(context.Background(), stream, time.Second)) +} + +func TestWaitForAck_UnexpectedMessage(t *testing.T) { + stream := &syncRecordingStream{recvMsgs: []*proto.SyncMappingsRequest{ + {Msg: &proto.SyncMappingsRequest_Init{Init: &proto.SyncMappingsInit{}}}, + }} + + assert.ErrorContains(t, waitForAck(context.Background(), stream, time.Second), "expected ack") +} + +func TestWaitForAck_TimesOutWhenProxyStopsAcking(t *testing.T) { + stream := newBlockingSyncStream(t) + + start := time.Now() + err := waitForAck(context.Background(), stream, 50*time.Millisecond) + + assert.ErrorContains(t, err, "no ack within") + assert.Less(t, time.Since(start), time.Second) +} + +func TestWaitForAck_ReturnsWhenConnectionCanceled(t *testing.T) { + stream := newBlockingSyncStream(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + assert.ErrorIs(t, waitForAck(ctx, stream, time.Minute), context.Canceled) +} + +func TestSendSnapshotSync_GivesUpWhenProxyStopsAcking(t *testing.T) { + const cluster = "cluster.example.com" + + ctrl := gomock.NewController(t) + mgr := rpservice.NewMockManager(ctrl) + mgr.EXPECT().GetGlobalServices(gomock.Any()).Return(makeServices(1, cluster), nil) + + s := newSnapshotTestServer(t, 10) + s.serviceManager = mgr + s.snapshotAckTimeout = 50 * time.Millisecond + + stream := newBlockingSyncStream(t) + conn := &proxyConnection{proxyID: "proxy-a", address: cluster, syncStream: stream} + + err := s.sendSnapshotSync(context.Background(), conn, stream) + + assert.ErrorContains(t, err, "no ack within") +} diff --git a/proxy/server.go b/proxy/server.go index 9ad81b50f..3bba38644 100644 --- a/proxy/server.go +++ b/proxy/server.go @@ -1303,6 +1303,10 @@ func (s *Server) proxyCapabilities() *proto.ProxyCapabilities { } func (s *Server) tryGetMappingUpdate(ctx context.Context, client proto.ProxyServiceClient, initialSyncDone *bool, onConnected func()) error { + // Canceling closes the stream on management; a bare return leaves it open. + ctx, cancel := context.WithCancel(ctx) + defer cancel() + connectTime := time.Now() mappingClient, err := client.GetMappingUpdate(ctx, &proto.GetMappingUpdateRequest{ ProxyId: s.ID, @@ -1325,6 +1329,10 @@ func (s *Server) tryGetMappingUpdate(ctx context.Context, client proto.ProxyServ } func (s *Server) trySyncMappings(ctx context.Context, client proto.ProxyServiceClient, initialSyncDone *bool, onConnected func()) error { + // Canceling closes the stream on management; a bare return leaves it open. + ctx, cancel := context.WithCancel(ctx) + defer cancel() + connectTime := time.Now() stream, err := client.SyncMappings(ctx) if err != nil { From 28657607c14d4b5135c48cb79fcb9f4fd0600b42 Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Fri, 16 Oct 2026 02:16:45 +0200 Subject: [PATCH 10/10] [client] Keep interface discovery out of the bind open path (#8226) * [client] Keep interface discovery out of the bind open path Since 0.81.0 a userspace-mode agent whose interface name is not covered by the interface blacklist deadlocks while bringing the device up, and the daemon, the GUI and the service stop all hang. wireguard-go holds the device net lock across bind.Open, our bind builds the UDP mux there, the mux enumerated interfaces in its constructor, and the interface filter probed every unlisted name through wgctrl. For the agent's own interface that probe connected to the daemon's own UAPI endpoint and waited, with no deadline, for a reply the handler could only give after taking the same lock. #7346 moved the first enumeration from the stdnet constructor, which ran before the device existed, to this call site. The mux no longer discovers interfaces when it is constructed. It records whether its socket is bound to an unspecified address and GetConn uses that instead of the length of the discovered list, while GetListenAddresses keeps refreshing on demand, which pion calls during candidate gathering outside the device locks. The WireGuard detector also answers for the agent's own interface without probing, so no filter asks the daemon about itself. * [client] Take the agent's own interface name in the detector constructor Every device implementation reports the configured name, so there is no later name to follow and the setter was state without a reader that needed it. The detector takes the name once and compares without a lock. * [client] Create the default mux Net in the constructor With discovery moved to GetListenAddresses, the nil Net fallback ran there as well, and the first concurrent callers raced on writing it. stdnet.NewNet does no discovery, so the constructor can create it before the mux is shared and the address refresh only reads it. * [client] Follow the created interface name in the detector Android ignores the configured interface name and keeps the TUN name returned by CreateUnmonitoredTUNFromFD, so the detector built from the configuration would still probe the agent's real interface. The engine updates the detector with wgInterface.Name() once the interface exists. * [client] Keep the detector's own interface name immutable On Android the kernel names the TUN and the name can change again on renew, while the UAPI socket cannot be opened there at all, so neither the probe nor a name set after creation can identify the agent's own interface reliably. Excluding that interface on Android belongs to the interface discovery the app provides. Everywhere else the configured name is the created name, so the constructor argument is enough and the detector needs no setter and no lock on the hot path. --- client/iface/bind/ice_bind_test.go | 88 ++++++++++++++++++++ client/iface/udpmux/mux.go | 20 ++--- client/iface/udpmux/mux_test.go | 99 +++++++++++++++++++++++ client/internal/engine.go | 2 +- client/internal/stdnet/wgdetector.go | 19 ++++- client/internal/stdnet/wgdetector_test.go | 36 +++++++++ 6 files changed, 250 insertions(+), 14 deletions(-) create mode 100644 client/iface/udpmux/mux_test.go diff --git a/client/iface/bind/ice_bind_test.go b/client/iface/bind/ice_bind_test.go index 0b8db7640..8dd82d7df 100644 --- a/client/iface/bind/ice_bind_test.go +++ b/client/iface/bind/ice_bind_test.go @@ -2,17 +2,22 @@ package bind import ( "fmt" + "io" "net" "net/netip" "sync" + "sync/atomic" "testing" "time" + "github.com/pion/transport/v3" "github.com/pion/transport/v3/stdnet" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/ipv4" "golang.org/x/net/ipv6" + "golang.zx2c4.com/wireguard/device" + "golang.zx2c4.com/wireguard/tun/netstack" "github.com/netbirdio/netbird/client/iface/wgaddr" ) @@ -326,3 +331,86 @@ func listenUDP(t *testing.T, network, addr string) *net.UDPConn { require.NoError(t, err) return conn } + +type countingNet struct { + transport.Net + calls atomic.Int64 + ifaces []*transport.Interface + before func() +} + +func (n *countingNet) Interfaces() ([]*transport.Interface, error) { + n.calls.Add(1) + if n.before != nil { + n.before() + } + return n.ifaces, nil +} + +func TestICEBind_OpenDoesNotDiscoverInterfaces(t *testing.T) { + n := newCountingNet() + iceBind := NewICEBind(n, testAddress(), 1280) + + for i := 0; i < 2; i++ { + _, port, err := iceBind.Open(0) + require.NoError(t, err) + require.NotZero(t, port, "the bind listens on a port") + assert.Equal(t, int64(0), n.calls.Load(), "opening the bind must not enumerate interfaces, attempt %d", i+1) + require.NoError(t, iceBind.Close()) + } + + _, _, err := iceBind.Open(0) + require.NoError(t, err) + t.Cleanup(func() { _ = iceBind.Close() }) + + mux, err := iceBind.GetICEMux() + require.NoError(t, err) + assert.NotEmpty(t, mux.SingleSocketUDPMux.GetListenAddresses(), "listen addresses come from the discovered interfaces") + assert.Equal(t, int64(1), n.calls.Load(), "interfaces are discovered when the listen addresses are asked for") +} + +func TestICEBind_DeviceUpDoesNotWaitOnInterfaceDiscovery(t *testing.T) { + tunDev, _, err := netstack.CreateNetTUN([]netip.Addr{netip.MustParseAddr("100.64.0.1")}, nil, 1280) + require.NoError(t, err) + + var dev *device.Device + n := newCountingNet() + n.before = func() { + _ = dev.IpcGetOperation(io.Discard) + } + iceBind := NewICEBind(n, testAddress(), 1280) + dev = device.NewDevice(tunDev, iceBind, device.NewLogger(device.LogLevelSilent, "")) + + upDone := make(chan error, 1) + go func() { + upDone <- dev.Up() + }() + + select { + case err := <-upDone: + require.NoError(t, err) + case <-time.After(5 * time.Second): + t.Fatal("device Up did not return: interface discovery ran under the device lock and waited on the device itself") + } + t.Cleanup(dev.Close) + + assert.Equal(t, int64(0), n.calls.Load(), "bringing the device up must not enumerate interfaces") + + mux, err := iceBind.GetICEMux() + require.NoError(t, err) + assert.NotEmpty(t, mux.SingleSocketUDPMux.GetListenAddresses(), "discovery that talks to the device succeeds once the device is up") + assert.Equal(t, int64(1), n.calls.Load(), "interfaces are discovered outside the device lock") +} + +func newCountingNet() *countingNet { + ifc := transport.NewInterface(net.Interface{Index: 1, MTU: 1500, Name: "eth0", Flags: net.FlagUp | net.FlagMulticast}) + ifc.AddAddress(&net.IPNet{IP: net.IPv4(192, 168, 1, 10), Mask: net.CIDRMask(24, 32)}) + return &countingNet{ifaces: []*transport.Interface{ifc}} +} + +func testAddress() wgaddr.Address { + return wgaddr.Address{ + IP: netip.MustParseAddr("100.64.0.1"), + Network: netip.MustParsePrefix("100.64.0.0/10"), + } +} diff --git a/client/iface/udpmux/mux.go b/client/iface/udpmux/mux.go index 3aa0b4d88..2f7287b53 100644 --- a/client/iface/udpmux/mux.go +++ b/client/iface/udpmux/mux.go @@ -47,6 +47,7 @@ type SingleSocketUDPMux struct { // for UDP connection listen at unspecified address localAddrsForUnspecified []net.Addr + unspecified bool } const maxAddrSize = 512 @@ -157,6 +158,9 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { if params.Logger == nil { params.Logger = getLogger() } + if params.Net == nil { + params.Net = stdnet.NewNet(context.Background(), nil, nil) + } mux := &SingleSocketUDPMux{ addressMap: map[string][]*udpMuxedConn{}, @@ -165,6 +169,7 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { connsIPv6: make(map[string]*udpMuxedConn), candidateConnMap: make(map[string]*udpMuxedConn), closedChan: make(chan struct{}, 1), + unspecified: isUnspecified(params.UDPConn.LocalAddr()), pool: &sync.Pool{ New: func() interface{} { // big enough buffer to fit both packet and address @@ -173,7 +178,6 @@ func NewSingleSocketUDPMux(params Params) *SingleSocketUDPMux { }, } - mux.updateLocalAddresses() return mux } @@ -199,10 +203,6 @@ func (m *SingleSocketUDPMux) updateLocalAddresses() { m.params.Logger.Errorf("LocalAddr expected IPV4 or IPV6, got %T", m.params.UDPConn.LocalAddr()) } if len(networks) > 0 { - if m.params.Net == nil { - m.params.Net = stdnet.NewNet(context.Background(), nil, nil) - } - ips, err := localInterfaces(m.params.Net, m.params.InterfaceFilter, nil, networks, true) if err == nil { for _, ip := range ips { @@ -241,10 +241,7 @@ func (m *SingleSocketUDPMux) GetListenAddresses() []net.Addr { // creates the connection if an existing one can't be found func (m *SingleSocketUDPMux) GetConn(ufrag string, addr net.Addr, candidateID string) (net.PacketConn, error) { // don't check addr for mux using unspecified address - m.mu.Lock() - lenLocalAddrs := len(m.localAddrsForUnspecified) - m.mu.Unlock() - if lenLocalAddrs == 0 && m.params.UDPConn.LocalAddr().String() != addr.String() { + if !m.unspecified && m.params.UDPConn.LocalAddr().String() != addr.String() { return nil, fmt.Errorf("invalid address %s", addr.String()) } @@ -508,3 +505,8 @@ func getLogger() logging.LeveledLogger { //fac.Writer = log.StandardLogger().Writer() return fac.NewLogger("ice") } + +func isUnspecified(addr net.Addr) bool { + udpAddr, ok := addr.(*net.UDPAddr) + return ok && udpAddr.IP.IsUnspecified() +} diff --git a/client/iface/udpmux/mux_test.go b/client/iface/udpmux/mux_test.go new file mode 100644 index 000000000..2e2770282 --- /dev/null +++ b/client/iface/udpmux/mux_test.go @@ -0,0 +1,99 @@ +package udpmux + +import ( + "fmt" + "net" + "sync" + "sync/atomic" + "testing" + + "github.com/pion/transport/v3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type countingNet struct { + transport.Net + calls atomic.Int64 + ifaces []*transport.Interface +} + +func (n *countingNet) Interfaces() ([]*transport.Interface, error) { + n.calls.Add(1) + return n.ifaces, nil +} + +func TestNewSingleSocketUDPMux_DoesNotDiscoverInterfaces(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + n := &countingNet{ifaces: []*transport.Interface{testInterface("eth0", net.IPv4(192, 168, 1, 10))}} + + mux := NewSingleSocketUDPMux(Params{UDPConn: conn, Net: n}) + t.Cleanup(func() { _ = mux.Close() }) + + assert.Equal(t, int64(0), n.calls.Load(), "constructing the mux must not enumerate interfaces") + + addrs := mux.GetListenAddresses() + assert.Equal(t, int64(1), n.calls.Load(), "listen addresses are discovered when they are asked for") + + port := conn.LocalAddr().(*net.UDPAddr).Port + require.Len(t, addrs, 1, "one address per discovered interface IP") + assert.Equal(t, fmt.Sprintf("192.168.1.10:%d", port), addrs[0].String(), "the discovered IP is paired with the socket port") +} + +func TestSingleSocketUDPMux_GetConnWithoutListenAddresses(t *testing.T) { + t.Run("unspecified socket accepts any address", func(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + n := &countingNet{} + mux := NewSingleSocketUDPMux(Params{UDPConn: conn, Net: n}) + t.Cleanup(func() { _ = mux.Close() }) + + port := conn.LocalAddr().(*net.UDPAddr).Port + pc, err := mux.GetConn("ufrag", &net.UDPAddr{IP: net.IPv4(192, 168, 1, 10), Port: port}, "candidate") + require.NoError(t, err) + assert.NotNil(t, pc, "a connection is created for a local address of an unspecified socket") + assert.Equal(t, int64(0), n.calls.Load(), "GetConn must not depend on a prior discovery") + }) + + t.Run("specific socket rejects a foreign address", func(t *testing.T) { + conn := listenUDP(t, net.IPv4(127, 0, 0, 1)) + mux := NewSingleSocketUDPMux(Params{UDPConn: conn}) + t.Cleanup(func() { _ = mux.Close() }) + + _, err := mux.GetConn("ufrag", &net.UDPAddr{IP: net.IPv4(192, 168, 1, 10), Port: 1234}, "candidate") + assert.Error(t, err, "an address the socket is not bound to must be rejected") + + pc, err := mux.GetConn("ufrag", conn.LocalAddr(), "candidate") + require.NoError(t, err) + assert.NotNil(t, pc, "the bound address is accepted") + }) +} + +func listenUDP(t *testing.T, ip net.IP) *net.UDPConn { + t.Helper() + conn, err := net.ListenUDP("udp4", &net.UDPAddr{IP: ip, Port: 0}) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + return conn +} + +func testInterface(name string, ip net.IP) *transport.Interface { + ifc := transport.NewInterface(net.Interface{Index: 1, MTU: 1500, Name: name, Flags: net.FlagUp | net.FlagMulticast}) + ifc.AddAddress(&net.IPNet{IP: ip, Mask: net.CIDRMask(24, 32)}) + return ifc +} + +func TestSingleSocketUDPMux_ConcurrentListenAddressesWithDefaultNet(t *testing.T) { + conn := listenUDP(t, net.IPv4zero) + mux := NewSingleSocketUDPMux(Params{UDPConn: conn}) + t.Cleanup(func() { _ = mux.Close() }) + + var wg sync.WaitGroup + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + assert.NotEmpty(t, mux.GetListenAddresses(), "every caller gets at least the socket address") + }() + } + wg.Wait() +} diff --git a/client/internal/engine.go b/client/internal/engine.go index fdf89ae06..c4bdf5121 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -387,7 +387,7 @@ func NewEngine( mgmClient: services.MgmClient, relayManager: services.RelayManager, peerStore: peerstore.NewConnStore(), - wgDetector: stdnet.NewWGDetector(), + wgDetector: stdnet.NewWGDetector(config.WgIfaceName), syncMsgMux: &sync.Mutex{}, config: config, mobileDep: mobileDep, diff --git a/client/internal/stdnet/wgdetector.go b/client/internal/stdnet/wgdetector.go index f34e469e1..8ed290190 100644 --- a/client/internal/stdnet/wgdetector.go +++ b/client/internal/stdnet/wgdetector.go @@ -34,6 +34,8 @@ type WGDetector struct { // probe is replaced in tests; it is the call this type exists to avoid repeating. probe func(string) bool + ownIface string + mu sync.RWMutex cache map[string]wgDetectorEntry @@ -41,11 +43,12 @@ type WGDetector struct { } // NewWGDetector returns a detector with the default time to live. -func NewWGDetector() *WGDetector { +func NewWGDetector(ownIface string) *WGDetector { return &WGDetector{ - ttl: wgDetectorTTL, - probe: probeWireGuard, - cache: make(map[string]wgDetectorEntry), + ttl: wgDetectorTTL, + probe: probeWireGuard, + ownIface: ownIface, + cache: make(map[string]wgDetectorEntry), } } @@ -57,6 +60,10 @@ func (d *WGDetector) IsWireGuard(iFace string) bool { return probeWireGuard(iFace) } + if d.isOwnInterface(iFace) { + return true + } + if isWireGuard, ok := d.cached(iFace); ok { return isWireGuard } @@ -104,6 +111,10 @@ func (d *WGDetector) store(iFace string, isWireGuard bool) { d.cache[iFace] = wgDetectorEntry{isWireGuard: isWireGuard, expireAt: now.Add(d.ttl)} } +func (d *WGDetector) isOwnInterface(iFace string) bool { + return d.ownIface != "" && iFace == d.ownIface +} + func probeWireGuard(iFace string) bool { wg, err := wgctrl.New() if err != nil { diff --git a/client/internal/stdnet/wgdetector_test.go b/client/internal/stdnet/wgdetector_test.go index 68260ae59..28cf85f6a 100644 --- a/client/internal/stdnet/wgdetector_test.go +++ b/client/internal/stdnet/wgdetector_test.go @@ -150,3 +150,39 @@ func TestWGDetectorDropsExpiredEntries(t *testing.T) { assert.Len(t, d.cache, 1, "expired entries of vanished interfaces must be dropped") assert.Contains(t, d.cache, "eth0", "the fresh answer must be kept") } + +func TestWGDetectorOwnInterfaceIsWireGuardWithoutProbing(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + d.ownIface = "NetBird" + + assert.True(t, d.IsWireGuard("NetBird"), "the agent's own interface is a WireGuard device") + assert.Equal(t, int64(0), calls.Load(), "the own interface must never reach the probe") + + assert.False(t, d.IsWireGuard("NetBird2"), "only the exact own name is answered without probing") + assert.Equal(t, int64(1), calls.Load(), "a different interface is probed") +} + +func TestWGDetectorWithoutOwnInterfaceProbesEmptyName(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + + assert.False(t, d.IsWireGuard(""), "an unnamed interface is not mistaken for the own one") + assert.Equal(t, int64(1), calls.Load(), "with no own interface set every name is probed") +} + +func TestInterfaceFilterRejectsOwnInterfaceRegardlessOfDisallowList(t *testing.T) { + d, calls := newCountingDetector(t, time.Minute, false) + d.ownIface = "NetBird" + filter := InterfaceFilter([]string{"wt"}, d) + + assert.False(t, filter("NetBird"), "the own interface is rejected even when the disallow list does not cover it") + assert.Equal(t, int64(0), calls.Load(), "the own interface is not probed") + + assert.True(t, filter("eth0"), "an ordinary interface stays available") + assert.Equal(t, int64(1), calls.Load(), "an ordinary interface is still probed") +} + +func TestNewWGDetectorKeepsTheOwnInterface(t *testing.T) { + d := NewWGDetector("NetbirdWT") + + assert.True(t, d.IsWireGuard("NetbirdWT"), "the constructor records the own interface") +}