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