Fix presigning bug with content-length and content-type in S3
#1216
Conversation
|
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
|
|
A new generated diff is ready to view.
|
|
A new doc preview is ready to view. |
CHANGELOG.next.toml
Outdated
| author = "jdisanti" | ||
|
|
||
| [[aws-sdk-rust]] | ||
| message = "Fixed a bug in S3 that prevented the `content-length` and `content-type` inputs from being included in a presigned request signature" |
There was a problem hiding this comment.
a sentence about the impact may be helpful, "With this fix, customers can generate presigned URLs that enforce content-length and content type" or something
| req.headers_mut().remove(USER_AGENT); | ||
| req.headers_mut().remove("X-Amz-User-Agent"); |
There was a problem hiding this comment.
maybe this is hard...this is probably hard...ideally we just wouldn't hit the UA middleware
There was a problem hiding this comment.
We could potentially add something to the property bag that tells the UA middleware to no-op, but I don't think it would be easy to customize the middleware in the presigning function as it currently is.
| use s3::presigning::config::PresigningConfig; | ||
| use std::error::Error; | ||
| use std::time::{Duration, SystemTime}; | ||
|
|
||
| macro_rules! presign_input { |
There was a problem hiding this comment.
comment on test macros is often helpful
| private val public: Boolean = true | ||
| private val public: Boolean = true, | ||
| /** Whether or not to include default values for content-length and content-type */ | ||
| private val includeDefaultPayloadHeaders: Boolean = true, |
There was a problem hiding this comment.
consider removing the default values here—since this function is only invoked in 2 (?) places, explicit is probably clearer on both sides
| @@ -76,16 +79,19 @@ open class MakeOperationGenerator( | |||
| val fnType = if (public) "pub async fn" else "async fn" | |||
|
|
|||
| implBlockWriter.docs("Consumes the builder and constructs an Operation<#D>", outputSymbol) | |||
| Attribute.Custom("allow(unused_mut)").render(implBlockWriter) // For codegen simplicity | |||
There was a problem hiding this comment.
| Attribute.Custom("allow(unused_mut)").render(implBlockWriter) // For codegen simplicity | |
| Attribute.AllowUnusedMut.render(implBlockWriter) // For codegen simplicity |
| *codegenScope | ||
| ) | ||
| } | ||
| for (header in protocol.additionalRequestHeaders(operationShape)) { |
There was a problem hiding this comment.
are these ones we want to always set? I guess so right? this is stuff like operation for JSON RPC?
There was a problem hiding this comment.
Yeah, it looks like it's only used by AWS JSON to add the x-amz-target header, which is needed to identify the operation to the service. We wouldn't want to remove these for this current use-case.
|
Going to add S3 to the codegen diff so that we can see these changes in the PR. |
|
It looks like it's not so simple to just include S3. It should already be in the diff output, but it seems like diff2html is dropping files when the diff is exceedingly large. Going to punt on this for now. |
|
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
|
|
A new doc preview is ready to view. |
|
A new doc preview is ready to view. |
|
A new generated diff is ready to view.
|
Motivation and Context
Description
Rather than remove
content-lengthandcontent-typewhen signing and creating a presigned request, this PR refactorsMakeOperationGeneratorto not produce default values for those headers when making an operation for presigning.Testing
content-lengthto a value in the input, and then verified there is a signing error when uploading a file that is not that length, and that it succeeds with the matching file size.content-typeto a value in the input, and then verified there is a signing error with a differentcontent-type, and that it succeeds when it matches.Checklist
CHANGELOG.next.tomlif I made changes to the AWS SDK, generated SDK code, or SDK runtime cratesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.