Authenticate xdist StatusDB connections - #358
Conversation
icemac
left a comment
There was a problem hiding this comment.
Reviewed the handshake against the branch (suite also run under xdist: 175 passed). The change itself is sound — the token is assigned before run_server starts, the with conn, suppress(...) nesting order is right, failures_db is always a ServerStatusDB where .token is read, the token survives execnet serialization, and both new tests fail before the change and pass after.
Three things on the error paths, though; the first two chain into a session hang under xdist.
— Comment created by Claude
| def run_connection(self, conn): | ||
| with suppress(ConnectionError): | ||
| with conn, suppress(ConnectionError): | ||
| authenticated = secrets.compare_digest(self._sock_recv(conn), self.token) |
There was a problem hiding this comment.
Non-ASCII or invalid-UTF-8 handshake input kills this thread instead of rejecting the client.
secrets.compare_digest raises TypeError when either str argument contains non-ASCII characters, and _sock_recv's buf.decode() raises UnicodeDecodeError on invalid UTF-8. Neither is a ConnectionError, so suppress(ConnectionError) does not catch them.
Sending "\u00e9\n" to the port on this branch dumps
Exception in thread Thread-3 (run_connection):
...
authenticated = secrets.compare_digest(self._sock_recv(conn), self.token)
TypeError: comparing strings with non-ASCII characters is not supported
to stderr in the middle of the user's test run, and the client receives b'' rather than the intended "0" rejection.
Since this handshake is exactly the code now expected to face arbitrary local input, it should compare raw bytes (recv the token undecoded and compare_digest(raw, self.token.encode())), or wrap the comparison so any malformed input yields a clean "0".
— Comment created by Claude
| super().__init__() | ||
| self.sock.connect(("127.0.0.1", sock_port)) | ||
| self._sock_send(self.sock, token) | ||
| if self._sock_recv(self.sock) != "1": |
There was a problem hiding this comment.
A worker can hang forever here (busy-spinning) if the server never answers the handshake.
_sock_recv loops on conn.recv(1) and only breaks on the b"\n" delimiter. At EOF recv returns b"", which is neither the delimiter nor an error, so the loop never terminates.
That is now directly reachable: whenever the server-side run_connection thread dies before replying (see the compare_digest comment above, or any other non-ConnectionError exception), the new with conn closes the socket, and this call sits in _sock_recv burning 100% of a core — the ConnectionError("StatusDB authentication failed") below is never raised. The xdist controller then waits on a worker that never finishes pytest_configure, hanging the whole session.
The same EOF gap leaves a live spinning thread on the server side: connecting and immediately closing takes threading.active_count() from 2 to 3 permanently.
_sock_recv should treat b"" as end-of-stream and raise ConnectionError; a settimeout on the handshake would be worth considering too.
— Comment created by Claude
| @@ -781,7 +787,12 @@ def run_server(self): | |||
| t.start() | |||
There was a problem hiding this comment.
Minor: the handshake buffers unboundedly before authentication, and connections are unlimited.
_sock_recv appends to buf with no length cap, and every accepted connection gets its own daemon thread with no connection limit — both before any authentication has happened. Any local process can connect and stream bytes without a newline until the controller runs out of memory.
Only a local DoS, but capping the pre-auth read at the known token length (64 hex chars) is nearly free and fits the threat model this PR is addressing.
— Comment created by Claude
Generate a per-session token on the xdist controller and pass it to workers through
workerinput.StatusDB now requires a successful handshake before processing commands and rejects clients with an invalid token.
Tests cover the authentication check and passing the connection details to workers.