feat(auth): add CredentialStore and cache GCP credentials

[wolo-lab]

Problem

The GCP provider (#1173) does a credential-service round trip — plus up to a ~10s pending poll — on every outbound request. There is no caching. adk-python has an InMemoryCredentialService, but it is not on this provider's path at all, so caching GCP credentials is new work rather than a port.

A credential cache is also exactly where cross-user leaks live, so most of the design here is about what the key must cover.

Summary

• Adds auth.CredentialStore and InMemoryCredentialStore: concurrency-safe, per-entry expiry, a small clock-skew margin exported as auth.ExpirySkew, and a paced sweep so a principal that resolves once and never returns does not keep its credential for the life of the process.
• The GCP provider caches through the store (a private in-memory one by default, or ProviderConfig.Store). Client.RetrieveCredential returns a *Retrieval carrying the service's expireTime, so entries expire when the credential does.
• The cache key covers everything that decides which credential comes back: the app, the end user, the resource, the scopes, the continue URI, and the *Client — which fixes both the service asked and the identity the ask is authenticated as. Components are length-prefixed before hashing, so no delimiter inside a scope or URI can collide two schemes. Entries are shared exactly among providers that share a Client, which is what makes one store safe behind several providers.
• Nothing is cached without a usable lifetime. An absent, unparseable, already-past, or too-short expiry is declined, and the far end is clamped to an hour so a wrong or injected expireTime cannot pin an entry. That hour is also the bound on how long a credential revoked before its expiry keeps being served; Client.CacheKey names the entry to Delete sooner.
• Caching never fails auth: a store read error, a write error, or a malformed hit all fall through to a fresh retrieval.

Two things worth flagging for review rather than reading out of the diff:

• Client.RetrieveCredential's signature changes, which apidiff reports as the only incompatible change in either package. auth/gcp is absent from v2.0.0, v2.1.0 and v2.2.0, so this reaches only someone tracking main.
• Concurrent cold misses on one key are not coalesced — N misses make N round trips. This is never worse than the per-request round trip it replaces, and singleflight would need a context detached from any one caller to avoid one cancellation failing everybody who joined. Left as a follow-up rather than folded into this change.

[karolpiotrowicz]

Adding the expiry to the payload and the cache in the same change is the right order of operations, and it directly answers the "no caching" concern from the provider PR. I reviewed only the diff against wolo/auth-gcp-provider; the rest of auth/gcp belongs to the earlier PRs in the stack.

The headline is a clean result. Because a credential cache is exactly where cross-user leaks live, I attacked the cache key rather than reading it, and it holds up:

• CredentialKey is a struct used directly as a map key, so there is no string concatenation and no delimiter/canonicalization surface. I tried 9 adversarial pairs (/, |, :, NUL delimiter shifting, empty components, case, Turkish dotless ı, NFC/NFD) — user a + key b/c vs user a/b + key c and friends — 0 collisions, with a negative control confirming the harness detects a real hit.
• Two users on one provider get their own tokens on both the cold and cached read; the same user id under two different app names does not share an entry.
• Excluding SessionID from the key is the right call, and I want to call it out explicitly since it's the kind of decision that looks like an omission later.

So I found no cross-principal disclosure. The findings below are about the dimensions the key doesn't cover, and about what protects it.

---

Main points

1. The cache key omits the scope dimension. Scopes and ContinueURI go on the wire and determine what the minted token authorizes, but they aren't in the key. Since ProviderConfig.Store is exported specifically so one store can back several providers, two providers for the same resource that differ only in scopes will collide. I measured it: a drive provider and a drive.readonly provider sharing one store, same user — the read-only provider was served the broad drive token and its own request never reached the service (1 service call total). The default per-provider store is unaffected (2 calls, correct tokens). Worth noting adk-python derives its credential key from a digest of the whole scheme, scopes included.

2. Nothing protects the cache key. Replacing provider.go:98 with key := auth.CredentialKey{} — one global entry shared by every user in the process — leaves the entire test suite green (70 packages, 0 failures), and that mutant is a genuine leak (bob receives alice's token). This isn't an unreasonable bar: the control mutant, removing the !r.ExpiresAt.IsZero() guard, is caught by TestProviderSkipsCacheWithoutExpiry. The gap is specific to the key. A two-user (and two-app) provider test asserting each principal gets its own token would close it, and it's the one thing I'd most like to see before this lands.

3. CredentialStore freezes as {Get, Set} with no way to invalidate. There's no Delete/Invalidate, so nothing can purge an entry ahead of its expiry — not a revocation, not a logout, not a downstream 401 — and the accepted lifetime is entirely service-chosen: a response claiming expireTime: 9999-12-31 is honoured verbatim. Because this is a bare exported interface with no unexported method, adding a method after release breaks every third-party implementer. Adding Delete (and clamping the accepted expiry) is cheap now and awkward later. The same argument applies to widening CredentialKey for point 1.

4. Eviction. The only removal is Get deleting the key it was asked for, so a principal that never returns retains its entry forever. Measured: 200k already-expired entries survive 10k Set/Get on a different live key, ~27 MiB retained. To be fair to the design, growth is O(distinct principals) at a few hundred bytes each, not O(requests) — re-caching the same keys millions of times adds nothing. Whether that needs an LRU/cap now depends on your expected tenant count, but a Delete on the interface is worth having regardless.

Smaller items

• auth/store.go:79 — platform.Now(ctx) runs caller-supplied code while s.mu is held, and sync.Mutex isn't reentrant: a TimeProvider that touches the store deadlocks it (confirmed). Hoisting the call above the Lock fixes it.
• auth/gcp/client.go:249-251 — the time.Parse error branch is the only uncovered block in the new code. The behaviour is safe (a malformed, empty, absent, past or proto-zero expireTime all correctly skip the cache — I checked all five), it's just unpinned; one table row would do it.
• auth/gcp/provider.go:54 — ProviderConfig.Store isn't set by any test, so the plumbing (and the documented "a store write failure must not fail auth" property) is unasserted. The same test that fixes point 2 can take a store and cover this at no extra cost.
• auth/store.go:100 — a store returning (nil, true, nil) makes Credential() return (nil, nil). auth.Transport fails closed on that, so it's minor, but treating a nil credential as a miss would be more robust.
• auth/store.go:52-54 — the "mirrors adk-python's InMemoryCredentialService" comment is a little off: the app→user→key bucketing does match, but the Python service has no expiry, its key includes scopes, and for GcpAuthProviderScheme (a CustomAuthScheme) it isn't in the path at all. Maybe "serves the same role as" plus a note that this adds per-entry expiry.
• auth/store.go:48 — a zero expiresAt meaning "never expires" makes the Go zero value the fail-open case. The provider guards it correctly; a second implementer might not.
• auth/store.go:55 — a zero-value InMemoryCredentialStore panics on Set (nil map) while Get is a clean miss. A lazy init, or a doc line requiring the constructor, would remove the asymmetry.
• auth/store_test.go:49 — the skew-window case uses the real wall clock while the very next test demonstrates the platform.WithTimeProvider seam.

Questions rather than findings

• Do the credential services actually mint scope-differentiated tokens for a single authProviders/* resource? If they don't, point 1 is only a documentation note — I couldn't verify this from outside.
• Is sharing one Store across providers a configuration you intend to support? Nothing in-repo does it today, and if it isn't intended, documenting that on the field would be an alternative to widening the key.
• Concurrent cold misses for one identity aren't coalesced (32 goroutines → 32 round trips). This is not a regression — I compared against pre-cache behaviour and the change is never worse, and far better in steady state (32 sequential resolves → 1 call vs 32) — but singleflight is already imported in the file if the cold-start burst matters against a per-user quota.

One cross-reference: the redirect issue noted on the earlier client PR becomes more durable with caching in place. An injected credential is now stored with an attacker-chosen expiry and kept being served after the source is unreachable, where previously each resolve refetched. Nothing to change here — it's an argument for fixing it upstream, and for clamping the expiry this PR accepts.

Build, go vet, gofmt, golangci-lint, staticcheck, go mod tidy -diff and go test -race are all clean, and govulncheck's two findings are identical at the base commit (zero delta). TestProviderCachesCredential correctly fails at the base commit, so the new coverage is real.

[karolpiotrowicz]

The key covers app, user and resource, but not Scopes or ContinueURI — and both are sent on the wire (retrieveRequest), so they determine what the minted token authorizes.

Because ProviderConfig.Store is exported so one store can back several providers, two providers for the same resource that differ only in scopes collide. I measured this with a shared store: a drive provider and a drive.readonly provider, same user — the read-only provider was served the broad drive token, and its own request never reached the service (1 service call total). With the default per-provider store there's no collision (2 calls, correct tokens each).

Direction matters: broad-then-narrow hands out more authority than was asked for; narrow-then-broad produces a confusing 403. For reference, adk-python derives its credential key from a digest of the whole scheme, scopes included.

Suggestion — widen the key while the package is still unreleased:

// in NewProvider, after the Scopes clone:
slices.Sort(scheme.Scopes)

// in Credential:
key := auth.CredentialKey{
	AppName: id.AppName,
	UserID:  id.UserID,
	Key:     p.scheme.Name + "|" + strings.Join(p.scheme.Scopes, ",") + "|" + p.scheme.ContinueURI,
}

If sharing a store across differently-scoped providers isn't a configuration you want to support, documenting that on ProviderConfig.Store would be a reasonable alternative.

[karolpiotrowicz]

Minor robustness point: Get returns (Credential, bool, error), so (nil, true, nil) is representable, and this returns it straight through — Credential() then hands the caller (nil, nil). I confirmed that with a store implementation returning a nil credential on a hit.

auth.Transport fails closed on a nil credential, so nothing in-repo breaks, but CredentialProvider is exported and third-party callers may not check. Treating it as a miss keeps the failure local:

if cred, ok, err := p.store.Get(ctx, key); err == nil && ok && cred != nil {
	return cred, nil
}

Rejecting a nil credential in Set would close the other half.

[karolpiotrowicz]

No test sets this field, so the store plumbing is unasserted — including the best-effort property described at line 120 ("a store write failure must not fail auth"), which is currently stated only in a comment. A fake store that records calls and one that always errors would pin both, and the two-user test suggested on line 98 could take the store as a parameter and cover this at no extra cost.

[karolpiotrowicz]

This test asserts the call count but never that two principals stay separate, and that turns out to be the only thing standing between the cache and a cross-user leak.

Concretely: replacing the key construction at provider.go:98 with key := auth.CredentialKey{} — one global entry shared by every user in the process — leaves the whole test suite green (70 packages, 0 failures), and that mutant genuinely leaks: with it, a second user receives the first user's token. The bar is reasonable, though — the control mutant (removing the !r.ExpiresAt.IsZero() guard) is caught, by TestProviderSkipsCacheWithoutExpiry on line 132. The gap is specific to the key.

A test along these lines would close it:

func TestProviderCacheIsolatesPrincipals(t *testing.T) {
	// server echoes the requesting userId back in the token
	// resolve for user-1 and user-2 (and for the same user under two app names)
	// assert each receives its own token, on the cold read and on the cached read
}

[karolpiotrowicz]

CredentialStore is a bare exported interface with no unexported method, so its method set freezes the moment this ships: any later addition breaks every third-party implementer. Two things are worth settling now rather than after release.

No invalidation. With only {Get, Set} there's no way to purge an entry ahead of its expiry — not on consent revocation, not on logout, not on a downstream 401. A Delete(ctx context.Context, key CredentialKey) error costs little today.

The accepted lifetime is entirely service-chosen. A response claiming expireTime: 9999-12-31T23:59:59Z is honoured verbatim — I checked, and three resolves then produce a single service call. Clamping to a sane maximum in the provider before calling Set would bound the exposure of any bad or injected expiry.

[karolpiotrowicz]

Making a zero expiresAt mean "never expires" puts the fail-open case on the Go zero value, in brand-new public API. The GCP provider handles it correctly (it only calls Set when the service reported an expiry), but that guard lives in the consumer rather than in the store, so a second implementer or a future caller gets a permanent entry by omission.

Either invert it (zero = already expired) or, if the current semantics are deliberate, make it explicit here — something like "a zero expiresAt means the entry never expires; callers that cannot establish a lifetime should not call Set".

[karolpiotrowicz]

The comparison is a bit off on the details. The app→user→key bucketing genuinely does match InMemoryCredentialService, but: that service has no expiry at all (this one adds per-entry expiry, which is an improvement); its credential key is a digest over the whole auth scheme, scopes included; and for GcpAuthProviderScheme — a CustomAuthScheme — CredentialManager returns the provider's credential directly and never consults the credential service.

Something like "serves the same role as adk-python's InMemoryCredentialService (app+user+key bucketing), and adds per-entry expiry" would be accurate.

[karolpiotrowicz]

A zero-value InMemoryCredentialStore is asymmetric: Get is a clean miss, but Set panics with assignment to entry in nil map. Since the type is exported, var s auth.InMemoryCredentialStore is a natural thing to write.

Either initialise lazily in Set under the lock (if s.entries == nil { s.entries = make(...) }) or state on the type that NewInMemoryCredentialStore is required.

[karolpiotrowicz]

This error branch is the only uncovered block in the new code. The behaviour is right — I checked malformed, empty, absent, past and 0001-01-01T00:00:00Z inputs, and all five correctly skip the cache (three resolves → three service calls), so it fails closed. It's just not pinned, on a path that parses input from a network service.

One row in the TestRetrieveCredential table with "expireTime":"not-a-time", asserting the credential still comes back with a zero ExpiresAt, would cover it.

Worth noting in passing that 0001-01-01T00:00:00Z parses to exactly time.Time{}, so "the service sent proto-zero" and "the service sent nothing" are indistinguishable downstream. Both mean "don't cache" today, so it's harmless — just a little surprising if the zero ever grows a second meaning.

[karolpiotrowicz]

This case asserts the clock-skew window against the real wall clock, while TestInMemoryCredentialStoreGetHonorsClock just below demonstrates the deterministic platform.WithTimeProvider seam. The 10s skew gives plenty of headroom so it's unlikely to flake in practice, but using the seam here too would make the intent ("an expiry inside the skew window is treated as expired") explicit rather than timing-dependent.

[karolpiotrowicz]

Reviewed at 2e12ee85, against the merge base 7e641247. The store's expiry logic is careful — the skew is applied in the conservative direction, the service's expiry is distrusted and capped, and the clock is resolved outside the lock with a comment explaining why. Three things need fixing before this lands — two cache-key defects and one revocation gap — and two more are worth settling in the same pass: a pair of key dimensions no test can fail on, and the store's growth behavior.

First, though: this branch does not merge into its own base. There are content conflicts in auth/gcp/provider.go and auth/gcp/provider_test.go, and the branch is 8 commits behind wolo/auth-gcp-provider, whose head moved to 6dbe3401 while this review was running. Those are the two files most of what follows is about, so it is worth rebasing before acting on any of it — some line references will move.

A shared store serves one caller identity's token to another

The cache key is {AppName, UserID, slot} at provider.go#L132, and the slot covers only Name, Scopes and ContinueURI (provider.go#L101-L105). The *Client is not in it, and the *Client is what fixes the minting identity — Config.HTTPClient is documented at client.go#L96-L100 as "used verbatim and ADC is not applied, so it must carry its own credentials". Two providers built with different HTTPClient identities and one shared Store therefore share a cache entry, and sharing a store across providers is this PR's own tested idiom.

The comment at provider.go#L129-L131 says the slot "covers everything that shapes the minted token". The caller identity shapes it and is not covered.

Reproduction — one endpoint, two caller identities, one shared store

// in auth/gcp, package gcp_test. Run: go test -run TestClientIdentityNotInKey ./auth/gcp/
type identityRT struct {
	base http.RoundTripper
	who  string
}

func (t identityRT) RoundTrip(r *http.Request) (*http.Response, error) {
	r = r.Clone(r.Context())
	r.Header.Set("X-Caller-Identity", t.who)
	return t.base.RoundTrip(r)
}

func TestClientIdentityNotInKey(t *testing.T) {
	var seen []string
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		who := r.Header.Get("X-Caller-Identity")
		seen = append(seen, who)
		// the service mints a token scoped to the CALLING service account
		_, _ = io.WriteString(w, `{"success":{"token":"tok-minted-for-`+who+
			`","header":"Authorization: Bearer","expireTime":"2999-01-01T00:00:00Z"}}`)
	}))
	defer srv.Close()

	store := auth.NewInMemoryCredentialStore()
	scheme := gcp.Scheme{Name: "projects/p/locations/l/authProviders/ap", Scopes: []string{"drive"}}
	mk := func(who string) auth.CredentialProvider {
		hc := &http.Client{Transport: identityRT{base: srv.Client().Transport, who: who}}
		c, _ := gcp.NewClient(t.Context(), &gcp.Config{HTTPClient: hc, AgentIdentityEndpoint: srv.URL})
		p, _ := gcp.NewProvider(scheme, &gcp.ProviderConfig{Client: c, Store: store})
		return p
	}

	_, _ = mk("sa-alpha").Credential(adkContext(t, "user-1"))
	cred, _ := mk("sa-beta").Credential(adkContext(t, "user-1"))
	bc := cred.(auth.BearerCredential)
	if bc.Token != "tok-minted-for-sa-beta" {
		t.Errorf("sa-beta provider served %q; service saw callers %v", bc.Token, seen)
	}
}

Observed: the sa-beta provider is served tok-minted-for-sa-alpha, and the service records only [sa-alpha] as a caller.

The slot encoding is not injective

provider.go#L104 joins with | and ,, neither of which is escaped or excluded from the components. resourceNameRE does bar | from Name, but that check runs inside RetrieveCredential after the store read, and it is never applied to Scopes or ContinueURI — which is where both collisions live.

Scopes:["a,b"]  and  Scopes:["a","b"]                          -> both slot to  "R|a,b|"
{Scopes:["x"], ContinueURI:"y|z"}  and  {Scopes:["x|y"], ContinueURI:"z"}  -> both slot to  "R|x|y|z"

Driven through NewProvider(...).Credential(ctx) with a shared store, each pair produces one service call instead of two, and the second provider is handed a token minted for the first one's scopes. Since the encoding is the thing at fault rather than any one component, length-prefixing or hashing a struct-encoded key would close the whole class rather than the two instances above — I have not run that change, so treat it as a direction and not a verified patch.

A revoked credential keeps working for up to an hour

store.go#L53-L56 documents Delete as the hook for "consent revocation, logout, or a downstream 401". Nothing outside tests calls it, and auth.Transport.RoundTrip returns a downstream 401 to the caller without invalidating anything.

Before this change every Credential() was a live round trip, so a revocation took effect on the very next request. After it, warming the cache and then making the service reject returns the cached token with a nil error and never re-consults the service, for min(service expiry, now+1h). That may well be the trade you intend for a credential cache — but right now the interface doc promises an invalidation path that no code path reaches, so either the wiring or the promise should change.

Two key dimensions have no test that can fail

AppName and ContinueURI are both in the key by construction and neither is covered. Deleting AppName from the key at provider.go#L132 leaves go test ./auth/... fully green, and so does dropping ContinueURI from the slot. The cause for the first is provider_test.go#L208, which hardcodes AppName: "app" at the only place a test context is built.

This is worth separating from ordinary coverage talk: schemeSlot is at 100% statement coverage and the ContinueURI deletion still survives. The UserID, Scopes and skew dimensions all do have tests that die when you break them, so the gap is specific rather than general.

A second, separate test problem sits next to it. TestProviderClampsServiceExpiry at provider_test.go#L388-L391 hand-writes the slot string and asserts only that no entry exists, with no positive control that one existed beforehand. Any change to the slot format turns it into a silent no-op that also stops guarding maxCachedLifetime.

Deleting the "don't cache without an expiry" guard also leaves the suite green

Removing if !r.ExpiresAt.IsZero() at provider.go#L155-L160 and calling Set unconditionally breaks no test, because TestProviderSkipsCacheWithoutExpiry passes no Store and so measures the default store's own rejection rather than the provider's guard. Since ProviderConfig.Store is exported, the guard is exactly the thing that protects a third-party store from being asked to cache a credential of unknown lifetime.

The same shape applies to the store's locking: removing every mutex from auth/store.go leaves go test -race ./auth/... green, because no test drives the store from two goroutines at once. The locking is correct as written — the green race run just is not evidence of it.

Growth

store.go#L119-L126 sweeps only on every 256th Set, Get evicts only the key it looked up, and there is no size cap. 255 principals that each resolve once and never return leave 255 expired entries resident indefinitely, which is the case store.go#L59-L60 already names. That is bearer-token material staying in the heap well past its usefulness.

Smaller things

• Get's three-value return is unspecified. store.go#L47-L48 says nothing about (nil, true, nil) or about a credential returned alongside a non-fatal error. The provider requires err == nil && ok && cred != nil, so a store reporting a degraded backend has its credential silently dropped and the cache becomes permanently ineffective. The comment at provider.go#L133-L134 says "the interface allows it", but the interface does not say so, and this is new exported surface so it is cheapest to pin now.
• A past-but-well-formed expireTime is written to the store. Only the upper bound is clamped. InMemoryCredentialStore shrugs it off on the next Get, but a TTL-based implementation computing expiresAt.Sub(now) gets a negative TTL.
• Concurrent cold callers are not coalesced. provider.go#L135-L160 is an unsynchronized check-then-act, so N concurrent misses on one key can each make their own round trip and each poll for up to the 10s timeout. singleflight is already imported and used for the much cheaper client init at provider.go#L187. This is still a strict improvement over the previous per-request round trip, so it is a follow-up rather than a blocker.
• RetrieveCredential's signature change is source-breaking, confirmed by apidiff as the only incompatible change in either package. It has never appeared in a tagged v2 release — auth/gcp/client.go is absent from v2.0.0, v2.1.0 and v2.2.0 — so this only reaches anyone tracking main. Worth a line in the PR body rather than any code change.
• expirySkew is unexported, so a third-party store cannot honor the tolerance the provider assumes, and the provider re-checks nothing after a hit.
• ConsentRequiredError.Key is a string documented as "the credential-store key", while the store's key is now a struct. That field predates this PR and is never populated, but the two now describe the same concept incompatibly.
• %+v on a CredentialKey at store.go#L110 puts the app name and end-user ID into an error string. No token material, so this is log hygiene rather than credential exposure.
• An empty AppName is accepted where an empty UserID is rejected, so two apps that both omit it share cache entries.

One claim I want to flag as not a problem, in case it comes up: parseExpireTime using time.RFC3339 is fine. Go's parser accepts fractional seconds after a seconds field regardless of the layout — .000Z, .123456Z and .123456789Z all parse cleanly on go1.26.6, so protobuf JSON timestamps are handled.