smengcl commented on code in PR #11562:
URL: https://github.com/apache/ozone/pull/11562#discussion_r4394927392
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/util/S3Utils.java:
##########
@@ -189,7 +190,17 @@ public static String validateSignatureHeader(HttpHeaders
headers, String resourc
throw ex;
}
- return xAmzContentSha256Header;
+ S3ContentSha256Type payloadType =
S3ContentSha256Type.fromHeaderValue(xAmzContentSha256Header);
+ if (payloadType.isUnsigned() || payloadType.isStreaming()) {
+ return xAmzContentSha256Header;
+ }
+ if (!xAmzContentSha256Header.matches("[0-9a-fA-F]{64}")) {
Review Comment:
nit: `String.matches` compiles the regex on every call, and a regular PUT
calls `validateSignatureHeader` twice. Use a precompiled constant:
```suggestion
if (!SHA256_HEX_PATTERN.matcher(xAmzContentSha256Header).matches()) {
```
```diff
+ private static final Pattern SHA256_HEX_PATTERN =
Pattern.compile("[0-9a-fA-F]{64}");
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/util/S3Utils.java:
##########
@@ -189,7 +190,17 @@ public static String validateSignatureHeader(HttpHeaders
headers, String resourc
throw ex;
}
- return xAmzContentSha256Header;
+ S3ContentSha256Type payloadType =
S3ContentSha256Type.fromHeaderValue(xAmzContentSha256Header);
+ if (payloadType.isUnsigned() || payloadType.isStreaming()) {
+ return xAmzContentSha256Header;
+ }
+ if (!xAmzContentSha256Header.matches("[0-9a-fA-F]{64}")) {
+ OS3Exception ex = S3ErrorTable.newError(S3ErrorTable.INVALID_ARGUMENT,
resource);
+ ex.setErrorMessage("The " + X_AMZ_CONTENT_SHA256 + " header must contain
64 hexadecimal characters");
Review Comment:
nit: the other two errors in this method start with `"An error occurred
(InvalidArgument): "`. Keep the same format:
```suggestion
ex.setErrorMessage("An error occurred (InvalidArgument): " +
"The " + X_AMZ_CONTENT_SHA256 + " header must contain 64
hexadecimal characters");
```
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/util/S3Utils.java:
##########
@@ -189,7 +190,17 @@ public static String validateSignatureHeader(HttpHeaders
headers, String resourc
throw ex;
}
- return xAmzContentSha256Header;
+ S3ContentSha256Type payloadType =
S3ContentSha256Type.fromHeaderValue(xAmzContentSha256Header);
+ if (payloadType.isUnsigned() || payloadType.isStreaming()) {
+ return xAmzContentSha256Header;
+ }
+ if (!xAmzContentSha256Header.matches("[0-9a-fA-F]{64}")) {
+ OS3Exception ex = S3ErrorTable.newError(S3ErrorTable.INVALID_ARGUMENT,
resource);
+ ex.setErrorMessage("The " + X_AMZ_CONTENT_SHA256 + " header must contain
64 hexadecimal characters");
+ throw ex;
+ }
+
+ return xAmzContentSha256Header.toLowerCase(Locale.ROOT);
Review Comment:
nit: the uppercase case is handled twice.
`S3ObjectWriteGuard.checkContentSha256` now uses `equalsIgnoreCase`, and that
alone covers all four call sites, while this lowercasing does not reach the two
multipart paths anyway (`ObjectEndpoint` part upload and
`ObjectEndpointStreaming.createMultipartKey` pass the raw header to
`checkContentSha256`).
Return the header unchanged and drop the `Locale` import;
`testNormalizeContentSha256` can go too, since the uppercase upload tests in
`TestObjectPut` and `TestPartUpload` already cover it end to end.
```suggestion
return xAmzContentSha256Header;
```
--
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]
