diff --git a/secretcache/cacheItem.go b/secretcache/cacheItem.go index caafffa..40f679e 100644 --- a/secretcache/cacheItem.go +++ b/secretcache/cacheItem.go @@ -29,7 +29,8 @@ type secretCacheItem struct { // The next scheduled refresh time for this item. Once the item is accessed // after this time, the item will be synchronously refreshed. - nextRefreshTime int64 + nextRefreshTime time.Time + *cacheObject } @@ -38,17 +39,22 @@ func newSecretCacheItem(config CacheConfig, client SecretsManagerAPIClient, secr return secretCacheItem{ versions: newLRUCache(10), cacheObject: &cacheObject{config: config, client: client, secretId: secretId, refreshNeeded: true}, - nextRefreshTime: time.Now().UnixNano(), + nextRefreshTime: time.Now(), } } // isRefreshNeeded determines if the cached item should be refreshed. +// Dual-check: monotonic clock (immune to wall clock jumps) OR wall clock +// (advances during macOS sleep when monotonic freezes). Extra API calls +// from a forward wall clock jump could occur, but is acceptable +// to avoid serving stale secrets. func (ci *secretCacheItem) isRefreshNeeded() bool { if ci.cacheObject.isRefreshNeeded() { return true } - return ci.nextRefreshTime <= time.Now().UnixNano() + // Check both monotonic and wall clock to determine if refresh is needed + return ci.nextRefreshTime.Compare(ci.timeNow()) <= 0 || ci.nextRefreshTime.Round(0).Compare(ci.timeNowWall()) <= 0 } // getVersionId gets the version id for the given version stage. @@ -103,7 +109,7 @@ func (ci *secretCacheItem) executeRefresh(ctx context.Context) (*secretsmanager. ttl = rand.Int63n(maxTTL/2) + maxTTL/2 } - ci.nextRefreshTime = time.Now().Add(time.Nanosecond * time.Duration(ttl)).UnixNano() + ci.nextRefreshTime = ci.timeNow().Add(time.Nanosecond * time.Duration(ttl)) return result, err } @@ -127,20 +133,19 @@ func (ci *secretCacheItem) getVersion(versionStage string) (*cacheVersion, bool) return secretCacheVersion, true } -// refresh the cached object on demand +// refreshNow forces a refresh with a jittered sleep to avoid retry storms. func (ci *secretCacheItem) refreshNow(ctx context.Context) { ci.refreshNeeded = true - // Generate a random number to have a sleep jitter to not get stuck in a retry loop - sleep := rand.Int63n((forceRefreshJitterSleep+1)-(forceRefreshJitterSleep/2)+1) + (forceRefreshJitterSleep / 2) + sleep := (rand.Int63n((forceRefreshJitterSleep+1)-(forceRefreshJitterSleep/2)+1) + (forceRefreshJitterSleep / 2)) * int64(time.Millisecond) if ci.err != nil { - exceptionSleep := ci.nextRefreshTime - time.Now().UnixNano() + exceptionSleep := int64(ci.nextRefreshTime.Sub(ci.timeNow())) if exceptionSleep > sleep { sleep = exceptionSleep } } - time.Sleep(time.Millisecond * time.Duration(sleep)) + time.Sleep(time.Duration(sleep)) ci.refresh(ctx) } @@ -160,7 +165,7 @@ func (ci *secretCacheItem) refresh(ctx context.Context) { delay := exceptionRetryDelayBase * math.Pow(exceptionRetryGrowthFactor, float64(ci.errorCount)) delay = math.Min(delay, exceptionRetryDelayMax) delayDuration := time.Millisecond * time.Duration(delay) - ci.nextRetryTime = time.Now().Add(delayDuration).UnixNano() + ci.nextRetryTime = ci.timeNow().Add(delayDuration) return } diff --git a/secretcache/cacheObject.go b/secretcache/cacheObject.go index 005ceee..ad39179 100644 --- a/secretcache/cacheObject.go +++ b/secretcache/cacheObject.go @@ -36,8 +36,33 @@ type cacheObject struct { refreshNeeded bool // The time to wait before retrying a failed AWS Secrets Manager request. - nextRetryTime int64 + nextRetryTime time.Time data interface{} + + // now overrides time.Now in tests. nil in production. + now func() time.Time + + // nowWall overrides the wall clock reading in tests. nil in production. + nowWall func() time.Time +} + +// Function used for overriding the time.Now in tests. In production, it will +// just return the result of the normal time.Now function +func (o *cacheObject) timeNow() time.Time { + if o.now != nil { + return o.now() + } + return time.Now() +} + +// timeNowWall returns the current time with the monotonic reading stripped, so +// comparisons against it use the wall clock. Utilized in tests to set a wall clock +// time +func (o *cacheObject) timeNowWall() time.Time { + if o.nowWall != nil { + return o.nowWall().Round(0) + } + return o.timeNow().Round(0) } // isRefreshNeeded determines if the cached object should be refreshed. @@ -50,9 +75,11 @@ func (o *cacheObject) isRefreshNeeded() bool { return false } - if o.nextRetryTime == 0 { + if o.nextRetryTime.IsZero() { return true } - return o.nextRetryTime <= time.Now().UnixNano() + // Compare both the monotonic and wall clock time to reduce possibility of secrets living longer than they should be + // Note: During normal comparison, the monotonic clock is used. Round(0) will force the wall clock reading to be used. + return o.nextRetryTime.Compare(o.timeNow()) <= 0 || o.nextRetryTime.Round(0).Compare(o.timeNowWall()) <= 0 } diff --git a/secretcache/cacheObjects_test.go b/secretcache/cacheObjects_test.go index cf389fc..77bbb2e 100644 --- a/secretcache/cacheObjects_test.go +++ b/secretcache/cacheObjects_test.go @@ -41,13 +41,13 @@ func TestIsRefreshNeededBase(t *testing.T) { t.Fatalf("Expected true when err is not nil") } - obj.nextRetryTime = time.Now().Add(time.Hour * 1).UnixNano() + obj.nextRetryTime = time.Now().Add(time.Hour * 1) if obj.isRefreshNeeded() { t.Fatalf("Expected false when nextRetryTime is in future") } - obj.nextRetryTime = time.Now().Add(-(time.Hour * 1)).UnixNano() + obj.nextRetryTime = time.Now().Add(-(time.Hour * 1)) if !obj.isRefreshNeeded() { t.Fatalf("Expected true when nextRetryTime is in past") } @@ -116,7 +116,7 @@ func TestRefreshNow(t *testing.T) { cacheItem.refreshNow(context.Background()) - if cacheItem.nextRefreshTime == refreshTime { + if cacheItem.nextRefreshTime.Equal(refreshTime) { t.Fatalf("Expected nextRefreshTime to be different") } @@ -126,6 +126,259 @@ func TestRefreshNow(t *testing.T) { } +// Verifies TTL check uses monotonic time: no refresh before TTL, refresh after. +func TestWallClockReset_CacheItemTTL_StuckRefresh(t *testing.T) { + clock := newFakeClock() + mockClient := &dummyClient{} + + cacheItem := secretCacheItem{ + versions: newLRUCache(10), + cacheObject: &cacheObject{ + secretId: "dummy-secret-name", + client: mockClient, + refreshNeeded: false, + now: clock.Now, + nowWall: clock.NowWall, + data: &secretsmanager.DescribeSecretOutput{ + ARN: getStrPtr("dummy-arn"), + Name: getStrPtr("dummy-name"), + }, + }, + nextRefreshTime: clock.Now().Add(time.Hour), + } + + if cacheItem.isRefreshNeeded() { + t.Fatalf("Expected no refresh needed when TTL has not expired") + } + + // Advance only the monotonic clock, so the wall clock cannot be what + // triggers the refresh below. + clock.AdvanceMonotonic(30 * time.Minute) + if cacheItem.isRefreshNeeded() { + t.Fatalf("Expected no refresh needed — only 30 minutes elapsed, TTL is 1 hour") + } + + clock.AdvanceMonotonic(time.Hour) + if !cacheItem.isRefreshNeeded() { + t.Fatalf("Expected refresh needed — 1h30m elapsed, exceeds 1 hour TTL") + } +} + +// Verifies exponential backoff respects the injectable clock. +func TestWallClockReset_ErrorBackoff_UsesCorrectClock(t *testing.T) { + clock := newFakeClock() + callCount := 0 + failingClient := &failingDummyClient{describeCallCount: &callCount} + + cacheItem := secretCacheItem{ + versions: newLRUCache(10), + cacheObject: &cacheObject{ + secretId: "dummy-secret-name", + client: failingClient, + refreshNeeded: true, + now: clock.Now, + nowWall: clock.NowWall, + }, + nextRefreshTime: clock.Now(), + } + + cacheItem.refresh(context.Background()) + if callCount != 1 { + t.Fatalf("Expected 1 call, got %d", callCount) + } + if cacheItem.err == nil { + t.Fatalf("Expected error to be set") + } + + if cacheItem.cacheObject.isRefreshNeeded() { + t.Fatalf("Expected no refresh during backoff period") + } + + clock.AdvanceMonotonic(time.Millisecond) + if cacheItem.cacheObject.isRefreshNeeded() { + t.Fatalf("Expected no refresh during backoff — only 1ms elapsed, backoff is 2ms") + } + + clock.AdvanceMonotonic(time.Millisecond) + if !cacheItem.cacheObject.isRefreshNeeded() { + t.Fatalf("Expected refresh needed, exceeds 2ms backoff") + } + + cacheItem.refresh(context.Background()) + if callCount != 2 { + t.Fatalf("Expected 2 calls, got %d", callCount) + } +} + +// Regression: refreshNow must not block when nextRefreshTime is in the past. +func TestWallClockReset_RefreshNow_DoesNotBlock(t *testing.T) { + clock := newFakeClock() + mockClient := &dummyClient{} + + // Simulate Wall clock being set 24 hours back in time + cacheItem := secretCacheItem{ + versions: newLRUCache(10), + cacheObject: &cacheObject{ + secretId: "dummy-secret-name", + client: mockClient, + refreshNeeded: false, + err: errors.New("previous API failure"), + errorCount: 3, + now: clock.Now, + nowWall: clock.NowWall, + }, + nextRefreshTime: clock.Now().Add(24 * time.Hour), + } + clock.AdvanceMonotonic(24 * time.Hour) + + // Verify that it will refresh within 6 seconds + // since the monotonic time should be correct + done := make(chan struct{}) + go func() { + cacheItem.refreshNow(context.Background()) + close(done) + }() + select { + case <-done: + case <-time.After(6 * time.Second): + t.Fatalf("refreshNow blocked longer than 6 seconds") + } +} + +// Wall clock fallback catches staleness when monotonic clock freezes (macOS sleep). +func TestDualCheck_WallClockFallback_MonotonicFrozen(t *testing.T) { + clock := newFakeClock() + mockClient := &dummyClient{} + + cacheItem := secretCacheItem{ + versions: newLRUCache(10), + cacheObject: &cacheObject{ + secretId: "dummy-secret-name", + client: mockClient, + refreshNeeded: false, + now: clock.Now, + nowWall: clock.NowWall, + data: &secretsmanager.DescribeSecretOutput{ + ARN: getStrPtr("dummy-arn"), + Name: getStrPtr("dummy-name"), + }, + }, + // TTL expires an hour from now by both clocks. + nextRefreshTime: clock.Now().Add(time.Hour), + } + + if cacheItem.isRefreshNeeded() { + t.Fatalf("Expected no refresh needed — TTL has not expired on either clock") + } + + // Simulate the host suspending for 24 hours: the monotonic clock freezes + // where it is while the wall clock keeps advancing. The monotonic branch + // still sees the TTL as an hour away, so only the wall clock fallback can + // catch that the secret is now stale. + clock.AdvanceWall(24 * time.Hour) + + if cacheItem.nextRefreshTime.Compare(clock.Now()) <= 0 { + t.Fatalf("Test precondition broken: monotonic branch should not see the TTL as expired") + } + + if !cacheItem.isRefreshNeeded() { + t.Fatalf("Expected refresh needed — wall clock advanced 24h past the TTL") + } +} + +// Wall clock fallback catches an elapsed retry backoff when the monotonic clock +// freezes (macOS sleep) after error. +func TestDualCheck_ErrorRetryTime_WallClockFallback_MonotonicFrozen(t *testing.T) { + clock := newFakeClock() + callCount := 0 + failingClient := &failingDummyClient{describeCallCount: &callCount} + + cacheItem := secretCacheItem{ + versions: newLRUCache(10), + cacheObject: &cacheObject{ + secretId: "dummy-secret-name", + client: failingClient, + refreshNeeded: true, + now: clock.Now, + nowWall: clock.NowWall, + }, + nextRefreshTime: clock.Now(), + } + + // Fail a refresh so err is set and nextRetryTime is armed. + cacheItem.refresh(context.Background()) + + if cacheItem.err == nil { + t.Fatalf("Expected error to be set") + } + + if cacheItem.nextRetryTime.IsZero() { + t.Fatalf("Expected nextRetryTime to be armed") + } + + if cacheItem.cacheObject.isRefreshNeeded() { + t.Fatalf("Expected no refresh — backoff has not elapsed on either clock") + } + + // Simulate the monotonic clock freezing by advancing the wall clock + clock.AdvanceWall(24 * time.Hour) + + if cacheItem.nextRetryTime.Compare(clock.Now()) <= 0 { + t.Fatalf("Test precondition broken: monotonic branch should not see the backoff as elapsed") + } + + if !cacheItem.cacheObject.isRefreshNeeded() { + t.Fatalf("Expected refresh needed — wall clock advanced 24h past the backoff") + } +} + +// fakeClock models the monotonic and wall clocks as separate offsets so tests +// can advance them independently. Wire Now into cacheObject.now and NowWall +// into cacheObject.nowWall. +type fakeClock struct { + base time.Time + monotonicOffset time.Duration + wallOffset time.Duration +} + +func newFakeClock() *fakeClock { + return &fakeClock{base: time.Now()} +} + +// Now is the monotonic reading. Only monotonicOffset moves it. +func (fc *fakeClock) Now() time.Time { + return fc.base.Add(fc.monotonicOffset) +} + +// NowWall is the wall clock reading, with the monotonic reading stripped so +// comparisons against it use the wall clock. Only wallOffset moves it. +func (fc *fakeClock) NowWall() time.Time { + return fc.base.Round(0).Add(fc.wallOffset) +} + +// AdvanceMonotonic moves the monotonic clock forward, leaving the wall clock +// where it is. +func (fc *fakeClock) AdvanceMonotonic(d time.Duration) { + fc.monotonicOffset += d +} + +// AdvanceWall moves the wall clock forward, leaving the monotonic clock where it +// is. This is what happens when the host suspends (e.g. macOS sleep) and the +// monotonic clock freezes while the wall clock keeps going. +func (fc *fakeClock) AdvanceWall(d time.Duration) { + fc.wallOffset += d +} + +type failingDummyClient struct { + SecretsManagerAPIClient + describeCallCount *int +} + +func (f *failingDummyClient) DescribeSecret(context context.Context, input *secretsmanager.DescribeSecretInput, opts ...func(*secretsmanager.Options)) (*secretsmanager.DescribeSecretOutput, error) { + *f.describeCallCount++ + return nil, errors.New("service unavailable") +} + type dummyClient struct { SecretsManagerAPIClient } diff --git a/secretcache/cacheVersion.go b/secretcache/cacheVersion.go index e5b9680..72b0a4b 100644 --- a/secretcache/cacheVersion.go +++ b/secretcache/cacheVersion.go @@ -55,8 +55,8 @@ func (cv *cacheVersion) refresh(ctx context.Context) { cv.err = err delay := exceptionRetryDelayBase * math.Pow(exceptionRetryGrowthFactor, float64(cv.errorCount)) delay = math.Min(delay, exceptionRetryDelayMax) - delayDuration := time.Nanosecond * time.Duration(delay) - cv.nextRetryTime = time.Now().Add(delayDuration).UnixNano() + delayDuration := time.Millisecond * time.Duration(delay) + cv.nextRetryTime = cv.timeNow().Add(delayDuration) return }