Minor refacotring to AwsV4HttpSigner impl (#4556)

* Move public methods to top of DefaultAwsV4HttpSigner And other minor refactoring. * V4Context is not Immutable * Remove content length from builder instead of signed request
aws · Oct 6, 2023 · aa3e3ed · aa3e3ed
1 parent 92e3ea8
commit aa3e3ed
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 36 deletions.
diff --git a/...ain/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java b/...ain/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java
@@ -54,6 +54,26 @@ public final class DefaultAwsV4HttpSigner implements AwsV4HttpSigner {
 
     private static final int DEFAULT_CHUNK_SIZE_IN_BYTES = 128 * 1024;
 
+    @Override
+    public SignedRequest sign(SignRequest<? extends AwsCredentialsIdentity> request) {
+        Checksummer checksummer = checksummer(request);
+        V4Properties v4Properties = v4Properties(request);
+        V4RequestSigner v4RequestSigner = v4RequestSigner(request, v4Properties);
+        V4PayloadSigner payloadSigner = v4PayloadSigner(request, v4Properties);
+
+        return doSign(request, checksummer, v4RequestSigner, payloadSigner);
+    }
+
+    @Override
+    public CompletableFuture<AsyncSignedRequest> signAsync(AsyncSignRequest<? extends AwsCredentialsIdentity> request) {
+        Checksummer checksummer = checksummer(request);
+        V4Properties v4Properties = v4Properties(request);
+        V4RequestSigner v4RequestSigner = v4RequestSigner(request, v4Properties);
+        V4PayloadSigner payloadSigner = v4PayloadAsyncSigner(request, v4Properties);
+
+        return doSign(request, checksummer, v4RequestSigner, payloadSigner);
+    }
+
     private static V4Properties v4Properties(BaseSignRequest<?, ? extends AwsCredentialsIdentity> request) {
         Clock signingClock = request.requireProperty(SIGNING_CLOCK, Clock.systemUTC());
         Instant signingInstant = signingClock.instant();
@@ -106,17 +126,6 @@ private static V4RequestSigner v4RequestSigner(
         return requestSigner.apply(v4Properties);
     }
 
-    private static boolean hasChecksumHeader(BaseSignRequest<?, ? extends AwsCredentialsIdentity> request) {
-        ChecksumAlgorithm checksumAlgorithm = request.property(CHECKSUM_ALGORITHM);
-
-        if (checksumAlgorithm != null) {
-            String checksumHeaderName = checksumHeaderName(checksumAlgorithm);
-            return request.request().firstMatchingHeader(checksumHeaderName).isPresent();
-        }
-
-        return false;
-    }
-
     private static Checksummer checksummer(BaseSignRequest<?, ? extends AwsCredentialsIdentity> request) {
         boolean isPayloadSigning = isPayloadSigning(request);
         boolean isEventStreaming = isEventStreaming(request.request());
@@ -161,14 +170,8 @@ private static Checksummer checksummer(BaseSignRequest<?, ? extends AwsCredentia
         return Checksummer.forPrecomputed256Checksum(UNSIGNED_PAYLOAD);
     }
 
-    private static boolean useChunkEncoding(boolean payloadSigningEnabled, boolean chunkEncodingEnabled,
-                                            boolean isTrailingOrFlexible) {
-
-        return (payloadSigningEnabled && chunkEncodingEnabled) || (chunkEncodingEnabled && isTrailingOrFlexible);
-    }
-
     private static V4PayloadSigner v4PayloadSigner(
-        BaseSignRequest<?, ? extends AwsCredentialsIdentity> request,
+        SignRequest<? extends AwsCredentialsIdentity> request,
         V4Properties properties) {
 
         boolean isPayloadSigning = isPayloadSigning(request);
@@ -200,7 +203,7 @@ private static V4PayloadSigner v4PayloadSigner(
     }
 
     private static V4PayloadSigner v4PayloadAsyncSigner(
-        BaseSignRequest<?, ? extends AwsCredentialsIdentity> request,
+        AsyncSignRequest<? extends AwsCredentialsIdentity> request,
         V4Properties properties) {
 
         boolean isPayloadSigning = request.requireProperty(PAYLOAD_SIGNING_ENABLED, true);
@@ -289,23 +292,20 @@ private static boolean isEventStreaming(SdkHttpRequest request) {
         return "application/vnd.amazon.eventstream".equals(request.firstMatchingHeader(Header.CONTENT_TYPE).orElse(""));
     }
 
-    @Override
-    public SignedRequest sign(SignRequest<? extends AwsCredentialsIdentity> request) {
-        Checksummer checksummer = checksummer(request);
-        V4Properties v4Properties = v4Properties(request);
-        V4RequestSigner v4RequestSigner = v4RequestSigner(request, v4Properties);
-        V4PayloadSigner payloadSigner = v4PayloadSigner(request, v4Properties);
+    private static boolean hasChecksumHeader(BaseSignRequest<?, ? extends AwsCredentialsIdentity> request) {
+        ChecksumAlgorithm checksumAlgorithm = request.property(CHECKSUM_ALGORITHM);
 
-        return doSign(request, checksummer, v4RequestSigner, payloadSigner);
+        if (checksumAlgorithm != null) {
+            String checksumHeaderName = checksumHeaderName(checksumAlgorithm);
+            return request.request().firstMatchingHeader(checksumHeaderName).isPresent();
+        }
+
+        return false;
     }
 
-    @Override
-    public CompletableFuture<AsyncSignedRequest> signAsync(AsyncSignRequest<? extends AwsCredentialsIdentity> request) {
-        Checksummer checksummer = checksummer(request);
-        V4Properties v4Properties = v4Properties(request);
-        V4RequestSigner v4RequestSigner = v4RequestSigner(request, v4Properties);
-        V4PayloadSigner payloadSigner = v4PayloadAsyncSigner(request, v4Properties);
+    private static boolean useChunkEncoding(boolean payloadSigningEnabled, boolean chunkEncodingEnabled,
+                                            boolean isTrailingOrFlexible) {
 
-        return doSign(request, checksummer, v4RequestSigner, payloadSigner);
+        return (payloadSigningEnabled && chunkEncodingEnabled) || (chunkEncodingEnabled && isTrailingOrFlexible);
     }
 }
diff --git a/...uth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/V4Context.java b/...uth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/V4Context.java
@@ -15,15 +15,17 @@
 
 package software.amazon.awssdk.http.auth.aws.internal.signer;
 
-import software.amazon.awssdk.annotations.Immutable;
 import software.amazon.awssdk.annotations.SdkInternalApi;
 import software.amazon.awssdk.http.SdkHttpRequest;
 
 /**
  * A container for data produced during and as a result of the SigV4 request signing process.
  */
 @SdkInternalApi
-@Immutable
+// TODO(sra-identity-auth): This is currently not @Immutable because signedRequest is a Builder. Is Builder needed? If it could
+//  hold reference to SdkHttpRequest instead, this class would be @Immutable.
+// TODO(sra-identity-auth): Consider if we can rename this to convey something more. maybe,
+//  V4RequestSigningResult/V4RequestSigningResultData? Note there is V4aContext similarly.
 public final class V4Context {
     private final String contentHash;
     private final byte[] signingKey;

diff --git a/...a/software/amazon/awssdk/http/auth/aws/internal/signer/AwsChunkedV4PayloadSignerTest.java b/...a/software/amazon/awssdk/http/auth/aws/internal/signer/AwsChunkedV4PayloadSignerTest.java
@@ -362,6 +362,8 @@ public void sign_withoutContentLength_calculatesContentLengthFromPayload() throw
             "x-amz-checksum-sha256:oVyCkrHRKru75BSGBfeHL732RWGP7lqw6AcqezTxVeI=\r\n\r\n";
 
         requestBuilder.putHeader("x-amz-content-sha256", "STREAMING-UNSIGNED-PAYLOAD-TRAILER");
+        requestBuilder.removeHeader(Header.CONTENT_LENGTH);
+
         V4CanonicalRequest canonicalRequest = new V4CanonicalRequest(
             requestBuilder.build(),
             "STREAMING-UNSIGNED-PAYLOAD-TRAILER",
@@ -380,7 +382,6 @@ public void sign_withoutContentLength_calculatesContentLengthFromPayload() throw
                                                                     .checksumAlgorithm(SHA256)
                                                                     .build();
 
-        v4Context.getSignedRequest().removeHeader(Header.CONTENT_LENGTH);
         signer.beforeSigning(requestBuilder, payload);
         ContentStreamProvider signedPayload = signer.sign(payload, v4Context);