The WebSocket path has no connect timeout, no idle timeout and no connection cap #62
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
From the v2.0.0 security audit.
proxy_websocketopens the backend connection withTcpStream::connectand no connect timeout, thencopy_bidirectionalruns with no idle timeout.proxy_timeout_secondsis not consulted anywhere on this path and there is nomax_connections.Each upgrade therefore pins a backend socket plus two tokio tasks for as long as the client stays silent. Compare the other two paths: the HTTP path has send/read idle watchdogs and a body cap, and L4 has
max_connections. The WebSocket path has neither.N upgrade requests that then go quiet exhaust the backend's connection pool, with nothing on the ER side to shed them.
The header-read timeout added in
56690efdoes not help: it bounds only the time to receive the request head, and these connections complete their head and upgrade normally.Fix
A connect timeout on the backend dial, an idle timeout on the tunnel (a WebSocket that is legitimately idle for a long time is normal, so this wants to be generous and probably configurable rather than reusing
proxy_timeout_seconds), and a cap on concurrent upgrades per service or globally.Worth deciding whether the cap belongs with L4's
max_connectionsas one shared concurrency setting rather than a third mechanism.Explicitly deferred out of v2.1. Recording the reason so this is a decision rather than an omission.
Every other audit finding went into the v2.1 milestone (#53, #55, #58, #59, #60, #61, #63, #64, #65, plus #67). This one is out because it is the only one that needs a design decision rather than a fix:
proxy_timeout_seconds" — a legitimately idle socket is normal, so the value and whether it is configurable per service are real choices.max_connections, and adding a third independent mechanism alongside the HTTP body cap and L4's cap is the wrong shape. That wants a single concurrency model across the three paths, which is a larger piece of work than v2.1 should carry.So it is deferred pending that decision, not because the exposure is acceptable. The exposure as it stands: N upgrade requests that then go quiet pin a backend socket and two tokio tasks each, with nothing on the ER side to shed them, and the 30s header-read timeout added in
56690efdoes not help because these connections complete their head normally.Mitigation available today: a backend with its own connection limits and idle handling absorbs this, which is the usual case. A deployment fronting a WebSocket service with no such limits is the one at risk.
Also noted while triaging: the pre-audit backlog (#8, #11, #16, #21, #24, #26, #29, #36, #37, #39, #46, #47, #48, #49, #50, #51) is untouched and remains unmilestoned — not deferred by this triage, simply not part of the audit's output.