From 0e38fcf409d1f602765e971e597edc23f0c0f844 Mon Sep 17 00:00:00 2001 From: riccardom Date: Thu, 1 Oct 2026 14:04:21 +0200 Subject: [PATCH] [management] Make the certificate challenge window one knob to turn Renewal was timed against the window in two different ways: the period derived from it, the sweep interval did not. Shortening the window to watch a renewal in an end-to-end run would have left the refresher still looking for due accounts every quarter of an hour, so nothing would have been renewed in time and the test would have reported the feature broken. Derive the sweep from the period, within bounds that keep a very short window from spinning and a normal one from checking less often than is useful, and allow the window itself to be set through the environment so a run can take seconds instead of half a day. A value that cannot be parsed or falls outside the bounds keeps the default, because a window nobody intended is a security property nobody chose, and an override is logged at warning level since it sets how long a device keeps passing the check after its key is gone. Every instance has to be given the same value: the window is part of the nonce, so instances that disagree reject each other's. --- management/server/certificate_challenge.go | 35 +++++++----- .../server/certificate_challenge_test.go | 22 ++++++-- shared/management/certposture/challenge.go | 48 ++++++++++++++++- .../management/certposture/challenge_test.go | 53 +++++++++++++++++++ 4 files changed, 140 insertions(+), 18 deletions(-) create mode 100644 shared/management/certposture/challenge_test.go diff --git a/management/server/certificate_challenge.go b/management/server/certificate_challenge.go index fcedd056e..22b0fd35d 100644 --- a/management/server/certificate_challenge.go +++ b/management/server/certificate_challenge.go @@ -15,19 +15,27 @@ import ( ) const ( - // certChallengePeriod is how often an account whose policies carry a certificate - // posture check is pushed a fresh challenge. A nonce is accepted for its own window - // and the one before it, so one issued at the very end of a window lives only - // certposture.Window. A third of that leaves a missed run well clear of the edge, - // where a half would put it exactly on it. - certChallengePeriod = certposture.Window / 3 - - // certChallengeTick is how often the refresher looks for accounts that are due. The - // period is measured in hours, so this only has to be fine enough to spread the - // accounts over it. - certChallengeTick = 15 * time.Minute + minCertChallengeTick = time.Second + maxCertChallengeTick = 15 * time.Minute ) +// certChallengePeriod is how often an account whose policies carry a certificate +// posture check is pushed a fresh challenge. A nonce is accepted for its own window and +// the one before it, so one issued at the very end of a window lives only one window. A +// third of that leaves a missed run well clear of the edge, where a half would put it +// exactly on it. +func certChallengePeriod() time.Duration { + return certposture.EffectiveWindow() / 3 +} + +// certChallengeTick is how often the refresher looks for accounts that are due. It is +// derived from the period rather than fixed, so shortening the challenge window for a +// test shortens this with it; the bounds keep a tiny window from spinning and a normal +// one from checking less often than is useful. +func certChallengeTick(period time.Duration) time.Duration { + return min(max(period/10, minCertChallengeTick), maxCertChallengeTick) +} + // certChallengeRefresher pushes a fresh certificate challenge to the peers of every // account that needs one, from a single goroutine. // @@ -52,10 +60,11 @@ type certChallengeRefresher struct { } func newCertChallengeRefresher(refresh func(ctx context.Context, accountID string) bool) *certChallengeRefresher { + period := certChallengePeriod() return &certChallengeRefresher{ due: map[string]time.Time{}, - period: certChallengePeriod, - tick: certChallengeTick, + period: period, + tick: certChallengeTick(period), now: time.Now, refresh: refresh, } diff --git a/management/server/certificate_challenge_test.go b/management/server/certificate_challenge_test.go index f5e8efe44..ba75528c0 100644 --- a/management/server/certificate_challenge_test.go +++ b/management/server/certificate_challenge_test.go @@ -58,10 +58,24 @@ func TestCertChallengePeriod_LeavesRoomForAMissedRun(t *testing.T) { // A nonce issued at the very end of a window is accepted for that window and the // next one only, so the shortest life a peer can be handed is one window. Renewing // has to stay clear of that edge even when a run is missed. - assert.Less(t, 2*certChallengePeriod, certposture.Window, + assert.Less(t, 2*certChallengePeriod(), certposture.EffectiveWindow(), "a missed refresh must still leave the peer's nonce valid, with margin") } +func TestCertChallengeTick_FollowsTheWindow(t *testing.T) { + // An end-to-end test shortens the challenge window to watch a renewal happen. The + // tick has to come down with it, or the refresher would still be looking for due + // accounts every quarter of an hour and never renew anything in time. + short := certChallengeTick(30 * time.Second) + assert.Less(t, short, 30*time.Second, "the tick must be shorter than the period it serves") + assert.GreaterOrEqual(t, short, minCertChallengeTick, "the tick must not spin") + + assert.Equal(t, maxCertChallengeTick, certChallengeTick(24*time.Hour), + "a long period must not stretch the tick without bound") + assert.Equal(t, minCertChallengeTick, certChallengeTick(time.Millisecond), + "a tiny period must not drive the tick below its floor") +} + func TestCertChallengeRefresher_SpreadsAccountsOverThePeriod(t *testing.T) { // Every account on an instance shares the same global challenge window, so a // restart arms them all at once. The offset is what keeps them from fanning out @@ -85,9 +99,9 @@ func TestCertChallengeRefresher_SpreadsAccountsOverThePeriod(t *testing.T) { } func TestCertChallengeOffset_IsStablePerAccount(t *testing.T) { - assert.Equal(t, offsetWithin("account-a", certChallengePeriod), offsetWithin("account-a", certChallengePeriod), + assert.Equal(t, offsetWithin("account-a", certChallengePeriod()), offsetWithin("account-a", certChallengePeriod()), "the same account must keep its slot across restarts") - assert.NotEqual(t, offsetWithin("account-a", certChallengePeriod), offsetWithin("account-b", certChallengePeriod), + assert.NotEqual(t, offsetWithin("account-a", certChallengePeriod()), offsetWithin("account-b", certChallengePeriod()), "two accounts must not share a slot") } @@ -124,7 +138,7 @@ func TestCertChallengeRefresher_TrackIsIdempotent(t *testing.T) { r.Track(context.Background(), "account-a") first := r.due["account-a"] - clock.Advance(certChallengePeriod) + clock.Advance(certChallengePeriod()) r.Track(context.Background(), "account-a") assert.Equal(t, first, r.due["account-a"], "re-tracking must not push the next run further out") diff --git a/shared/management/certposture/challenge.go b/shared/management/certposture/challenge.go index ce56bea0a..d616035f5 100644 --- a/shared/management/certposture/challenge.go +++ b/shared/management/certposture/challenge.go @@ -5,12 +5,27 @@ import ( "crypto/sha256" "encoding/binary" "errors" + "os" + "sync" "time" + + log "github.com/sirupsen/logrus" ) const ( + // Window is the default challenge window. A nonce is accepted for its own window + // and the one before it, so a peer re-proves possession of its key between once + // and twice per window. Window = 12 * time.Hour + // EnvWindow overrides Window, for end-to-end tests that cannot wait half a day to + // watch a renewal. Every management instance has to be given the same value: the + // window is part of the nonce, so instances that disagree reject each other's. + EnvWindow = "NB_CERT_CHALLENGE_WINDOW" + + minWindow = time.Second + maxWindow = 24 * time.Hour + challengeDomain = "netbird-cert-challenge-v1" windowLen = 8 nonceLen = windowLen + sha256.Size @@ -19,6 +34,37 @@ const ( NonceSize = nonceLen ) +var effectiveWindow = sync.OnceValue(resolveWindow) + +// EffectiveWindow returns the challenge window in force, which is Window unless +// EnvWindow overrides it. Everything timed against the window derives from this, so a +// test that shortens it shortens the renewal that goes with it. +func EffectiveWindow() time.Duration { + return effectiveWindow() +} + +func resolveWindow() time.Duration { + val := os.Getenv(EnvWindow) + if val == "" { + return Window + } + + window, err := time.ParseDuration(val) + if err != nil { + log.Warnf("failed to parse %s, keeping the %s certificate challenge window: %v", EnvWindow, Window, err) + return Window + } + if window < minWindow || window > maxWindow { + log.Warnf("%s of %s is outside %s..%s, keeping the %s certificate challenge window", EnvWindow, window, minWindow, maxWindow, Window) + return Window + } + + // Loud on purpose: this sets how long a device can pass the certificate check after + // its key has gone, and it has to match on every instance. + log.Warnf("certificate challenge window overridden to %s by %s", window, EnvWindow) + return window +} + var ( ErrNonceMalformed = errors.New("certificate challenge nonce is malformed") ErrNonceExpired = errors.New("certificate challenge nonce is expired") @@ -33,7 +79,7 @@ type Challenger struct { } func NewChallenger(secret []byte) *Challenger { - return &Challenger{secret: secret, window: Window} + return &Challenger{secret: secret, window: EffectiveWindow()} } func (c *Challenger) Nonce(peerKey []byte, now time.Time) []byte { diff --git a/shared/management/certposture/challenge_test.go b/shared/management/certposture/challenge_test.go new file mode 100644 index 000000000..91cb9d8b2 --- /dev/null +++ b/shared/management/certposture/challenge_test.go @@ -0,0 +1,53 @@ +package certposture + +import ( + "os" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestResolveWindow(t *testing.T) { + // An end-to-end run shortens the window so a renewal can be watched in seconds + // rather than half a day. Anything it cannot make sense of leaves the default in + // place, because a window nobody intended is a security property nobody chose. + tests := []struct { + name string + env string + want time.Duration + }{ + {name: "unset keeps the default", env: "", want: Window}, + {name: "a test-sized window is taken", env: "30s", want: 30 * time.Second}, + {name: "garbage keeps the default", env: "soon", want: Window}, + {name: "below the floor keeps the default", env: "10ms", want: Window}, + {name: "above the ceiling keeps the default", env: "100h", want: Window}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv(EnvWindow, tt.env) + if tt.env == "" { + require.NoError(t, os.Unsetenv(EnvWindow)) + } + assert.Equal(t, tt.want, resolveWindow()) + }) + } +} + +func TestChallenger_HonoursAShortenedWindow(t *testing.T) { + // The window is what a nonce is stamped with, so a shortened one has to make a + // nonce expire sooner, not just change a number in a log line. + short := 2 * time.Second + c := &Challenger{secret: []byte("secret"), window: short} + peerKey := []byte("peer-key") + + issued := time.Unix(1_790_000_000, 0) + nonce := c.Nonce(peerKey, issued) + + require.NoError(t, c.verifyNonce(nonce, peerKey, issued), "a nonce is valid when issued") + require.NoError(t, c.verifyNonce(nonce, peerKey, issued.Add(short)), "and through the window after it") + assert.ErrorIs(t, c.verifyNonce(nonce, peerKey, issued.Add(3*short)), ErrNonceExpired, + "a shortened window must actually expire the nonce sooner") +}