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

CHANGELOG.md

@@ -6,7 +6,51 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Security
+- **Privileged commands ran BEFORE the ACL permission check (c10k B1).**
+  Both connection handlers intercepted `EVAL`/`EVALSHA`/`SCRIPT`, `ACL`, and
+  `CLUSTER` above the ACL gate, and each intercept `continue`s on a match, so
+  the gate below it never ran — even though the comment attached to that gate
+  claimed it "must run before any command-specific handlers ... so that
+  low-privilege users cannot reach admin commands". Any authenticated account
+  could escalate to full admin: a user holding nothing but `~app:* +get` was
+  correctly refused a plain `SET` yet could run
+  `ACL SETUSER evil on nopass ~* +@all` (persisted), `CONFIG SET` (persisted),
+  arbitrary Lua via `EVAL`, `SCRIPT LOAD`, `REPLICAOF` and `BGSAVE`. The gate
+  now sits above every privileged intercept in both handlers, with `AUTH` and
+  `HELLO` lifted above it as Redis's `NO_AUTH` commands. Verified end-to-end at
+  1 and 4 shards (`tests/acl_privileged_intercepts.rs`).
+- **Unknown ACL users failed OPEN (c10k B2).** All three permission checks
+  started with `users.get(username)?`, and `None` is their "allowed" answer, so
+  a username missing from the table was granted everything. Nothing closes a
+  live session when its account disappears, so `ACL DELUSER alice` / `ACL LOAD`
+  silently PROMOTED every connection alice still had open to `~* +@all`.
+  Unknown users are now denied; `default` is guaranteed present after any load
+  (`ensure_default_user`) so a server whose ACL file omits it is not bricked.
+- **`CLIENT KILL USER` was inert and `CLIENT LIST` reported `user=default` for
+  everyone (c10k B3).** The client registry captured the username once, at
+  accept time; AUTH/HELLO updated only the connection-local copy. The primary
+  incident-response lever for a compromised credential matched nothing. AUTH
+  and HELLO now publish the adopted identity to the registry, and — matching
+  Redis — `ACL DELUSER` disconnects the sessions the deleted user still holds.
+- **The experimental io_uring bridge served commands with no auth (c10k B4).**
+  `MOON_URING=1` (tokio, Linux) binds a second `SO_REUSEPORT` listener on the
+  server's own port whose accept path has no auth gate, no ACL check and no
+  client registry — so on a server with `requirepass`/`aclfile` configured the
+  kernel load-balanced roughly half of all new connections onto a listener that
+  skipped authentication entirely. The documented limitation named maxclients
+  and `CLIENT LIST`/`KILL`, not this. The bridge now refuses to arm (loudly)
+  when authentication is configured and the shard stays on the tokio path.
+
 ### Fixed
+- **TLS park veto samples `wants_write()` after processing, not before (c10k
+  B5).** `Stream::task_park_safe` read `wants_write()` before
+  `process_new_packets()`, which can queue outbound bytes and still return
+  `Ok` — reachable in rustls 0.23 via the TLS 1.2 renegotiation rebuff
+  (`NoRenegotiation` warning alert) — so the connection could park owing the
+  peer a reply. (The TLS 1.3 KeyUpdate variant is *not* reachable: rustls
+  defers that reply until the next outbound record; `tests/tls_park_keyupdate.rs`
+  pins the behaviour so a future rustls change is caught.)
 - **A client that disconnects while blocked is now reaped (c10k hardening
   A1).** The infinite-wait `select!` behind `BLPOP`/`BRPOP`/`BLMOVE`/
   `BZPOPMIN`/`BZPOPMAX`/`BLMPOP`/`BZMPOP`/`BRPOPLPUSH` had exactly two arms,

src/acl/io.rs

@@ -113,7 +113,12 @@ pub fn acl_load(path: &str) -> std::io::Result<AclTable> {
 pub fn acl_table_from_config(config: &crate::config::ServerConfig) -> AclTable {
     if let Some(ref path) = config.aclfile {
         match acl_load(path) {
-            Ok(table) => {
+            Ok(mut table) => {
+                // c10k B2: the permission checks now DENY an unknown user, so
+                // an ACL file that never defines `default` would lock out
+                // every connection. Redis guarantees `default` exists; so do
+                // we, seeded from requirepass exactly as the no-file path below.
+                table.ensure_default_user(config.requirepass.as_deref());
                 return table;
             }
             Err(e) if e.kind() == std::io::ErrorKind::NotFound => {

src/acl/table.rs

@@ -299,6 +299,26 @@ impl AclTable {
         self.bump_version();
     }
 
+    /// Guarantee the `default` user exists, creating it from `requirepass`
+    /// (or nopass) if an ACL file did not define it. No-op when present.
+    ///
+    /// c10k hardening B2 support. The permission checks below now DENY an
+    /// unknown user instead of falling open, so `default` missing from the
+    /// table would lock out every connection on a server with no auth
+    /// configured at all. Redis likewise guarantees `default` always exists.
+    /// Call after any load that can produce an arbitrary user set (startup
+    /// aclfile load, `ACL LOAD`).
+    pub fn ensure_default_user(&mut self, requirepass: Option<&str>) {
+        if self.users.contains_key("default") {
+            return;
+        }
+        let user = match requirepass {
+            Some(p) if !p.is_empty() => AclUser::new_default_with_password(p),
+            _ => AclUser::new_default_nopass(),
+        };
+        self.set_user("default".to_string(), user);
+    }
+
     /// Bootstrap from ServerConfig. Loads aclfile if configured, otherwise creates
     /// the default user from requirepass (or nopass).
     pub fn load_or_default(config: &ServerConfig) -> Self {
@@ -386,7 +406,16 @@ impl AclTable {
         cmd: &[u8],
         _args: &[Frame],
     ) -> Option<String> {
-        let user = self.users.get(username)?;
+        // c10k hardening B2: an unknown user is DENIED, never allowed.
+        // This used to be `self.users.get(username)?` - `None` means
+        // "allowed", so a user that vanished from the table fell open to
+        // full `~* +@all`. `ACL DELUSER alice` / `ACL LOAD` do not close
+        // alice's live sessions, so revoking her account silently
+        // PROMOTED every connection she still had open. `default` is
+        // guaranteed present by `ensure_default_user`.
+        let Some(user) = self.users.get(username) else {
+            return Some(format!("user {} no longer exists", username));
+        };
         // Hot path: unrestricted user (default `on nopass ~* &* +@all`)
         // short-circuits before any per-command allocation. Profile showed
         // ~1% of CPU here for the lowercasing + HashSet probe; the
@@ -416,7 +445,16 @@ impl AclTable {
         args: &[Frame],
         is_write: bool,
     ) -> Option<String> {
-        let user = self.users.get(username)?;
+        // c10k hardening B2: an unknown user is DENIED, never allowed.
+        // This used to be `self.users.get(username)?` - `None` means
+        // "allowed", so a user that vanished from the table fell open to
+        // full `~* +@all`. `ACL DELUSER alice` / `ACL LOAD` do not close
+        // alice's live sessions, so revoking her account silently
+        // PROMOTED every connection she still had open. `default` is
+        // guaranteed present by `ensure_default_user`.
+        let Some(user) = self.users.get(username) else {
+            return Some(format!("user {} no longer exists", username));
+        };
         // Hot path: unrestricted user skips extract_command_keys + the
         // O(patterns*keys) glob match loop. Profile showed ~1.2% of CPU
         // here, most of it in glob_match and Vec allocation for the
@@ -456,7 +494,16 @@ impl AclTable {
 
     /// Check channel access for pub/sub.
     pub fn check_channel_permission(&self, username: &str, channel: &[u8]) -> Option<String> {
-        let user = self.users.get(username)?;
+        // c10k hardening B2: an unknown user is DENIED, never allowed.
+        // This used to be `self.users.get(username)?` - `None` means
+        // "allowed", so a user that vanished from the table fell open to
+        // full `~* +@all`. `ACL DELUSER alice` / `ACL LOAD` do not close
+        // alice's live sessions, so revoking her account silently
+        // PROMOTED every connection she still had open. `default` is
+        // guaranteed present by `ensure_default_user`.
+        let Some(user) = self.users.get(username) else {
+            return Some(format!("user {} no longer exists", username));
+        };
         if user.unrestricted {
             return None;
         }
@@ -571,6 +618,66 @@ mod tests {
         ServerConfig::parse_from(args)
     }
 
+    /// c10k hardening B2. All three permission checks used to start with
+    /// `self.users.get(username)?` — and `None` is their "allowed" answer, so
+    /// a username that is not in the table was granted everything. Nothing
+    /// closes a live session when its account goes away (`ACL DELUSER`, `ACL
+    /// LOAD` dropping a user), so revoking an account silently PROMOTED every
+    /// connection it had already authenticated to full `~* +@all`.
+    #[test]
+    fn unknown_user_is_denied_by_every_check() {
+        let table = AclTable::new(); // no users at all
+        let args: Vec<Frame> = vec![Frame::BulkString(Bytes::from_static(b"k"))];
+
+        assert!(
+            table
+                .check_command_permission("ghost", b"SET", &args)
+                .is_some(),
+            "unknown user must be denied commands, not allowed"
+        );
+        assert!(
+            table
+                .check_key_permission("ghost", b"SET", &args, true)
+                .is_some(),
+            "unknown user must be denied keys, not allowed"
+        );
+        assert!(
+            table.check_channel_permission("ghost", b"news").is_some(),
+            "unknown user must be denied channels, not allowed"
+        );
+    }
+
+    /// The flip side of the fail-closed change: `default` must always be
+    /// present, or a server whose ACL file never mentions it would refuse
+    /// every connection.
+    #[test]
+    fn ensure_default_user_creates_only_when_missing() {
+        let mut table = AclTable::new();
+        table.ensure_default_user(None);
+        let user = table.get_user("default").expect("default was created");
+        assert!(user.unrestricted(), "nopass default must be unrestricted");
+
+        // With requirepass, the created default carries the password.
+        let mut table = AclTable::new();
+        table.ensure_default_user(Some("hunter2"));
+        assert_eq!(
+            table.authenticate("default", "hunter2"),
+            Some("default".to_string())
+        );
+        assert_eq!(table.authenticate("default", "wrong"), None);
+
+        // Present already: left exactly as the file defined it.
+        let mut table = AclTable::new();
+        table.apply_setuser("default", &["on", "~app:*", "-@all", "+get"]);
+        table.ensure_default_user(Some("hunter2"));
+        assert!(
+            table
+                .check_command_permission("default", b"SET", &[])
+                .is_some(),
+            "an existing restricted default must not be overwritten by a fresh unrestricted one"
+        );
+    }
+
     #[test]
     fn default_user_is_unrestricted() {
         // Every construction path that yields a "fully open" default