Repository navigation
Harden gRPC transport - #266
benthecarman wants to merge 8 commits into
Conversation
|
👋 Thanks for assigning @joostjager as a reviewer! |
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?
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, |
4896da4 to
a0114ed
Compare
bb62b1d to
39459b6
Compare
| Ok((stream, _)) => { | ||
| Ok((stream, peer_addr)) => { | ||
| let handshake_permit = | ||
| match Arc::clone(&tls_handshake_semaphore).try_acquire_owned() { |
There was a problem hiding this comment.
The 64-slot pending-handshake semaphore is shared by all peers, and each slot can be held for up to TLS_HANDSHAKE_TIMEOUT (10 s) by a connection that never sends a ClientHello. One unauthenticated host can keep re-opening such connections as they expire and hold all 64 slots, so new legitimate connections hit the continue at line 740 and are dropped (existing connections keep working, but a new CLI run or a reconnect mostly fails).
This is the same starvation pattern as the body-read pool discussed above, just on the handshake side. Since peer_addr.ip() is already available here, could we also cap pending handshakes per IP (say 4–8), and maybe shorten the timeout to a few seconds? A TLS handshake on a healthy link takes milliseconds.
| Self { active: Mutex::new(BTreeMap::new()) } | ||
| } | ||
|
|
||
| fn try_acquire(&self, peer_ip: IpAddr) -> Result<RequestBodyPermit<'_>, GrpcStatus> { |
There was a problem hiding this comment.
Since the permit is only taken after authenticate_request and the permission check (see read_authorized_request), only holders of a valid macaroon can reach this limiter, so the macaroon id looks like the better key than the IP:
- Behind a reverse proxy, Tor or Docker NAT, all clients share one IP, so the whole deployment gets
MAX_BODY_READS_PER_PEER(2) concurrent body reads and the rest getUNAVAILABLE"Too many concurrent requests". - An attacker holding one low-privilege token can rotate source addresses (an IPv6 /64 gives plenty), and 4 addresses are enough to fill the 8 global slots.
- A leaked read-only token can starve the operator's admin token, because nothing is reserved per identity.
PendingMacaroonRequest::info already carries the macaroon id, so the limiter could take that instead of (or in addition to) the IP, keeping the global cap of 8 as a backstop. The existing saturation test would then also cover "one token can't starve another".
| let io_stream = TokioIo::new(tls_stream); | ||
| if let Err(err) = http2::Builder::new(TokioExecutor::new()).serve_connection(io_stream, node_service).await { | ||
| let mut builder = http2::Builder::new(TokioExecutor::new()); | ||
| builder.max_concurrent_streams(MAX_CONCURRENT_HTTP2_STREAMS); |
There was a problem hiding this comment.
The semaphore slot is released once the handshake succeeds (nice), but nothing else bounds the connections that follow: there is no connection cap, no idle timeout, and no HTTP/2 keepalive (http2::Builder has no .timer() / keep_alive_* configured). An unauthenticated client can complete handshakes quickly and keep thousands of idle connections open until the process runs out of file descriptors, which also affects the Lightning side of the node.
Could we add an overall connection limit plus an idle/keepalive timeout (and maybe max_header_list_size)? If you'd rather keep that for a follow-up, it would help to say so in the description, since the title and the "Bound pending TLS handshakes" commit suggest connection exhaustion is covered.
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.
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.
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>
Limit concurrent body reads per source IP across connections so one peer cannot occupy the global pool with replacement stalled requests. Keep the global limit and body deadline, and release peer records when reads finish, fail, or are cancelled. Test repeated saturation while another peer reads and authenticates a request, plus global capacity, address normalization, and cleanup. This change was made with assistance from Codex.
parse_grpc_timeout splits the value one byte before the end to separate the number from its unit. A multi-byte final character makes that index land inside a codepoint, and str::split_at panics. The server only calls it after HeaderValue::to_str, which rejects non-ASCII, but the function is public API, so reject non-ASCII input up front. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
39459b6 to
8c7880f
Compare
Various things found by project loupe. Nothing critical but all worth doing.
This PR was created with assistance from Codex and Claude Code.