fix: sign the Host header that is actually sent - #81
Open
tedder wants to merge 1 commit into
Open
Conversation
The Host header was signed with the port stripped unconditionally, while the request going out keeps the port whenever it is not the default for the scheme. The two disagreed, so the service computed a different signature and rejected the request with SignatureDoesNotMatch. This is #79, and before it #34. It was fixed once in 8e1417c (#63) and reverted in b328b86 after #65 reported a regression. Two changes, which together make the divergence unrepresentable: - get_host_header() computes the value the client will send: strip the port only when it is the default for that scheme. 443 is default for https but not for http, so http://host:443 keeps its port. IPv6 literals are no longer split on their first colon, which previously reduced https://[::1] to a single "[" regardless of any port. Credentials are stripped, since they sit in the netloc but are never sent in the Host header -- the old code signed everything before the first colon ('user'), and 8e1417c would have signed them along with the port. Neither matches the wire. - __call__ now sets the Host header instead of only signing it. The wire value is byte-identical either way, but signed value and sent value become the same string, so they cannot drift apart again if urllib3 ever changes how it derives the header. Verified against real SigV4 implementations on non-default ports, which is something AWS itself cannot test: every AWS endpoint is on 443, so this code path is unreachable there. RadosGW on 7480 and MinIO on 9111 both reject the old signature and accept the new one. Real AWS S3 is unaffected, including URLs with an explicit :443. The host header table gains the credential-bearing URL shapes, and a new test checks them against a real socket rather than trusting the oracle: the oracle models requests stripping credentials before it opens the connection, and that model should itself be tested. Without the fix it reports "AssertionError: 'user' != '127.0.0.1:52809'". Also flips the assertion added in #68, which was inverted: it asserted the signature did not match the wire, documenting the bug rather than testing the behaviour, while its httpx twin asserted the opposite for the same URL. Fixes #79 Fixes #34 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GKqoiAcwVRjYpwpDKcPqDQ
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Host header was signed with the port stripped unconditionally, while the request going out keeps the port whenever it is not the default for the scheme. The two disagreed, so the service computed a different signature and rejected the request with SignatureDoesNotMatch.
This is #79, and before it #34. It was fixed once in 8e1417c (#63) and reverted in b328b86 after #65 reported a regression.
Two changes, which together make the divergence unrepresentable:
get_host_header() computes the value the client will send: strip the port only when it is the default for that scheme. 443 is default for https but not for http, so http://host:443 keeps its port. IPv6 literals are no longer split on their first colon, which previously reduced https://[::1] to a single "[" regardless of any port. Credentials are stripped, since they sit in the netloc but are never sent in the Host header -- the old code signed everything before the first colon ('user'), and 8e1417c would have signed them along with the port. Neither matches the wire.
call now sets the Host header instead of only signing it. The wire value is byte-identical either way, but signed value and sent value become the same string, so they cannot drift apart again if urllib3 ever changes how it derives the header.
Verified against real SigV4 implementations on non-default ports, which is something AWS itself cannot test: every AWS endpoint is on 443, so this code path is unreachable there. RadosGW on 7480 and MinIO on 9111 both reject the old signature and accept the new one. Real AWS S3 is unaffected, including URLs with an explicit :443.
The host header table gains the credential-bearing URL shapes, and a new test checks them against a real socket rather than trusting the oracle: the oracle models requests stripping credentials before it opens the connection, and that model should itself be tested. Without the fix it reports "AssertionError: 'user' != '127.0.0.1:52809'".
Also flips the assertion added in #68, which was inverted: it asserted the signature did not match the wire, documenting the bug rather than testing the behaviour, while its httpx twin asserted the opposite for the same URL.
Fixes #79
Fixes #34
Claude-Session: https://claude.ai/code/session_01GKqoiAcwVRjYpwpDKcPqDQ