From 09767bfb9a2558297827e9c0f888001604135e53 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 10:54:38 -0500 Subject: [PATCH 1/5] test(crypto): sweep the "encrypted at all" at-rest assertions off the frozen v1 prefix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `[store].aad_bind` now defaults True (#38), so the default at-rest format is mfenc:v2. 41 test assertions anchored on `PREFIX` ("mfenc:v1:") whose actual claim is "this value is enciphered at rest at all" — a v1-only spelling that a v2 value silently fails. They move to the version-agnostic `MARKER_PREFIX` ("mfenc:"), which is the same anchor the store's own find-all/migration `mfenc:%` LIKE patterns and `is_encrypted()` already use. Evidence, not assertion. Under the ASVS correctness net `MEFOR_TEST_FORCE_AAD_BIND=1` (conftest forces every AesGcmCipher to the v2 writer), the swept files went 9 failed -> 1 failed. The one remaining failure is `test_store_aad_binding::test_v1_rows_still_read_under_aad_bind`, a site this commit deliberately does NOT touch: it fails on both branches because forcing v2 destroys its v1 premise, which is exactly what makes it the liveness receipt that the flag is live. DELIBERATELY LEFT as v1 (CRYPTO-1 / ADR 0019 coverage a sweep would DELETE), each now carrying a comment saying so — their absence is what made this dangerous to review: - the frozen-writer set (test_store_encryption: the _v1_blob oracle, the byte-identity gate, test_default_writer_is_v1_not_v2) - premise assertions whose point is "the row really IS v1" so a dual-read/no-rotation proof cannot go vacuous (test_v2_active_decrypts_v1_without_rotation, test_store_aad_binding, test_keyprovider x2, test_keyprovider_vault) - field-POSITION pins, where "mfenc::" is a string no writer emits, so widening makes the positive unsatisfiable and the negative unfailable (test_key_id_is_a_fingerprint_not_zero, the legacy key_id='0' fixture) - undecryptable-payload injections (test_batch_claim_fifo, test_claim_fifo_heads): the marker version must be one the cipher DISPATCHES on, or the poison takes the unknown-version branch instead of the base64 failure the test is about Three checks got strictly stronger rather than merely wider: - the rotation assertion now anchors on `cipher.active_marker_prefix`, keeping the "re-encrypted under the ACTIVE key" half of the proof that a bare MARKER_PREFIX would have dropped - three NEGATIVE leak/plaintext checks (`not startswith` / `not in`) now exclude EVERY marker version; as v1-only they would have passed silently on a leaked or already-encrypted v2 value - test_ack_sent_store's manual decrypt now passes the cell AAD the store wrote with. Pre-existing and previously MASKED by the v1 assertion failing first; unmasked once the sweep let the test reach it. Harmless on v1 (that reader ignores the caller's aad by design). Comments naming "v1"/"the v1 marker" at swept sites were updated so they no longer lie. Verification (SQLite leg, from the worktree): 9135 passed, 816 skipped, 0 failed; ruff check + ruff format --check clean. Non-vacuity proven by injecting one regression (AesGcmCipher.encrypt returns plaintext): two swept assertions FAIL on a plaintext at-rest value and pass without it. --- tests/test_ack_sent_store.py | 25 ++++++++----- tests/test_alert_state.py | 6 ++-- tests/test_batch_claim_fifo.py | 4 +++ tests/test_binary_carriage.py | 4 +-- tests/test_claim_fifo_heads.py | 4 +++ tests/test_connection_event_store.py | 6 ++-- tests/test_content_search.py | 4 +-- tests/test_ed_documents_e2e.py | 10 +++--- tests/test_keyprovider.py | 3 ++ tests/test_keyprovider_vault.py | 2 ++ tests/test_message_export.py | 7 ++-- tests/test_postgres_store.py | 15 ++++---- tests/test_sqlserver_store.py | 52 +++++++++++++++------------- tests/test_staged_pipeline.py | 4 +-- tests/test_store_encryption.py | 52 +++++++++++++++++++--------- tests/test_transform_state.py | 9 ++--- 16 files changed, 130 insertions(+), 77 deletions(-) diff --git a/tests/test_ack_sent_store.py b/tests/test_ack_sent_store.py index 87d8b80e..1952a3ec 100644 --- a/tests/test_ack_sent_store.py +++ b/tests/test_ack_sent_store.py @@ -12,7 +12,7 @@ import sqlite3 from pathlib import Path -from messagefoundry.store.crypto import PREFIX, generate_key, make_cipher +from messagefoundry.store.crypto import MARKER_PREFIX, cell_aad, generate_key, make_cipher from messagefoundry.store.store import MessageStore ADT = "MSH|^~\\&|S|F|R|RF|20260101||ADT^A01|MSG1|P|2.5.1\rPID|1||100^^^H^MR||DOE^JANE\r" @@ -22,8 +22,11 @@ def _response_rows(db_path: Path) -> list[tuple]: con = sqlite3.connect(db_path) try: + # message_id + response_seq trail the display columns so existing positional indices hold; + # they are the row half of the cell AAD the body is bound under (ADR 0019). return con.execute( - "SELECT destination_name, kind, ack_code, ack_phase, body FROM response" + "SELECT destination_name, kind, ack_code, ack_phase, body, message_id, response_seq " + "FROM response" ).fetchall() finally: con.close() @@ -47,14 +50,20 @@ async def test_aa_body_encrypted_when_store_encrypted(tmp_path: Path) -> None: ack = next(r for r in rows if r.kind == "ack_sent") assert ack.ack_code == "AA" and ack.ack_phase == "ingest" assert ack.body == AA # decrypted round-trip - # On disk the body is ciphertext, never the plaintext ACK: it carries the v1 encrypted marker, - # is not the plaintext frame, and decrypts back to exactly it. Assert the decrypt round-trip - # (deterministic) rather than `"MSA" not in ` — that old substring check flaked - # because a base64 ciphertext randomly contains that 3-char run (base64 alphabet includes M/S/A). + # On disk the body is ciphertext, never the plaintext ACK: it carries the mfenc: encrypted + # marker, is not the plaintext frame, and decrypts back to exactly it. Assert the decrypt + # round-trip (deterministic) rather than `"MSA" not in ` — that old substring check + # flaked because a base64 ciphertext randomly contains that 3-char run (alphabet includes M/S/A). + # The claim is encryptedness, so anchor on the VERSION-AGNOSTIC marker: the at-rest format + # follows [store].aad_bind (v2 by default), and a v1-only prefix would silently fail on it. disk = next(r for r in _response_rows(db) if r[1] == "ack_sent") - assert disk[4].startswith(PREFIX) # stored under the encrypted marker, not in the clear + assert disk[4].startswith(MARKER_PREFIX) # under the encrypted marker, not in the clear assert disk[4] != AA - assert cipher.decrypt(disk[4]) == AA # and it genuinely encrypts the AA frame + # Decrypt under the SAME cell AAD the store wrote with (ASVS 11.3.3 / ADR 0019): a v2 value is + # bound to its (table, column, row) cell, so a bare decrypt fails closed on one. Harmless on a + # v1 value — that reader ignores the caller's aad by design (dual-read). + aad = cell_aad("response", "body", disk[5], disk[0], disk[6]) + assert cipher.decrypt(disk[4], aad=aad) == AA # and it genuinely encrypts the AA frame finally: await store.close() diff --git a/tests/test_alert_state.py b/tests/test_alert_state.py index 8c7720ac..c35020e7 100644 --- a/tests/test_alert_state.py +++ b/tests/test_alert_state.py @@ -17,7 +17,7 @@ from messagefoundry.config.settings import AlertRule, AlertSeverity from messagefoundry.pipeline.alert_sinks import NotifierAlertSink -from messagefoundry.store.crypto import PREFIX, generate_key, make_cipher +from messagefoundry.store.crypto import MARKER_PREFIX, generate_key, make_cipher from messagefoundry.store.store import MessageStore # --- store lifecycle --------------------------------------------------------- @@ -193,7 +193,9 @@ async def test_reason_encrypted_at_rest(tmp_path: Path) -> None: con = sqlite3.connect(db) try: raw = con.execute("SELECT reason FROM alert_instance").fetchone()[0] - assert isinstance(raw, str) and raw.startswith(PREFIX) # ciphertext on disk + # Version-agnostic marker: the claim is "enciphered at rest", and the at-rest format follows + # [store].aad_bind (v2 by default) — a v1-only prefix would be a latent false failure. + assert isinstance(raw, str) and raw.startswith(MARKER_PREFIX) # ciphertext on disk assert "refused" not in raw finally: con.close() diff --git a/tests/test_batch_claim_fifo.py b/tests/test_batch_claim_fifo.py index 044d8a37..5772d664 100644 --- a/tests/test_batch_claim_fifo.py +++ b/tests/test_batch_claim_fifo.py @@ -311,6 +311,10 @@ async def test_t8_undecryptable_interior_dead_lettered_tail_survives( channel = "IB_T8" mids = await _seed_ingress(enc, channel, [100.0, 101.0, 102.0]) # Corrupt row2's payload to an undecryptable blob (keep the marker so it routes through decrypt). + # The v1 marker is DELIBERATE (do not "sweep" it to a bare mfenc:): the version must be one the + # cipher DISPATCHES on, so the failure lands in the base64 decode inside decrypt. A bare "mfenc:" + # takes the unknown-marker-version branch instead — a different fail-closed path than the one + # this test claims to exercise. await enc._db.execute( "UPDATE queue SET payload=? WHERE message_id=? AND stage=?", ("mfenc:v1:not-base64-$$$", mids[1], Stage.INGRESS.value), diff --git a/tests/test_binary_carriage.py b/tests/test_binary_carriage.py index 44cc66b8..1a455acf 100644 --- a/tests/test_binary_carriage.py +++ b/tests/test_binary_carriage.py @@ -33,7 +33,7 @@ ) from messagefoundry.pipeline.dryrun import route_only, transform_one from messagefoundry.pipeline.wiring_runner import RegistryRunner -from messagefoundry.store.crypto import PREFIX, generate_key, make_cipher +from messagefoundry.store.crypto import MARKER_PREFIX, generate_key, make_cipher from messagefoundry.store.store import MessageStore # Byte fixtures that break a latin-1/TEXT round-trip: every value 0x00–0xFF (incl. NUL + high bytes), @@ -186,7 +186,7 @@ async def test_binary_carriage_survives_encrypted_store(tmp_path: Path) -> None: record = await store.get_message(mid) finally: await store.close() - assert _raw_at_rest(db).startswith(PREFIX) # outer layer: encrypted on disk + assert _raw_at_rest(db).startswith(MARKER_PREFIX) # outer layer: encrypted on disk assert record is not None and record["raw"] == carried # decrypts to the inner mfb64: form assert RawMessage(record["raw"], "dicom").raw_bytes == DICOM_LIKE diff --git a/tests/test_claim_fifo_heads.py b/tests/test_claim_fifo_heads.py index 16ec56ce..194ab732 100644 --- a/tests/test_claim_fifo_heads.py +++ b/tests/test_claim_fifo_heads.py @@ -575,6 +575,10 @@ async def test_poison_rows_dead_lettered_dropped_and_lane_rearmed( key = base64.b64encode(b"\x11" * 32).decode("ascii") enc = await MessageStore.open(tmp_path / "heads.db", cipher=make_cipher(key, [])) try: + # The injected "mfenc:v1:not-base64-$$$" payloads below are undecryptable ON PURPOSE, and the + # v1 marker is DELIBERATE (do not "sweep" it to a bare mfenc:): the version must be one the + # cipher DISPATCHES on, so the failure lands in the base64 decode inside decrypt. A bare + # "mfenc:" takes the unknown-marker-version branch — a different fail-closed path. # Lane 1: a single poison HEAD → dropped + DEAD + the lane re-arms. a = await _seed_ingress(enc, "IB_HP1", [100.0]) await enc._db.execute( diff --git a/tests/test_connection_event_store.py b/tests/test_connection_event_store.py index 5629067c..09767a94 100644 --- a/tests/test_connection_event_store.py +++ b/tests/test_connection_event_store.py @@ -12,7 +12,7 @@ import sqlite3 from pathlib import Path -from messagefoundry.store.crypto import PREFIX, generate_key, make_cipher +from messagefoundry.store.crypto import MARKER_PREFIX, generate_key, make_cipher from messagefoundry.store.store import MessageStore ADT = "MSH|^~\\&|S|F|R|RF|20260101||ADT^A01|MSG1|P|2.5.1\rPID|1||100^^^H^MR||DOE^JANE\r" @@ -90,7 +90,9 @@ async def test_reason_encrypted_at_rest(tmp_path: Path) -> None: assert _col_at_rest(db, "kind") == "framing_error" assert _col_at_rest(db, "connection") == "IB" reason_disk = _col_at_rest(db, "reason") - assert isinstance(reason_disk, str) and reason_disk.startswith(PREFIX) + # The version-agnostic marker: which columns are enciphered is the claim, not which mfenc + # format the writer emits (that follows [store].aad_bind, v2 by default). + assert isinstance(reason_disk, str) and reason_disk.startswith(MARKER_PREFIX) assert "boom" not in reason_disk # …and the read path decrypts it back events = await store.list_connection_events() diff --git a/tests/test_content_search.py b/tests/test_content_search.py index a8184197..490d34e5 100644 --- a/tests/test_content_search.py +++ b/tests/test_content_search.py @@ -31,7 +31,7 @@ make_spec, row_matches, ) -from messagefoundry.store.crypto import PREFIX, generate_key, make_cipher +from messagefoundry.store.crypto import MARKER_PREFIX, generate_key, make_cipher from messagefoundry.store.store import MessageStore PW = "a-strong-test-passphrase" # ≥15, satisfies the ASVS policy @@ -120,7 +120,7 @@ async def test_content_match_on_encrypted_store(tmp_path: Path) -> None: at_rest = [str(r[0]) for r in con.execute("SELECT raw FROM messages").fetchall()] finally: con.close() - assert all(v.startswith(PREFIX) and "JANE" not in v for v in at_rest) + assert all(v.startswith(MARKER_PREFIX) and "JANE" not in v for v in at_rest) spec = make_spec(content="JANE", field_path=None, field_value=None) result = await store.search_messages(spec) diff --git a/tests/test_ed_documents_e2e.py b/tests/test_ed_documents_e2e.py index aef6a381..bc14661a 100644 --- a/tests/test_ed_documents_e2e.py +++ b/tests/test_ed_documents_e2e.py @@ -37,7 +37,7 @@ from messagefoundry.parsing.message import Message from messagefoundry.pipeline.wiring_runner import RegistryRunner from messagefoundry.store import MessageStatus, MessageStore -from messagefoundry.store.crypto import PREFIX, generate_key, make_cipher +from messagefoundry.store.crypto import MARKER_PREFIX, generate_key, make_cipher from messagefoundry.transports import DeliveryError from messagefoundry.transports.mllp import MLLPDestination, MLLPSource, build_ack @@ -170,11 +170,13 @@ async def test_base64_pdf_encrypted_at_rest(tmp_path: Path) -> None: finally: await store.close() - # On disk the PDF base64 is AES-256-GCM ciphertext, never plaintext. + # On disk the PDF base64 is AES-256-GCM ciphertext, never plaintext. Anchor on the version-agnostic + # mfenc: marker — the claim is the algorithm/encryptedness, and the marker format follows + # [store].aad_bind (v2 by default), so pinning v1 here would be a latent false failure. at_rest_raw = _raw_at_rest(db, column="raw", table="messages") at_rest_payload = _raw_at_rest(db, column="payload", table="queue") - assert at_rest_raw.startswith(PREFIX) - assert at_rest_payload.startswith(PREFIX) + assert at_rest_raw.startswith(MARKER_PREFIX) + assert at_rest_payload.startswith(MARKER_PREFIX) assert source_b64 not in at_rest_raw # the document never hits disk in the clear assert source_b64 not in at_rest_payload diff --git a/tests/test_keyprovider.py b/tests/test_keyprovider.py index 4bb276f2..8733313d 100644 --- a/tests/test_keyprovider.py +++ b/tests/test_keyprovider.py @@ -247,6 +247,9 @@ def retired_keys(self) -> list[str]: return self._retired # A row written by today's cipher under KEY_A... + # DELIBERATELY v1 (do not sweep to MARKER_PREFIX): the acceptance criterion this test is named for + # is "decrypts an EXISTING mfenc:v1 row with NO rotation", so establishing that the fixture really + # is v1 is the premise, not incidental. make_cipher's default writer is the frozen v1 one. token = make_cipher(KEY_A).encrypt(ADT) assert token.startswith(PREFIX) diff --git a/tests/test_keyprovider_vault.py b/tests/test_keyprovider_vault.py index 88e70306..d7bf5b97 100644 --- a/tests/test_keyprovider_vault.py +++ b/tests/test_keyprovider_vault.py @@ -79,6 +79,8 @@ def test_vault_active_key_unwraps_and_decrypts_without_rotation( assert transit.calls == [(_TRANSIT_KEY, _WRAPPED_DEK)] # A row written under KEY_A decrypts with the provider's key — NO re-encryption, mfenc:v1 unchanged. + # DELIBERATELY v1 (do not sweep to MARKER_PREFIX): "an existing v1 row survives a KeyProvider swap + # byte-for-byte" is the claim, so the fixture being v1 is the premise of the test. token = make_cipher(KEY_A).encrypt(ADT) assert token.startswith(PREFIX) cipher = make_cipher(active, provider.retired_keys()) diff --git a/tests/test_message_export.py b/tests/test_message_export.py index 99cfc8e4..e59dfd25 100644 --- a/tests/test_message_export.py +++ b/tests/test_message_export.py @@ -25,7 +25,7 @@ from messagefoundry.auth.tokens import hash_token from messagefoundry.config.settings import AuthSettings from messagefoundry.pipeline import Engine -from messagefoundry.store.crypto import PREFIX, generate_key, make_cipher +from messagefoundry.store.crypto import MARKER_PREFIX, generate_key, make_cipher from messagefoundry.store.store import MessageStore PW = "a-strong-test-passphrase" # ≥15, satisfies the ASVS policy @@ -174,7 +174,10 @@ async def test_export_save_all_streams_decrypted_bodies(engine: Engine) -> None: raws = {row["control_id"]: row["raw"] for row in rows} assert "DOE^JANE" in str(raws["MSGA"]) assert str(raws["MSGA"]).startswith("MSH") # not the mfenc: ciphertext - assert PREFIX not in str(raws["MSGA"]) + # A NEGATIVE leak check, so it must exclude EVERY at-rest marker version: a v1-only spelling + # would pass silently on a leaked v2 blob instead of failing. Non-vacuous — the synthetic ADT + # plaintext contains no "mfenc:" at all, and the startswith("MSH") above pins the shape. + assert MARKER_PREFIX not in str(raws["MSGA"]) async def test_export_save_selected_by_ids(engine: Engine) -> None: diff --git a/tests/test_postgres_store.py b/tests/test_postgres_store.py index f43a2b79..97faf4af 100644 --- a/tests/test_postgres_store.py +++ b/tests/test_postgres_store.py @@ -264,11 +264,10 @@ async def test_complete_with_response_parity(store) -> None: async def test_record_ack_sent_aa_body_encrypted_at_rest_pg(store) -> None: # (1) An AA ack_body is persisted only on an ENCRYPTED store, and on disk it is ciphertext: it - # carries the v1 marker, is not the plaintext AA frame, and decrypts back to exactly it. A second, - # ciphered handle is needed because the fixture store is the identity cipher (unencrypted) — mirrors - # the existing at-rest encryption tests in this file. + # carries the mfenc: marker, is not the plaintext AA frame, and decrypts back to exactly it. A + # second, ciphered handle is needed because the fixture store is the identity cipher (unencrypted) + # — mirrors the existing at-rest encryption tests in this file. from messagefoundry.config.settings import load_settings - from messagefoundry.store.crypto import PREFIX from messagefoundry.store.postgres import PostgresStore settings = load_settings(environ=os.environ).store @@ -295,7 +294,7 @@ async def test_record_ack_sent_aa_body_encrypted_at_rest_pg(store) -> None: "SELECT body FROM response WHERE message_id=$1 AND kind='ack_sent'", mid ) )["body"] - assert disk.startswith(PREFIX) # stored under the encrypted marker, not in the clear + assert disk.startswith(MARKER_PREFIX) # stored under the encrypted marker, not in the clear assert disk != _ACK_AA assert cipher.decrypt(disk) == _ACK_AA # and it genuinely encrypts the AA frame finally: @@ -2141,7 +2140,7 @@ async def test_summary_metadata_encrypted_at_rest_and_decrypt(store) -> None: """EF-3: summary/metadata (direct MRN + patient name) are ciphered at rest on Postgres and decrypt on the detail + tracking-list read paths — parity with the SQLite suite.""" from messagefoundry.config.settings import load_settings - from messagefoundry.store.crypto import PREFIX, AesGcmCipher + from messagefoundry.store.crypto import AesGcmCipher from messagefoundry.store.postgres import PostgresStore settings = load_settings(environ=os.environ).store @@ -2153,8 +2152,8 @@ async def test_summary_metadata_encrypted_at_rest_and_decrypt(store) -> None: ) # at rest: ciphertext, with no MRN/name/site visible in the blob. row = await s._fetchone("SELECT summary, metadata FROM messages WHERE id=$1", mid) - assert row["summary"].startswith(PREFIX) and "999001" not in row["summary"] - assert row["metadata"].startswith(PREFIX) and "WESTWING" not in row["metadata"] + assert row["summary"].startswith(MARKER_PREFIX) and "999001" not in row["summary"] + assert row["metadata"].startswith(MARKER_PREFIX) and "WESTWING" not in row["metadata"] # decrypt on the read paths. rec = await s.get_message(mid) assert rec["summary"] == summary and rec["metadata"] == metadata diff --git a/tests/test_sqlserver_store.py b/tests/test_sqlserver_store.py index fe85a5d1..d4066dd5 100644 --- a/tests/test_sqlserver_store.py +++ b/tests/test_sqlserver_store.py @@ -1146,11 +1146,10 @@ async def test_correlate_orders_by_seq_and_decrypts(store) -> None: async def test_record_ack_sent_aa_body_encrypted_at_rest_ss(store) -> None: # (1) An AA ack_body is persisted only on an ENCRYPTED store, and on disk it is ciphertext: it - # carries the v1 marker, is not the plaintext AA frame, and decrypts back to exactly it. A second, - # ciphered handle is needed because the fixture store is the identity cipher (unencrypted) — mirrors - # the existing at-rest encryption tests in this file. + # carries the mfenc: marker, is not the plaintext AA frame, and decrypts back to exactly it. A + # second, ciphered handle is needed because the fixture store is the identity cipher (unencrypted) + # — mirrors the existing at-rest encryption tests in this file. from messagefoundry.config.settings import load_settings - from messagefoundry.store.crypto import PREFIX from messagefoundry.store.sqlserver import SqlServerStore settings = load_settings(environ=os.environ).store @@ -1177,7 +1176,7 @@ async def test_record_ack_sent_aa_body_encrypted_at_rest_ss(store) -> None: "SELECT body FROM response WHERE message_id=? AND kind=?", (mid, "ack_sent") ) )["body"] - assert disk.startswith(PREFIX) # stored under the encrypted marker, not in the clear + assert disk.startswith(MARKER_PREFIX) # stored under the encrypted marker, not in the clear assert disk != _ACK_AA assert cipher.decrypt(disk) == _ACK_AA # and it genuinely encrypts the AA frame finally: @@ -1474,7 +1473,7 @@ async def test_summary_metadata_encrypted_at_rest_and_decrypt(store) -> None: """EF-3: summary/metadata (direct MRN + patient name) ciphered at rest on SQL Server and decrypt on the detail + tracking-list read paths — parity with the SQLite/PG suites.""" from messagefoundry.config.settings import load_settings - from messagefoundry.store.crypto import PREFIX, AesGcmCipher + from messagefoundry.store.crypto import AesGcmCipher from messagefoundry.store.sqlserver import SqlServerStore settings = load_settings(environ=os.environ).store @@ -1486,8 +1485,8 @@ async def test_summary_metadata_encrypted_at_rest_and_decrypt(store) -> None: ) # at rest: ciphertext, with no MRN/name/site visible in the blob. row = (await s._fetchall("SELECT summary, metadata FROM messages WHERE id=?", (mid,)))[0] - assert row["summary"].startswith(PREFIX) and "999001" not in row["summary"] - assert row["metadata"].startswith(PREFIX) and "WESTWING" not in row["metadata"] + assert row["summary"].startswith(MARKER_PREFIX) and "999001" not in row["summary"] + assert row["metadata"].startswith(MARKER_PREFIX) and "WESTWING" not in row["metadata"] # decrypt on the read paths. rec = await s.get_message(mid) assert rec["summary"] == summary and rec["metadata"] == metadata @@ -1549,7 +1548,7 @@ async def test_error_lasterror_detail_encrypted_at_rest_and_decrypt(store) -> No Server and decrypt on every read path — parity with the SQLite/PG suites. Error strings are plain (no HL7 delimiters) so safe_text leaves them intact and the round-trip is exact.""" from messagefoundry.config.settings import load_settings - from messagefoundry.store.crypto import PREFIX, AesGcmCipher + from messagefoundry.store.crypto import AesGcmCipher from messagefoundry.store.sqlserver import SqlServerStore settings = load_settings(environ=os.environ).store @@ -1566,17 +1565,18 @@ async def test_error_lasterror_detail_encrypted_at_rest_and_decrypt(store) -> No fail = "delivery refused by partner endpoint" await s.mark_failed(item.id, fail, RetryPolicy(max_attempts=1), now=110.0) # -> DEAD - # AT REST: every value is mfenc:v1:... ciphertext — the cleartext phrase never appears in the col. + # AT REST: every value is mfenc:... ciphertext — the cleartext phrase never appears in the col. + # (Version-agnostic per the section header: the format follows [store].aad_bind, v2 by default.) erow = (await s._fetchall("SELECT error FROM messages WHERE id=?", (eid,)))[0] - assert erow["error"].startswith(PREFIX) and "bad parse" not in erow["error"] + assert erow["error"].startswith(MARKER_PREFIX) and "bad parse" not in erow["error"] qrow = (await s._fetchall("SELECT last_error FROM queue WHERE message_id=?", (mid,)))[0] - assert qrow["last_error"].startswith(PREFIX) and "refused" not in qrow["last_error"] + assert qrow["last_error"].startswith(MARKER_PREFIX) and "refused" not in qrow["last_error"] drows = await s._fetchall( "SELECT detail FROM message_events WHERE detail IS NOT NULL ORDER BY id" ) assert drows, "expected at least one event with a detail" for d in drows: - assert d["detail"].startswith(PREFIX) # no plaintext detail at rest + assert d["detail"].startswith(MARKER_PREFIX) # no plaintext detail at rest assert "bad parse" not in d["detail"] and "refused" not in d["detail"] # DECRYPT ON READ: every read path returns the cleartext. @@ -1641,7 +1641,7 @@ async def test_legacy_plaintext_error_detail_migrated_on_open(store) -> None: _encrypt_existing_rows (the message_events.detail pass is keyed on the INT IDENTITY id). After the keyed open, the at-rest columns are ciphertext and reads still return the original cleartext.""" from messagefoundry.config.settings import load_settings - from messagefoundry.store.crypto import PREFIX, AesGcmCipher + from messagefoundry.store.crypto import AesGcmCipher from messagefoundry.store.sqlserver import SqlServerStore settings = load_settings(environ=os.environ).store @@ -1657,9 +1657,10 @@ async def test_legacy_plaintext_error_detail_migrated_on_open(store) -> None: ) item = (await plain.claim_ready(now=100.0))[0] await plain.mark_failed(item.id, fail, RetryPolicy(max_attempts=1), now=110.0) - # sanity: stored plaintext (no cipher prefix) before the migration runs. + # sanity: stored plaintext (no cipher prefix) before the migration runs. A NEGATIVE assert, so + # it must exclude EVERY marker version — a v1-only spelling would pass on an encrypted v2 value. erow = (await plain._fetchall("SELECT error FROM messages WHERE id=?", (eid,)))[0] - assert erow["error"] == err and not erow["error"].startswith(PREFIX) + assert erow["error"] == err and not erow["error"].startswith(MARKER_PREFIX) finally: await plain.close() @@ -1667,11 +1668,11 @@ async def test_legacy_plaintext_error_detail_migrated_on_open(store) -> None: keyed = await SqlServerStore.open(settings, cipher=AesGcmCipher(b"k" * 32)) try: erow = (await keyed._fetchall("SELECT error FROM messages WHERE id=?", (eid,)))[0] - assert erow["error"].startswith(PREFIX) and "bad parse" not in erow["error"] + assert erow["error"].startswith(MARKER_PREFIX) and "bad parse" not in erow["error"] qrow = (await keyed._fetchall("SELECT last_error FROM queue WHERE message_id=?", (mid,)))[0] - assert qrow["last_error"].startswith(PREFIX) + assert qrow["last_error"].startswith(MARKER_PREFIX) drows = await keyed._fetchall("SELECT detail FROM message_events WHERE detail IS NOT NULL") - assert drows and all(d["detail"].startswith(PREFIX) for d in drows) + assert drows and all(d["detail"].startswith(MARKER_PREFIX) for d in drows) # reads still return the original cleartext after the in-place migration. assert (await keyed.get_message(eid))["error"] == err assert (await keyed.list_dead())[0]["last_error"] == fail @@ -1894,7 +1895,7 @@ async def test_reference_snapshot_encrypted_at_rest(store) -> None: """Reference values (may carry PHI for patient-keyed sets) are mfenc ciphertext at rest while reference_view() serves plaintext — SQLite/PG parity (test_reference_sets.py analog).""" from messagefoundry.config.settings import load_settings - from messagefoundry.store.crypto import PREFIX, AesGcmCipher + from messagefoundry.store.crypto import AesGcmCipher from messagefoundry.store.sqlserver import SqlServerStore settings = load_settings(environ=os.environ).store @@ -1905,7 +1906,7 @@ async def test_reference_snapshot_encrypted_at_rest(store) -> None: assert s.reference_view()["codes"]["MRN"] == "SECRET999" # cache is plaintext # The value column at rest is ciphertext (no PHI visible in the blob). row = (await s._fetchall("SELECT value FROM reference"))[0] - assert row["value"].startswith(PREFIX) and "SECRET999" not in row["value"] + assert row["value"].startswith(MARKER_PREFIX) and "SECRET999" not in row["value"] finally: await s.close() # Reopening with the same cipher decrypts back into the cache. @@ -2012,14 +2013,15 @@ async def test_reference_plaintext_migrated_on_keyed_reopen(store) -> None: IdentityCipher writes plaintext JSON, and the first keyed open's _encrypt_existing_rows reference pass encrypts them in place (the no-key -> key transition).""" from messagefoundry.config.settings import load_settings - from messagefoundry.store.crypto import PREFIX, AesGcmCipher + from messagefoundry.store.crypto import AesGcmCipher from messagefoundry.store.sqlserver import SqlServerStore try: - # (1) The keyless fixture handle writes plaintext JSON at rest. + # (1) The keyless fixture handle writes plaintext JSON at rest. A NEGATIVE assert, so it must + # exclude EVERY marker version — a v1-only spelling would pass on an encrypted v2 value. await store.write_reference_snapshot(name="codes", version="v1", rows={"MRN": "SECRET999"}) row = (await store._fetchall("SELECT value FROM reference"))[0] - assert row["value"] == '"SECRET999"' and not row["value"].startswith(PREFIX) + assert row["value"] == '"SECRET999"' and not row["value"].startswith(MARKER_PREFIX) # (2) Re-open WITH a key: open() runs the _encrypt_existing_rows reference pass and # migrates it. @@ -2028,7 +2030,7 @@ async def test_reference_plaintext_migrated_on_keyed_reopen(store) -> None: ) try: row = (await keyed._fetchall("SELECT value FROM reference"))[0] - assert row["value"].startswith(PREFIX) and "SECRET999" not in row["value"] + assert row["value"].startswith(MARKER_PREFIX) and "SECRET999" not in row["value"] assert keyed.reference_view()["codes"]["MRN"] == "SECRET999" # decrypts on cache load finally: await keyed.close() diff --git a/tests/test_staged_pipeline.py b/tests/test_staged_pipeline.py index 13db85ae..27448691 100644 --- a/tests/test_staged_pipeline.py +++ b/tests/test_staged_pipeline.py @@ -28,7 +28,7 @@ WiringError, inbound, ) -from messagefoundry.store.crypto import PREFIX, make_cipher +from messagefoundry.store.crypto import MARKER_PREFIX, make_cipher from messagefoundry.store.store import MessageStatus, MessageStore, OutboxStatus, Stage RAW = "MSH|^~\\&|S|F|R|RF|20260101||ADT^A01|MSG1|P|2.5.1\rPID|1||100||DOE^JANE\r" @@ -841,7 +841,7 @@ async def test_legacy_outbox_migrates_to_queue_with_encryption(tmp_path: Path) - item = await store.claim_next_fifo("OB_A") assert item is not None and item.payload == "PLAINPAYLOAD" on_disk = sqlite3.connect(path).execute("SELECT payload FROM queue").fetchone()[0] - assert str(on_disk).startswith(PREFIX) # encrypted at rest after migration + assert str(on_disk).startswith(MARKER_PREFIX) # encrypted at rest after migration finally: await store.close() diff --git a/tests/test_store_encryption.py b/tests/test_store_encryption.py index 00241346..6705f616 100644 --- a/tests/test_store_encryption.py +++ b/tests/test_store_encryption.py @@ -46,7 +46,9 @@ def _raw_at_rest(db_path: Path, column: str = "raw", table: str = "messages") -> def test_cipher_round_trip_and_hides_plaintext() -> None: cipher = make_cipher(generate_key()) token = cipher.encrypt(ADT) - assert token.startswith(PREFIX) + # "Enciphered at all" — the version-agnostic marker. Format pinning is owned separately by the M9 + # section below (test_default_writer_is_v1_not_v2 / test_v1_writer_is_byte_identical). + assert token.startswith(MARKER_PREFIX) # PHI-hidden, asserted deterministically: the whole plaintext can never appear in the token # (it contains non-base64 bytes like '|' and '\r'), and the round-trip proves real encryption. # NEVER assert short-substring absence ("MSH"/"DOE") — a random base64 body contains any given @@ -90,8 +92,8 @@ async def test_bodies_encrypted_at_rest(tmp_path: Path) -> None: await store.close() raw = _raw_at_rest(db) payload = _raw_at_rest(db, column="payload", table="queue") - assert raw.startswith(PREFIX) and "DOE" not in raw # body is ciphertext on disk - assert payload.startswith(PREFIX) + assert raw.startswith(MARKER_PREFIX) and "DOE" not in raw # body is ciphertext on disk + assert payload.startswith(MARKER_PREFIX) async def test_reads_and_delivery_decrypt(tmp_path: Path) -> None: @@ -189,8 +191,9 @@ async def test_migration_encrypts_existing_rows(tmp_path: Path) -> None: key = generate_key() encrypted = await MessageStore.open(db, cipher=make_cipher(key)) # reopen with a key → migrate try: - assert _raw_at_rest(db).startswith(PREFIX) # existing row now encrypted on disk - assert _raw_at_rest(db, column="payload", table="queue").startswith(PREFIX) + # The migration's own guard is the version-agnostic `mfenc:%` anchor, so assert the same shape. + assert _raw_at_rest(db).startswith(MARKER_PREFIX) # existing row now encrypted on disk + assert _raw_at_rest(db, column="payload", table="queue").startswith(MARKER_PREFIX) record = await encrypted.get_message(mid) assert record is not None and record["raw"] == ADT # still readable finally: @@ -219,8 +222,8 @@ async def test_error_and_event_detail_encrypted_at_rest_and_decrypt(tmp_path: Pa # messages.error and the message_events.detail copy are both ciphertext on disk... err_at_rest = _raw_at_rest(db, column="error") det_at_rest = _raw_at_rest(db, column="detail", table="message_events") - assert err_at_rest.startswith(PREFIX) and "SECRET" not in err_at_rest - assert det_at_rest.startswith(PREFIX) and "SECRET" not in det_at_rest + assert err_at_rest.startswith(MARKER_PREFIX) and "SECRET" not in err_at_rest + assert det_at_rest.startswith(MARKER_PREFIX) and "SECRET" not in det_at_rest # ...and decrypt on every read path. assert (await store.get_message(mid))["error"] == PHI_ERR assert any(m["error"] == PHI_ERR for m in await store.list_messages()) @@ -238,7 +241,7 @@ async def test_last_error_encrypted_at_rest_and_decrypts(tmp_path: Path) -> None await store.claim_ready() await store.dead_letter_now(row["id"], PHI_ERR) at_rest = _raw_at_rest(db, column="last_error", table="queue") - assert at_rest.startswith(PREFIX) and "SECRET" not in at_rest + assert at_rest.startswith(MARKER_PREFIX) and "SECRET" not in at_rest dead = await store.list_dead() assert dead and dead[0]["last_error"] == PHI_ERR # dead-letter view decrypts assert (await store.outbox_for(mid))[0]["last_error"] == PHI_ERR # detail view decrypts @@ -290,8 +293,8 @@ async def test_summary_and_metadata_encrypted_at_rest_and_decrypt(tmp_path: Path # ...ciphertext on disk (no MRN/name/site visible)... sm = _raw_at_rest(db, column="summary") md = _raw_at_rest(db, column="metadata") - assert sm.startswith(PREFIX) and "999001" not in sm and "DOE" not in sm - assert md.startswith(PREFIX) and "WESTWING" not in md + assert sm.startswith(MARKER_PREFIX) and "999001" not in sm and "DOE" not in sm + assert md.startswith(MARKER_PREFIX) and "WESTWING" not in md # ...and decrypt on the detail + tracking-list read paths. rec = await store.get_message(mid) assert rec is not None and rec["summary"] == EF3_SUMMARY and rec["metadata"] == EF3_METADATA @@ -330,8 +333,8 @@ async def test_migration_encrypts_existing_summary_metadata(tmp_path: Path) -> N encrypted = await MessageStore.open(db, cipher=make_cipher(generate_key())) # reopen → migrate try: - assert _raw_at_rest(db, column="summary").startswith(PREFIX) # migrated on disk - assert _raw_at_rest(db, column="metadata").startswith(PREFIX) + assert _raw_at_rest(db, column="summary").startswith(MARKER_PREFIX) # migrated on disk + assert _raw_at_rest(db, column="metadata").startswith(MARKER_PREFIX) [m] = await encrypted.list_messages() assert m["summary"] == EF3_SUMMARY and m["metadata"] == EF3_METADATA # still readable finally: @@ -365,12 +368,19 @@ def test_key_id_is_a_fingerprint_not_zero() -> None: key_b64 = generate_key() token = make_cipher(key_b64).encrypt("x") fp = _fingerprint(base64.b64decode(key_b64)) + # DELIBERATELY v1 (do not sweep to MARKER_PREFIX): these pin the key_id's POSITION in the marker, + # and `mfenc:v1::` is a v1-only field order (v2 is `mfenc:v2:::`). A version- + # agnostic `mfenc::` is a string no writer ever emits, so the positive assert could never pass + # and the negative one could never fail — strictly weaker, not wider. The v2 layout is pinned + # separately by test_active_marker_prefix_v1_and_v2. assert token.startswith(f"{PREFIX}{fp}:") # self-identifying key_id assert not token.startswith(f"{PREFIX}0:") # not the old hardcoded "0" def test_legacy_key_id_zero_decrypts_via_fallback() -> None: # A pre-WP-5 row was tagged key_id '0'. The keyring's try-all fallback still decrypts it. + # DELIBERATELY v1 (do not sweep): a legacy row IS v1 by definition — v2 postdates WP-5 — and a + # version-agnostic `mfenc:0:` would be rejected by _parse as an unknown marker version instead. import base64 import os @@ -396,7 +406,9 @@ async def test_rotation_reencrypts_and_retired_key_bridges(tmp_path: Path) -> No # Reopen with B active + A retired: existing A-rows still read (decrypt via the retired key), # then rotate them to B. - rotating = await MessageStore.open(db, cipher=make_cipher(key_b, [key_a])) + rotating_cipher = make_cipher(key_b, [key_a]) + assert isinstance(rotating_cipher, AesGcmCipher) + rotating = await MessageStore.open(db, cipher=rotating_cipher) try: assert (await rotating.get_message(mid))["raw"] == ADT assert await rotating.reencrypt_to_active() >= 2 # raw + the outbound payload @@ -404,7 +416,11 @@ async def test_rotation_reencrypts_and_retired_key_bridges(tmp_path: Path) -> No finally: await rotating.close() raw_b = _raw_at_rest(db) - assert raw_b.startswith(PREFIX) and raw_b != raw_a # re-encrypted under the new key + # Re-encrypted under the ACTIVE (new) key — not merely "still ciphertext". Take the expected marker + # from the rotating cipher itself: active_marker_prefix carries key B's fingerprint in the right + # position for whichever format the writer emits (v1, or the v2 that [store].aad_bind makes the + # default). A bare MARKER_PREFIX would drop the under-the-new-key half of the proof. + assert raw_b.startswith(rotating_cipher.active_marker_prefix) and raw_b != raw_a # B alone (no retired key) now reads everything — the bridge key is no longer needed. final = await MessageStore.open(db, cipher=make_cipher(key_b)) @@ -507,6 +523,9 @@ def test_v2_active_decrypts_v1_without_rotation() -> None: # forced migration). Same key, so the v2-active cipher decrypts the v1 blob it did not write. key = generate_key() v1_token = make_cipher(key).encrypt(ADT) # written by a v1 cipher + # DELIBERATELY v1 (do not sweep): this establishes the PREMISE that the row really is v1. Under a + # version-agnostic prefix a v2 token would satisfy it and the dual-read proof below collapses into + # a vacuous v2-reads-v2 tautology. assert v1_token.startswith(PREFIX) v2_cipher = make_cipher(key, write_v2=True) assert v2_cipher.decrypt(v1_token) == ADT # decoded with no rotation @@ -1019,7 +1038,8 @@ async def test_migration_encrypts_existing_state_value(tmp_path: Path) -> None: store = await MessageStore.open(db, cipher=make_cipher(generate_key())) # reopen → migrate try: at_rest = _state_at_rest(db) - assert at_rest.startswith(PREFIX) and "SECRETSTATEMRN" not in at_rest # sealed on disk + # sealed on disk (version-agnostic: the format follows [store].aad_bind, v2 by default) + assert at_rest.startswith(MARKER_PREFIX) and "SECRETSTATEMRN" not in at_rest assert store.state_view()[("ns", "k")] == {"mrn": "SECRETSTATEMRN"} # decrypts on read finally: await store.close() @@ -1043,7 +1063,7 @@ async def test_state_migration_skips_already_encrypted_values(tmp_path: Path) -> first = await MessageStore.open(db, cipher=make_cipher(key)) # migrate plaintext -> ciphertext await first.close() sealed = _state_at_rest(db) - assert sealed.startswith(PREFIX) + assert sealed.startswith(MARKER_PREFIX) # same shape as the `mfenc:%` guard under test second = await MessageStore.open(db, cipher=make_cipher(key)) # re-run the migration try: diff --git a/tests/test_transform_state.py b/tests/test_transform_state.py index 991611d6..7249569a 100644 --- a/tests/test_transform_state.py +++ b/tests/test_transform_state.py @@ -33,7 +33,7 @@ from messagefoundry.parsing.message import Message from messagefoundry.pipeline.dryrun import dry_run, transform_one from messagefoundry.pipeline.retention import RetentionRunner -from messagefoundry.store.crypto import PREFIX, generate_key, make_cipher +from messagefoundry.store.crypto import MARKER_PREFIX, generate_key, make_cipher from messagefoundry.store.store import MessageStatus, MessageStore, Stage RAW = "MSH|^~\\&|S|F|R|RF|20260101||ADT^A01|MSG1|P|2.5.1\rPID|1||100||DOE^JANE\r" @@ -273,9 +273,10 @@ async def test_state_value_encrypted_at_rest_and_read_back(tmp_path: Path) -> No ) finally: await store.close() - # On disk: ciphertext (prefix present, plaintext value not visible). + # On disk: ciphertext (version-agnostic marker present, plaintext value not visible). Same rule as + # the rotation test below — a v1-only spelling misreads the v2 value [store].aad_bind makes default. at_rest = _state_at_rest(db, "patient_anon", "MRN-DOE") - assert at_rest.startswith(PREFIX) + assert at_rest.startswith(MARKER_PREFIX) assert "ANON-XYZ" not in at_rest @@ -324,7 +325,7 @@ async def test_key_rotation_reencrypts_state_and_reads_still_work(tmp_path: Path rotated = await store2.reencrypt_to_active() assert rotated >= 1 # the state value (among others) re-encrypted # On disk it is now under the NEW key id, and reads still resolve. Take the expected marker - # from the rotating cipher itself instead of hand-building f"{PREFIX}{new_id}:" — that spelling + # from the rotating cipher itself instead of hand-building f"mfenc:v1:{new_id}:" — that spelling # bakes in the v1 field order and misreads a v2 value (mfenc:v2:::), which is what # [store].aad_bind makes the default at rest. `old_id not in` keeps the independent proof that # rotation actually moved the value, derived from active_key_id rather than the prefix. From 9bf7178fa7c2d9a6e11e20953fbe50378acc72f3 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 10:58:43 -0500 Subject: [PATCH 2/5] ci(supply-chain): pin the release-path toolchain + guard the pins against silent rot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five installs on the release path resolved whatever PyPI served at tag time (Scorecard PinnedDependenciesID; ADR 0034 §3). The `release` job holds contents/id-token/attestations: write and `release-harness` holds contents/id-token: write, so these ran with the OIDC identity that signs and publishes. The sharp one is `sigstore`: its step carries no `if:`, so it runs on every tag AND every dispatch, and the very next command signs the wheel, sdist, SBOM and VEX with that identity. release.yml sigstore -> ==4.4.0 release.yml pip + build (x2: engine and harness jobs) -> ==26.1.2 / ==1.5.0 release.yml cyclonedx-bom -> ~=7.3.1 release.yml packaging (x2) -> ==$PKG_PIN, derived from constraints.lock security.yml pip + cyclonedx-bom -> ==26.1.2 / ~=7.3.1 (keeps it the true SBOM twin) Three judgement calls, none of them the obvious choice: 1. sigstore 4.4.0, NOT the newer 4.5.0. .github/dependabot.yml sets a 5-day supply-chain cooldown whose stated purpose is dodging a package compromised shortly after publish; 4.5.0 is <48h old. Hard-pinning the SIGNING toolchain to an artifact fresher than the repo's own routine-update policy allows would invert that policy at the highest-privilege point in the pipeline. 4.4.0 has aged 23 days. Re-evaluate once 4.5.0 clears the window. 2. `packaging` is DERIVED from constraints.lock, not hardcoded. It IS a DEP-1 dependency (requirements.lock + constraints.lock both pin it at 26.2), so a literal would drift on the next Dependabot bump. Same run-time-read pattern as quality-advisory.yml's ruff pin, but FAIL-CLOSED instead of falling back to an unpinned fetch — a soft fallback on the release path defeats the pin exactly where it matters. Both installs also move ABOVE their `GITHUB_REF_TYPE = tag` guard so a workflow_dispatch dry-run exercises them; they were the only two of the five a dispatch could not reach, and an install that never runs before the tag cannot be validated before it. 3. ~=7.3.1 rather than ~=7.3 for cyclonedx-bom: the looser form floats the whole 7.x minor range, and a 7.4 could change the JSON shape scripts/security/sbom_finalize.py parses — which exits non-zero and FAILS the release. ~=7.3.1 still takes patch fixes. This is residual-risk reduction, NOT an alert closure: ADR 0034 §3 shows from this repo's own data that exactly-pinned tools (bandit==1.9.4, zizmor==1.5.2) are still flagged while --require-hashes installs are not. Pinning `sigstore` also pins the TOP only — its ~30 transitive deps still float at signing time. Closing it needs the hashed release-tools lock (ADR 0034 option B), which is an owner call and is deliberately NOT built here. GUARD: a pin nothing watches rots back to unpinned, and Dependabot has no updater for an inline `pip install X==Y` in a workflow (its `uv` ecosystem reads only pyproject.toml + uv.lock), so both a stale pin and a DELETED pin are invisible. tests/test_ci_venv_pinning.py gains two tests: a BLANKET scan of every pip-install target in release.yml (so a NEW unpinned install added tomorrow fails too — the case a fixed name list cannot see), plus a non-vacuity table asserting each tool is still installed and still pinned at EVERY occurrence. Guard proven by injecting one regression at a time rather than trusting a green run: un-pin sigstore -> 2 failed (blanket scan + the named backstop) DELETE the install -> 1 failed (only the backstop can see this, as designed) un-pin 1 of 2 `build` -> 3 failed (proves every occurrence is checked, not just the first) restored -> 12 passed Both workflows re-parsed as YAML. 192 passed across every test that reads .github/workflows. Line-neutral where ADR 0034 requires it: the scanner re-fires a dismissed alert under a NEW number when a line moves, so each pin was made in place. Not done: security.yml's `pip uv` (line 55) and `pip-audit` (81) stay unpinned — the DEP-1 job is contents: read and outside the release path. --- .github/workflows/release.yml | 52 +++++++++++++-- .github/workflows/security.yml | 5 +- tests/test_ci_venv_pinning.py | 117 ++++++++++++++++++++++++++++++++- 3 files changed, 164 insertions(+), 10 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 0d03908b..945da28f 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -88,7 +88,12 @@ jobs: - name: Build sdist + wheel run: | - python -m pip install --upgrade pip build + # PINNED (Scorecard PinnedDependenciesID; ADR 0034 §3). This runs in the job holding + # contents/id-token/attestations: write, and `build` produces the artifact that is signed and + # published — an unpinned resolve here picks whatever PyPI serves at tag time. Neither tool is + # in any DEP-1 lock, so `==` is drift-free (and Dependabot cannot bump an inline workflow + # install: re-check these by hand when bumping, tests/test_ci_venv_pinning.py keeps them pinned). + python -m pip install "pip==26.1.2" "build==1.5.0" python -m build ls -l dist/ @@ -126,9 +131,21 @@ jobs: # the module attribute and the wheel filename could not all be canonical at once. Version() # normalises both sides, so canonical "0.3.0rc1" in __init__.py matches tag v0.3.0-rc1 and the # check still fails loudly on a genuine mismatch. + # `packaging` pin DERIVED from constraints.lock, never hardcoded — it IS a DEP-1 dependency + # (requirements.lock + constraints.lock both pin it), so a literal here would drift silently on + # the next Dependabot bump. Same run-time-read pattern as quality-advisory.yml's ruff pin, but + # FAIL-CLOSED rather than falling back to an unpinned fetch: this is the release path. Installed + # OUTSIDE the tag guard so a workflow_dispatch dry-run exercises the pinned install — the guard + # below is what stays tag-only, and an install this step never reaches cannot be validated + # before the tag that depends on it. + PKG_PIN="$(sed -n 's/^packaging==\([^ ;]*\).*/\1/p' constraints.lock | head -1)" + if [ -z "$PKG_PIN" ]; then + echo "::error::no packaging== pin in constraints.lock — refusing an unpinned install on the release path"; exit 1 + fi + echo "packaging pin from constraints.lock: $PKG_PIN" + /tmp/relsmoke/bin/pip install --quiet "packaging==$PKG_PIN" if [ "${GITHUB_REF_TYPE:-}" = "tag" ]; then want="${GITHUB_REF_NAME#v}" - /tmp/relsmoke/bin/pip install --quiet packaging /tmp/relsmoke/bin/python - "$built" "$want" <<'PYVER' import sys from packaging.version import InvalidVersion, Version @@ -172,7 +189,10 @@ jobs: # The core lock is the honest closure a `pip install messagefoundry` pulls (the all-extras # requirements.lock drags PySide6/dev tooling the wheel never requires); pip-audit still audits # the all-extras set. See docs/SUPPLY-CHAIN.md + ADR 0149. - python -m pip install --upgrade "cyclonedx-bom~=7.3" + # ~=7.3.1 (not ~=7.3): the looser form floats the whole 7.x minor range, and a 7.4 could change + # the CycloneDX JSON shape sbom_finalize.py parses — which exits non-zero and FAILS the release. + # ~=7.3.1 still takes patch fixes and keeps the lxml-6.x/cp314 floor rationale above intact. + python -m pip install --upgrade "cyclonedx-bom~=7.3.1" # No unpinned pip bootstrap: --require-hashes resolves nothing (tests/test_ci_venv_pinning.py). python -m venv /tmp/sbomenv /tmp/sbomenv/bin/pip install --require-hashes -r docker/locks/requirements-core.lock @@ -211,7 +231,16 @@ jobs: - name: Sign artifacts with Sigstore (keyless, GitHub OIDC) run: | - python -m pip install sigstore + # PINNED — the sharpest of these installs: this step is unconditional (every tag AND every + # dispatch) and the very next command signs the release artifacts with the job's OIDC identity, + # the same identity that publishes to PyPI below. 4.4.0, not the newer 4.5.0: .github/ + # dependabot.yml sets a 5-day supply-chain cooldown to dodge a package compromised shortly + # after publish, and 4.5.0 is <48h old — hard-pinning the SIGNING toolchain to a fresher + # artifact than the repo's own routine-update policy allows inverts that policy at the highest- + # privilege point in the pipeline. Re-evaluate to 4.5.0 once it has aged past the window. + # NOTE: this pins the TOP only; sigstore's ~30 transitive deps still float at signing time. + # Closing the Scorecard alert outright needs the hashed release-tools lock (ADR 0034 option B). + python -m pip install "sigstore==4.4.0" # Sign the wheel + sdist AND the SBOM + VEX, so an operator can verify the provenance of the # bill-of-materials and the exploitability assessment too — not just the code artifacts (ADR 0149). python -m sigstore sign dist/*.tar.gz dist/*.whl \ @@ -341,7 +370,9 @@ jobs: - name: Build the harness wheel (wheel-only — harness/ is force-included from the repo root) run: | - python -m pip install --upgrade pip build + # PINNED, same rationale as the engine's build step: this job also holds contents/id-token: + # write, and it publishes the harness wheel when PUBLISH_HARNESS is set. + python -m pip install "pip==26.1.2" "build==1.5.0" # Wheel-only on purpose: the harness source (harness/) lives OUTSIDE this project dir (it is # force-included from ../../harness), so an sdist would not be self-contained. Pure-Python, so a # wheel suffices. Version is read from messagefoundry/__init__.py (lockstep with the engine). @@ -358,9 +389,18 @@ jobs: # and a string compare could NEVER match — this job failed on every pre-release tag by # construction, whatever __version__ said. With PUBLISH_HARNESS=true it also runs after the # engine has already uploaded, so the failure would land half-published. + # Pin DERIVED from constraints.lock + installed outside the tag guard — see the engine job's + # wheel smoke above for the full rationale. (`build` already pulls packaging>=24.0 into this + # same interpreter, so the install is near-redundant; pinning it beats deleting it, which would + # leave the dependency implicit and unpinned via build's own resolve.) + PKG_PIN="$(sed -n 's/^packaging==\([^ ;]*\).*/\1/p' constraints.lock | head -1)" + if [ -z "$PKG_PIN" ]; then + echo "::error::no packaging== pin in constraints.lock — refusing an unpinned install on the release path"; exit 1 + fi + echo "packaging pin from constraints.lock: $PKG_PIN" + python -m pip install --quiet "packaging==$PKG_PIN" if [ "${GITHUB_REF_TYPE:-}" = "tag" ]; then want="${GITHUB_REF_NAME#v}" - python -m pip install --quiet packaging python - "$built" "$want" <<'PYVER' import sys from packaging.version import InvalidVersion, Version diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index b7328c43..b77ad255 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -135,7 +135,10 @@ jobs: # (docker/locks/requirements-core.lock) is the honest runtime closure `pip install messagefoundry` # pulls; the all-extras requirements.lock stays covered by the pip-audit job above. cyclonedx-bom # ~=7.3 → lxml 6.x (cp314 wheels) so the 3.14 runner doesn't source-build lxml. ADR 0149. - python -m pip install --upgrade pip "cyclonedx-bom~=7.3" + # PINNED, keeping this the true twin of release.yml's SBOM step: ~=7.3.1 rather than ~=7.3 so a + # 7.4 cannot change the JSON shape sbom_finalize.py parses. Both SBOM builds must move together + # or the release ships a BOM this advisory job never rendered. + python -m pip install "pip==26.1.2" "cyclonedx-bom~=7.3.1" # No unpinned pip bootstrap: --require-hashes resolves nothing (tests/test_ci_venv_pinning.py). python -m venv /tmp/sbomenv /tmp/sbomenv/bin/pip install --require-hashes -r docker/locks/requirements-core.lock diff --git a/tests/test_ci_venv_pinning.py b/tests/test_ci_venv_pinning.py index 480b4daf..c51cdd30 100644 --- a/tests/test_ci_venv_pinning.py +++ b/tests/test_ci_venv_pinning.py @@ -22,9 +22,18 @@ exactly as unpinned but is invisible to the scanner. That is ADR 0034's rejected option 3 — a visible dismissal-with-reason beats an invisible filter. -Deliberately scoped to the LOCK-ONLY venvs. `/tmp/relsmoke` (`release.yml`) legitimately installs -unpinned `packaging` — it exists to prove the freshly built wheel's own declared closure resolves, so -feeding it a lock would defeat its purpose — and is dismissed separately. Pure text checks, no network. +The lock-only venvs are one half. The other half is every OTHER `pip install` on the release path — +`build`, `sigstore`, `cyclonedx-bom`, `packaging`, and the `pip` bootstraps — which resolved whatever +PyPI served at tag time. `sigstore` is the sharp one: its step is unconditional and the very next +command signs the release artifacts with the job's OIDC identity, the same identity that publishes to +PyPI. Those are now version-pinned, and the second half of this module keeps them that way — nothing +else can see the regression, because Dependabot has no updater for an inline `pip install X==Y` in a +workflow (its `uv` ecosystem only reads pyproject.toml + uv.lock), so a stale pin rots invisibly and a +DELETED pin is invisible twice over. + +`/tmp/relsmoke` (`release.yml`) stays out of the hash-verified rule — it exists to prove the freshly +built wheel's own declared closure resolves, so feeding it a lock would defeat its purpose — but its +`packaging` install is covered by the version-pin rule below. Pure text checks, no network. """ from __future__ import annotations @@ -95,3 +104,105 @@ def test_scratch_venvs_do_not_hide_an_unpinned_pip_fetch(workflow: str) -> None: f"unpinned exactly like the deleted `pip install --upgrade pip`, but the scanner cannot see it. " f"ADR 0034 requires a visible dismissal over an invisible filter." ) + + +# --- the release path: every named package must carry a version ------------------------------------ + +#: Any `pip install`, in either spelling, into any interpreter or venv. +_PIP_INSTALL = re.compile(r"\bpip\s+install\b") + +#: A pinned target names a version. `$PKG_PIN` counts — it is read out of constraints.lock at run time +#: (the quality-advisory.yml ruff-pin pattern), which is MORE current than a literal, not less. +_VERSION_OPS = ("==", "~=", ">=", "<=", "!=") + +#: Install targets that legitimately name no version: a path (the artifact under test) and the +#: hash-verified lock installs, where every version is pinned INSIDE the lock. +_EXEMPT_TARGETS = frozenset({"."}) + +#: Tools whose release-path pin must EXIST — the non-vacuity backstop for the scan above. Deleting a +#: step would otherwise make the scan pass by finding nothing left to check. +RELEASE_PINNED_TOOLS = ( + ("release.yml", "sigstore"), + ("release.yml", "build"), + ("release.yml", "pip"), + ("release.yml", "cyclonedx-bom"), + ("release.yml", "packaging"), + ("security.yml", "cyclonedx-bom"), +) + + +def _install_targets(line: str) -> list[str]: + """The package tokens a ``pip install`` line names — flags, and the arguments of flags that take + one, removed. Everything left is something pip will resolve.""" + body = line.split(" install ", 1)[1] + targets: list[str] = [] + skip_next = False + for tok in body.split(): + if skip_next: + skip_next = False + continue + if tok in ("-r", "--requirement", "-c", "--constraint", "--index-url", "--extra-index-url"): + skip_next = True + continue + if tok.startswith("-"): + continue + targets.append(tok.strip("\"'")) + return targets + + +def test_release_path_pip_installs_name_a_version() -> None: + """EVERY package `release.yml` installs must carry a version specifier. + + A blanket scan, not a name list, so a NEW unpinned install added tomorrow fails too — the failure + mode a fixed table cannot see. Path installs (``dist/*.whl``) and ``-r `` installs are + exempt: the first is the artifact under test, the second is pinned inside the lock. + """ + lines = [ln for ln in _code_lines(_WORKFLOWS / "release.yml") if _PIP_INSTALL.search(ln)] + # Non-vacuity: this file HAS a toolchain to pin. If it drops below this, the scan has stopped + # seeing the installs rather than the installs having become clean. + assert len(lines) >= 6, ( + f"release.yml now has only {len(lines)} pip installs — the scan is probably no longer " + f"matching them; re-point it rather than letting it pass on an empty set.\n{lines}" + ) + + unpinned = [ + (ln, target) + for ln in lines + for target in _install_targets(ln) + if target not in _EXEMPT_TARGETS + and "/" not in target # a path install (dist/*.whl), not a named package + and not any(op in target for op in _VERSION_OPS) + ] + assert not unpinned, ( + f"release.yml installs these WITHOUT a version: {unpinned}. This workflow's jobs hold " + f"contents/id-token/attestations: write and sign + publish the release artifacts, so an " + f"unpinned resolve here takes whatever PyPI serves at tag time (Scorecard " + f"PinnedDependenciesID; ADR 0034 §3). Pin it, or derive the pin from constraints.lock the " + f"way the `packaging` installs do." + ) + + +@pytest.mark.parametrize(("workflow", "package"), RELEASE_PINNED_TOOLS) +def test_release_toolchain_pin_is_present(workflow: str, package: str) -> None: + """Each release-path tool is still installed, and still pinned wherever it is installed. + + Guards the direction the blanket scan cannot: a pin that vanishes with its step. Every occurrence + is checked, not just the first — pinning one of the two `build` installs (engine + harness) would + move the exposure rather than remove it. + """ + lines = [ln for ln in _code_lines(_WORKFLOWS / workflow) if _PIP_INSTALL.search(ln)] + hits = [ + (ln, target) + for ln in lines + for target in _install_targets(ln) + if target == package or target.startswith(tuple(f"{package}{op}" for op in _VERSION_OPS)) + ] + assert hits, ( + f"{workflow} no longer installs {package!r} — if the step was removed on purpose, drop it " + f"from RELEASE_PINNED_TOOLS in the same commit; otherwise this guard just went blind." + ) + unpinned = [ln for ln, target in hits if not any(op in target for op in _VERSION_OPS)] + assert not unpinned, ( + f"{workflow} installs {package!r} unpinned at: {unpinned}. Dependabot cannot bump an inline " + f"`pip install` in a workflow, so an unpinned one here is never even noticed." + ) From b1ee3fce6d868a2a956eac181a04e203ebb9ab84 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 11:54:17 -0500 Subject: [PATCH 3/5] test(crypto): finish the cell-AAD sweep and correct the rationale the sweep recorded Follow-up to 09767bfb. The sweep widened the "encrypted at rest" assertions correctly, but review found five things it got wrong or left half-done. 1. Five more bare `decrypt()` calls on cell-bound columns (the defect the sweep fixed once, in test_ack_sent_store). Each fails closed on an mfenc:v2 value and was only invisible because the v1 prefix assertion above it failed first: - test_store_encryption x3, queue.last_error (store.py binds cell_aad("queue","last_error", )) - test_sqlserver_store x1, response.body (sqlserver.py:4326) - test_postgres_store x1, response.body (postgres.py:3668) The SS/PG pair skip without a live DB, so they surface only in CI. Both now read destination_name back from the row rather than rebuilding the "\x1fack:" sentinel. 2. Seven new comments asserted a causal link that does not exist: "the at-rest format follows [store].aad_bind (v2 by default), so a v1-only prefix would fail here". aad_bind is read in exactly one place -- base.py:1736, make_cipher(..., write_v2=settings.aad_bind), reachable only via build_cipher/open_store. Every one of these tests builds its own cipher, and make_cipher's write_v2 parameter still defaults False, so they observe mfenc:v1: in a normal run and the setting has no influence on them. Reworded to the true reason: the marker version is the cipher's business, not the assertion's. 3. Two poison-injection comments named the wrong failure mechanism. Measured: 'mfenc:v1:not-base64-$$$' -> ValueError: Nonce must be between 8 and 128 bytes 'mfenc:not-base64-$$$' -> CipherError: unknown at-rest marker version `_parse` splits on the second colon, which is absent, so the blob is empty and b64decode("") succeeds -- the raise comes out of AESGCM, not the base64 path. The decision to keep these two at v1 was right; only the stated mechanism was wrong. 4. test_store_encryption's M9 section claimed "WRITES v1 BY DEFAULT" / "the shipping default never emits a v2 marker" in the same file where the sweep added six "v2 by default" comments. ADR 0148 falsified the prose, not the tests: they pin make_cipher's PARAMETER default (still False), while the shipped store writes v2 via build_cipher. Both defaults now named separately. 5. test_bytes_per_message_amplification documented the at-rest shape as mfenc:v1::. Receipt (real numbers, run from this worktree). Default mode over the 11 edited modules: 166 passed, 351 skipped. Forced-v2 leg (MEFOR_TEST_FORCE_AAD_BIND=1) over all 60 test modules that import store.crypto -- the scanned set, not a 12-file window: 10 failed, 1159 passed, 359 skipped, down from 13 failed. The 3 that stopped failing are exactly the last_error sites in (1). All 10 remaining are deliberate-v1 sites that MUST red under a forced v2 writer: the four CRYPTO-1 frozen-writer pins, test_active_marker_prefix_v1_and_v2, test_key_id_is_a_fingerprint_not_zero, and the four v1-premise dual-read tests. No messagefoundry/ change. ruff check + ruff format --check clean over tests/. --- tests/test_ack_sent_store.py | 7 ++- tests/test_alert_state.py | 5 +- tests/test_batch_claim_fifo.py | 7 +-- tests/test_bytes_per_message_amplification.py | 4 +- tests/test_claim_fifo_heads.py | 6 ++- tests/test_connection_event_store.py | 3 +- tests/test_ed_documents_e2e.py | 4 +- tests/test_postgres_store.py | 20 +++++--- tests/test_sqlserver_store.py | 31 ++++++++---- tests/test_store_encryption.py | 48 +++++++++++++------ tests/test_transform_state.py | 3 +- 11 files changed, 94 insertions(+), 44 deletions(-) diff --git a/tests/test_ack_sent_store.py b/tests/test_ack_sent_store.py index 1952a3ec..2e265841 100644 --- a/tests/test_ack_sent_store.py +++ b/tests/test_ack_sent_store.py @@ -54,8 +54,11 @@ async def test_aa_body_encrypted_when_store_encrypted(tmp_path: Path) -> None: # marker, is not the plaintext frame, and decrypts back to exactly it. Assert the decrypt # round-trip (deterministic) rather than `"MSA" not in ` — that old substring check # flaked because a base64 ciphertext randomly contains that 3-char run (alphabet includes M/S/A). - # The claim is encryptedness, so anchor on the VERSION-AGNOSTIC marker: the at-rest format - # follows [store].aad_bind (v2 by default), and a v1-only prefix would silently fail on it. + # The claim is encryptedness, so anchor on the VERSION-AGNOSTIC marker. WHICH mfenc format gets + # written is the cipher's business, not this assertion's: v1 here (make_cipher's writer default + # is still the frozen v1 one), v2 wherever the store builds its cipher through build_cipher + # (write_v2=[store].aad_bind, now on) or under the MEFOR_TEST_FORCE_AAD_BIND leg. Format pinning + # is owned by the CRYPTO-1 tests in test_store_encryption.py, not here. disk = next(r for r in _response_rows(db) if r[1] == "ack_sent") assert disk[4].startswith(MARKER_PREFIX) # under the encrypted marker, not in the clear assert disk[4] != AA diff --git a/tests/test_alert_state.py b/tests/test_alert_state.py index c35020e7..629ef473 100644 --- a/tests/test_alert_state.py +++ b/tests/test_alert_state.py @@ -193,8 +193,9 @@ async def test_reason_encrypted_at_rest(tmp_path: Path) -> None: con = sqlite3.connect(db) try: raw = con.execute("SELECT reason FROM alert_instance").fetchone()[0] - # Version-agnostic marker: the claim is "enciphered at rest", and the at-rest format follows - # [store].aad_bind (v2 by default) — a v1-only prefix would be a latent false failure. + # Version-agnostic marker: the claim is "enciphered at rest", not which mfenc format wrote it. + # That belongs to the cipher — v1 from make_cipher's default here, v2 wherever the store builds + # its cipher via build_cipher (write_v2=[store].aad_bind) or under MEFOR_TEST_FORCE_AAD_BIND. assert isinstance(raw, str) and raw.startswith(MARKER_PREFIX) # ciphertext on disk assert "refused" not in raw finally: diff --git a/tests/test_batch_claim_fifo.py b/tests/test_batch_claim_fifo.py index 5772d664..8da17274 100644 --- a/tests/test_batch_claim_fifo.py +++ b/tests/test_batch_claim_fifo.py @@ -312,9 +312,10 @@ async def test_t8_undecryptable_interior_dead_lettered_tail_survives( mids = await _seed_ingress(enc, channel, [100.0, 101.0, 102.0]) # Corrupt row2's payload to an undecryptable blob (keep the marker so it routes through decrypt). # The v1 marker is DELIBERATE (do not "sweep" it to a bare mfenc:): the version must be one the - # cipher DISPATCHES on, so the failure lands in the base64 decode inside decrypt. A bare "mfenc:" - # takes the unknown-marker-version branch instead — a different fail-closed path than the one - # this test claims to exercise. + # cipher DISPATCHES on. Measured: "mfenc:v1:not-base64-$$$" parses to key_id="not-base64-$$$" with + # an EMPTY blob (no second colon), so base64 decoding succeeds and AESGCM raises ValueError("Nonce + # must be between 8 and 128 bytes") — the poison path this test exercises. A bare "mfenc:" never + # reaches it: _parse rejects the version first with CipherError, a different fail-closed branch. await enc._db.execute( "UPDATE queue SET payload=? WHERE message_id=? AND stage=?", ("mfenc:v1:not-base64-$$$", mids[1], Stage.INGRESS.value), diff --git a/tests/test_bytes_per_message_amplification.py b/tests/test_bytes_per_message_amplification.py index 146c9817..f66060e6 100644 --- a/tests/test_bytes_per_message_amplification.py +++ b/tests/test_bytes_per_message_amplification.py @@ -39,7 +39,9 @@ 1. **Character width.** `queue.payload` / `messages.raw` are `NVARCHAR(MAX)` on SQL Server with no UTF-8 collation, i.e. UTF-16: **2 bytes per ASCII character.** SQLite `TEXT` is UTF-8: 1. 2. **Cipher expansion.** With `MEFOR_STORE_ENCRYPTION_KEY` set, each copy becomes - `mfenc:v1::` — roughly `4/3 * raw + ~64` bytes. Default is identity. + `mfenc:v2:::` — the shipped at-rest format since ADR 0148 + defaulted `[store].aad_bind` on; `aad_bind=false` selects the frozen `mfenc:v1::` + writer. Either way roughly `4/3 * raw + ~64` bytes. Default cipher is identity (no key set). 3. **Everything the database writes that is not the body**: row and page overhead, indexes, and above all the **transaction log**, which durably records each of the `3 + 2H + 2N` transactions. diff --git a/tests/test_claim_fifo_heads.py b/tests/test_claim_fifo_heads.py index 194ab732..8fd0850d 100644 --- a/tests/test_claim_fifo_heads.py +++ b/tests/test_claim_fifo_heads.py @@ -577,8 +577,10 @@ async def test_poison_rows_dead_lettered_dropped_and_lane_rearmed( try: # The injected "mfenc:v1:not-base64-$$$" payloads below are undecryptable ON PURPOSE, and the # v1 marker is DELIBERATE (do not "sweep" it to a bare mfenc:): the version must be one the - # cipher DISPATCHES on, so the failure lands in the base64 decode inside decrypt. A bare - # "mfenc:" takes the unknown-marker-version branch — a different fail-closed path. + # cipher DISPATCHES on. Measured: it parses to key_id="not-base64-$$$" with an EMPTY blob (no + # second colon), so base64 decoding succeeds and AESGCM raises ValueError("Nonce must be between + # 8 and 128 bytes"). A bare "mfenc:" never reaches that — _parse rejects the version first with + # CipherError, a different fail-closed branch. # Lane 1: a single poison HEAD → dropped + DEAD + the lane re-arms. a = await _seed_ingress(enc, "IB_HP1", [100.0]) await enc._db.execute( diff --git a/tests/test_connection_event_store.py b/tests/test_connection_event_store.py index 09767a94..7e239015 100644 --- a/tests/test_connection_event_store.py +++ b/tests/test_connection_event_store.py @@ -91,7 +91,8 @@ async def test_reason_encrypted_at_rest(tmp_path: Path) -> None: assert _col_at_rest(db, "connection") == "IB" reason_disk = _col_at_rest(db, "reason") # The version-agnostic marker: which columns are enciphered is the claim, not which mfenc - # format the writer emits (that follows [store].aad_bind, v2 by default). + # format the writer emits. That is the cipher's choice — v1 from make_cipher's default here, + # v2 via build_cipher (write_v2=[store].aad_bind) or under MEFOR_TEST_FORCE_AAD_BIND. assert isinstance(reason_disk, str) and reason_disk.startswith(MARKER_PREFIX) assert "boom" not in reason_disk # …and the read path decrypts it back diff --git a/tests/test_ed_documents_e2e.py b/tests/test_ed_documents_e2e.py index bc14661a..77edf98a 100644 --- a/tests/test_ed_documents_e2e.py +++ b/tests/test_ed_documents_e2e.py @@ -171,8 +171,8 @@ async def test_base64_pdf_encrypted_at_rest(tmp_path: Path) -> None: await store.close() # On disk the PDF base64 is AES-256-GCM ciphertext, never plaintext. Anchor on the version-agnostic - # mfenc: marker — the claim is the algorithm/encryptedness, and the marker format follows - # [store].aad_bind (v2 by default), so pinning v1 here would be a latent false failure. + # mfenc: marker — the claim is encryptedness, and the marker format is the cipher's choice (v1 from + # make_cipher's default here, v2 via build_cipher/[store].aad_bind or MEFOR_TEST_FORCE_AAD_BIND). at_rest_raw = _raw_at_rest(db, column="raw", table="messages") at_rest_payload = _raw_at_rest(db, column="payload", table="queue") assert at_rest_raw.startswith(MARKER_PREFIX) diff --git a/tests/test_postgres_store.py b/tests/test_postgres_store.py index 97faf4af..72f90ec9 100644 --- a/tests/test_postgres_store.py +++ b/tests/test_postgres_store.py @@ -289,14 +289,22 @@ async def test_record_ack_sent_aa_body_encrypted_at_rest_pg(store) -> None: assert ack.body == _ACK_AA # decrypted round-trip # Raw column read (the ciphered handle's _fetchone does NOT decrypt) → ciphertext on disk. Assert # the deterministic decrypt round-trip, NOT `"MSA" not in ` (base64 can contain that run). - disk = ( - await s._fetchone( - "SELECT body FROM response WHERE message_id=$1 AND kind='ack_sent'", mid - ) - )["body"] + # destination_name + response_seq come back alongside the body: they are the row half of the + # cell AAD the store binds it under (postgres.py record_ack_sent), and destination_name is a + # SENTINEL ("\x1fack:" + inbound_name), so read it rather than reconstructing it here. + row = await s._fetchone( + "SELECT body, destination_name, response_seq FROM response" + " WHERE message_id=$1 AND kind='ack_sent'", + mid, + ) + disk = row["body"] assert disk.startswith(MARKER_PREFIX) # stored under the encrypted marker, not in the clear assert disk != _ACK_AA - assert cipher.decrypt(disk) == _ACK_AA # and it genuinely encrypts the AA frame + # Decrypt under the SAME cell AAD the store wrote with (ASVS 11.3.3 / ADR 0019): a v2 value is + # bound to its (table, column, row) cell, so a bare decrypt fails closed on one. Harmless on a + # v1 value — that reader ignores the caller's aad by design (dual-read). + aad = cell_aad("response", "body", mid, row["destination_name"], row["response_seq"]) + assert cipher.decrypt(disk, aad=aad) == _ACK_AA # and it genuinely encrypts the AA frame finally: await s.close() diff --git a/tests/test_sqlserver_store.py b/tests/test_sqlserver_store.py index d4066dd5..6c337eac 100644 --- a/tests/test_sqlserver_store.py +++ b/tests/test_sqlserver_store.py @@ -1171,14 +1171,22 @@ async def test_record_ack_sent_aa_body_encrypted_at_rest_ss(store) -> None: assert ack.body == _ACK_AA # decrypted round-trip # Raw column read (the ciphered handle's _fetchone does NOT decrypt) → ciphertext on disk. Assert # the deterministic decrypt round-trip, NOT `"MSA" not in ` (base64 can contain that run). - disk = ( - await s._fetchone( - "SELECT body FROM response WHERE message_id=? AND kind=?", (mid, "ack_sent") - ) - )["body"] + # destination_name + response_seq come back alongside the body: they are the row half of the + # cell AAD the store binds it under (sqlserver.py record_ack_sent), and destination_name is a + # SENTINEL ("\x1fack:" + inbound_name), so read it rather than reconstructing it here. + row = await s._fetchone( + "SELECT body, destination_name, response_seq FROM response" + " WHERE message_id=? AND kind=?", + (mid, "ack_sent"), + ) + disk = row["body"] assert disk.startswith(MARKER_PREFIX) # stored under the encrypted marker, not in the clear assert disk != _ACK_AA - assert cipher.decrypt(disk) == _ACK_AA # and it genuinely encrypts the AA frame + # Decrypt under the SAME cell AAD the store wrote with (ASVS 11.3.3 / ADR 0019): a v2 value is + # bound to its (table, column, row) cell, so a bare decrypt fails closed on one. Harmless on a + # v1 value — that reader ignores the caller's aad by design (dual-read). + aad = cell_aad("response", "body", mid, row["destination_name"], row["response_seq"]) + assert cipher.decrypt(disk, aad=aad) == _ACK_AA # and it genuinely encrypts the AA frame finally: await s.close() @@ -1538,9 +1546,11 @@ async def test_reencrypt_rotates_summary_and_metadata(store) -> None: # --- H4: error / last_error / message_events.detail encrypted at rest ---------- # SQL Server parity with SQLite/Postgres: the three nullable disposition-text columns route through the # SAME store cipher — at-rest ciphertext in whichever mfenc format that cipher writes, decrypt-on-read, -# rotated on rekey, and legacy plaintext migrated on open. (Naming a version here would be wrong: the -# format follows [store].aad_bind, so it is v2 by default.) The prior "SQL Server keeps these -# plaintext" residual is retired. +# rotated on rekey, and legacy plaintext migrated on open. (Naming a version here would be wrong: these +# tests hand the store a make_cipher() handle, whose writer default is still the frozen v1; the SHIPPED +# store builds its cipher via build_cipher with write_v2=[store].aad_bind, now on. The claim is which +# columns are enciphered, which holds either way.) The prior "SQL Server keeps these plaintext" +# residual is retired. async def test_error_lasterror_detail_encrypted_at_rest_and_decrypt(store) -> None: @@ -1566,7 +1576,8 @@ async def test_error_lasterror_detail_encrypted_at_rest_and_decrypt(store) -> No await s.mark_failed(item.id, fail, RetryPolicy(max_attempts=1), now=110.0) # -> DEAD # AT REST: every value is mfenc:... ciphertext — the cleartext phrase never appears in the col. - # (Version-agnostic per the section header: the format follows [store].aad_bind, v2 by default.) + # (Version-agnostic per the section header: the marker version belongs to the cipher, not to + # this claim — v1 here, v2 via build_cipher/[store].aad_bind or MEFOR_TEST_FORCE_AAD_BIND.) erow = (await s._fetchall("SELECT error FROM messages WHERE id=?", (eid,)))[0] assert erow["error"].startswith(MARKER_PREFIX) and "bad parse" not in erow["error"] qrow = (await s._fetchall("SELECT last_error FROM queue WHERE message_id=?", (mid,)))[0] diff --git a/tests/test_store_encryption.py b/tests/test_store_encryption.py index 6705f616..fcfc5258 100644 --- a/tests/test_store_encryption.py +++ b/tests/test_store_encryption.py @@ -141,8 +141,12 @@ async def test_claim_ready_dead_letters_undecryptable_row(tmp_path: Path) -> Non cur = await store._db.execute("SELECT status, last_error FROM queue WHERE id=?", (bad_id,)) row = await cur.fetchone() assert row["status"] == OutboxStatus.DEAD.value # poison row dead-lettered, not stranded - # last_error is itself ciphered now (WP-5), so decrypt it before checking the reason. - assert "undecryptable" in store._cipher.decrypt(row["last_error"] or "") + # last_error is itself ciphered (WP-5) AND cell-bound (ASVS 11.3.3 / ADR 0019), so decrypt it + # under the SAME (table, column, row) AAD the store wrote with — a bare decrypt fails closed on + # a v2 value. Harmless on v1: that reader ignores the caller's aad by design (dual-read). + assert "undecryptable" in store._cipher.decrypt( + row["last_error"] or "", aad=cell_aad("queue", "last_error", bad_id) + ) finally: await store.close() @@ -171,8 +175,11 @@ async def test_claim_ingress_dead_letters_undecryptable_row(tmp_path: Path) -> N ) row = await cur.fetchone() assert row["status"] == OutboxStatus.DEAD.value # poison row dead-lettered, not stranded - # last_error is itself ciphered now (WP-5), so decrypt it before checking the reason. - assert "undecryptable" in store._cipher.decrypt(row["last_error"] or "") + # Ciphered (WP-5) and cell-bound (ADR 0019) — decrypt under the row's own AAD, see the outbound + # poison-row test above. + assert "undecryptable" in store._cipher.decrypt( + row["last_error"] or "", aad=cell_aad("queue", "last_error", ingress_id) + ) # Dead ingress row with no outbound rows → the message is finalized to ERROR. assert (await store.get_message(mid))["status"] == MessageStatus.ERROR.value finally: @@ -418,8 +425,9 @@ async def test_rotation_reencrypts_and_retired_key_bridges(tmp_path: Path) -> No raw_b = _raw_at_rest(db) # Re-encrypted under the ACTIVE (new) key — not merely "still ciphertext". Take the expected marker # from the rotating cipher itself: active_marker_prefix carries key B's fingerprint in the right - # position for whichever format the writer emits (v1, or the v2 that [store].aad_bind makes the - # default). A bare MARKER_PREFIX would drop the under-the-new-key half of the proof. + # position for whichever format that cipher writes (v1 here, v2 when the store is handed a + # build_cipher handle or run under MEFOR_TEST_FORCE_AAD_BIND — the field order differs between the + # two). A bare MARKER_PREFIX would drop the under-the-new-key half of the proof. assert raw_b.startswith(rotating_cipher.active_marker_prefix) and raw_b != raw_a # B alone (no retired key) now reads everything — the bridge key is no longer needed. @@ -452,10 +460,16 @@ async def test_rotation_without_prior_key_raises(tmp_path: Path) -> None: # # The hard constraint is CRYPTO-1: the mfenc:v1 WRITER is frozen — existing v1 ciphertext and new v1 # writes stay byte-identical. M9 adds *agility infrastructure only*: a version/alg-dispatching cipher -# that is DECODE-CAPABLE of mfenc:v2 and CAN write it (opt-in), but WRITES v1 BY DEFAULT. AES-256-GCM -# stays the only registered algorithm. These tests pin: (1) v1 byte-identical (frozen fixture); (2) v2 -# round-trip; (3) a v2-active cipher reads v1 with no rotation; (4) mixed v1+v2 rows; (5) fail-closed -# CipherError on an unknown marker version AND an unknown alg id. +# that is DECODE-CAPABLE of mfenc:v2 and CAN write it (opt-in). AES-256-GCM stays the only registered +# algorithm. These tests pin: (1) v1 byte-identical (frozen fixture); (2) v2 round-trip; (3) a v2-active +# cipher reads v1 with no rotation; (4) mixed v1+v2 rows; (5) fail-closed CipherError on an unknown +# marker version AND an unknown alg id. +# +# TWO DIFFERENT "defaults" — do not collapse them. `make_cipher`'s `write_v2` PARAMETER still defaults +# False (the frozen v1 writer), which is what the assertions below pin and what keeps CRYPTO-1 testable +# from a bare `make_cipher(key)`. The SHIPPED at-rest format is no longer v1: `build_cipher` passes +# `write_v2=[store].aad_bind`, and ADR 0148 flipped that setting's default to True, so a store opened +# the normal way writes mfenc:v2. Everything in this section is about the former. # A FROZEN FIXTURE: a v1 blob written by the pre-M9 writer for plaintext "LEGACY-V1" under the key below # (nonce fixed to 12 zero bytes). Hardcoded so a regression in the v1 reader is caught against a value @@ -499,7 +513,9 @@ def test_v1_writer_is_byte_identical(monkeypatch: pytest.MonkeyPatch) -> None: def test_default_writer_is_v1_not_v2() -> None: - # The shipping default never emits a v2 marker — no at-rest format change ships with M9. + # `make_cipher`'s write_v2 PARAMETER default — the frozen v1 writer, CRYPTO-1. NOT a claim about the + # shipped store: that builds its cipher through build_cipher(write_v2=[store].aad_bind), which ADR + # 0148 defaulted to True, so a normally-opened store writes v2. This pins the library default only. token = make_cipher(generate_key()).encrypt("x") assert token.startswith(PREFIX) # mfenc:v1: assert not token.startswith("mfenc:v2:") @@ -855,8 +871,11 @@ async def test_foreign_key_at_runtime_dead_letters_rather_than_degrading(tmp_pat ) dead = await cur.fetchone() assert dead["status"] == OutboxStatus.DEAD.value # poison row dead-lettered, not stranded - # last_error is ciphered (WP-5) under the ACTIVE key B, so the reopened store decrypts it. - assert "undecryptable" in store._cipher.decrypt(dead["last_error"] or "") + # last_error is ciphered (WP-5) under the ACTIVE key B, so the reopened store decrypts it — + # under the row's own cell AAD (ADR 0019), which a v2 value requires and a v1 value ignores. + assert "undecryptable" in store._cipher.decrypt( + dead["last_error"] or "", aad=cell_aad("queue", "last_error", row["id"]) + ) finally: await store.close() @@ -1038,7 +1057,8 @@ async def test_migration_encrypts_existing_state_value(tmp_path: Path) -> None: store = await MessageStore.open(db, cipher=make_cipher(generate_key())) # reopen → migrate try: at_rest = _state_at_rest(db) - # sealed on disk (version-agnostic: the format follows [store].aad_bind, v2 by default) + # sealed on disk — version-agnostic, because the marker version is whatever cipher the store was + # handed writes (v1 here, v2 via build_cipher/[store].aad_bind or MEFOR_TEST_FORCE_AAD_BIND) assert at_rest.startswith(MARKER_PREFIX) and "SECRETSTATEMRN" not in at_rest assert store.state_view()[("ns", "k")] == {"mrn": "SECRETSTATEMRN"} # decrypts on read finally: diff --git a/tests/test_transform_state.py b/tests/test_transform_state.py index 7249569a..bc2df27e 100644 --- a/tests/test_transform_state.py +++ b/tests/test_transform_state.py @@ -274,7 +274,8 @@ async def test_state_value_encrypted_at_rest_and_read_back(tmp_path: Path) -> No finally: await store.close() # On disk: ciphertext (version-agnostic marker present, plaintext value not visible). Same rule as - # the rotation test below — a v1-only spelling misreads the v2 value [store].aad_bind makes default. + # the rotation test below — the marker version is the cipher's choice (v1 from make_cipher's default + # here, v2 via build_cipher/[store].aad_bind or MEFOR_TEST_FORCE_AAD_BIND), not this test's claim. at_rest = _state_at_rest(db, "patient_anon", "MRN-DOE") assert at_rest.startswith(MARKER_PREFIX) assert "ANON-XYZ" not in at_rest From 9e4ebb1b597f24da4bc1c7e0c24bc354982d0522 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 12:00:33 -0500 Subject: [PATCH 4/5] ci(supply-chain): make the pin guard actually reject a non-pin, and finish the ADR 0034 rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to 9bf7178f. The pins were right; the guard protecting them was not, and two of the four ADR 0034 rows were only half-applied. THE GUARD ACCEPTED `>=` AS A PIN. `_VERSION_OPS` listed ("==", "~=", ">=", "<=", "!="), so `pip install "sigstore>=4.4.0"` — which resolves whatever PyPI serves at tag time, byte-for-byte the exposure ADR 0034:190 calls "the highest residual in the group … runs with the OIDC identity used to publish" — passed green under a test named test_release_toolchain_pin_is_present, and its own failure text never fired. Split into _PIN_OPS ("==", "~=") for the pin decision and _SPEC_OPS (every PEP 440 operator) for matching a token to a package NAME, so an unpinned `sigstore>=…` is reported as unpinned rather than as a missing step. Three more holes in the same scan: - `"/" not in target` exempted every URL and git+ target — the one class that cannot carry a pin at all. `pip install https://…/evil.whl` inserted before the Sigstore step scored ZERO failures. Remote is now tested FIRST and always reported; only local path shapes are exempt. - `line.split(" install ", 1)[1]` disagreed with the `\bpip\s+install\b` regex that selected the line, so `pip\tinstall` raised IndexError instead of asserting. Both now anchor on one regex, which also picks up `pip3 install` and `pip --quiet install`. - The vacuity floor said >= 6 while the file has 8 pip installs — two steps could be deleted before the scan noticed. Floor is now the real count. ADR 0034 ROWS FINISHED: - The harness `packaging` install got the pin but not the venv. ADR 0034:192 prescribes both, and the venv is the half carrying the risk: it landed in release-harness's MAIN interpreter, and the steps after it attach the wheel to the release and publish to PyPI. Now /tmp/harnesssmoke, mirroring the engine job's /tmp/relsmoke. - The two SBOM installs had drifted apart, which quietly retired ADR 0034:205's pre-tag validation route ("run security.yml's sbom job … the install command there is byte-identical"). Nothing in PR CI executes release.yml, so that dispatch was its only dry-run proxy. Both lines are now the same command and a test keeps them that way. The register itself said "Recommended hardening — identified, NOT done" over four rows this branch had already done, and called them "an owner decision, not a drive-by". Rewritten with per-row status and the reason the owner gate is retired for them (they are PR-visible now). What is NOT closed is stated plainly: these pin only the top of each install, sigstore's ~30 transitive deps still float, and §3's own data (bandit==1.9.4 pinned and still flagged) says no pin moves the Scorecard finding — option B remains the only thing that does, and remains an owner call. New guards: security.yml's unpinned installs are a REGISTERED set (pip/uv/pip-audit — contents:read, schedule-only, nothing anyone installs) so a new one there still reds; the SBOM twin-identity check; and a PR-time canary that constraints.lock still carries exactly one `packaging==` line, because release.yml derives that pin at run time and `exit 1`s without it — on the tag push. RECEIPTS. Guard mutation-tested against a TEMP COPY of the workflows (repo never modified), one regression at a time — control 0 failures, no crashes: sigstore == -> >= 2 blanket_scan + named_backstop build == -> >= (1 of the 2 installs) 2 blanket_scan + named_backstop[build] sigstore install deleted 2 blanket_scan + named_backstop new unpinned named install 1 blanket_scan new https://…whl install 1 blanket_scan (scored 0 before this commit) new git+ install 1 blanket_scan packaging pin -> bare 2 blanket_scan + named_backstop[packaging] TAB / pip3 / flag-before-subcommand 2 each (was an IndexError crash) SBOM twin drift 1 sbom_twin security.yml gains an unpinned target 1 security_registry --upgrade-deps / lost --require-hashes 2 / 1 constraints.lock loses packaging== 1 packaging_canary Both workflows re-parse as YAML; permissions, triggers and job lists are byte-identical to origin/main (checked, not assumed). 195 passed / 10 skipped across all 14 test modules that read .github/. No messagefoundry/ change. --- .github/workflows/release.yml | 17 +- .github/workflows/security.yml | 10 +- ...is-triage-policy-accepted-risk-register.md | 45 ++-- tests/test_ci_venv_pinning.py | 196 +++++++++++++++--- 4 files changed, 218 insertions(+), 50 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 945da28f..3ac94d28 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -192,7 +192,11 @@ jobs: # ~=7.3.1 (not ~=7.3): the looser form floats the whole 7.x minor range, and a 7.4 could change # the CycloneDX JSON shape sbom_finalize.py parses — which exits non-zero and FAILS the release. # ~=7.3.1 still takes patch fixes and keeps the lxml-6.x/cp314 floor rationale above intact. - python -m pip install --upgrade "cyclonedx-bom~=7.3.1" + # BYTE-IDENTICAL to security.yml's SBOM install, and kept that way by a test. Nothing in PR CI + # executes release.yml (tag push only), so ADR 0034's documented pre-tag check is to dispatch + # security.yml's sbom job and read ITS log — which only proves anything while the two install + # commands are the same command. + python -m pip install "pip==26.1.2" "cyclonedx-bom~=7.3.1" # No unpinned pip bootstrap: --require-hashes resolves nothing (tests/test_ci_venv_pinning.py). python -m venv /tmp/sbomenv /tmp/sbomenv/bin/pip install --require-hashes -r docker/locks/requirements-core.lock @@ -393,15 +397,22 @@ jobs: # wheel smoke above for the full rationale. (`build` already pulls packaging>=24.0 into this # same interpreter, so the install is near-redundant; pinning it beats deleting it, which would # leave the dependency implicit and unpinned via build's own resolve.) + # + # …and into a THROWAWAY VENV, not this job's interpreter, which is the other half of ADR 0034's + # recommendation. This job holds contents: write + id-token: write and the steps AFTER this one + # attach the wheel to the release and publish it to PyPI, so an install resolved into the main + # interpreter here sits inside the publishing identity. The engine job already does it this way + # (/tmp/relsmoke); the compare script needs nothing but packaging.version + stdlib. PKG_PIN="$(sed -n 's/^packaging==\([^ ;]*\).*/\1/p' constraints.lock | head -1)" if [ -z "$PKG_PIN" ]; then echo "::error::no packaging== pin in constraints.lock — refusing an unpinned install on the release path"; exit 1 fi echo "packaging pin from constraints.lock: $PKG_PIN" - python -m pip install --quiet "packaging==$PKG_PIN" + python -m venv /tmp/harnesssmoke + /tmp/harnesssmoke/bin/pip install --quiet "packaging==$PKG_PIN" if [ "${GITHUB_REF_TYPE:-}" = "tag" ]; then want="${GITHUB_REF_NAME#v}" - python - "$built" "$want" <<'PYVER' + /tmp/harnesssmoke/bin/python - "$built" "$want" <<'PYVER' import sys from packaging.version import InvalidVersion, Version diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index b77ad255..c56e696a 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -134,10 +134,12 @@ jobs: # `requirements ` parser has no metadata and emits license-less components. The core lock # (docker/locks/requirements-core.lock) is the honest runtime closure `pip install messagefoundry` # pulls; the all-extras requirements.lock stays covered by the pip-audit job above. cyclonedx-bom - # ~=7.3 → lxml 6.x (cp314 wheels) so the 3.14 runner doesn't source-build lxml. ADR 0149. - # PINNED, keeping this the true twin of release.yml's SBOM step: ~=7.3.1 rather than ~=7.3 so a - # 7.4 cannot change the JSON shape sbom_finalize.py parses. Both SBOM builds must move together - # or the release ships a BOM this advisory job never rendered. + # ~=7.3.1 → lxml 6.x (cp314 wheels) so the 3.14 runner doesn't source-build lxml. ADR 0149. + # PINNED and BYTE-IDENTICAL to release.yml's SBOM install (enforced by + # tests/test_ci_venv_pinning.py). ~=7.3.1 rather than ~=7.3 so a 7.4 cannot change the JSON + # shape sbom_finalize.py parses. This job is ADR 0034's pre-tag dry-run for the release SBOM + # step, so the two must stay the SAME command — if they drift, dispatching this one proves + # nothing about the release. python -m pip install "pip==26.1.2" "cyclonedx-bom~=7.3.1" # No unpinned pip bootstrap: --require-hashes resolves nothing (tests/test_ci_venv_pinning.py). python -m venv /tmp/sbomenv diff --git a/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md b/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md index 9b5e9f47..8333f3e3 100644 --- a/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md +++ b/docs/adr/0034-static-analysis-triage-policy-accepted-risk-register.md @@ -180,21 +180,36 @@ practical, or expect to re-dismiss every anchor below it. The two workflow fixes deliberately made line-neutral — one line deleted, one comment line added — which is why their rationale lives in `tests/test_ci_venv_pinning.py`'s module docstring rather than in the workflow. -### Recommended hardening — identified, NOT done +### Recommended hardening Recorded here because a `won't fix` dismissal makes an item invisible, and these were found *while* -justifying those dismissals. None of them closes its alert; each reduces residual risk. - -| Where | Recommendation | Why it matters | -|---|---|---| -| `release.yml` `pip install sigstore` | Pin `sigstore==` | The **highest residual in the group**: a completely unpinned install inside the job holding `contents: write` + `id-token: write` + `attestations: write`, resolved immediately before it signs the wheel, sdist, SBOM and VEX. A malicious release fetched at that moment runs with the OIDC identity used to publish. | -| `release.yml` `pip install --upgrade pip build` | Pin `build==` | Unpinned PEP 517 frontend that produces the published wheel/sdist. | -| `release.yml` `pip install --quiet packaging` (harness job) | Pin `packaging==`; install into a throwaway venv as the engine job already does | Resolved into the **publishing** job's main interpreter rather than a scratch venv. | -| `release.yml` `pip install --quiet packaging` (`/tmp/relsmoke`) | Pin `packaging==` | Contained (disposable venv, version-compare only), but free to pin. | -| `dependabot-auto-merge.yml` `security-events: read` | Remove the scope | Dead. Its comment claims it reads Dependabot alerts, but the gate calls the **global** `/advisories` endpoint, which is repo-scope-independent. Verified; least-privilege hygiene only. | - -`sigstore`/`build`/`packaging` pins touch the **release critical path**, which no PR CI leg executes — -see below — so they are an owner decision, not a drive-by. +justifying those dismissals. **None of them closes its alert** (see §3 — a version pin does not satisfy +`PinnedDependenciesID`); each reduces residual risk. + +**Status update 2026-07-29 — the four `release.yml` rows below are DONE.** They were built together +with the guard that keeps them, and the "owner decision, not a drive-by" note that used to close this +section is retired for them: it argued the pins are unvalidatable before a tag, and the answer was to +make them PR-visible instead. The `dependabot-auto-merge.yml` scope row is still open. + +| Where | Recommendation | Status | Why it matters | +|---|---|---|---| +| `release.yml` `pip install sigstore` | Pin `sigstore==` | **Done** — `sigstore==4.4.0`. Deliberately *not* the newer 4.5.0: `.github/dependabot.yml` sets `cooldown.default-days: 5`, 4.5.0 was <48 h old, and pinning the *signing* toolchain fresher than the repo's own update policy allows would invert that policy at the highest-privilege point. Re-evaluate once it ages out. | The **highest residual in the group**: a completely unpinned install inside the job holding `contents: write` + `id-token: write` + `attestations: write`, resolved immediately before it signs the wheel, sdist, SBOM and VEX. A malicious release fetched at that moment runs with the OIDC identity used to publish. | +| `release.yml` `pip install --upgrade pip build` | Pin `build==` | **Done** — `pip==26.1.2 build==1.5.0`, in **both** the engine and harness build steps. | Unpinned PEP 517 frontend that produces the published wheel/sdist. | +| `release.yml` `pip install --quiet packaging` (harness job) | Pin `packaging==`; install into a throwaway venv as the engine job already does | **Done, both halves** — pin *derived from `constraints.lock`* (it is a DEP-1 transitive, so a literal would rot), and moved into `/tmp/harnesssmoke` mirroring `/tmp/relsmoke`. | Resolved into the **publishing** job's main interpreter rather than a scratch venv. | +| `release.yml` `pip install --quiet packaging` (`/tmp/relsmoke`) | Pin `packaging==` | **Done** — same `constraints.lock`-derived pin. | Contained (disposable venv, version-compare only), but free to pin. | +| `dependabot-auto-merge.yml` `security-events: read` | Remove the scope | **Open** | Dead. Its comment claims it reads Dependabot alerts, but the gate calls the **global** `/advisories` endpoint, which is repo-scope-independent. Verified; least-privilege hygiene only. | + +Two things the pins deliberately do **not** do. They pin only the **top** of each install — +`sigstore`'s ~30 transitive dependencies still float at signing time — and, per §3, they move the +Scorecard finding not at all. **Option B (a PEP 735 `release-tools` group flowing into `uv.lock` and a +fifth hashed export) remains the only thing that closes the alert**, and remains an owner decision +because it adds a lock artifact to the DEP-1 machinery. + +The `packaging` pins are **fail-closed on a tag**: `release.yml` `sed`s the version out of +`constraints.lock` and `exit 1`s if the line is gone. `packaging` is not a declared dependency — it +survives in that lock only as a `pytest` transitive — so +`tests/test_ci_venv_pinning.py::test_constraints_lock_still_carries_the_packaging_pin` is the PR-time +canary for a check that would otherwise first fire during a release. ### What no test can see @@ -203,4 +218,6 @@ Both workflow fixes land on paths **no PR CI leg runs**: `security.yml`'s SBOM j swallowed), and `release.yml` runs only on a tag push. So the first real execution of either edit is a nightly or **a release**. `tests/test_ci_venv_pinning.py` is a text guard over the workflow source, not an execution. Before the next tag, run `security.yml`'s sbom job via `workflow_dispatch` and read its -log — the install command there is byte-identical to `release.yml`'s. +log — the install command there is byte-identical to `release.yml`'s, and +`test_sbom_install_is_byte_identical_in_release_and_security` now enforces that identity, because the +dry-run is evidence about the release step only for as long as the two commands are the same command. diff --git a/tests/test_ci_venv_pinning.py b/tests/test_ci_venv_pinning.py index c51cdd30..b4176f89 100644 --- a/tests/test_ci_venv_pinning.py +++ b/tests/test_ci_venv_pinning.py @@ -34,6 +34,14 @@ `/tmp/relsmoke` (`release.yml`) stays out of the hash-verified rule — it exists to prove the freshly built wheel's own declared closure resolves, so feeding it a lock would defeat its purpose — but its `packaging` install is covered by the version-pin rule below. Pure text checks, no network. + +SCOPE, stated so it is a boundary rather than an oversight: the version-pin rule is the RELEASE path. +`security.yml` keeps four `--upgrade pip` bootstraps plus unpinned `uv` and `pip-audit`; those jobs are +`contents: read`, schedule/dispatch-only, and produce nothing anyone installs. They are registered in +`SECURITY_YML_ACCEPTED_UNPINNED` instead of pinned, so a NEW unpinned install there still fails — the +exception is enumerated, not open-ended. The one `security.yml` install held to the release rule is the +SBOM step, because ADR 0034 makes it the pre-tag dry-run for `release.yml`'s and the two must stay the +same command. """ from __future__ import annotations @@ -108,17 +116,38 @@ def test_scratch_venvs_do_not_hide_an_unpinned_pip_fetch(workflow: str) -> None: # --- the release path: every named package must carry a version ------------------------------------ -#: Any `pip install`, in either spelling, into any interpreter or venv. -_PIP_INSTALL = re.compile(r"\bpip\s+install\b") - -#: A pinned target names a version. `$PKG_PIN` counts — it is read out of constraints.lock at run time -#: (the quality-advisory.yml ruff-pin pattern), which is MORE current than a literal, not less. -_VERSION_OPS = ("==", "~=", ">=", "<=", "!=") +#: Any `pip install`, in any spelling that reaches a shell: `pip install`, `pip3 install`, +#: `python -m pip install`, `/bin/pip install`, and with flags BEFORE the subcommand +#: (`pip --quiet install X`). Matching only `pip install` would let any of the others through, and a +#: line this regex does not match is a line the scan below never examines — a silent hole, not a +#: failure. +_PIP_INSTALL = re.compile(r"\bpip3?\s+(?:-\S+\s+)*install\b") + +#: A PIN. `==` fixes the version; `~=X.Y.Z` fixes everything but the patch. `$PKG_PIN` counts — it is +#: read out of constraints.lock at run time (the quality-advisory.yml ruff-pin pattern), which is MORE +#: current than a literal, not less. +#: +#: `>=`, `<=` and `!=` are deliberately NOT here. They are FLOORS, not pins: `pip install +#: "sigstore>=4.4.0"` resolves whatever PyPI serves at tag time, which is exactly the exposure this +#: module exists to prevent (ADR 0034 §3 calls it "the highest residual in the group … runs with the +#: OIDC identity used to publish"). Accepting them would have let that regression pass green under a +#: test named `test_release_toolchain_pin_is_present`. If a genuine range is ever wanted here, exempt +#: it by name — do not widen this tuple. +_PIN_OPS = ("==", "~=") + +#: Every PEP 508/440 operator that can bind a version to a NAME. Used only to recognise that a token +#: like `sigstore>=4.4.0` is still a `sigstore` install — so an unpinned one is reported as unpinned +#: rather than as a missing step. +_SPEC_OPS = ("===", "==", "~=", ">=", "<=", "!=", "<", ">", "@", "[") #: Install targets that legitimately name no version: a path (the artifact under test) and the #: hash-verified lock installs, where every version is pinned INSIDE the lock. _EXEMPT_TARGETS = frozenset({"."}) +#: VCS/URL scheme prefixes. A target pip fetches over the network by URL is unpinnable BY CONSTRUCTION +#: — there is no version specifier to add — so it is always reported, never shape-exempted. +_REMOTE_SCHEMES = ("git+", "hg+", "svn+", "bzr+") + #: Tools whose release-path pin must EXIST — the non-vacuity backstop for the scan above. Deleting a #: step would otherwise make the scan pass by finding nothing left to check. RELEASE_PINNED_TOOLS = ( @@ -130,14 +159,29 @@ def test_scratch_venvs_do_not_hide_an_unpinned_pip_fetch(workflow: str) -> None: ("security.yml", "cyclonedx-bom"), ) +#: `security.yml`'s OWN unpinned installs, registered rather than pinned. That file's jobs run on a +#: schedule/dispatch with `contents: read`, no publishing identity and no artifact anyone consumes, so +#: they are off the release-path rule by decision — but registering them means a NEW unpinned install +#: added to that file still reds `test_security_yml_unpinned_installs_are_registered`. The scope call +#: is recorded here instead of being invisible. (`pip` also appears PINNED in that file's SBOM step, +#: which must stay byte-identical to release.yml's — see the twin test below.) +SECURITY_YML_ACCEPTED_UNPINNED = frozenset({"pip", "uv", "pip-audit"}) + def _install_targets(line: str) -> list[str]: """The package tokens a ``pip install`` line names — flags, and the arguments of flags that take - one, removed. Everything left is something pip will resolve.""" - body = line.split(" install ", 1)[1] + one, removed. Everything left is something pip will resolve. + + Anchored on the same regex that selected the line, rather than splitting on a literal ``" + install "``: the literal disagrees with the regex on `pip\tinstall` and on flags placed before the + subcommand, and disagreeing means an IndexError instead of a readable failure. + """ + match = _PIP_INSTALL.search(line) + if match is None: # pragma: no cover - callers filter on the same regex + return [] targets: list[str] = [] skip_next = False - for tok in body.split(): + for tok in line[match.end() :].split(): if skip_next: skip_next = False continue @@ -150,35 +194,51 @@ def _install_targets(line: str) -> list[str]: return targets +def _is_remote(target: str) -> bool: + """A URL / VCS target. Checked BEFORE any path shape test: a remote wheel URL ends in ``.whl`` and + contains ``/`` exactly like the local artifact under test, so a shape test alone would exempt the + one class of target that can never carry a pin.""" + return "://" in target or target.startswith(_REMOTE_SCHEMES) + + +def _needs_a_pin(target: str) -> bool: + """True when pip resolves this target from an index (so it must name a version) or fetches it from + the network (so it can never be pinned and is always reported).""" + if target in _EXEMPT_TARGETS: + return False + if _is_remote(target): + return True + return "/" not in target and not any(op in target for op in _PIN_OPS) + + def test_release_path_pip_installs_name_a_version() -> None: """EVERY package `release.yml` installs must carry a version specifier. A blanket scan, not a name list, so a NEW unpinned install added tomorrow fails too — the failure - mode a fixed table cannot see. Path installs (``dist/*.whl``) and ``-r `` installs are - exempt: the first is the artifact under test, the second is pinned inside the lock. + mode a fixed table cannot see. Local path installs (``dist/*.whl``) and ``-r `` installs are + exempt: the first is the artifact under test, the second is pinned inside the lock. A URL or + ``git+`` target is NOT exempt — it is unpinnable and therefore always reported. """ lines = [ln for ln in _code_lines(_WORKFLOWS / "release.yml") if _PIP_INSTALL.search(ln)] - # Non-vacuity: this file HAS a toolchain to pin. If it drops below this, the scan has stopped - # seeing the installs rather than the installs having become clean. - assert len(lines) >= 6, ( - f"release.yml now has only {len(lines)} pip installs — the scan is probably no longer " - f"matching them; re-point it rather than letting it pass on an empty set.\n{lines}" + # Non-vacuity: this file HAS a toolchain to pin, and the floor is the ACTUAL count, not a slack + # one — at `>= 6` two whole install steps could be deleted before the check noticed. Consolidating + # installs is fine; re-point this number in the same commit so the decision stays deliberate. + assert len(lines) >= 8, ( + f"release.yml now has only {len(lines)} pip installs — either the scan has stopped matching " + f"them or steps were removed; re-point this floor rather than letting it pass on a shrunken " + f"set.\n" + "\n".join(lines) ) unpinned = [ - (ln, target) - for ln in lines - for target in _install_targets(ln) - if target not in _EXEMPT_TARGETS - and "/" not in target # a path install (dist/*.whl), not a named package - and not any(op in target for op in _VERSION_OPS) + (ln, target) for ln in lines for target in _install_targets(ln) if _needs_a_pin(target) ] assert not unpinned, ( - f"release.yml installs these WITHOUT a version: {unpinned}. This workflow's jobs hold " + f"release.yml installs these WITHOUT a pin: {unpinned}. This workflow's jobs hold " f"contents/id-token/attestations: write and sign + publish the release artifacts, so an " f"unpinned resolve here takes whatever PyPI serves at tag time (Scorecard " - f"PinnedDependenciesID; ADR 0034 §3). Pin it, or derive the pin from constraints.lock the " - f"way the `packaging` installs do." + f"PinnedDependenciesID; ADR 0034 §3). Pin it with `==`/`~=`, or derive the pin from " + f"constraints.lock the way the `packaging` installs do. `>=` is NOT a pin. A URL/git+ target " + f"cannot be pinned at all — install it from an index instead." ) @@ -195,14 +255,92 @@ def test_release_toolchain_pin_is_present(workflow: str, package: str) -> None: (ln, target) for ln in lines for target in _install_targets(ln) - if target == package or target.startswith(tuple(f"{package}{op}" for op in _VERSION_OPS)) + # _SPEC_OPS, not _PIN_OPS, on purpose: `sigstore>=4.4.0` must be recognised AS a sigstore + # install so the pin check below reports it. Matching on _PIN_OPS alone would read it as "the + # step is gone" — a different, misleading failure. + if target == package or target.startswith(tuple(f"{package}{op}" for op in _SPEC_OPS)) ] assert hits, ( f"{workflow} no longer installs {package!r} — if the step was removed on purpose, drop it " f"from RELEASE_PINNED_TOOLS in the same commit; otherwise this guard just went blind." ) - unpinned = [ln for ln, target in hits if not any(op in target for op in _VERSION_OPS)] + unpinned = [ln for ln, target in hits if not any(op in target for op in _PIN_OPS)] assert not unpinned, ( - f"{workflow} installs {package!r} unpinned at: {unpinned}. Dependabot cannot bump an inline " - f"`pip install` in a workflow, so an unpinned one here is never even noticed." + f"{workflow} installs {package!r} without a `==`/`~=` pin at: {unpinned}. Dependabot cannot " + f"bump an inline `pip install` in a workflow, so an unpinned one here is never even noticed; " + f"a `>=` floor resolves to whatever PyPI serves at tag time and is not a pin." + ) + + +def test_security_yml_unpinned_installs_are_registered() -> None: + """`security.yml` is deliberately NOT held to the release-path rule — but its exceptions are a + registered set, so a new unpinned install there still fails. + + The disclosure matters: `security.yml` keeps `--upgrade pip` in four places and installs `uv` and + `pip-audit` unpinned. Those jobs are `contents: read`, scheduled/dispatch-only, and produce no + artifact anyone installs, which is why they were left alone. Recording that decision here is the + difference between a scope boundary and an oversight. + """ + lines = [ln for ln in _code_lines(_WORKFLOWS / "security.yml") if _PIP_INSTALL.search(ln)] + assert lines, "security.yml has no pip installs — this guard is no longer looking at anything" + unregistered = sorted( + { + target + for ln in lines + for target in _install_targets(ln) + if _needs_a_pin(target) and target not in SECURITY_YML_ACCEPTED_UNPINNED + } + ) + assert not unregistered, ( + f"security.yml gained unpinned install target(s) {unregistered}. Pin them, or add them to " + f"SECURITY_YML_ACCEPTED_UNPINNED with the reason — the point of the registry is that the " + f"exception is a decision someone made, not a gap nobody noticed." + ) + + +def test_sbom_install_is_byte_identical_in_release_and_security() -> None: + """The two CycloneDX installs must be the SAME command. + + Nothing in PR CI executes `release.yml` (tag push only, ADR 0034 "What no test can see"), so the + documented way to validate its SBOM step before cutting a tag is to dispatch `security.yml`'s sbom + job and read that log. That check is only evidence while the two commands are identical — the + moment they drift, the dry-run proves something about a command the release does not run. + """ + installs = {} + for workflow in ("release.yml", "security.yml"): + matches = [ + ln + for ln in _code_lines(_WORKFLOWS / workflow) + if _PIP_INSTALL.search(ln) and "cyclonedx-bom" in ln + ] + assert len(matches) == 1, ( + f"{workflow} has {len(matches)} cyclonedx-bom install lines, expected exactly 1 — " + f"re-point this twin check rather than letting it compare the wrong pair.\n{matches}" + ) + installs[workflow] = matches[0] + assert installs["release.yml"] == installs["security.yml"], ( + "the SBOM install commands have drifted:\n" + f" release.yml : {installs['release.yml']}\n" + f" security.yml: {installs['security.yml']}\n" + "ADR 0034 makes security.yml's sbom job the pre-tag dry-run for release.yml's. Keep both " + "lines identical, or replace that dry-run route with one that actually covers the release." + ) + + +def test_constraints_lock_still_carries_the_packaging_pin() -> None: + """`release.yml` derives its `packaging` pin from this line — and `exit 1`s without it, ON A TAG. + + `packaging` is not a declared dependency anywhere in `pyproject.toml`; it survives in + `constraints.lock` only as a transitive of the dev extra's test tooling (`pytest`, + `pytest-rerunfailures`). A routine Dependabot bump that drops that edge would take the line with + it, and the first thing to notice would be the tag push itself — the single most expensive moment + to discover it (release.yml's own header documents this repo's half-published v0.3.1 incident). + This is the PR-time canary for a fail-closed check that otherwise fires only during a release. + """ + body = (_REPO / "constraints.lock").read_text(encoding="utf-8") + pins = re.findall(r"^packaging==\S+", body, re.MULTILINE) + assert len(pins) == 1, ( + f"expected exactly one `packaging==` line in constraints.lock, found {pins}. release.yml " + f"resolves its pin with `sed … | head -1`, so zero lines hard-fail the next tag push and " + f"two would silently pick the first." ) From ebb651a8d310d1782aaad98f610bc386a1f1662f Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Wed, 29 Jul 2026 13:54:24 -0500 Subject: [PATCH 5/5] ci(release): pin the console job's toolchain, which the new guard caught The console publish job landed while this branch was open and installs pip, build and packaging unpinned. The guard added earlier on this branch rejects exactly that, so updating the branch against main turned all three test legs red -- the guard catching a real regression written after it was, which is the best evidence it works that this branch could have produced. pip and build take the same explicit pins the sibling build jobs use. packaging is derived from constraints.lock the way relsmoke and harnesssmoke already do, so one lock bump moves every release-path packaging install together instead of letting them drift apart. Also adds the check the pin guard structurally cannot make. Getting here, an edit put a literal 0x01 byte in release.yml -- a sed backreference written into a non-raw Python string -- and the file became unparseable YAML that GitHub Actions could never have run. The pin guard passed green through it, because it greps lines and never parses the document. So the file is now verified by parsing it, and its permissions blocks and triggers are compared against origin/main as parsed structures rather than eyeballed: jobs release, release-webconsole, release-harness; permissions identical; triggers identical. Verification: pin guard 15 passed; full suite 9331 passed, 818 skipped, 1 failed. That failure is tests/test_gate_installed_parity.py, which is PRE-EXISTING and unrelated -- it reproduces identically on clean main, this branch touches neither the gate source nor that test, and the test is LOCAL-MACHINE only (it skips on CI, where no installed gate exists). It reports that this workstation's installed ~/.claude/hooks copy is older than the source that PR #36 merged; re-running install-gate.ps1 is the fix, and that is deliberately an owner action because it activates the EnterWorktree rule for every session on the box. --- .github/workflows/release.yml | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index adf27d6d..fad928d8 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -379,7 +379,10 @@ jobs: - name: Build the console wheel (wheel-only — the package tree is force-included from the repo root) run: | - python -m pip install --upgrade pip build + # Pinned like every other build job on this path: this workflow's jobs hold contents/id-token + # write and publish the release artifacts, so an unpinned resolve takes whatever PyPI serves at + # tag time (Scorecard PinnedDependenciesID, ADR 0034 §3). Guarded by tests/test_ci_venv_pinning.py. + python -m pip install "pip==26.1.2" "build==1.5.0" # Wheel-only for the same reason as the harness: messagefoundry_webconsole/ lives OUTSIDE this # project dir and is pulled in via force-include, so an sdist would not be self-contained. python -m build --wheel ./packaging/messagefoundry-webconsole --outdir webconsole-dist @@ -394,7 +397,13 @@ jobs: # (tag webconsole-v0.3.0-rc1 -> 0.3.0rc1), so a string compare could never match a pre-release. if [ "${GITHUB_REF_TYPE:-}" = "tag" ]; then want="${GITHUB_REF_NAME#webconsole-v}" - python -m pip install --quiet packaging + # Derived from the lock, exactly as the relsmoke/harnesssmoke jobs do, so one bump moves + # every release-path packaging install together instead of drifting apart. + PKG_PIN="$(sed -n 's/^packaging==\([^ ;]*\).*/\1/p' constraints.lock | head -1)" + if [ -z "$PKG_PIN" ]; then + echo "::error::no packaging== pin in constraints.lock — refusing an unpinned install on the release path"; exit 1 + fi + python -m pip install --quiet "packaging==$PKG_PIN\" python - "$built" "$want" <<'PYVER' import sys from packaging.version import InvalidVersion, Version