chihsuan commented on code in PR #11589: URL: https://github.com/apache/ozone/pull/11589#discussion_r4434163023
########## hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/TrailerChecksum.java: ########## @@ -0,0 +1,132 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.ozone.s3; + +import java.security.MessageDigest; +import java.util.zip.Checksum; +import org.apache.hadoop.ozone.s3.exception.S3ErrorTable; + +/** Incrementally verifies a checksum over the decoded payload, independently of chunk signatures. */ +final class TrailerChecksum { Review Comment: Could we use a small abstract class with CRC and digest implementations? Each one then handles only its own type, and `verify` lives in one place. For example: ```java abstract class TrailerChecksum { abstract void update(byte[] b, int off, int len); abstract byte[] finish(); void verify(byte[] expected, String resource) { if (!MessageDigest.isEqual(expected, finish())) { throw S3ErrorTable.newError(S3ErrorTable.BAD_DIGEST, resource); } } static TrailerChecksum of(MessageDigest digest) { return new TrailerChecksum() { void update(byte[] b, int off, int len) { digest.update(b, off, len); } byte[] finish() { return digest.digest(); } }; } // of(Checksum crc, int length): finish() converts crc.getValue() to big-endian bytes } ``` ########## hadoop-ozone/integration-test-s3/src/test/java/org/apache/hadoop/ozone/s3/awssdk/v2/AbstractS3SDKV2Tests.java: ########## @@ -332,6 +334,40 @@ public void testPutObject() { assertEquals("\"37b51d194a7513e45b56f6524f2d51f2\"", getObjectResponse.eTag()); } + @ParameterizedTest + @ValueSource(strings = {"", "bar"}) + public void testPutObjectAsyncWithTrailingChecksum(String content) throws Exception { Review Comment: Would it be worth testing a sync `putObject` of `dir/` on an FSO bucket? The sync client sends a signed trailer, and `testCreateDirectory` uses an OBS bucket. ########## hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java: ########## @@ -258,11 +258,14 @@ Response handlePutRequest(ObjectRequestContext context, String keyPath, InputStr StringUtils.endsWith(keyPath, "/") ) { context.setAction(S3GAction.CREATE_DIRECTORY); - if (STREAMING_UNSIGNED_PAYLOAD_TRAILER.equals(getHeaders().getHeaderString(S3Consts.X_AMZ_CONTENT_SHA256))) { - S3ChunkInputStreamInfo info = getS3ChunkInputStreamInfo(body, length, amzDecodedLength, keyPath); + String payloadMode = getHeaders().getHeaderString(S3Consts.X_AMZ_CONTENT_SHA256); + if (STREAMING_UNSIGNED_PAYLOAD_TRAILER.equals(payloadMode) + || STREAMING_AWS4_HMAC_SHA256_PAYLOAD_TRAILER.equals(payloadMode)) { Review Comment: nit: Could we reuse `amzContentSha256` from above? ```suggestion if (STREAMING_UNSIGNED_PAYLOAD_TRAILER.equals(amzContentSha256) || STREAMING_AWS4_HMAC_SHA256_PAYLOAD_TRAILER.equals(amzContentSha256)) { ``` ########## hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/TestSignedChunksInputStream.java: ########## @@ -331,6 +337,70 @@ void testMultiChunksWithTrailer() throws Exception { } } + static Stream<Arguments> checksumCases() { + return Stream.of( + Arguments.of(TrailerHeader.CRC32, "y/Q5Jg=="), + Arguments.of(TrailerHeader.CRC32C, "4waSgw=="), + Arguments.of(TrailerHeader.CRC64NVME, "rosUhgp5mIg="), + Arguments.of(TrailerHeader.SHA1, "98O8HYCOBHMq32eZZczDTKeuNEE="), + Arguments.of(TrailerHeader.SHA256, "FeKw08M4keuw8e9gnsQZQgwg4yDOlMZfvIwzEkSOsiU=")) + .flatMap(args -> Stream.of(false, true).flatMap(buffered -> Stream.of(false, true) + .map(verify -> Arguments.of(args.get()[0], args.get()[1], buffered, verify)))); + } + + @ParameterizedTest + @MethodSource("checksumCases") + void verifiesPayloadChecksum(TrailerHeader header, String value, boolean buffered, boolean verify) Review Comment: Could we add a multi-chunk case read at a non-zero offset? A wrong offset in `updateDigest(b, off, len)` still passes these single-chunk tests. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
