Skip to content

redis: add AWS ElastiCache, Azure AD, and GCP Memorystore dynamic-auth backends - #265

Open
reyortiz3 wants to merge 7 commits into
mainfrom
add-redis-dynamic-auth
Open

redis: add AWS ElastiCache, Azure AD, and GCP Memorystore dynamic-auth backends#265
reyortiz3 wants to merge 7 commits into
mainfrom
add-redis-dynamic-auth

Conversation

@reyortiz3

Copy link
Copy Markdown
Contributor

Summary

  • Mirrors the postgres package's IAM/Entra ID/OAuth2 dynamic-auth pattern for redis, adding three backends: AWS ElastiCache/MemoryDB IAM, Azure Entra ID (Azure Cache for Redis), and GCP Memorystore for Redis Cluster IAM.
  • go-redis has no pgx-style BeforeConnect hook and keeps long-lived pooled connections (unlike pgx's frequent reconnects), so tokens are refreshed differently here: NewClient leaves Password unset and installs an Options.OnConnect hook that mints a fresh token and issues AUTH itself, plus a backend-scoped ConnMaxLifetime (used when Config.ConnMaxLifetime is zero) that makes go-redis periodically retire and redial pooled connections before a token would expire.
  • AWS ElastiCache/MemoryDB has no auth.BuildAuthToken-style helper like RDS, so awsiam.go hand-signs a presigned SigV4 "connect" request per AWS's documented IAM-auth token format.
  • Config.DynamicAuth requires Config.Username (the IAM/ACL identity) and forbids a static Config.Password.

Test plan

  • task lint — 0 issues
  • task testredis and postgres packages pass with -race; the one repo-wide failure (networking.TestValidateCallbackPort) is pre-existing/environmental (a local kubectl process holding port 8090), confirmed unrelated by reproducing it on main with redis/ unstaged
  • task license-check — passes
  • New table-driven tests cover: config validation (mutual exclusion of Password/DynamicAuth, required Username, exactly-one-backend, AWS region/cluster-name requirements), each backend's token-func construction, and client.go's OnConnect/ConnMaxLifetime wiring (including that an explicit Config.ConnMaxLifetime overrides the backend default, and that backend-construction errors propagate out of NewClient)

🤖 Generated with Claude Code

reyortiz3 and others added 2 commits September 1, 2026 10:23
…h backends

Mirrors postgres's IAM/Entra ID/OAuth2 dynamic-auth pattern for Redis and
Valkey. Unlike pgx, go-redis has no per-dial BeforeConnect hook and keeps
long-lived pooled connections, so tokens are refreshed via an OnConnect
hook that performs AUTH itself plus a backend-scoped ConnMaxLifetime that
forces periodic reconnects before a token would expire.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@ChrisJBurns ChrisJBurns left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-Agent Consensus Review

Agents consulted: cloud-auth-security, redis-lifecycle, general-code-review, codex, aws-conflict-resolution

Consensus Summary

# Finding Consensus Severity Action
1 Dynamic authentication runs after go-redis initialization 10/10 HIGH Fix
2 Failover sends data-node IAM credentials to Sentinel daemons 10/10 HIGH Fix
3 GCP token refresh retains the client-construction context 10/10 HIGH Fix
4 AWS tokens use the wrong SigV4 payload hash 9/10 HIGH Fix
5 GCP IAM authentication sends the wrong Redis username 9/10 HIGH Fix
6 Dynamic auth permits plaintext cloud credentials 8/10 HIGH Fix
7 AWS credentials are rebuilt for every pooled connection 9/10 MEDIUM Fix
8 ElastiCache serverless tokens omit ResourceType 8/10 MEDIUM Fix
9 Core token and authenticated-connection paths lack behavioral tests 8/10 MEDIUM Fix
10 ServiceName accepts values outside its documented enum 7/10 MEDIUM Fix

Overall

This PR adds Redis dynamic authentication for AWS, Azure, and GCP, but the central OnConnect approach is incompatible with go-redis v9.22's initialization order. Authentication occurs after HELLO and initialization commands, which breaks nonzero databases, downgrades otherwise valid clients to RESP2, and sends data-plane credentials to Sentinel daemons.

The provider implementations also have blocking protocol errors: AWS signs the wrong canonical payload and GCP sends an unsupported service-account username. GCP refresh lifetime and transport-security handling introduce additional runtime and credential-exposure risks. These need to be addressed before merge.

Documentation

redis/doc.go and the comments on Config.Username/DynamicAuth should be updated with the corrected credential-provider mechanism, provider-specific GCP username behavior, mandatory TLS requirements, and the supported ElastiCache resource types.


Generated with Codex using the pr-review workflow

Comment thread redis/client.go Outdated
Comment thread redis/client.go Outdated
Comment thread redis/gcpiam.go Outdated
Comment thread redis/awsiam.go Outdated
Comment thread redis/config.go Outdated
Comment thread redis/config.go
Comment thread redis/awsiam.go Outdated
Comment thread redis/awsiam.go
Comment thread redis/client_test.go Outdated
Comment thread redis/config.go Outdated
reyortiz3 and others added 2 commits September 2, 2026 16:57
…validation gaps

Address PR #265 review findings:
- Switch from OnConnect to Options.CredentialsProviderContext: go-redis
  only calls OnConnect after HELLO/AUTH and SELECT have completed, which
  broke non-zero DB selection and silently downgraded RESP3 connections
  to RESP2. CredentialsProviderContext resolves before that handshake.
  This also stops Sentinel discovery connections from receiving data-node
  IAM credentials, since FailoverOptions propagates
  CredentialsProviderContext (unlike OnConnect) only to the master
  client, never to the internal sentinel-daemon connections.
- Use a background context for GCP's DefaultTokenSource construction so a
  canceled NewClient construction context can't poison later token
  refreshes.
- Fix the AWS SigV4 payload hash: ElastiCache/MemoryDB requires SHA-256 of
  the empty body, not "UNSIGNED-PAYLOAD".
- Make GCP Memorystore IAM auth token-only (no username), matching its
  documented contract; Config.Username must now be empty for that backend
  and required for AWS/Azure.
- Require verified TLS when DynamicAuth is configured (bearer tokens over
  plaintext/unverified TLS are replayable), with an explicit
  AllowInsecureTransport escape hatch.
- Cache the AWS credential-provider chain at construction instead of
  reloading it (IMDS/web-identity/STS discovery) on every token mint.
- Add ElastiCache/MemoryDB Serverless support via ResourceType, and
  validate ServiceName/ResourceType against their supported enums.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@reyortiz3

Copy link
Copy Markdown
Contributor Author

Pushed a fix for all 10 findings from the multi-agent review:

HIGH

  1. Switched Options.OnConnectOptions.CredentialsProviderContext, which go-redis resolves during the HELLO/AUTH handshake in initConn, before RESP3 negotiation and SELECTOnConnect only ran afterward, breaking non-zero DB selection and silently downgrading RESP3 connections. Added a miniredis-based regression test (TestCredentialsProviderContext_ResolvesBeforeHelloAndDBSelect).
  2. Fixed as a side effect of (1): go-redis's FailoverOptions.sentinelOptions() copies OnConnect onto its internal Sentinel-daemon connections but never copies CredentialsProviderContext — so dynamic-auth credentials for the data-node identity no longer reach the Sentinel daemons at all.
  3. GCP's token source now captures context.Background() instead of NewClient's (possibly short-lived) construction context.
  4. AWS SigV4 payload hash corrected to the empty-body SHA-256 constant instead of UNSIGNED-PAYLOAD.
  5. GCP Memorystore IAM auth is now token-only (empty username); Config.Username is validated as forbidden for that backend and required for AWS/Azure.
  6. Dynamic auth now requires verified TLS (Config.TLS set, InsecureSkipVerify false), with an explicit DynamicAuthConfig.AllowInsecureTransport opt-out.

MEDIUM
7. AWS credential-provider chain is now loaded once at construction and cached in the closure, instead of being reloaded (IMDS/web-identity/STS) on every token mint.
8. Added DynamicAuthAWSElastiCacheIAM.ResourceType for ElastiCache/MemoryDB Serverless.
9. Added a behavioral test exercising the real go-redis handshake against miniredis, plus deterministic SigV4-signing tests using a fake static AWS credentials provider. Did not add a full mocking seam for Azure/GCP token acquisition — consistent with this package's and postgres's existing precedent of not invoking real cloud SDKs in unit tests.
10. ServiceName (and the new ResourceType) are now validated against their documented enums.

All existing + new tests pass (task test, -race), task lint is clean, task license-check passes. Replied inline on each finding with the specific fix and resolved the threads.

reyortiz3 and others added 3 commits September 2, 2026 16:59
Fixes the Go Vulnerability Check CI failure on this branch (also present
on main's current HEAD independently of this PR's changes).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants