fix(boto3): Fix botocore SigV4 failures caused by post-sign trace propagation - #7050
fix(boto3): Fix botocore SigV4 failures caused by post-sign trace propagation#7050pabloDeputter wants to merge 38 commits into
Conversation
- merge Sentry baggage with existing vendor (e.g. Datadog) baggage in botocore's`before-sign` hook; avoiding post-sign header tampering that invalidates the SigV4 signature. - Skip propagation for presigned requests Fixes: #7031 & PY-2667
This comment was marked as outdated.
This comment was marked as outdated.
… SigV4 headers Refs: #7031 & PY-2667
|
I haven't forgot about this, it's just complex so I'll likely only re-review fully at the start of next week. |
…` + support for SigV4 query/presigned authentication Refs: #7031 & PY-2667
…ssues Refs: #7031 & PY-2667
…` since it's not used anymore
…ion` string instead of all headers
- Record existing and signed headers in `putheader()` so trace propagation can avoid reparsing `_buffer` on every request. Refs: #7031 & PY-2667
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3ce7587. Configure here.
alexander-alderman-webb
left a comment
There was a problem hiding this comment.
Next round 😃.
I've also pushed directly to your branch to limit the behavior to the affected HTTPConnection subclasses. I remove the changes to aiohttp as well, they can be in a separate PR when we get around to it (this one is big enough).
ericapisani
left a comment
There was a problem hiding this comment.
Still working through the changes but have some initial comments
| return | ||
|
|
||
| def _replace_header(request: "AWSRequest", key: str, value: str) -> None: | ||
| # HTTPHeaders appends on assignment, so delete existing values first. |
There was a problem hiding this comment.
Because there's subtly different ways that headers can be handled when multiple values are set on the same header key, I think we should add a bit more context to this comment.
It'd be worth clarifying what "append" (ideally with a concrete example) means as it could be interpreted as:
- adding to a list (e.g: [foo, bar, baz])
- appending to a comma separated string (e.g. foo, bar, baz)
And I think it may also be worth pointing out that this is coming from email.message.Message from the standard library as this is what the botocore package is using under the hood.
There was a problem hiding this comment.
Updated the comment to clarify that it creates another header field rather than extending, @alexander-alderman-webb was also confused about this 😄
| "build": sys.version, | ||
| } | ||
|
|
||
| _SENTRY_HEADER_NAMES = frozenset((BAGGAGE_HEADER_NAME, "sentry-trace")) |
There was a problem hiding this comment.
We also have a constant for sentry-trace that you can use here
| _SENTRY_HEADER_NAMES = frozenset((BAGGAGE_HEADER_NAME, "sentry-trace")) | |
| _SENTRY_HEADER_NAMES = frozenset((BAGGAGE_HEADER_NAME, SENTRY_TRACE_HEADER_NAME)) |
There was a problem hiding this comment.
Yup good idea, didn't know this existed
| original_putheader: "Callable[..., Any]", | ||
| ) -> "Callable[..., Any]": | ||
| """ | ||
| Responsible for tracking request and signed headers. |
There was a problem hiding this comment.
Similar to the other comment, I think we should point future devs reading this to the specific "signed headers" that are being referred to here (in this case botocore's SignedHeaders).
| Responsible for tracking request and signed headers. | |
| Responsible for tracking request and `SignedHeaders`. |
There was a problem hiding this comment.
Yup, changed it to "Responsible for tracking which sentry headers are present and whether they are listed in AWS SigV4 SignedHeaders."
| ) | ||
|
|
||
|
|
||
| def _get_aws_sigv4_signed_headers( |
There was a problem hiding this comment.
Assuming I'm understanding this correctly, we've got 2 distinct code paths here:
- searching for the signed headers in the Authorization header (lines 1705-1716)
- searching for the signed headers in the URL's query string (lines 1718-1734)
If this were a public API and we wanted to optimize for user convenience, I can see the value of supporting the passing in both authorization and url and then searching in the correct place based on if the value is present or not, but because this is strictly for internal use, I think it'd be cleaner to have two distinct helper functions for each path:
_get_aws_sigv4_signed_headers_from_authorization_header_get_aws_sigv4_signed_headers_from_url_query_string
since at the calling sites, we already know where these headers are.
There was a problem hiding this comment.
agreed, I left it like that because originally we didn't read from the query string. I split it into 2 separate functions.
| if authorization is not None: | ||
| # only AWS SigV4 authorization has the SignedHeaders parameter. | ||
| value = authorization.lstrip() | ||
| if value.startswith(("AWS4-HMAC-SHA256", "AWS4-ECDSA-P256-SHA256")): |
There was a problem hiding this comment.
Although these aren't used very widely in the broader SDK codebase, what are your thoughts on doing something similar to the _SENTRY_HEADER_NAMES constant with the SigV4 algorithms?
_AWS_SIGV4_SIGNING_ALGORITHMS = frozenset(("AWS4-HMAC-SHA256", "AWS4-ECDSA-P256-SHA256"))
There was a problem hiding this comment.
I think that's okay. I'm doing it in stdlib.py with _SENTRY_HEADER_NAMES as well. Not sure whether we have a separate file for such constants, but I put it at the top of utils.py.
| with capture_internal_exceptions(): | ||
| authorization = values[0] | ||
| if isinstance(authorization, bytes): | ||
| authorization = authorization.decode("ascii", "ignore") |
There was a problem hiding this comment.
To mirror the codecs used in the putheader method - it looks like we want latin-1 decoding instead of ascii here.
| authorization = authorization.decode("ascii", "ignore") | |
| authorization = authorization.decode("latin-1", "ignore") |

Description
Summary of issue
baggagewas not included inSignedHeaders. Any later modifications to the value did not invalidate the request.before-signevent. It addsbaggage, ... andx-datadog-*before signing. Any later modifications to the value DO invalidate the request, thus later HTTP-client injection is suppressed to avoid duplicate headers.before-signhandler writes the baggage to the AWS requestbaggagein the SigV4 signaturebaggagevalue403 ForbiddenorSignatureDoesNotMatch.Changes
before-signhandler, so finalbaggageandsentry-tracevalues are created before SigV4 signing.http.clientpropagation is delayed untilendheaders(), when the complete request headers and SigV4SignedHeadersare available. Existingbaggageheader is never mutated after it already was signed.Issues
Resolves: #7031 & #7031
Related issues in dd-trace-py: #19477 & #19358
Reminders
uv run ruff.feat:,fix:,ref:,meta:)