-
Notifications
You must be signed in to change notification settings - Fork 977
Disable payload signing for https requests #6691
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.
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
base: master
Are you sure you want to change the base?
Disable payload signing for https requests #6691
Conversation
| assertThat(signedRequest.request().firstMatchingHeader("X-Amz-Region-Set")).hasValue("aws-global"); | ||
| assertThat(signedRequest.request().firstMatchingHeader("Authorization")).isPresent(); | ||
| assertThat(signedRequest.request().firstMatchingHeader("x-amz-content-sha256")).contains(PAYLOAD_SHA256_HEX); | ||
| assertThat(signedRequest.request().firstMatchingHeader("x-amz-content-sha256")).hasValue("UNSIGNED-PAYLOAD"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing default behavior
| assertThat(signedRequest.request().firstMatchingHeader("X-Amz-Region-Set")).hasValue("aws-global"); | ||
| assertThat(signedRequest.request().firstMatchingHeader("Authorization")).isPresent(); | ||
| assertThat(signedRequest.request().firstMatchingHeader("x-amz-content-sha256")).contains(PAYLOAD_SHA256_HEX); | ||
| assertThat(signedRequest.request().firstMatchingHeader("x-amz-content-sha256")).hasValue("UNSIGNED-PAYLOAD"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing default behavior
...ws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/util/ChecksumUtil.java
Outdated
Show resolved
Hide resolved
...uth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/signer/AwsV4FamilyHttpSigner.java
Outdated
Show resolved
Hide resolved
| if (!isPayloadSigningEnabled) { | ||
| if (payloadSigningEnabled != null) { | ||
| // presigning requests should always have a null payload, and should always be unsigned-payload | ||
| if (!isEncrypted && hasPayload && !payloadSigningEnabled) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If payloadSigningEnabled is not null, i.e., configured by the user or set on the service model, should we always enable payload signing regardless of isEncrypted?
Addresses:
JAVA-8739Background
The SDK currently signs payloads for all requests (HTTP and HTTPS). This differs from V1, which only signed payloads for HTTP requests. For HTTPS requests, payload signing is redundant since TLS already provides integrity protection, and it causes measurable performance degradation. This change follows the same behavior of v1 where HTTPS requests' payloads were not signed.
Benchmarking Approach
Performance was measured using JMH with a stubbed DynamoDB client to isolate signing overhead. I chose two binary payload a 10kb and 400kb (DDB limit) to see if this exacerbates the performance bottleneck.
Results
Flamegraphs
Before
After