fix(security): ACL gate before privileged intercepts, fail-closed unknown users, real CLIENT KILL USER (c10k cluster B)

[TinDang97]

Stacked on #428 (fix/c10k-blocked-client-eof, itself on #427←#426). B1 reorders the command-dispatch chain in the same two handler files A1 rewrites the read loop of. Review the top three commits only; the base merges first.

Cluster B of the c10k hardening review (tmp/C10K-HARDENING-REVIEW.md). Four security fixes and one ordering fix.

B1 — privileged commands ran BEFORE the ACL gate 🔴

Both handlers intercept EVAL/EVALSHA/SCRIPT, ACL and CLUSTER above the ACL permission check, and each intercept continues on a match — so the gate below it never ran. The comment attached to that gate claimed the exact invariant the code above it violated:

Must run before any command-specific handlers (CONFIG, REPLICAOF, etc.) so that low-privilege users cannot reach admin commands.

Verified exploitable on main before the fix. A user holding nothing but ~app:* +get is correctly refused a plain SET, and then:

command	result on main
ACL SETUSER evil on nopass ~* +@all	+OK — and it persisted
CONFIG SET maxmemory 999999999	+OK — and it persisted
EVAL "return 'pwned'" 0	pwned — arbitrary Lua
SCRIPT LOAD / REPLICAOF no one / BGSAVE	all accepted

Any authenticated account escalates to full admin. The gate now sits above every privileged intercept in both handlers, with AUTH and HELLO lifted above it — Redis marks both NO_AUTH, and gating them would strand a restricted client with no way to re-authenticate and break the RESP3 handshake.

Why this survived: the consistency suites authenticate as default, which is unrestricted and takes the acl_skip_allowed() fast path. They are structurally blind to ACL ordering.

B2 — unknown ACL users failed OPEN 🟠

check_command_permission, check_key_permission and check_channel_permission each began with self.users.get(username)? — and None is their allowed answer. Nothing closes a live session when its account disappears, so ACL DELUSER alice (or an ACL LOAD that drops her) silently promoted every connection alice still held to full ~* +@all.

Unknown users are denied now. The matching guard is ensure_default_user: default is inserted from requirepass (exactly as the no-aclfile path does) after any load that could omit it, so fail-closed cannot brick a server whose ACL file never mentions default.

B3 — CLIENT KILL USER was inert 🟡

client_registry::register captures the username once, at accept time, when it is always default; AUTH/HELLO updated only the connection-local copy. CLIENT LIST reported user=default for every session and CLIENT KILL USER alice returned 0 — the primary incident-response lever for a compromised credential, useless.

All 13 assignment sites across the three handlers now go through one ConnectionState::adopt_user, which publishes to the registry as well as refreshing the ACL cache. With identity tracked, ACL DELUSER also disconnects the sessions the deleted user still holds, as Redis does.

B4 — the io_uring bridge served commands with no auth 🔴

MOON_URING=1 (tokio, Linux) binds a second SO_REUSEPORT listener on the server's own port and serves from its own accept path — no auth gate, no ACL check, no client registry. On a server that has configured authentication the kernel load-balances new connections between a listener that enforces auth and one that does not. The documented limitation named maxclients and CLIENT LIST/KILL; it did not name this.

The bridge now refuses to arm (at error!) when requirepass/aclfile is set and the shard stays on the pure-tokio path. Refusing is the fail-closed answer and matches how every other failure in that block degrades; killing the process over an experimental opt-in would be worse.

B5 — TLS park veto sampled wants_write() too early 🟡, and the review's stated cause was wrong

Stream::task_park_safe read wants_write() before process_new_packets(). Processing a buffered record can queue outbound bytes and still return Ok, so the veto read a pre-processing state, passed, and parked the connection owing the peer a reply.

The review named TLS 1.3 KeyUpdate(update_requested) as the trigger. It isn't reachable — rustls parks its KeyUpdate reply in queued_key_update_message and only moves it into sendable_tls on the next outbound record, so wants_write() reads false under either ordering. I found this while writing the red test: the premise assertion failed.

The reachable case is the TLS 1.2 renegotiation rebuff (common_state.rs: a post-handshake ClientHello → send_warning_alert(NoRenegotiation) → Ok), and moon builds rustls with tls12 enabled. That one cannot be driven without forging encrypted records, so it has no e2e test. What ships instead is a canary: tests/tls_park_keyupdate.rs pins rustls's deferral on a real in-memory 1.3 session pair, so if a future rustls starts queueing eagerly the test fails loudly and the KeyUpdate case becomes live — at which point the reordered veto already handles it.

Verification

RED proven by reverting src/ and re-running, at both shard configs:

	shards=1	shards=4
B1 ACL SETUSER evil …	+OK	+OK
B3 CLIENT LIST	user=default	user=default
B3 ACL DELUSER	session survived	session survived

Tests added

• tests/acl_privileged_intercepts.rs — 9 escalation vectors must be NOPERM, with a GET positive control, no-side-effect checks on the denied ACL SETUSER/CONFIG SET, and AUTH/HELLO still exempt.
• tests/acl_user_revocation.rs — CLIENT LIST reports the post-AUTH user; CLIENT KILL USER tears the session down and leaves others alone; ACL DELUSER disconnects.
• tests/tls_park_keyupdate.rs — the B5 canary.
• Unit: unknown-user denial ×3, ensure_default_user (creates only when missing, honours requirepass), uring_bridge_allowed.

Gates — fmt; clippy -D warnings on default and runtime-tokio,jemalloc (macOS + Linux VM); lib 4452 monoio / 3617 tokio; the conn/auth integration suites (maxclients_reject_parity, parked_idle_parity, idle_downshift_parity, tls_idle_downshift_parity, client_tracking_invalidation, plus the stack's own blocking_peer_eof / blocking_ghost_waiter) green on both runtimes, re-run after the rebase onto #428.

Broad regression — scripts/test-consistency.sh on the Linux VM: 209/209 PASS. scripts/test-commands.sh --skip-bench shows 13 failures — byte-identical on origin/main built and run the same way (bash mangling null bytes in the KNN $q param); pre-existing, not from this branch.

Notes

• .planning submodule pointer intentionally untouched.
• Remaining in the campaign: clusters C (resource limits), D2–D4, E, F.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 059e3859-44ab-4350-8d7d-9240a0bd6287

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Fix ACL bypasses: gate privileged intercepts, fail-closed users, real kill-by-user

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Move ACL enforcement ahead of all privileged command intercepts (AUTH/HELLO exempt).
• Deny unknown ACL users and ensure default always exists after ACL loads.
• Track post-AUTH identity in client registry; enforce revocation via DELUSER disconnects.
• Refuse io_uring bridge when auth is configured; fix TLS park safety ordering and add canary test.

Diagram

graph TD
  C((Client)) --> H["Connection handlers"] --> A["AUTH/HELLO (NO_AUTH)"] --> U["ConnectionState.adopt_user"] --> R["Client registry"]
  H --> G["ACL gate"] --> P["Privileged intercepts"]
  G --> T["ACL table"]
  T --> D["ensure_default_user"]
  P --> K["ACL DELUSER => kill_clients"] --> R

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Unify handler dispatch pipelines

• ➕ Eliminates duplicated ordering logic between monoio and sharded handlers
• ➕ Makes ACL gate placement a single invariant, reducing future regressions
• ➖ Larger refactor with higher short-term risk and more merge conflicts
• ➖ May constrain runtime-specific optimizations currently present in each handler

2. Centralize ACL enforcement as a single pre-dispatch function

• ➕ Keeps current architecture but forces consistent gate ordering
• ➕ Easier to audit and test compared to scattered intercept ordering
• ➖ Still leaves multiple call sites (one per handler loop)
• ➖ Requires careful handling of NO_AUTH exemptions and MULTI/queued commands

3. Make client registry user a live reference (e.g., Arc/atomic swap)

• ➕ Avoids needing explicit update calls on AUTH/HELLO success
• ➕ Reduces chance of future paths forgetting to publish identity
• ➖ More complex memory/locking model for the registry
• ➖ Harder to reason about snapshot semantics for CLIENT LIST output

Recommendation: The PR’s approach is the right minimal-risk fix for a verified privilege-escalation path: reorder the gate, fail-closed unknown users, and publish identity updates via a single adopt_user API. Longer-term, consider consolidating handler dispatch or extracting a shared ACL pre-dispatch routine to prevent reintroducing ordering drift across independent handler implementations.

Files changed (14) +1197 / -131

Enhancement (1) +24 / -0

core.rsAdd 'adopt_user' to publish identity to registry +24/-0

Add 'adopt_user' to publish identity to registry

• Adds a single entry point for changing authenticated identity, refreshing ACL cache and updating the client registry so CLIENT LIST and CLIENT KILL USER reflect post-AUTH/HELLO state.

src/server/conn/core.rs

Bug fix (9) +387 / -131

io.rsEnsure 'default' exists after aclfile load +6/-1

Ensure 'default' exists after aclfile load

• After loading an ACL file, calls 'ensure_default_user()' seeded from 'requirepass' to prevent fail-closed behavior from locking out all connections when 'default' is omitted.

src/acl/io.rs

table.rsFail-closed unknown users; add 'ensure_default_user' + tests +110/-3

Fail-closed unknown users; add 'ensure_default_user' + tests

• Introduces 'ensure_default_user()' and updates command/key/channel permission checks to deny unknown usernames instead of implicitly allowing them. Adds unit tests to verify unknown users are denied and that 'default' is created only when missing.

src/acl/table.rs

acl.rsDisconnect sessions on DELUSER; preserve default on ACL LOAD +20/-0

Disconnect sessions on DELUSER; preserve default on ACL LOAD

• Tracks revoked usernames during 'ACL DELUSER' and disconnects their live sessions via 'client_registry::kill_clients' after releasing the ACL lock. Ensures 'default' is reinserted on 'ACL LOAD' if missing, using runtime 'requirepass'.

src/command/acl.rs

dispatch.rsUse 'adopt_user' for AUTH/HELLO identity updates +4/-8

Use 'adopt_user' for AUTH/HELLO identity updates

• Replaces direct 'current_user' assignment + cache refresh with 'ConnectionState::adopt_user' in AUTH/HELLO code paths so the registry receives identity updates.

src/server/conn/handler_monoio/dispatch.rs

mod.rsMove ACL gate ahead of privileged intercepts (monoio path) +53/-25

Move ACL gate ahead of privileged intercepts (monoio path)

• Reorders the dispatch loop so the ACL gate executes before privileged intercept handlers (EVAL/SCRIPT/CLUSTER/ACL routing), while keeping AUTH/HELLO ahead of the gate as NO_AUTH commands. Leaves an explanatory comment where the old gate used to be.

src/server/conn/handler_monoio/mod.rs

mod.rsMove ACL gate ahead of privileged intercepts (sharded path) +98/-78

Move ACL gate ahead of privileged intercepts (sharded path)

• Hoists AUTH/HELLO handling above the ACL gate for post-auth flows and moves the ACL permission checks above command-specific privileged intercepts. Replaces identity updates with 'adopt_user' and removes the old, misplaced ACL check block.

src/server/conn/handler_sharded/mod.rs

handler_single.rsPublish post-AUTH identity in single-connection handler +5/-10

Publish post-AUTH identity in single-connection handler

• Updates all AUTH/HELLO success paths to call 'adopt_user', ensuring the client registry reflects the authenticated username consistently across runtimes/handlers.

src/server/conn/handler_single.rs

event_loop.rsRefuse io_uring bridge when auth is configured + unit test +68/-0

Refuse io_uring bridge when auth is configured + unit test

• Introduces 'uring_bridge_allowed()' and prevents arming the experimental io_uring bridge when 'requirepass' or 'aclfile' is set, logging an error and falling back to tokio I/O. Adds a unit test asserting the bridge is denied under authentication.

src/shard/event_loop.rs

stream.rsFix TLS park safety: process packets before wants_write check +23/-6

Fix TLS park safety: process packets before wants_write check

• Reorders 'task_park_safe()' to call 'process_new_packets()' before checking 'session.wants_write()', preventing task parking while outbound TLS bytes are queued by packet processing. Adds detailed rationale and references the new KeyUpdate canary test.

vendor/monoio-rustls/src/stream.rs

Tests (3) +742 / -0

acl_privileged_intercepts.rsE2E test: privileged intercepts must be ACL-gated +259/-0

E2E test: privileged intercepts must be ACL-gated

• Adds an integration-style test that spawns the 'moon' binary, provisions a low-privilege user, and verifies privileged commands (ACL/CONFIG/EVAL/SCRIPT/REPLICAOF/BGSAVE/CLUSTER) return NOPERM and have no side effects. Runs under both single-shard and multi-shard modes.

tests/acl_privileged_intercepts.rs

acl_user_revocation.rsE2E test: CLIENT KILL USER + DELUSER disconnect semantics +254/-0

E2E test: CLIENT KILL USER + DELUSER disconnect semantics

• Adds tests that verify CLIENT LIST shows the post-AUTH username, CLIENT KILL USER matches and disconnects the correct session, and ACL DELUSER disconnects live sessions of the deleted user. Runs under both 1 and 4 shards.

tests/acl_user_revocation.rs

tls_park_keyupdate.rsCanary test for rustls KeyUpdate deferral assumption +229/-0

Canary test for rustls KeyUpdate deferral assumption

• Adds an in-memory rustls client/server session test asserting KeyUpdate replies are deferred (wants_write remains false after processing). This pins assumptions behind the reordered TLS park veto and flags future rustls behavior changes.

tests/tls_park_keyupdate.rs

Documentation (1) +44 / -0

CHANGELOG.mdDocument c10k hardening fixes (B1–B5) +44/-0

Document c10k hardening fixes (B1–B5)

• Adds detailed changelog entries describing the ACL bypass, fail-open unknown users, inert CLIENT KILL USER, io_uring auth bypass, and TLS park ordering issue. References new tests that validate the security invariants.

CHANGELOG.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. DELUSER may drop reply ✓ Resolved 🐞 Bug ≡ Correctness

Description

ACL DELUSER now calls client_registry::kill_clients(..., None), which force-shuts down matching
sockets immediately; if the caller deletes its own username, its socket can be shut down before the
DELUSER integer reply is flushed. This breaks expected command semantics (reply-then-disconnect) and
can surface to clients as a connection error instead of a valid DELUSER response.

Code

src/command/acl.rs[R197-202]

+            drop(table);
+            for name in revoked {
+                crate::client_registry::kill_clients(
+                    &crate::client_registry::KillFilter::User(name),
+                    None,
+                );

Relevance

●●● Strong

Team often accepts ordering fixes preventing replies/records from being dropped due to premature
shutdown/teardown.

PR-#95
PR-#17

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new ACL DELUSER path performs out-of-band kills immediately, and it passes None as
self_id, while kill_clients only avoids fd shutdown when self_id == Some(entry.id); therefore
a matching caller can be force-closed mid-command before its reply is written.

src/command/acl.rs[166-205]
src/client_registry.rs[315-333]
src/command/acl.rs[23-30]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ACL DELUSER` disconnects all sessions for deleted users by calling `client_registry::kill_clients(..., None)`. Because `self_id` is `None`, `kill_clients` will also `shutdown(2)` the socket for the connection *executing* `ACL DELUSER` if it matches the deleted user, which can prevent the integer reply from being delivered.

### Issue Context
`client_registry::kill_clients` has explicit support for self-kill semantics (`self_id`) to avoid fd shutdown until after the reply is sent. `handle_acl` currently does not receive the caller's client id, so it cannot pass it through.

### Fix Focus Areas
- src/command/acl.rs[166-205]
- src/client_registry.rs[315-333]
- src/server/conn/handler_sharded/mod.rs[562-606]
- src/server/conn/handler_monoio/mod.rs[1086-1136]

### Suggested approach
1. Thread the caller `client_id` into `handle_acl(...)` (add a parameter).
2. In the `DELUSER` path, call:
  - `kill_clients(filter, Some(client_id))` for each revoked username (so self-kill stays cooperative and the reply can flush).
3. Ensure connection handlers close the connection promptly when `kill_flag` is set (already true today), so self-deletion still disconnects after the response.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 55 rules

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}