From 4836c6cb48815eaa4d35eeb7825277ae27529320 Mon Sep 17 00:00:00 2001 From: BCC Agent Date: Mon, 13 Jul 2026 12:22:22 -0400 Subject: [PATCH 1/2] chore(#68): rotate signing keys, key-rotation re-attestation mode, harden show-seed-b64 Wires the maintainer's freshly-rotated signing keys into BCC (issue #68 finding 5: the old key was shared between catalog+release and had been exposed to CI), adds a key-rotation re-attestation mode to the Catalog Console so the Console can actually re-sign under the new key, and hardens the show-seed-b64 CLI prompt that led to a private key being pasted into a chat. ## New keys (#68 finding 5) - bcc_core.CATALOG_PUBKEYS -> new CATALOG pubkey (0s24PmkZcTT5yxNDdyTPHl5fyxArrHNPJKBjnXoQd8k=). Signs data/catalog.json, Console-only, offline. - .github/workflows/ci.yml EXPECTED_CATALOG_PUBKEY_B64 -> same new catalog pubkey (the CI trust anchor added for #68 finding 4). - scripts/sign_checksums.py RELEASE_PUBKEYS -> new RELEASE pubkey (6BnPgJEHJFyVltFoLTCNadIsehjy00iiW8IRlC1TfhA=). Signs SHA256SUMS only, CI-resident. - README.md 'Verifying your download' / 'Signing keys' sections filled in with both pubkeys, explicit about which key is which. The old key 082NOwVB7uURkvfyS3+knJ+40Fk6C9unsF47+2uPKo4= is retired (it was shared and exposed to CI) and is deliberately NOT retained in either trust list -- keeping a burned key in CATALOG_PUBKEYS would defeat the point of rotating it. ## Key-rotation re-attestation mode (Catalog Console) Problem: after the key swap, the existing data/catalog.json.sig (signed with the OLD key) no longer verifies under the NEW CATALOG_PUBKEYS, but catalog content is unchanged, so diff_catalogs(last_signed, current) is empty -- and can_sign()'s empty-diff guard (load-bearing, #68 finding 1) correctly refuses to sign an empty changeset. Without a rotation-aware path, the Console could never re-sign and CI would stay red forever. Fix: treat rotation as a full re-attestation, not a diff. - catalog_review.py: ReviewSession/start_review gain reattest: bool = False. When set, changes is built via diff_catalogs(None, new_catalog) -- every entry presented as if newly added, requiring a fresh acknowledgement -- instead of diffing against old_catalog. can_sign() is UNCHANGED: it still refuses a genuinely-empty changeset and still enforces the TOCTOU blob-SHA pin and the blocking-risk check, because reattest sessions simply never produce an empty changeset (unless the catalog itself is empty). - catalog_console.py: adds catalog_signature_valid_at(repo_dir, commit, raw), which calls bcc_core.verify_catalog_signature directly (never reimplemented) to detect whether the committed .sig verifies under the CURRENT CATALOG_PUBKEYS. ReviewWindow._on_load's source=main path uses this to decide reattest=True/False, and shows a loud, explicit red banner ("KEY ROTATION IN PROGRESS...") whenever reattest mode is entered -- never silent. Status text and Sign-button gating flow through the same can_sign()/all_entries_acknowledged() path as normal review. - tests/test_catalog_review.py: 6 new tests covering re-attest mode (one change-entry per server, gating until all acknowledged, then permits), confirming normal mode still refuses an empty diff (rotation path is not a general bypass), and confirming reattest mode still enforces the blocking-risk check and the TOCTOU pin. ## show-seed-b64 hardening (Task 3) The maintainer ran show-seed-b64 --release, saw an ambiguous prompt, and pasted the printed PRIVATE seed into a chat believing it was public. - cmd_show_seed_b64: passphrase prompt is now explicit ("Passphrase for the release signing key (the one YOU chose when generating it)"). A loud three-line warning banner prints to STDERR immediately before the seed ("!!! PRIVATE KEY BELOW..."); the seed itself stays alone on STDOUT so piping into pbcopy or a CI secret field still works cleanly. - cmd_keygen: labels for both key kinds now say PUBLIC/PRIVATE explicitly and state safety properties inline (safe to commit vs. never commit), so the printed output can't be mistaken for the other key's. ## Verification - ruff check . / ruff format --check .: clean. - pytest: full suite green (360 passed, 1 skipped). - Delete-the-check-and-watch-it-fail: each of can_sign()'s four gates (empty-diff, TOCTOU pin, blocking-risk, acknowledge-all) and the reattest branch itself were individually removed and confirmed to turn a test red, then restored -- see PR description for the exact failures. Not touched: data/catalog.json (content-signed, maintainer's job via the Console). No private key generated, requested, or committed. bcc.py untouched (open PR #67). --- .github/workflows/ci.yml | 2 +- README.md | 21 ++++- bcc_core.py | 2 +- catalog_console.py | 154 ++++++++++++++++++++++++++++++----- catalog_review.py | 37 ++++++++- scripts/sign_checksums.py | 16 ++-- tests/test_catalog_review.py | 92 +++++++++++++++++++++ 7 files changed, 292 insertions(+), 32 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b2cd6c4..646fe2a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -120,7 +120,7 @@ jobs: # below to the new key's base64 form, as its own reviewed change. - name: Verify data/catalog.json.sig env: - EXPECTED_CATALOG_PUBKEY_B64: "082NOwVB7uURkvfyS3+knJ+40Fk6C9unsF47+2uPKo4=" + EXPECTED_CATALOG_PUBKEY_B64: "0s24PmkZcTT5yxNDdyTPHl5fyxArrHNPJKBjnXoQd8k=" run: | python - <<'PY' import base64, os, pathlib, sys diff --git a/README.md b/README.md index c04b894..9e108ae 100644 --- a/README.md +++ b/README.md @@ -110,12 +110,29 @@ refuses to run without `--release` specifically so the catalog seed can't be exported by habit or muscle memory. **Release signing public key** (Ed25519, base64, raw 32 bytes) — this is -the RELEASE key, not the catalog key: +the RELEASE key, which signs `SHA256SUMS` (release checksums). It does +**not** sign `data/catalog.json` and is not the key `bcc_core.CATALOG_PUBKEYS` +trusts: ``` - +6BnPgJEHJFyVltFoLTCNadIsehjy00iiW8IRlC1TfhA= ``` +The catalog public key (Ed25519, base64, raw 32 bytes) — this is the key +that signs `data/catalog.json` and is trusted via `bcc_core.CATALOG_PUBKEYS` +and the CI trust anchor in `.github/workflows/ci.yml`. It is listed here +for completeness, not because you need it to verify a download — use the +*release* key above for that: + +``` +0s24PmkZcTT5yxNDdyTPHl5fyxArrHNPJKBjnXoQd8k= +``` + +Both keys above were rotated 2026-07 — see [issue #68](../../issues/68) +finding 5. The prior (shared) key is retired and is deliberately **not** +kept in either trust list; retaining a burned key would defeat the point +of rotating it. + ## Run from source ```bash diff --git a/bcc_core.py b/bcc_core.py index c81d5ea..a39f824 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -2193,7 +2193,7 @@ CATALOG_DENIED_ENV_KEYS = frozenset( # still trust an older key: verify_catalog_signature() accepts a match # against ANY key in this list. CATALOG_PUBKEYS: list[bytes] = [ - base64.b64decode("082NOwVB7uURkvfyS3+knJ+40Fk6C9unsF47+2uPKo4="), + base64.b64decode("0s24PmkZcTT5yxNDdyTPHl5fyxArrHNPJKBjnXoQd8k="), ] # Domain-separation prefix for the signed message. The signature covers diff --git a/catalog_console.py b/catalog_console.py index 736d67d..95603d4 100644 --- a/catalog_console.py +++ b/catalog_console.py @@ -283,6 +283,30 @@ def last_signed_catalog_raw(repo_dir: Path, commit: str) -> bytes | None: return review.find_last_signed_catalog_raw(candidates, sig_bytes, core.CATALOG_PUBKEYS) +def catalog_signature_valid_at(repo_dir: Path, commit: str, raw: bytes) -> bool: + """Whether data/catalog.json.sig as of `commit` verifies `raw` against + the CURRENTLY-TRUSTED bcc_core.CATALOG_PUBKEYS. + + False whenever there is no .sig file, or the .sig exists but was + produced by a key that is not (or no longer) in CATALOG_PUBKEYS -- most + notably right after a catalog signing-key rotation (issue #68 finding 5 + follow-up), when the committed .sig was produced by the now-retired old + key. This is the rotation-detection primitive ReviewWindow._on_load + uses to decide whether to enter re-attestation mode + (review.start_review(..., reattest=True)) instead of the ordinary + diff-against-last-signed path. + + Delegates to bcc_core.verify_catalog_signature -- never reimplemented, + per this module's docstring ("one source of truth"). + """ + try: + sig_sha = blob_sha_at(repo_dir, commit, SIG_PATH) + except GitError: + return False + sig_bytes = blob_bytes(repo_dir, sig_sha) + return core.verify_catalog_signature(raw, sig_bytes, core.CATALOG_PUBKEYS) + + def commit_and_push_signed_catalog( repo_dir: Path, raw_bytes: bytes, signature: bytes, *, branch: str = "main" ) -> str: @@ -709,6 +733,22 @@ class ReviewWindow(QMainWindow): top.addLayout(side) root.addLayout(top) + # KEY-ROTATION RE-ATTESTATION BANNER (issue #68 finding 5 follow-up). + # Hidden until _on_load() detects that the loaded catalog's + # signature does NOT verify under the currently-trusted + # bcc_core.CATALOG_PUBKEYS (catalog_signature_valid_at()) -- most + # commonly, right after the maintainer rotates the catalog signing + # key. This mode must NEVER be entered silently: this banner is the + # only thing standing between "every entry needs re-review" and a + # maintainer wondering why the diff view suddenly shows 19 + # "added" entries with no explanation. + self.reattest_banner = plain_label("") + self.reattest_banner.setStyleSheet( + "background-color: #c62828; color: white; font-weight: bold; padding: 8px;" + ) + self.reattest_banner.setVisible(False) + root.addWidget(self.reattest_banner) + self.scroll = QScrollArea() self.scroll.setWidgetResizable(True) self.card_container = QWidget() @@ -737,6 +777,7 @@ class ReviewWindow(QMainWindow): def _on_load(self): row = self.source_list.currentRow() + reattest = False try: if row <= 0: # source = main: diff against the last catalog a maintainer @@ -750,16 +791,32 @@ class ReviewWindow(QMainWindow): commit = fetch_ref(self.repo_dir, loaded_ref) new_raw, new_blob_sha = read_catalog_at_commit(self.repo_dir, commit) new_catalog = core.load_catalog(new_raw) - old_raw = last_signed_catalog_raw(self.repo_dir, commit) - old_catalog = ( - core.load_catalog(old_raw) - if old_raw is not None - else { - "schema": 1, - "version": 0, - "servers": [], - } - ) + + # KEY-ROTATION RE-ATTESTATION (issue #68 finding 5 + # follow-up): if the committed .sig does not verify under + # the CURRENTLY-trusted bcc_core.CATALOG_PUBKEYS, the + # trusted key changed since this catalog was last signed. + # The new key has never vouched for ANY of this catalog's + # content, so there is nothing meaningful to diff against + # -- every entry needs a fresh acknowledgement under the + # new key. Do NOT fall through to last_signed_catalog_raw() + # in this case: it walks history looking for a match under + # the (now-untrusted) old key's signature, which is exactly + # backwards after a rotation. + if not catalog_signature_valid_at(self.repo_dir, commit, new_raw): + reattest = True + old_catalog = {"schema": 1, "version": 0, "servers": []} + else: + old_raw = last_signed_catalog_raw(self.repo_dir, commit) + old_catalog = ( + core.load_catalog(old_raw) + if old_raw is not None + else { + "schema": 1, + "version": 0, + "servers": [], + } + ) else: pr = self._prs[row - 1] loaded_ref = pr.head_ref @@ -781,8 +838,21 @@ class ReviewWindow(QMainWindow): # from the SAME ref that was reviewed, instead of a hardcoded # "main" -- see review.sign_precondition and issue #68 finding 1. self.session = review.start_review( - new_blob_sha, old_catalog, new_catalog, loaded_ref=loaded_ref + new_blob_sha, old_catalog, new_catalog, loaded_ref=loaded_ref, reattest=reattest ) + if reattest: + self.reattest_banner.setText( + "KEY ROTATION IN PROGRESS -- the trusted catalog signing key changed. " + "The existing data/catalog.json.sig does NOT verify under the current " + "bcc_core.CATALOG_PUBKEYS, so it is NOT trusted. Every one of the " + f"{len(self.session.changes)} entries below must be re-reviewed and " + "acknowledged before the new key can re-sign this catalog -- this is " + "the intended cost of rotating the key, not a bug." + ) + self.reattest_banner.setVisible(True) + else: + self.reattest_banner.setVisible(False) + self.reattest_banner.setText("") self._render_cards() def _render_cards(self): @@ -817,9 +887,15 @@ class ReviewWindow(QMainWindow): all_ack = review.all_entries_acknowledged(self.session) self.sign_btn.setEnabled(all_ack) pending = len(self.session.changes) - len(self.session.acknowledged) - self.status_label.setText( - f"{len(self.session.changes)} changed entries, {pending} not yet acknowledged." - ) + if self.session.reattest: + self.status_label.setText( + f"RE-ATTESTATION under the new key: {len(self.session.changes)} entries, " + f"{pending} not yet acknowledged." + ) + else: + self.status_label.setText( + f"{len(self.session.changes)} changed entries, {pending} not yet acknowledged." + ) def _on_sign(self): assert self.session is not None @@ -920,8 +996,12 @@ def cmd_keygen(args: argparse.Namespace) -> int: ) print() print( - "Public key (base64) -- hand this to whoever maintains bcc_core.py so " - "they can add it to CATALOG_PUBKEYS (this tool does not edit that file):" + "PUBLIC key (base64, safe to commit/share) -- hand this to whoever " + "maintains bcc_core.py so they can add it to CATALOG_PUBKEYS (this tool " + "does not edit that file). There is no PRIVATE-key output for the catalog " + "key: it is never meant to leave this machine, and this tool has no " + "command that exports it (show-seed-b64 refuses without --release, and " + "even then only exports the release key)." ) print(f" {pubkey_b64}") else: @@ -932,14 +1012,23 @@ def cmd_keygen(args: argparse.Namespace) -> int: "contain it)." ) print() - print("1. Public key (base64) -- paste into scripts/sign_checksums.py RELEASE_PUBKEYS:") + print( + "1. PUBLIC key (base64, safe to commit/share) -- paste into " + "scripts/sign_checksums.py RELEASE_PUBKEYS:" + ) print(f" {pubkey_b64}") print() print( - "2. Private key -- add it as the Gitea repo secret RELEASE_SIGNING_KEY " - "(base64 of the 32-byte private seed). Get that value with:" + "2. PRIVATE key (secret -- NEVER commit, NEVER paste into chat/email) -- " + "add it as the Gitea repo secret RELEASE_SIGNING_KEY (base64 of the " + "32-byte private seed). This command does not print it; fetch it " + "separately, when you're ready to paste it straight into the Gitea " + "secret field, with:" + ) + print( + " python catalog_console.py show-seed-b64 --release " + "# prints the PRIVATE key -- read the warning banner it shows" ) - print(" python catalog_console.py show-seed-b64 --release # prints the raw key") return 0 @@ -959,12 +1048,35 @@ def cmd_show_seed_b64(args: argparse.Namespace) -> int: file=sys.stderr, ) return 1 - passphrase = getpass.getpass("Release signing key passphrase: ") + # The prior wording ("Release signing key passphrase:") was ambiguous + # enough that a maintainer who ran this command, saw that prompt, and + # then saw a base64 blob printed with zero surrounding context, pasted + # the output into a chat believing it was the PUBLIC key. It is not -- + # it is the raw private seed. Every string this command prints from here + # down exists to make that mistake structurally harder to make again. + passphrase = getpass.getpass( + "Passphrase for the release signing key (the one YOU chose when generating it): " + ) try: seed = unlock_signing_key(passphrase, kind="release") except (FileNotFoundError, ValueError) as e: print(f"error: {e}", file=sys.stderr) return 1 + + # Loud banner on STDERR, seed alone on STDOUT -- so `show-seed-b64 + # --release | pbcopy` (or piping into the Gitea secret field) still + # gets ONLY the seed, while anyone watching the terminal still sees the + # warning. Never merge these into one stream. + print("!!! PRIVATE KEY BELOW -- this is the RELEASE_SIGNING_KEY secret value.", file=sys.stderr) + print( + "!!! Paste it ONLY into the Gitea secret field. Never into chat, email, " + "a file, or a commit.", + file=sys.stderr, + ) + print( + "!!! Anyone holding this value can forge release checksum signatures.", + file=sys.stderr, + ) print(base64.b64encode(seed).decode("ascii")) return 0 diff --git a/catalog_review.py b/catalog_review.py index c3c5669..5a97834 100644 --- a/catalog_review.py +++ b/catalog_review.py @@ -442,18 +442,43 @@ class ReviewSession: from the reviewed ref on every PR review (the bug that made the PR path unable to sign at all, and forced everyone onto the vacuous main-vs-itself path instead). + + `reattest` marks a KEY-ROTATION re-attestation pass (issue #68 finding 5 + follow-up): the currently-trusted `bcc_core.CATALOG_PUBKEYS` key changed + and the existing `data/catalog.json.sig` no longer verifies under it. + Content-wise nothing may have changed -- `diff_catalogs(old, new)` can be + genuinely empty -- but the NEW key has never vouched for any of this + catalog before, so every entry needs a first-time attestation under the + new key, not a diff against the old one. When `reattest` is set, + `changes` is built as "every entry in `new_catalog`, presented as if + newly added" (via `diff_catalogs(None, new_catalog)`) instead of a + diff against `old_catalog`, so the acknowledge-gate in `can_sign()` + requires re-reviewing everything the new key will sign -- which is the + intended cost of a key rotation, not a bypass of the empty-diff guard. """ pinned_blob_sha: str old_catalog: dict new_catalog: dict loaded_ref: str = "main" + reattest: bool = False changes: list[EntryChange] = field(default_factory=list) acknowledged: set[str] = field(default_factory=set) def __post_init__(self) -> None: if not self.changes: - self.changes = diff_catalogs(self.old_catalog, self.new_catalog) + if self.reattest: + # Every entry in the new catalog is treated as though it + # were newly added -- because, under the NEW signing key, + # it is: nothing this key signs was ever attested by it + # before. Reusing diff_catalogs(None, new) (rather than a + # bespoke code path) means the same "added" risk predicate + # (risk_new_entry) and the same EntryChange shape the rest + # of this module and the GUI already know how to render + # apply here unmodified. + self.changes = diff_catalogs(None, self.new_catalog) + else: + self.changes = diff_catalogs(self.old_catalog, self.new_catalog) def start_review( @@ -461,12 +486,14 @@ def start_review( old_catalog: dict, new_catalog: dict, loaded_ref: str = "main", + reattest: bool = False, ) -> ReviewSession: return ReviewSession( pinned_blob_sha=pinned_blob_sha, old_catalog=old_catalog, new_catalog=new_catalog, loaded_ref=loaded_ref, + reattest=reattest, ) @@ -531,6 +558,14 @@ def can_sign(session: ReviewSession, current_blob_sha: str) -> SignDecision: mean "nothing to sign", never "sign unlocked". (Issue #68 finding 1; this is what let commit b08cf21 sign all 19 entries with zero of them ever reviewed.) + + This gate is unaffected by `session.reattest`: a key-rotation + re-attestation session's `changes` is built from + `diff_catalogs(None, new_catalog)` (see ReviewSession), which is + empty ONLY if the catalog itself has zero entries -- a genuinely + empty catalog either way. Rotation never manufactures a non-empty + changeset out of an empty one; it just changes *what* "non-empty" + is computed against. 1. TOCTOU: `current_blob_sha` (fetched fresh, immediately before signing, from the ref that was actually reviewed -- see sign_precondition()) must match the blob SHA pinned when review began. If the bytes on the diff --git a/scripts/sign_checksums.py b/scripts/sign_checksums.py index d4a5f88..8c44215 100755 --- a/scripts/sign_checksums.py +++ b/scripts/sign_checksums.py @@ -53,13 +53,17 @@ DOMAIN_PREFIX = b"bcc-release-v1|" # the signature on every past release: verification accepts a match against # ANY key here. # -# Empty until the maintainer generates the release keypair (separately from -# the catalog keypair) and pastes the public half in: +# Populated by the maintainer via: # python catalog_console.py keygen --release -# This is intentionally NOT pre-populated with a placeholder that looks -# like a real key -- release.yml's signing-smoke-test fails closed (loudly) -# on an empty list rather than silently verifying against nothing. -RELEASE_PUBKEYS: list[bytes] = [] +# Rotated 2026-07 (issue #68 finding 5 / #68 CI-exposure incident): the +# original key was shared with the catalog key and had been exposed to CI, +# so both keypairs were regenerated as separate, disjoint keys. This list +# holds only the current release key -- if release.yml's signing-smoke-test +# ever sees this list empty, it fails closed (loudly) rather than silently +# verifying against nothing. +RELEASE_PUBKEYS: list[bytes] = [ + base64.b64decode("6BnPgJEHJFyVltFoLTCNadIsehjy00iiW8IRlC1TfhA="), +] CHUNK_SIZE = 1024 * 1024 diff --git a/tests/test_catalog_review.py b/tests/test_catalog_review.py index 8249d7c..2fbe463 100644 --- a/tests/test_catalog_review.py +++ b/tests/test_catalog_review.py @@ -491,6 +491,98 @@ def test_sign_precondition_defaults_to_main_when_loaded_ref_unset(): assert decision.ok is True +# --------------------------------------------------------------------------- # +# ReviewSession(reattest=True): key-rotation re-attestation (issue #68 +# finding 5 follow-up). After rotating bcc_core.CATALOG_PUBKEYS, the +# existing data/catalog.json.sig no longer verifies under the new key even +# though catalog CONTENT is unchanged -- diff_catalogs(old, new) would be +# empty, and an empty changeset must never unlock Sign (can_sign gate 0). +# Re-attestation mode sidesteps that correctly: instead of diffing against +# the (now-untrustworthy) last-signed content, it treats every entry as +# requiring a fresh acknowledgement, exactly like a brand-new catalog. +# --------------------------------------------------------------------------- # +def test_reattest_mode_with_unchanged_content_yields_one_change_per_entry(): + same = _catalog(_entry(id="a"), _entry(id="b"), _entry(id="c")) + session = r.start_review("sha1", same, same, reattest=True) + assert len(session.changes) == 3 + assert {c_.entry_id for c_ in session.changes} == {"a", "b", "c"} + # Presented "as if newly added" -- old_catalog plays no role here. + assert all(c_.status == "added" for c_ in session.changes) + assert all(c_.old is None for c_ in session.changes) + + +def test_reattest_mode_can_sign_refuses_until_all_acknowledged_then_permits(): + same = _catalog(_entry(id="a"), _entry(id="b"), _entry(id="c")) + session = r.start_review("sha1", same, same, reattest=True) + + decision = r.can_sign(session, "sha1") + assert decision.ok is False + assert "acknowledged" in decision.reason.lower() + + r.acknowledge_entry(session, "a") + r.acknowledge_entry(session, "b") + decision = r.can_sign(session, "sha1") + assert decision.ok is False # "c" still outstanding + assert "acknowledged" in decision.reason.lower() + + r.acknowledge_entry(session, "c") + decision = r.can_sign(session, "sha1") + assert decision.ok is True + assert decision.reason is None + + +def test_normal_mode_with_unchanged_content_still_refuses_empty_diff(): + """The rotation path must NOT become a general bypass of the empty-diff + guard: reattest=False (the default) against identical old/new catalogs + must behave exactly as before -- can_sign refuses with 'nothing to + sign', full stop.""" + same = _catalog(_entry(id="a"), _entry(id="b")) + session = r.start_review("sha1", same, same) # reattest defaults False + assert session.changes == [] + decision = r.can_sign(session, "sha1") + assert decision.ok is False + assert "nothing to sign" in decision.reason.lower() + + +def test_reattest_mode_still_enforces_blocking_risk_check(): + """Re-attestation must not relax the blocking-risk gate: a + disallowed-command entry blocks Sign even with every entry + acknowledged.""" + cat = _catalog( + _entry(id="a"), + _entry(id="evil", config={"command": "bash", "args": ["-c", "rm -rf /"]}), + ) + session = r.start_review("sha1", cat, cat, reattest=True) + assert len(session.changes) == 2 + r.acknowledge_entry(session, "a") + r.acknowledge_entry(session, "evil") + assert r.all_entries_acknowledged(session) is True + decision = r.can_sign(session, "sha1") + assert decision.ok is False + assert "blocking" in decision.reason.lower() + + +def test_reattest_mode_still_enforces_toctou_pin(): + """Re-attestation must not relax the TOCTOU blob-SHA pin: acknowledging + everything is not enough if the bytes moved underneath the review.""" + same = _catalog(_entry(id="a")) + session = r.start_review("sha1", same, same, reattest=True) + r.acknowledge_entry(session, "a") + decision = r.can_sign(session, "sha1") + assert decision.ok is True # sanity: matches when blob is unchanged + + decision = r.can_sign(session, "sha2-a-new-commit-landed") + assert decision.ok is False + assert "mismatch" in decision.reason.lower() or "changed" in decision.reason.lower() + + +def test_reattest_defaults_to_false(): + """start_review()'s reattest parameter defaults to False -- normal + (diff-based) review is the default behaviour, never silently entered.""" + session = r.start_review("sha1", _catalog(), _catalog(_entry())) + assert session.reattest is False + + # --------------------------------------------------------------------------- # # find_last_signed_catalog_raw: what source=main diffs against # --------------------------------------------------------------------------- # -- 2.52.0 From aa40f8e139e6204a5ce4bfe06358ad6e46a31537 Mon Sep 17 00:00:00 2001 From: BCC Agent Date: Mon, 13 Jul 2026 13:10:36 -0400 Subject: [PATCH 2/2] feat(catalog-console): a `keys` status command, and complete rotation without a red main (#62, #68) Two problems from issue #68's follow-up review: 1. The maintainer -- the only person who will ever use this tool -- cannot reliably tell which of the two signing keys is which or what state either is in. He already pasted a private key into a chat window because a prompt was ambiguous. That's a defect in this tool, not user error. 2. PR #71 rotates bcc_core.CATALOG_PUBKEYS, which makes data/catalog.json.sig (signed by the retired key) stop verifying and the CI catalog-signature job go red. The Console could previously only load/sign against `main`, so the only way through was to merge a red PR and fix main afterwards -- normalizing exactly the alarm fatigue this whole design exists to prevent. Task 1 -- `python catalog_console.py keys`: A plain-English-first status report for BOTH keys: purpose, where the private half lives, whether it exists locally, its fingerprint, whether that fingerprint matches every place its public half is expected to be committed (bcc_core.CATALOG_PUBKEYS, ci.yml's trust anchor, and scripts/sign_checksums.RELEASE_PUBKEYS -- checked independently, since issue #68 finding 4 was exactly bcc_core.py and ci.yml silently drifting apart), and whether data/catalog.json.sig currently verifies -- ending with the exact command to run next. Needs no passphrase and never touches private key bytes: a plaintext public-key cache (store_public_key/load_public_key) is written alongside the existing encrypted private blob at keygen time, precisely so this command can report a fingerprint without decrypting anything. The status/report logic (key_status, render_key_status_report, recommend_next_steps, fingerprint_pubkey, extract_pubkey_list_literal, extract_ci_trust_anchor_pubkey) is pure and lives in catalog_review.py; cmd_keys in catalog_console.py is a thin printer over it, per the project's existing pure-core/thin-GUI split. Task 2 -- rotation completable without a red main: ReviewWindow now offers a "current branch" source (auto-detected via `current_branch()`, or --ref to name one explicitly) alongside "main" and open PRs. Loading it runs the exact same diff-against-last-signed / rotation-detection logic "main" always used (_load_own_ref, extracted from the old hardcoded-to-main _on_load), just parameterized on the ref. Signing now pushes to session.loaded_ref, never a hardcoded "main" (commit_and_push_signed_catalog's branch param was already there -- only the call site was wrong). The ref-list computation itself is a pure function (compute_own_refs) so this seam is unit-testable without git or Qt. None of can_sign()'s guards (empty-diff, acknowledge-all, blocking-risk, TOCTOU) were touched. This lets a rotation branch be reviewed, re-attested (every entry, since the new key never vouched for any of them -- issue #68 finding 5 follow-up), signed, and pushed to ITS OWN branch before it's ever merged. Task 3 -- label the keys everywhere: PassphraseDialog now shows which key (CATALOG vs RELEASE) and its fingerprint before the passphrase field, both in its window title and its prompt text -- the exact ambiguity that led to a private key being pasted into a chat window. cmd_keygen's stored-key confirmation now reads "CATALOG private key encrypted..." / "RELEASE private key encrypted..." instead of a capitalized-lowercase kind. The reattest banner now says "CATALOG signing key" / "CATALOG key" throughout instead of "the key". PySide6's import is now guarded (try/except -> _PYSIDE6_AVAILABLE) and every GUI class definition that depends on it moved under `if _PYSIDE6_AVAILABLE:`. `keygen`, `show-seed-b64`, and the new `keys` command have no GUI dependency and now work (and are testable) in an environment without PySide6 -- which is exactly this repo's own `test` CI job (pytest + cryptography only, no PySide6). `gui` fails with a clear message instead of an ImportError stack trace if it's missing. Tests: 26 new pure-function tests in tests/test_catalog_review.py (fingerprint_pubkey, extract_pubkey_list_literal, extract_ci_trust_anchor_pubkey, key_status, recommend_next_steps, render_key_status_report) and a new tests/test_catalog_console_git.py (14 tests) covering compute_own_refs, current_branch, commit_and_push_signed_catalog's branch targeting, and catalog_sig_status_on_disk against real local git repos -- importing catalog_console.py directly, proving it works without PySide6. 400 passed, 1 skipped (pre-existing). ruff check / ruff format --check clean. --- README.md | 17 +- catalog_console.py | 1219 +++++++++++++++++++---------- catalog_review.py | 242 ++++++ tests/test_catalog_console_git.py | 215 +++++ tests/test_catalog_review.py | 242 ++++++ 5 files changed, 1513 insertions(+), 422 deletions(-) create mode 100644 tests/test_catalog_console_git.py diff --git a/README.md b/README.md index 9e108ae..89361c9 100644 --- a/README.md +++ b/README.md @@ -90,6 +90,21 @@ key, because they protect different things and live in different places: | Generated with | `python catalog_console.py keygen` | `python catalog_console.py keygen --release` | | Exported for CI with | *(never — there is no supported way to export this key)* | `python catalog_console.py show-seed-b64 --release` | +**Confused about which key is which, or what state either is in?** Run: + +```bash +python catalog_console.py keys +``` + +It needs no passphrase (it never touches private key bytes) and prints a +plain-English report for both keys: where each private half lives, whether +it's present on this machine, its fingerprint, whether that fingerprint +matches what's actually committed in `bcc_core.py`, `ci.yml`'s trust +anchor, and `scripts/sign_checksums.py`, and whether +`data/catalog.json.sig` currently verifies — ending with the exact command +to run next for whatever state it finds. This is the check that would have +caught [issue #68](../../issues/68)'s finding 5 incident before it happened. + **Why two keys:** the catalog key is the root of trust for what BCC actually *executes* on a user's machine — every `command`/`args` pair in the shipped catalog is only there because this key signed it. If that key @@ -207,7 +222,7 @@ file is also listed, marked *legacy*, so you can copy them over. - `bcc.spec` — PyInstaller build spec (cross-platform). - `scripts/build_icons.py` — regenerates `icons/app.icns` and `icons/app.ico` from source PNGs. - `scripts/sign_checksums.py` — generates and Ed25519-signs the release `SHA256SUMS` manifest (see [Verifying your download](#verifying-your-download)). -- `catalog_console.py` / `catalog_review.py` — **maintainer-only**, never shipped to users (excluded from `bcc.spec`; see `tests/test_catalog_console_packaging.py`). The Catalog Console: review + sign `data/catalog.json`, and generate/manage both signing keys (`keygen`, `keygen --release`) — see [Signing keys](#signing-keys). +- `catalog_console.py` / `catalog_review.py` — **maintainer-only**, never shipped to users (excluded from `bcc.spec`; see `tests/test_catalog_console_packaging.py`). The Catalog Console: review + sign `data/catalog.json` (against `main`, an open PR, or the branch you have checked out — `--ref ` to be explicit, e.g. mid key-rotation, so a rotation can be signed and pushed to its own branch *before* it's merged, never forcing a red `main`), generate/manage both signing keys (`keygen`, `keygen --release`), and report on their status (`keys`, no passphrase needed) — see [Signing keys](#signing-keys). ## Building from source diff --git a/catalog_console.py b/catalog_console.py index 95603d4..938944b 100644 --- a/catalog_console.py +++ b/catalog_console.py @@ -10,14 +10,20 @@ for data/catalog.json (issue #62). Flow: Load -> Review -> Sign. 1. Load -- pick a source: an open Gitea PR touching data/catalog.json, - or the current tip of `main`. The Console fetches the exact - git blob (via a local clone's git plumbing) and PINS its - blob SHA *and the ref it came from* for the rest of this - review pass. For a PR, the diff is against `main`; for - `main`, the diff is against the last catalog a maintainer - actually SIGNED (the bytes covered by the current - data/catalog.json.sig), never against itself -- an empty - diff must mean "nothing to sign", never "sign unlocked". + the current tip of `main`, or the branch this checkout is + currently ON (or --ref names) -- the latter exists so a + catalog-signing-key rotation, or any other catalog change + landed on a branch, can be reviewed and SIGNED before that + branch is ever merged, instead of merging a PR that leaves + main red and fixing it up afterwards (issue #68). The + Console fetches the exact git blob (via a local clone's git + plumbing) and PINS its blob SHA *and the ref it came from* + for the rest of this review pass. For a PR, the diff is + against `main`; for `main` or a branch, the diff is against + the last catalog a maintainer actually SIGNED on that same + ref (the bytes covered by the current data/catalog.json.sig + there), never against itself -- an empty diff must mean + "nothing to sign", never "sign unlocked". 2. Review -- a semantic diff (catalog_review.diff_catalogs), one card per changed entry, with risk annotations (catalog_review.entry_risk_findings). A registry lookup for @@ -37,13 +43,23 @@ Flow: Load -> Review -> Sign. blocking risk finding (catalog_review.sign_precondition / can_sign -- the TOCTOU fix). On success, writes data/catalog.json + data/catalog.json.sig and commits BOTH - in a single commit, then pushes -- so main is never red - between a catalog merge and its signature. Signing uses the + in a single commit, then pushes to the SAME ref that was + reviewed (never a hardcoded "main") -- so that ref is never + red between a catalog merge and its signature, whether + that ref is `main` or a rotation branch. Signing uses the CATALOG key ONLY -- see "Two signing keys" below. The signature must be the artefact of an actual review, not a step that follows one. Signing IS the approval act. +Confused about which key is which, or what state either is in? Run +`python catalog_console.py keys` -- it needs no passphrase and prints a +plain-English status report for both keys: where each private half lives, +whether it's present, its fingerprint, whether that fingerprint matches +what's committed in bcc_core.py / ci.yml / scripts/sign_checksums.py, and +whether data/catalog.json.sig currently verifies -- ending with exactly +what to run next. + Two signing keys (issue #68 finding 5): the CATALOG key (offline, Console-only, `keygen` / `keygen --release` picks which) is the root of trust for what BCC executes and must never touch CI. The RELEASE key is @@ -69,6 +85,7 @@ import urllib.error import urllib.request from dataclasses import dataclass from pathlib import Path +from typing import ClassVar import bcc_core as core import catalog_review as review @@ -197,6 +214,89 @@ def unlock_signing_key(passphrase: str, kind: str = "catalog") -> bytes: return review.decrypt_private_key(blob, passphrase) +def _pubkey_cache_file(kind: str) -> Path: + assert kind in _KEY_KINDS, f"unknown key kind {kind!r}, expected one of {_KEY_KINDS}" + return KEY_STORAGE_DIR / f"signing_key_{kind}.pub" + + +def store_public_key(pubkey: bytes, kind: str = "catalog") -> None: + """Cache the PUBLIC half of a signing key, in plain base64, next to its + encrypted private counterpart (OS keychain if available, else the file + cache). Public keys are not secret -- this cache exists purely so + `catalog_console.py keys` can report a fingerprint and compare it + against what's committed in source WITHOUT ever decrypting (or asking + for a passphrase to unlock) the private key. Called by cmd_keygen() + right after a keypair is generated. + """ + keyring = _keyring_module() + pub_b64 = base64.b64encode(pubkey).decode("ascii") + if keyring is not None: + try: + keyring.set_password(_KEYRING_SERVICE, f"{_keyring_username(kind)}-pub", pub_b64) + return + except Exception: + pass # fall through to the file-based cache + KEY_STORAGE_DIR.mkdir(parents=True, exist_ok=True) + _pubkey_cache_file(kind).write_text(pub_b64 + "\n", encoding="utf-8") + + +def load_public_key(kind: str = "catalog") -> bytes | None: + """The cached PUBLIC key of the given `kind`, or None if no key of that + kind has been generated yet (or it predates public-key caching -- an + older keygen run that never called store_public_key). Never touches the + encrypted private blob and never asks for a passphrase.""" + keyring = _keyring_module() + if keyring is not None: + try: + pub_b64 = keyring.get_password(_KEYRING_SERVICE, f"{_keyring_username(kind)}-pub") + if pub_b64: + return base64.b64decode(pub_b64) + except Exception: + pass + path = _pubkey_cache_file(kind) + if not path.exists(): + return None + text = path.read_text(encoding="utf-8").strip() + if not text: + return None + try: + return base64.b64decode(text) + except ValueError: + return None + + +def has_encrypted_key(kind: str = "catalog") -> bool: + """Whether a private key of this `kind` has been generated on this + machine -- checked WITHOUT decrypting or asking for a passphrase, so + `catalog_console.py keys` can report existence unconditionally.""" + keyring = _keyring_module() + if keyring is not None: + try: + if keyring.get_password(_KEYRING_SERVICE, _keyring_username(kind)): + return True + except Exception: + pass + return _key_storage_file(kind).exists() + + +def describe_local_key_location(kind: str) -> str: + """Human-readable description of where a `kind` key's PRIVATE half + lives on this machine, for the `keys` report and the sign-flow prompt. + Never the release key's CI location -- that's cmd_keys' job to append, + since it's true regardless of whether a local copy also exists.""" + keyring = _keyring_module() + if keyring is not None: + try: + if keyring.get_password(_KEYRING_SERVICE, _keyring_username(kind)): + return "on this machine, in the OS keychain (encrypted, passphrase-protected)" + except Exception: + pass + key_file = _key_storage_file(kind) + if key_file.exists(): + return f"on this machine, at {key_file} (encrypted, passphrase-protected)" + return "not generated yet" + + # --------------------------------------------------------------------------- # # git plumbing against a local clone. The clone's `origin` remote is assumed # to already carry credentials (the "tokened remote" every other BCC @@ -223,6 +323,40 @@ def fetch_ref(repo_dir: Path, ref: str) -> str: return _git(repo_dir, "rev-parse", "FETCH_HEAD").strip() +def current_branch(repo_dir: Path) -> str | None: + """The branch currently checked out at `repo_dir`, or None if it can't + be determined (detached HEAD, bare repo, mid-rebase, ...). This is what + lets the Console offer "sign against the branch I already have checked + out" as a source (issue #68 rotation-completability fix) without the + maintainer having to type the branch name -- ReviewWindow defaults to + it unless --ref names one explicitly. + """ + try: + name = _git(repo_dir, "rev-parse", "--abbrev-ref", "HEAD").strip() + except GitError: + return None + return None if name in ("", "HEAD") else name + + +def compute_own_refs(explicit_ref: str | None, detected_branch: str | None) -> list[str]: + """ "main" plus (if different) `explicit_ref` or `detected_branch` -- the + exact list of refs ReviewWindow offers as "own" sources (loadable, + signable, AND pushable directly, unlike a PR's read-only + refs/pull//head). Pure: `detected_branch` is injected (normally + current_branch(repo_dir)) so this ref-resolution seam -- the fix for + issue #68's "rotation can't complete without a red main" -- is testable + without git or Qt. "main" is always index 0 so a stale/absent + list-widget selection still defaults sanely (see ReviewWindow._on_load's + row-clamping, and cmd_gui/ReviewWindow.__init__ for how `explicit_ref` + is threaded from --ref). + """ + refs = ["main"] + branch = explicit_ref or detected_branch + if branch and branch not in refs: + refs.append(branch) + return refs + + def blob_sha_at(repo_dir: Path, commit: str, path: str) -> str: """The git blob SHA of `path` as it exists at `commit`. This is what gets pinned at review-start and re-checked immediately before signing @@ -313,11 +447,18 @@ def commit_and_push_signed_catalog( """Write data/catalog.json + data/catalog.json.sig and commit BOTH in a single commit, then push to `branch`. Returns the new commit SHA. + `branch` MUST be the same ref that was actually reviewed + (ReviewWindow._on_sign passes `session.loaded_ref`, never a hardcoded + "main" -- issue #68 completability fix): a catalog change reviewed on a + branch has to be signed and pushed to THAT branch so the branch itself + is never red, rather than landing the signature on main after a merge. + This is deliberate: if signing happened in a commit AFTER the catalog - merge, main would be red (payload present, signature missing) between - every catalog merge and its signing commit. Routine red-main trains - exactly the alarm fatigue this whole design exists to prevent. Emitting - one commit with both files means main is never in that state. + merge, `branch` would be red (payload present, signature missing) + between the catalog merge and its signing commit. Routine red branches + train exactly the alarm fatigue this whole design exists to prevent. + Emitting one commit with both files means `branch` is never in that + state -- whether `branch` is main or a rotation-in-progress branch. """ _git(repo_dir, "checkout", branch) _git(repo_dir, "pull", "--ff-only", "origin", branch) @@ -507,452 +648,587 @@ def registry_fetcher(ref: review.PackageRef) -> dict | None: return None +# --------------------------------------------------------------------------- # +# Key status ("keys" command, issue #62/#68 follow-up: "make key handling +# comprehensible"). File I/O only -- the actual status/report logic is pure +# and lives in catalog_review (key_status / render_key_status_report), so +# it's unit-testable without touching disk. These wrappers read the THREE +# places a signing key's public half is expected to be committed, from +# `repo_dir`'s working tree, so `keys` reports on whatever ref/branch is +# actually checked out there (never this process's own sys.path import). +# --------------------------------------------------------------------------- # + + +def read_catalog_pubkeys_from_source(repo_dir: Path) -> list[bytes]: + path = repo_dir / "bcc_core.py" + if not path.exists(): + return [] + return review.extract_pubkey_list_literal(path.read_text(encoding="utf-8"), "CATALOG_PUBKEYS") + + +def read_release_pubkeys_from_source(repo_dir: Path) -> list[bytes]: + path = repo_dir / "scripts" / "sign_checksums.py" + if not path.exists(): + return [] + return review.extract_pubkey_list_literal(path.read_text(encoding="utf-8"), "RELEASE_PUBKEYS") + + +def read_ci_trust_anchor_pubkeys(repo_dir: Path) -> list[bytes]: + path = repo_dir / ".github" / "workflows" / "ci.yml" + if not path.exists(): + return [] + pubkey = review.extract_ci_trust_anchor_pubkey(path.read_text(encoding="utf-8")) + return [pubkey] if pubkey is not None else [] + + +def catalog_sig_status_on_disk(repo_dir: Path, committed_catalog_pubkeys: list[bytes]) -> str: + """ "valid" / "invalid" / "missing" for data/catalog.json.sig as it sits + in `repo_dir`'s WORKING TREE right now (not a git ref -- the `keys` + command deliberately reports on-disk state, which is what "the current + checked-out branch" concretely means and avoids a network fetch just to + print a status line). Verified against `committed_catalog_pubkeys` + (whatever bcc_core.py on this same working tree currently says), not + the running process's own bcc_core import, so this stays correct no + matter which ref/branch happens to be checked out. + """ + catalog_path = repo_dir / CATALOG_PATH + sig_path = repo_dir / SIG_PATH + if not catalog_path.exists() or not sig_path.exists(): + return "missing" + raw = catalog_path.read_bytes() + sig = sig_path.read_bytes() + if core.verify_catalog_signature(raw, sig, committed_catalog_pubkeys): + return "valid" + return "invalid" + + # --------------------------------------------------------------------------- # # GUI (PySide6). Everything above this line has no Qt dependency and is # exercised by tests/test_catalog_review.py; everything below is a thin # shell that calls into it. # --------------------------------------------------------------------------- # -from PySide6.QtCore import Qt, QThread, Signal # noqa: E402 -from PySide6.QtWidgets import ( # noqa: E402 - QApplication, - QCheckBox, - QDialog, - QDialogButtonBox, - QFormLayout, - QGroupBox, - QHBoxLayout, - QLabel, - QLineEdit, - QListWidget, - QListWidgetItem, - QMainWindow, - QMessageBox, - QPushButton, - QScrollArea, - QVBoxLayout, - QWidget, -) +# PySide6 is imported defensively: `keygen`, `show-seed-b64`, and `keys` +# (the commands a maintainer runs most often, and the ones this file's +# tests/py_compile-only CI environment can exercise) have no GUI dependency +# at all and must keep working even somewhere PySide6 isn't installed or +# won't import (e.g. no system Qt libs). Only `gui` needs it -- cmd_gui() +# checks _PYSIDE6_AVAILABLE and fails with a clear message instead of an +# ImportError stack trace if it's missing. +try: + from PySide6.QtCore import Qt, QThread, Signal + from PySide6.QtWidgets import ( + QApplication, + QCheckBox, + QDialog, + QDialogButtonBox, + QFormLayout, + QGroupBox, + QHBoxLayout, + QLabel, + QLineEdit, + QListWidget, + QListWidgetItem, + QMainWindow, + QMessageBox, + QPushButton, + QScrollArea, + QVBoxLayout, + QWidget, + ) +except ImportError as _pyside6_exc: # pragma: no cover - only hit where PySide6 is absent + _PYSIDE6_IMPORT_ERROR: str | None = str(_pyside6_exc) +else: + _PYSIDE6_IMPORT_ERROR = None + +_PYSIDE6_AVAILABLE = _PYSIDE6_IMPORT_ERROR is None -def plain_label(text: object) -> QLabel: - """A QLabel guaranteed to render `text` as plain text, never HTML. +if _PYSIDE6_AVAILABLE: # pragma: no branch - GUI class defs, only skipped where PySide6 is absent - Qt's QLabel auto-interprets HTML by default (Qt.AutoText), which means - an attacker-controlled description/notes/URL/package-name string - containing `` or `` would render as markup instead - of visible text -- exactly the kind of thing that could hide a homoglyph - swap or make a risk warning easy to miss. Every catalog-derived string - shown by this Console MUST go through this helper (or otherwise set - Qt.PlainText explicitly) rather than a bare QLabel(...). - """ - label = QLabel(html.escape(str(text))) - label.setTextFormat(Qt.PlainText) - label.setWordWrap(True) - return label + def plain_label(text: object) -> QLabel: + """A QLabel guaranteed to render `text` as plain text, never HTML. + Qt's QLabel auto-interprets HTML by default (Qt.AutoText), which means + an attacker-controlled description/notes/URL/package-name string + containing `` or `` would render as markup instead + of visible text -- exactly the kind of thing that could hide a homoglyph + swap or make a risk warning easy to miss. Every catalog-derived string + shown by this Console MUST go through this helper (or otherwise set + Qt.PlainText explicitly) rather than a bare QLabel(...). + """ + label = QLabel(html.escape(str(text))) + label.setTextFormat(Qt.PlainText) + label.setWordWrap(True) + return label -_SEVERITY_PREFIX = {"blocking": "✖ BLOCKING", "warning": "⚠ WARNING", "info": "ℹ INFO"} + _SEVERITY_PREFIX = {"blocking": "✖ BLOCKING", "warning": "⚠ WARNING", "info": "ℹ INFO"} + class RegistryLookupWorker(QThread): + """Off-UI-thread registry lookups, mirroring bcc.py's ConnTester/ + SpawnTester pattern. Never blocks the review UI on a slow/dead network.""" -class RegistryLookupWorker(QThread): - """Off-UI-thread registry lookups, mirroring bcc.py's ConnTester/ - SpawnTester pattern. Never blocks the review UI on a slow/dead network.""" + done = Signal(object) # list[review.RegistryInfo] - done = Signal(object) # list[review.RegistryInfo] + def __init__(self, refs: list[review.PackageRef], all_entry_ids: list[str]): + super().__init__() + self._refs = refs + self._all_entry_ids = all_entry_ids - def __init__(self, refs: list[review.PackageRef], all_entry_ids: list[str]): - super().__init__() - self._refs = refs - self._all_entry_ids = all_entry_ids + def run(self): + results = [ + review.lookup_registry_info(ref, registry_fetcher, self._all_entry_ids) + for ref in self._refs + ] + self.done.emit(results) - def run(self): - results = [ - review.lookup_registry_info(ref, registry_fetcher, self._all_entry_ids) - for ref in self._refs - ] - self.done.emit(results) + class EntryCard(QWidget): + """One changed catalog entry: the diff, risk findings, and the + acknowledge checkbox that gates Sign. `command`/`args` are rendered + visually dominant (bold-weight, larger, first) since they're the fields + that execute. + """ + acknowledged_changed = Signal(str, bool) -class EntryCard(QWidget): - """One changed catalog entry: the diff, risk findings, and the - acknowledge checkbox that gates Sign. `command`/`args` are rendered - visually dominant (bold-weight, larger, first) since they're the fields - that execute. - """ + def __init__(self, change: review.EntryChange, all_entry_ids: list[str]): + super().__init__() + self.change = change + self._all_entry_ids = all_entry_ids + self._worker: RegistryLookupWorker | None = None - acknowledged_changed = Signal(str, bool) + outline = QVBoxLayout(self) + box = QGroupBox(f"[{change.status.upper()}] {change.entry_id}") + outline.addWidget(box) + layout = QVBoxLayout(box) - def __init__(self, change: review.EntryChange, all_entry_ids: list[str]): - super().__init__() - self.change = change - self._all_entry_ids = all_entry_ids - self._worker: RegistryLookupWorker | None = None + entry = change.new or change.old or {} + config = entry.get("config") or {} - outline = QVBoxLayout(self) - box = QGroupBox(f"[{change.status.upper()}] {change.entry_id}") - outline.addWidget(box) - layout = QVBoxLayout(box) + cmd_label = plain_label(f"command: {config.get('command', '(none)')}") + cmd_label.setStyleSheet("font-weight: bold; font-size: 13pt;") + layout.addWidget(cmd_label) - entry = change.new or change.old or {} - config = entry.get("config") or {} + args_label = plain_label(f"args: {config.get('args', [])}") + args_label.setStyleSheet("font-weight: bold;") + layout.addWidget(args_label) - cmd_label = plain_label(f"command: {config.get('command', '(none)')}") - cmd_label.setStyleSheet("font-weight: bold; font-size: 13pt;") - layout.addWidget(cmd_label) + for fc in change.field_changes: + if fc.field in ("config.command", "config.args"): + continue # already shown dominant, above + layout.addWidget(plain_label(f"{fc.field}: {fc.old!r} -> {fc.new!r}")) - args_label = plain_label(f"args: {config.get('args', [])}") - args_label.setStyleSheet("font-weight: bold;") - layout.addWidget(args_label) - - for fc in change.field_changes: - if fc.field in ("config.command", "config.args"): - continue # already shown dominant, above - layout.addWidget(plain_label(f"{fc.field}: {fc.old!r} -> {fc.new!r}")) - - findings = review.entry_risk_findings(change) - for finding in findings: - prefix = _SEVERITY_PREFIX.get(finding.severity, finding.severity.upper()) - flabel = plain_label(f"{prefix}: {finding.message}") - if finding.severity == "blocking": - flabel.setStyleSheet("color: #c62828; font-weight: bold;") - elif finding.severity == "warning": - flabel.setStyleSheet("color: #ef6c00;") - else: - flabel.setStyleSheet("color: #1565c0;") - layout.addWidget(flabel) - - self.registry_label = plain_label("Registry lookup: loading...") - layout.addWidget(self.registry_label) - recheck_btn = QPushButton("Re-check") - recheck_btn.clicked.connect(self._run_registry_lookup) - layout.addWidget(recheck_btn) - - self.blocking = any(f.severity == "blocking" for f in findings) - self.checkbox = QCheckBox( - "I have reviewed this entry, including command/args and the risk" - " annotations above, and approve it." - ) - if self.blocking: - self.checkbox.setEnabled(False) - self.checkbox.setToolTip( - "This entry has a BLOCKING finding and cannot be acknowledged " - "until the underlying change is fixed (edit the PR, don't sign around it)." - ) - self.checkbox.toggled.connect( - lambda checked: self.acknowledged_changed.emit(change.entry_id, checked) - ) - layout.addWidget(self.checkbox) - - # Registry lookup is the one check a reviewer can't do by eye -- it's - # what catches a typosquatted/hijacked package (it already caught - # firecrawl-mcp in the seed data). It must run automatically as soon - # as the card exists, not wait on a click a tired maintainer might - # skip at 11pm. Off the GUI thread (RegistryLookupWorker is a - # QThread) and fails soft: a dead/slow registry can never gate - # review or signing, it just leaves this entry's lookup showing - # "unavailable". The "Re-check" button above stays for retrying a - # failed/unavailable lookup by hand. - self._run_registry_lookup() - - def _run_registry_lookup(self): - entry = self.change.new or {} - refs = review.extract_package_refs(entry) - if not refs: - self.registry_label.setText("Registry lookup: no npm/PyPI package in this entry.") - return - self.registry_label.setText("Registry lookup: loading...") - self._worker = RegistryLookupWorker(refs, self._all_entry_ids) - self._worker.done.connect(self._on_registry_result) - self._worker.start() - - def _on_registry_result(self, results: list[review.RegistryInfo]): - lines = [] - for info in results: - if not info.available: - lines.append(f"{info.ref.name}: unavailable (network/registry unreachable)") - continue - neighbor_note = ( - f" | NEAR-NEIGHBOUR of: {', '.join(info.near_neighbor_ids)}" - if info.near_neighbor_ids - else "" - ) - lines.append( - f"{info.ref.name}: publisher={info.publisher!r} age_days={info.age_days} " - f"last_release={info.last_release} downloads={info.downloads}{neighbor_note}" - ) - text = "Registry lookup:\n" + "\n".join(lines) - self.registry_label.setText(html.escape(text)) - self.registry_label.setTextFormat(Qt.PlainText) - - -class PassphraseDialog(QDialog): - def __init__(self, prompt: str, parent=None): - super().__init__(parent) - self.setWindowTitle("Signing key passphrase") - layout = QFormLayout(self) - self.edit = QLineEdit() - self.edit.setEchoMode(QLineEdit.EchoMode.Password) - layout.addRow(prompt, self.edit) - buttons = QDialogButtonBox( - QDialogButtonBox.StandardButton.Ok | QDialogButtonBox.StandardButton.Cancel - ) - buttons.accepted.connect(self.accept) - buttons.rejected.connect(self.reject) - layout.addRow(buttons) - - def passphrase(self) -> str: - return self.edit.text() - - -class ReviewWindow(QMainWindow): - def __init__(self, repo_dir: Path): - super().__init__() - self.repo_dir = repo_dir - self.session: review.ReviewSession | None = None - self.cards: dict[str, EntryCard] = {} - - self.setWindowTitle("BCC Catalog Console -- maintainer-only, never shipped") - central = QWidget() - self.setCentralWidget(central) - root = QVBoxLayout(central) - - top = QHBoxLayout() - self.source_list = QListWidget() - self.source_list.addItem(QListWidgetItem("main (current tip)")) - top.addWidget(self.source_list, 1) - - side = QVBoxLayout() - load_btn = QPushButton("Load selected source") - load_btn.clicked.connect(self._on_load) - side.addWidget(load_btn) - refresh_prs_btn = QPushButton("Refresh open PR list") - refresh_prs_btn.clicked.connect(self._refresh_pr_list) - side.addWidget(refresh_prs_btn) - side.addStretch(1) - top.addLayout(side) - root.addLayout(top) - - # KEY-ROTATION RE-ATTESTATION BANNER (issue #68 finding 5 follow-up). - # Hidden until _on_load() detects that the loaded catalog's - # signature does NOT verify under the currently-trusted - # bcc_core.CATALOG_PUBKEYS (catalog_signature_valid_at()) -- most - # commonly, right after the maintainer rotates the catalog signing - # key. This mode must NEVER be entered silently: this banner is the - # only thing standing between "every entry needs re-review" and a - # maintainer wondering why the diff view suddenly shows 19 - # "added" entries with no explanation. - self.reattest_banner = plain_label("") - self.reattest_banner.setStyleSheet( - "background-color: #c62828; color: white; font-weight: bold; padding: 8px;" - ) - self.reattest_banner.setVisible(False) - root.addWidget(self.reattest_banner) - - self.scroll = QScrollArea() - self.scroll.setWidgetResizable(True) - self.card_container = QWidget() - self.card_layout = QVBoxLayout(self.card_container) - self.scroll.setWidget(self.card_container) - root.addWidget(self.scroll, 1) - - self.status_label = plain_label("Load a source to begin review.") - root.addWidget(self.status_label) - - self.sign_btn = QPushButton("Sign") - self.sign_btn.setEnabled(False) - self.sign_btn.clicked.connect(self._on_sign) - root.addWidget(self.sign_btn) - - self._token = token_from_git_remote(self.repo_dir) - self._prs: list[CatalogPR] = [] - self._refresh_pr_list() - - def _refresh_pr_list(self): - self._prs = list_open_catalog_prs(self._token) - while self.source_list.count() > 1: - self.source_list.takeItem(1) - for pr in self._prs: - self.source_list.addItem(QListWidgetItem(f"PR #{pr.number}: {pr.title}")) - - def _on_load(self): - row = self.source_list.currentRow() - reattest = False - try: - if row <= 0: - # source = main: diff against the last catalog a maintainer - # actually SIGNED (the bytes covered by the current - # data/catalog.json.sig), never against itself. Diffing - # main-vs-main is what made an empty diff -> instantly - # "signable" in the first place (issue #68 finding 1) -- - # this is the only path that reaches sign_catalog_bytes(), - # so if it can't be trusted nothing can. - loaded_ref = "main" - commit = fetch_ref(self.repo_dir, loaded_ref) - new_raw, new_blob_sha = read_catalog_at_commit(self.repo_dir, commit) - new_catalog = core.load_catalog(new_raw) - - # KEY-ROTATION RE-ATTESTATION (issue #68 finding 5 - # follow-up): if the committed .sig does not verify under - # the CURRENTLY-trusted bcc_core.CATALOG_PUBKEYS, the - # trusted key changed since this catalog was last signed. - # The new key has never vouched for ANY of this catalog's - # content, so there is nothing meaningful to diff against - # -- every entry needs a fresh acknowledgement under the - # new key. Do NOT fall through to last_signed_catalog_raw() - # in this case: it walks history looking for a match under - # the (now-untrusted) old key's signature, which is exactly - # backwards after a rotation. - if not catalog_signature_valid_at(self.repo_dir, commit, new_raw): - reattest = True - old_catalog = {"schema": 1, "version": 0, "servers": []} + findings = review.entry_risk_findings(change) + for finding in findings: + prefix = _SEVERITY_PREFIX.get(finding.severity, finding.severity.upper()) + flabel = plain_label(f"{prefix}: {finding.message}") + if finding.severity == "blocking": + flabel.setStyleSheet("color: #c62828; font-weight: bold;") + elif finding.severity == "warning": + flabel.setStyleSheet("color: #ef6c00;") else: - old_raw = last_signed_catalog_raw(self.repo_dir, commit) - old_catalog = ( - core.load_catalog(old_raw) - if old_raw is not None - else { - "schema": 1, - "version": 0, - "servers": [], - } - ) - else: - pr = self._prs[row - 1] - loaded_ref = pr.head_ref - commit = fetch_ref(self.repo_dir, loaded_ref) - old_commit = fetch_ref(self.repo_dir, "main") + flabel.setStyleSheet("color: #1565c0;") + layout.addWidget(flabel) - new_raw, new_blob_sha = read_catalog_at_commit(self.repo_dir, commit) - new_catalog = core.load_catalog(new_raw) - old_raw, _old_sha = read_catalog_at_commit(self.repo_dir, old_commit) - old_catalog = core.load_catalog(old_raw) + self.registry_label = plain_label("Registry lookup: loading...") + layout.addWidget(self.registry_label) + recheck_btn = QPushButton("Re-check") + recheck_btn.clicked.connect(self._run_registry_lookup) + layout.addWidget(recheck_btn) - except (GitError, ValueError) as e: - QMessageBox.critical(self, "Load failed", html.escape(str(e))) - return - - self._new_raw = new_raw - # `loaded_ref` is pinned into the session (not just this method's - # local variable) so _on_sign can re-resolve the TOCTOU blob SHA - # from the SAME ref that was reviewed, instead of a hardcoded - # "main" -- see review.sign_precondition and issue #68 finding 1. - self.session = review.start_review( - new_blob_sha, old_catalog, new_catalog, loaded_ref=loaded_ref, reattest=reattest - ) - if reattest: - self.reattest_banner.setText( - "KEY ROTATION IN PROGRESS -- the trusted catalog signing key changed. " - "The existing data/catalog.json.sig does NOT verify under the current " - "bcc_core.CATALOG_PUBKEYS, so it is NOT trusted. Every one of the " - f"{len(self.session.changes)} entries below must be re-reviewed and " - "acknowledged before the new key can re-sign this catalog -- this is " - "the intended cost of rotating the key, not a bug." + self.blocking = any(f.severity == "blocking" for f in findings) + self.checkbox = QCheckBox( + "I have reviewed this entry, including command/args and the risk" + " annotations above, and approve it." + ) + if self.blocking: + self.checkbox.setEnabled(False) + self.checkbox.setToolTip( + "This entry has a BLOCKING finding and cannot be acknowledged " + "until the underlying change is fixed (edit the PR, don't sign around it)." + ) + self.checkbox.toggled.connect( + lambda checked: self.acknowledged_changed.emit(change.entry_id, checked) + ) + layout.addWidget(self.checkbox) + + # Registry lookup is the one check a reviewer can't do by eye -- it's + # what catches a typosquatted/hijacked package (it already caught + # firecrawl-mcp in the seed data). It must run automatically as soon + # as the card exists, not wait on a click a tired maintainer might + # skip at 11pm. Off the GUI thread (RegistryLookupWorker is a + # QThread) and fails soft: a dead/slow registry can never gate + # review or signing, it just leaves this entry's lookup showing + # "unavailable". The "Re-check" button above stays for retrying a + # failed/unavailable lookup by hand. + self._run_registry_lookup() + + def _run_registry_lookup(self): + entry = self.change.new or {} + refs = review.extract_package_refs(entry) + if not refs: + self.registry_label.setText("Registry lookup: no npm/PyPI package in this entry.") + return + self.registry_label.setText("Registry lookup: loading...") + self._worker = RegistryLookupWorker(refs, self._all_entry_ids) + self._worker.done.connect(self._on_registry_result) + self._worker.start() + + def _on_registry_result(self, results: list[review.RegistryInfo]): + lines = [] + for info in results: + if not info.available: + lines.append(f"{info.ref.name}: unavailable (network/registry unreachable)") + continue + neighbor_note = ( + f" | NEAR-NEIGHBOUR of: {', '.join(info.near_neighbor_ids)}" + if info.near_neighbor_ids + else "" + ) + lines.append( + f"{info.ref.name}: publisher={info.publisher!r} age_days={info.age_days} " + f"last_release={info.last_release} downloads={info.downloads}{neighbor_note}" + ) + text = "Registry lookup:\n" + "\n".join(lines) + self.registry_label.setText(html.escape(text)) + self.registry_label.setTextFormat(Qt.PlainText) + + class PassphraseDialog(QDialog): + """Prompts for a signing key's passphrase. + + `prompt` must say plainly WHICH key (CATALOG or RELEASE) is about to be + unlocked, and its fingerprint when known -- issue #62/#68 follow-up: a + maintainer must see which key he's about to type a passphrase for + BEFORE typing it, not infer it from context. This is the exact ambiguity + that led to a private key being pasted into a chat window. + """ + + def __init__(self, prompt: str, window_title: str = "Signing key passphrase", parent=None): + super().__init__(parent) + self.setWindowTitle(window_title) + layout = QFormLayout(self) + layout.addRow(plain_label(prompt)) + self.edit = QLineEdit() + self.edit.setEchoMode(QLineEdit.EchoMode.Password) + layout.addRow("Passphrase:", self.edit) + buttons = QDialogButtonBox( + QDialogButtonBox.StandardButton.Ok | QDialogButtonBox.StandardButton.Cancel + ) + buttons.accepted.connect(self.accept) + buttons.rejected.connect(self.reject) + layout.addRow(buttons) + + def passphrase(self) -> str: + return self.edit.text() + + class ReviewWindow(QMainWindow): + #: Placeholder empty catalog used when there is nothing to diff against + #: yet (no prior signature, or a key rotation invalidated the old one). + _EMPTY_CATALOG: ClassVar[dict] = {"schema": 1, "version": 0, "servers": []} + + def __init__(self, repo_dir: Path, ref: str | None = None): + super().__init__() + self.repo_dir = repo_dir + self.session: review.ReviewSession | None = None + self.cards: dict[str, EntryCard] = {} + + # "Own" refs this clone can load/diff/sign+push against directly -- + # issue #68 rotation-completability fix. "main" is always offered; + # if the checkout is on a different branch (or --ref names one + # explicitly), that branch is offered too, so a rotation in + # progress on a branch can be reviewed, signed, and pushed to ITS + # OWN ref -- completing the rotation before merge, never forcing a + # red main in between. See _load_own_ref() / _on_sign(). + self._own_refs = self._compute_own_refs(ref) + + self.setWindowTitle("BCC Catalog Console -- maintainer-only, never shipped") + central = QWidget() + self.setCentralWidget(central) + root = QVBoxLayout(central) + + top = QHBoxLayout() + self.source_list = QListWidget() + for own_ref in self._own_refs: + label = ( + f"{own_ref} (current tip)" + if own_ref == "main" + else f"{own_ref} (current branch)" + ) + self.source_list.addItem(QListWidgetItem(label)) + top.addWidget(self.source_list, 1) + + side = QVBoxLayout() + load_btn = QPushButton("Load selected source") + load_btn.clicked.connect(self._on_load) + side.addWidget(load_btn) + refresh_prs_btn = QPushButton("Refresh open PR list") + refresh_prs_btn.clicked.connect(self._refresh_pr_list) + side.addWidget(refresh_prs_btn) + side.addStretch(1) + top.addLayout(side) + root.addLayout(top) + + # KEY-ROTATION RE-ATTESTATION BANNER (issue #68 finding 5 follow-up). + # Hidden until _on_load() detects that the loaded catalog's + # signature does NOT verify under the currently-trusted + # bcc_core.CATALOG_PUBKEYS (catalog_signature_valid_at()) -- most + # commonly, right after the maintainer rotates the catalog signing + # key. This mode must NEVER be entered silently: this banner is the + # only thing standing between "every entry needs re-review" and a + # maintainer wondering why the diff view suddenly shows 19 + # "added" entries with no explanation. + self.reattest_banner = plain_label("") + self.reattest_banner.setStyleSheet( + "background-color: #c62828; color: white; font-weight: bold; padding: 8px;" ) - self.reattest_banner.setVisible(True) - else: self.reattest_banner.setVisible(False) - self.reattest_banner.setText("") - self._render_cards() + root.addWidget(self.reattest_banner) - def _render_cards(self): - while self.card_layout.count(): - item = self.card_layout.takeAt(0) - if item.widget(): - item.widget().deleteLater() - self.cards.clear() + self.scroll = QScrollArea() + self.scroll.setWidgetResizable(True) + self.card_container = QWidget() + self.card_layout = QVBoxLayout(self.card_container) + self.scroll.setWidget(self.card_container) + root.addWidget(self.scroll, 1) - assert self.session is not None - all_ids = sorted( - {e.get("id") for e in (self.session.new_catalog.get("servers") or []) if e.get("id")} - ) - for change in self.session.changes: - card = EntryCard(change, all_ids) - card.acknowledged_changed.connect(self._on_acknowledge_changed) - self.cards[change.entry_id] = card - self.card_layout.addWidget(card) - self.card_layout.addStretch(1) - self._update_status() + self.status_label = plain_label("Load a source to begin review.") + root.addWidget(self.status_label) - def _on_acknowledge_changed(self, entry_id: str, checked: bool): - assert self.session is not None - if checked: - review.acknowledge_entry(self.session, entry_id) - else: - review.unacknowledge_entry(self.session, entry_id) - self._update_status() + self.sign_btn = QPushButton("Sign") + self.sign_btn.setEnabled(False) + self.sign_btn.clicked.connect(self._on_sign) + root.addWidget(self.sign_btn) - def _update_status(self): - assert self.session is not None - all_ack = review.all_entries_acknowledged(self.session) - self.sign_btn.setEnabled(all_ack) - pending = len(self.session.changes) - len(self.session.acknowledged) - if self.session.reattest: - self.status_label.setText( - f"RE-ATTESTATION under the new key: {len(self.session.changes)} entries, " - f"{pending} not yet acknowledged." + self._token = token_from_git_remote(self.repo_dir) + self._prs: list[CatalogPR] = [] + self._refresh_pr_list() + + def _compute_own_refs(self, explicit_ref: str | None) -> list[str]: + """Thin GUI-side wrapper: injects current_branch(self.repo_dir) + (a git call) into the pure compute_own_refs() -- see that + function's docstring for what this list actually means.""" + return compute_own_refs(explicit_ref, current_branch(self.repo_dir)) + + def _refresh_pr_list(self): + self._prs = list_open_catalog_prs(self._token) + while self.source_list.count() > len(self._own_refs): + self.source_list.takeItem(len(self._own_refs)) + for pr in self._prs: + self.source_list.addItem(QListWidgetItem(f"PR #{pr.number}: {pr.title}")) + + def _load_own_ref(self, loaded_ref: str): + """Load + diff `loaded_ref` -- "main" or the maintainer's own + working branch, ANY ref this clone's origin can fetch directly (as + opposed to a PR's read-only refs/pull//head). Diffs against the + last catalog a maintainer actually SIGNED on that ref (never + against itself -- an empty diff must mean "nothing to sign", never + "sign unlocked", issue #68 finding 1) and enters KEY-ROTATION + RE-ATTESTATION mode if the committed .sig doesn't verify under the + currently-trusted bcc_core.CATALOG_PUBKEYS (issue #68 finding 5 + follow-up). This is the exact logic "main" always used -- pulled + into its own method, parameterized on the ref, so a rotation on a + branch gets the SAME treatment as one on main and can be signed + (and pushed back to that SAME branch, see _on_sign) before merge -- + the completability fix this method exists for. + + Returns (new_raw, new_blob_sha, new_catalog, old_catalog, reattest). + """ + commit = fetch_ref(self.repo_dir, loaded_ref) + new_raw, new_blob_sha = read_catalog_at_commit(self.repo_dir, commit) + new_catalog = core.load_catalog(new_raw) + + if not catalog_signature_valid_at(self.repo_dir, commit, new_raw): + reattest = True + old_catalog = dict(self._EMPTY_CATALOG) + else: + reattest = False + old_raw = last_signed_catalog_raw(self.repo_dir, commit) + old_catalog = ( + core.load_catalog(old_raw) if old_raw is not None else dict(self._EMPTY_CATALOG) + ) + + return new_raw, new_blob_sha, new_catalog, old_catalog, reattest + + def _on_load(self): + row = self.source_list.currentRow() + if row < 0: + row = 0 # nothing explicitly selected -- default to index 0 ("main") + try: + if row < len(self._own_refs): + loaded_ref = self._own_refs[row] + new_raw, new_blob_sha, new_catalog, old_catalog, reattest = self._load_own_ref( + loaded_ref + ) + else: + pr = self._prs[row - len(self._own_refs)] + loaded_ref = pr.head_ref + reattest = False + commit = fetch_ref(self.repo_dir, loaded_ref) + old_commit = fetch_ref(self.repo_dir, "main") + + new_raw, new_blob_sha = read_catalog_at_commit(self.repo_dir, commit) + new_catalog = core.load_catalog(new_raw) + old_raw, _old_sha = read_catalog_at_commit(self.repo_dir, old_commit) + old_catalog = core.load_catalog(old_raw) + + except (GitError, ValueError) as e: + QMessageBox.critical(self, "Load failed", html.escape(str(e))) + return + + self._new_raw = new_raw + # `loaded_ref` is pinned into the session (not just this method's + # local variable) so _on_sign can re-resolve the TOCTOU blob SHA + # from the SAME ref that was reviewed, instead of a hardcoded + # "main" -- see review.sign_precondition and issue #68 finding 1. + self.session = review.start_review( + new_blob_sha, old_catalog, new_catalog, loaded_ref=loaded_ref, reattest=reattest ) - else: - self.status_label.setText( - f"{len(self.session.changes)} changed entries, {pending} not yet acknowledged." + if reattest: + self.reattest_banner.setText( + "KEY ROTATION IN PROGRESS -- the trusted CATALOG signing key changed. " + "The existing data/catalog.json.sig does NOT verify under the current " + "bcc_core.CATALOG_PUBKEYS, so it is NOT trusted. Every one of the " + f"{len(self.session.changes)} entries below must be re-reviewed and " + "acknowledged before the new CATALOG key can re-sign this catalog -- " + "this is the intended cost of rotating the CATALOG key, not a bug." + ) + self.reattest_banner.setVisible(True) + else: + self.reattest_banner.setVisible(False) + self.reattest_banner.setText("") + self._render_cards() + + def _render_cards(self): + while self.card_layout.count(): + item = self.card_layout.takeAt(0) + if item.widget(): + item.widget().deleteLater() + self.cards.clear() + + assert self.session is not None + all_ids = sorted( + { + e.get("id") + for e in (self.session.new_catalog.get("servers") or []) + if e.get("id") + } ) + for change in self.session.changes: + card = EntryCard(change, all_ids) + card.acknowledged_changed.connect(self._on_acknowledge_changed) + self.cards[change.entry_id] = card + self.card_layout.addWidget(card) + self.card_layout.addStretch(1) + self._update_status() - def _on_sign(self): - assert self.session is not None + def _on_acknowledge_changed(self, entry_id: str, checked: bool): + assert self.session is not None + if checked: + review.acknowledge_entry(self.session, entry_id) + else: + review.unacknowledge_entry(self.session, entry_id) + self._update_status() - def _resolve_blob_sha(ref: str) -> str: - # Re-fetch fresh, immediately before signing, from the SAME ref - # that was reviewed (session.loaded_ref) -- NEVER hardcode - # "main" here. Hardcoding "main" is the bug that made the PR - # review path unable to sign at all: _on_load() pins the PR - # head's blob SHA, so comparing against main's SHA differs by - # definition for any PR that actually changes the catalog, and - # this refused every PR review permanently (see issue #68 - # finding 1 and review.sign_precondition's docstring). - return blob_sha_at(self.repo_dir, fetch_ref(self.repo_dir, ref), CATALOG_PATH) + def _update_status(self): + assert self.session is not None + all_ack = review.all_entries_acknowledged(self.session) + self.sign_btn.setEnabled(all_ack) + pending = len(self.session.changes) - len(self.session.acknowledged) + if self.session.reattest: + self.status_label.setText( + f"RE-ATTESTATION under the new key: {len(self.session.changes)} entries, " + f"{pending} not yet acknowledged." + ) + else: + self.status_label.setText( + f"{len(self.session.changes)} changed entries, {pending} not yet acknowledged." + ) - try: - decision = review.sign_precondition(self.session, _resolve_blob_sha) - except GitError as e: - QMessageBox.critical(self, "Sign failed", html.escape(str(e))) - return + def _on_sign(self): + assert self.session is not None - if not decision.ok: - QMessageBox.warning(self, "Cannot sign", html.escape(decision.reason or "")) - # Only the TOCTOU blob-SHA-mismatch reason should trigger a - # reload -- "mismatch" appears ONLY in that reason (deliberately - # checked instead of the broader "changed", which also matches - # "Not every CHANGED entry has been acknowledged yet" and would - # wrongly force a reload -- and with it a fresh review.py - # ReviewSession -- every time a reviewer pauses partway through - # ticking checkboxes). - if decision.reason and "mismatch" in decision.reason.lower(): - # Force a genuine re-review against the new bytes -- this - # must call _on_load() (which re-fetches and re-diffs), NOT - # re-invoke the same stale resolver, or this becomes the - # infinite loop described in issue #68 finding 1: re-pinning - # the same wrong SHA forever instead of ever converging. - self._on_load() - return + def _resolve_blob_sha(ref: str) -> str: + # Re-fetch fresh, immediately before signing, from the SAME ref + # that was reviewed (session.loaded_ref) -- NEVER hardcode + # "main" here. Hardcoding "main" is the bug that made the PR + # review path unable to sign at all: _on_load() pins the PR + # head's blob SHA, so comparing against main's SHA differs by + # definition for any PR that actually changes the catalog, and + # this refused every PR review permanently (see issue #68 + # finding 1 and review.sign_precondition's docstring). + return blob_sha_at(self.repo_dir, fetch_ref(self.repo_dir, ref), CATALOG_PATH) - dialog = PassphraseDialog("Enter CATALOG signing key passphrase:", self) - if dialog.exec() != QDialog.DialogCode.Accepted: - return - try: - seed = unlock_signing_key(dialog.passphrase()) - except (FileNotFoundError, ValueError) as e: - QMessageBox.critical(self, "Sign failed", html.escape(str(e))) - return + try: + decision = review.sign_precondition(self.session, _resolve_blob_sha) + except GitError as e: + QMessageBox.critical(self, "Sign failed", html.escape(str(e))) + return - signature = review.sign_catalog_bytes(self._new_raw, seed) - try: - new_commit = commit_and_push_signed_catalog(self.repo_dir, self._new_raw, signature) - except GitError as e: - QMessageBox.critical(self, "Commit/push failed", html.escape(str(e))) - return + if not decision.ok: + QMessageBox.warning(self, "Cannot sign", html.escape(decision.reason or "")) + # Only the TOCTOU blob-SHA-mismatch reason should trigger a + # reload -- "mismatch" appears ONLY in that reason (deliberately + # checked instead of the broader "changed", which also matches + # "Not every CHANGED entry has been acknowledged yet" and would + # wrongly force a reload -- and with it a fresh review.py + # ReviewSession -- every time a reviewer pauses partway through + # ticking checkboxes). + if decision.reason and "mismatch" in decision.reason.lower(): + # Force a genuine re-review against the new bytes -- this + # must call _on_load() (which re-fetches and re-diffs), NOT + # re-invoke the same stale resolver, or this becomes the + # infinite loop described in issue #68 finding 1: re-pinning + # the same wrong SHA forever instead of ever converging. + self._on_load() + return - QMessageBox.information(self, "Signed", f"Signed and pushed as commit {new_commit[:12]}.") - self.sign_btn.setEnabled(False) + # Show WHICH key is about to be used, and its fingerprint, BEFORE + # the passphrase field even appears -- issue #62/#68 follow-up. A + # maintainer must never have to infer which key a bare "Enter + # passphrase" prompt means; that ambiguity is exactly what led to a + # private key being pasted into a chat window. + catalog_pubkey = load_public_key("catalog") + if catalog_pubkey is not None: + fp_note = f"fingerprint {review.fingerprint_pubkey(catalog_pubkey)}" + else: + fp_note = "fingerprint unknown (generated before fingerprint caching -- re-run keygen to cache it)" + prompt = ( + f"About to sign with the CATALOG signing key ({fp_note}).\n" + "This is the root of trust for what BCC executes on a user's machine -- " + "it is never the RELEASE key.\n\n" + "Enter the CATALOG key's passphrase:" + ) + dialog = PassphraseDialog(prompt, "CATALOG signing key passphrase", self) + if dialog.exec() != QDialog.DialogCode.Accepted: + return + try: + seed = unlock_signing_key(dialog.passphrase(), kind="catalog") + except (FileNotFoundError, ValueError) as e: + QMessageBox.critical(self, "Sign failed", html.escape(str(e))) + return + + signature = review.sign_catalog_bytes(self._new_raw, seed) + try: + # Push to the SAME ref that was reviewed (session.loaded_ref), + # never a hardcoded "main" -- issue #68 completability fix. A + # rotation (or any other catalog change) reviewed on a branch + # must land on THAT branch so it can be signed and pushed + # before the branch is ever merged, instead of forcing a merge + # of a red PR followed by a fix-up on main. + new_commit = commit_and_push_signed_catalog( + self.repo_dir, self._new_raw, signature, branch=self.session.loaded_ref + ) + except GitError as e: + QMessageBox.critical(self, "Commit/push failed", html.escape(str(e))) + return + + QMessageBox.information( + self, + "Signed", + f"Signed with the CATALOG key and pushed to {self.session.loaded_ref} " + f"as commit {new_commit[:12]}.", + ) + self.sign_btn.setEnabled(False) # --------------------------------------------------------------------------- # @@ -981,9 +1257,10 @@ def cmd_keygen(args: argparse.Namespace) -> int: blob = review.encrypt_private_key(seed, passphrase) where = store_encrypted_key(blob, kind=kind) + store_public_key(pubkey, kind=kind) # non-secret; lets `keys` fingerprint without a passphrase pubkey_b64 = base64.b64encode(pubkey).decode("ascii") - print(f"{kind.capitalize()} private key encrypted and stored in: {where}") + print(f"{kind.upper()} private key encrypted and stored in: {where}") print() if kind == "catalog": print( @@ -1082,6 +1359,15 @@ def cmd_show_seed_b64(args: argparse.Namespace) -> int: def cmd_gui(args: argparse.Namespace) -> int: + if not _PYSIDE6_AVAILABLE: + print( + "error: PySide6 is not available in this Python environment, so the GUI " + f"can't launch ({_PYSIDE6_IMPORT_ERROR}). `keygen`, `show-seed-b64`, and " + "`keys` don't need it and still work here.", + file=sys.stderr, + ) + return 1 + repo_dir = Path(args.repo).resolve() if not (repo_dir / CATALOG_PATH).exists(): print( @@ -1092,18 +1378,98 @@ def cmd_gui(args: argparse.Namespace) -> int: app = QApplication(sys.argv) app.setApplicationName("BCC Catalog Console") - win = ReviewWindow(repo_dir) + win = ReviewWindow(repo_dir, ref=args.ref) win.resize(900, 700) win.show() return app.exec() +def cmd_keys(args: argparse.Namespace) -> int: + """`python catalog_console.py keys` -- "which key is what, and what + state is everything in?" (issue #62/#68 follow-up). Reads the CURRENT + WORKING TREE at --repo (so it reports on whatever branch/ref is + actually checked out there -- issue #68 rotation-completability fix + means that's often not "main" during a rotation), gathers every input + the pure catalog_review.key_status()/render_key_status_report() need, + and prints the result. Never touches, decrypts, or prints a private key + -- everything gathered here is a cached PUBLIC key, a fingerprint, file + text, or a signature verification result. + """ + repo_dir = Path(args.repo).resolve() + branch = current_branch(repo_dir) or "(unknown -- detached HEAD or not a git checkout)" + print(f"Catalog Console -- key status for {repo_dir}") + print(f"Checked-out ref: {branch}") + print() + + committed_catalog_pubkeys = read_catalog_pubkeys_from_source(repo_dir) + ci_trust_anchor_pubkeys = read_ci_trust_anchor_pubkeys(repo_dir) + committed_release_pubkeys = read_release_pubkeys_from_source(repo_dir) + + catalog_status = review.key_status( + "catalog", + display_name="CATALOG", + purpose=( + "Signs the server list that BCC writes into your Claude config. This is " + "what decides which programs run on a user's machine -- the root of trust." + ), + private_key_location=describe_local_key_location("catalog"), + local_exists=has_encrypted_key("catalog"), + local_pubkey=load_public_key("catalog"), + locations=[ + ("bcc_core.CATALOG_PUBKEYS", committed_catalog_pubkeys), + ("ci.yml trust anchor (EXPECTED_CATALOG_PUBKEY_B64)", ci_trust_anchor_pubkeys), + ], + catalog_sig_status=catalog_sig_status_on_disk(repo_dir, committed_catalog_pubkeys), + ) + + release_where = describe_local_key_location("release") + if release_where == "not generated yet": + release_where = ( + "not generated yet, and not verifiable from here as present in CI " + "(check the Gitea repo secret RELEASE_SIGNING_KEY directly)" + ) + else: + release_where = ( + f"{release_where} -- intended to be pasted into the Gitea secret " + "RELEASE_SIGNING_KEY (via `show-seed-b64 --release`) and not kept as the " + "primary copy once that's done" + ) + release_status = review.key_status( + "release", + display_name="RELEASE", + purpose=( + "Signs the SHA256SUMS checksum manifest for release downloads only. " + "CI-resident on purpose: a CI compromise burns this key, never the " + "catalog key -- that asymmetry is the whole point of having two keys." + ), + private_key_location=release_where, + local_exists=has_encrypted_key("release"), + local_pubkey=load_public_key("release"), + locations=[("scripts/sign_checksums.py RELEASE_PUBKEYS", committed_release_pubkeys)], + catalog_sig_status=None, + ) + + print(review.render_key_status_report([catalog_status, release_status])) + return 0 + + def build_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser(description=__doc__) sub = parser.add_subparsers(dest="command") p_gui = sub.add_parser("gui", help="launch the review/sign GUI (default)") p_gui.add_argument("--repo", default=".", help="path to a BCC git checkout (default: cwd)") + p_gui.add_argument( + "--ref", + default=None, + help=( + "branch to offer as an additional load/sign/push source, alongside " + "'main' and any open PRs -- e.g. a key-rotation branch, so rotation " + "can be reviewed and signed BEFORE merge instead of forcing a red main " + "(issue #68). Defaults to whatever branch --repo is currently checked " + "out on; only needed if that's not the branch you mean." + ), + ) p_gui.set_defaults(func=cmd_gui) p_keygen = sub.add_parser( @@ -1131,10 +1497,21 @@ def build_parser() -> argparse.ArgumentParser: ) p_seed.set_defaults(func=cmd_show_seed_b64) + p_keys = sub.add_parser( + "keys", + help=( + "print a plain-English status report: which key is what, where its " + "private half lives, whether it matches what's committed, and whether " + "data/catalog.json.sig currently verifies" + ), + ) + p_keys.add_argument("--repo", default=".", help="path to a BCC git checkout (default: cwd)") + p_keys.set_defaults(func=cmd_keys) + return parser -_SUBCOMMANDS = ("gui", "keygen", "show-seed-b64", "-h", "--help") +_SUBCOMMANDS = ("gui", "keygen", "show-seed-b64", "keys", "-h", "--help") def main(argv: list[str] | None = None) -> int: diff --git a/catalog_review.py b/catalog_review.py index 5a97834..d0f0f8b 100644 --- a/catalog_review.py +++ b/catalog_review.py @@ -16,6 +16,8 @@ new surface" recurring-bug lesson). from __future__ import annotations +import base64 +import hashlib import os import re from collections.abc import Callable @@ -851,3 +853,243 @@ _NON_ASCII_RE = re.compile(r"[^\x00-\x7f]") def contains_non_ascii(s: str) -> bool: return bool(_NON_ASCII_RE.search(s)) + + +# --------------------------------------------------------------------------- # +# Key status reporting (issue #62/#68 follow-up: "make key handling +# comprehensible"). Pure functions only -- `catalog_console.py cmd_keys` is a +# thin printer that gathers inputs (local key caches, source-file text, the +# catalog + its .sig) and hands them here. NEVER touches private key bytes: +# every input/output here is a public key, a fingerprint, or a status string. +# --------------------------------------------------------------------------- # + + +def fingerprint_pubkey(pubkey: bytes) -> str: + """Short, human-comparable fingerprint of a raw Ed25519 public key: the + first 16 hex chars of its SHA-256 digest, grouped in 4s (e.g. "3F2A 9C1B + 44DE 08AA") so two fingerprints can be eyeballed for a mismatch the way a + PGP fingerprint is. Deliberately NOT the raw base64 pubkey itself in the + default short form (that's available via the full committed value in the + report) -- a fixed-width grouped hex string is easier to compare at a + glance and to read aloud/type over chat if needed. Never derived from, + and never printed alongside, any private key material. + """ + digest = hashlib.sha256(pubkey).hexdigest().upper()[:16] + return " ".join(digest[i : i + 4] for i in range(0, len(digest), 4)) + + +_PUBKEY_LIST_B64_RE = re.compile(r'base64\.b64decode\(\s*"([^"]+)"\s*\)') + + +def extract_pubkey_list_literal(source_text: str, var_name: str) -> list[bytes]: + """Best-effort extraction of a `: list[bytes] = [...]` literal + (each entry a `base64.b64decode("...")` call, matching the exact style + bcc_core.CATALOG_PUBKEYS and scripts.sign_checksums.RELEASE_PUBKEYS are + both written in) straight out of Python source TEXT. + + Deliberately a regex over text, not an import: `catalog_console.py keys` + must report on whatever ref/branch is checked out at the inspected repo + path, which may not be (and need not be) importable from the running + process's own sys.path. Returns [] if the variable isn't found in this + exact shape -- callers treat that as "nothing committed here", not an + error, since a report that can't parse a file should say so plainly + rather than crash the whole `keys` command over one malformed file. + """ + match = re.search( + rf"{re.escape(var_name)}\s*:\s*list\[bytes\]\s*=\s*\[(.*?)\]", source_text, re.DOTALL + ) + if not match: + return [] + keys: list[bytes] = [] + for b64 in _PUBKEY_LIST_B64_RE.findall(match.group(1)): + try: + keys.append(base64.b64decode(b64)) + except ValueError: + continue + return keys + + +_CI_TRUST_ANCHOR_RE = re.compile(r'EXPECTED_CATALOG_PUBKEY_B64:\s*"([^"]+)"') + + +def extract_ci_trust_anchor_pubkey(ci_yml_text: str) -> bytes | None: + """Best-effort extraction of ci.yml's `EXPECTED_CATALOG_PUBKEY_B64` trust + anchor (issue #68 finding 4) from the workflow file's TEXT. Returns None + if the constant isn't found -- the `keys` report shows that plainly + ("not found in ci.yml") rather than raising. + """ + match = _CI_TRUST_ANCHOR_RE.search(ci_yml_text) + if not match: + return None + try: + return base64.b64decode(match.group(1)) + except ValueError: + return None + + +@dataclass(frozen=True) +class PubkeyLocationCheck: + """One place in the source tree a key's public half is expected to be + committed, and whether the fingerprint(s) found there match the key + stored locally.""" + + location: str + committed_fingerprints: tuple[str, ...] + status: str # "match" | "mismatch" | "unknown" (no local key to compare against) + + +@dataclass(frozen=True) +class KeyStatus: + """Everything `catalog_console.py keys` reports about ONE signing key. + Built by key_status() below; rendered by render_key_status_report(). + Never carries private key material -- every field here is safe to print. + """ + + kind: str # "catalog" | "release" + display_name: str # "CATALOG" | "RELEASE" + purpose: str # one-line plain-English purpose + private_key_location: str # human-readable, e.g. "on this machine, in the OS keychain" + local_exists: bool + local_fingerprint: str | None + locations: tuple[PubkeyLocationCheck, ...] + catalog_sig_status: str | None = None # "valid" | "invalid" | "missing" | None (n/a) + + +def key_status( + kind: str, + *, + display_name: str, + purpose: str, + private_key_location: str, + local_exists: bool, + local_pubkey: bytes | None, + locations: list[tuple[str, list[bytes]]], + catalog_sig_status: str | None = None, +) -> KeyStatus: + """Pure assembly of a KeyStatus from already-resolved inputs (no file or + git I/O here -- that's catalog_console.py's job). `locations` is a list + of (label, committed_pubkeys) pairs, e.g. + [("bcc_core.CATALOG_PUBKEYS", [...]), ("ci.yml trust anchor", [...])], + so a key can be checked against every place its public half is expected + to be committed, independently -- this is the check that would have + caught bcc_core.CATALOG_PUBKEYS and ci.yml's trust anchor silently + drifting apart (issue #68 finding 4 was exactly that kind of drift). + """ + checks: list[PubkeyLocationCheck] = [] + for label, committed_pubkeys in locations: + fps = tuple(fingerprint_pubkey(pk) for pk in committed_pubkeys) + if local_pubkey is None: + status = "unknown" + elif local_pubkey in committed_pubkeys: + status = "match" + else: + status = "mismatch" + checks.append( + PubkeyLocationCheck(location=label, committed_fingerprints=fps, status=status) + ) + + return KeyStatus( + kind=kind, + display_name=display_name, + purpose=purpose, + private_key_location=private_key_location, + local_exists=local_exists, + local_fingerprint=fingerprint_pubkey(local_pubkey) if local_pubkey is not None else None, + locations=tuple(checks), + catalog_sig_status=catalog_sig_status, + ) + + +_LOCATION_STATUS_ICON = {"match": "✅", "mismatch": "❌", "unknown": "⚠️"} +_LOCATION_STATUS_VERDICT = { + "match": "MATCHES the local private key", + "mismatch": "DOES NOT MATCH the local private key", + "unknown": "cannot compare -- no local key to check against", +} +_CATALOG_SIG_STATUS_LINE = { + "valid": "✅ data/catalog.json.sig verifies under the committed CATALOG_PUBKEYS.", + "invalid": ( + "❌ data/catalog.json.sig does NOT verify under the committed CATALOG_PUBKEYS -- " + "the catalog needs re-signing (Load → acknowledge all → Sign)." + ), + "missing": ( + "⚠️ data/catalog.json.sig is missing entirely -- the catalog has never been signed." + ), +} + + +def recommend_next_steps(statuses: list[KeyStatus]) -> list[str]: + """The pure "what to do next" logic behind the keys report's closing + section -- one concrete, runnable-looking instruction per problem found, + naming the exact key involved (never just "the key"). Returns a single + reassuring line if nothing needs attention.""" + steps: list[str] = [] + for s in statuses: + if not s.local_exists: + flag = " --release" if s.kind == "release" else "" + steps.append( + f"{s.display_name} key has never been generated on this machine -- run " + f"`python catalog_console.py keygen{flag}`." + ) + continue + for loc in s.locations: + if loc.status == "mismatch": + steps.append( + f"{s.display_name} key's local fingerprint does not match " + f"{loc.location} -- update {loc.location} to the fingerprint shown " + "above (or, if this is unexpected, treat the committed key as " + "untrusted and investigate before doing anything else)." + ) + elif loc.status == "unknown": + steps.append( + f"{s.display_name} key's local fingerprint could not be checked against " + f"{loc.location} -- re-run keygen (or, for an older install, unlock the " + "key once) so its public half is cached locally." + ) + if s.kind == "catalog" and s.catalog_sig_status in ("invalid", "missing"): + steps.append( + "The catalog needs re-signing: run `python catalog_console.py gui --repo .` " + "and Load → acknowledge every entry → Sign. If main is red because " + "of a key rotation, load the branch with the rotation instead of main " + "(current-branch / --ref source) so the fix lands before merge." + ) + if not steps: + steps.append("Everything is consistent -- no action needed.") + return steps + + +def render_key_status_report(statuses: list[KeyStatus]) -> str: + """Render a full, plain-English-first key status report as one string. + `catalog_console.py cmd_keys` prints this verbatim -- the CLI is a thin + printer over this pure function, which is what makes the report's + content (not just its plumbing) unit-testable.""" + lines: list[str] = [] + for s in statuses: + lines.append(f"=== {s.display_name} KEY ===") + lines.append(s.purpose) + lines.append(f"Private half lives: {s.private_key_location}") + if s.local_exists and s.local_fingerprint: + lines.append(f"Exists locally: yes (fingerprint {s.local_fingerprint})") + elif s.local_exists: + lines.append("Exists locally: yes (fingerprint unknown -- re-run keygen to cache it)") + else: + lines.append("Exists locally: no") + for loc in s.locations: + icon = _LOCATION_STATUS_ICON.get(loc.status, "?") + fps = ( + ", ".join(loc.committed_fingerprints) + if loc.committed_fingerprints + else "(nothing committed here)" + ) + verdict = _LOCATION_STATUS_VERDICT.get(loc.status, loc.status) + lines.append(f" {icon} {loc.location}: {fps} -- {verdict}") + if s.catalog_sig_status is not None: + lines.append( + f"Catalog signature: {_CATALOG_SIG_STATUS_LINE.get(s.catalog_sig_status, s.catalog_sig_status)}" + ) + lines.append("") + + lines.append("What to do next:") + for step in recommend_next_steps(statuses): + lines.append(f" - {step}") + return "\n".join(lines) diff --git a/tests/test_catalog_console_git.py b/tests/test_catalog_console_git.py new file mode 100644 index 0000000..0578550 --- /dev/null +++ b/tests/test_catalog_console_git.py @@ -0,0 +1,215 @@ +"""Tests for catalog_console.py's non-Qt git plumbing and ref-resolution +seam (issue #68 rotation-completability fix). + +catalog_console.py is importable here WITHOUT PySide6 -- its Qt import is +guarded (`_PYSIDE6_AVAILABLE`) precisely so `keygen`, `show-seed-b64`, +`keys`, and this git plumbing stay usable (and testable) wherever PySide6 +isn't installed, including this CI test job, which never installs it. If +PySide6 genuinely isn't importable in this environment, that itself +exercises the guard path -- see test_module_imports_without_pyside6. +""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import catalog_console as cc +import catalog_review as review + +_SEED_CATALOG = b'{"schema": 1, "version": 1, "servers": []}' +_SEED_SIG = b"\x00" * 64 + + +def _run(*args: str, cwd: Path) -> None: + subprocess.run(["git", *args], cwd=cwd, check=True, capture_output=True) + + +def _init_bare_and_clone(tmp_path: Path) -> tuple[Path, Path]: + """A bare "origin" repo with `main` and `rotation-branch` both seeded + with a catalog + (dummy) signature, plus a working clone with `origin` + already configured -- mirroring the tokened-remote clone + catalog_console.py's git plumbing is always run against.""" + origin = tmp_path / "origin.git" + _run("init", "--bare", str(origin), cwd=tmp_path) + + seed = tmp_path / "seed" + _run("clone", str(origin), str(seed), cwd=tmp_path) + _run("config", "user.email", "test@example.com", cwd=seed) + _run("config", "user.name", "Test", cwd=seed) + + (seed / "data").mkdir() + (seed / "data" / "catalog.json").write_bytes(_SEED_CATALOG) + (seed / "data" / "catalog.json.sig").write_bytes(_SEED_SIG) + _run("add", "-A", cwd=seed) + _run("commit", "-m", "seed", cwd=seed) + _run("push", "origin", "HEAD:refs/heads/main", cwd=seed) + _run("checkout", "-b", "rotation-branch", cwd=seed) + _run("push", "origin", "HEAD:refs/heads/rotation-branch", cwd=seed) + + clone = tmp_path / "work" + _run("clone", str(origin), str(clone), cwd=tmp_path) + _run("config", "user.email", "test@example.com", cwd=clone) + _run("config", "user.name", "Test", cwd=clone) + return origin, clone + + +# --------------------------------------------------------------------------- # +# The module must stay importable without PySide6 -- this IS the fix that +# lets `keys`/`keygen`/`show-seed-b64` (and this whole test file) run +# somewhere PySide6 isn't installed. +# --------------------------------------------------------------------------- # +def test_module_imports_without_pyside6(): + assert hasattr(cc, "_PYSIDE6_AVAILABLE") + # This CI test job never installs PySide6 (see .github/workflows/ci.yml + # "Install test dependencies": pytest + cryptography only) -- so on CI, + # this assertion is itself proof the guard is doing its job. Locally, + # where a maintainer's env DOES have PySide6, it's fine either way; the + # only real assertion this test needs is "importing the module never + # raises", which happened just by getting this far. + assert cc._PYSIDE6_AVAILABLE in (True, False) + + +def test_cmd_gui_fails_soft_without_pyside6(monkeypatch, capsys): + if cc._PYSIDE6_AVAILABLE: + return # nothing to prove where PySide6 IS available + import argparse + + args = argparse.Namespace(repo=".", ref=None) + assert cc.cmd_gui(args) == 1 + assert "PySide6" in capsys.readouterr().err + + +# --------------------------------------------------------------------------- # +# compute_own_refs: the PURE ref-resolution seam. No git, no Qt. +# --------------------------------------------------------------------------- # +def test_compute_own_refs_defaults_to_main_only(): + assert cc.compute_own_refs(None, None) == ["main"] + + +def test_compute_own_refs_adds_detected_branch(): + assert cc.compute_own_refs(None, "chore/68-key-rotation") == [ + "main", + "chore/68-key-rotation", + ] + + +def test_compute_own_refs_explicit_ref_overrides_detected_branch(): + assert cc.compute_own_refs("explicit-branch", "detected-branch") == [ + "main", + "explicit-branch", + ] + + +def test_compute_own_refs_does_not_duplicate_main(): + assert cc.compute_own_refs(None, "main") == ["main"] + assert cc.compute_own_refs("main", "some-other-branch") == ["main"] + + +# --------------------------------------------------------------------------- # +# current_branch: git plumbing, no Qt. +# --------------------------------------------------------------------------- # +def test_current_branch_detects_checked_out_branch(tmp_path): + _origin, clone = _init_bare_and_clone(tmp_path) + _run("fetch", "origin", "rotation-branch", cwd=clone) + _run("checkout", "-B", "rotation-branch", "origin/rotation-branch", cwd=clone) + assert cc.current_branch(clone) == "rotation-branch" + + +def test_current_branch_none_on_detached_head(tmp_path): + _origin, clone = _init_bare_and_clone(tmp_path) + commit = cc.fetch_ref(clone, "main") + _run("checkout", commit, cwd=clone) + assert cc.current_branch(clone) is None + + +# --------------------------------------------------------------------------- # +# commit_and_push_signed_catalog: MUST target the given branch, never a +# hardcoded "main" -- issue #68's completability fix. This is exactly the +# bug that, before the fix, would have made ReviewWindow._on_sign push a +# PR/branch review's signature straight to main regardless of what was +# actually reviewed. +# --------------------------------------------------------------------------- # +def test_commit_and_push_signed_catalog_targets_the_given_branch_not_main(tmp_path): + _origin, clone = _init_bare_and_clone(tmp_path) + + new_raw = b'{"schema": 1, "version": 2, "servers": []}' + new_sig = b"\x01" * 64 + cc.commit_and_push_signed_catalog(clone, new_raw, new_sig, branch="rotation-branch") + + rotation_commit = cc.fetch_ref(clone, "rotation-branch") + rotation_raw, _sha = cc.read_catalog_at_commit(clone, rotation_commit) + assert rotation_raw == new_raw + + # main on the shared origin must be COMPLETELY untouched by a sign that + # was reviewed and pushed against rotation-branch. + main_commit = cc.fetch_ref(clone, "main") + main_raw, _sha = cc.read_catalog_at_commit(clone, main_commit) + assert main_raw == _SEED_CATALOG + + +def test_commit_and_push_signed_catalog_still_defaults_to_main(tmp_path): + """Backward-compatible default: callers that don't pass `branch` (there + are none left in catalog_console.py itself, but the signature keeps the + default for any other caller / test fixture) still push to main.""" + _origin, clone = _init_bare_and_clone(tmp_path) + + new_raw = b'{"schema": 1, "version": 2, "servers": []}' + new_sig = b"\x01" * 64 + cc.commit_and_push_signed_catalog(clone, new_raw, new_sig) + + main_commit = cc.fetch_ref(clone, "main") + main_raw, _sha = cc.read_catalog_at_commit(clone, main_commit) + assert main_raw == new_raw + + rotation_commit = cc.fetch_ref(clone, "rotation-branch") + rotation_raw, _sha = cc.read_catalog_at_commit(clone, rotation_commit) + assert rotation_raw == _SEED_CATALOG # untouched + + +# --------------------------------------------------------------------------- # +# catalog_sig_status_on_disk: the check behind `keys`' "does catalog.json.sig +# currently verify?" line -- this is precisely the check that would have +# caught the current chore/68-key-rotation state (bcc_core.CATALOG_PUBKEYS +# rotated, data/catalog.json.sig still signed by the retired key). +# --------------------------------------------------------------------------- # +def test_catalog_sig_status_on_disk_valid(tmp_path): + seed, pub = review.generate_keypair() + raw = b'{"schema": 1, "version": 1, "servers": []}' + sig = review.sign_catalog_bytes(raw, seed) + + (tmp_path / "data").mkdir() + (tmp_path / "data" / "catalog.json").write_bytes(raw) + (tmp_path / "data" / "catalog.json.sig").write_bytes(sig) + + assert cc.catalog_sig_status_on_disk(tmp_path, [pub]) == "valid" + + +def test_catalog_sig_status_on_disk_invalid_when_pubkey_rotated(tmp_path): + """The exact chore/68-key-rotation scenario: signed by an OLD key, but + the committed pubkey list now only has the NEW key.""" + old_seed, _old_pub = review.generate_keypair() + _new_seed, new_pub = review.generate_keypair() + raw = b'{"schema": 1, "version": 1, "servers": []}' + sig = review.sign_catalog_bytes(raw, old_seed) + + (tmp_path / "data").mkdir() + (tmp_path / "data" / "catalog.json").write_bytes(raw) + (tmp_path / "data" / "catalog.json.sig").write_bytes(sig) + + assert cc.catalog_sig_status_on_disk(tmp_path, [new_pub]) == "invalid" + + +def test_catalog_sig_status_on_disk_missing_when_no_sig_file(tmp_path): + (tmp_path / "data").mkdir() + (tmp_path / "data" / "catalog.json").write_bytes(b"{}") + assert cc.catalog_sig_status_on_disk(tmp_path, []) == "missing" + + +def test_catalog_sig_status_on_disk_missing_when_no_catalog_file(tmp_path): + (tmp_path / "data").mkdir() + (tmp_path / "data" / "catalog.json.sig").write_bytes(b"\x00" * 64) + assert cc.catalog_sig_status_on_disk(tmp_path, []) == "missing" diff --git a/tests/test_catalog_review.py b/tests/test_catalog_review.py index 2fbe463..b5f4c65 100644 --- a/tests/test_catalog_review.py +++ b/tests/test_catalog_review.py @@ -785,3 +785,245 @@ def test_contains_non_ascii_true(): def test_contains_non_ascii_false(): assert r.contains_non_ascii("package") is False + + +# --------------------------------------------------------------------------- # +# fingerprint_pubkey +# --------------------------------------------------------------------------- # +def test_fingerprint_pubkey_is_deterministic(): + pub = b"\x01" * 32 + assert r.fingerprint_pubkey(pub) == r.fingerprint_pubkey(pub) + + +def test_fingerprint_pubkey_differs_for_different_keys(): + assert r.fingerprint_pubkey(b"\x01" * 32) != r.fingerprint_pubkey(b"\x02" * 32) + + +def test_fingerprint_pubkey_never_contains_the_key_bytes_themselves(): + pub = b"\x42" * 32 + fp = r.fingerprint_pubkey(pub) + assert pub.hex() not in fp.lower().replace(" ", "") + + +# --------------------------------------------------------------------------- # +# extract_pubkey_list_literal / extract_ci_trust_anchor_pubkey: text parsing +# for `catalog_console.py keys`, exercised here with no file I/O. +# --------------------------------------------------------------------------- # +def test_extract_pubkey_list_literal_single_key(): + _seed, pub = r.generate_keypair() + import base64 + + text = ( + "CATALOG_PUBKEYS: list[bytes] = [\n" + f' base64.b64decode("{base64.b64encode(pub).decode()}"),\n' + "]\n" + ) + assert r.extract_pubkey_list_literal(text, "CATALOG_PUBKEYS") == [pub] + + +def test_extract_pubkey_list_literal_multiple_keys(): + import base64 + + pubs = [r.generate_keypair()[1] for _ in range(2)] + body = ",\n".join(f' base64.b64decode("{base64.b64encode(p).decode()}")' for p in pubs) + text = f"RELEASE_PUBKEYS: list[bytes] = [\n{body},\n]\n" + assert r.extract_pubkey_list_literal(text, "RELEASE_PUBKEYS") == pubs + + +def test_extract_pubkey_list_literal_missing_variable_returns_empty(): + assert r.extract_pubkey_list_literal("some unrelated text", "CATALOG_PUBKEYS") == [] + + +def test_extract_pubkey_list_literal_does_not_match_a_different_variable(): + import base64 + + _seed, pub = r.generate_keypair() + text = f'OTHER_PUBKEYS: list[bytes] = [base64.b64decode("{base64.b64encode(pub).decode()}")]\n' + assert r.extract_pubkey_list_literal(text, "CATALOG_PUBKEYS") == [] + + +def test_extract_ci_trust_anchor_pubkey_found(): + import base64 + + _seed, pub = r.generate_keypair() + text = f' EXPECTED_CATALOG_PUBKEY_B64: "{base64.b64encode(pub).decode()}"\n' + assert r.extract_ci_trust_anchor_pubkey(text) == pub + + +def test_extract_ci_trust_anchor_pubkey_missing_returns_none(): + assert r.extract_ci_trust_anchor_pubkey("no anchor here") is None + + +# --------------------------------------------------------------------------- # +# key_status / render_key_status_report / recommend_next_steps +# --------------------------------------------------------------------------- # +def _kw(**overrides): + base = dict( + kind="catalog", + display_name="CATALOG", + purpose="Signs the catalog.", + private_key_location="on this machine", + local_exists=True, + local_pubkey=b"\x01" * 32, + locations=[("bcc_core.CATALOG_PUBKEYS", [b"\x01" * 32])], + catalog_sig_status="valid", + ) + base.update(overrides) + return base + + +def test_key_status_reports_match_when_local_pubkey_in_committed_list(): + status = r.key_status(**_kw()) + assert status.locations[0].status == "match" + + +def test_key_status_reports_mismatch_when_local_pubkey_not_in_committed_list(): + status = r.key_status(**_kw(locations=[("bcc_core.CATALOG_PUBKEYS", [b"\x02" * 32])])) + assert status.locations[0].status == "mismatch" + + +def test_key_status_reports_unknown_when_no_local_pubkey(): + status = r.key_status(**_kw(local_pubkey=None, local_exists=False)) + assert status.locations[0].status == "unknown" + assert status.local_fingerprint is None + + +def test_key_status_never_carries_a_local_fingerprint_when_key_absent(): + status = r.key_status(**_kw(local_pubkey=None, local_exists=False)) + assert status.local_exists is False + assert status.local_fingerprint is None + + +def test_key_status_fingerprint_matches_fingerprint_pubkey_helper(): + pub = b"\x03" * 32 + status = r.key_status(**_kw(local_pubkey=pub, locations=[("x", [pub])])) + assert status.local_fingerprint == r.fingerprint_pubkey(pub) + + +def test_key_status_checks_multiple_locations_independently(): + """A key can match one committed location and mismatch another -- this + is exactly the drift issue #68 finding 4 was about (bcc_core.py and + ci.yml silently disagreeing on the trust anchor).""" + pub = b"\x04" * 32 + other = b"\x05" * 32 + status = r.key_status( + **_kw( + local_pubkey=pub, + locations=[ + ("bcc_core.CATALOG_PUBKEYS", [pub]), + ("ci.yml trust anchor", [other]), + ], + ) + ) + assert status.locations[0].status == "match" + assert status.locations[1].status == "mismatch" + + +def test_recommend_next_steps_flags_never_generated_key(): + status = r.key_status(**_kw(local_exists=False, local_pubkey=None, locations=[])) + steps = r.recommend_next_steps([status]) + assert any("keygen" in s and "CATALOG" in s for s in steps) + + +def test_recommend_next_steps_release_key_uses_release_flag(): + status = r.key_status( + kind="release", + display_name="RELEASE", + purpose="Signs checksums.", + private_key_location="not generated yet", + local_exists=False, + local_pubkey=None, + locations=[], + ) + steps = r.recommend_next_steps([status]) + assert any("keygen --release" in s for s in steps) + + +def test_recommend_next_steps_flags_mismatch_by_location_name(): + status = r.key_status(**_kw(locations=[("bcc_core.CATALOG_PUBKEYS", [b"\x99" * 32])])) + steps = r.recommend_next_steps([status]) + assert any("bcc_core.CATALOG_PUBKEYS" in s and "CATALOG" in s for s in steps) + + +def test_recommend_next_steps_flags_invalid_catalog_signature(): + status = r.key_status(**_kw(catalog_sig_status="invalid")) + steps = r.recommend_next_steps([status]) + assert any("re-signing" in s for s in steps) + + +def test_recommend_next_steps_flags_missing_catalog_signature(): + status = r.key_status(**_kw(catalog_sig_status="missing")) + steps = r.recommend_next_steps([status]) + assert any("re-signing" in s for s in steps) + + +def test_recommend_next_steps_all_clear_when_nothing_wrong(): + status = r.key_status(**_kw()) + steps = r.recommend_next_steps([status]) + assert steps == ["Everything is consistent -- no action needed."] + + +def test_recommend_next_steps_release_key_has_no_catalog_signature_advice(): + """A mismatched RELEASE key must never trigger catalog-signing advice -- + the two keys' remediation paths must not bleed into each other.""" + status = r.key_status( + kind="release", + display_name="RELEASE", + purpose="Signs checksums.", + private_key_location="on this machine", + local_exists=True, + local_pubkey=b"\x06" * 32, + locations=[("scripts/sign_checksums.py RELEASE_PUBKEYS", [b"\x07" * 32])], + catalog_sig_status=None, + ) + steps = r.recommend_next_steps([status]) + assert not any("re-signing" in s for s in steps) + assert any("RELEASE" in s for s in steps) + + +def test_render_key_status_report_never_prints_private_key_material(): + """The report string must be built ONLY from public inputs. Sanity + check: no field on KeyStatus/PubkeyLocationCheck is capable of holding + private key bytes in the first place (there's no such field to leak), + and the render function only touches fields that exist -- this test + guards against a future field addition reintroducing that risk.""" + status = r.key_status(**_kw()) + text = r.render_key_status_report([status]) + assert "CATALOG" in text + assert ( + "purpose" not in text.lower() or "Signs the catalog." in text + ) # sanity, not a real secret + # No 64-hex-char (or longer) run anywhere -- a raw 32-byte seed/sig + # would show up as one if it were ever accidentally interpolated in. + import re as _re + + assert not _re.search(r"[0-9a-fA-F]{64,}", text) + + +def test_render_key_status_report_names_the_specific_key_not_generic_the_key(): + status = r.key_status(**_kw()) + text = r.render_key_status_report([status]) + assert "CATALOG KEY" in text + assert "the key" not in text.lower() + + +def test_render_key_status_report_includes_catalog_signature_line_only_for_catalog(): + catalog_status = r.key_status(**_kw()) + release_status = r.key_status( + kind="release", + display_name="RELEASE", + purpose="Signs checksums.", + private_key_location="on this machine", + local_exists=True, + local_pubkey=b"\x08" * 32, + locations=[("scripts/sign_checksums.py RELEASE_PUBKEYS", [b"\x08" * 32])], + catalog_sig_status=None, + ) + text = r.render_key_status_report([catalog_status, release_status]) + assert text.count("Catalog signature:") == 1 + + +def test_render_key_status_report_ends_with_what_to_do_next_section(): + status = r.key_status(**_kw()) + text = r.render_key_status_report([status]) + assert "What to do next:" in text -- 2.52.0