pgwire: bound startup and pre-auth frame sizes per call site - #38809
Draft
jubrad wants to merge 1 commit into
Draft
pgwire: bound startup and pre-auth frame sizes per call site#38809jubrad wants to merge 1 commit into
jubrad wants to merge 1 commit into
Conversation
`parse_frame_len` applied a single 64 MiB ceiling, `MAX_FRAME_SIZE`, to three call sites whose real requirements differ by orders of magnitude. The startup path inherited a limit meant for bulk `COPY` traffic, so four unauthenticated bytes on a plaintext socket made `decode_startup` allocate and zero-fill 64 MiB before reading any of the body. The ceiling is now an argument, so each site states the bound it needs and a new caller cannot silently inherit 64 MiB: * `decode_startup` takes `MAX_STARTUP_FRAME_SIZE` (10,000 bytes, matching PostgreSQL's `MAX_STARTUP_PACKET_LENGTH`). environmentd passes `MAX_FORWARDED_STARTUP_FRAME_SIZE`, which adds an allowance for the two parameters balancerd appends in transit. Without it a frame that just fits the client budget would be accepted by balancerd and then rejected behind it. * balancerd's `Codec` takes `MAX_PREAUTH_FRAME_SIZE` (16 KiB). It only ever decodes a credential before the connection is spliced. * pgwire's `Codec` serves one connection across both phases, so the bound starts at `MAX_PREAUTH_FRAME_SIZE` and `allow_post_auth_frames` widens it once authentication succeeds. Closes: CLO-270 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
parse_frame_lenapplies one ceiling,MAX_FRAME_SIZE(64 MiB), to three call sites whose real requirements differ by orders of magnitude. The startup path inherited a limit sized for bulkCOPYtraffic, anddecode_startupsizes its buffer from the declared length before reading any of the body:resizewrites every byte, so the pages are committed rather than lazily mapped. Four bytes from an unauthenticated client, on a plaintext socket before any TLS handshake, commit 64 MiB. That is roughly 16 connections per 1 GiB balancerd pod, on a component every tenant in a region shares.Found by a penetration test.
Linear: CLO-270, a sub-issue of CLO-267.
Solution
The ceiling becomes an argument to
parse_frame_len, so each call site states the bound it actually needs and a caller added later cannot silently inherit 64 MiB.decode_startupMAX_STARTUP_FRAME_SIZE, 10,000 bytesCodecMAX_PREAUTH_FRAME_SIZE, 16 KiBCodecMAX_PREAUTH_FRAME_SIZE, thenMAX_FRAME_SIZEdecode_startuptakes 10,000 bytes, matching PostgreSQL'sMAX_STARTUP_PACKET_LENGTH, so any client that can complete a PostgreSQL handshake can complete this one.environmentd passes
MAX_FORWARDED_STARTUP_FRAME_SIZE, which adds a 128 byte allowance. balancerd appendsmz_connection_uuidandmz_forwarded_forto the parameters before forwarding, up to 119 bytes, so a frame that just fits the client budget arrives downstream larger than the client sent it. Without the allowance such a connection would be accepted by balancerd and then rejected behind it, surfacing as a proxy error with no obvious cause.balancerd's
Codeconly ever decodes one frame, the client's credential, beforecopy_bidirectionalsplices the connection and the remaining bytes are proxied unframed. It never needs the wider bound.pgwire's
Codecserves one connection across both phases, so no single constant can be tight enough for a credential and wide enough for a largeBindparameter. The bound starts tight andFramedConn::allow_post_auth_frameswidens it, called immediately beforeAuthenticationOk.StateMachineis constructed in exactly one place, after that call, so no authenticated path can skip the raise.Testing
New unit tests in
mz-pgwire-common:test_startup_frame_over_budget_is_rejected_before_allocatingasserts an oversized declaration is refused and, crucially, that no buffer was sized first. It fails against the previous bound.test_startup_frame_within_budget_is_acceptedguards against over-tightening.test_forwarded_startup_params_fit_the_allowancebuilds a frame at exactly the client budget, appends the two forwarded parameters at their widest possible values, and asserts it still decodes downstream. It fails if a third forwarded parameter is added without raising the allowance.test_startup_body_wait_has_no_deadlinerecords what this change does not fix, and pins that a stalled connection is now bounded by the startup budget rather thanMAX_FRAME_SIZE.New integration tests in
mz-balancerd:test_pgwire_oversized_startup_frame_is_rejecteddrives a real balancerd listener and asserts an oversized declaration is refused while a frame at the budget is still served, so the rejection is the budget doing its job rather than balancerd refusing everything.test_pgwire_startup_preauth_connections_are_heldrecords the remaining gap.Gotchas for the reviewer
decode_startupstill pre-allocates. At 10,000 bytes the memset is irrelevant, so this keeps the diff small. Reading incrementally removes the remaining amplification but produces no ceiling on its own, so it is follow-up polish rather than part of the fix.src/pgwire/src/codec.rsstill has no aggregateMAX_REQUEST_SIZEguard, unlike balancerd's. The comment onenvironmentd/tests/server.rssays that was removed deliberately so large parameters work, and it is unchanged here.Release notes
This release will reject pgwire startup messages larger than 10,000 bytes, matching PostgreSQL's limit, and hold pre-authentication messages to 16 KiB.
🤖 Generated with Claude Code