fix: Reuse IdP-issued client tokens until near expiry - #6687
fix: Reuse IdP-issued client tokens until near expiry#6687larrysingleton007 wants to merge 1 commit into
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6687 +/- ##
==========================================
+ Coverage 46.81% 46.85% +0.04%
==========================================
Files 415 415
Lines 50399 50456 +57
Branches 7214 7224 +10
==========================================
+ Hits 23592 23643 +51
- Misses 25157 25159 +2
- Partials 1650 1654 +4
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
franciscojavierarceo
left a comment
franciscojavierarceo
left a comment
There was a problem hiding this comment.
The cache key omits token_refresh_margin_seconds, but the stored deadline is calculated using that value. If two configs share the same IdP/client credentials and use different margins—the new option is explicitly configurable—the stricter config can reuse a token until the looser config's deadline. That violates the requested safety margin. We should include the margin in the key or store the token's true expiry and apply each caller's margin when reading; add a regression that uses both configs without manually clearing the cache.
|
@franciscojavierarceo One behavior change worth flagging: a token already inside the margin now gets cached instead of skipped. Every read still rejects it for that caller so nothing observable changes, and a config with a smaller margin can legitimately use it. The regression exercises both configs against the shared entry without clearing the cache, and also checks the reverse direction stays cheap, with the narrow-margin config reusing what the wide one wrote. I verified it fails on the old code, with the wide config receiving the narrow config's token. I also dropped the manual cache clear from 326 permissions tests pass. |
|
@franciscojavierarceo @ntkathole Green and ready for another look. It also still needs a |
|
@franciscojavierarceo @ntkathole All checks green, no conflicts with current master. It also still needs a |
|
@larrysingleton007 please resolve the conflicts |
|
@ntkathole done, master merged and pushed. The only conflict was 326 permissions tests pass on the merged tree, with ruff and mypy clean. The cache-key fix and its regression are unchanged. The other two, #6689 and #6690, merge cleanly against current master, so nothing needed there. |
|
@ntkathole both addressed in 2a73cb7. Pruning. Inserting on a miss now first drops entries whose stored expiry has passed. Keys are credential identities, so the cache was already bounded by the number of distinct configs, but a long-lived process that rotates credentials would have retained every retired one. Revocation. Added One deliberate deviation from what you asked, and I'd rather flag it than quietly do less. I did not retry the rejected call. All four interceptor methods share Two new tests: one asserting invalidation forces a refetch and is idempotent, one asserting an expired entry is pruned while an unexpired one survives. 328 permissions tests pass, ruff and mypy clean. |
|
@larrysingleton007 not able to rebase and merge, can you rebase and possibly squash the commit? |
On the client_secret branch every outbound RPC built a fresh auth-token factory, manager and OIDCDiscoveryService, paying a discovery GET plus a token POST per call and discarding the token. All three auth interceptors invoke it per RPC, so a batch materialization loop multiplied IdP load by request count and could trip IdP rate limits. Measured before: 5 calls = 5 discovery GETs + 5 token POSTs. After: 1 and 1. Caches IdP tokens in a module-level dict keyed by the token-request identity, since the interceptors build a fresh manager per call and instance state would not survive. Expiry comes from the token's own exp claim, falling back to the token endpoint's expires_in; a token whose expiry is unknowable is not cached, preserving per-call behaviour for opaque tokens. The cache stores true expiry and applies the caller's token_refresh_margin_seconds on read, rather than storing a deadline. The margin is not part of the key, so baking it in let a config with a wider margin reuse a token past its own safety window when another config sharing the same credentials had written the entry. token_refresh_margin_seconds is configurable on OidcClientAuthConfig (default 30, gt=0) rather than hardcoded. Inserting on a miss prunes entries whose stored expiry has passed. Keys are credential identities so the cache is bounded by distinct configs, but a long-lived process rotating credentials would otherwise retain every retired identity. Reuse means a token the IdP revokes mid-life keeps being presented until its own expiry, where fetching per call self-corrected. Adds OidcAuthClientManager.invalidate_token and a transport-agnostic invalidate_auth_token(auth_config), wired into the gRPC interceptor: an UNAUTHENTICATED response drops the cached token so the next call refetches, bounding staleness to the rejected request. The call is not retried, because all four interceptor methods share that path and a stream's request_iterator may already be consumed. 329 permissions tests pass. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
2a73cb7 to
202e30d
Compare
|
@ntkathole rebased onto current master and squashed to a single commit, 202e30d. The branch is no longer behind, and the three merge commits are gone. One note on how I did it, since it affects what you're reviewing. Master had moved three commits past this branch's last merge, so I brought those in first and then collapsed everything onto Re-verified after the rebase, since master's newer commits touched |
|
@ntkathole both review threads are resolved now. I'd replied in the conversation rather than inside the threads, which is why they stayed open — my mistake. Summary of where they landed: pruning on cache miss is implemented and covered by a test; eviction on Two things still block the merge, neither of them code in this PR. The failing check is @franciscojavierarceo the |
|
Following up on the red check with evidence rather than just asking for a re-run.
Both are on master with no PR involved. The failure here, For this PR specifically, that test builds its session from So a re-run should clear it. Separately, |
4 participants
What this PR does / why we need it
This is the client-side sibling of #6683. On the
client_secret(client credentials / ROPC) branch, every outbound RPC builds a fresh auth manager and throws away the token it fetched:get_auth_tokenconstructs a new factory, manager, andOIDCDiscoveryServiceper call (client_auth_token.py).sending_headers, and the HTTP wrapper on every session reuse.So each request pays a discovery GET plus a token POST against the IdP for a token that is typically valid for an hour. Measured with counting mocks on master: 5 outbound calls produce 5 discovery GETs and 5 token POSTs. Beyond the added latency, a batch loop multiplies IdP load by request count, which can trip provider rate limits.
The fix caches issued tokens keyed by the token-request identity (discovery URL, client id and secret, username, password) and reuses each until shortly before expiry, taken from the token's own
expclaim with the token response'sexpires_inas a fallback. After the change the same measurement performs 1 discovery GET and 1 token POST.The cache is module level because the interceptors construct a fresh manager per RPC, so instance state cannot survive between calls. Only the
client_secretbranch is affected; static tokens,token_env_var, and mounted service account tokens were already cheap and are untouched.A few details worth reviewer attention:
token_refresh_margin_seconds(default 30) is exposed onOidcClientAuthConfigrather than hardcoded, following the same request you made on fix: Reuse the OIDC JWKS client across requests #6683. It rejects non-positive values, since a zero or negative margin would allow reuse right up to or past expiry. It sits on the client config rather than the sharedOidcAuthConfigbecause only clients fetch tokens from the IdP.expires_in) is deliberately not cached, preserving today's per-call behavior rather than guessing a lifetime.One interaction worth flagging:
HttpSessionManager.get_sessionre-callsget_auth_tokenon every session cache hit, with a comment from #5895 explaining it does so "in case it expired". After this change that call returns a cached token rather than a freshly fetched one. The intent still holds, because the cache never returns a token with less than the margin remaining, but the mechanism changes and it seemed better to say so than to let a reviewer find it.Testing: 7 new unit tests covering reuse until expiry, refusal to cache inside the refresh margin, the
expires_infallback for opaque tokens, no caching when expiry is unknowable, cache keying across distinct configs, the configurable margin changing reuse behavior, and rejection of non-positive margins. An autouse fixture resets the module cache around every test so no test inherits or leaks a cached token. 315 permissions tests pass; ruff and mypy are clean.Which issue(s) this PR fixes
Fixes #6684