Harden gRPC transport - #266
Conversation
Limit concurrent handshakes and apply a deadline so unauthenticated connections cannot retain sockets and tasks without bound. This commit was created with assistance from Codex.
Limit HTTP/2 streams and concurrent body collection so unauthenticated clients cannot multiply the per-request body allocation without bound. Reject requests that omit authentication before reading their bodies. This commit was created with assistance from Codex.
Omit oversized encoded error messages from response headers and trailers so attacker-controlled paths cannot cause large response allocations. This commit was created with assistance from Codex.
|
👋 Thanks for assigning @joostjager as a reviewer! |
Mark a stream terminal after an unrecoverable framing, decoding, or transport error so later reads cannot repeat the same failure forever. This commit was created with assistance from Codex.
joostjager
left a comment
joostjager
left a comment
There was a problem hiding this comment.
I raised the risk of hand-rolling previously in PR #220. This PR reinforces that. I think we should reconsider this path and consider migrating to tonic.
| runtime.spawn(async move { | ||
| match acceptor.accept(stream).await { | ||
| Ok(tls_stream) => { | ||
| let _handshake_permit = handshake_permit; |
There was a problem hiding this comment.
[P1] Release this permit once the TLS handshake completes. Because _handshake_permit remains in scope across serve_connection(...).await, it is held for the entire HTTP/2 connection. An unauthenticated peer can complete 64 TLS handshakes, keep those connections idle, and cause every subsequent connection to be rejected indefinitely.
| let shutdown_rx = self.shutdown_rx.clone(); | ||
| let (request_parts, request_body) = req.into_parts(); | ||
| let future: Self::Future = Box::pin(async move { | ||
| let body_permit = match REQUEST_BODY_SEMAPHORE.try_acquire() { |
There was a problem hiding this comment.
[P1] Add a mandatory server-side deadline for request-body collection. This global permit is held while read_request_body(...).await waits without a timeout, and only the presence of x-auth has been checked. One unauthenticated client can leave eight HTTP/2 bodies unfinished and force every legitimate RPC to fail with UNAVAILABLE.
There was a problem hiding this comment.
The 30-second timeout bounds each individual stalled body, but does not prevent starvation. An unauthenticated client can continuously open replacement streams as the old ones expire, repeatedly occupying all eight global try_acquire slots and keeping legitimate RPCs at UNAVAILABLE. Could we prevent one unauthenticated peer from monopolizing this pool, and add a regression test for sustained saturation?
Release the handshake permit as soon as the TLS handshake completes. Holding it for the connection's lifetime let an unauthenticated peer complete 64 handshakes, keep the connections idle, and block every later connection indefinitely. This fixup was created with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Apply a server-side deadline to request body collection. The body-read permit was held while the body was awaited without a timeout, so a client that never finished sending could hold one of the eight slots indefinitely and fail every legitimate RPC with UNAVAILABLE. This fixup was created with assistance from Claude Code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
I asked claude to do an analysis on this while it would have fixed some of these, some of these small issues were on our auth schema which still operates outside of it |
Agreed, |
3 participants
Various things found by project loupe. Nothing critical but all worth doing.
This PR was created with assistance from Codex and Claude Code.