Skip to content

fix(bedrock): exclude connection header from SigV4 signing - #3567

Open
lazerg wants to merge 2 commits into
openai:mainfrom
lazerg:fix/issue-3563-bedrock-connection-header
Open

fix(bedrock): exclude connection header from SigV4 signing#3567
lazerg wants to merge 2 commits into
openai:mainfrom
lazerg:fix/issue-3563-bedrock-connection-header

Conversation

@lazerg

@lazerg lazerg commented Aug 2, 2026

Copy link
Copy Markdown
  • I understand that this repository is auto-generated and my pull request may not be merged

Changes being requested

BedrockAwsAuth.sign passes the hop-by-hop Connection header to the SigV4 signer. Proxies like Bedrock Mantle rewrite or drop that header on the way through, so Bedrock rebuilds a different canonical request and rejects valid AWS credentials with a 401 signature mismatch. This drops connection before signing, the same way botocore leaves hop-by-hop headers out of the signature.

src/openai/lib/ is hand-written and the generator does not touch it, per CONTRIBUTING.md.

Additional context & links

Fixes #3563

@lazerg
lazerg requested a review from a team as a code owner August 2, 2026 15:59

@jbeckwith-oai jbeckwith-oai 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.

[P2] Cover the complete volatile-header set supported by this dependency range

The new _HOP_BY_HOP_HEADERS tuple contains only connection, but the Bedrock extra still supports botocore>=1.40.0. Older supported botocore releases do not exclude keep-alive, proxy-authenticate, proxy-authorization, te, trailer, or upgrade. With botocore 1.40.61 at this head, a request containing those fields produced SignedHeaders=content-type;host;keep-alive;proxy-authenticate;proxy-authorization;te;trailer;upgrade;..., so an intermediary rewriting any of them causes the same 401 signature mismatch this PR is meant to fix. Connection can also nominate additional hop-by-hop field names that intermediaries must remove.

AWS explicitly says not to sign the full volatile transport-header set: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-create-signed-request.html. Botocore addressed the same gap comprehensively in boto/botocore#3643. Please either exclude the complete set (including case variants and Connection-nominated fields) or raise the botocore floor to a release containing that upstream fix, and parameterize the regression test across the set.

Also, the current implementation removes Connection from the outgoing request entirely because the caller later replaces its headers with auth.sign() output. The stated requirement is to exclude it from SignedHeaders; botocore preserves volatile headers while leaving them unsigned. Please preserve an explicit caller Connection: close unless dropping that transport instruction is intentional and covered as an API change.

The targeted conformance suite passes 21/21, both sync and async paths exclude mixed-case Connection in an end-to-end mock transport check, and Ruff, formatting, Pyright, and diff checks are clean. GitHub reports no CI checks for this branch.

@lazerg

lazerg commented Aug 4, 2026

Copy link
Copy Markdown
Author

@jbeckwith-oai thanks, this was a real gap. Verified each point against botocore source and the AWS doc, and pushed a fix.

What I verified

  • AWS's SigV4 guidance (https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_sigv-create-signed-request.html, "Create a canonical request") states directly: "Do not include hop-by-hop headers that are frequently altered during transit... including connection, x-amzn-trace-id, user-agent, keep-alive, transfer-encoding, TE, trailer, upgrade, proxy-authorization, and proxy-authenticate."
  • botocore codifies exactly this in botocore/auth.py as SIGNED_HEADERS_BLACKLIST (connection, expect, keep-alive, proxy-authenticate, proxy-authorization, te, trailer, transfer-encoding, upgrade, user-agent, x-amzn-trace-id), checked in the pinned 1.42.97. SigV4Auth.headers_to_sign() filters that set from the signature while leaving the headers on the request, so botocore signs but never removes them.
  • The gap is real across our floor. That full blacklist only exists from Exclude additional hop-by-hop headers from SigV4 signing boto/botocore#3643 onward, which postdates releases inside botocore>=1.40.0; as you observed, 1.40.61 still signs keep-alive and the rest. So the previous single-connection tuple left the other volatile headers to be signed by any older-but-supported botocore, which is the same 401 exposure.

What I changed

  • Expanded the client-side exclusion set to the complete blacklist above (src/openai/lib/_bedrock_auth.py, _UNSIGNED_HEADERS), mirroring botocore verbatim. I kept this client-side rather than raising the botocore floor so behavior is deterministic across the whole supported range without forcing a dependency bump on users; on newer botocore it is simply redundant with the upstream filter.
  • Case handling: matching is on name.lower(), so mixed-case variants are covered. The regression test now feeds each header in Title-Case (Keep-Alive, Proxy-Authorization, Te, ...) to prove it.
  • Preserving the caller's transport instruction: fixed. sign() now re-adds the caller's volatile headers to its output after signing, so an explicit Connection: close stays on the outgoing request (unsigned) instead of being dropped. This matches botocore's "leave on the request, exclude from the signature" semantics. Previously the header was gone because the caller replaces its headers with the sign() output.
  • Tests parameterized across all 11 headers, each asserted both absent from SignedHeaders and preserved in the returned headers with its value intact.

On Connection-nominated field names (RFC 7230 6.1)

I did not implement parsing the Connection value to treat nominated field names as hop-by-hop, and I think that is correct to leave out here. botocore's own comprehensive fix (#3643) uses the fixed blacklist and does not parse Connection to nominate additional names, and the AWS doc does not require it. Excluding a nominated header client-side that botocore would still sign would make our SignedHeaders diverge from the reference signer, so matching botocore's fixed set is the safer contract. Happy to add it if you consider it in scope, but flagging that it goes beyond what upstream does.

Bedrock conformance suite is green (31 passed, both sync and async paths). Ruff, ruff format, and Pyright are clean.

@lazerg
lazerg requested a review from jbeckwith-oai August 4, 2026 00:44
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.

Bedrock SigV4 signing includes Connection header, causing 401 signature mismatch through Mantle

2 participants