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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bbd449a
only add the header when it's already exists
TingDaoK Sep 3, 2024
f1e047d
make the behavior more clear from the Python code instead of buried b…
TingDaoK Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions botocore/crt/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

body_header = (
awscrt.auth.AwsSignedBodyHeaderType.X_AMZ_CONTENT_SHA_256
)
Expand Down Expand Up @@ -169,7 +169,7 @@ def _should_sha256_sign_payload(self, request):
# bit of metadata through the request context.
return request.context.get('payload_signing_enabled', True)

def _should_add_content_sha256_header(self, explicit_payload):
def _should_add_content_sha256_header(self, existing_sha256, explicit_payload):
# only add X-Amz-Content-SHA256 header if payload is explicitly set
return explicit_payload is not None

Expand Down Expand Up @@ -278,7 +278,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):
body_header = (
awscrt.auth.AwsSignedBodyHeaderType.X_AMZ_CONTENT_SHA_256
)
Expand Down Expand Up @@ -371,7 +371,7 @@ def _should_sha256_sign_payload(self, request):
# bit of metadata through the request context.
return request.context.get('payload_signing_enabled', True)

def _should_add_content_sha256_header(self, explicit_payload):
def _should_add_content_sha256_header(self, existing_sha256, explicit_payload):
# only add X-Amz-Content-SHA256 header if payload is explicitly set
return explicit_payload is not None

Expand Down Expand Up @@ -421,7 +421,7 @@ def _should_sha256_sign_payload(self, request):
# checks.
return super()._should_sha256_sign_payload(request)

def _should_add_content_sha256_header(self, explicit_payload):
def _should_add_content_sha256_header(self, existing_sha256, explicit_payload):
# Always add X-Amz-Content-SHA256 header
return True

Expand Down Expand Up @@ -494,6 +494,10 @@ def _apply_signing_changes(self, aws_request, signed_crt_request):
# fragment - 4
aws_request.url = urlunsplit((p[0], p[1], p[2], signed_query, p[4]))

def _should_add_content_sha256_header(self, existing_sha256, explicit_payload):
# only add X-Amz-Content-SHA256 header if header already set to UNSIGNED_PAYLOAD
return existing_sha256 == UNSIGNED_PAYLOAD
Comment on lines +498 to +499
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 •
edited
Loading

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.



class CrtS3SigV4AsymQueryAuth(CrtSigV4AsymQueryAuth):
"""S3 SigV4A auth using query parameters.
Expand All @@ -512,7 +516,7 @@ def _should_sha256_sign_payload(self, request):
# payload. Instead, you use a constant string "UNSIGNED-PAYLOAD".
return False

def _should_add_content_sha256_header(self, explicit_payload):
def _should_add_content_sha256_header(self, existing_sha256, explicit_payload):
# Never add X-Amz-Content-SHA256 header
return False

Expand Down Expand Up @@ -591,6 +595,10 @@ def _apply_signing_changes(self, aws_request, signed_crt_request):
# fragment - 4
aws_request.url = urlunsplit((p[0], p[1], p[2], signed_query, p[4]))

def _should_add_content_sha256_header(self, existing_sha256, explicit_payload):
# only add X-Amz-Content-SHA256 header if header already set to UNSIGNED_PAYLOAD
return existing_sha256 == UNSIGNED_PAYLOAD


class CrtS3SigV4QueryAuth(CrtSigV4QueryAuth):
"""S3 SigV4 auth using query parameters.
Expand All @@ -611,7 +619,7 @@ def _should_sha256_sign_payload(self, request):
# payload. Instead, you use a constant string "UNSIGNED-PAYLOAD".
return False

def _should_add_content_sha256_header(self, explicit_payload):
def _should_add_content_sha256_header(self, existing_sha256, explicit_payload):
# Never add X-Amz-Content-SHA256 header
return False

Expand Down
Loading