fix(aiohttp): Preserve SigV4-signed propagation headers

[Robinbinu]

Fixes #7426.

Cause

aiobotocore sends botocore's already-signed requests through aiohttp.

Since #7050 the Boto3 integration adds sentry-trace and baggage at before-sign, so both end up named in SignedHeaders. The aiohttp client integration then rewrites them on the way out. For baggage it appends:

if key == BAGGAGE_HEADER_NAME and params.headers.get(BAGGAGE_HEADER_NAME):
    params.headers[key] += "," + value

The value that gets sent no longer matches the value that was signed, so the service rejects the request with SignatureDoesNotMatch.

stdlib already skips propagation headers named in SignedHeaders, also added in #7050. That is why synchronous boto3 is unaffected and only the aiobotocore path breaks. This applies the same rule on the aiohttp side, reusing the existing _get_aws_sigv4_signed_headers_from_authorization_header helper. Headers that are not named in SignedHeaders are still added, so propagation is unchanged.

How I isolated it

Against a real Cloudflare R2 bucket on 2.69.0, holding credentials, botocore 1.43.75 and aiobotocore 3.9.1 constant:

all default integrations       SignatureDoesNotMatch
AioHttpIntegration disabled    OK
Boto3Integration disabled      OK
synchronous boto3              OK
this patch, all integrations   OK

Worth flagging for triage: R2 is not being strict about what may be signed. With no SDK in the picture, botocore signing baggage, sentry-trace, x-koodle-test or x-amz-meta-* is accepted every time. The only problem is the header changing after it was signed.

Why it is easy to miss

Calls only fail while a transaction is active. Work outside one keeps succeeding, so a background index warm at startup completed normally and every request after it failed. That made it look intermittent and credential-shaped rather than like a dependency change. In our case a rebuild moved 2.63.0 to 2.69.0 with no code change and quietly took down all object storage reads and writes.

Tests

test_outgoing_trace_headers_leave_sigv4_signed_headers_alone asserts a signed sentry-trace and baggage pass through untouched. It fails without the fix, with the appended baggage shown above.

test_outgoing_trace_headers_added_when_not_signed covers the other direction: an unsigned baggage is still added alongside a signed sentry-trace, so the guard cannot silently drop propagation.

Full tests/integrations/aiohttp/ passes (145). tests/integrations/boto3/ has 3 failures that are already present on master without this change.

Update

Also covered the presigned URL case. stdlib reads signed header names from the Authorization header and, when there is none, from X-Amz-SignedHeaders in the query string. The first version of this patch only handled the former, so a presigned URL fetched through aiohttp would still have had its signed baggage rewritten. There is now a test for it, which fails without the fix in the same way.

Suite is 146 passing.

[Robinbinu]

Could a maintainer take a look when there is time?

Flagging it because this is a regression in 2.69.0 rather than a feature request, and the failure mode is quiet. S3 calls through aiobotocore fail only while a transaction is active, so object storage stops working while the rest of the service looks healthy and keeps serving. We lost about an hour of writes before tracing it back to the SDK, having first gone down the path of suspecting our own credentials.

The change is small and mirrors the guard stdlib already has, and it comes with a test that fails without it. @pabloDeputter, tagging you since it touches the path added in #7050.

Happy to rework it if you would rather solve it in the Boto3 integration instead, or to drop the second test if it reads as redundant.

[pabloDeputter]

Looks good; there is some behavior that doesn't yet match stdlib. See my comments for more info.

[pabloDeputter]

This only skips headers listed in SignedHeaders, so an existing unsigned sentry-trace is still overwritten. Could we match the behavior in stdlib.py and leave any existing sentry-trace untouched.

[pabloDeputter]

adding a test for this would be nice as well; we could adapt test_aws_http_connection_appends_baggage_but_preserves_sentry_trace from to verify that sentry-trace remains unchanged while unsigned baggage is appended.

[pabloDeputter]

Would be nice to use the same headers_to_skip approach as in the synchronous implementation? It would include an existing sentry-trace + signed propagation headers from the authorization header or uRL.

[Robinbinu]

Good catch on the sentry-trace overwrite, that was a real hole. Pushed aa7050d.

All three points addressed:

Switched to the headers_to_skip shape from stdlib. It now starts with an existing sentry-trace, then adds whichever propagation headers are named in SignedHeaders, read from the Authorization header or from the query string for presigned URLs. The injection loop skips anything in that set, so the behaviour matches what stdlib documents: sentry-trace added if missing and left alone if existing or signed, baggage added if missing, appended if unsigned, left alone if signed.

Added test_outgoing_trace_headers_append_baggage_but_preserve_sentry_trace, adapted from test_aws_http_connection_appends_baggage_but_preserves_sentry_trace. Neither header is signed there, so it covers exactly the case you flagged. It fails on the previous revision with assert 'd3d9ee5b...-1' == 'existing-trace', so it does pin the overwrite rather than just passing.

_SENTRY_HEADER_NAMES is defined in aiohttp.py rather than shared with stdlib, to keep the diff inside the integration under review. It is built from the same two imported constants so the values cannot drift, but say the word and I will hoist it somewhere common instead.

aiohttp suite is 147 passing. The 3 failures in tests/integrations/boto3/test_s3.py (test_streaming[True-True], test_streaming[True-False], test_omit_url_data_if_parsing_fails[True]) are present unchanged on master, I checked with the patch stashed.

Re-verified end to end against a live R2 bucket with all default integrations enabled: LIST and PUT both succeed inside an active transaction.

[Robinbinu]

Also, separately from the technical bits: thank you for the fast turnaround on this.

You reviewed a drive-by PR from someone you have never seen before, within the hour, and caught a case I had already convinced myself was finished. That sentry-trace overwrite would have shipped otherwise, and it would have been a nasty one to find later given how quietly this whole class of bug fails.

Appreciate the time, and sorry for the extra round trip.

[alexander-alderman-webb]

You can share the set that already exists in stdlib.py by putting this in consts.py, for example.

my own review

[Robinbinu]

Thank you @pabloDeputter and @alexander-alderman-webb

[Robinbinu]

The Web 1 (3.7) failure is unrelated to this PR, it is a flaky test. Could someone re-run that job? I do not have rights to.

It failed on tests/integrations/django/test_cache_module.py::test_cache_spans_get_span_name[get-args4-kwargs4-]:

AssertionError: assert 'p{\x16nq&L߯Z\x0cs-L؂' == ''

That parameter set feeds 16 random bytes as the cache key and expects an empty description:

("get", [uuid.uuid4().bytes], {}, ""),

_key_as_string returns "" only when key.decode() raises UnicodeDecodeError. Most random 16-byte strings are not valid UTF-8, so it usually returns "" and the test passes. Occasionally they are valid UTF-8, it decodes to garbage, and the assertion fails.

Measured on this branch, 200k draws:

non-empty: 14 / 200000  (0.0070%)
  example: '\n,۵$1J޸D;i\x10ҡp'
  example: "|_\x1bJ\x08\x08G̾ɚ'žо"

Same shape as the CI failure. It is rare per test, but it runs on every job in the matrix, so it surfaces occasionally on unrelated PRs.

Worth fixing separately if you agree: a fixed byte string that can never decode, for example b"\xff\xfe", would test the same branch deterministically. Happy to open a separate PR for it rather than mixing it in here.