From 0d3eacc62ab38b917e13cf04b23ec65bc81f1336 Mon Sep 17 00:00:00 2001 From: James Rosewell Date: Sun, 6 Sep 2026 16:03:18 +0100 Subject: [PATCH] Refuse redirects when fetching a key, and read a naive now as UTC Found in review after the fetch was merged. The fetch followed redirects. urllib follows a redirect to any host and to any of http, https or ftp, so a creator whose domain answered 302 to some other place, or to plain http, would have that other place's key trusted as its own, and a network attacker able to bend the creator's DNS, or a creator that was simply misconfigured, could put a key there and have forgeries verify. The Java port refuses a change of protocol, so the Python port was the weaker of the two. The default transport now uses an opener whose redirect handler refuses every redirect, so the 3xx comes back as the response code and the caller reads the key as unavailable, which is what it is. The stand in end point learned to answer with a redirect to another host, and the test proves the key is not fetched from there. The test fails when redirects are followed again. public_key_url now checks the scheme where the URL is built, not only before the request, so a caller who only wants the URL cannot be handed one whose scheme is really a prefix naming some other host. public_key_response_at raised TypeError when given a naive datetime as now, because it compared it with an aware date, while the schedule reads a naive datetime as UTC. The two now agree. The README's fetch example asked a real resolver for creator.invalid and asserted the answer. RFC 6761 only says resolvers should refuse the name, and under a TLS intercepting proxy the answer is a block page, which reads as an unreadable key rather than an unavailable one and fails the README test. The example now passes a transport that refuses, so it shows the same shape and makes no request. Added the two pass check that tells selecting by start from anything else: the genuine identifier verifies under the published schedule and reads as not matching when every start is moved a week later. 163 tests, up from 161. --- README.md | 13 +++++++++--- owid/endpoints.py | 4 ++++ owid/public_key_fetch.py | 33 +++++++++++++++++++++++++++++-- tests/key_end_point.py | 14 +++++++++++++ tests/test_public_key_fetch.py | 22 +++++++++++++++++++++ tests/test_public_key_schedule.py | 26 ++++++++++++++++++++++++ 6 files changed, 107 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index bfa4b00..a3dea53 100644 --- a/README.md +++ b/README.md @@ -178,12 +178,19 @@ and the minute, up to 1024 of them before the store is emptied, and ```python from owid import SignatureStatus, public_key_fetch -# A creator on a domain that cannot exist, so the example shows the shape of -# the call and the status a key that cannot be obtained produces. +# A creator whose key this example never actually asks for. The transport +# below stands in for the network and refuses, so the example shows the +# shape of the call and the status a key that cannot be obtained produces +# without touching a resolver or a proxy. remote_creator = Creator("creator.invalid", Crypto.new()) remote = remote_creator.create_string("from another creator") -fetched = public_key_fetch.signature_status(remote, "https") +def unreachable(url, timeout): + raise OSError("this example makes no request") + +fetched = public_key_fetch.signature_status( + remote, "https", transport=unreachable +) if fetched is SignatureStatus.KEY_UNAVAILABLE: # The key could not be obtained, so the signature was never examined. # Only SIGNATURE_INVALID means the identifier should be distrusted. diff --git a/owid/endpoints.py b/owid/endpoints.py index 8b1d3a9..8141137 100644 --- a/owid/endpoints.py +++ b/owid/endpoints.py @@ -113,6 +113,10 @@ def public_key_response_at( "received '{0}'".format(format) ) moment = now if now is not None else datetime.now(timezone.utc) + if moment.tzinfo is None: + # Read as UTC, the only zone the wire format knows, so this agrees + # with the schedule rather than refusing to compare. + moment = moment.replace(tzinfo=timezone.utc) asked = moment if date is not None and date != "": minutes = _minutes(date) diff --git a/owid/public_key_fetch.py b/owid/public_key_fetch.py index aea6a8b..edeb52a 100644 --- a/owid/public_key_fetch.py +++ b/owid/public_key_fetch.py @@ -103,6 +103,15 @@ def public_key_url(owid: Owid, scheme: str) -> str: raise OwidError("the OWID is missing") if scheme is None or not scheme.strip(): raise OwidError("the scheme is missing") + if scheme.strip().lower() not in _ACCEPTED_SCHEMES: + # Checked here as well as before the request, so a caller who + # only wants the URL cannot be handed one that names some other + # host through a scheme that is really a prefix. + raise OwidError( + "the scheme must be http or https, received {0}".format( + _quoted(scheme) + ) + ) domain = owid.domain _check_domain(domain) minutes = io.minutes_since_base(owid.date) @@ -259,15 +268,35 @@ def _read(url: str, domain: str, transport: Optional[Transport]) -> str: return body.decode("utf-8", errors="replace") +class _NoRedirects(urllib.request.HTTPRedirectHandler): + """Refuses every redirect, so the key is only ever read from the + creator domain the OWID names and over the scheme the caller chose. + + urllib follows a redirect to any host and any of http, https or ftp, + so without this a creator whose domain answered 302 to some other + host, or to plain http, would have that other place's key trusted + as its own, and a network attacker could put a key there. Returning + None makes urlopen raise the 3xx as an HTTPError, which the transport + hands back as the response code and the caller reads as the key + being unavailable, which is what it is.""" + + def redirect_request(self, req, fp, code, msg, headers, newurl): + return None + + +_opener = urllib.request.build_opener(_NoRedirects()) + + def _urllib_transport(url: str, timeout: float) -> Tuple[int, bytes]: """The transport used unless the caller supplies one. A refusal carrying a response code is returned as that code, and only the failure to obtain - any response at all is raised.""" + any response at all is raised. Redirects are not followed (see + _NoRedirects).""" request = urllib.request.Request( url, headers={"Accept": "text/plain"}, method="GET" ) try: - with urllib.request.urlopen(request, timeout=timeout) as response: + with _opener.open(request, timeout=timeout) as response: return response.status, response.read(MAXIMUM_RESPONSE_BYTES + 1) except urllib.error.HTTPError as refused: # A response arrived, so the code is the answer. The body of a diff --git a/tests/key_end_point.py b/tests/key_end_point.py index 235d225..016631c 100644 --- a/tests/key_end_point.py +++ b/tests/key_end_point.py @@ -58,6 +58,9 @@ class Answer(Enum): SCHEDULE = "schedule" #: Text shaped like a PEM that no key can be read out of. BROKEN_KEY = "broken-key" + #: A redirect to a host that is not the creator, which a client must + #: not follow. + REDIRECT = "redirect" class _Server(ThreadingHTTPServer): @@ -73,6 +76,12 @@ def do_GET(self) -> None: # noqa: N802 - the name is the protocol's. values = urllib.parse.parse_qs(query, keep_blank_values=True) date = values.get("date", [None])[0] end_point.record(date) + if end_point.answer is Answer.REDIRECT: + self.send_response(302) + self.send_header("Location", "http://elsewhere.invalid/key.pem") + self.send_header("Content-Length", "0") + self.end_headers() + return try: body = end_point.body(date) except ValueError: @@ -138,6 +147,11 @@ def record(self, date: Optional[str]) -> None: with self._lock: self._dates.append(date) + @property + def answer(self) -> Answer: + """What this end point serves.""" + return self._answer + def body(self, date: Optional[str]) -> Optional[str]: """The body to serve, or None where the end point has no key. Raises ValueError where the date is not a count of minutes.""" diff --git a/tests/test_public_key_fetch.py b/tests/test_public_key_fetch.py index c4f6da5..724162e 100644 --- a/tests/test_public_key_fetch.py +++ b/tests/test_public_key_fetch.py @@ -219,6 +219,28 @@ def test_an_end_point_that_cannot_be_reached_is_key_unavailable( "a connection that is refused leaves the signature unjudged", ) + def test_a_redirect_is_not_followed(self) -> None: + """A creator whose domain answers with a redirect does not get the + key at the other end trusted as its own. The answer is that the key + is unavailable, carrying the 302, and the request that would have + gone to the other host is never made. Without this a network + attacker who could bend a creator's DNS or a misconfigured creator + could substitute the key, and forgeries would verify.""" + owid = key_fixtures.identifier() + end_point = self.end_point(Answer.REDIRECT) + with self.assertRaises(PublicKeyFetchError) as refused: + public_key_fetch._public_key_pem_at_url( + end_point.url_for(owid), owid.domain + ) + self.assertIs(SignatureStatus.KEY_UNAVAILABLE, refused.exception.status) + self.assertEqual(302, refused.exception.status_code) + self.assertIs( + SignatureStatus.KEY_UNAVAILABLE, + public_key_fetch._signature_status_at_url( + owid, end_point.url_for(owid), ALONE + ), + ) + def test_a_key_that_cannot_be_read_is_invalid_key(self) -> None: """Text shaped like a PEM that holds no key is a fault in the key, and never a signature that does not match.""" diff --git a/tests/test_public_key_schedule.py b/tests/test_public_key_schedule.py index b6df65a..6178bfb 100644 --- a/tests/test_public_key_schedule.py +++ b/tests/test_public_key_schedule.py @@ -119,6 +119,32 @@ def test_the_last_key_is_not_the_key_in_force(self) -> None: "the key in force now is the one that has started", ) + def test_verified_under_the_published_starts_and_invalid_when_shifted( + self, + ) -> None: + """The two pass check that tells selecting by start from anything + else. Under the published schedule the genuine identifier verifies. + Move every start one week later, keeping every key, and the same + identifier must read as not matching, because the key now chosen + for its date is the one that was in force the week before. A + selection that ignored the starts would answer the same both + times.""" + owid = key_fixtures.identifier() + published = key_fixtures.scheduled_keys() + as_published = PublicKeySchedule( + DatedPublicKey(key.starts_at, key.pem) for key in published + ) + shifted = PublicKeySchedule( + DatedPublicKey(key.starts_at + timedelta(days=7), key.pem) + for key in published + ) + self.assertIs( + SignatureStatus.SIGNATURE_VALID, as_published.signature_status(owid) + ) + self.assertIs( + SignatureStatus.SIGNATURE_INVALID, shifted.signature_status(owid) + ) + def test_selection_ignores_the_moment_the_keys_were_generated(self) -> None: """The shape that broke the .NET port. Thirteen of the published keys were generated in one batch on 1 September 2026 and cover the weeks