Move S3Presigner fully to SRA with HttpSigner #4569
Conversation
| <dependency> | ||
| <groupId>software.amazon.awssdk</groupId> | ||
| <artifactId>identity-spi</artifactId> | ||
| <version>${awsjavasdk.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>software.amazon.awssdk</groupId> | ||
| <artifactId>http-auth-spi</artifactId> | ||
| <version>${awsjavasdk.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
DefaultS3Presigner directly depends on these, so declared explicitly, instead of coming in transitively.
| // A fixed signingClock is used, so that the current time used by the signing logic, as well as to determine expiration | ||
| // are the same. | ||
| Instant signingInstant = Instant.now(); | ||
| Clock signingClock = Clock.fixed(signingInstant, ZoneOffset.UTC); |
There was a problem hiding this comment.
An alternative to this would be to let the HttpSigner use current time inside it's logic, and later to calculate expiration to set inside presignedRequest in initializePresignedRequest we could extract the current time value used by the HttpSigner by reading the value out of X-Amz-Date query param inserted by the HttpSigner. initializePresignedRequest already has some logic to extract from X-Amz-SignedHeaders query param. But it seemed better to not rely more on implementation details of the HttpSigner.
| @@ -227,6 +228,7 @@ public void getObject_Sigv4PresignerHonorsSignatureDuration() { | |||
|
|
|||
| assertThat(presigned.httpRequest().rawQueryParameters().get("X-Amz-Expires").get(0)).satisfies(expires -> { | |||
| assertThat(expires).containsOnlyDigits(); | |||
| // TODO(sra-identity-auth): This should be isEqualTo(1234)? | |||
There was a problem hiding this comment.
I think it's a bug if isCloseTo was needed for some reason earlier instead of isEqualTo check. Not sure why it was so from the beginning, but couldn't change it right now because of derived attributes behavior for PRESIGNER_EXPIRATION. I have an internal discussion thread about it. Will update that separately.
There was a problem hiding this comment.
Addressing the derived attributes and isCloseTo in #4602
…a-ia-s3presigner-full-sra
|
SonarCloud Quality Gate failed.
|
gosar
merged commit 7d22755
into
feature/master/sra-identity-auth-testing
* Set TIME_OFFSET attribtue in SRA signin methods (#4483) * Verify that execution attributes set by old interceptors can be read by new signers. (#4494) * Move S3Presigner fully to SRA with HttpSigner (#4569) * Remove derived attribute logic for PRESIGNER_EXPIRATION (#4602) * Remove derived attribute logic for PRESIGNER_EXPIRATION Because of the 2 instant() calls in the write/read mappings, a basic write this attribute and read it back, would give different values. Since this attribute should only be set/read explicitly by our presigner code, we don't need this mapping logic at all. Also, update S3PresignerTest to assert isEqualTo instead of isCloseTo and remove unnecessary override * Remove PRESIGNER_EXPIRATION mirroring test * Enable SRA Auth for S3 * Make follow up changes from PR 4396 (#4454) * Make SigingStage updates similar to AsyncSigningStage. * Simplify (Async)SigningStageTests based on updated logic. * Minor typos * If authType=none don't use the old signer, even if overridden (#4523) * Add authType=none tests to AwsExecutionContentBuilderTest Some of these tests are failing. Will fix them in next commit * If authType=none don't use the old signer, even if overridden * Update signing stage logic for authType=none * Add asserts on identity resolution for authType=none * Add a missing http-auth-aws dependecy to aws-core * Add a changelog entry --------- Co-authored-by: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Co-authored-by: Matthew Miller <millem@amazon.com> Co-authored-by: Jaykumar Gosar <5666661+gosar@users.noreply.github.com>
Motivation and Context
S3PresignerTest.accessPointArn_multiRegion_useArnRegionTrue_correctEndpointAndSigner currently fails in feature/master/sra-identity-auth-testing (with useSraAuth=true) with
X-Amz-AlgorithmThe reason is that DefaultS3Presigner is partially on SRA. i.e., it uses S3AuthSchemeResolver, but uses old Signer. And S3ResolveEndpointInterceptor won't have logic to use to old sigv4a Signer because of this conditional logic in codegen. Instead of solving this via more codegen changes, we might as well move the presigner fully to SRA and use the new HttpSigner from auth scheme resolution, which will select the correct sigv4/sigv4a HttpSigner.
Modifications
Note, testing this end-to-end, noticed some behaviors of presign implementation for HttpSigner that are being fixed in Update presigner so host is set and content-hash is excluded from signed-headers #4560. This PR needs that change for unit and integ tests to pass.
Testing
Picked up #4560 locally and successfully ran unit and integ tests in the s3 module.
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License