fix(common/redis): re-AUTH new pool connections with fresh Entra token - #4360
fix(common/redis): re-AUTH new pool connections with fresh Entra token#4360paulstadler-mesh wants to merge 15 commits into
Conversation
Fixes dapr#3554. When useEntraID is true, the redis component was snapshotting the initial Entra access token into redis.UniversalOptions.Password at component-init time. Every new pool connection opened by go-redis afterwards replayed that static AUTH at connection establishment — but the token has a finite TTL (1–24h depending on the credential type and audience). After expiry, any new connection (idle timeout, max-conn-age, network blip, demand spike, pool churn) would fail AUTH with WRONGPASS, breaking direct state-store callers in particular. dapr#3632 fixed the background AUTH ACL refresh goroutine being killed by init context cancellation, but only re-AUTHs *existing* connections; it never updated Options.Password, so new connections continued to fail with the original snapshot once it expired. This change installs an OnConnect hook on every variant of the underlying go-redis client (v8/v9, plain/cluster/failover) that fetches a fresh token from the cached azcore.TokenCredential and runs AUTH ACL on every newly dialed connection. azcore caches tokens internally until ~5 minutes before expiry, so per-connection GetToken calls are inexpensive in steady state. The static Password snapshot is no longer set, so go-redis skips its built-in AUTH and lets OnConnect handle authentication. StartEntraIDTokenRefreshBackgroundRoutine is unchanged — it remains the safety net for long-lived connections that survive across token-TTL boundaries without ever being pool-cycled. Reproduction (against Azure Managed Redis with a workload-identity-bound managed identity that has the default access policy): 1. Configure a state.redis component with useEntraID: true. 2. Deploy a pod that issues frequent state-store calls (or set a tight maxConnAge to force pool cycling). 3. Within ~24h the daprd sidecar's first pool replacement after the initial token's TTL elapses fails AUTH with "WRONGPASS invalid username-password pair", causing the readiness probe to flip and the sidecar to crash-loop. With this change: - All four go-redis client constructors install an OnConnect callback that AUTHs each new connection with a freshly-acquired token. - Pods running with `maxConnAge: 5m` continue to be healthy across many full pool-cycle windows with zero WRONGPASS errors. Per the documented AMR auth contract (Microsoft Learn: "Use Microsoft Entra for cache authentication with Azure Managed Redis"), the AUTH command must use `User = Object ID of the managed identity`. The OID is parsed once from the initial token at component init and stored on Settings; OnConnect uses it for every subsequent AUTH. Refs: dapr#3554, dapr#3632 Signed-off-by: Paul Stadler <paul.stadler@meshconnect.com>
9625256 to
e5e90bf
Compare
|
Hi @yaron2 @shivamkm07 — this PR closes the loop on #3632: that one fixed the AUTH-refresh goroutine, but Options.Password was still snapshotted at init, so new pool connections (idle timeout, max-conn-age, network blip) kept replaying the original token and failing AUTH after its TTL. This wires an OnConnect hook so every dial AUTHs with a freshly-fetched token. Reproduced against Azure Managed Redis, soaked successfully across many maxConnAge: 5m pool-cycle windows. Could one of you /ok-to-test and take a look? Happy to add a unit test asserting OnConnect is wired when UseEntraID is true if you'll tell me the shape you'd prefer (matching an existing pattern in the package). |
|
Friendly bump on this — it's been a few weeks since I tagged @yaron2 / @shivamkm07, and I've rebased onto current Some context on impact, since I think it's broader than it might look. This fixes #3554: Entra ID auth against Azure Managed Redis fails with We've been running this patch in dev and production against Azure Managed Redis and it cleanly resolves the post-TTL failures. One thing worth flagging for any Azure/Microsoft folks watching: per the AKS Dapr extension's issue-handling policy, Dapr component fixes are expected to land in Dapr OSS first and are then picked up by the managed Happy to add tests, split the change, or adjust the approach if that helps it land. Any chance it could target the next 1.17 patch? |
There was a problem hiding this comment.
Pull request overview
This PR fixes Redis authentication failures when using Microsoft Entra ID by ensuring new go-redis pool connections authenticate with a fresh access token (rather than replaying an expired token captured at component initialization).
Changes:
- Add
OnConnecthooks for go-redis v8/v9 (including failover/cluster variants) to runAUTH ACL <oid> <fresh-token>on each newly-dialed connection. - Refactor Entra ID initialization to stop snapshotting the initial token into
Settings.Password, and instead store the parsedoidplus the AzureTokenCredentialonSettings. - Add
Settings.EntraIDFetchAuthArgshelper to obtain the ACL username + fresh token on demand.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| common/component/redis/v8client.go | Wires Entra ID OnConnect callback for v8 clients; introduces per-connection AUTH path. |
| common/component/redis/v9client.go | Wires Entra ID OnConnect callback for v9 clients; introduces per-connection AUTH path. |
| common/component/redis/settings.go | Adds Entra ID runtime fields and helper for fetching fresh tokens/ACL args. |
| common/component/redis/redis.go | Renames/refactors Entra init and updates token refresh goroutine to use derived OID username. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- AuthACL: Exec the pipeline so the AUTH ACL command is actually sent. UniversalClient exposes only Cmdable (not StatefulCmdable), so AUTH must go through a pipeline — but it was never Exec'd, making the background token-refresh goroutine a silent no-op. - EntraIDFetchAuthArgs: return a clear error when the derived OID username is empty, instead of issuing AUTH ACL "" <token> and surfacing a confusing WRONGPASS. - Add unit tests asserting OnConnect is wired (and Password unset) when UseEntraID is true, and not wired when false, across all v8/v9 node/cluster/failover variants; plus EntraIDFetchAuthArgs coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Paul Stadler <paul.stadler@meshconnect.com>
f6445ab to
3ac139f
Compare
- Store entraIDTokenCredential as azcore.TokenCredential (value) instead
of *azcore.TokenCredential, removing the pointer-to-interface antipattern
and the awkward deref in EntraIDFetchAuthArgs.
- StartEntraIDTokenRefreshBackgroundRoutine now takes *Settings instead of
duplicating username/credential args, removing two sources of truth; the
refresh loop reads the OID and credential off Settings.
- InitEntraIDCredential no longer returns the credential (it is stored on
Settings); returns only the initial token expiry.
- Extract the duplicated token-fetch into Settings.entraIDToken and a single
entraIDRedisScope const, used by init, OnConnect, and the refresh loop.
- Document OnConnect failure/timeout behavior on entraIDOnConnect{V8,V9}.
- Add TestEntraIDOnConnectCallback that invokes the OnConnect callbacks and
asserts they surface token-fetch / missing-OID failures as dial errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Paul Stadler <paul.stadler@meshconnect.com>
An empty oid claim would set an empty Redis ACL username and only surface as a confusing connect-time error; validate it during init instead. Signed-off-by: Paul Stadler <paul.stadler@meshconnect.com>
c903445 to
daa9460
Compare
Resolves conflicts from dapr#4408 (OIDC private_key_jwt auth): - v8/v9 AuthACL: take main's pipeline.Exec fix (equivalent to ours, now redundant) - redis.go: keep the *Settings-based EntraID refresh signature and 2-value InitEntraIDCredential return - settings.go: union of the new OIDC config fields and the EntraID runtime fields/helpers Signed-off-by: Paul Stadler <paul.stadler@meshconnect.com>
|
cc @cicoyle @JoshVanL — would one of you be able to take a look at this, or point me to the right reviewer? (I can't add reviewers directly as an external contributor.) Quick context on why it matters:
Happy to make any changes you'd like. Thanks! |
- Unexport entraIDFetchAuthArgs: it is internal wiring (OnConnect + refresh routine, tests are in-package), so it need not be part of the public API. - Clarify the refresh-goroutine comment: AUTH is per-connection, so it re-authenticates pooled connections as they are exercised, not all at once. - Clarify the token-parse comment: signature/expiry are intentionally not verified; Redis does authoritative validation at AUTH. - Add v9 missing-OID OnConnect test for parity with v8. Signed-off-by: Paul Stadler <paul.stadler@meshconnect.com>
|
@mikeee — you're listed as the reviewer on this one, so tagging you directly. Would you be able to take a look at this stage? It's in a clean spot now:
Happy to make any changes you'd like. Thanks! |
|
Hi @mikeee @cicoyle @JoshVanL — quick update with some production data on this fix. We needed it in the meantime, so we've been running this exact code as a pluggable component next to stock daprd 1.17.9 — now serving the Redis state store for ~30 services across our dev/sandbox/prod clusters (Azure Managed Redis + Entra ID, actors included). Zero I also have a deterministic test for the token-TTL boundary (rotating stub credential + miniredis password rotation — asserts new connections auth with the refreshed token, and that the pre-fix snapshot behavior fails the same scenario). Happy to push it to this branch — I think it covers the earlier review comment about validating fresh-token behavior end-to-end. Happy to make any changes needed to get this over the line. Thanks! |
Description
Fixes #3554, the long-standing report that Entra ID auth against Azure Managed Redis stops working after the initial token's TTL elapses.
When
useEntraID: true, the redis component snapshotted the initial Entra access token intoredis.UniversalOptions.Passwordat component-init time. Every new pool connection opened by go-redis afterwards replayed that static AUTH at connection establishment — but the token has a finite TTL (1–24h depending on credential type and audience). After expiry, any new connection (idle timeout,maxConnAge, network blip, demand spike, pool churn) fails AUTH withWRONGPASS invalid username-password pair.#3632 fixed the background AUTH ACL refresh goroutine being killed by init context cancellation, but only re-AUTHs existing connections; it never updated
Options.Password, so new connections continued to fail with the original snapshot once it expired. This explains the multiple post-#3632 reports on #3554 from users on 1.15.9, 1.16.1, 1.17.x ("token IS being refreshed in logs but pool reconnects fail").What this PR does
Installs an
OnConnecthook on every variant of the underlying go-redis client (v8/v9, plain/cluster/failover) that:*azcore.TokenCredential.AUTH ACL <oid> <fresh-token>on the newly-dialed connection.azcorecaches tokens internally until ~5 minutes before expiry, so per-connectionGetTokencalls are inexpensive in steady state.The static
Passwordsnapshot is no longer set, so go-redis skips its built-in AUTH during connection init and letsOnConnecthandle authentication. The OID — used as the Redis ACL username, per the Microsoft Entra for AMR documentation — is parsed once from the initial token at component init and stored onSettings;OnConnectuses it for every subsequent AUTH.StartEntraIDTokenRefreshBackgroundRoutineis unchanged. It remains the safety net for long-lived connections that survive across token-TTL boundaries without ever being pool-cycled.Issue reference
Please reference the issue this PR will close: #3554
Checklist
go vet ./common/component/redis/...,go build ./common/component/redis/...,go buildagainst all 5 redis-using components:state/redis,pubsub/redis,bindings/redis,lock/redis,configuration/redis)go test ./common/component/redis/... -count=1—ok 1.554s)common/component/redis/entraid_onconnect_test.goassertsOnConnectis wired andPasswordis unset whenUseEntraIDis true, and not wired when false, across all v8/v9 node/cluster/failover variants; plusEntraIDFetchAuthArgscoverage)Testing
Reproduction
The bug was reproduced empirically against an Azure Managed Redis cluster with a workload-identity-bound managed identity that has the default access policy, using daprd 1.17.2:
state.rediscomponent configured asuseEntraID: true, redisType: cluster, enableTLS: true.DaprClient.GetState/SaveState(Dapr Actors-only consumers don't surface this because actor state-store access is lazy).CrashLoopBackOffwith the daprd sidecar loggingWRONGPASS invalid username-password pairfromstate.rediscalls. The crossover aligns with the initial token's TTL.I also verified the AUTH contract independently with a manual probe from inside the pod:
https://redis.azure.com/.defaultand returns a token with the correctaud/appid/oid.AUTH <token>(no username) returnsWRONGPASS.AUTH <oid> <token>returns+OK.This matches the documented AMR auth contract — confirms it's a client-side issue, not an authorization-policy issue.
After this change
dapr/daprv1.17.2 withgo.modpointing at this branch viareplace, deployed to the same cluster, configured the state-store withmaxConnAge: 5mto force the pool to recycle every minute (rather than waiting for the natural ambient cadence).state.redisandpubsub.redis) loaded successfully.WRONGPASSerrors across this period and zero sidecar restarts.Edge cases considered
CLUSTER SHARDSgets the OnConnect hook applied, so dials to discovered IPs use a fresh token each time, not the original snapshot.FailoverOptions.OnConnectis wired.Settings.UsernameandSettings.Passwordare now empty whenuseEntraIDis true. The pre-existing validation inInitEntraIDCredential(formerlyGetEntraIDCredentialAndSetInitialTokenAsPassword) already errored when these were configured externally withuseEntraID, so no behavior change for misconfiguration.redisUsername/redisPasswordanduseEntraID: falseare unaffected (the OnConnect hook is gated ons.UseEntraID).Also fixed here:
AuthACLpipeline was never executedWhile debugging, I noticed that
(c v8Client).AuthACLand the v9 equivalent usedc.client.Pipeline()to send the AUTH ACL command, but never calledPipeline.Exec(ctx). In go-redis v8/v9 that means the queued AUTH command was never actually sent over the wire — the background refresh goroutine had been a silent no-op since #3632 landed, so long-lived idle connections never received the periodic re-AUTH the goroutine was meant to provide.This is now fixed in this PR (both v8 and v9): the pipeline is
Exec'd and its error propagated. Note that go-redis'sUniversalClientonly exposesCmdable(notStatefulCmdable), so AUTH genuinely has to go through a pipeline here — callingAuthACLdirectly on the client doesn't compile; the missing piece was theExeccall.Other review follow-ups addressed
EntraIDFetchAuthArgsnow returns a clear error when the derived OID username is empty, instead of issuingAUTH ACL "" <token>and surfacing a confusingWRONGPASS.Refs