Skip to content

fix: verify signed streaming upload chunks - #243

Open
perfloop[bot] wants to merge 9 commits into
mainfrom
perfloop-pr-open-enz90kdnhj
Open

perfloop[bot] wants to merge 9 commits into
mainfrom
perfloop-pr-open-enz90kdnhj

Conversation

@perfloop

@perfloop perfloop Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Signing-mode forwarding now verifies a client's standard AWS streaming-payload signatures before TAG removes the chunk framing and re-signs the decoded body. TAG checks each request-seeded data-chunk HMAC and the terminal HMAC, checks the declared decoded length, and does not dispatch upstream until the full stream is authenticated.

Forward, ForwardWithCapture, and ForwardTeeingBody share this path. The outgoing request contains the decoded bytes and is signed with UNSIGNED-PAYLOAD; the decoded-length header removed before transport is excluded from the new signature. Invalid or missing chunk signatures, including an invalid zero-byte terminal signature, fail with a signature mismatch before upstream dispatch. The integrity test also checks byte-exact forwarding for valid streams.

Staging uses a temporary file and a process-local reservation pool. It reserves the declared length before reading when known, or each next chunk otherwise, and caps active staging at half the temporary-filesystem space available when the first stage is admitted. The shared pool lock no longer covers file writes: each stage serializes its own write and cleanup. A blocked-write test verifies that another in-budget upload can reserve, write, and release while the first writer is blocked; an over-budget reservation remains rejected. Partial-write accounting is tested. Cancellation or incomplete HTTP/1 ingress interrupts the read, closes the connection instead of reusing it, and removes the staged file and reservation. Temporary storage proportional to the body is still required, and other filesystem users are outside the process-local accounting.

The guarantee applies only to header-authenticated STREAMING-AWS4-HMAC-SHA256-PAYLOAD bodies routed through signing-mode upstream forwarding. Unsigned-trailer streams, presigned URLs, and tiered local-tier PUTs are outside it; non-streaming and other markers retain their existing handling. The README, security guide, and S3 compatibility guide document these limits. No public API or configuration changes.

The required assertion was violated at f79710f81ce45df9799757e1c538853794c25031 and is satisfied on the final source. The reproducible claim command is bash -c "assertion='For signing-mode NewForwarder requests using STREAMING-AWS4-HMAC-SHA256-PAYLOAD with a known credential, a current valid header-based SigV4 signature, well-formed chunk framing, matching decoded length, no independent checksum header, and sufficient temporary-filesystem capacity to stage the body, Forward, ForwardWithCapture, and ForwardTeeingBody must deliver exactly the decoded bytes only after every request-seeded data and terminal HMAC verifies; an altered or missing data or terminal HMAC, including an invalid zero-byte terminal HMAC, must return ErrCodeSignatureMismatch before any upstream request is dispatched.'; log=$(mktemp \"${TMPDIR:-/tmp}/perfloop-signed-stream.XXXXXX\") || { printf '{\"assertion\":\"%s\",\"outcome\":\"inconclusive\"}\\n' \"$assertion\"; exit 0; }; go test -v ./proxy -run '^TestSigningForwarderSignedStreamIntegrity$' -count=1 >\"$log\" 2>&1; status=$?; cat \"$log\" >&2; property_failure=0; for expected in 'valid signed stream was rejected as an authentication failure' 'valid stream was not dispatched successfully' 'valid stream was not re-signed as the exact decoded payload' 'invalid signed stream was not rejected as a signature mismatch before upstream dispatch'; do if grep -Fq -- \"$expected\" \"$log\"; then property_failure=1; break; fi; done; if [ \"$status\" -eq 0 ] && grep -Fq -- '--- PASS: TestSigningForwarderSignedStreamIntegrity' \"$log\"; then outcome=satisfied; elif [ \"$status\" -ne 0 ] && grep -Fq -- '--- FAIL: TestSigningForwarderSignedStreamIntegrity' \"$log\" && [ \"$property_failure\" -eq 1 ]; then outcome=violated; else outcome=inconclusive; fi; rm -f \"$log\"; printf '{\"assertion\":\"%s\",\"outcome\":\"%s\"}\\n' \"$assertion\" \"$outcome\"; exit 0". The recorded checks were go test ./proxy -count=1, go test -race ./proxy -count=1, make build, make lint-ci, and make test.

Target CI also runs Test / test's make test-integration, make test-race, and make test-coverage, S3 Compatibility Tests (s3-compat-tests (default) and s3-compat-tests (tiered)), S3 Compatibility Tests (Cluster) (s3-compat-tests-cluster (default)), and Lint PR Title / lint.


Generated by Perfloop. Measurements and checks.


Note

High Risk
Changes authentication and upload handling on the signing forwarder path: chunk verification, temp-disk staging, and dispatch timing affect security-sensitive PUT flows and resource use under concurrent large uploads.

Overview
Signing-mode upstream forwarding no longer strips STREAMING-AWS4-HMAC-SHA256-PAYLOAD chunk framing without checking per-chunk SigV4. For header-authenticated (non-presigned) streaming PUTs, TAG now validates the request header first, verifies each data-chunk HMAC and the terminal HMAC, checks X-Amz-Decoded-Content-Length when present, stages the decoded bytes in a temp file, and only then re-signs and forwards as UNSIGNED-PAYLOAD. Invalid or tampered streams return signature mismatch before any upstream dispatch.

Forward, ForwardWithCapture, and ForwardTeeingBody share decodeIncomingBody; outgoing signatures omit stripped chunked headers via headersForForwardedSigning. A process-local staging budget reserves up to half of available temp-filesystem space (per chunk when length is unknown), holds reservations until the staged file is removed, and rejects over-capacity uploads early. Failed or canceled HTTP/1 ingress sets Connection: close and uses ResponseController read deadlines (Unwrap on responseTracker / statusRecorder) so incomplete bodies do not block keep-alive.

Presigned URLs, unsigned-trailer streams, tiered local-cache PUTs, and transparent proxy mode are unchanged; docs in README, security.md, and s3-compatibility.md describe the new guarantee and limits. Large test coverage exercises integrity, capacity, cleanup, and real HTTP/1 cancel paths.

Reviewed by Cursor Bugbot for commit a1ccd91. Bugbot is set up for automated code reviews on this repo. Configure here.

Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
@greptile-apps

greptile-apps Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Critical risk] Adds signature verification for streaming uploads in signing mode.

The PR appears safe to merge; the remaining issue is a non-blocking test robustness gap.

Fix All in Claude CodeFindings

  1. P2 Blocked-write test can hang ▶

Summary

The PR verifies signed AWS streaming chunks and their terminal signature before signing and forwarding decoded bytes. It also adds temporary-file capacity accounting and cancellation cleanup.

  • The latest change moves file writes outside the shared budget lock and adds concurrency and partial-write tests.
  • The blocked-write test has a timeout gap that can make a regression stall the suite.

Reviews (3) · Last reviewed commit: "fix: avoid serializing signed stream sta..."

Comment thread proxy/signed_chunked_body.go Outdated
Comment thread proxy/signed_chunked_body.go Outdated
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Comment thread proxy/signed_chunked_body.go
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
}

overBudget := &signedStreamStageReservation{budget: budget, dir: dir}
if err := overBudget.reservePayload(11); !errors.Is(err, errSignedStreamStagingCapacity) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Blocked-write test can hang If write starts holding the shared budget lock during file I/O again, this synchronous reservation blocks while the first writer waits to be unblocked. The test never reaches its timed check or the call that unblocks the writer, so it hangs until the suite timeout instead of reporting the regression promptly. Run this reservation with a timeout or check it after unblocking the writer.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The test can hang if the reservation blocks while the writer is held, preventing the test from reaching the unblock. I’ll make the reservation non-blocking to the test’s control flow and ensure the writer is unblocked before asserting a timeout or failure.

Source feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant