Skip to content

Minor refacotring to AwsV4HttpSigner impl #4556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gosar merged 3 commits into from
Oct 6, 2023
Merged

Minor refacotring to AwsV4HttpSigner impl #4556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c19a19
Move public methods to top of DefaultAwsV4HttpSigner
gosar Oct 6, 2023
b44f83d
V4Context is not Immutable
gosar Oct 6, 2023
703e8ba
Remove content length from builder instead of signed request
gosar Oct 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 33 additions & 33 deletions ...ain/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
}
6 changes: 4 additions & 2 deletions ...uth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/V4Context.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +27 to +28
Copy link
Contributor

@cenedhryn cenedhryn Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, V4Context is a terrible name. I like V4RequestSigningResult or V4RequestSigningOutput

Copy link
Contributor Author

@gosar gosar Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will take up the TODOs separately.

Copy link
Contributor

@haydenbaker haydenbaker Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it needs to be a builder anymore with the payload-signer now having a dedicated method to modify the request that is ran before request-signing.

Copy link
Contributor Author

@gosar gosar Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, I noticed later it can be changed, but didn't take that on immediately. It's SdkInternal, so non-blocking, so was planing to address both TODOs later.

public final class V4Context {
private final String contentHash;
private final byte[] signingKey;
Expand Down
3 changes: 2 additions & 1 deletion ...a/software/amazon/awssdk/http/auth/aws/internal/signer/AwsChunkedV4PayloadSignerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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);

Expand Down