Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Extends S3 Gateway SigV4 streaming verification to include the HMAC-SHA256 trailer signature for STREAMING-AWS4-HMAC-SHA256-PAYLOAD-TRAILER, validating trailer framing and rejecting tampered trailers before committing data.
Changes:
- Add trailer signature verification to the signature chain (
ChunksValidator.validateTrailer) and enforce trailer presence for the-TRAILERalgorithm. - Update
SignedChunksInputStreamto parse/validate trailing checksum header +x-amz-trailer-signature, including final terminator and EOF. - Add unit and endpoint tests for valid/tampered trailers across PUT and multipart uploads.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/ChunksValidator.java | Adds trailer signature verification and refactors shared signature validation. |
| hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java | Enables verification for the -TRAILER algorithm and requires x-amz-trailer. |
| hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/SignedChunksInputStream.java | Implements trailer parsing/framing validation and computes trailer header hash for verification. |
| hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/util/S3Consts.java | Introduces X_AMZ_TRAILER header constant. |
| hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/signature/TestChunksValidator.java | Adds direct validator tests for trailer signature acceptance/rejection. |
| hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/signature/SignatureTestUtils.java | Adds helpers to generate streaming bodies with trailers and compute trailer signatures. |
| hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestObjectPut.java | Adds endpoint coverage for valid/tampered/malformed trailer bodies and selection logic. |
| hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestUploadWithStream.java | Adds PUT-with-stream coverage for trailer variant success/failure. |
| hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPartUpload.java | Adds multipart part upload coverage for trailer variant success/failure. |
| hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/TestSignedChunksInputStream.java | Adds parsing/validation tests for trailer signature, terminator, and invalid header declarations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // The final zero-byte chunk has no payload terminator when trailing headers follow it. | ||
| if (validator != null && trailerHeader == null) { | ||
| readChunkTerminator(); | ||
| } | ||
| validateChunk(); | ||
| if (trailerHeader != null) { | ||
| validateTrailer(); | ||
| } |
| private void validateSignature(String signature, String stringToSign) { | ||
| byte[] expected = hmacSha256(stringToSign); | ||
| // Constant-time comparison to avoid leaking the signature via timing. Decoding the hex also | ||
| // makes the comparison case-insensitive, as the signature may be sent in either case. | ||
| if (!MessageDigest.isEqual(expected, DatatypeConverter.parseHexBinary(chunkSignature))) { | ||
| if (!MessageDigest.isEqual(expected, DatatypeConverter.parseHexBinary(signature))) { | ||
| throw newError(SIGNATURE_DOES_NOT_MATCH, resource); | ||
| } |
There was a problem hiding this comment.
Thanks for working on this! @rich7420 Overall, this looks good.
I noticed that the STS acceptance job failed, and it looks like this change caused it. Could you take a look?
I also left a few small notes inline.
Gargi-jais11
left a comment
Gargi-jais11
left a comment
There was a problem hiding this comment.
Thanks @rich7420 for the PR.
Please find few comments to fix.
| this.trailerHeader = trailerHeader == null ? null : trailerHeader.trim().toLowerCase(Locale.ROOT); | ||
| if (this.trailerHeader != null && !CHECKSUM_TRAILER_PATTERN.matcher(this.trailerHeader).matches()) { | ||
| throw invalidBody("Invalid x-amz-trailer header"); |
There was a problem hiding this comment.
AWS allows x-amz-trailer to list multiple trailing header names (comma-separated). We only accept a single x-amz-checksum-* here via CHECKSUM_TRAILER_PATTERN. Please either document “single trailer only” or split x-amz-trailer, read each trailer line in validateTrailer(), and hash canonical trailing headers
| // The final zero-byte chunk has no payload terminator when trailing headers follow it. | ||
| if (validator != null && trailerHeader == null) { | ||
| readChunkTerminator(); | ||
| } | ||
| validateChunk(); | ||
| if (trailerHeader != null) { | ||
| validateTrailer(); | ||
| } |
What changes were proposed in this pull request?
S3 Gateway currently accepts
STREAMING-AWS4-HMAC-SHA256-PAYLOAD-TRAILERuploads without verifying their signatures. Extend the existing chunk verification to authenticate the final zero-byte chunk and checksum trailer before committing the key or multipart part.Validate the declared checksum header and trailer framing, including the final CRLF. This verifies the trailer signature; checksum calculation and storage are unchanged.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15142
How was this patch tested?
https://github.com/rich7420/ozone/actions/runs/34317896799