feat: forward copy-source SSE-C headers on CopyObject#123
Draft
alukach wants to merge 5 commits into
Draft
Conversation
CopyObject was rejected outright (501) since 0.6.0 because the presigned forward path drops the copy-source header and would overwrite the destination with an empty body. Delegate the copy to the backend S3 instead: a signed PUT to the destination carries x-amz-copy-source pointing at the source's backend bucket/key, via the same raw-signed send_raw path batch delete uses. Native S3 copy needs one endpoint that can read source and write destination, so this covers same-store copies (rename, metadata edit, cross-bucket within one account); cross-store and UploadPartCopy are rejected with a clear 501. - types.rs: add S3Operation::CopyObject (dest bucket/key + parsed source bucket/key/version); PUT method, PutObject action (dest write). Source read is authorized separately at dispatch. - api/request.rs: parse a copy-source PUT into CopyObject; parse_copy_source handles the `/bucket/key?versionId=` wire format and percent-decodes the key. UploadPartCopy (copy-source + uploadId) still 501s. - proxy.rs: execute_copy resolves+authorizes the source as a read, guards same_backing_store (same endpoint/region/creds), builds the backend copy-source header (build_copy_source_header re-encodes into the source key space), signs and sends the PUT, and passes the CopyObjectResult XML straight through. Copy-relevant client headers are forwarded and signed. - route_handler.rs: box HandlerAction::NeedsBody — the wider S3Operation pushed the enum past clippy's large-variant threshold. - docs: operations.md/errors.md now describe same-store copy, not a blanket rejection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An empty `versionId` (`bucket/key?versionId=`) failed the `!v.is_empty()` match guard and fell through to the fallback arm, which used the untrimmed string — leaving `?versionId=` glued onto the source key and silently copying from a non-existent key instead of erroring. Always split on the path; treat an empty version as `None`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add three tests driving the full copy path through resolve_request against the CaptureHeadersBackend, mirroring the multipart integration test: - copy_object_end_to_end_sends_resigned_backend_put: parse → authorize dest+source → same-store check → build backend copy-source → re-sign → send_raw, asserting the exact wire request (rewritten/encoded copy-source, forwarded copy-relevant headers, empty-payload hash, copy headers present in SignedHeaders). - copy_object_forwards_version_id_to_backend: versionId rides through. - cross_store_copy_is_rejected_501: different backends → 501, backend never contacted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The `test_copy_object_rejected` test asserted the pre-implementation behavior (all copies → 501). Same-store copy now works, so replace it with two tests exercised live against MinIO through the Workers gateway: - test_copy_object_same_bucket: copy within one bucket returns 200 and the destination has the source bytes. - test_copy_object_cross_bucket_same_store: copy across two virtual buckets on the same backing store (public-data → private-uploads) is a native same-store copy and succeeds. Cross-store rejection stays covered by the Rust unit test (cross_store_copy_is_rejected_501); the integration env has a single backend. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
To read an SSE-C-encrypted source, a CopyObject must carry the x-amz-copy-source-server-side-encryption-customer-* headers so the backend can decrypt it. These start with x-amz-copy-source-..., so they slipped past the allowlist's x-amz-server-side-encryption* prefix (which only matches the destination SSE headers) and were dropped before signing — an SSE-C source would 400 against real S3. - proxy.rs: add x-amz-copy-source-server-side-encryption prefix to is_copy_forward_header; label both SSE prefixes (dest vs source). - proxy.rs: extend copy_object_end_to_end_sends_resigned_backend_put to send a source SSE-C header and assert it reaches the backend and is signed (fails without the fix). - test_smoke.py: TestCopyObjectSseCSource — real-S3 regression, gated by SMOKE_WRITE_BUCKET. MinIO refuses SSE-C over the Docker loop's plaintext HTTP, so this can only run against real S3. - docs: note both SSE prefixes in the forwarded copy-header list. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @alukach's task in 1m 18s —— View job Review checklist
✅ No blocking issues — safe to merge. The fix is a correct, minimal one-liner: Minor non-blocking notes:
No security issues: forwarding the client-supplied SSE-C customer key/algorithm to the backend is required for S3 SSE-C semantics (the key must reach S3 on every request touching that object), and this only forwards headers the client itself already sent — it doesn't originate or cache key material. Simplify (ponytail): nothing to cut — this is already the smallest fix (one 💰 Estimated review cost: $0.49 · 1m18s · 16 turns |
|
📖 Docs preview deployed to https://multistore-docs-pr-123.development-seed.workers.dev
|
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
1 participant
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.
What I'm changing
Same-store CopyObject (#121) forwards a client-supplied allowlist of copy-relevant headers onto the backend PUT. The allowlist matches destination SSE headers via the `x-amz-server-side-encryption*` prefix — but the headers that decrypt an SSE-C-encrypted source are named `x-amz-copy-source-server-side-encryption-customer-*`. Those start with `x-amz-copy-source-...`, so they miss that prefix and every other match arm, and get dropped before the request is signed. Copying an SSE-C source therefore 400s against real S3 (the backend can't decrypt the source without the customer key).
Surfaced as a non-blocking review observation on #121. Not a regression — the pre-#121 code rejected all copies — but worth fixing before SSE-C sources come up in anger.
How I did it
Test plan
🤖 Generated with Claude Code