Skip to content

Conversation

jasdel
Copy link
Contributor

@jasdel jasdel commented Jun 7, 2018

Fixes the bug which would allow the X-Amz-Content-Sha256 header to be
promoted to the query string when presigning a S3 request.

This bug also was preventing users from setting their own sha256 value
for a presigned URL. Presigned requests generated with the custom sha256
would of always failed with invalid signature.

S3 presign requests without a user specified X-Amz-Content-Sha256 will
no longer include the X-Amz-Content-Sha256 in the header nor query. The
X-Amz-Content-Sha256 header only needs to be set if it contains a non
UNSIGNED-PAYLOAD value.

Related to aws/aws-sdk-go#1974

@jasdel jasdel self-assigned this Jun 7, 2018
@jasdel jasdel requested a review from xibz June 7, 2018 20:03
hash := req.Header.Get("X-Amz-Content-Sha256")
if e, a := "UNSIGNED-PAYLOAD", hash; e != a {
t.Errorf("expect %v, got %v", e, a)
if a := req.Header.Get("X-Amz-Content-Sha256"); len(a) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a test where the service isn't "s3"? To ensure it is included during presigning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added additional tests to cover these cases.

Fixes the bug which would allow the X-Amz-Content-Sha256 header to be
promoted to the query string when presigning a S3 request.

This bug also was preventing users from setting their own sha256 value
for a presigned URL. Presigned requests generated with the custom sha256
would of always failed with invalid signature.

S3 presign requests without a user specified X-Amz-Content-Sha256 will
no longer include the X-Amz-Content-Sha256 in the header nor query. The
X-Amz-Content-Sha256 header only needs to be set if it contains a non
UNSIGNED-PAYLOAD value.

Related to aws/aws-sdk-go#1974
@jasdel jasdel force-pushed the bug/S3ContentSha branch from 26d8fee to 62ff400 Compare June 12, 2018 19:50
@jasdel jasdel merged commit f401f4e into aws:master Jun 12, 2018
@jasdel jasdel deleted the bug/S3ContentSha branch June 12, 2018 20:48
jasdel added a commit to jasdel/aws-sdk-go-v2 that referenced this pull request Sep 27, 2018
* Synced the V2 SDK with latests AWS service API definitions.

* Fix SDK Go 1.11 connection reset handling (aws#207)
	* Fixes how the SDK checks for connection reset errors when making API calls to be compatiable with Go 1.11.
* `aws/signer/v4`: Fix X-Amz-Content-Sha256 being in to query for presign (aws#188)
	* Fixes the bug which would allow the X-Amz-Content-Sha256 header to be promoted to the query string when presigning a S3 request.  This bug also was preventing users from setting their own sha256 value for a presigned URL. Presigned requests generated with the custom sha256 would of always failed with invalid signature.
	* Related to aws/aws-sdk-go#1974

* Cleanup SDK README and example documenation.
* `service/s3/s3manager`: Add doc for sequential download (aws#201)
	Adds documentation for downloading object sequentially with the S3 download manager.
* `aws/credentials`: Update Credentials cache to have less overhead (aws#184)
	* Updates the Credentials type's cache of the CredentialsValue to be synchronized with an atomic value in addition to the Mutex. This reduces the overhead applications will encounter when many concurrent API requests are being made.
	* Related to: aws/aws-sdk-go#1973
* `service/dynamodb/dynamodbattribute`: Add support for custom struct tag keys (aws#203)
	* Adds support for (un)marshaling Go types using custom struct tag keys. The new `MarshalOptions.TagKey` allows the user to specify the tag key to use when (un)marshaling struct fields.  Adds support for struct tags such as `yaml`, `toml`, etc. Support for these keys are in name only, and require the tag value format and values to be supported by the package's Marshalers.
* `internal/ini`: Add custom INI parser for shared config/credentials file (aws#209)
	* Related to: aws/aws-sdk-go#2024
jasdel added a commit that referenced this pull request Sep 27, 2018
### Services
* Synced the V2 SDK with latests AWS service API definitions.

### SDK Bugs
* Fix SDK Go 1.11 connection reset handling (#207)
	* Fixes how the SDK checks for connection reset errors when making API calls to be compatible with Go 1.11.
* `aws/signer/v4`: Fix X-Amz-Content-Sha256 being in to query for presign (#188)
	* Fixes the bug which would allow the X-Amz-Content-Sha256 header to be promoted to the query string when presigning a S3 request.  This bug also was preventing users from setting their own sha256 value for a presigned URL. Presigned requests generated with the custom sha256 would of always failed with invalid signature.
	* Related to aws/aws-sdk-go#1974

### SDK Enhancements
* Cleanup SDK README and example documentation.
* `service/s3/s3manager`: Add doc for sequential download (#201)
	Adds documentation for downloading object sequentially with the S3 download manager.
* `aws/credentials`: Update Credentials cache to have less overhead (#184)
	* Updates the Credentials type's cache of the CredentialsValue to be synchronized with an atomic value in addition to the Mutex. This reduces the overhead applications will encounter when many concurrent API requests are being made.
	* Related to: aws/aws-sdk-go#1973
* `service/dynamodb/dynamodbattribute`: Add support for custom struct tag keys (#203)
	* Adds support for (un)marshaling Go types using custom struct tag keys. The new `MarshalOptions.TagKey` allows the user to specify the tag key to use when (un)marshaling struct fields.  Adds support for struct tags such as `yaml`, `toml`, etc. Support for these keys are in name only, and require the tag value format and values to be supported by the package's Marshalers.
* `internal/ini`: Add custom INI parser for shared config/credentials file (#209)
	* Related to: aws/aws-sdk-go#2024
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