Skip to content
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

Jump to bottom

Implement v4a aws-chunked payload signer and s3-crt delegation #4350

Merged
merged 5 commits into from
Sep 5, 2023
Merged

Implement v4a aws-chunked payload signer and s3-crt delegation #4350

merged 5 commits into from
Sep 5, 2023

Conversation

haydenbaker
Copy link
Contributor

Motivation and Context

Modifications

  • Copies chunk-encoding code from http-auth-aws, such that it doesn't need to be protected/shared right now (the implementation may change, and we don't want to block on this)
  • Adds the necessary code to decide on chunk-encoding, payload signing, etc.

Testing

  • run old and new tests, make sure pass

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

@haydenbaker haydenbaker requested a review from a team as a code owner August 25, 2023 21:46
@haydenbaker haydenbaker changed the base branch from master to feature/master/sra-identity-auth August 25, 2023 21:46
@haydenbaker haydenbaker force-pushed the haydenbaker/sra-ia-awss3v4crt-signer-composed branch from 5b00087 to 69e9b15 Compare August 28, 2023 17:49
haydenbaker
haydenbaker commented Aug 28, 2023
View reviewed changes
...t/src/main/java/software/amazon/awssdk/http/auth/aws/crt/internal/chunkedencoding/Chunk.java Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore.

haydenbaker
haydenbaker commented Aug 28, 2023
View reviewed changes
...oftware/amazon/awssdk/http/auth/aws/crt/internal/chunkedencoding/ChunkExtensionProvider.java Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore.

haydenbaker
haydenbaker commented Aug 28, 2023
View reviewed changes
...a/software/amazon/awssdk/http/auth/aws/crt/internal/chunkedencoding/ChunkHeaderProvider.java Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore.

haydenbaker
haydenbaker commented Aug 28, 2023
View reviewed changes
...java/software/amazon/awssdk/http/auth/aws/crt/internal/chunkedencoding/ChunkInputStream.java Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore.

haydenbaker
haydenbaker commented Aug 28, 2023
View reviewed changes
...ware/amazon/awssdk/http/auth/aws/crt/internal/chunkedencoding/ChunkedEncodedInputStream.java Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore.

haydenbaker
haydenbaker commented Aug 28, 2023
View reviewed changes
...ain/java/software/amazon/awssdk/http/auth/aws/crt/internal/chunkedencoding/DefaultChunk.java Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore.

haydenbaker
haydenbaker commented Aug 28, 2023
View reviewed changes
.../java/software/amazon/awssdk/http/auth/aws/crt/internal/chunkedencoding/TrailerProvider.java Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore.

haydenbaker
haydenbaker commented Aug 28, 2023
View reviewed changes
...ain/java/software/amazon/awssdk/http/auth/aws/crt/internal/io/SdkLengthAwareInputStream.java Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignore.

zoewangg
zoewangg reviewed Aug 28, 2023
View reviewed changes
...ava/software/amazon/awssdk/http/auth/aws/crt/internal/signer/DefaultAwsCrtV4aHttpSigner.java Outdated
@@ -47,6 +52,47 @@
@SdkInternalApi
public final class DefaultAwsCrtV4aHttpSigner implements AwsV4aHttpSigner {

private static final int DEFAULT_CHUNK_SIZE_IN_BYTES = 128 * 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use the same value else where? If so, should we share the constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the AWS module, I don't think we need to share this one.

...rt/src/main/java/software/amazon/awssdk/http/auth/aws/crt/internal/signer/RollingSigner.java Outdated
HttpRequestBodyStream crtBody = new CrtInputStream(() -> new ByteArrayInputStream(chunkBody));
CompletableFuture<byte[]> future = AwsSigner.signChunk(crtBody, previousSignature, signingConfig);
try {
return future.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

joinLikeSync? https://github.com/aws/aws-sdk-java-v2/blob/feature/master/sra-identity-auth/utils/src/main/java/software/amazon/awssdk/utils/CompletableFutureUtils.java#L247

...ava/software/amazon/awssdk/http/auth/aws/crt/internal/signer/DefaultAwsCrtV4aHttpSigner.java Outdated
@@ -135,12 +144,72 @@ private AwsSigningConfig signingConfig(SignRequest<?, ? extends AwsCredentialsId
}

if (!isPayloadSigning) {
signingConfig.setSignedBodyValue(AwsSigningConfig.AwsSignedBodyValue.UNSIGNED_PAYLOAD);
if (isChunkEncoding) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: nested if-else is a bit hard to read. Can we split up the block a bit?

if (isPayloadSigning) {
    configurePayloadSigning();
} else {
   configureNonPayloadSigning();
}

Copy link
Contributor Author

@haydenbaker haydenbaker Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm wondering if we can handle complex decisions like this in a more clean way (not just here, but in other places as well), any ideas? I think it's easy to reason about right now, but it would be nice if we could do (pseudocode):

STREAMING_UNSIGNED_TRAILER = !isPayloadSigning & isChunked & isTrailing
STREAMING_SIGNED = isPayloadSigning & isChunked
...
switch (payloadType)
  case STREAMING_SIGNED:
    // configure here
  case STREAMING_UNSIGNED_TRAILER:
    // configure here
  ...
   default:
      throw new UnsupportedOperationException()

We could probably do a bitmask, but might not be as easy to reason through.

...rt/src/main/java/software/amazon/awssdk/http/auth/aws/crt/internal/signer/RollingSigner.java
}

private static byte[] signChunk(byte[] chunkBody, byte[] previousSignature, AwsSigningConfig signingConfig) {
HttpRequestBodyStream crtBody = new CrtInputStream(() -> new ByteArrayInputStream(chunkBody));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to close the body afterwards?

Copy link
Contributor Author

@haydenbaker haydenbaker Aug 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good question. Closing would need to happen somewhere downstream (such as in AwsSigner.signChunk()) or in CrtInputStream itself). It doesn't happen in AwsSigner, so perhaps we can add it to CrtInputStream. Done.

...rt/src/main/java/software/amazon/awssdk/http/auth/aws/crt/internal/signer/RollingSigner.java Outdated
configCopy.setSignatureType(AwsSigningConfig.AwsSignatureType.HTTP_REQUEST_TRAILING_HEADERS);
CompletableFuture<AwsSigningResult> future = AwsSigner.sign(httpHeaderList, previousSignature, configCopy);
try {
return future.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, joinLikeSync

@haydenbaker haydenbaker force-pushed the haydenbaker/sra-ia-awss3v4crt-signer-composed branch from f6f4624 to dcdf605 Compare August 29, 2023 19:24
zoewangg
zoewangg reviewed Aug 29, 2023
View reviewed changes
...ava/software/amazon/awssdk/http/auth/aws/crt/internal/signer/DefaultAwsCrtV4aHttpSigner.java Outdated
private static void configureUnsignedPayload(AwsSigningConfig signingConfig, boolean isChunkEncoding, boolean isTrailing) {
if (isChunkEncoding) {
if (isTrailing) {
signingConfig.setSignedBodyValue(AwsSigningConfig.AwsSignedBodyValue.STREAMING_UNSIGNED_PAYLOAD_TRAILER);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we use static import? signingConfig.setSignedBodyValue(STREAMING_UNSIGNED_PAYLOAD_TRAILER). Same as others

...s-crt/src/main/java/software/amazon/awssdk/http/auth/aws/crt/internal/signer/V4aContext.java

public V4aContext(SdkHttpRequest signedRequest) {
public V4aContext(SdkHttpRequest.Builder signedRequest, byte[] signature, AwsSigningConfig signingConfig) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason we changed it to the builder instance? Is this just for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is changed because the payload signing step needs access to the request-builder from the context in order to update the request-headers (such as removing content-length, etc.)

@haydenbaker haydenbaker force-pushed the haydenbaker/sra-ia-awss3v4crt-signer-composed branch 2 times, most recently from 4852d00 to 5b6a418 Compare September 1, 2023 19:04
zoewangg
zoewangg approved these changes Sep 1, 2023
View reviewed changes
...ava/software/amazon/awssdk/http/auth/aws/crt/internal/signer/AwsChunkedV4aPayloadSigner.java
}

private void setupChecksumTrailer(ChunkedEncodedInputStream.Builder builder) {
// TODO: Set up checksumming of chunks and add as a trailer
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we use TODO(sra-identity-and-auth) naming convention. Ignore this if this TODO gets deleted in other PR

@haydenbaker haydenbaker force-pushed the haydenbaker/sra-ia-awss3v4crt-signer-composed branch from 5b6a418 to 25cdb4a Compare September 5, 2023 15:18
@haydenbaker haydenbaker merged commit 0034c78 into feature/master/sra-identity-auth Sep 5, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dagnir dagnir dagnir approved these changes

@zoewangg zoewangg zoewangg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haydenbaker @dagnir @zoewangg