hevinhsu commented on code in PR #9294:
URL: https://github.com/apache/ozone/pull/9294#discussion_r2549323177
##########
hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/signature/StringToSignProducer.java:
##########
@@ -70,19 +80,47 @@ public final class StringToSignProducer {
DateTimeFormatter.ofPattern("yyyyMMdd'T'HHmmss'Z'")
.withZone(ZoneOffset.UTC);
+ private static final Set<String> VALID_UNSIGNED_PAYLOADS;
+
+ static {
+ Set<String> set = new HashSet<>();
+ set.add(UNSIGNED_PAYLOAD);
+ set.add(STREAMING_UNSIGNED_PAYLOAD_TRAILER);
+ set.add(STREAMING_AWS4_HMAC_SHA256_PAYLOAD);
+ set.add(STREAMING_AWS4_HMAC_SHA256_PAYLOAD_TRAILER);
+ set.add(STREAMING_AWS4_ECDSA_P256_SHA256_PAYLOAD);
+ set.add(STREAMING_AWS4_ECDSA_P256_SHA256_PAYLOAD_TRAILER);
+ VALID_UNSIGNED_PAYLOADS = Collections.unmodifiableSet(set);
+ }
+
private StringToSignProducer() {
}
public static String createSignatureBase(
SignatureInfo signatureInfo,
ContainerRequestContext context
) throws Exception {
- return createSignatureBase(signatureInfo,
+ LowerCaseKeyStringMap lowerCaseKeyStringMap =
LowerCaseKeyStringMap.fromHeaderMap(context.getHeaders());
+ String signatureBase = createSignatureBase(signatureInfo,
context.getUriInfo().getRequestUri().getScheme(),
context.getMethod(),
- LowerCaseKeyStringMap.fromHeaderMap(context.getHeaders()),
+ lowerCaseKeyStringMap,
fromMultiValueToSingleValueMap(
context.getUriInfo().getQueryParameters()));
+
+ // validate x-amz-content-sha256
+ String payloadHash = getPayloadHash(lowerCaseKeyStringMap,
!signatureInfo.isSignPayload());
+ if (!VALID_UNSIGNED_PAYLOADS.contains(payloadHash)) {
+ byte[] payload = IOUtils.toByteArray(context.getEntityStream());
+ context.setEntityStream(new ByteArrayInputStream(payload));
Review Comment:
Hi @ivandika3 ,
I've update the implmentation for the SHA-256 verification flow by
integrating `MultiDigestInputStream`, enabling us to compute the digest during
the write pipeline and suppress the `commit` call when a mismatch is detected.
### Implementation details
1. **Removed try-with-resources**
Since `close()` always triggers `commit`, I replaced the
try-with-resources block with explicit lifecycle control so we can skip the
commit path when validation fails.
2. **Manual close vs cleanup**
* When the digest **matches**, I call `close()` explicitly.
* When it **mismatches**, I need to release resources **without
committing**.
To achieve this, I added a `cleanup()` method containing only the
cleanup logic extracted from `close()`, without triggering the commit.
3. **Changes in client I/O code**
Since `cleanup()` needs to be accessible, I had to touch the client I/O
layer (`KeyOutputStream` / `KeyDataStreamOutput`).
This is the part I’m a bit concerned about — the changes might be
considered *out of scope* because they affect the client I/O API surface, not
just S3 validation logic.
### Review question
Would you consider this approach acceptable, or should I explore an
alternative to avoid modifying the client I/O code?
PTAL when you have time — thanks!
--
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]
