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

Presigning content type bug #2944

Merged
merged 13 commits into from
May 26, 2023
Merged

Conversation

dlm6693
Copy link
Contributor

@dlm6693 dlm6693 commented May 17, 2023

This addresses a bug in URL presigning. For certain operations, like Polly's SynthesizeSpeech, the URL used for presigning does not match what is modeled for the request. Typically it is sent as a POST with API parameters attached to a body, but through presigning, it is a GET with parameters attached in the URL query.

This issue arises in the RestJson serializers where the input params are still treated as a body, so a Content-Type header is injected, causing a signature mismatch in the presigned URL that is returned.

This PR generally removes this header for presigning from requests using methods where bodies should not be present or should have no meaning.

UPDATE: After some issues were discovered when running presigning for the JSON-RPC protocol, we've decided to scale back this solution to only be applied as a customization for Polly's synthesize_speech when generating a presigned URL.

@dlm6693 dlm6693 requested a review from nateprewitt May 17, 2023 19:59
@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (1f46917) 93.29% compared to head (b047736) 93.29%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2944   +/-   ##
========================================
  Coverage    93.29%   93.29%           
========================================
  Files           64       64           
  Lines        13588    13591    +3     
========================================
+ Hits         12677    12680    +3     
  Misses         911      911           
Impacted Files Coverage Δ
botocore/handlers.py 95.21% <100.00%> (+0.03%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dlm6693 dlm6693 requested a review from nateprewitt May 17, 2023 23:07
},
HttpMethod=request_method,
)
assert ('content-type' in url) == content_type_present
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to ask if we could be more specific with this assertion, but it turns out we only add this to the SignedHeaders section and not the query parameters themselves. That means anyone using the URL needs to also have a client to pass the headers. That's likely expected when a body is present, but we should validate this against a couple other SDKs first.

Let's see what Java v2 and JS v3 do in these cases.

Copy link
Contributor Author

@dlm6693 dlm6693 May 18, 2023

Choose a reason for hiding this comment

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

Here's a PutObject example for Java v2. It looks like its expected to manually set headers after generating the presigned URL: https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/examples-s3-presign.html

/ Upload content to the Amazon S3 bucket by using this URL.
URL url = presignedRequest.url();

// Create the connection and use it to upload the new object.
HttpURLConnection connection = (HttpURLConnection) url.openConnection();
connection.setDoOutput(true);
connection.setRequestProperty("Content-Type","text/plain");
connection.setRequestProperty("x-amz-meta-author","Mary Doe");
connection.setRequestProperty("x-amz-meta-version","1.0.0.0");

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 also seeing manually providing headers in the PutObject example in this blog post for the JS v3: https://aws.amazon.com/blogs/developer/generate-presigned-url-modular-aws-sdk-javascript/

const response = await fetch(presignedUrl, {
    method: "PUT",
    body: payload,
    headers: {
         "Content-Length": statSync(filePath).size
    }
});

I can try to look for other examples if you like.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're mostly concerned about the content of the string.

Copy link
Contributor Author

@dlm6693 dlm6693 May 18, 2023

Choose a reason for hiding this comment

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

Sure. Here's a redacted example from JS v3. Note that content-length is in SignedHeaders:

https://bar.s3.us-east-1.amazonaws.com/baz?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Content-
Sha256=UNSIGNED-PAYLOAD&X-Amz-Credential=xxx%2F20230518%2Fus-east-1%2Fs3%2Faws4_request
&X-Amz-Date=20230518T212834Z&X-Amz-Expires=3600&X-Amz-Security-Token=xxx&X-Amz-Signature
=xxx&X-Amz-SignedHeaders=content-length%3Bhost&x-id=PutObject

Copy link
Contributor Author

@dlm6693 dlm6693 May 18, 2023

Choose a reason for hiding this comment

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

Sorry this took awhile, but here's a redacted Java v2 example.

https://foo.s3.amazonaws.com/bar?X-Amz-Security-Token=xxx&X-Amz-Algorithm=AWS4-HMAC-SHA256&X-
Amz-Date=20230518T223024Z&X-Amz-SignedHeaders=host&X-Amz-Expires=600&X-Amz-
Credential=xxx%2F20230518%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Signature=xxx

@dlm6693 dlm6693 force-pushed the presigning-content-type-bug branch from 2ff9ce3 to 4f898bd Compare May 19, 2023 13:48
@dlm6693 dlm6693 force-pushed the presigning-content-type-bug branch from 4f898bd to 560b76d Compare May 24, 2023 21:25
@dlm6693 dlm6693 requested a review from nateprewitt May 24, 2023 21:27
@dlm6693 dlm6693 requested a review from nateprewitt May 25, 2023 21:10
Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

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

🚀

@dlm6693 dlm6693 merged commit 0e9d748 into boto:develop May 26, 2023
@dlm6693 dlm6693 deleted the presigning-content-type-bug branch May 26, 2023 00:34
dlm6693 added a commit to dlm6693/botocore that referenced this pull request May 26, 2023
* Remove `Content-Type` from request_dict in URL presigning if request method shouldn't have a body

* cleanup

* add region to test client

* remove some methods. update tests

* mock patch `generate_presigned_url`

* changelog and comment cleanup

* pr feedback

* only delete header if custom http method provided

* cleanup

* changelog update

* remove scope setting in fixture

* scale back to only remove `Content-Type` header for `synthesize_speech`

* pr feedback
aws-sdk-python-automation added a commit that referenced this pull request May 26, 2023
* release-1.29.142:
  Bumping version to 1.29.142
  Update to latest partitions and endpoints
  Update to latest models
  Presigning content type bug (#2944)
  Accessibility Enhancements (#2949)
dlm6693 added a commit to dlm6693/botocore that referenced this pull request May 30, 2023
* Remove `Content-Type` from request_dict in URL presigning if request method shouldn't have a body

* cleanup

* add region to test client

* remove some methods. update tests

* mock patch `generate_presigned_url`

* changelog and comment cleanup

* pr feedback

* only delete header if custom http method provided

* cleanup

* changelog update

* remove scope setting in fixture

* scale back to only remove `Content-Type` header for `synthesize_speech`

* pr feedback
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.

3 participants