Skip to content

Fix Content-Length requirement, header updates in payload signer, http fallback to signed #4492

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
Merged
merged 4 commits into from
Sep 29, 2023
Merged

Fix Content-Length requirement, header updates in payload signer, http fallback to signed #4492

merged 4 commits into from
Sep 29, 2023

Conversation

haydenbaker
Copy link
Contributor

@haydenbaker haydenbaker commented Sep 27, 2023 •
edited
Loading

Motivation and Context

  • Content-Length should be pre-calculated for aws-chunked content-encoding and set, so that the http-client does not need to buffer the payload in memory to calculate it for us
  • Updates to headers on the request by the payload signer must happen before request is signed, otherwise the canonical request that is signed will be incorrect
  • Unsigned-payload with HTTP should fall back to signed-payload, just as the old signers do

Testing

  • Run new and existing tests, make sure pass

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 force-pushed the haydenbaker/sra-ia-signing-header-fix branch from 9b81e1a to 8c99bb1 Compare September 27, 2023 20:51
gosar
gosar reviewed Sep 27, 2023
View reviewed changes
...st/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSignerTest.java
@@ -291,4 +292,19 @@ public void sign_WithEventStreamContentTypeAndUnsignedPayload_Throws() {

assertThrows(UnsupportedOperationException.class, () -> signer.signAsync(request));
}

@Test
public void sign_WithPayloadSigningFalseAndHttp_FallsBackToPayloadSigning() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have additional test for sign_WithPayloadSigningNullAndHttp_DefaultsToPayloadSigning? I know the default is true, but this is double checking that our defaulting logic is fine in the face of http/https.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@haydenbaker haydenbaker changed the title Fix header updates in payload signer, http fallback to signed Fix Content-Length requirement, header updates in payload signer, http fallback to signed Sep 28, 2023
@haydenbaker haydenbaker marked this pull request as ready for review September 28, 2023 16:18
@haydenbaker haydenbaker requested a review from a team as a code owner September 28, 2023 16:18
@haydenbaker haydenbaker force-pushed the haydenbaker/sra-ia-signing-header-fix branch from 8c99bb1 to 38a2fb9 Compare September 28, 2023 16:45
@haydenbaker haydenbaker force-pushed the haydenbaker/sra-ia-signing-header-fix branch from 38a2fb9 to e96ef09 Compare September 28, 2023 17:01
haydenbaker
haydenbaker commented Sep 28, 2023
View reviewed changes
...ava/software/amazon/awssdk/http/auth/aws/crt/internal/signer/AwsChunkedV4aPayloadSigner.java Outdated
encodedContentLength += calculateChunksLength(contentLength, 0);
break;
case STREAMING_ECDSA_SIGNED_PAYLOAD_TRAILER: {
long extensionsLength = 161; // ;chunk-signature:<sigv4a-ecsda hex signature, 64 bytes>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix comment

cenedhryn
cenedhryn reviewed Sep 28, 2023
View reviewed changes
...aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/util/SignerUtils.java
@@ -252,4 +254,21 @@ public static byte[] hash(byte[] data) {
public static byte[] hash(String text) {
return hash(text.getBytes(StandardCharsets.UTF_8));
}

private static int readAll(InputStream inputStream) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment why it's safe to use this method on the supplied stream

cenedhryn
cenedhryn approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@cenedhryn cenedhryn left a comment

Choose a reason for hiding this comment

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

Approving - didn't look at naming etc, which could be refactored later.

@haydenbaker haydenbaker force-pushed the haydenbaker/sra-ia-signing-header-fix branch from 1ea28c4 to aab03c8 Compare September 28, 2023 23:07
@haydenbaker haydenbaker merged commit 43b5b43 into feature/master/sra-identity-auth Sep 29, 2023
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 9 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 2 Security Hotspots
Code Smell A 453 Code Smells

84.5% 84.5% Coverage
4.6% 4.6% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@rphilippen rphilippen mentioned this pull request Feb 4, 2025
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gosar gosar gosar left review comments

@cenedhryn cenedhryn cenedhryn 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 @gosar @cenedhryn