diff --git a/CLAUDE.md b/CLAUDE.md index fa8a7f1..c487a10 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -3653,3 +3653,49 @@ Reproduced end to end before fixing, against real wheels on Python 3.12: two builds of the same version, `--upgrade` leaves the old code in place and prints the same sentence the colleague's Mac printed, `--force-reinstall --no-deps` replaces it. + +### `urllib` does not let `SSLCertVerificationError` out, and a fake said it did + +The certificate fix above shipped and **failed on the machine it was written +for, with the exact traceback it was meant to prevent**. The retry was written + +```python +except ssl.SSLCertVerificationError: +``` + +and urllib never raises that from `urlopen`. It catches it and re-raises +`urllib.error.URLError(err)`, carrying the original on `.reason`. So the +`except` matched nothing, ever, and the fallback could not fire. + +The unit test passed throughout, because the stub it used raised the bare SSL +error — **a shape real urllib never produces**. That is the whole lesson: a +fake that agrees with the author is worse than no test, because it converts an +untested path into a tested-looking one. The same sentence is already in this +file about `UPDATE ... RETURNING` and about the in-memory API fake, and it was +written again here anyway. + +`_is_cert_failure` checks the exception **and** its `.reason`, and the tests +now raise `URLError(SSLCertVerificationError(...))`, which is what his +traceback shows. A plain `URLError` — "no route to host" — is re-raised +untouched: retrying that with a different CA list changes nothing except how +long the operator waits for the real message, and a test asserts the second +attempt never happens. + +Beside the stubs there is now a **real** reproduction, +`test_against_a_real_machine_with_no_trust_store`, opt-in behind +`BEHAVISION_NETWORK_TESTS=1` because it reaches github.com. Python's default +context honours `SSL_CERT_FILE`, so pointing it at an empty file gives a +default context that trusts nobody — the python.org condition exactly — while +certifi is loaded by path and is unaffected. + +It skips rather than passes where it cannot reproduce that, and the difference +is measured rather than assumed: + +``` +macOS Command Line Tools LibreSSL 2.8.3 128 CAs with SSL_CERT_FILE empty + (reads the system keychain) +python.org / pyenv build OpenSSL 3.5.8 0 CAs -> reproduces it +``` + +Checked for teeth by putting the shipped `except` back: both the corrected stub +test and the live one fail, and pass again when it is restored. diff --git a/behavision/model_assets.py b/behavision/model_assets.py index 893dc75..23b5b54 100644 --- a/behavision/model_assets.py +++ b/behavision/model_assets.py @@ -5,6 +5,7 @@ from __future__ import annotations import logging import shutil import ssl +import urllib.error import urllib.request from pathlib import Path @@ -65,6 +66,27 @@ def _https_context() -> "ssl.SSLContext | None": return ssl.create_default_context(cafile=certifi.where()) +def _is_cert_failure(err: BaseException) -> bool: + """Is this a certificate-verification failure, however it is wrapped? + + `urllib` does NOT let `ssl.SSLCertVerificationError` out. It catches it and + re-raises `urllib.error.URLError(err)`, carrying the original on `.reason` + - so `except ssl.SSLCertVerificationError` around `urlopen` matches + nothing, ever. + + That is not a subtlety this file gets to record academically: the first + version of the fallback below was written exactly that way, shipped, and + failed on the machine it was written for with the very traceback it was + meant to prevent. The unit test passed throughout, because the fake it + used raised the bare SSL error - a shape real urllib never produces. A + stub that agrees with the author is worse than no test, and the test now + raises what urllib raises. + """ + reason = getattr(err, "reason", None) + return isinstance(err, ssl.SSLCertVerificationError) or \ + isinstance(reason, ssl.SSLCertVerificationError) + + def _urlopen(url: str, timeout: float = 60.0): """Open a URL, falling back to certifi's CA bundle on a verify failure. @@ -75,7 +97,13 @@ def _urlopen(url: str, timeout: float = 60.0): """ try: return urllib.request.urlopen(url, timeout=timeout) - except ssl.SSLCertVerificationError: + except (urllib.error.URLError, ssl.SSLCertVerificationError) as err: + # Only a certificate problem is worth a second attempt. "No route to + # host" and "connection refused" arrive as URLError too, and retrying + # those with a different CA list changes nothing except how long the + # operator waits for the real message. + if not _is_cert_failure(err): + raise ctx = _https_context() if ctx is None: raise diff --git a/tests/test_model_download.py b/tests/test_model_download.py index 3684991..edf98a4 100644 --- a/tests/test_model_download.py +++ b/tests/test_model_download.py @@ -9,6 +9,7 @@ perfectly - numpy, onnxruntime, faiss, all of it - and then could not fetch a """ import io import logging +import os import ssl import urllib.request @@ -33,9 +34,18 @@ class _Resp(io.BytesIO): def _verify_error(): - return ssl.SSLCertVerificationError( + """What urlopen ACTUALLY raises, which is not what it looks like. + + urllib catches ssl.SSLCertVerificationError and re-raises + urllib.error.URLError(err), carrying the original on `.reason`. The first + version of this test raised the bare SSL error - a shape real urllib never + produces - so it passed against a fallback that could never fire, and the + fix shipped and failed on the machine it was written for with the exact + traceback it was meant to prevent. + """ + return urllib.error.URLError(ssl.SSLCertVerificationError( "[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: " - "unable to get local issuer certificate") + "unable to get local issuer certificate")) def test_a_machine_with_no_trust_store_falls_back_to_the_bundled_one(monkeypatch): @@ -74,12 +84,22 @@ def test_a_working_trust_store_is_used_as_is(monkeypatch): def test_a_real_network_failure_is_not_disguised_as_a_certificate_problem(monkeypatch): + """A URLError is not automatically a certificate problem. + + "No route to host" and "connection refused" arrive as URLError too, and + retrying those with a different CA list changes nothing except how long + the operator waits for the real message. + """ + calls = [] + def fake(url, timeout=None, context=None): + calls.append(context) raise urllib.error.URLError("no route to host") monkeypatch.setattr(urllib.request, "urlopen", fake) with pytest.raises(urllib.error.URLError): model_assets._urlopen("https://example.invalid/m.onnx") + assert calls == [None], f"a dead network was retried as a CA problem: {calls}" def test_progress_lines_survive_the_rewrite(tmp_path, monkeypatch, caplog): @@ -103,3 +123,42 @@ def test_progress_lines_survive_the_rewrite(tmp_path, monkeypatch, caplog): pct = [ln for ln in lines if ln.startswith("download: face detector ")] assert pct, f"no progress lines at all: {lines}" assert "download: face detector 100%" in pct, pct + + +@pytest.mark.skipif(not os.environ.get("BEHAVISION_NETWORK_TESTS"), + reason="set BEHAVISION_NETWORK_TESTS=1 to reach github.com") +def test_against_a_real_machine_with_no_trust_store(tmp_path, monkeypatch): + """The whole thing, over the real network, with the real failure. + + Every stub above is a statement about what I believe urllib does, and the + first version of this file proved how much that is worth. Python's default + context honours SSL_CERT_FILE, so pointing it at an EMPTY file reproduces + a python.org macOS build exactly: a default context that trusts nobody. + certifi is loaded by path and is unaffected by the variable, so if the + fallback works here it works there. + """ + empty = tmp_path / "no-cas.pem" + empty.write_text("") + monkeypatch.setenv("SSL_CERT_FILE", str(empty)) + monkeypatch.setenv("SSL_CERT_DIR", str(tmp_path / "nothing")) + + # Not every interpreter can be put into that state, and pretending + # otherwise would turn this into a test that passes by not running. A + # Python linked against LibreSSL - which is what macOS Command Line Tools + # ships - reads the system keychain and ignores SSL_CERT_FILE entirely: + # measured, 128 CAs loaded with the variable pointing at an empty file. A + # python.org build links OpenSSL and honours it: 0 CAs, which is the + # condition being reproduced. + if ssl.create_default_context().get_ca_certs(): + pytest.skip("this interpreter ignores SSL_CERT_FILE (%s), so the " + "no-trust-store condition cannot be reproduced here" + % ssl.OPENSSL_VERSION) + + # The premise: the default context really is broken in this process. + with pytest.raises(urllib.error.URLError) as caught: + urllib.request.urlopen(model_assets.YUNET_URL, timeout=30) + assert model_assets._is_cert_failure(caught.value), caught.value + + dest = tmp_path / "yunet.onnx" + model_assets._fetch(model_assets.YUNET_URL, dest, "face detector") + assert dest.stat().st_size > 200_000