From 41e9ef21945523741c78185bf6c04ea1b5fdca49 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 27 Oct 2026 17:22:09 -0500 Subject: [PATCH 1/5] Revocation-guard census records the two Vault client sites as guarded The census listed the Vault KV secret provider and Transit key provider _build_client sites as Exempt and "Ungated". Since engine PR 2324 both are gated: each calls mount_strict_reply_adapter, which runs _refuse_an_unrevoked_vault_hop at construction and before each send. That guard decides through revocation_hop_disposition and never calls RevocationHopGuard, so the census gains a second root guard. The three names on the chain join _GUARDS. A sabotage of either site's mount call, or of either chain link, turns the census red. BACKLOG #3587 Proposed PR title: Revocation-guard census records the two Vault client sites as guarded (BACKLOG #3587) Banner: SHIPPED -- tests/test_revocation_guard_census.py lists both Vault _build_client sites as Guarded, tied to _refuse_an_unrevoked_vault_hop. --- tests/test_revocation_guard_census.py | 72 ++++++++++++++++++--------- 1 file changed, 48 insertions(+), 24 deletions(-) diff --git a/tests/test_revocation_guard_census.py b/tests/test_revocation_guard_census.py index d3a40cb690..9e193323bd 100644 --- a/tests/test_revocation_guard_census.py +++ b/tests/test_revocation_guard_census.py @@ -19,15 +19,20 @@ the listener gates are a different control. **What "guarded" proves.** The site's function, or the one its entry names, calls a name in -:data:`_GUARDS`, and every such name reaches ``RevocationHopGuard.capture``. That ties the site to a -guard call. It does not prove that every branch reaches the call, or that the call refuses: the -behaviour tests (at least ``tests/test_revocation_guard_remaining_hops.py`` and -``tests/test_hop_refusal_revocation.py``) prove that. A second hop added inside a function that is -already guarded passes here too, so read the function in review. +:data:`_GUARDS`, and every such name reaches one of the two guards in :data:`_GUARD_ROOTS`. That +ties the site to a guard call. It does not prove that every branch +reaches the call, or that the call refuses: the behaviour tests (at least +``tests/test_revocation_guard_remaining_hops.py``, ``tests/test_hop_refusal_revocation.py`` and +``tests/test_vault_revocation_hop_guard.py``) prove that. A second hop added inside a function that +is already guarded passes here too, so read the function in review. **It goes red.** Delete the guard call from one guarded site and :func:`test_every_guarded_site_calls_a_guard` fails naming that site; measured when this file was written, by deleting the ``RevocationHopGuard.capture`` call in ``DicomScuDestination.__init__``. +Measured again when the two Vault ``_build_client`` sites became Guarded (vault BACKLOG #3587): +renaming the ``mount_strict_reply_adapter`` call in either one failed that test naming it, and +renaming either link of the chain from the mount to the Vault guard failed +:func:`test_every_guard_name_reaches_the_guard`. """ from __future__ import annotations @@ -84,16 +89,29 @@ #: The stdlib and truststore names. Matched only as a call: a bare ``SSLContext`` also names a type. _STDLIB_BUILDERS = frozenset({"create_default_context", "SSLContext"}) -#: Names whose call runs a revocation guard. Each one other than ``capture`` must reach ``capture`` -#: through the others (:func:`test_every_guard_name_reaches_the_guard`). -_GUARDS = frozenset( +#: The guards every other name in :data:`_GUARDS` must reach. The Vault hop guard, in +#: ``transports/strict_requests.py`` (vault BACKLOG #2983), decides through +#: ``tls_policy.revocation_hop_disposition`` and never calls ``RevocationHopGuard``; its docstring +#: says why. +_GUARD_ROOTS = frozenset( { "capture", # RevocationHopGuard.capture, matched only on that receiver + "_refuse_an_unrevoked_vault_hop", + } +) + +#: Names whose call runs a revocation guard. Each one outside :data:`_GUARD_ROOTS` must reach a root +#: through the others (:func:`test_every_guard_name_reaches_the_guard`). +_GUARDS = _GUARD_ROOTS | frozenset( + { "refuse_unrevoked_verified_hop", "_refuse_store_revocation", "_refuse_forward_revocation", "_refuse_idp_revocation", "idp_revocation_guards", + # transports/strict_requests.py, the Vault hops (vault BACKLOG #2983) + "refuse_an_unrevoked_hop", # StrictReplyAdapter's, at the mount and before each send + "mount_strict_reply_adapter", } ) @@ -101,6 +119,13 @@ _UNGATED = ( "an ungated verifying hop, listed in docs/DEPLOYMENT.md under 'Ungated verifying TLS hop'" ) +#: What the two Vault entries' guard covers, and what this census does not follow there. +_VAULT_GUARD = ( + "through mount_strict_reply_adapter, which runs _refuse_an_unrevoked_vault_hop at " + "construction, and the adapter runs it again before each send (vault BACKLOG #2983). The leg to an https:// proxy has its own guard, " + "_refuse_an_unrevoked_proxy_leg (vault BACKLOG #3561), handed on as a value this census " + "does not follow" +) _TRAY = "the Windows tray probing the engine's own API on this box" _LISTENER = ( "a SERVER context for an inbound listener. Its client-certificate revocation gate is " @@ -162,6 +187,12 @@ def __post_init__(self) -> None: note="the SMART and OAuth2 token hops" ), "messagefoundry/transports/soap.py::SoapDestination.__init__": Guarded(), + "messagefoundry/config/secretprovider_vault.py::_build_client": Guarded( + note=f"the Vault KV secret provider hop, {_VAULT_GUARD}" + ), + "messagefoundry/store/keyprovider_vault.py::_build_client": Guarded( + note=f"the Vault Transit key provider hop, which the Transit cipher shares, {_VAULT_GUARD}" + ), # --- exempt: ungated verifying hops ------------------------------------------------------------ "messagefoundry/auth/ldap.py::LdapAuthenticator.__init__": Exempt(f"LDAPS: {_UNGATED}"), "messagefoundry/pipeline/alert_sinks.py::_build_no_redirect_opener": Exempt( @@ -178,15 +209,6 @@ def __post_init__(self) -> None: "messagefoundry/transports/ai_broker.py::AiBroker.__init__": Exempt( f"the AI-broker endpoint: {_UNGATED}" ), - "messagefoundry/config/secretprovider_vault.py::_build_client": Exempt( - "the Vault KV secret provider hop. Ungated, and NOT in docs/DEPLOYMENT.md's ungated table: " - "its requests-based client takes one CA bundle and no revocation flag, so no CRL can reach " - "it (tls_policy refuses [tls].crl_file there). Found by this census; unfiled" - ), - "messagefoundry/store/keyprovider_vault.py::_build_client": Exempt( - "the Vault Transit key provider hop. Ungated, and NOT in docs/DEPLOYMENT.md's ungated " - "table, for the same reason as the KV provider above. Found by this census; unfiled" - ), # --- exempt: not a partner hop ----------------------------------------------------------------- "messagefoundry/apiclient/client.py::_build_verify_context": Exempt( "the engine's own HTTP client library (ADR 0088), dialing the engine's API from a client " @@ -376,16 +398,18 @@ def test_every_guarded_site_calls_a_guard() -> None: def test_every_guard_name_reaches_the_guard() -> None: """A wrapper that stopped calling the guard would leave every site it covers green above. - Followed through the wrappers to ``capture``, so two wrappers that only call each other are - hollow too. Keyed by bare function name, so a same-named def elsewhere in the package could - stand in for a wrapper; the guard names here are unique today.""" + Followed through the wrappers to a root, so two wrappers that only call each other are hollow + too. Keyed by bare function name, so a same-named def elsewhere in the package could stand in + for a wrapper; the guard names here are unique today.""" defs = _scan().defs assert callable(getattr(RevocationHopGuard, "capture", None)) + # The Vault root is read from the scan, not imported: its module needs the [vault] extra. + unrooted = sorted(name for name in _GUARD_ROOTS - {"capture"} if name not in defs) + assert not unrooted, f"root guard name(s) no longer defined in messagefoundry/: {unrooted}" # 6 guard names when this floor was set, so 4 lets two go with no edit here (BACKLOG #3512). It - # fails a list emptied or cut to "capture" alone, over which the check below passes on nothing. - assert "capture" in _GUARDS, sorted(_GUARDS) + # fails a list cut to the roots alone, over which the check below passes on nothing. assert len(_GUARDS) >= 4, f"only {len(_GUARDS)} guard names, floor 4" - reaches = {"capture"} + reaches = set(_GUARD_ROOTS) grew = True while grew: grew = False @@ -394,7 +418,7 @@ def test_every_guard_name_reaches_the_guard() -> None: reaches.add(name) grew = True hollow = sorted(_GUARDS - reaches) - assert not hollow, f"guard name(s) that never reach RevocationHopGuard.capture: {hollow}" + assert not hollow, f"guard name(s) that never reach a root in {sorted(_GUARD_ROOTS)}: {hollow}" def test_every_builder_name_is_still_defined() -> None: From 3ffbdc45b5a68dbbb31eb8f9c1764c21b1f8ea4e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 27 Oct 2026 17:34:36 -0500 Subject: [PATCH 2/5] Census review round one: tie the Vault root to its authority Round one of code review on the census change found these, all fixed: - The unlisted-site message now names mount_strict_reply_adapter for an hvac Vault client, not only RevocationHopGuard. - Each guard name but capture must have exactly one def in the package, so a same-named method cannot stand in for a wrapper. - A new test ties the Vault root to _crossed_unrevoked and revocation_hop_disposition. - The Vault note says no CRL lever is built, and what the census does not follow. The floor counts wrappers, not roots. The two new controls were sabotaged and went red. BACKLOG #3587 Proposed PR title: Revocation-guard census records the two Vault client sites as guarded (BACKLOG #3587) Banner: SHIPPED -- tests/test_revocation_guard_census.py lists both Vault _build_client sites as Guarded, tied to _refuse_an_unrevoked_vault_hop. --- tests/test_revocation_guard_census.py | 76 ++++++++++++++++++++------- 1 file changed, 56 insertions(+), 20 deletions(-) diff --git a/tests/test_revocation_guard_census.py b/tests/test_revocation_guard_census.py index 9e193323bd..99b7632f66 100644 --- a/tests/test_revocation_guard_census.py +++ b/tests/test_revocation_guard_census.py @@ -19,9 +19,9 @@ the listener gates are a different control. **What "guarded" proves.** The site's function, or the one its entry names, calls a name in -:data:`_GUARDS`, and every such name reaches one of the two guards in :data:`_GUARD_ROOTS`. That -ties the site to a guard call. It does not prove that every branch -reaches the call, or that the call refuses: the behaviour tests (at least +:data:`_GUARDS`, and every such name reaches a guard in :data:`_GUARD_ROOTS`. That ties the site +to a guard call. It does not prove that every branch reaches the call, or that the call refuses, +or that a guard checks a CRL: the behaviour tests (at least ``tests/test_revocation_guard_remaining_hops.py``, ``tests/test_hop_refusal_revocation.py`` and ``tests/test_vault_revocation_hop_guard.py``) prove that. A second hop added inside a function that is already guarded passes here too, so read the function in review. @@ -32,7 +32,9 @@ Measured again when the two Vault ``_build_client`` sites became Guarded (vault BACKLOG #3587): renaming the ``mount_strict_reply_adapter`` call in either one failed that test naming it, and renaming either link of the chain from the mount to the Vault guard failed -:func:`test_every_guard_name_reaches_the_guard`. +:func:`test_every_guard_name_reaches_a_root`. So did a second def named ``refuse_an_unrevoked_hop`` +in another module, and renaming the Vault guard's ``_crossed_unrevoked`` call failed +:func:`test_the_vault_root_decides_through_the_authority`. """ from __future__ import annotations @@ -43,7 +45,7 @@ from pathlib import Path from messagefoundry.config.tls_policy import RevocationHopGuard -from tests._ast_sites import callee_name +from tests._ast_sites import callee_name, calls_to, find_funcs, named_func, parse_source _ROOT = Path(__file__).resolve().parents[1] _PACKAGE = _ROOT / "messagefoundry" @@ -101,7 +103,7 @@ ) #: Names whose call runs a revocation guard. Each one outside :data:`_GUARD_ROOTS` must reach a root -#: through the others (:func:`test_every_guard_name_reaches_the_guard`). +#: through the others (:func:`test_every_guard_name_reaches_a_root`). _GUARDS = _GUARD_ROOTS | frozenset( { "refuse_unrevoked_verified_hop", @@ -122,9 +124,10 @@ #: What the two Vault entries' guard covers, and what this census does not follow there. _VAULT_GUARD = ( "through mount_strict_reply_adapter, which runs _refuse_an_unrevoked_vault_hop at " - "construction, and the adapter runs it again before each send (vault BACKLOG #2983). The leg to an https:// proxy has its own guard, " - "_refuse_an_unrevoked_proxy_leg (vault BACKLOG #3561), handed on as a value this census " - "does not follow" + "construction (vault BACKLOG #2983). No CRL lever is built for these hops, so off the box " + "the hop crosses an enforcing posture only on its vault_revocation_attested pair. This census " + "follows the construction-time call only, not the adapter's check before each send, and not " + "_refuse_an_unrevoked_proxy_leg (vault BACKLOG #3561), handed on as a value" ) _TRAY = "the Windows tray probing the engine's own API on this box" _LISTENER = ( @@ -368,9 +371,10 @@ def test_every_site_is_in_the_census() -> None: unlisted = sorted(_scan().sites - _CENSUS.keys()) assert not unlisted, ( f"verifying client TLS context(s) with no census entry: {unlisted}. Add a guard " - f"(RevocationHopGuard.capture, or refuse_unrevoked_verified_hop for an https hop) and list " - f"the site as Guarded, or list it as Exempt with the reason. See ADR 0173 and " - f"docs/DEPLOYMENT.md, 'Revocation-guard behavior'." + f"(RevocationHopGuard.capture, refuse_unrevoked_verified_hop for an https hop, or " + f"mount_strict_reply_adapter for an hvac Vault client, which must not use " + f"RevocationHopGuard) and list the site as Guarded, or list it as Exempt with the reason. " + f"See ADR 0173 and docs/DEPLOYMENT.md, 'Revocation-guard behavior'." ) @@ -395,20 +399,32 @@ def test_every_guarded_site_calls_a_guard() -> None: assert not unguarded, f"census sites marked Guarded whose guard call is gone: {unguarded}" -def test_every_guard_name_reaches_the_guard() -> None: +def _def_count(name: str) -> int: + """How many ``def``s named ``name`` the package holds, nested ones and methods included.""" + return sum( + len(find_funcs(parse_source(source), name)) + for path in sorted(_PACKAGE.rglob("*.py")) + if name in (source := path.read_text(encoding="utf-8")) + ) + + +def test_every_guard_name_reaches_a_root() -> None: """A wrapper that stopped calling the guard would leave every site it covers green above. Followed through the wrappers to a root, so two wrappers that only call each other are hollow - too. Keyed by bare function name, so a same-named def elsewhere in the package could stand in - for a wrapper; the guard names here are unique today.""" + too. Keyed by bare function name, so each guard name but ``capture`` must have exactly one def + in the package: a second same-named def could otherwise stand in for a wrapper. ``capture`` is + matched only on ``RevocationHopGuard``, and ``transports/mllp.py`` defines another one.""" defs = _scan().defs assert callable(getattr(RevocationHopGuard, "capture", None)) - # The Vault root is read from the scan, not imported: its module needs the [vault] extra. - unrooted = sorted(name for name in _GUARD_ROOTS - {"capture"} if name not in defs) - assert not unrooted, f"root guard name(s) no longer defined in messagefoundry/: {unrooted}" - # 6 guard names when this floor was set, so 4 lets two go with no edit here (BACKLOG #3512). It + # Counted from the source, not imported: the Vault guard's module needs the [vault] extra. + counts = {name: _def_count(name) for name in sorted(_GUARDS - {"capture"})} + not_one = {name: n for name, n in counts.items() if n != 1} + assert not not_one, f"guard name(s) without exactly one def in messagefoundry/: {not_one}" + # 5 wrappers when this floor was set, so 3 lets two go with no edit here (BACKLOG #3512). It # fails a list cut to the roots alone, over which the check below passes on nothing. - assert len(_GUARDS) >= 4, f"only {len(_GUARDS)} guard names, floor 4" + wrappers = _GUARDS - _GUARD_ROOTS + assert len(wrappers) >= 3, f"only {len(wrappers)} guard names outside the roots, floor 3" reaches = set(_GUARD_ROOTS) grew = True while grew: @@ -421,6 +437,26 @@ def test_every_guard_name_reaches_the_guard() -> None: assert not hollow, f"guard name(s) that never reach a root in {sorted(_GUARD_ROOTS)}: {hollow}" +def test_the_vault_root_decides_through_the_authority() -> None: + """The Vault root is not ``RevocationHopGuard``, so its tie to the decision is checked here. + + It must call ``_crossed_unrevoked``, and that must call ``revocation_hop_disposition``, the + authority every other revocation guard uses. This shows the calls exist, not that their answer + is used: a root that returns first stays green here, and the behaviour tests in + ``tests/test_vault_revocation_hop_guard.py`` catch that.""" + tree = parse_source( + (_PACKAGE / "transports" / "strict_requests.py").read_text(encoding="utf-8") + ) + root = named_func(tree, "_refuse_an_unrevoked_vault_hop") + decider = named_func(tree, "_crossed_unrevoked") + assert calls_to(root, {"_crossed_unrevoked"}, bare_only=True), ( + "the Vault root no longer decides" + ) + assert calls_to(decider, {"revocation_hop_disposition"}, bare_only=True), ( + "_crossed_unrevoked no longer asks tls_policy.revocation_hop_disposition" + ) + + def test_every_builder_name_is_still_defined() -> None: """A renamed builder would drop all its callers from the census without a red.""" scan = _scan() From fc2227de5e95341ad44dbbba2857f0750d5f8db1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 27 Oct 2026 17:54:38 -0500 Subject: [PATCH 3/5] Vault hop docstrings point at the pins, and the marker scan counts every call BACKLOG #3489, the docstring items PR 2540 deferred, plus two still open. - strict_requests: cut the sentence restating vault_failure_text's rule (re-review low 5); the CA-directory paragraph drops the restated versions and date (slice 2 item 9) and no longer says a directory file "wrote no prompt" on Windows, where a prompt is not visible (item 4). - trust_anchors.anchor_cadata: the POSIX pty run follows the Windows run it is "later" than, and points at the cafile pin for its versions (item 9). - vault_failure_text and the marker test docstring point at the VaultHopRefused docstring for the CA path and CA text (batch 252 Manager finding 1). - The marked-refusal scan now must see every VaultHopRefused( call the source spells, so its floor cannot lag again (low 2 and batch 252 Manager finding 2 recurred: floor 15, 17 calls). Proposed PR title: Vault hop docstrings point at the pins, and the marker scan counts every call (BACKLOG #3489) Proposed banner: PARTIAL -- slice 2 items 4 and 9, re-review low 5 and the batch 252 Manager's two findings built on branch b342-3489b. Re-review lows 1 and 3 declined: outside PR 2298's diff, each needs its own row. Body item 7 declined: no change can give PR 2298 a third review. Closing step 2 still open: it needs the reviewer's report. --- messagefoundry/auth/trust_anchors.py | 18 +++++----- messagefoundry/transports/strict_requests.py | 37 +++++++++----------- tests/test_vault_hop_refusal_marker.py | 22 ++++++++---- 3 files changed, 40 insertions(+), 37 deletions(-) diff --git a/messagefoundry/auth/trust_anchors.py b/messagefoundry/auth/trust_anchors.py index 5493b5fda3..fd1d8ba58f 100644 --- a/messagefoundry/auth/trust_anchors.py +++ b/messagefoundry/auth/trust_anchors.py @@ -543,18 +543,16 @@ def anchor_cadata(data: bytes, spec: AnchorSpec) -> str: **An encryption header name after the first BEGIN refuses, before that load** (BACKLOG #2270), whether or not it sits inside a block. A ``Proc-Type: 4,ENCRYPTED`` header sends OpenSSL to its password callback, and ``cadata=`` sets - none, so OpenSSL falls back to its default one, which reads the terminal. The POSIX terminal run - was done later, off-version (BACKLOG #3090, 2026-10-12): on CPython 3.12.3 / OpenSSL 3.0.13 on - Linux with a pseudo-terminal, the same ``cadata=`` load wrote ``Enter PEM pass phrase:`` and was - still waiting at 5 seconds, while ``cafile=`` refused at once with ``PEM lib``. - ``tests/test_encrypted_pem_cafile_pin.py`` holds that table and pins it on each CI platform. - What was measured first, on Windows, on - CPython 3.14.6 / OpenSSL 3.5.7 in a child process with stdin on the null device: the + none, so OpenSSL falls back to its default one, which reads the terminal. Measured on Windows, + on CPython 3.14.6 / OpenSSL 3.5.7 in a child process with stdin on the null device: the ``cadata=`` load of a certificate block carrying ``Proc-Type: 4,ENCRYPTED`` and ``DEK-Info:`` did not return within 20 seconds and was killed, while ``cafile=`` on the same bytes refused at - once with ``PEM lib``. So a start or a reload could hang on this file. A CA certificate never - carries an RFC 1421 encryption header, so ``Proc-Type:`` and ``DEK-Info:`` both refuse, in any - case, anywhere after the first BEGIN of the text the load would get, and OpenSSL is never + once with ``PEM lib``. So a start or a reload could hang on this file. A later run under a POSIX + pseudo-terminal (BACKLOG #3090) saw the same ``cadata=`` load write the password prompt and + wait. ``tests/test_encrypted_pem_cafile_pin.py`` holds that run's table, with its versions, and + pins it on each CI platform. A CA certificate never carries an RFC 1421 encryption header, so + ``Proc-Type:`` and ``DEK-Info:`` both refuse, in any case, anywhere after the first BEGIN of the + text the load would get, and OpenSSL is never handed that text (:func:`has_encryption_header`). That is wider than the hang: a private-key block encrypted in that legacy header format beside a certificate loads through ``cadata=``, and refuses here, and so does the header name in a comment between two blocks or after the diff --git a/messagefoundry/transports/strict_requests.py b/messagefoundry/transports/strict_requests.py index f15ff8756e..90a67e086f 100644 --- a/messagefoundry/transports/strict_requests.py +++ b/messagefoundry/transports/strict_requests.py @@ -310,34 +310,31 @@ def _refuse_encrypted_ca_text(self) -> None: **The refusal is a** :class:`VaultHopRefused`, **like the proxy leg's CA load refusal in** :meth:`_narrow_the_proxy_leg`. urllib3 does not catch that type, so it - reaches the caller as it is, and :func:`vault_failure_text` keeps its text. The base - ``InsecureHopRefused`` would reach the caller too, but that function would give only - its type name; - :class:`VaultHopRefused` says why. A urllib3 ``SSLError`` raised here would not reach - the caller as it is. Measured with urllib3 2.8.0, one raised before the proxy has - connected comes out of requests as a ``ProxyError``. The providers would then report a - type name only. + reaches the caller as it is, and :func:`vault_failure_text` keeps its text. A urllib3 + ``SSLError`` raised here would not reach the caller as it is. Measured with urllib3 + 2.8.0, one raised before the proxy has connected comes out of requests as a + ``ProxyError``. The providers would then report a type name only. **A CA file is not checked, because the load itself refuses it.** That holds for the encryption header only: a CA file holding a private key loads, and nothing here refuses it (BACKLOG #3745 covers the CA text alone). ``ca_certs`` is a path. Measured, a path load of an encrypted ``CERTIFICATE`` or ``X509 CRL`` block raised - ``PEM lib`` at once and wrote no prompt. A check here would also open the file twice. - ``tests/test_encrypted_pem_cafile_pin.py`` holds both measurements, with the versions + ``PEM lib`` at once, and no prompt was seen. A check here would also open the file + twice. ``tests/test_encrypted_pem_cafile_pin.py`` holds both measurements, with the versions and terminal conditions, and pins them on each platform CI runs. **A CA directory is not refused, because it was not seen to wait.** ``ca_cert_dir`` reaches OpenSSL as ``capath``. OpenSSL reads a directory's files when it verifies a peer, not when the directory is named, so a check of the text handed to the load, like the one above, does not cover one. So it was measured, for Python's ``ssl`` only, - with a real handshake (2026-10-12), on CPython 3.14.6 / OpenSSL 3.5.7 on - Windows and, off-version, on CPython 3.12.3 / OpenSSL 3.0.13 on Linux. A directory - file holding an encrypted ``CERTIFICATE`` or ``X509 CRL`` block, under the name - OpenSSL looks up, wrote no prompt and never waited, in every terminal condition - tried, a pseudo-terminal among them. The handshake refused the peer at once with a - verification error, so the hop fails closed. ``tests/test_encrypted_pem_capath_pin.py`` - holds that table and its controls, and pins it on each platform CI runs. A directory - does reach both loads today. With neither ``MEFOR_STORE_VAULT_CA_FILE`` nor + with a real handshake on Windows and Linux. A directory file holding an encrypted + ``CERTIFICATE`` or ``X509 CRL`` block, under the name OpenSSL looks up, never waited, + in every terminal condition tried, a pseudo-terminal among them. The handshake refused + the peer at once with a verification error, so the hop fails closed. No prompt was + seen either; on Windows a console prompt is not visible to the caller, and + ``tests/test_encrypted_pem_capath_pin.py`` says what that reading rests on there. That + file holds the table, with its versions and controls, and pins it on each platform CI + runs. A directory does reach both loads today. With neither ``MEFOR_STORE_VAULT_CA_FILE`` nor ``MEFOR_SECRETS_VAULT_CA_FILE`` set, hvac and requests choose the CA. At least two routes can name a directory: hvac's ``VAULT_CAPATH``, used when ``VAULT_CACERT`` is not set, and requests' ``REQUESTS_CA_BUNDLE`` or ``CURL_CA_BUNDLE``. requests passes a @@ -762,9 +759,9 @@ def vault_failure_text(exc: BaseException) -> str: A refusal from this hop keeps its own text: a :class:`VaultHopRefused` from the hop checks, or an :class:`~messagefoundry.transports.bounded_read.EgressReplyError` from the reply reader. Both are built here and in ``bounded_read`` from the hop's fixed connector label and fixed - reason text. They name no part of the address, the proxy URL, the token or the reply. Any - other failure gives only its type name, as before, because its text can echo ciphertext, a - request body or a URL. + reason text. They name no part of the address, the proxy URL, the token or the reply, and + :class:`VaultHopRefused` lists what else its text never names. Any other failure gives only + its type name, as before, because its text can echo ciphertext, a request body or a URL. **A bare** ``InsecureHopRefused`` **gives only its type name too (vault BACKLOG #3247).** :class:`VaultHopRefused` says why. diff --git a/tests/test_vault_hop_refusal_marker.py b/tests/test_vault_hop_refusal_marker.py index 982dcd6906..f059f0cc87 100644 --- a/tests/test_vault_hop_refusal_marker.py +++ b/tests/test_vault_hop_refusal_marker.py @@ -19,6 +19,7 @@ from __future__ import annotations import ast +import re from pathlib import Path from typing import Any @@ -140,15 +141,22 @@ def _is_fixed_text(call: ast.Call, constants: frozenset[str]) -> bool: def test_every_marked_refusal_is_built_from_fixed_text() -> None: - """The marker means the text names no address, proxy URL, token or reply. This holds each - site to a shape that cannot interpolate one. It does not read a listed helper's own text.""" - tree = parse_source(_HOME.read_text(encoding="utf-8")) + """The marker means the text names nothing the ``VaultHopRefused`` docstring says it never + names. This holds each site to a shape that cannot interpolate a caller's value. It does not + read a listed helper's own text.""" + source = _HOME.read_text(encoding="utf-8") + tree = parse_source(source) constants = _string_constants(tree) sites = call_sites(tree, _MARKER) - # 15 marked refusals when this floor was last set (BACKLOG #3458, #3745). A scan that sees few - # of them, or none, leaves the rest unchecked. Lower the floor only if refusals were really - # merged or removed, and raise it when one is added. - assert len(sites) >= 15, f"the scan saw only {len(sites)} marked refusal(s) in {_HOME.name}" + # A scan that sees few of the refusals, or none, leaves the rest unchecked. So the AST scan + # must see every call the source text spells, and a hand-kept floor cannot lag (BACKLOG + # #3489: one sat at 6, then 15, while the module grew). The fixed floor only stops a module + # that lost all of them passing on zero against zero. + spelled = len(re.findall(rf"(?= 10, f"the scan saw only {len(sites)} marked refusal(s) in {_HOME.name}" loose = [site.lineno for site in sites if not _is_fixed_text(site, constants)] assert loose == [], f"a marked refusal interpolates something at lines {loose}" # The constants the sites use today are found, so the lookup is not passing on an empty set. From ab89fb516b760bd0e24aa344449f5e0df11a97c8 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 27 Oct 2026 18:06:23 -0500 Subject: [PATCH 4/5] Review round 1: keep the hand-kept floor, keep "off-version", reflow BACKLOG #3489. Fixes from the code-review round 1 on this branch: - The text-count check on VaultHopRefused( calls went red on a mention and missed an alias, so the floor is a hand-kept 17 again. - "off-version" is back for the Linux and POSIX pty runs, and anchor_cadata says the pty case is pinned on POSIX only. - vault_failure_text points at VaultHopRefused for what the hop's text never names, rather than restating part of the list. - Docstring lines rewrapped to the 100-column limit. Proposed PR title: Vault hop docstrings point at the pins, and the marked-refusal floor matches the module (BACKLOG #3489) Proposed banner: PARTIAL -- slice 2 items 4 and 9, re-review low 5 and the batch 252 Manager's two findings built on branch b342-3489b (the floor recurred at 15 against 17 calls; now 17). Re-review lows 1 and 3 declined: outside PR 2298's diff, each needs its own row. Body item 7 declined: no change can give PR 2298 a third review. Closing step 2 still open: it needs the reviewer's report. --- messagefoundry/auth/trust_anchors.py | 15 +++---- messagefoundry/transports/strict_requests.py | 41 ++++++++++---------- tests/test_vault_hop_refusal_marker.py | 21 ++++------ 3 files changed, 36 insertions(+), 41 deletions(-) diff --git a/messagefoundry/auth/trust_anchors.py b/messagefoundry/auth/trust_anchors.py index fd1d8ba58f..81a8a50a14 100644 --- a/messagefoundry/auth/trust_anchors.py +++ b/messagefoundry/auth/trust_anchors.py @@ -547,13 +547,14 @@ def anchor_cadata(data: bytes, spec: AnchorSpec) -> str: on CPython 3.14.6 / OpenSSL 3.5.7 in a child process with stdin on the null device: the ``cadata=`` load of a certificate block carrying ``Proc-Type: 4,ENCRYPTED`` and ``DEK-Info:`` did not return within 20 seconds and was killed, while ``cafile=`` on the same bytes refused at - once with ``PEM lib``. So a start or a reload could hang on this file. A later run under a POSIX - pseudo-terminal (BACKLOG #3090) saw the same ``cadata=`` load write the password prompt and - wait. ``tests/test_encrypted_pem_cafile_pin.py`` holds that run's table, with its versions, and - pins it on each CI platform. A CA certificate never carries an RFC 1421 encryption header, so - ``Proc-Type:`` and ``DEK-Info:`` both refuse, in any case, anywhere after the first BEGIN of the - text the load would get, and OpenSSL is never - handed that text (:func:`has_encryption_header`). That is wider than the hang: a private-key + once with ``PEM lib``. So a start or a reload could hang on this file. A later run, off-version, + under a POSIX pseudo-terminal (BACKLOG #3090) saw the same ``cadata=`` load write the password + prompt and wait. ``tests/test_encrypted_pem_cafile_pin.py`` holds that run's table, with its + versions. It pins the path loads on each CI platform, and the pseudo-terminal case on POSIX + only. A CA certificate never carries an RFC 1421 encryption header, so ``Proc-Type:`` and + ``DEK-Info:`` both refuse, in any case, anywhere after the first BEGIN of the text the load + would get, and OpenSSL is never handed that text (:func:`has_encryption_header`). That is wider + than the hang: a private-key block encrypted in that legacy header format beside a certificate loads through ``cadata=``, and refuses here, and so does the header name in a comment between two blocks or after the last one. The diff --git a/messagefoundry/transports/strict_requests.py b/messagefoundry/transports/strict_requests.py index 90a67e086f..10549b0521 100644 --- a/messagefoundry/transports/strict_requests.py +++ b/messagefoundry/transports/strict_requests.py @@ -317,29 +317,30 @@ def _refuse_encrypted_ca_text(self) -> None: **A CA file is not checked, because the load itself refuses it.** That holds for the encryption header only: a CA file holding a private key loads, and nothing here - refuses it (BACKLOG #3745 covers the CA text alone). ``ca_certs`` is a - path. Measured, a path load of an encrypted ``CERTIFICATE`` or ``X509 CRL`` block raised - ``PEM lib`` at once, and no prompt was seen. A check here would also open the file - twice. ``tests/test_encrypted_pem_cafile_pin.py`` holds both measurements, with the versions + refuses it (BACKLOG #3745 covers the CA text alone). ``ca_certs`` is a path. Measured, + a path load of an encrypted ``CERTIFICATE`` or ``X509 CRL`` block raised ``PEM lib`` + at once, and no prompt was seen. A check here would also open the file twice. + ``tests/test_encrypted_pem_cafile_pin.py`` holds both measurements, with the versions and terminal conditions, and pins them on each platform CI runs. **A CA directory is not refused, because it was not seen to wait.** ``ca_cert_dir`` reaches OpenSSL as ``capath``. OpenSSL reads a directory's files when it verifies a peer, not when the directory is named, so a check of the text handed to the load, like the one above, does not cover one. So it was measured, for Python's ``ssl`` only, - with a real handshake on Windows and Linux. A directory file holding an encrypted - ``CERTIFICATE`` or ``X509 CRL`` block, under the name OpenSSL looks up, never waited, - in every terminal condition tried, a pseudo-terminal among them. The handshake refused - the peer at once with a verification error, so the hop fails closed. No prompt was - seen either; on Windows a console prompt is not visible to the caller, and - ``tests/test_encrypted_pem_capath_pin.py`` says what that reading rests on there. That - file holds the table, with its versions and controls, and pins it on each platform CI - runs. A directory does reach both loads today. With neither ``MEFOR_STORE_VAULT_CA_FILE`` nor - ``MEFOR_SECRETS_VAULT_CA_FILE`` set, hvac and requests choose the CA. At least two - routes can name a directory: hvac's ``VAULT_CAPATH``, used when ``VAULT_CACERT`` is not - set, and requests' ``REQUESTS_CA_BUNDLE`` or ``CURL_CA_BUNDLE``. requests passes a - directory on as ``ca_cert_dir``. A refusal here would break a hop that works and stop - nothing that was seen to wait, so the directory is loaded as before.""" + with a real handshake on Windows and, off-version, on Linux. A directory file holding + an encrypted ``CERTIFICATE`` or ``X509 CRL`` block, under the name OpenSSL looks up, + never waited, in every terminal condition tried, a pseudo-terminal among them. The + handshake refused the peer at once with a verification error, so the hop fails + closed. No prompt was seen either. On Windows a console prompt is not visible to the + caller. ``tests/test_encrypted_pem_capath_pin.py`` says what the Windows reading rests + on. It holds the table, with its versions and controls, and pins it on each platform + CI runs. A directory does reach both loads today. With neither + ``MEFOR_STORE_VAULT_CA_FILE`` nor ``MEFOR_SECRETS_VAULT_CA_FILE`` set, hvac and + requests choose the CA. At least two routes can name a directory: hvac's + ``VAULT_CAPATH``, used when ``VAULT_CACERT`` is not set, and requests' + ``REQUESTS_CA_BUNDLE`` or ``CURL_CA_BUNDLE``. requests passes a directory on as + ``ca_cert_dir``. A refusal here would break a hop that works and stop nothing that was + seen to wait, so the directory is loaded as before.""" if isinstance(self.ca_cert_data, str) and has_encryption_header(self.ca_cert_data): # Fixed text: it names no part of the CA text it refuses. raise VaultHopRefused( @@ -759,9 +760,9 @@ def vault_failure_text(exc: BaseException) -> str: A refusal from this hop keeps its own text: a :class:`VaultHopRefused` from the hop checks, or an :class:`~messagefoundry.transports.bounded_read.EgressReplyError` from the reply reader. Both are built here and in ``bounded_read`` from the hop's fixed connector label and fixed - reason text. They name no part of the address, the proxy URL, the token or the reply, and - :class:`VaultHopRefused` lists what else its text never names. Any other failure gives only - its type name, as before, because its text can echo ciphertext, a request body or a URL. + reason text. What the hop's text never names is listed once, in :class:`VaultHopRefused`. + Any other failure gives only its type name, as + before, because its text can echo ciphertext, a request body or a URL. **A bare** ``InsecureHopRefused`` **gives only its type name too (vault BACKLOG #3247).** :class:`VaultHopRefused` says why. diff --git a/tests/test_vault_hop_refusal_marker.py b/tests/test_vault_hop_refusal_marker.py index f059f0cc87..afe8755d3f 100644 --- a/tests/test_vault_hop_refusal_marker.py +++ b/tests/test_vault_hop_refusal_marker.py @@ -19,7 +19,6 @@ from __future__ import annotations import ast -import re from pathlib import Path from typing import Any @@ -142,21 +141,15 @@ def _is_fixed_text(call: ast.Call, constants: frozenset[str]) -> bool: def test_every_marked_refusal_is_built_from_fixed_text() -> None: """The marker means the text names nothing the ``VaultHopRefused`` docstring says it never - names. This holds each site to a shape that cannot interpolate a caller's value. It does not - read a listed helper's own text.""" - source = _HOME.read_text(encoding="utf-8") - tree = parse_source(source) + names. This holds each site to a shape built only from the connector label, literals, module + constants and the listed helpers. It does not read a listed helper's own text.""" + tree = parse_source(_HOME.read_text(encoding="utf-8")) constants = _string_constants(tree) sites = call_sites(tree, _MARKER) - # A scan that sees few of the refusals, or none, leaves the rest unchecked. So the AST scan - # must see every call the source text spells, and a hand-kept floor cannot lag (BACKLOG - # #3489: one sat at 6, then 15, while the module grew). The fixed floor only stops a module - # that lost all of them passing on zero against zero. - spelled = len(re.findall(rf"(?= 10, f"the scan saw only {len(sites)} marked refusal(s) in {_HOME.name}" + # 17 marked refusals when this floor was last set (BACKLOG #3489). A scan that sees few of + # them, or none, leaves the rest unchecked. Lower the floor only if refusals were really + # merged or removed, and raise it when one is added. + assert len(sites) >= 17, f"the scan saw only {len(sites)} marked refusal(s) in {_HOME.name}" loose = [site.lineno for site in sites if not _is_fixed_text(site, constants)] assert loose == [], f"a marked refusal interpolates something at lines {loose}" # The constants the sites use today are found, so the lookup is not passing on an empty set. From 262b364edc6aea11b6a498f8570f3c79c61bffb1 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 27 Oct 2026 19:47:10 -0500 Subject: [PATCH 5/5] Guard the census one-def walk with a floor, and pin the guard roots test_every_guard_name_reaches_a_root asserted `not not_one` over a walk with no earlier count, so the absence-assert lint failed all three pytest legs on PR 2586. An earlier `assert len(counts) >= 4` now fails a walk over nothing. `_GUARD_ROOTS` is pinned exactly, so a hollow wrapper moved into the roots needs an edit to the test that review sees (PR 2586 open finding 11). Proposed PR title: Vault revocation census and docstrings: the two Vault client sites are recorded as guarded, and the hop docstrings point at the pins (BACKLOG #3587, #3489) BACKLOG #3587 --- tests/test_revocation_guard_census.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_revocation_guard_census.py b/tests/test_revocation_guard_census.py index 99b7632f66..44f4ee1780 100644 --- a/tests/test_revocation_guard_census.py +++ b/tests/test_revocation_guard_census.py @@ -417,8 +417,13 @@ def test_every_guard_name_reaches_a_root() -> None: matched only on ``RevocationHopGuard``, and ``transports/mllp.py`` defines another one.""" defs = _scan().defs assert callable(getattr(RevocationHopGuard, "capture", None)) + # Pinned exactly, so a hollow wrapper moved into the roots needs an edit here that review sees. + assert sorted(_GUARD_ROOTS) == ["_refuse_an_unrevoked_vault_hop", "capture"], _GUARD_ROOTS # Counted from the source, not imported: the Vault guard's module needs the [vault] extra. counts = {name: _def_count(name) for name in sorted(_GUARDS - {"capture"})} + # 8 names when this floor was set; 4 is the Vault root plus the wrapper floor below. It fails a + # walk over nothing, over which the one-def check would pass. + assert len(counts) >= 4, f"only {len(counts)} guard names besides capture, floor 4" not_one = {name: n for name, n in counts.items() if n != 1} assert not not_one, f"guard name(s) without exactly one def in messagefoundry/: {not_one}" # 5 wrappers when this floor was set, so 3 lets two go with no edit here (BACKLOG #3512). It