Skip to content

Commit 55d17c0

Browse files
alukachclaude
andauthored
fix(proxy): stream-resign aws-chunked uploads instead of decoding (#92)
* fix(proxy): stream-resign aws-chunked uploads instead of decoding Modern AWS clients (aws-cli >= 2.23, recent SDKs) frame PutObject and UploadPart bodies as `Content-Encoding: aws-chunked` with a streaming sentinel (`x-amz-content-sha256: STREAMING-...`). The proxy forwarded them untouched — PutObject via a presigned URL (UNSIGNED-PAYLOAD), UploadPart via a re-signed raw request — and S3 only de-chunks a request signed with a matching streaming sentinel, so it stored the raw chunk envelope (`7\r\nhello!\n\r\n0\r\n<trailer>`) as the object. Every aws-chunked write was corrupted. For the common unsigned-payload variant (`STREAMING-UNSIGNED-PAYLOAD-TRAILER`, the client default), re-sign only the request seed with the backend credentials — reusing the client's streaming sentinel and the de-chunk headers (content-encoding, x-amz-decoded-content-length, x-amz-trailer) — and stream the chunk framing straight through for S3 to de-chunk. Zero-copy: the worker never buffers or hashes the payload, which matters on the Cloudflare Workers memory ceiling. PutObject and UploadPart share this path. Signed-chunk uploads (`STREAMING-AWS4-HMAC-SHA256-PAYLOAD`) carry per-chunk signatures bound to the client's key that can't be re-signed to the backend credentials, so they are rejected with NotImplemented (501) rather than silently corrupted. Content-Length is forwarded but left unsigned: the runtime owns the streaming transfer framing, and S3 sizes the payload from x-amz-decoded-content-length and the chunk framing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(proxy): reject aws-chunked streaming uploads on non-S3 backends Review follow-up to the stream-resign change. Three fixes from a 4-agent review: - Bug: the PutObject streaming arm reached `build_streaming_forward` (which hardcodes S3 seed signing) without a backend-type guard, so a default aws-chunked PUT to a non-S3 backend (azure) mis-routed into S3 signing instead of rejecting — unlike UploadPart, which guards earlier. Guard at the point the S3 assumption is made; reject with InvalidRequest. - Drop the unreachable `Internal` re-read of x-amz-content-sha256: the classifier already validated it, so it now returns the sentinel for the caller to thread through as the payload hash. - Strengthen the UploadPart test to assert partNumber/uploadId survive into the forwarded URL, and add a non-S3 streaming-rejection regression test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(proxy): trim aws-chunked over-engineering leftovers Ponytail over-engineering pass on the aws-chunked streaming work: - Delete the unused `STREAMING_PAYLOAD` const — zero references repo-wide; the new `aws_chunked` module classifies the sentinel by substring, so the literal is dead. - Consolidate the non-S3 backend gate into `try_streaming_forward` so `build_streaming_forward` becomes a pure constructor. Behavior unchanged: PutObject streaming to a non-S3 backend still rejects 400, covered by `streaming_put_on_non_s3_backend_is_rejected`. - Drop two redundant `aws_chunked` unit assertions — the signed `-TRAILER` variant and the hex-hash case each re-hit a branch already covered. No behavior change. cargo fmt / clippy -D warnings / test (111 passing) / wasm32 check all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(proxy): forward flexible-checksum headers on multipart ops (#93) * fix(proxy): forward flexible-checksum headers on multipart ops Modern AWS SDKs/CLI (botocore / CLI v2 >= ~2.23) enable CRC32 data-integrity checksums by default. For a multipart upload that means: - CreateMultipartUpload sends `x-amz-checksum-algorithm` (declares the MPU's checksum algorithm), and - CompleteMultipartUpload echoes the per-part / full-object checksums via `x-amz-checksum-type` / `x-amz-checksum-crc32` plus the per-part values in the XML body. `execute_multipart` only forwarded `content-type`/`content-length`/`content-md5` and dropped every `x-amz-checksum-*` header. So the MPU was initialized with no checksum context while parts were stored *with* CRC32 checksums (the streaming re-sign path forwards the `x-amz-trailer`), and S3 rejected CompleteMultipartUpload with `InvalidPart`: An error occurred (InvalidPart) when calling the CompleteMultipartUpload operation: One or more of the specified parts could not be found... Forward the client's `x-amz-checksum-*` and `x-amz-sdk-checksum-algorithm` headers on this raw-signed path. It signs every header present (`sign_s3_request`), so they are covered by the signature — unlike the presigned PutObject path, where unsigned `x-amz-*` headers are rejected. This also fixes a plain (non-chunked) UploadPart carrying a header checksum, which routes through the same function. Workaround for clients until deployed: set `AWS_REQUEST_CHECKSUM_CALCULATION=when_required`. Adds a functional regression test driving `handle_with_body` with a CompleteMultipartUpload and a header-capturing backend, asserting the checksum headers both reach the backend and appear in the re-signed SignedHeaders. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(smoke): multipart upload regression test against real S3 The dropped-checksum bug (InvalidPart on CompleteMultipartUpload) only reproduces against real AWS S3. Verified empirically that MinIO RELEASE.2025-09 does NOT reproduce it: an MPU created without a checksum algorithm but with CRC32 parts + completion succeeds on MinIO and is rejected by S3. So the existing MinIO integration multipart tests can't guard this — a real-S3 smoke test is required. Add `TestMultipartUpload` to the smoke suite: a checksum-forced (`ChecksumAlgorithm=CRC32`) two-part upload via boto3's transfer manager (mirroring `aws s3 cp`), asserting the round-trip. It is gated on `SMOKE_WRITE_BUCKET`, so it self-skips until a writable bucket is configured. Wire a `smoke-writable` bucket + a github-actions write scope into wrangler.deploy.toml (OIDC-federated, no long-lived keys). Activation needs the backend bucket created, the OIDC role granted S3 write perms, and SMOKE_WRITE_BUCKET=smoke-writable set in the smoke job. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(smoke): reuse multistore-test-bucket for the write smoke test Point the smoke-writable bucket at the existing federated-test backend bucket (multistore-test-bucket) under a smoke-writable/ prefix instead of a separate bucket. The role already has PutObject/GetObject/DeleteObject/ListBucket (covering Create/Upload/Complete); only s3:AbortMultipartUpload need be added so failed uploads clean up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: pass SMOKE_WRITE_BUCKET to the smoke-test job Wires the multipart write smoke test (TestMultipartUpload) to run on every deploy that exercises the smoke suite — including per-PR preview deploys, not just main. Gated behind the SMOKE_WRITE_BUCKET repo/environment variable: unset (default) -> the test self-skips; set to smoke-writable -> it runs against the freshly-deployed worker and real S3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * ci: run the multipart write smoke test on every preview Drop the opt-in repo-variable gate; instead pass smoke_write_bucket as a deploy.yml input and have preview.yml set it to smoke-writable unconditionally. Every PR preview now exercises the real-S3 multipart upload (no main deploy needed); staging/production leave the input empty, so the test self-skips there unless they opt in the same way. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent b0f5b7f commit 55d17c0

8 files changed

Lines changed: 651 additions & 34 deletions

File tree

.github/workflows/deploy.yml

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,11 @@ on:
2020
required: false
2121
type: string
2222
default: ""
23+
smoke_write_bucket:
24+
description: "Virtual bucket for the multipart write smoke test; empty skips it"
25+
required: false
26+
type: string
27+
default: ""
2328
set_secrets:
2429
description: "Whether to set worker secrets after deploy"
2530
required: false
@@ -112,6 +117,10 @@ jobs:
112117
# here on both preview (PR) and staging deploys, against the stable
113118
# staging OIDC issuer — see preview.yml / staging.yml.
114119
FEDERATION_TEST_KEY: ${{ vars.FEDERATION_TEST_KEY }}
120+
# Virtual bucket the multipart write smoke test uploads to
121+
# (TestMultipartUpload). Set per-caller via the `smoke_write_bucket` input
122+
# (preview.yml passes `smoke-writable`); empty -> the test self-skips.
123+
SMOKE_WRITE_BUCKET: ${{ inputs.smoke_write_bucket }}
115124
steps:
116125
- uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
117126
- uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0

.github/workflows/preview.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ jobs:
2121
worker_name: multistore-proxy-pr-${{ github.event.pull_request.number }}
2222
wrangler_config: wrangler.deploy.toml
2323
environment: preview
24+
# Run the real-S3 multipart write smoke test on every preview.
25+
smoke_write_bucket: smoke-writable
2426
# Mint tokens with the STABLE staging issuer rather than this PR's own URL.
2527
# All deployments share OIDC_PROVIDER_KEY, so a preview can sign assertions
2628
# whose `iss` is the staging URL; AWS then validates them against the

crates/core/src/aws_chunked.rs

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
//! Detection of AWS SigV4 streaming ("aws-chunked") uploads.
2+
//!
3+
//! Modern AWS clients (aws-cli ≥ 2.23, recent SDKs) send `PutObject`/`UploadPart`
4+
//! bodies as `Content-Encoding: aws-chunked` framing with an
5+
//! `x-amz-content-sha256: STREAMING-…` sentinel rather than a plain payload. S3
6+
//! only de-chunks that framing for a request signed with the matching streaming
7+
//! sentinel, so the proxy must re-sign the request seed (not presign) and stream
8+
//! the framing through untouched — see `ProxyGateway::build_streaming_forward`.
9+
10+
use http::HeaderMap;
11+
12+
/// How a streaming ("aws-chunked") upload's chunks are authenticated — which
13+
/// decides whether the proxy can forward the framing after re-signing.
14+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
15+
pub(crate) enum StreamingUpload {
16+
/// `STREAMING-UNSIGNED-PAYLOAD[-TRAILER]` — chunks are framed but not
17+
/// individually signed. The proxy can re-sign the request seed with the
18+
/// backend credentials and stream the framing through for S3 to de-chunk.
19+
Unsigned,
20+
/// `STREAMING-AWS4-HMAC-SHA256-PAYLOAD[-TRAILER]` — each chunk carries a
21+
/// signature chained from the client's signing key, which cannot survive the
22+
/// proxy re-signing the request to the backend with different credentials.
23+
/// The proxy cannot forward these.
24+
Signed,
25+
}
26+
27+
/// Classify a request body from its `x-amz-content-sha256`, returning the
28+
/// upload kind together with the sentinel value (so the caller can reuse it as
29+
/// the seed's payload hash without re-reading it). `None` for an ordinary
30+
/// (non-streaming) upload.
31+
pub(crate) fn streaming_upload(headers: &HeaderMap) -> Option<(StreamingUpload, &str)> {
32+
let sentinel = headers.get("x-amz-content-sha256")?.to_str().ok()?;
33+
let variant = sentinel.strip_prefix("STREAMING-")?;
34+
let kind = if variant.contains("UNSIGNED") {
35+
StreamingUpload::Unsigned
36+
} else {
37+
StreamingUpload::Signed
38+
};
39+
Some((kind, sentinel))
40+
}
41+
42+
#[cfg(test)]
43+
mod tests {
44+
use super::*;
45+
46+
fn headers(content_sha256: &str) -> HeaderMap {
47+
let mut h = HeaderMap::new();
48+
if !content_sha256.is_empty() {
49+
h.insert("x-amz-content-sha256", content_sha256.parse().unwrap());
50+
}
51+
h
52+
}
53+
54+
#[test]
55+
fn unsigned_streaming_is_unsigned() {
56+
// The aws-cli/SDK default (CRC64NVME trailer), and the no-trailer form.
57+
// The sentinel is returned verbatim (the `-TRAILER` suffix matters).
58+
assert_eq!(
59+
streaming_upload(&headers("STREAMING-UNSIGNED-PAYLOAD-TRAILER")),
60+
Some((
61+
StreamingUpload::Unsigned,
62+
"STREAMING-UNSIGNED-PAYLOAD-TRAILER"
63+
))
64+
);
65+
assert_eq!(
66+
streaming_upload(&headers("STREAMING-UNSIGNED-PAYLOAD")),
67+
Some((StreamingUpload::Unsigned, "STREAMING-UNSIGNED-PAYLOAD"))
68+
);
69+
}
70+
71+
#[test]
72+
fn signed_streaming_is_signed() {
73+
assert_eq!(
74+
streaming_upload(&headers("STREAMING-AWS4-HMAC-SHA256-PAYLOAD")),
75+
Some((
76+
StreamingUpload::Signed,
77+
"STREAMING-AWS4-HMAC-SHA256-PAYLOAD"
78+
))
79+
);
80+
}
81+
82+
#[test]
83+
fn non_streaming_is_none() {
84+
// Anything without the `STREAMING-` prefix (a plain hash, `UNSIGNED-PAYLOAD`,
85+
// or a missing header) is not a streaming upload.
86+
assert_eq!(streaming_upload(&headers("UNSIGNED-PAYLOAD")), None);
87+
assert_eq!(streaming_upload(&headers("")), None);
88+
}
89+
}

crates/core/src/backend/request_signer.rs

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,3 @@ pub fn hash_payload(payload: &[u8]) -> String {
152152

153153
/// The SigV4 sentinel for unsigned payloads (used with streaming uploads).
154154
pub const UNSIGNED_PAYLOAD: &str = "UNSIGNED-PAYLOAD";
155-
156-
/// The SigV4 sentinel for streaming payloads.
157-
pub const STREAMING_PAYLOAD: &str = "STREAMING-AWS4-HMAC-SHA256-PAYLOAD";

crates/core/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
2424
pub mod api;
2525
pub mod auth;
26+
pub(crate) mod aws_chunked;
2627
pub mod backend;
2728
pub mod error;
2829
pub mod maybe_send;

0 commit comments

Comments
 (0)