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") +}