Skip to content

Support async V4 payload signing #6314

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dagnir
Copy link
Contributor

@dagnir dagnir commented Aug 1, 2025

Motivation and Context

This commit adds support for SigV4 signing of async request payloads.

In addition this commit moves the support for trailing checksums from HttpChecksumStage to the V4 signer implementation; this puts it in line with how sync chunked bodies are already handled.

This fills a gap in our signing logic where payloads that would normally be signed are not.

Modifications

  • Remove trailing checksum path for SRA signed requests in HttpChecksumStage; trailing checksums taken care of in AwsChunkedV4PayloadSigner, same as for sync payloads
  • Support payload signing and trailers for async requests in AwsChunkedV4PayloadSigner
  • Internal refactor of AwschunkedV4PayloadSigner to reduce code duplication

Testing

  • New unit tests
  • Enable previously disabled tests that were not passing
  • Ran integ tests

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

@dagnir dagnir force-pushed the dongie/chunked-encoded-sigv4-async branch 2 times, most recently from 4085d5e to 8a30699 Compare August 1, 2025 22:40
@dagnir dagnir changed the title Dongie/chunked encoded sigv4 async Support async V4 payload signing Aug 1, 2025
@dagnir dagnir force-pushed the dongie/chunked-encoded-sigv4-async branch 4 times, most recently from f80adb0 to 9fa0bad Compare August 5, 2025 21:01
Comment on lines -187 to -189
"4000" + CRLF + contentString.substring(0, 16 * KB) + CRLF
+ "4000" + CRLF + contentString.substring(16 * KB, 32 * KB) + CRLF
+ "1400" + CRLF + contentString.substring(32 * KB) + CRLF
Copy link
Contributor Author

@dagnir dagnir Aug 5, 2025

Choose a reason for hiding this comment

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

FYI this is because we use the default chunk size in DefaultAwsV4HttpSigner now, which is 128 KiB, so the 37KiB payload fits in a single chunk:

if (protocol == Protocol.HTTP && clientType == ClientType.SYNC) {
void verifyPutObjectHeaders(Protocol protocol, ChecksumAlgorithm checksumAlgorithm) {
String streamingSha256;
if (protocol == Protocol.HTTP) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fallback to body content signing now works for async requests

@dagnir dagnir force-pushed the dongie/chunked-encoded-sigv4-async branch from 9fa0bad to 395a9cb Compare August 5, 2025 21:04
This commit adds support for SigV4 signing of async request payloads.

In addition this commit moves the support for trailing checksums from
HttpChecksumStage to the V4 signer implementation; this puts it in line
with how sync chunked bodies are already handled.
@dagnir dagnir force-pushed the dongie/chunked-encoded-sigv4-async branch from 395a9cb to 7c7941c Compare August 5, 2025 21:14
@dagnir dagnir marked this pull request as ready for review August 5, 2025 21:14
@dagnir dagnir requested a review from a team as a code owner August 5, 2025 21:14
Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

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

Is there a performance regression risk for customers that have been using http to avoid tls overhead?

@dagnir
Copy link
Contributor Author

dagnir commented Aug 6, 2025

Is there a performance regression risk for customers that have been using http to avoid tls overhead?

Yes, this is probably true, but this is a functionality gap rather than a feature, so I think this is okay. I don't have concrete numbers, but I will start some long running tests to see.

 - Rename to computeAndMoveContentLength
 - Rename variable to chunkedPayload
 - Fix javadocs
 - Sign payload only if non-null
@dagnir dagnir force-pushed the dongie/chunked-encoded-sigv4-async branch from 0a7ec87 to a222719 Compare August 6, 2025 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants