From aa3e3ed1ae405e07c56e55658a983f93db9ebcd2 Mon Sep 17 00:00:00 2001 From: Jaykumar Gosar <5666661+gosar@users.noreply.github.com> Date: Fri, 6 Oct 2023 10:16:08 -0700 Subject: [PATCH] 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 --- .../signer/DefaultAwsV4HttpSigner.java | 66 +++++++++---------- .../auth/aws/internal/signer/V4Context.java | 6 +- .../signer/AwsChunkedV4PayloadSignerTest.java | 3 +- 3 files changed, 39 insertions(+), 36 deletions(-) diff --git a/core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java b/core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java index afc5fcc3b931..f6ef26b46518 100644 --- a/core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java +++ b/core/http-auth-aws/src/main/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 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 signAsync(AsyncSignRequest 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 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 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 request) { boolean isPayloadSigning = isPayloadSigning(request); boolean isEventStreaming = isEventStreaming(request.request()); @@ -161,14 +170,8 @@ private static Checksummer checksummer(BaseSignRequest request, + SignRequest request, V4Properties properties) { boolean isPayloadSigning = isPayloadSigning(request); @@ -200,7 +203,7 @@ private static V4PayloadSigner v4PayloadSigner( } private static V4PayloadSigner v4PayloadAsyncSigner( - BaseSignRequest request, + AsyncSignRequest 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 request) { - Checksummer checksummer = checksummer(request); - V4Properties v4Properties = v4Properties(request); - V4RequestSigner v4RequestSigner = v4RequestSigner(request, v4Properties); - V4PayloadSigner payloadSigner = v4PayloadSigner(request, v4Properties); + private static boolean hasChecksumHeader(BaseSignRequest 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 signAsync(AsyncSignRequest 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/core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/V4Context.java b/core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/V4Context.java index 7e4430e8048f..c11c0805c4f9 100644 --- a/core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/V4Context.java +++ b/core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/V4Context.java @@ -15,7 +15,6 @@ 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; @@ -23,7 +22,10 @@ * 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/core/http-auth-aws/src/test/java/software/amazon/awssdk/http/auth/aws/internal/signer/AwsChunkedV4PayloadSignerTest.java b/core/http-auth-aws/src/test/java/software/amazon/awssdk/http/auth/aws/internal/signer/AwsChunkedV4PayloadSignerTest.java index 7d3137bccada..b767018d710c 100644 --- a/core/http-auth-aws/src/test/java/software/amazon/awssdk/http/auth/aws/internal/signer/AwsChunkedV4PayloadSignerTest.java +++ b/core/http-auth-aws/src/test/java/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);