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 19 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
Codecov Results 📊✅ 104653 passed | ⏭️ 6692 skipped | Total: 111345 | Pass Rate: 93.99% | Execution Time: 364m 10s 📊 Comparison with Base Branch
All tests are passing successfully. ✅ Patch coverage is 95.92%. Project has 2486 uncovered lines. Files with missing lines (3)
Coverage diff@@ Coverage Diff @@
## main #PR +/-##
==========================================
+ Coverage 90.14% 90.16% +0.02%
==========================================
Files 193 193 —
Lines 25183 25274 +91
Branches 9176 9224 +48
==========================================
+ Hits 22700 22788 +88
- Misses 2483 2486 +3
- Partials 1429 1434 +5Generated by Codecov Action |
| return rv | ||
|
|
||
| def endheaders(self: "HTTPConnection", *args: "Any", **kwargs: "Any") -> "Any": | ||
| trace_headers = getattr(self, "_sentrysdk_trace_headers", ()) |
There was a problem hiding this comment.
Does calling sentry_sdk.get_current_scope().iter_trace_propagation_headers() here instead of in putrequest() work?
It would be best to avoid stashing stuff on the HTTPConnection instance if we can help it.
There was a problem hiding this comment.
Good suggestion :) header generation is moved into endheaders() instead of being stored on the connection.
… 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
| client.meta.events.register("before-sign", _inject_third_party_baggage) | ||
| client.meta.events.register_last( |
There was a problem hiding this comment.
boto3 event handlers leak across tests via shared default session
Event handlers registered on client.meta.events leak across tests because boto3.client() uses the global default session. Create a dedicated boto3.Session() per test, or unregister handlers afterwards.
Evidence
boto3.client()internally uses a global defaultSession, whoseevent_emitteris shared across all callers.tests/integrations/boto3/test_s3.pyavoids this exact pitfall by creating a dedicatedboto3.Session()at module scope.tests/integrations/boto3/aws_mock.pyexplicitly callsmeta.events.unregister('before-send', self)on teardown.- The new test file never unregisters its
before-signhandlers, so handlers from earlier parametrized runs or test functions persist in the shared emitter.
Identified by Warden · code-review · SV9-PJR
There was a problem hiding this comment.
boto3.client() reuses the global session, event emitters are copied. See below for example:
import boto3
c1 = boto3.client("dynamodb")
c2 = boto3.client("dynamodb")
calls = []
def handler(**kwargs):
calls.append(kwargs)
c1.meta.events.register("before-sign", handler)
# handler registered on c1 is not called for c2.
c2.meta.events.emit("before-sign", request=None, signature_version="v4")
assert calls == []
assert c1.meta.events._emitter is not c2.meta.events._emitter…ssues Refs: #7031 & PY-2667
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 & PY-2667
Related issues in dd-trace-py: #19477 & #19358
Reminders
uv run ruff.feat:,fix:,ref:,meta:)