-
Notifications
You must be signed in to change notification settings - Fork 190
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
aajtodd
wants to merge
1
commit into
main
Choose a base branch
from
atodd/rfc-disable-signing
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
RFC: Disable Payload Signing | ||
============================= | ||
|
||
> Status: RFC | ||
> | ||
> Applies to: client | ||
|
||
For a summarized list of proposed changes, see the [Changes Checklist](#changes-checklist) section. | ||
|
||
This RFC proposes a way for users to disable payload signing for requests with a streaming binary payload. | ||
|
||
|
||
The user experience if this RFC is implemented | ||
---------------------------------------------- | ||
|
||
|
||
In the current version of the SDK, users are unable to control how a request is signed. | ||
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 | ||
the cost of reading the payload into memory or hashing it at the cost of giving up some integrity checks. | ||
This may be a desired tradeoff though when performance is the top concern or when other checksums are already being used | ||
(e.g. [S3 checksums](https://aws.amazon.com/blogs/aws/new-additional-checksum-algorithms-for-amazon-s3/)). | ||
|
||
|
||
|
||
Once this RFC is implemented, users will have the ability to selectively disable payload signing by customizing the | ||
request. | ||
|
||
```rust,ignore | ||
let resp = s3.put_object() | ||
.bucket(bucket_name) | ||
.key(key_name) | ||
.body(body) | ||
.customize() | ||
.disable_payload_signing(); | ||
``` | ||
|
||
This new `disable_payload_signing()` method will only be added selectively to requests with a binary stream as the | ||
payload. | ||
|
||
|
||
How to actually implement this RFC | ||
---------------------------------- | ||
|
||
Each service client operation can be customized to (e.g.) override specific configuration for just that operation | ||
invocation. See [RFC-0017](./rfc0017_customizable_client_operations.html) | ||
for more details. | ||
|
||
In each generated service a [`CustomizableOperation<T, E, B>`](https://github.com/awslabs/aws-sdk-rust/blob/release-2024-04-11/sdk/s3/src/client/customize.rs#L3) | ||
is generated. These are exposed as part of the [fluent builder](https://github.com/awslabs/aws-sdk-rust/blob/release-2024-04-11/sdk/s3/src/operation/put_object/builders.rs#L152) for each operation. | ||
|
||
|
||
A new `impl` block specific to only binary streaming requests will be generated that | ||
exposes this new `disable_payload_signing()` method. | ||
|
||
```rust,ignore | ||
|
||
impl CustomizableOperation< | ||
crate::operation::put_object::PutObjectOutput, | ||
crate::operation::put_object::PutObjectError, | ||
PutObjectFluentBuilder | ||
> { | ||
|
||
/// Disable payload signing for a single operation invocation. | ||
fn disable_payload_signing(mut self) -> Self { | ||
// register a plugin that overrides the default signing parameters | ||
// and sets `signing_options.payload_override` to `SignableBody::UnsignedPayload` | ||
self.runtime_plugins | ||
.push(::aws_runtime::auth::DisablePayloadSigningPlugin::new()); | ||
} | ||
} | ||
|
||
``` | ||
|
||
|
||
|
||
Changes checklist | ||
----------------- | ||
|
||
- [ ] Create a new runtime plugin that disables payload signing | ||
- [ ] Update codegen to generate a specific `impl` block for requests with a `@streaming blob` payload that | ||
registers the new plugin | ||
- [ ] Add new integration test(s) | ||
- [ ] Update developer guide(s) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What services does this actually work for? I was under the impression this only worked for S3.
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.
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).
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.
Yeah, I think we should allow list this until we've verified it actually works in other services.
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.
I'm good with that, will update the RFC to mention this.