Repository navigation
Conversation
Hostnames like 'myhost.internal.' (with a trailing dot) are valid FQDNs used to mark fully-qualified names in DNS. However, TLS certificates use 'myhost.internal' (without the dot), so passing the raw FQDN through to the SSL handshake causes CERTIFICATE_VERIFY_FAILED: Host name mismatch. Add Origin.normalized_host as the single source of truth: it decodes the host bytes to ASCII and strips any trailing FQDN dot. All connection, proxy, and SOCKS callsites now use origin.normalized_host, and Origin.__str__ does the same. Raw Origin.host bytes are preserved. Fixes encode/httpcore#1063. Adopts the approach of upstream PR encode/httpcore#1068 (same author as the httpcorexyz fork commit); see also alternative proposals pydantic#1067, pydantic#1074, pydantic#1086. Co-Authored-By: naarob <laforge@forge-sync.local>
Merging this PR will not alter performance
Comparing 
Footnotes
|
mbeijen
changed the title
…dn-sni # Conflicts: # src/httpcore2/httpcore2/_async/http_proxy.py # src/httpcore2/httpcore2/_sync/http_proxy.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/httpcore2/test_models.py">
<violation number="1" location="tests/httpcore2/test_models.py:69">
P3: The docstring and test name say `URL.host` strips the trailing dot, but the assertion checks that `url.host` keeps it (`b"myhost.internal."`). Rename the test to `test_url_host_preserves_trailing_dot` and fix the docstring to say the raw host keeps the dot and only `Origin.__str__` strips it.</violation>
</file>
<file name="tests/httpcore2/_async/test_trailing_dot.py">
<violation number="1" location="tests/httpcore2/_async/test_trailing_dot.py:50">
P2: `connect_tcp` ignores its `host` argument, so the test never checks the DNS/connect path this PR changes. Record `host` in the backend and assert it equals `"myhost.internal"`.</violation>
</file>
<file name="src/httpcore2/httpcore2/_sync/socks_proxy.py">
<violation number="1" location="src/httpcore2/httpcore2/_sync/socks_proxy.py:252">
P3: The `sni_hostname` override bypasses the new normalization. An explicit `sni_hostname="myhost.internal."` still sends the trailing dot to `start_tls`, which reproduces the Host name mismatch this PR fixes. Consider stripping the trailing dot from the override too, or documenting that the override must not include it.</violation>
<violation number="2" location="src/httpcore2/httpcore2/_sync/socks_proxy.py:252">
P2: The three SOCKS5 changes (proxy connect host, SOCKS5 CONNECT target host, and `server_hostname`) have no test. Add a case to the SOCKS test files with an origin like `https://myhost.internal./` that asserts the stripped host reaches `connect_tcp`, the SOCKS5 request, and `start_tls`.</violation>
</file>
<file name="src/httpcore2/httpcore2/_models.py">
<violation number="1" location="src/httpcore2/httpcore2/_models.py:173">
P2: Stripping the final dot before DNS resolution turns an absolute hostname into a search-list candidate. With resolver settings such as `ndots:5`, `myhost.internal.` may resolve as `myhost.internal.<search-domain>` instead of the requested host; preserve the dot for DNS/SOCKS resolution and strip it only for TLS SNI.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| local_address: str | None = None, | ||
| socket_options: typing.Iterable[SOCKET_OPTION] | None = None, | ||
| ) -> AsyncNetworkStream: | ||
| self.stream = RecordingAsyncStream(list(self._buffer)) |
There was a problem hiding this comment.
P2: connect_tcp ignores its host argument, so the test never checks the DNS/connect path this PR changes. Record host in the backend and assert it equals "myhost.internal".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/httpcore2/_async/test_trailing_dot.py, line 50:
<comment>`connect_tcp` ignores its `host` argument, so the test never checks the DNS/connect path this PR changes. Record `host` in the backend and assert it equals `"myhost.internal"`.</comment>
<file context>
@@ -0,0 +1,70 @@
+ local_address: str | None = None,
+ socket_options: typing.Iterable[SOCKET_OPTION] | None = None,
+ ) -> AsyncNetworkStream:
+ self.stream = RecordingAsyncStream(list(self._buffer))
+ return self.stream
+
</file context>
| kwargs = { | ||
| "ssl_context": ssl_context, | ||
| "server_hostname": sni_hostname or self._remote_origin.host.decode("ascii"), | ||
| "server_hostname": sni_hostname or self._remote_origin.normalized_host, |
There was a problem hiding this comment.
P2: The three SOCKS5 changes (proxy connect host, SOCKS5 CONNECT target host, and server_hostname) have no test. Add a case to the SOCKS test files with an origin like https://myhost.internal./ that asserts the stripped host reaches connect_tcp, the SOCKS5 request, and start_tls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/httpcore2/httpcore2/_sync/socks_proxy.py, line 252:
<comment>The three SOCKS5 changes (proxy connect host, SOCKS5 CONNECT target host, and `server_hostname`) have no test. Add a case to the SOCKS test files with an origin like `https://myhost.internal./` that asserts the stripped host reaches `connect_tcp`, the SOCKS5 request, and `start_tls`.</comment>
<file context>
@@ -249,7 +249,7 @@ def handle_request(self, request: Request) -> Response:
kwargs = {
"ssl_context": ssl_context,
- "server_hostname": sni_hostname or self._remote_origin.host.decode("ascii"),
+ "server_hostname": sni_hostname or self._remote_origin.normalized_host,
"timeout": timeout,
}
</file context>
| FQDN like `myhost.internal.` does not cause certificate verification | ||
| failures. The raw `host` bytes are left untouched. | ||
| """ | ||
| return self.host.decode("ascii").removesuffix(".") |
There was a problem hiding this comment.
P2: Stripping the final dot before DNS resolution turns an absolute hostname into a search-list candidate. With resolver settings such as ndots:5, myhost.internal. may resolve as myhost.internal.<search-domain> instead of the requested host; preserve the dot for DNS/SOCKS resolution and strip it only for TLS SNI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/httpcore2/httpcore2/_models.py, line 173:
<comment>Stripping the final dot before DNS resolution turns an absolute hostname into a search-list candidate. With resolver settings such as `ndots:5`, `myhost.internal.` may resolve as `myhost.internal.<search-domain>` instead of the requested host; preserve the dot for DNS/SOCKS resolution and strip it only for TLS SNI.</comment>
<file context>
@@ -161,6 +161,17 @@ def __init__(self, scheme: bytes, host: bytes, port: int) -> None:
+ FQDN like `myhost.internal.` does not cause certificate verification
+ failures. The raw `host` bytes are left untouched.
+ """
+ return self.host.decode("ascii").removesuffix(".")
+
def __eq__(self, other: typing.Any) -> bool:
</file context>
|
|
||
|
|
||
| def test_url_host_strips_trailing_dot() -> None: | ||
| """URL.host used for SNI should not carry the trailing dot.""" |
There was a problem hiding this comment.
P3: The docstring and test name say URL.host strips the trailing dot, but the assertion checks that url.host keeps it (b"myhost.internal."). Rename the test to test_url_host_preserves_trailing_dot and fix the docstring to say the raw host keeps the dot and only Origin.__str__ strips it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/httpcore2/test_models.py, line 69:
<comment>The docstring and test name say `URL.host` strips the trailing dot, but the assertion checks that `url.host` keeps it (`b"myhost.internal."`). Rename the test to `test_url_host_preserves_trailing_dot` and fix the docstring to say the raw host keeps the dot and only `Origin.__str__` strips it.</comment>
<file context>
@@ -49,6 +49,31 @@ def test_url_origin_socks5() -> None:
+
+
+def test_url_host_strips_trailing_dot() -> None:
+ """URL.host used for SNI should not carry the trailing dot."""
+ url = httpcore2.URL("https://myhost.internal.:8443/")
+ assert url.host == b"myhost.internal."
</file context>
| """URL.host used for SNI should not carry the trailing dot.""" | |
| """URL.host keeps the raw trailing dot; Origin.__str__ strips it for SNI.""" |
| kwargs = { | ||
| "ssl_context": ssl_context, | ||
| "server_hostname": sni_hostname or self._remote_origin.host.decode("ascii"), | ||
| "server_hostname": sni_hostname or self._remote_origin.normalized_host, |
There was a problem hiding this comment.
P3: The sni_hostname override bypasses the new normalization. An explicit sni_hostname="myhost.internal." still sends the trailing dot to start_tls, which reproduces the Host name mismatch this PR fixes. Consider stripping the trailing dot from the override too, or documenting that the override must not include it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/httpcore2/httpcore2/_sync/socks_proxy.py, line 252:
<comment>The `sni_hostname` override bypasses the new normalization. An explicit `sni_hostname="myhost.internal."` still sends the trailing dot to `start_tls`, which reproduces the Host name mismatch this PR fixes. Consider stripping the trailing dot from the override too, or documenting that the override must not include it.</comment>
<file context>
@@ -249,7 +249,7 @@ def handle_request(self, request: Request) -> Response:
kwargs = {
"ssl_context": ssl_context,
- "server_hostname": sni_hostname or self._remote_origin.host.decode("ascii"),
+ "server_hostname": sni_hostname or self._remote_origin.normalized_host,
"timeout": timeout,
}
</file context>
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| if self._uds is None: | ||
| kwargs = { | ||
| "host": self._origin.host.decode("ascii"), | ||
| "host": self._origin.normalized_host, |
There was a problem hiding this comment.
Medium: Preserve absolute hostnames during DNS resolution
With a DNS search suffix and ndots:5, stripping the dot changes http://api.example.com./ from an absolute lookup into a search-path lookup, allowing an attacker controlling api.example.com.<search-suffix> to receive the request, including Authorization headers and request bodies. Preserve the raw hostname for TCP and SOCKS destination resolution in both sync and async paths; strip the dot only for TLS server_hostname.
| "host": self._origin.normalized_host, | |
| "host": self._origin.host.decode("ascii"), |
PR overviewUpdates hostname handling to strip trailing dots from fully qualified domain names for TLS SNI. One issue remains open: removing the trailing dot before destination resolution can turn an absolute hostname into a DNS search-path lookup. With a search suffix and settings such as ndots:5, an attacker controlling the resulting search-suffixed hostname could receive requests, including Authorization headers and bodies. The original hostname needs to be preserved for TCP and SOCKS resolution, with dot stripping limited to TLS SNI. Open issues (1)
Fixed/addressed: 0 · PR risk: 6/10 |
…still open; pydantic#1007 updated
A request to an FQDN with a trailing dot, such as
https://myhost.internal./, is valid DNS. But the dot was passed through to TLS SNI and certificate verification, which fail withCERTIFICATE_VERIFY_FAILED: Host name mismatchbecause certificates never carry the dot. The rawOrigin.hostbytes are kept as-is, so connection pooling and equality are unchanged; only the hostname handed to the network layer is normalized.Fixes encode/httpcore#1063. Follows the approach of encode/httpcore#1068 (naarob, also the author of the httpcorexyz commit this is ported from). Alternatives were proposed in encode/httpcore#1067, encode/httpcore#1074 and encode/httpcore#1086.
Note: this change was prepared with AI assistance (Claude Code).