Skip to content

fix(boto3): Fix botocore SigV4 failures caused by post-sign trace propagation - #7050

Open
pabloDeputter wants to merge 38 commits into
masterfrom
pablo/fix-botocore-sigv4-trace-propagation
Open

fix(boto3): Fix botocore SigV4 failures caused by post-sign trace propagation#7050
pabloDeputter wants to merge 38 commits into
masterfrom
pablo/fix-botocore-sigv4-trace-propagation

Conversation

@pabloDeputter

@pabloDeputter pabloDeputter commented Aug 5, 2026

Copy link
Copy Markdown
Member

Description

Summary of issue

  • pre-4.12.0 ddtrace basically behaved like Sentry before: propagation headers were added by the HTTP client after botocore already had calculated the SigV4 signature. So baggage was not included in SignedHeaders. Any later modifications to the value did not invalidate the request.
  • 4.12.0 moves the header injection to botocore's before-sign event. It adds baggage, ... and x-datadog-* before signing. Any later modifications to the value DO invalidate the request, thus later HTTP-client injection is suppressed to avoid duplicate headers.
  • How failure happens:
    1. incoming FastAPI request contains Sentry or other W3C baggage
    2. datadog extracts that baggage into its active context
    3. boto3 request is created
    4. datadog’s before-sign handler writes the baggage to the AWS request
    5. botocore includes baggage in the SigV4 signature
    6. sentry’s httplib runs afterwards and updates or adds another baggage value
    7. AWS receives a different baggage value from the one that was signed. Because request was "tampered" with after signing, AWS rejects the request with 403 Forbidden or SignatureDoesNotMatch.

Changes

  • inject sentry propagation through before-sign handler, so final baggage and sentry-trace values are created before SigV4 signing.
  • existing third-party baggage is merged into one header.
  • http.client propagation is delayed until endheaders(), when the complete request headers and SigV4 SignedHeaders are available. Existing baggage header is never mutated after it already was signed.
  • propagation is skipped for presigned requests, since adding headers could require a future caller to reproduce these exact headers.

Issues

Resolves: #7031 & #7031
Related issues in dd-trace-py: #19477 & #19358

Reminders

- 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
@github-actions

This comment was marked as outdated.

- trace-header injection is delayed until `endheaders()` after all request headers are available.
- avoid duplicate propagation headers.
- no tampering/modifying of already signed baggage

Fixes: #7031 & PY-2667
@pabloDeputter
pabloDeputter marked this pull request as ready for review August 6, 2026 11:12
@pabloDeputter
pabloDeputter requested a review from a team as a code owner August 6, 2026 11:12
Comment thread sentry_sdk/integrations/stdlib.py Outdated
Comment thread sentry_sdk/integrations/stdlib.py Outdated
Comment thread sentry_sdk/integrations/stdlib.py Outdated
Comment thread sentry_sdk/integrations/stdlib.py Outdated
@alexander-alderman-webb

Copy link
Copy Markdown
Contributor

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
Comment thread sentry_sdk/integrations/stdlib.py Outdated
Comment thread tests/integrations/boto3/test_trace_propagation.py

@alexander-alderman-webb alexander-alderman-webb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks really good broadly as fixes the issue without disrupting existing users (including those that not using boto3).

My comments are mainly concerned with the patching approach. It looks more complex than needed to achieve the result.

Comment thread sentry_sdk/integrations/stdlib.py Outdated
Comment thread sentry_sdk/integrations/stdlib.py Outdated
Comment thread sentry_sdk/integrations/boto3.py
- Record existing and signed headers in `putheader()` so trace
propagation can avoid reparsing `_buffer` on every request.

Refs: #7031 & PY-2667
Comment thread sentry_sdk/integrations/stdlib.py Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ 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.

Comment thread sentry_sdk/integrations/stdlib.py
Comment thread sentry_sdk/integrations/stdlib.py

@alexander-alderman-webb alexander-alderman-webb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

Comment thread sentry_sdk/utils.py Outdated
Comment thread tests/test_utils.py Outdated
Comment thread tests/integrations/stdlib/test_httplib.py Outdated
Comment thread sentry_sdk/integrations/stdlib.py Outdated
Comment thread sentry_sdk/integrations/stdlib.py
Comment thread sentry_sdk/integrations/boto3.py
Comment thread sentry_sdk/integrations/stdlib.py Outdated

@ericapisani ericapisani left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still working through the changes but have some initial comments

Comment thread sentry_sdk/integrations/boto3.py Outdated
return

def _replace_header(request: "AWSRequest", key: str, value: str) -> None:
# HTTPHeaders appends on assignment, so delete existing values first.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated the comment to clarify that it creates another header field rather than extending, @alexander-alderman-webb was also confused about this 😄

Comment thread sentry_sdk/integrations/stdlib.py Outdated
"build": sys.version,
}

_SENTRY_HEADER_NAMES = frozenset((BAGGAGE_HEADER_NAME, "sentry-trace"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We also have a constant for sentry-trace that you can use here

Suggested change
_SENTRY_HEADER_NAMES = frozenset((BAGGAGE_HEADER_NAME, "sentry-trace"))
_SENTRY_HEADER_NAMES = frozenset((BAGGAGE_HEADER_NAME, SENTRY_TRACE_HEADER_NAME))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup good idea, didn't know this existed

Comment thread sentry_sdk/integrations/stdlib.py Outdated
original_putheader: "Callable[..., Any]",
) -> "Callable[..., Any]":
"""
Responsible for tracking request and signed headers.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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).

Suggested change
Responsible for tracking request and signed headers.
Responsible for tracking request and `SignedHeaders`.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup, changed it to "Responsible for tracking which sentry headers are present and whether they are listed in AWS SigV4 SignedHeaders."

Comment thread sentry_sdk/utils.py Outdated
)


def _get_aws_sigv4_signed_headers(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

agreed, I left it like that because originally we didn't read from the query string. I split it into 2 separate functions.

Comment thread sentry_sdk/utils.py Outdated
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")):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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"))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Comment thread sentry_sdk/integrations/stdlib.py Outdated
with capture_internal_exceptions():
authorization = values[0]
if isinstance(authorization, bytes):
authorization = authorization.decode("ascii", "ignore")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To mirror the codecs used in the putheader method - it looks like we want latin-1 decoding instead of ascii here.

Suggested change
authorization = authorization.decode("ascii", "ignore")
authorization = authorization.decode("latin-1", "ignore")

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.

S3 HeadObject returns 403 when sentry_sdk.start_span and Datadog botocore instrumentation are both active

3 participants