Skip to content

fix(queue): authenticate Redis connections when credentials are configured - #260

Merged
ChiragAgg5k merged 4 commits into
mainfrom
fix/queue-redis-auth
Sep 11, 2026
Merged

fix(queue): authenticate Redis connections when credentials are configured#260
ChiragAgg5k merged 4 commits into
mainfrom
fix/queue-redis-auth

Conversation

@ChiragAgg5k

Copy link
Copy Markdown
Member

What does this PR do?

Connection\Redis has accepted $user and $password in its constructor since 2.0, but getRedis() only ever called connect(). Against a Redis with requirepass or ACL users every publish and consume failed with NOAUTH Authentication required. Downstream this is appwrite/appwrite#13554: the Appwrite publisher pool and the worker consumer both go through this class, so a self-hosted install with an external, password-protected Redis cannot process any queue.

  • Connection\Redis::getRedis() calls auth() right after connect() when a password is configured, using the ACL [user, password] pair when a user is set and the bare password otherwise. A rejected credential throws RedisException inside the existing connect retry loop, so a wrong password fails loudly with the host and port in the message instead of hanging.
  • Connection\RedisCluster gains optional $user / $password constructor parameters and passes them as phpredis's \RedisCluster $auth argument, so cluster mode can authenticate too. Existing call sites are unaffected.
  • docker-compose.yml adds a redis-auth node on host port 16380 with requirepass secretpw and an ACL user worker, and the e2e suite gains RedisAuthTest.

Supersedes #247, which covered Connection\Redis only and had no test.

Test Plan

RedisAuthTest covers the default user with a password, an ACL user, missing credentials, and a wrong password. With the fix:

Redis Auth (Tests\E2E\Adapter\RedisAuth)
 ✔ Password authenticates default user
 ✔ User and password authenticate acl user
 ✔ Missing credentials are rejected
 ✔ Wrong password fails loudly
OK (4 tests, 8 assertions)

The same file against main with src/ stashed:

 ✘ Password authenticates default user
   ├ RedisException: NOAUTH Authentication required.
 ✘ User and password authenticate acl user
   ├ RedisException: NOAUTH Authentication required.
 ✔ Missing credentials are rejected
 ✘ Wrong password fails loudly
   ├ Failed asserting that exception message 'NOAUTH Authentication required.' contains 'WRONGPASS'.
Tests: 4, Assertions: 4, Errors: 2, Failures: 1.

Server-side confirmation that the connection is authenticated as the ACL user rather than default. After new Connection\Redis('127.0.0.1', 16380, 'worker', 'workerpw')->rightPush(...), CLIENT LIST on the redis-auth node shows the pushing client as:

id=181 addr=192.168.156.1:29434 ... cmd=rpush user=worker tot-cmds=2

tot-cmds=2 is AUTH followed by RPUSH.

End to end in Appwrite: with _APP_REDIS_HOST pointed at a Redis where the default user is disabled and only an ACL user exists, the API, cache, pubsub, lock and realtime pools authenticate (they receive an already-authenticated \Redis), while the queue publisher kept failing every hour with [StatsResources] Failed to publish stats resources message: NOAUTH Authentication required. and the worker's BRPOP connections showed user=default. Those two are the paths this class owns. The Appwrite-side change that passes the DSN credentials into this constructor is ready and will bump utopia-php/queue once this is tagged.

bin/monorepo check queue --linked: Pint passes, Rector clean. PHPStan reports 5 errors (Nats::tick, Pool::maintain) that are identical on main without this change.

Related PRs and Issues

…gured

Connection\Redis accepted a user and password but never called auth(), so every publish and consume against a password-protected Redis failed with NOAUTH. Authenticate right after connect(), using the ACL [user, password] form when a user is set, and give RedisCluster the same optional credentials through phpredis's auth argument. Adds a requirepass + ACL compose node and an e2e test covering the default user, an ACL user, missing and wrong credentials.
@greptile-apps

greptile-apps Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The implementation appears safe to merge, although the existing non-blocking gap in Redis Cluster authentication coverage remains.

Summary

  • Standalone connections authenticate after connecting, using either a password or an ACL username/password pair.
  • Cluster connections pass optional credentials through phpredis’s authentication constructor argument.
  • A password-protected Redis fixture and end-to-end tests cover successful standalone authentication and rejected credentials.
  • Changes since the previous review replace Redis error-message assertions with observable operation and server-state checks.

Reviews (2) · Last reviewed commit: "test(queue): assert auth rejection by it..."

Comment thread packages/queue/tests/Queue/E2E/Adapter/RedisAuthTest.php Outdated
Comment thread packages/queue/src/Queue/Connection/RedisCluster.php
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown

Benchmark results

queue — workload shapes across both concurrency axes (4 cores, 600 messages, median of 3)

workload shape redis msg/s nats msg/s
io 1p x 1c 37 38
io 1p x 4c 149 151
io 4p x 1c 148 149
cpu 1p x 1c 157 171
cpu 1p x 4c 171 176
cpu 4p x 1c 384 395
mixed 1p x 1c 61 63
mixed 1p x 4c 162 171
mixed 4p x 1c 236 245

Np x Mc = N consumer processes x M handler coroutines. io yields and should follow
coroutines; cpu does not and should follow processes; mixed is in between.

Shared CI runners — treat absolute numbers as rough, compare modes within a run. Commit 4e7a784.

…er credentials

Greptile flagged the NOAUTH/WRONGPASS message assertions as coupled to Redis
protocol wording. A rejected push now has to throw and leave nothing on the
server for a trusted connection to find, which holds across wording changes.

RedisCluster gained credential parameters without a test that exercised them.
A single-node cluster with requirepass and an ACL user is enough for phpredis
to run the cluster handshake and authenticate, so the same four cases now run
against it too.
Greptile flagged the NOAUTH/WRONGPASS message assertions as coupled to Redis
protocol wording. A rejected push now has to throw and leave nothing on the
server for a trusted connection to find, which holds across wording changes.

Also drops the single-node authenticated cluster fixture added in the previous
commit: cluster credentials are forwarded verbatim to phpredis and are not
worth another compose service in this suite.
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