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.
| throw invalidBody("Invalid trailing signature"); | ||
| } | ||
| if (validator != null) { | ||
| validator.validateTrailer(matcher.group(1), sha256Hex(name + ":" + value + "\n")); |
There was a problem hiding this comment.
nit: Could we reuse DigestUtils.sha256Hex here? Then the local sha256Hex helper can go.
| ChunksValidator validator = newTrailerValidator(); | ||
| assertSignatureMismatch(() -> validator.validateTrailer( | ||
| TRAILER_SIGNATURE.substring(0, TRAILER_SIGNATURE.length() - 1) + "0", | ||
| "invalid-trailer-hash")); |
There was a problem hiding this comment.
I wonder if this test could change only the signature. Right now, the chain has not been advanced, and the hash is fake too, so a broken signature check might still pass.
| + "1234567890\r\n" | ||
| + "05;chunk-signature=" + FAKE_SIGNATURE + "\r\n" | ||
| + "abcde\r\n"; | ||
| void testPutObjectWithValidSignedChunksAndTrailer() throws Exception { |
There was a problem hiding this comment.
Would it be worth adding a test that uploads an empty object with a trailer?
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