From d6263ad79cd684ee304f7ed4b0741f6c64981732 Mon Sep 17 00:00:00 2001 From: Sabu Siyad Date: Mon, 26 Oct 2026 23:29:16 +0530 Subject: [PATCH 1/2] fix(bench): Check backup URLs before the agent downloads them The agent downloads the database and files backups for a restore from the URLs that Press sends. Each request now goes only to a public address, and the connection uses the address that was checked. The download follows up to 3 redirects, and each target goes through the same check. Webhooks and Remote File in Press use the same checks. --- agent/remote_url.py | 106 ++++++++++++++++++++ agent/tests/test_remote_url.py | 174 +++++++++++++++++++++++++++++++++ agent/utils.py | 4 +- 3 files changed, 283 insertions(+), 1 deletion(-) create mode 100644 agent/remote_url.py create mode 100644 agent/tests/test_remote_url.py diff --git a/agent/remote_url.py b/agent/remote_url.py new file mode 100644 index 00000000..dd8eb11c --- /dev/null +++ b/agent/remote_url.py @@ -0,0 +1,106 @@ +"""Open URLs that Press passes on from a customer, such as backup files to restore. + +Each request goes only to a public address. The host is resolved once, and the +connection is pinned to the address that was checked. Redirects are followed by +hand, and each target is checked the same way. +""" + +from __future__ import annotations + +import ipaddress +import socket +from contextlib import contextmanager +from typing import TYPE_CHECKING +from urllib.parse import urljoin, urlsplit, urlunsplit + +import requests +from requests.adapters import HTTPAdapter +from urllib3.connectionpool import HTTPSConnectionPool + +if TYPE_CHECKING: + from collections.abc import Iterator + +MAX_REDIRECTS = 3 + + +class RemoteURLError(Exception): + """The URL is not http(s), cannot be resolved, or resolves to an address that is not public.""" + + +@contextmanager +def open_remote_url(url: str) -> Iterator[requests.Response]: + """Stream a GET of `url`, following up to MAX_REDIRECTS redirects.""" + for _ in range(MAX_REDIRECTS + 1): + hostname = _hostname(url) + ip = _resolve_public_ip(hostname) + with requests.Session() as session: + session.mount(f"{urlsplit(url).scheme}://", _PinnedIPAdapter(ip, hostname)) + with session.get(url, stream=True, allow_redirects=False) as response: + if not response.is_redirect: + yield response + return + location = response.headers["Location"] + url = urljoin(url, location) + raise RemoteURLError("The URL redirects too many times.") + + +def _hostname(url: str) -> str: + parts = urlsplit(url) + if parts.scheme not in ("http", "https"): + raise RemoteURLError("Only http and https URLs are allowed.") + if not parts.hostname: + raise RemoteURLError("The URL has no host.") + return parts.hostname + + +def _resolve_public_ip(hostname: str) -> str: + """Resolve `hostname`, refusing if any address it maps to is not public.""" + try: + addresses = socket.getaddrinfo(hostname, None) + except socket.gaierror as error: + raise RemoteURLError(f"Could not resolve the host '{hostname}'.") from error + + ips = [str(address[4][0]) for address in addresses] + for ip in ips: + if not _is_public(ip): + raise RemoteURLError("The URL resolves to an address that is not public.") + # Prefer IPv4, as the server may not have IPv6 egress + return next((ip for ip in ips if ipaddress.ip_address(ip).version == 4), ips[0]) + + +def _is_public(ip: str) -> bool: + address = ipaddress.ip_address(ip) + if isinstance(address, ipaddress.IPv6Address) and address.ipv4_mapped: + return address.ipv4_mapped.is_global + return address.is_global + + +def _authority(host: str, port: int | None) -> str: + """Format host and port as a URL authority, bracketing an IPv6 literal.""" + host = f"[{host}]" if ":" in host else host + return f"{host}:{port}" if port else host + + +class _PinnedIPAdapter(HTTPAdapter): + """Connect to the checked IP, and keep the Host header and the TLS check on the hostname.""" + + def __init__(self, ip: str, hostname: str, **kwargs): + self.ip = ip + self.hostname = hostname + self.is_https = False + super().__init__(**kwargs) + + def send(self, request, stream=False, timeout=None, verify=True, cert=None, proxies=None): + parts = urlsplit(str(request.url)) + self.is_https = parts.scheme == "https" + # Build both authorities from host and port only, so userinfo does not go into the Host header + request.headers["Host"] = _authority(self.hostname, parts.port) + request.url = urlunsplit(parts._replace(netloc=_authority(self.ip, parts.port))) + return super().send(request, stream, timeout, verify, cert, proxies) + + def get_connection(self, url, proxies=None): + pool = super().get_connection(url, proxies) + if self.is_https and isinstance(pool, HTTPSConnectionPool): + pool.assert_hostname = self.hostname + pool.conn_kw["server_hostname"] = self.hostname + return pool diff --git a/agent/tests/test_remote_url.py b/agent/tests/test_remote_url.py new file mode 100644 index 00000000..fb3d64d3 --- /dev/null +++ b/agent/tests/test_remote_url.py @@ -0,0 +1,174 @@ +from __future__ import annotations + +import io +import os +import shutil +import socket +import tempfile +import threading +import unittest +from http.server import BaseHTTPRequestHandler, HTTPServer +from unittest.mock import patch + +import requests +from urllib3.connectionpool import HTTPSConnectionPool + +from agent import remote_url +from agent.remote_url import RemoteURLError, _PinnedIPAdapter +from agent.utils import download_file + + +def resolve_to(*ips): + """Fake socket.getaddrinfo that resolves any host to `ips`.""" + return [(socket.AF_INET, socket.SOCK_STREAM, 6, "", (ip, 0)) for ip in ips] + + +def resolve_by_host(hosts): + """Fake socket.getaddrinfo that resolves each host in `hosts` to its IP.""" + return lambda hostname, *args, **kwargs: resolve_to(hosts[hostname]) + + +def fake_responses(*responses): + """Fake HTTPAdapter.send that answers with (status, Location) in turn, and records each Host.""" + hosts = [] + queue = list(responses) + + def send(self, request, *args, **kwargs): + hosts.append(request.headers["Host"]) + status, location = queue.pop(0) + response = requests.Response() + response.status_code = status + response.raw = io.BytesIO(b"backup") + if location: + response.headers["Location"] = location + return response + + return send, hosts + + +class TestDownloadFile(unittest.TestCase): + def setUp(self): + self.directory = tempfile.mkdtemp() + + def tearDown(self): + shutil.rmtree(self.directory) + + def test_download_follows_redirect_to_a_public_host(self): + send, hosts = fake_responses((302, "https://bucket.example.com/backup.sql.gz"), (200, None)) + with patch( + "agent.remote_url.socket.getaddrinfo", + resolve_by_host({"site.example.com": "93.184.216.34", "bucket.example.com": "93.184.216.35"}), + ), patch("requests.adapters.HTTPAdapter.send", send): + path = download_file("https://site.example.com/backup.sql.gz", prefix=self.directory) + + self.assertEqual(hosts, ["site.example.com", "bucket.example.com"]) + with open(path, "rb") as f: + self.assertEqual(f.read(), b"backup") + + def test_download_refuses_redirect_that_fails_the_check_before_requesting_it(self): + send, hosts = fake_responses((302, "http://files.example.com/backup.sql.gz"), (200, None)) + with patch( + "agent.remote_url.socket.getaddrinfo", + resolve_by_host({"site.example.com": "93.184.216.34", "files.example.com": "10.0.0.5"}), + ), patch("requests.adapters.HTTPAdapter.send", send), self.assertRaisesRegex( + RemoteURLError, "not public" + ): + download_file("https://site.example.com/backup.sql.gz", prefix=self.directory) + + self.assertEqual(hosts, ["site.example.com"]) + self.assertEqual(os.listdir(self.directory), []) + + def test_download_gives_up_after_too_many_redirects(self): + send, hosts = fake_responses(*[(302, "/again")] * (remote_url.MAX_REDIRECTS + 1)) + with patch("agent.remote_url.socket.getaddrinfo", return_value=resolve_to("93.184.216.34")), patch( + "requests.adapters.HTTPAdapter.send", send + ), self.assertRaisesRegex(RemoteURLError, "redirects too many times"): + download_file("https://site.example.com/backup.sql.gz", prefix=self.directory) + + self.assertEqual(len(hosts), remote_url.MAX_REDIRECTS + 1) + + def test_download_refuses_url_that_is_not_http(self): + for url in ("file:///tmp/backup.sql.gz", "ftp://example.com/backup.sql.gz"): + with self.subTest(url=url), self.assertRaisesRegex(RemoteURLError, "http and https"): + download_file(url, prefix=self.directory) + + def test_download_refuses_host_with_any_address_that_is_not_public(self): + for ips in (("10.0.0.5",), ("93.184.216.34", "127.0.0.1"), ("::ffff:127.0.0.1",)): + with self.subTest(ips=ips), patch( + "agent.remote_url.socket.getaddrinfo", return_value=resolve_to(*ips) + ), patch("requests.adapters.HTTPAdapter.send") as send, self.assertRaisesRegex( + RemoteURLError, "not public" + ): + download_file("https://files.example.com/backup.sql.gz", prefix=self.directory) + send.assert_not_called() + + def test_download_refuses_host_that_does_not_resolve(self): + with patch( + "agent.remote_url.socket.getaddrinfo", side_effect=socket.gaierror + ), self.assertRaisesRegex(RemoteURLError, "Could not resolve"): + download_file("https://missing.example.com/backup.sql.gz", prefix=self.directory) + + def test_download_connects_to_the_checked_address_and_keeps_the_hostname(self): + """A real request: only the check can resolve the name, so it arrives only through the pinned IP.""" + received = {} + getaddrinfo = socket.getaddrinfo + + def resolve(host, port, *args, **kwargs): + if host == "files.example.com" and port is None: + return resolve_to("127.0.0.1") + if host == "127.0.0.1": + return getaddrinfo(host, port, *args, **kwargs) + raise socket.gaierror + + class Handler(BaseHTTPRequestHandler): + def do_GET(self): + received["host"] = self.headers["Host"] + self.send_response(200) + self.send_header("Content-Length", "6") + self.end_headers() + self.wfile.write(b"backup") + + def log_message(self, format, *args): + pass + + server = HTTPServer(("127.0.0.1", 0), Handler) + threading.Thread(target=server.serve_forever, daemon=True).start() + port = server.server_address[1] + try: + with patch("agent.remote_url.socket.getaddrinfo", resolve), patch( + "agent.remote_url._is_public", return_value=True + ): + path = download_file(f"http://files.example.com:{port}/backup.sql.gz", prefix=self.directory) + finally: + server.shutdown() + server.server_close() + + self.assertEqual(received["host"], f"files.example.com:{port}") + with open(path, "rb") as f: + self.assertEqual(f.read(), b"backup") + + +class TestPinnedIPAdapter(unittest.TestCase): + def test_https_request_goes_to_the_ip_and_checks_the_certificate_of_the_hostname(self): + adapter = _PinnedIPAdapter("93.184.216.34", "bucket.example.com") + captured = {} + + def capture(self, request, *args, **kwargs): + captured["url"] = request.url + captured["host"] = request.headers["Host"] + pool = adapter.get_connection(request.url) + assert isinstance(pool, HTTPSConnectionPool) + captured["assert_hostname"] = pool.assert_hostname + captured["server_hostname"] = pool.conn_kw["server_hostname"] + response = requests.Response() + response.status_code = 200 + return response + + with requests.Session() as session, patch("requests.adapters.HTTPAdapter.send", capture): + session.mount("https://", adapter) + session.get("https://user:secret@bucket.example.com/backup.sql.gz", allow_redirects=False) + + self.assertTrue(captured["url"].startswith("https://93.184.216.34/")) + self.assertEqual(captured["host"], "bucket.example.com") + self.assertEqual(captured["assert_hostname"], "bucket.example.com") + self.assertEqual(captured["server_hostname"], "bucket.example.com") diff --git a/agent/utils.py b/agent/utils.py index dd66e8d3..606302e5 100644 --- a/agent/utils.py +++ b/agent/utils.py @@ -16,6 +16,8 @@ import requests +from agent.remote_url import open_remote_url + # ponytail: one ceiling for web and jobs; it only stops a hung mount, a big tree finishes well under it GET_SIZE_TIMEOUT = 15 * 60 @@ -74,7 +76,7 @@ def download_file(url, prefix): filename = secrets.token_urlsafe(16) + ext local_filename = os.path.join(prefix, filename) - with requests.get(url, stream=True) as r: + with open_remote_url(url) as r: total_size = int(r.headers.get("content-length", 0)) chunk_size = 1024 * 1024 if total_size > (100 * 1024 * 1024) else 8192 r.raise_for_status() From 31c582729752923ffd8ca68a99027a8f09ddaa48 Mon Sep 17 00:00:00 2001 From: Sabu Siyad Date: Mon, 26 Oct 2026 23:41:07 +0530 Subject: [PATCH 2/2] test(bench): Download over real TLS in the backup URL tests The adapter test replaced the request, so it never ran a TLS handshake. The new tests serve HTTPS on 127.0.0.1 with a test CA. A certificate for the hostname is accepted, and the SNI is the hostname. A certificate for another name, or for the pinned IP, is rejected. urllib3 sends server_hostname for SNI and checks the certificate against it, so the adapter no longer sets assert_hostname. --- agent/remote_url.py | 2 +- agent/tests/test_remote_url.py | 200 +++++++++++++++++++++++---------- 2 files changed, 144 insertions(+), 58 deletions(-) diff --git a/agent/remote_url.py b/agent/remote_url.py index dd8eb11c..9661def9 100644 --- a/agent/remote_url.py +++ b/agent/remote_url.py @@ -101,6 +101,6 @@ def send(self, request, stream=False, timeout=None, verify=True, cert=None, prox def get_connection(self, url, proxies=None): pool = super().get_connection(url, proxies) if self.is_https and isinstance(pool, HTTPSConnectionPool): - pool.assert_hostname = self.hostname + # urllib3 sends this name for SNI and checks the certificate against it pool.conn_kw["server_hostname"] = self.hostname return pool diff --git a/agent/tests/test_remote_url.py b/agent/tests/test_remote_url.py index fb3d64d3..7afa2e19 100644 --- a/agent/tests/test_remote_url.py +++ b/agent/tests/test_remote_url.py @@ -1,20 +1,27 @@ from __future__ import annotations +import datetime import io +import ipaddress import os import shutil import socket +import ssl import tempfile import threading import unittest +from contextlib import contextmanager from http.server import BaseHTTPRequestHandler, HTTPServer from unittest.mock import patch import requests -from urllib3.connectionpool import HTTPSConnectionPool +from cryptography import x509 +from cryptography.hazmat.primitives import hashes, serialization +from cryptography.hazmat.primitives.asymmetric import ec +from cryptography.x509.oid import NameOID from agent import remote_url -from agent.remote_url import RemoteURLError, _PinnedIPAdapter +from agent.remote_url import RemoteURLError from agent.utils import download_file @@ -46,6 +53,101 @@ def send(self, request, *args, **kwargs): return send, hosts +def resolve_only_for_the_check(hostname, ip="127.0.0.1"): + """Fake socket.getaddrinfo where only the check can resolve `hostname`, so a request reaches `ip` + only through the pinned address.""" + getaddrinfo = socket.getaddrinfo + + def resolve(host, port, *args, **kwargs): + if host == hostname and port is None: + return resolve_to(ip) + if host == ip: + return getaddrinfo(host, port, *args, **kwargs) + raise socket.gaierror + + return resolve + + +@contextmanager +def local_server(received, tls_context=None): + """Serve b"backup" on 127.0.0.1, record the Host header in `received`, and yield the port.""" + + class Handler(BaseHTTPRequestHandler): + def do_GET(self): + received["host"] = self.headers["Host"] + self.send_response(200) + self.send_header("Content-Length", "6") + self.end_headers() + self.wfile.write(b"backup") + + def log_message(self, format, *args): + pass + + class Server(HTTPServer): + def handle_error(self, request, client_address): + pass # A client that rejects the certificate drops the connection + + server = Server(("127.0.0.1", 0), Handler) + if tls_context: + server.socket = tls_context.wrap_socket(server.socket, server_side=True) + threading.Thread(target=server.serve_forever, daemon=True).start() + try: + yield server.server_address[1] + finally: + server.shutdown() + server.server_close() + + +def _certificate(subject, issuer, public_key, signing_key, *, ca, names=()): + now = datetime.datetime.now(datetime.timezone.utc) + builder = ( + x509.CertificateBuilder() + .subject_name(subject) + .issuer_name(issuer) + .public_key(public_key) + .serial_number(x509.random_serial_number()) + .not_valid_before(now - datetime.timedelta(minutes=5)) + .not_valid_after(now + datetime.timedelta(days=1)) + .add_extension(x509.BasicConstraints(ca=ca, path_length=None), critical=True) + ) + if names: + builder = builder.add_extension(x509.SubjectAlternativeName(list(names)), critical=False) + return builder.sign(signing_key, hashes.SHA256()) + + +def write_certificates(directory, name): + """Write a CA and a server certificate for `name`. Return the CA path and a server context.""" + ca_key = ec.generate_private_key(ec.SECP256R1()) + ca_name = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, "Test CA")]) + ca = _certificate(ca_name, ca_name, ca_key.public_key(), ca_key, ca=True) + + try: + san = x509.IPAddress(ipaddress.ip_address(name)) + except ValueError: + san = x509.DNSName(name) + key = ec.generate_private_key(ec.SECP256R1()) + subject = x509.Name([x509.NameAttribute(NameOID.COMMON_NAME, name)]) + certificate = _certificate(subject, ca_name, key.public_key(), ca_key, ca=False, names=[san]) + + ca_path = os.path.join(directory, "ca.pem") + with open(ca_path, "wb") as f: + f.write(ca.public_bytes(serialization.Encoding.PEM)) + chain_path = os.path.join(directory, "server.pem") + with open(chain_path, "wb") as f: + f.write(certificate.public_bytes(serialization.Encoding.PEM)) + f.write( + key.private_bytes( + serialization.Encoding.PEM, + serialization.PrivateFormat.PKCS8, + serialization.NoEncryption(), + ) + ) + + context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + context.load_cert_chain(chain_path) + return ca_path, context + + class TestDownloadFile(unittest.TestCase): def setUp(self): self.directory = tempfile.mkdtemp() @@ -111,64 +213,48 @@ def test_download_refuses_host_that_does_not_resolve(self): def test_download_connects_to_the_checked_address_and_keeps_the_hostname(self): """A real request: only the check can resolve the name, so it arrives only through the pinned IP.""" received = {} - getaddrinfo = socket.getaddrinfo - - def resolve(host, port, *args, **kwargs): - if host == "files.example.com" and port is None: - return resolve_to("127.0.0.1") - if host == "127.0.0.1": - return getaddrinfo(host, port, *args, **kwargs) - raise socket.gaierror - - class Handler(BaseHTTPRequestHandler): - def do_GET(self): - received["host"] = self.headers["Host"] - self.send_response(200) - self.send_header("Content-Length", "6") - self.end_headers() - self.wfile.write(b"backup") - - def log_message(self, format, *args): - pass - - server = HTTPServer(("127.0.0.1", 0), Handler) - threading.Thread(target=server.serve_forever, daemon=True).start() - port = server.server_address[1] - try: - with patch("agent.remote_url.socket.getaddrinfo", resolve), patch( - "agent.remote_url._is_public", return_value=True - ): - path = download_file(f"http://files.example.com:{port}/backup.sql.gz", prefix=self.directory) - finally: - server.shutdown() - server.server_close() + with local_server(received) as port, patch( + "agent.remote_url.socket.getaddrinfo", resolve_only_for_the_check("files.example.com") + ), patch("agent.remote_url._is_public", return_value=True): + path = download_file(f"http://files.example.com:{port}/backup.sql.gz", prefix=self.directory) self.assertEqual(received["host"], f"files.example.com:{port}") with open(path, "rb") as f: self.assertEqual(f.read(), b"backup") -class TestPinnedIPAdapter(unittest.TestCase): - def test_https_request_goes_to_the_ip_and_checks_the_certificate_of_the_hostname(self): - adapter = _PinnedIPAdapter("93.184.216.34", "bucket.example.com") - captured = {} - - def capture(self, request, *args, **kwargs): - captured["url"] = request.url - captured["host"] = request.headers["Host"] - pool = adapter.get_connection(request.url) - assert isinstance(pool, HTTPSConnectionPool) - captured["assert_hostname"] = pool.assert_hostname - captured["server_hostname"] = pool.conn_kw["server_hostname"] - response = requests.Response() - response.status_code = 200 - return response - - with requests.Session() as session, patch("requests.adapters.HTTPAdapter.send", capture): - session.mount("https://", adapter) - session.get("https://user:secret@bucket.example.com/backup.sql.gz", allow_redirects=False) - - self.assertTrue(captured["url"].startswith("https://93.184.216.34/")) - self.assertEqual(captured["host"], "bucket.example.com") - self.assertEqual(captured["assert_hostname"], "bucket.example.com") - self.assertEqual(captured["server_hostname"], "bucket.example.com") +class TestDownloadFileOverTLS(unittest.TestCase): + """Real TLS handshakes on 127.0.0.1, with a test CA the client trusts.""" + + def setUp(self): + self.directory = tempfile.mkdtemp() + + def tearDown(self): + shutil.rmtree(self.directory) + + def download(self, certificate_name, received, sni): + ca_path, context = write_certificates(self.directory, certificate_name) + context.sni_callback = lambda ssl_socket, server_name, ssl_context: sni.append(server_name) + with local_server(received, context) as port, patch( + "agent.remote_url.socket.getaddrinfo", resolve_only_for_the_check("bucket.example.com") + ), patch("agent.remote_url._is_public", return_value=True), patch.dict( + os.environ, {"REQUESTS_CA_BUNDLE": ca_path} + ): + return download_file( + f"https://user:secret@bucket.example.com:{port}/backup.sql.gz", prefix=self.directory + ), port + + def test_download_accepts_a_trusted_certificate_for_the_hostname(self): + received, sni = {}, [] + path, port = self.download("bucket.example.com", received, sni) + + self.assertEqual(sni, ["bucket.example.com"]) + self.assertEqual(received["host"], f"bucket.example.com:{port}") + with open(path, "rb") as f: + self.assertEqual(f.read(), b"backup") + + def test_download_rejects_a_trusted_certificate_for_another_name(self): + # 127.0.0.1 is the pinned address: its certificate must not pass for the hostname either + for name in ("other.example.com", "127.0.0.1"): + with self.subTest(name=name), self.assertRaisesRegex(requests.exceptions.SSLError, "match"): + self.download(name, {}, [])