Repository navigation
strip trailing dot from FQDN hostnames for TLS SNI #1007
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -161,6 +161,17 @@ def __init__(self, scheme: bytes, host: bytes, port: int) -> None: | |
| self.host = host | ||
| self.port = port | ||
|
|
||
| @property | ||
| def normalized_host(self) -> str: | ||
| """Hostname decoded to ASCII with any trailing FQDN dot removed. | ||
|
|
||
| Use this wherever the hostname is passed to network operations (TLS | ||
| SNI, DNS resolution, socket connect) so that a trailing dot in an | ||
| FQDN like `myhost.internal.` does not cause certificate verification | ||
| failures. The raw `host` bytes are left untouched. | ||
| """ | ||
| return self.host.decode("ascii").removesuffix(".") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Stripping the final dot before DNS resolution turns an absolute hostname into a search-list candidate. With resolver settings such as Prompt for AI agents |
||
|
|
||
| def __eq__(self, other: typing.Any) -> bool: | ||
| return ( | ||
| isinstance(other, Origin) | ||
|
|
@@ -171,9 +182,8 @@ def __eq__(self, other: typing.Any) -> bool: | |
|
|
||
| def __str__(self) -> str: | ||
| scheme = self.scheme.decode("ascii") | ||
| host = self.host.decode("ascii") | ||
| port = str(self.port) | ||
| return f"{scheme}://{host}:{port}" | ||
| return f"{scheme}://{self.normalized_host}:{port}" | ||
|
|
||
|
|
||
| class URL: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,7 +219,7 @@ def handle_request(self, request: Request) -> Response: | |
| try: | ||
| # Connect to the proxy | ||
| kwargs = { | ||
| "host": self._proxy_origin.host.decode("ascii"), | ||
| "host": self._proxy_origin.normalized_host, | ||
| "port": self._proxy_origin.port, | ||
| "timeout": timeout, | ||
| } | ||
|
|
@@ -230,7 +230,7 @@ def handle_request(self, request: Request) -> Response: | |
| # Connect to the remote host using socks5 | ||
| kwargs = { | ||
| "stream": stream, | ||
| "host": self._remote_origin.host.decode("ascii"), | ||
| "host": self._remote_origin.normalized_host, | ||
| "port": self._remote_origin.port, | ||
| "auth": self._proxy_auth, | ||
| "timeouts": timeouts, | ||
|
|
@@ -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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The three SOCKS5 changes (proxy connect host, SOCKS5 CONNECT target host, and Prompt for AI agents
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The Prompt for AI agents |
||
| "timeout": timeout, | ||
| } | ||
| with Trace("start_tls", logger, request, kwargs) as trace: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| """Async tests for trailing-dot FQDN hostname normalisation (issue #1063).""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import ssl | ||
| import typing | ||
|
|
||
| import pytest | ||
|
|
||
| import httpcore2 | ||
| from httpcore2 import ( | ||
| SOCKET_OPTION, | ||
| AsyncMockBackend, | ||
| AsyncMockStream, | ||
| AsyncNetworkStream, | ||
| Origin, | ||
| ) | ||
|
|
||
|
|
||
| class RecordingAsyncStream(AsyncMockStream): | ||
| """AsyncMockStream that records the server_hostname passed to start_tls().""" | ||
|
|
||
| def __init__(self, buffer: list[bytes]) -> None: | ||
| super().__init__(buffer) | ||
| self.start_tls_hostname: str | None = None | ||
|
|
||
| async def start_tls( | ||
| self, | ||
| ssl_context: ssl.SSLContext, | ||
| server_hostname: str | None = None, | ||
| timeout: float | None = None, | ||
| ) -> AsyncNetworkStream: | ||
| self.start_tls_hostname = server_hostname | ||
| return self | ||
|
|
||
|
|
||
| class RecordingAsyncBackend(AsyncMockBackend): | ||
| def __init__(self, buffer: list[bytes]) -> None: | ||
| super().__init__(buffer) | ||
| self.stream: RecordingAsyncStream | None = None | ||
|
|
||
| async def connect_tcp( | ||
| self, | ||
| host: str, | ||
| port: int, | ||
| timeout: float | None = None, | ||
| local_address: str | None = None, | ||
| socket_options: typing.Iterable[SOCKET_OPTION] | None = None, | ||
| ) -> AsyncNetworkStream: | ||
| self.stream = RecordingAsyncStream(list(self._buffer)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Prompt for AI agents |
||
| return self.stream | ||
|
|
||
|
|
||
| @pytest.mark.anyio | ||
| async def test_sni_hostname_strips_trailing_dot() -> None: | ||
| """server_hostname passed to start_tls() must not carry the trailing dot.""" | ||
| origin = Origin(b"https", b"myhost.internal.", 443) | ||
| network_backend = RecordingAsyncBackend( | ||
| [ | ||
| b"HTTP/1.1 200 OK\r\n", | ||
| b"Content-Length: 0\r\n", | ||
| b"\r\n", | ||
| ] | ||
| ) | ||
| async with httpcore2.AsyncHTTPConnection(origin=origin, network_backend=network_backend) as conn: | ||
| async with conn.stream("GET", "https://myhost.internal./") as response: | ||
| await response.aread() | ||
|
|
||
| assert network_backend.stream is not None | ||
| assert network_backend.stream.start_tls_hostname == "myhost.internal" | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| """Async tests for trailing-dot FQDN hostname normalisation (issue #1063).""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import ssl | ||
| import typing | ||
|
|
||
| import pytest | ||
|
|
||
| import httpcore2 | ||
| from httpcore2 import ( | ||
| SOCKET_OPTION, | ||
| MockBackend, | ||
| MockStream, | ||
| NetworkStream, | ||
| Origin, | ||
| ) | ||
|
|
||
|
|
||
| class RecordingAsyncStream(MockStream): | ||
| """MockStream that records the server_hostname passed to start_tls().""" | ||
|
|
||
| def __init__(self, buffer: list[bytes]) -> None: | ||
| super().__init__(buffer) | ||
| self.start_tls_hostname: str | None = None | ||
|
|
||
| def start_tls( | ||
| self, | ||
| ssl_context: ssl.SSLContext, | ||
| server_hostname: str | None = None, | ||
| timeout: float | None = None, | ||
| ) -> NetworkStream: | ||
| self.start_tls_hostname = server_hostname | ||
| return self | ||
|
|
||
|
|
||
| class RecordingAsyncBackend(MockBackend): | ||
| def __init__(self, buffer: list[bytes]) -> None: | ||
| super().__init__(buffer) | ||
| self.stream: RecordingAsyncStream | None = None | ||
|
|
||
| def connect_tcp( | ||
| self, | ||
| host: str, | ||
| port: int, | ||
| timeout: float | None = None, | ||
| local_address: str | None = None, | ||
| socket_options: typing.Iterable[SOCKET_OPTION] | None = None, | ||
| ) -> NetworkStream: | ||
| self.stream = RecordingAsyncStream(list(self._buffer)) | ||
| return self.stream | ||
|
|
||
|
|
||
|
|
||
| def test_sni_hostname_strips_trailing_dot() -> None: | ||
| """server_hostname passed to start_tls() must not carry the trailing dot.""" | ||
| origin = Origin(b"https", b"myhost.internal.", 443) | ||
| network_backend = RecordingAsyncBackend( | ||
| [ | ||
| b"HTTP/1.1 200 OK\r\n", | ||
| b"Content-Length: 0\r\n", | ||
| b"\r\n", | ||
| ] | ||
| ) | ||
| with httpcore2.HTTPConnection(origin=origin, network_backend=network_backend) as conn: | ||
| with conn.stream("GET", "https://myhost.internal./") as response: | ||
| response.read() | ||
|
|
||
| assert network_backend.stream is not None | ||
| assert network_backend.stream.start_tls_hostname == "myhost.internal" |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -49,6 +49,31 @@ def test_url_origin_socks5() -> None: | |||||
| assert str(origin) == "socks5h://127.0.0.1:1080" | ||||||
|
|
||||||
|
|
||||||
| def test_origin_str_strips_trailing_dot() -> None: | ||||||
| """Origin.__str__ must strip the trailing dot from FQDNs. | ||||||
|
|
||||||
| 'myhost.internal.' is a valid FQDN but TLS certificates use | ||||||
| 'myhost.internal' (without the dot). Passing the raw hostname | ||||||
| to ssl_wrap_socket would cause CERTIFICATE_VERIFY_FAILED. | ||||||
| """ | ||||||
| origin = httpcore2.Origin(b"https", b"myhost.internal.", 443) | ||||||
| assert str(origin) == "https://myhost.internal:443" | ||||||
|
|
||||||
|
|
||||||
| def test_origin_str_no_trailing_dot_unchanged() -> None: | ||||||
| origin = httpcore2.Origin(b"https", b"example.com", 443) | ||||||
| assert str(origin) == "https://example.com:443" | ||||||
|
|
||||||
|
|
||||||
| def test_url_host_strips_trailing_dot() -> None: | ||||||
| """URL.host used for SNI should not carry the trailing dot.""" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: The docstring and test name say Prompt for AI agents
Suggested change
|
||||||
| url = httpcore2.URL("https://myhost.internal.:8443/") | ||||||
| assert url.host == b"myhost.internal." | ||||||
| assert url.port is not None | ||||||
| origin = httpcore2.Origin(b"https", url.host, url.port) | ||||||
| assert str(origin) == "https://myhost.internal:8443" | ||||||
|
|
||||||
|
|
||||||
| # Request | ||||||
|
|
||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Medium: Preserve absolute hostnames during DNS resolution
With a DNS search suffix and
ndots:5, stripping the dot changeshttp://api.example.com./from an absolute lookup into a search-path lookup, allowing an attacker controllingapi.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 TLSserver_hostname.