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.

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

rfc to disable payload signing #3583

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Apr 12, 2024

Motivation and Context

Original issue: awslabs/aws-sdk-rust#1087

Description

RFC-44 proposes a new disable_payload_signing() operation customization that would allow users to disable Sigv4(a) payload signing for operation requests that target an @streaming blob shape.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@aajtodd aajtodd requested review from a team as code owners April 12, 2024 15:47
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Copy link
Collaborator

@jdisanti jdisanti left a comment

Choose a reason for hiding this comment

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

RFC looks good. Just one question.

Most requests must include a hash of the payload as part of the signature. However, requests with a
[streaming](https://smithy.io/2.0/spec/streaming.html#smithy-api-streaming-trait) `blob` shape as the payload can
sometimes (e.g. S3 `PutObject`) choose to skip payload hashing and exclude it from the signature. When calculating the
signature the body hash is instead set to the literal constant `UNSIGNED-PAYLOAD`. This has the advantage of not paying
Copy link
Collaborator

Choose a reason for hiding this comment

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

What services does this actually work for? I was under the impression this only worked for S3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have a comprehensive list and I doubt we can definitively know without actually trying to send an unsigned payload. There are only something like ~30 or less binary streaming operations across all of AWS services IIRC.

We could limit this to just S3 for now if we wanted to be conservative (and likely the only use case most will care about).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think we should allow list this until we've verified it actually works in other services.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm good with that, will update the RFC to mention this.

Copy link
Contributor

@ysaito1001 ysaito1001 left a comment

Choose a reason for hiding this comment

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

RFC looks good. Will the RFC briefly describe how DisablePayloadSigningPlugin is implemented?

github-merge-queue bot pushed a commit that referenced this pull request Dec 4, 2024
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
#3583

## Description
<!--- Describe your changes in detail -->
This PR adds the ability for users to disable payload signing with an
operation customization.

## Testing
<!--- Please describe in detail how you tested your changes -->
<!--- Include details of your testing environment, and the tests you ran
to -->
<!--- see how your change affects other areas of the code, etc. -->
This PR includes tests.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] For changes to the AWS SDK, generated SDK code, or SDK runtime
crates, I have created a changelog entry Markdown file in the
`.changelog` directory, specifying "aws-sdk-rust" in the `applies_to`
key.

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants