feat(auth): add cache-through token caching for azure_ad and gcp_adc - #861
feat(auth): add cache-through token caching for azure_ad and gcp_adc#861szedan-rh wants to merge 8 commits into
Conversation
Introduce apis::token_cache::TokenCache, a reusable cache-through credential cache: a request that finds the cache stale acquires a fresh value inline, double-checked under an exclusive lock so concurrent callers racing an empty cache trigger at most one fetch. There is no background refresh loop and no server-side retry/backoff; a failed fetch is simply not cached, so the next request tries again. Retrofit azure_ad onto it, removing its per-instance background refresher thread and private tokio runtime entirely. Implement gcp_adc's metadata-server token fetch (GCE/GKE Workload Identity) on the same cache, unblocking what was previously a permanent 503 stub; service-account key file fetch remains a documented follow-up, now failing closed with a clear "not implemented" reason instead of silently 503ing forever. refresh_ratio is removed from both filters' config (meaningless under cache-through, since there is no scheduled ahead-of-time refresh). gcp_adc gains a metadata_host config field, mirroring azure_ad's authority_host, so a non-default or test metadata endpoint can be configured directly. Signed-off-by: szedan <szedan@redhat.com>
Fixes the lint CI failure on the cache-through credential caching change -- cargo clippy passed locally but nightly rustfmt was not run. Signed-off-by: szedan <szedan@redhat.com>
Fixes the second lint CI failure -- xtask lint-filter-docs and sync-example-readme were not run locally after the cache-through change updated both filters' module docs and config surface. Signed-off-by: szedan <szedan@redhat.com>
…redential-cache-through
|
Generally fine with the approach. There is the pathological timeouts on network partitions: probably should file an issue about that in order to follow up. |
alexsnaps
left a comment
There was a problem hiding this comment.
There are some failures in CI too
|
(ai): The PR checklist claims user-facing and generated documentation were updated, but docs/filters/azure_ad.md still shows refresh_ratio in the example and docs/filters/gcp_adc.md still states token acquisition is not implemented and documents refresh_ratio. cargo xtask lint-filter-docs reports both files stale; make lint includes this check and will fail until cargo xtask generate-filter-docs is run." |
- effective_margin: drop unneeded pub(crate) visibility, it's only used within this file - replace the two explicit drop(guard) calls with a single #[expect(clippy::significant_drop_tightening)] on get_or_refresh -- the guard's last use is immediately before each return either way, so the manual drops were restating that rather than shortening the actual hold time - log a warning when a fetched token's TTL is at or below the cache's margin, so an unusually short TTL from the token endpoint is visible instead of silently reducing the validity window - fix a second stale "background refresher" comment in the azure_ad integration test that the doc-regeneration commit missed The write-lock-held-during-fetch latency pile-up under a sustained IdP network partition, also raised in review, is filed as a follow-up: ai#864. Signed-off-by: szedan <szedan@redhat.com>
|
The approach look good. I'm wondering if we could iterate on it and possible split it up into two separate times;
And I think we need to address what happens if the the refresh never happens, e.g. the server is down. Not sure we can just "queue" up requests for ever. At some point we need to start returning some form of error to the client to free up the requests. |
I think soft expiry (ttl-margin) / hard expiry (ttl) is a reasonable idea to explore. On the failure scenario fair pushback, I was overstating it as "IdP down," and you're right that's rare for GCP/Azure specifically. The sharper risk isn't IdP availability though, it's request duration: even with a perfectly healthy IdP, a pass-through request that picks up the token right as it crosses into the soft -> hard window could still be mid-flight when hard expiry hits, purely because its own request ran longer than the margin nothing to do with refresh succeeding or failing. That's the actual reason the margin exists today (never hand out a token that could outlive the request holding it), so pass-through would need an explicit answer for that, not just a faster/rarer refresh-failure path. I'd rather not fold this into the current PR it adds real synchronization complexity (a second "who's refreshing" signal separate from the read/write lock) on top of what we deliberately scoped down to. If we want to pursue it, I'd want to spec it as its own follow-up with an explicit answer to "is it OK for a request to receive a token that might expire before its own response completes," rather than assume the hard-expiry backstop dissolves that risk. |
Addresses CodeQL alert praxis-proxy#8 (rust/cleartext-transmission) on praxis-proxy#861. The metadata endpoint is only safe to reach over plain HTTP because it is link-local and never routable off the VM/host -- that part of the finding doesn't apply. But metadata_host is operator-configurable, and nothing stopped it from being pointed anywhere: a misconfiguration would send the same plaintext request, and receive the access token in the response, over a real network path instead of staying host-local. validate_metadata_host now rejects anything other than metadata.google.internal or a loopback address (127.0.0.1/localhost, which tests already use to point at a local mock server). Signed-off-by: szedan <szedan@redhat.com>
|
@alexsnaps / @aslakknutsen can you please take another look? |
Addresses Alex's two review comments on the metadata_host restriction in praxis-proxy#861: - localhost is a hostname resolved via DNS/etc/hosts, not a fixed address like 127.0.0.1 -- it could be remapped to point anywhere, which would defeat the loopback restriction entirely. Only 127.0.0.1 (and the real metadata.google.internal) are accepted now. - metadata_host's doc comment still described the old, broader behavior ("a non-default metadata server"); updated it to match the actual restriction and regenerated docs/filters/gcp_adc.md. Signed-off-by: szedan <szedan@redhat.com>
The refresh tokens doesn't have TTL for seconds or minutes right, we're talking closer to 1h+? While not perfect, we could make the margin for soft expiry something like 5m and also use a margin for hard expiry at e.g. 1m. It creates over time a few more refresh calls, but also gives plenty of time to try to refresh even with a few retries and even pre hard expiry requests have a certain time buffer to complete before it's invalid. |
Fair point on the TTL scale with hour-long tokens, your numbers hold up: pass-through requests would still have ≥1m of guaranteed validity, more conservative than the 30s margin we already ship today. |
Summary
Adds
apis::token_cache::TokenCache, a reusable cache-through credentialcache: a request that finds the cache stale acquires a fresh value inline,
double-checked under an exclusive lock so concurrent callers racing an
empty cache trigger at most one fetch. There is no background refresh loop
and no server-side retry/backoff; a failed fetch is simply not cached, so
the next request tries again.
azure_adonto it, removing its per-instance backgroundrefresher thread and private tokio runtime entirely.
gcp_adc's metadata-server token fetch (GCE/GKE WorkloadIdentity) on the same cache, unblocking what was previously a permanent
503 stub. Service-account key file fetch remains a documented follow-up
(needs JWT signing, not currently a workspace dependency) — it now fails
closed with a clear "not implemented" reason instead of silently
503ing forever.
refresh_ratiois removed from both filters' config: it's meaninglessunder cache-through, since there's no scheduled ahead-of-time refresh.
gcp_adcgains ametadata_hostconfig field, mirroringazure_ad'sauthority_host, so a non-default or test metadata endpoint can beconfigured directly.
This is the smaller, "make it work" cache-through implementation the
team converged on as a faster path than the full refresh-ahead
credential-store design — see discussion on #811.
Related issue
Closes #811
(No equivalent tracking issue exists yet for the
gcp_adchalf of thisPR — that spike, praxis#554, lives in the
praxis-proxy/praxisrepo.)Validation
cargo test -p praxis-ai-apisandcargo test -p praxis-ai-filters --features azure-ad-filter,gcp-adc-filterazure_ad/gcp_adcexample-configsuites in
tests/integrationmake lint(workspace + experimental features)make test-unitChecklist
Signed-off-bytrailer.Breaking changes
refresh_ratiois removed from theazure_adandgcp_adcfilterconfig schemas. Any deployment setting it will now fail config
validation (
deny_unknown_fields) and must remove the field.