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

only add the content-sha256 header when it's already exists for CrtSigV4QueryAuth and CrtSigV4AsymQueryAuth #3243

Closed
wants to merge 2 commits into from

Conversation

TingDaoK
Copy link

@TingDaoK TingDaoK commented Sep 3, 2024

  • For CRT signer, override _should_add_content_sha256_header for CrtSigV4QueryAuth and CrtSigV4AsymQueryAuth to add the content_sha256 header if the request to sign already have the header, instead of relying on the mix of the configurations and the headers in request to sign.
  • So that it matches the SDK signer behavior to not add the extra x-amz-content-sha256 header when _should_sha256_sign_payload is disabled.

Extra notes:

Will update the CRT version and add tests.

@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 93.12%. Comparing base (e0f04ab) to head (f1e047d).
Report is 52 commits behind head on develop.

Files with missing lines Patch % Lines
botocore/crt/auth.py 0.00% 11 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3243      +/-   ##
===========================================
- Coverage    93.15%   93.12%   -0.04%     
===========================================
  Files           66       66              
  Lines        14287    14291       +4     
===========================================
- Hits         13309    13308       -1     
- Misses         978      983       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -81,7 +81,7 @@ def add_auth(self, request):
else:
explicit_payload = UNSIGNED_PAYLOAD

if self._should_add_content_sha256_header(explicit_payload):
if self._should_add_content_sha256_header(existing_sha256, explicit_payload):
Copy link
Contributor

Choose a reason for hiding this comment

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

We should always add new positional arguments at the end of the function signature. I would also recommend making this an optional parameter that defaults to None.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we also missed an update somewhere? I think this will be resolved if you follow the advice above, but we should figure out where we're missing.

TypeError: _should_add_content_sha256_header() takes 2 positional arguments but 3 were given
```

Copy link
Author

Choose a reason for hiding this comment

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

I saw the error, but I don't understand why... Looks like we are not declaring _should_add_content_sha256_header anywhere else, and I updated the param for _should_add_content_sha256_header in CrtSigV4QueryAuth. Interesting.

Comment on lines +498 to +499
# only add X-Amz-Content-SHA256 header if header already set to UNSIGNED_PAYLOAD
return existing_sha256 == UNSIGNED_PAYLOAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we only adding this if it's unsigned? What if we have an actual payload we're signing for?

Copy link
Author

@TingDaoK TingDaoK Sep 5, 2024

Choose a reason for hiding this comment

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

It's a temporary workaround we want to add for vpc-lattice and not want to introduce the change for others (Even though that it might be more reasonable to add the header to sign for other service as well, and it will match the behavior for the botocore signer). And they only supports UNSIGNED-PAYLOAD

We decided to have an other workaround as the short term patch within CRT now. awslabs/aws-c-auth#249

We will have further discussion about what's the proper way to handle their use case in the near future, and it should be across SDKs not only for CRT.

So, no need for this PR now.

@TingDaoK
Copy link
Author

TingDaoK commented Sep 5, 2024

I'll close this PR as we don't need this change now.

We already kicked off the release for the alternative change in CRT, as long as we pick up the latest version v0.21.4 once it's release, we should be good

@TingDaoK TingDaoK closed this Sep 5, 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.

3 participants