fix(storage): re-read the Azure federated token file on every exchange - #6687
Open
siva-abstract-security wants to merge 3 commits into
Open
Conversation
`WorkloadIdentityCredential` reads the file pointed to by `AZURE_FEDERATED_TOKEN_FILE` once, when the credential is built, and reuses that client assertion for the lifetime of the process. Kubernetes rotates the projected service account token roughly hourly, while the access token obtained from Entra ID lives ~24h, so no token exchange is attempted for a day. By then the assertion the credential holds is long expired: Entra rejects the exchange with `AADSTS700024`, `azure_core` classifies the 401 as non-retryable, and the indexing pipeline dies with no path to recovery short of a restart. In practice only the indexer is affected. `StorageResolver::resolve()` builds a fresh `Storage` per call, so the searcher and the janitor re-read the token file on each operation, while the indexer holds one `Storage` for the life of the pipeline. Upgrading `azure_identity` is not an option here: `azure_storage_blobs` was never published past 0.21.0 and requires `azure_core ^0.21`, while `azure_identity >= 0.22` implements a different `TokenCredential` trait, so a bump means porting the whole Azure backend to the rewritten SDK. Add `RefreshingWorkloadIdentityCredential`, which runs the same federated credentials flow but reads the assertion from disk on every exchange and refreshes shortly before the access token expires rather than after. It is used only when all three workload identity environment variables are set; otherwise `from_uri()` falls through to the stock `azure_identity` credential chain, so access key and non-Azure paths are unchanged. The cached token is held in an `ArcSwapOption` rather than behind a lock, so no lock is held across the token exchange; refreshes are rare and idempotent, so a race between two callers is harmless. Cache entries record the scopes they were minted for, so a token is never reused for scopes it was not issued against. Tests inject a fake token endpoint through the `HttpClient` seam that `federated_credentials_flow::perform` already exposes. It accepts only the assertion the token file currently holds and rejects anything else with `AADSTS700024`, mirroring the real endpoint. One test reproduces the failure against the stock `WorkloadIdentityCredential`, another shows the new credential picks up the rotated assertion and recovers. Closes quickwit-oss#6672
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be3989f567
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ive a failed refresh Addresses review feedback on quickwit-oss#6687. Defer to the stock credential chain in two cases where it would not have built a file-backed workload identity credential either, even though a projected token file is present: - an explicit `AZURE_CREDENTIAL_KIND` selecting another provider. The workload identity environment variables are injected automatically by the webhook, so they are routinely present even when an operator deliberately configured a different credential. Only `environment` and `workloadidentity` resolve to a workload identity credential upstream, so only those two are stood in for, using the same case and space normalization `SpecificAzureCredential` applies. - an inline `AZURE_FEDERATED_TOKEN`, which upstream prefers over the file. Reading the file instead could authenticate as a different subject, and an inline assertion has nothing to re-read. Also stop failing storage operations when a refresh fails inside the expiration margin. The margin exists precisely so a failed refresh can be retried while the token in hand is still usable, but the error was propagated regardless, so a transient hiccup near expiry would fail requests for up to five minutes even though a valid token was cached. Serve the cached token until it genuinely expires, and surface the error only once there is nothing left to fall back on.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51f1fcb6b0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| azure_storage_config.resolve_access_key() | ||
| { | ||
| StorageCredentials::access_key(storage_account_name.clone(), access_key) | ||
| } else if let Some(credential) = RefreshingWorkloadIdentityCredential::from_env() { |
There was a problem hiding this comment.
When no access key is configured and workload-identity env vars are present, but that provider cannot produce a token before this custom cache is populated (for example, an unreadable projected token file or a misconfigured federated credential) while a later default provider such as managed identity or Azure CLI would succeed, this branch installs only the custom credential and never reaches azure_identity::create_credential(). The previous path used the SDK default credential chain, which tries later sources after token errors, so these deployments now fail every Azure storage request instead of authenticating via their configured fallback; keep the refreshing workload credential inside a fallback chain rather than replacing the whole default chain.
Useful? React with 👍 / 👎.
…utation `get_token` runs on every storage request and the Azure backend keeps up to `max_concurrent_uploads` requests in flight, so the refresh path was reachable by a hundred callers at once. Two consequences: every in-flight request would exchange simultaneously when the token entered the refresh margin, and if the token endpoint were failing, every request would keep re-exchanging for the whole margin. The second is the dangerous one -- it turns a brief outage into a retry storm against a service that throttles, which can extend the outage it is reacting to. Widening the margin to five minutes made this materially worse than the 20s window the stock token cache uses. A caller now claims the right to exchange with a compare-and-swap on the last attempt timestamp, and callers that already hold a usable token keep using it rather than piling on. This bounds exchanges to one per cooldown whether the endpoint is healthy or failing, and it also bounds the warning log to one per cooldown instead of one per request. Callers with nothing usable in hand never consult the cooldown, so a cold start behind a brief outage is never held back. A wall clock that steps backwards is treated as the cooldown having elapsed, rather than blocking refreshes until the clock catches up. Separately, clamp the lifetime taken from `expires_in`. `OffsetDateTime + Duration` panics on overflow, so a malformed response could have taken the process down. Clamping rather than saturating also means a nonsensical lifetime produces an earlier refresh instead of a token that is never refreshed at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6672.
Problem
WorkloadIdentityCredential::create()readsAZURE_FEDERATED_TOKEN_FILEexactly once, when the credential is built, and holds that client assertion for the lifetime of the process:Kubernetes rotates the projected service account token roughly hourly, but the access token returned by Entra ID lives ~24h, so the
TokenCachemeans no exchange is even attempted for a day — the staleness is invisible until then. At T+24h the credential retries the exchange with an assertion that expired ~23h earlier, Entra respondsAADSTS700024,azure_coreclassifies the 401 as non-retryable, and the indexing pipeline dies. There is no self-healing; only a pod restart recovers it.In practice only the indexer is affected.
StorageResolver::resolve()builds a freshStorageper call, so the searcher and janitor re-read the token file on each operation, while the indexer holds oneStoragefor the life of the pipeline.Why not bump
azure_identityThis is fixed upstream in
azure_identity0.22, but that version can't be reached from here:azure_storage_blobswas never published past 0.21.0 and requiresazure_core ^0.21, andStorageCredentials::token_credential()wants anArc<dyn azure_core@0.21::auth::TokenCredential>, whereasazure_identity >= 0.22implements a different trait. A bump means porting the whole Azure backend to the rewritten SDK, which seems well out of scope for this bug.Fix
RefreshingWorkloadIdentityCredentialruns the samefederated_credentials_flow, but reads the assertion from disk on every exchange and refreshes 5 minutes before the access token expires rather than after.It is used only when all three workload identity environment variables are set — the same condition under which
EnvironmentCredentialwould have selectedWorkloadIdentityCredential. Otherwisefrom_uri()falls through to the stockazure_identity::create_credential()chain, so access-key and non-Azure paths are untouched. The inlineAZURE_FEDERATED_TOKENvariant also falls through, since there is no file to re-read.Two details I'd particularly welcome opinions on:
ArcSwapOptionrather than behind a lock, so no lock is held across the token exchange. Refreshes are rare (roughly daily) and idempotent, so two callers racing is harmless and seemed preferable to serialising on aRwLock.azure_storagecurrently only ever requestsSTORAGE_TOKEN_SCOPE, so a single unkeyed slot would work today, but fix: sovereign Azure storage token scopes for managed identity #6666 makes the scope vary by cloud and an unkeyed cache would then hand back a public-cloud token for a sovereign scope.Tests
federated_credentials_flow::performalready takes anArc<dyn HttpClient>, so the tests inject a fake token endpoint that accepts only the assertion the token file currently holds and rejects anything else withAADSTS700024, mirroring the real endpoint. It records every assertion presented, so the tests assert on what the credential actually sent rather than only on the outcome.test_upstream_credential_ignores_the_rotated_token_filereproduces the bug against the stockWorkloadIdentityCredential: after rotation it presents["assertion-hour-0", "assertion-hour-0"]and is rejected.test_refreshing_credential_survives_token_rotationruns the identical scenario through the new credential:["assertion-hour-0", "assertion-hour-24"], and the exchange succeeds.Verified by mutation: reintroducing the one-shot read fails exactly the two tests that assert the fix, with the same
Unauthorizedseen in production.Worth noting for anyone debugging this in the wild —
AADSTS700024does not survive into the error object.azure_corekeeps the status and drops the response body, which is a good part of why the failure is hard to diagnose from indexer logs.cargo test -p quickwit-storage --features azure --libpasses (75/75), along with clippy,cargo +nightly fmt --check, license-header and log-format checks.What I have not verified
The new credential has not yet run against real Entra past a 24h boundary — that is in progress on an Azure cluster and I'll report back. Everything above is verified in-process against the fake endpoint.