From 9dbc53b6705d3aff4dd0bc34a433fc6c9c4ae4a7 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Fri, 11 Sep 2026 19:26:51 -0500 Subject: [PATCH] appview: stop serving service tokens past their expiry, and challenge the client when the hold rejects one Seen in production on 2026-09-11: three cold pulls of a 22-layer image failed with BLOB_UNKNOWN for layers that exist. The hold had answered 403 "service token authentication failed: token has expired", and the same blobs served fine a minute later. Three things lined up. The registry middleware's validation cache kept a fetched service token for a flat 45 seconds regardless of its real remaining life, so a token fetched with 12 seconds left was still handed to the hold half a minute after it died. The registry JWT is stamped from the auth cache's expiry, which trailed the real exp by only 10 seconds, while distribution accepts a JWT for 60 seconds past its exp, so a client could hold an accepted JWT for most of a minute after the credential behind it was gone. And the hold's 403 was flattened to BLOB_UNKNOWN, so the client failed instead of re-authenticating. Now the validation cache bounds an entry by the token's exp minus a shared ServiceTokenSafetyMargin of 60 seconds, the same margin the auth cache and the JWT stamp use, chosen to equal distribution's leeway so the last instant a JWT is accepted is the service token's real exp. A PDS that grants less than the margin gets half its remaining life instead of an already-past deadline. When the hold rejects the service token as expired or missing, the appview drops both cached copies and returns a 401 challenge so Docker and crane re-run the token dance and retry; a genuine permission denial stays a 403, and a hold that is down still maps to blob unknown. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01EvFJr4Dwz8p2NDAeXmgmBt --- pkg/appview/authgate/service_auth_fetcher.go | 6 +- pkg/appview/middleware/registry.go | 135 +++++++++-- .../middleware/validation_cache_test.go | 151 ++++++++++++ pkg/appview/storage/context.go | 9 + pkg/appview/storage/proxy_blob_auth_test.go | 226 ++++++++++++++++++ pkg/appview/storage/proxy_blob_store.go | 89 ++++++- pkg/appview/storage/proxy_blob_store_test.go | 40 +++- pkg/auth/cache.go | 67 +++++- pkg/auth/cache_test.go | 96 ++++++++ pkg/auth/token/handler.go | 16 +- pkg/auth/token/handler_margin_test.go | 81 +++++++ 11 files changed, 867 insertions(+), 49 deletions(-) create mode 100644 pkg/appview/middleware/validation_cache_test.go create mode 100644 pkg/appview/storage/proxy_blob_auth_test.go create mode 100644 pkg/auth/token/handler_margin_test.go diff --git a/pkg/appview/authgate/service_auth_fetcher.go b/pkg/appview/authgate/service_auth_fetcher.go index 0d8ae2f..a983a6c 100644 --- a/pkg/appview/authgate/service_auth_fetcher.go +++ b/pkg/appview/authgate/service_auth_fetcher.go @@ -89,8 +89,10 @@ func (f *ServiceAuthFetcher) Fetch(ctx context.Context, did, authMethod string) } // GetOrFetchServiceToken* caches as a side effect — read the granted - // expiry back. The cached value already has the 10s safety margin from - // pkg/auth/cache.go applied. + // expiry back. The cached value already has auth.ServiceTokenSafetyMargin + // subtracted from the PDS-granted exp (pkg/auth/cache.go), and the token + // handler stamps the JWT's exp from exactly this value, so the JWT cannot + // outlive the service token behind it. _, expiresAt := f.cache.Get(did, holdDID) if expiresAt.IsZero() { return time.Time{}, errors.New("service-auth fetched but cache miss") diff --git a/pkg/appview/middleware/registry.go b/pkg/appview/middleware/registry.go index 56a19db..104b7e3 100644 --- a/pkg/appview/middleware/registry.go +++ b/pkg/appview/middleware/registry.go @@ -35,6 +35,21 @@ const pullerDIDKey contextKey = "puller.did" // hasPushScopeKey is the context key for storing whether the JWT has push scope const hasPushScopeKey contextKey = "token.has_push_scope" +// validationCacheTTL is the longest a fetched service token is reused from the +// validation cache. It covers a typical Docker push, whose many blob requests +// would otherwise each race on OAuth/DPoP. +// +// It is a ceiling, not the actual lifetime: getOrFetch also clamps the entry to +// the token's own exp minus auth.ServiceTokenSafetyMargin. Without that clamp a +// token fetched with 12s of life left was still served for the full 45s, so the +// hold saw an expired service token and answered 403 "token has expired" on +// blobs that exist. +const validationCacheTTL = 45 * time.Second + +// validationCacheErrorTTL is how long a failed fetch is remembered so +// concurrent requests fast-fail instead of stampeding the PDS. +const validationCacheErrorTTL = 5 * time.Second + // validationCacheEntry stores a validated service token with expiration type validationCacheEntry struct { serviceToken string @@ -154,15 +169,14 @@ func (vc *validationCache) getOrFetch(ctx context.Context, cacheKey string, fetc entry.inFlight = false if err != nil { - // Cache errors for 5 seconds (fast-fail for subsequent requests) + // Cache errors briefly (fast-fail for subsequent requests) entry.err = err - entry.validUntil = time.Now().Add(5 * time.Second) + entry.validUntil = time.Now().Add(validationCacheErrorTTL) entry.serviceToken = "" } else { - // Cache token for 45 seconds (covers typical Docker push operation) entry.err = nil entry.serviceToken = serviceToken - entry.validUntil = time.Now().Add(45 * time.Second) + entry.validUntil = tokenValidUntil(serviceToken) } // Signal completion to waiting goroutines @@ -172,6 +186,65 @@ func (vc *validationCache) getOrFetch(ctx context.Context, cacheKey string, fetc return serviceToken, err } +// tokenValidUntil bounds a cached service token by its own exp claim, not just +// by the flat validation-cache TTL. +// +// The cache used to pin any successful fetch for validationCacheTTL regardless +// of how much life the token actually had. The token is minted with a fixed +// absolute expiry, so a fetch that landed near the end of one (the auth cache +// hands back a token until it is close to expiry, and the PDS may grant less +// than asked) left the appview presenting a dead credential to the hold for the +// rest of the 45s. The hold answered 403 "token has expired" and cold pulls +// failed on layers that exist. +// +// A token whose exp cannot be parsed keeps the flat TTL: the appview cannot do +// better than its previous behaviour for a token shape it does not understand, +// and pkg/auth's cache applies the same fallback. +func tokenValidUntil(serviceToken string) time.Time { + validUntil := time.Now().Add(validationCacheTTL) + + exp, err := auth.ServiceTokenExpiry(serviceToken) + if err != nil { + slog.Warn("Service token exp unreadable, using flat validation cache TTL", + "component", "registry/middleware", + "error", err, + "ttl", validationCacheTTL) + return validUntil + } + + // Same margin the auth cache and the registry JWT's exp use, so all three + // stop trusting the token at the same moment. + if safe := exp.Add(-auth.ServiceTokenSafetyMargin); safe.Before(validUntil) { + return safe + } + return validUntil +} + +// invalidate expires the entry for cacheKey so the next getOrFetch re-mints. +// Called when the hold rejects the token we handed it: the entry is stale by +// definition and replaying it would fail the client's retry the same way. +// +// The entry is expired in place rather than deleted from the map because +// concurrent goroutines already hold the pointer; an in-flight fetch is left +// alone because it is about to store a fresh token anyway. +func (vc *validationCache) invalidate(cacheKey string) { + vc.mu.RLock() + entry, exists := vc.entries[cacheKey] + vc.mu.RUnlock() + + if !exists { + return + } + + entry.mu.Lock() + if !entry.inFlight { + entry.serviceToken = "" + entry.err = nil + entry.validUntil = time.Time{} + } + entry.mu.Unlock() +} + // LabelChecker checks whether content has been taken down via ATProto labels. type LabelChecker interface { IsTakenDown(did, repository string) (bool, error) @@ -451,6 +524,9 @@ func (nr *NamespaceResolver) Repository(ctx context.Context, name reference.Name // IMPORTANT: Use PULLER's DID/PDS for service token, not owner's! // The puller (authenticated user) needs to authenticate to the hold service. var serviceToken string + // invalidateServiceToken is handed to the blob store so it can drop this + // token when the hold rejects it; nil unless we actually fetched one. + var invalidateServiceToken func() authMethod, _ := ctx.Value(authMethodKey).(string) pullerDID, _ := ctx.Value(pullerDIDKey).(string) hasPushScope, _ := ctx.Value(hasPushScopeKey).(bool) @@ -537,6 +613,16 @@ func (nr *NamespaceResolver) Repository(ctx context.Context, name reference.Name // Generic service token error return nil, nr.authErrorMessage(fmt.Sprintf("Failed to obtain storage credentials: %v", fetchErr)) } + + // Both caches have to go: the validation cache would otherwise + // replay the rejected token for the rest of its window, and + // pkg/auth's cache would hand the same one straight back to the + // refetch. + vc := nr.validationCache + invalidateServiceToken = func() { + vc.invalidate(cacheKey) + auth.InvalidateServiceToken(pullerDID, holdDID) + } } } else { slog.Debug("Skipping service token fetch for unauthenticated request", @@ -631,26 +717,27 @@ func (nr *NamespaceResolver) Repository(ctx context.Context, name reference.Name // 3. The refresher already caches sessions efficiently (in-memory + DB) // 4. Caching the repository with a stale ATProtoClient causes refresh token errors registryCtx := &storage.RegistryContext{ - DID: did, - Handle: handle, - HoldDID: holdDID, - HoldURL: holdURL, - PDSEndpoint: pdsEndpoint, - Repository: repositoryName, - ServiceToken: serviceToken, // Cached service token from puller's PDS - ATProtoClient: atprotoClient, - AuthMethod: authMethod, // Auth method from JWT token - PullerDID: pullerDID, // Authenticated user making the request - PullerPDSEndpoint: pullerPDSEndpoint, // Puller's PDS for service token refresh - HasPushScope: hasPushScope, // Whether JWT has push scope (for pull stats filtering) - Anonymous: pullerDID == "", // No puller identity: hold decides via captain.Public - AutoRemoveUntagged: prefs.AutoRemoveUntagged, - Database: nr.database, - Authorizer: nr.authorizer, - Refresher: nr.refresher, - ReadmeFetcher: nr.readmeFetcher, - WebhookDispatcher: nr.webhookDispatcher, - ManifestRefChecker: nr.manifestRefChecker, + DID: did, + Handle: handle, + HoldDID: holdDID, + HoldURL: holdURL, + PDSEndpoint: pdsEndpoint, + Repository: repositoryName, + ServiceToken: serviceToken, // Cached service token from puller's PDS + InvalidateServiceToken: invalidateServiceToken, + ATProtoClient: atprotoClient, + AuthMethod: authMethod, // Auth method from JWT token + PullerDID: pullerDID, // Authenticated user making the request + PullerPDSEndpoint: pullerPDSEndpoint, // Puller's PDS for service token refresh + HasPushScope: hasPushScope, // Whether JWT has push scope (for pull stats filtering) + Anonymous: pullerDID == "", // No puller identity: hold decides via captain.Public + AutoRemoveUntagged: prefs.AutoRemoveUntagged, + Database: nr.database, + Authorizer: nr.authorizer, + Refresher: nr.refresher, + ReadmeFetcher: nr.readmeFetcher, + WebhookDispatcher: nr.webhookDispatcher, + ManifestRefChecker: nr.manifestRefChecker, } return storage.NewRoutingRepository(repo, registryCtx), nil diff --git a/pkg/appview/middleware/validation_cache_test.go b/pkg/appview/middleware/validation_cache_test.go new file mode 100644 index 0000000..d4672f8 --- /dev/null +++ b/pkg/appview/middleware/validation_cache_test.go @@ -0,0 +1,151 @@ +package middleware + +import ( + "context" + "encoding/base64" + "fmt" + "testing" + "time" + + "atcr.io/pkg/auth" +) + +// serviceTokenExpiring builds an unsigned JWT whose exp claim is the given +// time. The validation cache only reads exp; the hold is what verifies +// signatures. +func serviceTokenExpiring(at time.Time) string { + payload := fmt.Sprintf(`{"exp":%d}`, at.Unix()) + return "header." + base64.RawURLEncoding.EncodeToString([]byte(payload)) + ".signature" +} + +// entryValidUntil reads back the cached entry's deadline. +func entryValidUntil(t *testing.T, vc *validationCache, cacheKey string) time.Time { + t.Helper() + + vc.mu.RLock() + entry, ok := vc.entries[cacheKey] + vc.mu.RUnlock() + if !ok { + t.Fatalf("no validation cache entry for %q", cacheKey) + } + + entry.mu.Lock() + defer entry.mu.Unlock() + return entry.validUntil +} + +// A token with only 20s of life must not be pinned for the flat 45s TTL: that +// is the production bug, where the appview kept presenting a dead service token +// and the hold answered 403 "token has expired" on blobs that exist. +func TestValidationCache_BoundsEntryByTokenExpiry(t *testing.T) { + vc := newValidationCache() + cacheKey := "did:plc:puller:did:web:hold.example.com" + + realExp := time.Now().Add(20 * time.Second) + token := serviceTokenExpiring(realExp) + + got, err := vc.getOrFetch(context.Background(), cacheKey, func() (string, error) { + return token, nil + }) + if err != nil { + t.Fatalf("getOrFetch() error = %v", err) + } + if got != token { + t.Fatalf("getOrFetch() returned %q, want the fetched token", got) + } + + validUntil := entryValidUntil(t, vc, cacheKey) + + want := realExp.Add(-auth.ServiceTokenSafetyMargin) + if validUntil.After(want) { + t.Errorf("entry valid until %v, want no later than %v (exp minus %v)", + validUntil, want, auth.ServiceTokenSafetyMargin) + } + + if flat := time.Now().Add(validationCacheTTL); !validUntil.Before(flat) { + t.Errorf("entry valid until %v, which is the flat %v TTL: the token's own exp was ignored", + validUntil, validationCacheTTL) + } +} + +// A long-lived token is still capped by the flat TTL, so the cache keeps +// rechecking the underlying session rather than holding one token for minutes. +func TestValidationCache_LongLivedTokenKeepsFlatTTL(t *testing.T) { + vc := newValidationCache() + cacheKey := "did:plc:puller:did:web:hold.example.com" + + token := serviceTokenExpiring(time.Now().Add(1 * time.Hour)) + + if _, err := vc.getOrFetch(context.Background(), cacheKey, func() (string, error) { + return token, nil + }); err != nil { + t.Fatalf("getOrFetch() error = %v", err) + } + + validUntil := entryValidUntil(t, vc, cacheKey) + + want := time.Now().Add(validationCacheTTL) + if diff := validUntil.Sub(want); diff < -2*time.Second || diff > 2*time.Second { + t.Errorf("entry valid until %v, want ~%v (the flat TTL)", validUntil, want) + } +} + +// A token whose exp cannot be read falls back to the behaviour that shipped +// before: the flat TTL. The appview can't do better for a shape it doesn't +// understand, and refusing to cache would stampede the PDS. +func TestValidationCache_UnparsableTokenKeepsFlatTTL(t *testing.T) { + vc := newValidationCache() + cacheKey := "did:plc:puller:did:web:hold.example.com" + + if _, err := vc.getOrFetch(context.Background(), cacheKey, func() (string, error) { + return "not-a-jwt", nil + }); err != nil { + t.Fatalf("getOrFetch() error = %v", err) + } + + validUntil := entryValidUntil(t, vc, cacheKey) + + want := time.Now().Add(validationCacheTTL) + if diff := validUntil.Sub(want); diff < -2*time.Second || diff > 2*time.Second { + t.Errorf("entry valid until %v, want ~%v (the flat TTL)", validUntil, want) + } +} + +// invalidate must force the next call to re-mint, which is what makes the 401 +// we send to Docker worth retrying. +func TestValidationCache_InvalidateForcesRefetch(t *testing.T) { + vc := newValidationCache() + cacheKey := "did:plc:puller:did:web:hold.example.com" + + fetches := 0 + fetch := func() (string, error) { + fetches++ + return serviceTokenExpiring(time.Now().Add(5 * time.Minute)), nil + } + + if _, err := vc.getOrFetch(context.Background(), cacheKey, fetch); err != nil { + t.Fatalf("getOrFetch() error = %v", err) + } + if _, err := vc.getOrFetch(context.Background(), cacheKey, fetch); err != nil { + t.Fatalf("getOrFetch() error = %v", err) + } + if fetches != 1 { + t.Fatalf("second call fetched again (%d fetches), expected a cache hit", fetches) + } + + vc.invalidate(cacheKey) + + if _, err := vc.getOrFetch(context.Background(), cacheKey, fetch); err != nil { + t.Fatalf("getOrFetch() after invalidate error = %v", err) + } + if fetches != 2 { + t.Errorf("got %d fetches after invalidate, want 2", fetches) + } +} + +// invalidate on a key that was never cached is a no-op, not a panic: the blob +// store calls it on whatever request hit the rejection. +func TestValidationCache_InvalidateUnknownKey(t *testing.T) { + vc := newValidationCache() + vc.invalidate("did:plc:nobody:did:web:hold.example.com") +} diff --git a/pkg/appview/storage/context.go b/pkg/appview/storage/context.go index 7cc40cb..da1eb6e 100644 --- a/pkg/appview/storage/context.go +++ b/pkg/appview/storage/context.go @@ -75,6 +75,15 @@ type RegistryContext struct { HasPushScope bool // Whether the JWT token has push scope (used to filter pull stats) Anonymous bool // Request carries no puller identity (anonymous pull); hold decides via captain.Public + // InvalidateServiceToken drops every process-level cache holding the + // puller's service token for this hold. Called when the hold rejects the + // token as expired or invalid: without it the client's retry (prompted by + // the 401 we return) would be handed the very same dead token and fail + // again. Set by the registry middleware, which owns the per-process + // validation cache; nil on requests that never fetched a token (anonymous + // pulls) and in tests. + InvalidateServiceToken func() + // Per-request user preferences AutoRemoveUntagged bool // Whether to auto-delete untagged manifests on tag overwrite diff --git a/pkg/appview/storage/proxy_blob_auth_test.go b/pkg/appview/storage/proxy_blob_auth_test.go new file mode 100644 index 0000000..b9e9845 --- /dev/null +++ b/pkg/appview/storage/proxy_blob_auth_test.go @@ -0,0 +1,226 @@ +package storage + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/distribution/distribution/v3" + "github.com/distribution/distribution/v3/registry/api/errcode" + "github.com/opencontainers/go-digest" +) + +// The two bodies below are copied from what the hold actually writes. Its OCI +// blob handler answers every read authorization failure with +// http.Error(w, "authorization failed: "+err.Error(), http.StatusForbidden), +// and the error underneath is either the service-token wrapper from +// ValidateBlobReadAccess or an *AuthError. Same status, opposite meaning. +const ( + holdExpiredTokenBody = "authorization failed: service token authentication failed: token has expired" + holdPermissionBody = "authorization failed: access denied for blob:read: crew member lacks permission (required: blob:read or blob:write)" +) + +const authTestDigest = digest.Digest("sha256:abc123def456abc123def456abc123def456abc123def456abc123def456abcd") + +// statWithHoldFailure points a blob store at a hold that rejects the presign +// with the given status and body, and returns Stat's error plus whether the +// service token was invalidated. +func statWithHoldFailure(t *testing.T, status int, body string) (error, bool) { + t.Helper() + + holdServer := newMockHoldServer(t, "http://s3.invalid") + defer holdServer.Close() + + holdServer.mu.Lock() + holdServer.PresignAuthFailure = &mockAuthFailure{Status: status, Body: body} + holdServer.mu.Unlock() + + invalidated := false + store := createTestProxyBlobStore(t, holdServer.URL) + store.ctx.PullerDID = "did:plc:puller" + store.ctx.InvalidateServiceToken = func() { invalidated = true } + + _, err := store.Stat(context.Background(), authTestDigest) + return err, invalidated +} + +// The production failure: the hold rejects our service token as expired, which +// used to surface as BLOB_UNKNOWN (or a bare 403) and fail the pull outright. +// It has to become a 401 so BearerChallenge attaches WWW-Authenticate and the +// client re-runs the token dance. +func TestStat_HoldRejectsExpiredServiceToken_ReturnsUnauthorized(t *testing.T) { + err, invalidated := statWithHoldFailure(t, http.StatusForbidden, holdExpiredTokenBody) + + var ecErr errcode.Error + if !errors.As(err, &ecErr) { + t.Fatalf("Stat() error = %v (%T), want an errcode.Error", err, err) + } + if ecErr.Code != errcode.ErrorCodeUnauthorized { + t.Errorf("Stat() code = %v, want %v", ecErr.Code, errcode.ErrorCodeUnauthorized) + } + if !invalidated { + t.Error("the stale service token was not invalidated: the client's retry would be handed the same dead token") + } +} + +// A crew member without read access authenticated fine. Sending them back to +// re-authenticate would loop forever, so this stays a 403. +func TestStat_HoldDeniesPermission_StaysDenied(t *testing.T) { + err, invalidated := statWithHoldFailure(t, http.StatusForbidden, holdPermissionBody) + + var ecErr errcode.Error + if !errors.As(err, &ecErr) { + t.Fatalf("Stat() error = %v (%T), want an errcode.Error", err, err) + } + if ecErr.Code != errcode.ErrorCodeDenied { + t.Errorf("Stat() code = %v, want %v", ecErr.Code, errcode.ErrorCodeDenied) + } + if invalidated { + t.Error("a permission denial must not throw away a perfectly good service token") + } +} + +// A bare 401 from the hold is a challenge by definition, whatever its body. +func TestStat_HoldAnswers401_ReturnsUnauthorized(t *testing.T) { + err, invalidated := statWithHoldFailure(t, http.StatusUnauthorized, "unauthorized") + + var ecErr errcode.Error + if !errors.As(err, &ecErr) { + t.Fatalf("Stat() error = %v (%T), want an errcode.Error", err, err) + } + if ecErr.Code != errcode.ErrorCodeUnauthorized { + t.Errorf("Stat() code = %v, want %v", ecErr.Code, errcode.ErrorCodeUnauthorized) + } + if !invalidated { + t.Error("a 401 from the hold should drop the credential it rejected") + } +} + +// An anonymous pull that the hold refuses keeps its existing 401 "authenticate +// first" path, and must not touch a service token it never had. +func TestStat_AnonymousDenial_StillAsksForCredentials(t *testing.T) { + holdServer := newMockHoldServer(t, "http://s3.invalid") + defer holdServer.Close() + + holdServer.mu.Lock() + holdServer.PresignAuthFailure = &mockAuthFailure{ + Status: http.StatusForbidden, + Body: holdPermissionBody, + } + holdServer.mu.Unlock() + + store := createTestProxyBlobStore(t, holdServer.URL) + store.ctx.Anonymous = true + store.ctx.ServiceToken = "" + + _, err := store.Stat(context.Background(), authTestDigest) + + var ecErr errcode.Error + if !errors.As(err, &ecErr) { + t.Fatalf("Stat() error = %v (%T), want an errcode.Error", err, err) + } + if ecErr.Code != errcode.ErrorCodeUnauthorized { + t.Errorf("Stat() code = %v, want %v", ecErr.Code, errcode.ErrorCodeUnauthorized) + } + if ecErr.Message != "authentication required" { + t.Errorf("Stat() message = %q, want the anonymous challenge to be unchanged", ecErr.Message) + } +} + +// A hold that is broken, rather than one that refused us, must keep answering +// the way it did before: blob-unknown, not a re-auth challenge. A client that +// re-authenticated over a 5xx would just burn tokens against a sick hold. +func TestStat_HoldServerError_StillBlobUnknown(t *testing.T) { + holdServer := newMockHoldServer(t, "http://s3.invalid") + defer holdServer.Close() + + holdServer.mu.Lock() + holdServer.PresignError = errors.New("internal error") + holdServer.mu.Unlock() + + store := createTestProxyBlobStore(t, holdServer.URL) + store.ctx.InvalidateServiceToken = func() { + t.Error("a 5xx must not invalidate the service token") + } + + _, err := store.Stat(context.Background(), authTestDigest) + if !errors.Is(err, distribution.ErrBlobUnknown) { + t.Errorf("Stat() error = %v, want %v", err, distribution.ErrBlobUnknown) + } +} + +// Same for a hold that is simply unreachable. +func TestStat_HoldUnreachable_StillBlobUnknown(t *testing.T) { + holdServer := newMockHoldServer(t, "http://s3.invalid") + holdURL := holdServer.URL + holdServer.Close() + + store := createTestProxyBlobStore(t, holdURL) + store.ctx.InvalidateServiceToken = func() { + t.Error("a connection failure must not invalidate the service token") + } + + _, err := store.Stat(context.Background(), authTestDigest) + if !errors.Is(err, distribution.ErrBlobUnknown) { + t.Errorf("Stat() error = %v, want %v", err, distribution.ErrBlobUnknown) + } +} + +// ServeBlob runs the same presign path, so the challenge has to reach the +// client from there too: on a GET, distribution calls Stat and then ServeBlob. +func TestServeBlob_HoldRejectsExpiredServiceToken_ReturnsUnauthorized(t *testing.T) { + holdServer := newMockHoldServer(t, "http://s3.invalid") + defer holdServer.Close() + + holdServer.mu.Lock() + holdServer.PresignAuthFailure = &mockAuthFailure{ + Status: http.StatusForbidden, + Body: holdExpiredTokenBody, + } + holdServer.mu.Unlock() + + invalidated := false + store := createTestProxyBlobStore(t, holdServer.URL) + store.ctx.InvalidateServiceToken = func() { invalidated = true } + + req := httptest.NewRequest(http.MethodGet, "/v2/alice/img/blobs/"+authTestDigest.String(), nil) + err := store.ServeBlob(context.Background(), httptest.NewRecorder(), req, authTestDigest) + + var ecErr errcode.Error + if !errors.As(err, &ecErr) { + t.Fatalf("ServeBlob() error = %v (%T), want an errcode.Error", err, err) + } + if ecErr.Code != errcode.ErrorCodeUnauthorized { + t.Errorf("ServeBlob() code = %v, want %v", ecErr.Code, errcode.ErrorCodeUnauthorized) + } + if !invalidated { + t.Error("the stale service token was not invalidated") + } +} + +func TestIsHoldAuthenticationFailure(t *testing.T) { + tests := []struct { + name string + status int + body string + want bool + }{ + {"expired service token", http.StatusForbidden, holdExpiredTokenBody, true}, + {"dpop failure", http.StatusForbidden, "authorization failed: DPoP authentication failed: bad proof", true}, + {"no auth scheme", http.StatusForbidden, "authorization failed: invalid authorization scheme: expected 'Bearer' or 'DPoP'", true}, + {"any 401", http.StatusUnauthorized, "", true}, + {"crew member lacks permission", http.StatusForbidden, holdPermissionBody, false}, + {"not a crew member", http.StatusForbidden, "authorization failed: access denied for blob:read: user is not a crew member (required: blob:read or blob:write)", false}, + {"unrecognised 403", http.StatusForbidden, "authorization failed: failed to get captain record: boom", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isHoldAuthenticationFailure(tt.status, []byte(tt.body)); got != tt.want { + t.Errorf("isHoldAuthenticationFailure(%d, %q) = %v, want %v", tt.status, tt.body, got, tt.want) + } + }) + } +} diff --git a/pkg/appview/storage/proxy_blob_store.go b/pkg/appview/storage/proxy_blob_store.go index db4561c..3691759 100644 --- a/pkg/appview/storage/proxy_blob_store.go +++ b/pkg/appview/storage/proxy_blob_store.go @@ -10,6 +10,7 @@ import ( "log/slog" "net/http" "strconv" + "strings" "sync" "time" @@ -567,6 +568,58 @@ func (p *ProxyBlobStore) presign(ctx context.Context, method string, dgst digest return blob, nil } +// maxHoldErrorBody bounds how much of a hold error response is read for +// classification. The hold's is a single http.Error line. +const maxHoldErrorBody = 4 << 10 + +// holdAuthFailureMarkers are the substrings that mark a hold 401/403 as "we +// could not authenticate you" rather than "you may not read this". +// +// The hold answers every blob authorization failure the same way +// (pkg/hold/pds/xrpc.go): http.Error with 403 and a body of +// "authorization failed: " + err.Error(). Two shapes come out of that, and they +// need opposite handling: +// +// authorization failed: service token authentication failed: token has expired +// authorization failed: access denied for blob:read: crew member lacks permission (required: blob:read or blob:write) +// +// The first says our own credential is stale, which a retry with a fresh one +// fixes, so it has to reach the client as a 401 challenge. The second is the +// hold's verdict on the user and must stay a 403, or the client would be sent +// to re-authenticate in a loop it can never win. +// +// Anything unrecognised is treated as a permission denial. That is the +// conservative direction: a misread denial costs one failed request, whereas a +// misread challenge costs a re-auth loop. +var holdAuthFailureMarkers = []string{ + // "service token authentication failed", "DPoP authentication failed" + "authentication failed", + // pds.ErrTokenExpired, in case the hold ever reports it unwrapped + "token has expired", + "missing authorization header", + "invalid authorization header format", + "invalid authorization scheme", + "missing token", +} + +// isHoldAuthenticationFailure reports whether a hold rejection is about the +// credential we presented rather than about what the caller is allowed to do. +func isHoldAuthenticationFailure(status int, body []byte) bool { + // A 401 is by definition a challenge: the hold is asking for credentials, + // not refusing a request it understood. + if status == http.StatusUnauthorized { + return true + } + + lower := strings.ToLower(string(body)) + for _, marker := range holdAuthFailureMarkers { + if strings.Contains(lower, marker) { + return true + } + } + return false +} + // getPresignedURL asks the hold for a presigned URL for a blob operation, and // for reads gets the blob's size back with it. func (p *ProxyBlobStore) getPresignedURL(ctx context.Context, operation string, dgst digest.Digest) (presignedBlob, error) { @@ -591,10 +644,38 @@ func (p *ProxyBlobStore) getPresignedURL(ctx context.Context, operation string, } defer resp.Body.Close() - if resp.StatusCode == http.StatusForbidden && p.ctx.Anonymous { - // Stale local captain cache let an anonymous request through, but the - // hold says private. Surface a 401 so the client re-authenticates. - return presignedBlob{}, errcode.ErrorCodeUnauthorized.WithMessage("authentication required") + if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { + if p.ctx.Anonymous { + // Stale local captain cache let an anonymous request through, but the + // hold says private. Surface a 401 so the client re-authenticates. + return presignedBlob{}, errcode.ErrorCodeUnauthorized.WithMessage("authentication required") + } + + // The body is a short http.Error line; cap the read anyway so a + // misbehaving hold cannot make us buffer a response of its choosing. + bodyBytes, _ := io.ReadAll(io.LimitReader(resp.Body, maxHoldErrorBody)) + + if isHoldAuthenticationFailure(resp.StatusCode, bodyBytes) { + // Our credential is stale, not the user's permissions. Drop the + // cached token so the retry mints a new one, and answer with a 401 + // so BearerChallenge attaches WWW-Authenticate and the client runs + // the token dance again instead of failing the pull. + slog.Warn("Hold rejected the service token, challenging client to re-authenticate", + "component", "proxy_blob_store", + "hold_did", p.ctx.HoldDID, + "puller_did", p.ctx.PullerDID, + "status", resp.StatusCode, + "body", strings.TrimSpace(string(bodyBytes))) + if p.ctx.InvalidateServiceToken != nil { + p.ctx.InvalidateServiceToken() + } + return presignedBlob{}, errcode.ErrorCodeUnauthorized.WithMessage("service token expired, re-authenticate") + } + + // A real permission decision: the puller authenticated fine and may + // not read this hold. Sending them back to re-authenticate would just + // loop, so keep it a 403. + return presignedBlob{}, errcode.ErrorCodeDenied.WithMessage("read access denied") } if resp.StatusCode == http.StatusNotFound { diff --git a/pkg/appview/storage/proxy_blob_store_test.go b/pkg/appview/storage/proxy_blob_store_test.go index 5e5d1dc..5fd582f 100644 --- a/pkg/appview/storage/proxy_blob_store_test.go +++ b/pkg/appview/storage/proxy_blob_store_test.go @@ -262,8 +262,10 @@ func TestDoAuthenticatedRequest_AnonymousWhenNoToken(t *testing.T) { } // TestGetPresignedURL_HoldForbiddenMapsByAnonymity verifies a hold 403 becomes a -// client 401 for anonymous requests (stale captain cache) but stays a generic -// error for authenticated ones. +// client 401 for anonymous requests (stale captain cache) and, for an +// authenticated request whose rejection names no authentication failure, a 403 +// DENIED: the caller got through the door and may not read. The authentication +// cases live in proxy_blob_auth_test.go. func TestGetPresignedURL_HoldForbiddenMapsByAnonymity(t *testing.T) { holdServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusForbidden) @@ -289,7 +291,7 @@ func TestGetPresignedURL_HoldForbiddenMapsByAnonymity(t *testing.T) { } }) - t.Run("authenticated -> generic error", func(t *testing.T) { + t.Run("authenticated -> 403 denied", func(t *testing.T) { store := NewProxyBlobStore(&RegistryContext{ DID: "did:plc:owner", HoldDID: "did:web:hold.example.com", @@ -298,11 +300,12 @@ func TestGetPresignedURL_HoldForbiddenMapsByAnonymity(t *testing.T) { Anonymous: false, }) _, err := store.getPresignedURL(context.Background(), "GET", dgst) - if err == nil { - t.Fatal("expected an error for authenticated 403") + ec, ok := err.(errcode.Error) + if !ok { + t.Fatalf("expected errcode.Error, got %T: %v", err, err) } - if _, ok := err.(errcode.Error); ok { - t.Errorf("expected a generic (non-errcode) error for authenticated 403, got errcode: %v", err) + if ec.Code != errcode.ErrorCodeDenied { + t.Errorf("expected DENIED, got %v", ec.Code) } }) } @@ -676,6 +679,13 @@ type mockHoldServer struct { AbortError error PresignError error + // PresignAuthFailure, when set, makes the presign endpoint answer the way + // the real hold answers a blob authorization failure: an http.Error line + // with the hold's status and body shape (pkg/hold/pds/xrpc.go writes + // "authorization failed: " + err.Error() with status 403 for every one). + // It is checked before PresignError so a test can inject either. + PresignAuthFailure *mockAuthFailure + // AbortHook, when set, runs as an abort request is handled. It exists so a // test can order the abort against other events (an in-flight part // finishing, say) rather than only counting aborts after the fact. @@ -685,6 +695,13 @@ type mockHoldServer struct { UploadID string } +// mockAuthFailure is one hold authorization rejection: the status it answers +// with and the body it writes. +type mockAuthFailure struct { + Status int + Body string +} + type mockInitiateCall struct { Digest string } @@ -793,6 +810,15 @@ func newMockHoldServer(t *testing.T, s3URL string) *mockHoldServer { switch { case strings.Contains(r.URL.Path, atproto.SyncGetBlob): + if m.PresignAuthFailure != nil { + // http.Error, exactly as the hold sends it: text/plain with a + // trailing newline, not the JSON the success path uses. + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.WriteHeader(m.PresignAuthFailure.Status) + fmt.Fprintln(w, m.PresignAuthFailure.Body) + return + } + if m.PresignError != nil { w.WriteHeader(http.StatusInternalServerError) fmt.Fprintf(w, `{"error":"%s"}`, m.PresignError.Error()) diff --git a/pkg/auth/cache.go b/pkg/auth/cache.go index 3decaaa..9ffae9d 100644 --- a/pkg/auth/cache.go +++ b/pkg/auth/cache.go @@ -1,7 +1,9 @@ // Package auth provides service token caching and management for AppView. // Service tokens are JWTs issued by a user's PDS to authorize AppView to // act on their behalf when communicating with hold services. Tokens are -// cached with automatic expiry parsing and 10-second safety margins. +// cached with automatic expiry parsing and a safety margin +// (ServiceTokenSafetyMargin) so the cache stops serving a token before the +// hold would reject it. package auth import ( @@ -14,6 +16,28 @@ import ( "time" ) +// ServiceTokenSafetyMargin is how far ahead of a service token's real exp the +// AppView stops treating it as usable. Every cache that holds a service token +// (this one, and the registry middleware's per-process validation cache) +// subtracts it, and the registry JWT's exp is stamped from the value this +// package returns, so the JWT never outlives the credential behind it. +// +// It is 60s because that is exactly distribution's token.Leeway: the registry +// auth package accepts a registry JWT for 60s past its exp. With a 60s margin +// the JWT is stamped at (service token exp - 60s), so the last moment +// distribution will accept it is the service token's real exp. Shrinking this +// below distribution's leeway reopens the window this constant closes: a client +// would hold an accepted JWT while the service token behind it is already dead, +// the hold would answer 403 "token has expired", and the pull would fail +// instead of re-authenticating. +const ServiceTokenSafetyMargin = 60 * time.Second + +// unparsableTokenTTL is how long a token whose exp claim could not be read is +// cached. PDS-granted service tokens are requested with a 5 minute expiry (see +// servicetoken.go), so a fixed 50s is comfortably inside any plausible real +// lifetime and the next request re-mints. +const unparsableTokenTTL = 50 * time.Second + // serviceTokenEntry represents a cached service token. type serviceTokenEntry struct { token string @@ -63,17 +87,34 @@ func (c *Cache) Get(did, holdDID string) (string, time.Time) { } // Set stores token for (did, holdDID), parsing its JWT exp claim and -// applying a 10s safety margin so the cache expires before the real -// token does. Falls back to a 50s TTL if the JWT can't be parsed. +// applying ServiceTokenSafetyMargin so the cache expires before the real +// token does. Falls back to unparsableTokenTTL if the JWT can't be parsed. +// +// A PDS is free to grant less than the margin (ATCR asks for 5 minutes; +// reference PDSes grant up to an hour, others may grant less). Subtracting a +// 60s margin from a 30s token would store an entry that is already expired, +// which Get would evict on sight, so every single request would re-mint: a +// refetch storm against the user's PDS. In that case the entry is kept for half +// of whatever life the token actually has instead, which is always positive +// while the token is alive and still leaves headroom proportional to it. A +// token that arrives already expired gets a past expiry, which is correct: it +// is unusable and the next call must mint a new one. func (c *Cache) Set(did, holdDID, token string) error { cacheKey := did + ":" + holdDID expiry, err := parseJWTExpiry(token) if err != nil { - slog.Warn("Failed to parse JWT expiry, using default 50s", "error", err, "cacheKey", cacheKey) - expiry = time.Now().Add(50 * time.Second) + slog.Warn("Failed to parse JWT expiry, using fallback TTL", + "error", err, "cacheKey", cacheKey, "ttl", unparsableTokenTTL) + expiry = time.Now().Add(unparsableTokenTTL) + } else if remaining := time.Until(expiry); remaining <= ServiceTokenSafetyMargin { + slog.Warn("PDS granted a service token shorter than the safety margin", + "cacheKey", cacheKey, + "grantedLife", remaining.Round(time.Second), + "margin", ServiceTokenSafetyMargin) + expiry = time.Now().Add(remaining / 2) } else { - expiry = expiry.Add(-10 * time.Second) + expiry = expiry.Add(-ServiceTokenSafetyMargin) } c.mu.Lock() @@ -163,7 +204,7 @@ func GetServiceToken(did, holdDID string) (token string, expiresAt time.Time) { } // SetServiceToken stores token under (did, holdDID) in the default cache, -// applying the standard 10s safety margin against the JWT's exp claim. +// applying ServiceTokenSafetyMargin against the JWT's exp claim. func SetServiceToken(did, holdDID, token string) error { return defaultCache.Set(did, holdDID, token) } @@ -191,6 +232,18 @@ func DefaultCache() *Cache { return defaultCache } +// ServiceTokenExpiry reports the exp claim of a PDS-issued service token, +// without verifying its signature (we trust tokens minted by the user's PDS, +// and the hold verifies them anyway). +// +// Exported for callers that hold a service token outside this cache and must +// not keep it past its real life. The registry middleware's validation cache is +// the one that matters: it used to pin every token for a flat 45s, so a token +// with 12s left was handed to the hold for another 33s after it died. +func ServiceTokenExpiry(token string) (time.Time, error) { + return parseJWTExpiry(token) +} + // parseJWTExpiry extracts the exp claim from a JWT without verifying its // signature. We trust tokens from the user's PDS, so signature // verification isn't needed here. diff --git a/pkg/auth/cache_test.go b/pkg/auth/cache_test.go index 221987b..d17c098 100644 --- a/pkg/auth/cache_test.go +++ b/pkg/auth/cache_test.go @@ -1,6 +1,8 @@ package auth import ( + "encoding/base64" + "fmt" "testing" "time" ) @@ -232,3 +234,97 @@ func TestCache_PackageFunctionsDelegateToDefault(t *testing.T) { t.Errorf("after InvalidateServiceToken, DefaultCache().Get() = %q, want empty", tok) } } + +// testServiceToken builds an unsigned JWT whose exp claim is expiresAt. Only +// the payload is meaningful: the cache reads exp without verifying anything. +func testServiceToken(expiresAt time.Time) string { + payload := fmt.Sprintf(`{"exp":%d}`, expiresAt.Unix()) + return "header." + base64.RawURLEncoding.EncodeToString([]byte(payload)) + ".signature" +} + +func TestSetServiceToken_AppliesSafetyMargin(t *testing.T) { + defaultCache.Clear() + + did := "did:plc:margin" + holdDID := "did:web:hold.example.com" + + realExp := time.Now().Add(5 * time.Minute) + if err := SetServiceToken(did, holdDID, testServiceToken(realExp)); err != nil { + t.Fatalf("SetServiceToken() error = %v", err) + } + + _, expiresAt := GetServiceToken(did, holdDID) + if expiresAt.IsZero() { + t.Fatal("expected the token to be cached") + } + + want := realExp.Add(-ServiceTokenSafetyMargin) + if diff := expiresAt.Sub(want); diff < -2*time.Second || diff > 2*time.Second { + t.Errorf("cached expiry off by %v (want exp minus %v)", diff, ServiceTokenSafetyMargin) + } + + // The point of the margin: the cache must stop serving the token at least + // distribution's 60s JWT leeway before the hold would reject it. + if got := realExp.Sub(expiresAt); got < 60*time.Second { + t.Errorf("cache serves the token until %v before its real exp, want >= 60s", got) + } +} + +func TestSetServiceToken_ShortGrantKeepsPositiveTTL(t *testing.T) { + defaultCache.Clear() + + did := "did:plc:shortgrant" + holdDID := "did:web:hold.example.com" + + // A PDS that grants far less than the safety margin. Subtracting the margin + // outright would cache an already-expired entry and make every request + // re-mint, so the cache keeps half the remaining life instead. + realExp := time.Now().Add(20 * time.Second) + if err := SetServiceToken(did, holdDID, testServiceToken(realExp)); err != nil { + t.Fatalf("SetServiceToken() error = %v", err) + } + + token, expiresAt := GetServiceToken(did, holdDID) + if token == "" { + t.Fatal("short-lived token should still be cached, not dropped on sight") + } + if !expiresAt.After(time.Now()) { + t.Fatalf("cached expiry %v is not in the future", expiresAt) + } + if !expiresAt.Before(realExp) { + t.Errorf("cached expiry %v should be before the token's real exp %v", expiresAt, realExp) + } +} + +func TestSetServiceToken_UnparsableExpUsesFallbackTTL(t *testing.T) { + defaultCache.Clear() + + did := "did:plc:unparsable" + holdDID := "did:web:hold.example.com" + + if err := SetServiceToken(did, holdDID, "not-a-jwt"); err != nil { + t.Fatalf("SetServiceToken() error = %v", err) + } + + _, expiresAt := GetServiceToken(did, holdDID) + want := time.Now().Add(unparsableTokenTTL) + if diff := expiresAt.Sub(want); diff < -5*time.Second || diff > 5*time.Second { + t.Errorf("expiry off by %v (want ~%v from now)", diff, unparsableTokenTTL) + } +} + +func TestServiceTokenExpiry(t *testing.T) { + want := time.Now().Add(3 * time.Minute).Truncate(time.Second) + + got, err := ServiceTokenExpiry(testServiceToken(want)) + if err != nil { + t.Fatalf("ServiceTokenExpiry() error = %v", err) + } + if !got.Equal(want) { + t.Errorf("ServiceTokenExpiry() = %v, want %v", got, want) + } + + if _, err := ServiceTokenExpiry("not-a-jwt"); err == nil { + t.Error("expected an error for a token that is not a JWT") + } +} diff --git a/pkg/auth/token/handler.go b/pkg/auth/token/handler.go index 0a164bd..03b4fa5 100644 --- a/pkg/auth/token/handler.go +++ b/pkg/auth/token/handler.go @@ -710,11 +710,17 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } if !res.expiresAt.IsZero() { - // Cap JWT lifetime at the service-auth's expiry. The cache's - // expiresAt already includes a 10s safety margin - // (pkg/auth/cache.go:71), so this guarantees the service-auth - // is still cache-valid for any /v2/* request the JWT can - // authorize. We never extend beyond the configured default. + // Cap JWT lifetime at the service-auth's expiry, stamping exactly + // the value the fetcher returned. That value already has + // auth.ServiceTokenSafetyMargin subtracted from the PDS-granted + // exp, and the margin is deliberately >= distribution's + // token.Leeway (60s): the registry auth package accepts this JWT + // for Leeway past its exp, so the last instant a client can use it + // is still inside the service token's real life. Do not shrink the + // margin below that leeway or the gap reopens, and Docker never + // gets the 401 that would make it re-authenticate. + // + // We never extend beyond the configured default. until := time.Until(res.expiresAt) if until < issueExp { issueExp = until diff --git a/pkg/auth/token/handler_margin_test.go b/pkg/auth/token/handler_margin_test.go new file mode 100644 index 0000000..65da223 --- /dev/null +++ b/pkg/auth/token/handler_margin_test.go @@ -0,0 +1,81 @@ +package token + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + disttoken "github.com/distribution/distribution/v3/registry/auth/token" + + "atcr.io/pkg/auth" +) + +// The margin exists to cover distribution's JWT leeway. distribution's registry +// auth accepts a token for Leeway past its exp, and the JWT's exp is stamped +// from (service token exp - margin). If the margin ever drops below the leeway, +// a client can hold an accepted JWT after the service token behind it is dead, +// the hold answers 403 "token has expired", and the pull fails with no 401 to +// make the client re-authenticate. That is the production bug this closes. +func TestServiceTokenSafetyMarginCoversDistributionLeeway(t *testing.T) { + if auth.ServiceTokenSafetyMargin < disttoken.Leeway { + t.Fatalf("auth.ServiceTokenSafetyMargin is %v, which is under distribution's token.Leeway of %v: "+ + "a registry JWT would stay acceptable after its service token expired", + auth.ServiceTokenSafetyMargin, disttoken.Leeway) + } +} + +// The handler must stamp exactly the expiry the fetcher hands it. That value +// already has the margin subtracted (pkg/auth/cache.go), so stamping anything +// later would reopen the window, and the margin has to survive the round trip +// through the token response. +func TestHandler_ServiceAuthFetcher_MarginSurvivesStamping(t *testing.T) { + keyPath := getSharedTestKey(t) + issuer, err := NewIssuer(keyPath, "atcr.io", "registry", 15*time.Minute) + if err != nil { + t.Fatalf("NewIssuer() error = %v", err) + } + + deviceStore, database := setupTestDeviceStore(t) + deviceSecret := createTestDevice(t, deviceStore, database, "did:plc:alice123", "alice.bsky.social") + + handler := NewHandler(issuer, deviceStore) + + // What a 5 minute PDS grant looks like coming out of the cache. + serviceTokenExp := time.Now().Add(5 * time.Minute) + handler.SetServiceAuthFetcher(&stubServiceAuthFetcher{ + expiresAt: serviceTokenExp.Add(-auth.ServiceTokenSafetyMargin), + }) + + req := httptest.NewRequest(http.MethodGet, "/auth/token?service=registry&scope=repository:alice.bsky.social/myapp:pull,push", nil) + req.SetBasicAuth("alice", deviceSecret) + w := httptest.NewRecorder() + + handler.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d. Body: %s", w.Code, w.Body.String()) + } + + var resp TokenResponse + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("decode response: %v", err) + } + + jwtExpiresAt := time.Now().Add(time.Duration(resp.ExpiresIn) * time.Second) + + // The last instant distribution will accept this JWT must still be inside + // the service token's real life. + lastAccepted := jwtExpiresAt.Add(disttoken.Leeway) + if lastAccepted.After(serviceTokenExp.Add(2 * time.Second)) { + t.Errorf("JWT is accepted until %v but the service token dies at %v", + lastAccepted, serviceTokenExp) + } + + // And it must still be a usable token, not one squeezed to nothing. + if resp.ExpiresIn < 200 { + t.Errorf("expires_in = %d, want ~240s (5 min grant minus the %v margin)", + resp.ExpiresIn, auth.ServiceTokenSafetyMargin) + } +}