chungen0126 commented on code in PR #11222:
URL: https://github.com/apache/ozone/pull/11222#discussion_r4069542528
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java:
##########
@@ -822,7 +824,15 @@ protected S3ChunkInputStreamInfo getS3ChunkInputStreamInfo(
if (hasUnsignedPayload(amzContentSha256Header)) {
chunkInputStream = new UnsignedChunksInputStream(body);
} else {
- chunkInputStream = new SignedChunksInputStream(body, keyPath);
+ String trailerHeader =
STREAMING_AWS4_HMAC_SHA256_PAYLOAD_TRAILER.equals(amzContentSha256Header)
+ ? getHeaders().getHeaderString(X_AMZ_TRAILER) : null;
+ if
(STREAMING_AWS4_HMAC_SHA256_PAYLOAD_TRAILER.equals(amzContentSha256Header)
+ && StringUtils.isBlank(trailerHeader)) {
+ OS3Exception ex = newError(INVALID_ARGUMENT, keyPath);
+ ex.setErrorMessage("The " + X_AMZ_TRAILER + " header is required for
signed trailing headers");
+ throw ex;
+ }
Review Comment:
I noticed that SignedChunksInputStream also validates the x-amz-trailer
header. Could we consolidate these validation checks in one place?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/protocolPB/OzoneManagerProtocolServerSideTranslatorPB.java:
##########
@@ -185,6 +187,13 @@ private OMResponse internalProcessRequest(OMRequest
request) throws ServiceExcep
// If request has S3Authentication, validate S3 credentials.
// If current OM is leader and then proceed with the request.
S3SecurityUtil.validateS3Credential(request, ozoneManager);
+ if (ozoneManager.isSecurityEnabled() && request.getCmdType() ==
OzoneManagerProtocolProtos.Type.CreateKey
+ && request.getCreateKeyRequest().getDerivedKeyPiggyBacking()) {
+ derivedKey =
ByteString.copyFrom(ozoneManager.getS3DerivedKey(request.getS3Authentication()));
+ // Only the RPC response needs this key. Older followers must not
derive it during apply either.
+ request =
request.toBuilder().setCreateKeyRequest(request.getCreateKeyRequest().toBuilder()
+ .clearDerivedKeyPiggyBacking()).build();
+ }
} catch (IOException ex) {
Review Comment:
Consider wrapping this logic into a method to keep here cleaner.
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/SignedChunksInputStream.java:
##########
@@ -100,6 +107,9 @@ public class SignedChunksInputStream extends InputStream {
/** Signature parsed from the current chunk header line. */
private String chunkSignature;
+ /** Checksum header declared by x-amz-trailer, or null for a regular signed
stream. */
+ private final String trailerHeader;
Review Comment:
I feel that using an `Enum` for trailerHeader would be much more elegant:
1. We wouldn't need to do null checks anymore.
2. It also removes the need for `CHECKSUM_TRAILER_PATTERN`.
What do you think?
--
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]
