fix(catalog-console): close the vacuous review gate; split catalog/release signing keys
CI / Lint (ruff) (pull_request) Successful in 9s
CI / Tests (py3.10 / ubuntu-latest) (pull_request) Successful in 11s
CI / Tests (py3.12 / windows-latest) (pull_request) Successful in 34s
CI / Tests (py3.12 / ubuntu-latest) (pull_request) Successful in 9s
CI / Tests (py3.13 / ubuntu-latest) (pull_request) Successful in 10s
CI / Catalog signature (pull_request) Successful in 6s
CI / Lint (ruff) (pull_request) Successful in 9s
CI / Tests (py3.10 / ubuntu-latest) (pull_request) Successful in 11s
CI / Tests (py3.12 / windows-latest) (pull_request) Successful in 34s
CI / Tests (py3.12 / ubuntu-latest) (pull_request) Successful in 9s
CI / Tests (py3.13 / ubuntu-latest) (pull_request) Successful in 10s
CI / Catalog signature (pull_request) Successful in 6s
Fixes #68 findings 1 and 5.
Finding 1 -- the review gate signed without reviewing anything:
- ReviewWindow._on_sign hardcoded "main" as the TOCTOU comparison ref, so
any PR review (where _on_load pins the PR head's blob SHA) could never
sign; the only working path was main-vs-itself, whose empty diff made
can_sign() vacuously True (set() <= set()). Commit b08cf21 signed 19
entries through exactly that path with zero of them reviewed.
- can_sign() now refuses an empty changeset outright, and itself checks
has_blocking_risk() across every changed entry rather than trusting the
GUI to have disabled a checkbox.
- ReviewSession now carries loaded_ref (the exact ref reviewed); a new pure
sign_precondition(session, resolve_blob_sha) resolves the TOCTOU SHA from
that ref, never a hardcoded "main". _on_load's retry path re-diffs
instead of re-pinning the same stale SHA, so a blob-mismatch refusal
can't loop forever.
- source="main" now diffs against the last catalog a maintainer actually
SIGNED (walking catalog.json's git history until a version verifies
against the current .sig), not against itself.
- Replaced the theatre-only test_no_acknowledge_all_function_exists (only
asserted no function was *named* acknowledge_all) with a test that also
exercises the real gate. Added can_sign/sign_precondition coverage for
the empty-diff, blocking-risk, and ref-resolution seams -- each verified
to fail when its guard is removed.
Finding 5 -- the catalog key and release key were the same CI-resident key:
- scripts/sign_checksums.py gets its own RELEASE_PUBKEYS (separate from
bcc_core.CATALOG_PUBKEYS) and a verify_checksums_against_any() helper.
- release.yml's signing-smoke-test now verifies RELEASE_SIGNING_KEY against
RELEASE_PUBKEYS only -- it no longer imports bcc_core/CATALOG_PUBKEYS at
all, so this workflow can never compare a CI secret against the
catalog's root of trust.
- catalog_console.py: keygen/show-seed-b64 gain --release, with separate
keychain/file storage per key kind. show-seed-b64 refuses to run without
--release, so the catalog seed can't be exported to a CI secret by habit.
- README documents both keys' trust properties and the asymmetry: a CI
compromise burns the release key, never the catalog key.
The maintainer must rotate the catalog key (it was CI-resident, so treat it
as burned for catalog use) and generate a fresh release key -- see the PR
description for the exact steps. No key is generated or committed here.
This commit is contained in:
+97
-6
@@ -24,6 +24,7 @@ from urllib.parse import urlsplit
|
||||
|
||||
from bcc_core import _CATALOG_SIG_DOMAIN as CATALOG_SIG_DOMAIN
|
||||
from bcc_core import CATALOG_ALLOWED_COMMANDS
|
||||
from bcc_core import verify_catalog_signature as _verify_catalog_signature
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
# Semantic diff
|
||||
@@ -432,11 +433,21 @@ def has_blocking_risk(change: EntryChange) -> bool:
|
||||
class ReviewSession:
|
||||
"""State for one review pass. `pinned_blob_sha` is the git blob SHA of
|
||||
data/catalog.json as it existed the moment review began -- see
|
||||
can_sign()."""
|
||||
can_sign()/sign_precondition().
|
||||
|
||||
`loaded_ref` is the exact ref this review was loaded from ("main", or a
|
||||
PR's `refs/pull/<n>/head`) -- see issue #68 finding 1. It exists so the
|
||||
Sign path can re-resolve the TOCTOU blob SHA from *the ref that was
|
||||
actually reviewed*, instead of a hardcoded "main" that silently diverges
|
||||
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).
|
||||
"""
|
||||
|
||||
pinned_blob_sha: str
|
||||
old_catalog: dict
|
||||
new_catalog: dict
|
||||
loaded_ref: str = "main"
|
||||
changes: list[EntryChange] = field(default_factory=list)
|
||||
acknowledged: set[str] = field(default_factory=set)
|
||||
|
||||
@@ -445,12 +456,39 @@ class ReviewSession:
|
||||
self.changes = diff_catalogs(self.old_catalog, self.new_catalog)
|
||||
|
||||
|
||||
def start_review(pinned_blob_sha: str, old_catalog: dict, new_catalog: dict) -> ReviewSession:
|
||||
def start_review(
|
||||
pinned_blob_sha: str,
|
||||
old_catalog: dict,
|
||||
new_catalog: dict,
|
||||
loaded_ref: str = "main",
|
||||
) -> ReviewSession:
|
||||
return ReviewSession(
|
||||
pinned_blob_sha=pinned_blob_sha, old_catalog=old_catalog, new_catalog=new_catalog
|
||||
pinned_blob_sha=pinned_blob_sha,
|
||||
old_catalog=old_catalog,
|
||||
new_catalog=new_catalog,
|
||||
loaded_ref=loaded_ref,
|
||||
)
|
||||
|
||||
|
||||
def find_last_signed_catalog_raw(
|
||||
candidates: list[bytes], sig: bytes, pubkeys: list[bytes]
|
||||
) -> bytes | None:
|
||||
"""Given `candidates` (candidate raw catalog.json byte-strings -- e.g.
|
||||
successive historical versions from git log, most-recent-first),
|
||||
return the first one whose signature verifies against `sig`/`pubkeys`,
|
||||
or None if none do.
|
||||
|
||||
This is how source="main" review diffs against "the last catalog a
|
||||
maintainer actually signed" instead of against itself (issue #68
|
||||
finding 1): `catalog_console.last_signed_catalog_raw` walks
|
||||
data/catalog.json's git history on main and hands the candidates here.
|
||||
"""
|
||||
for raw in candidates:
|
||||
if _verify_catalog_signature(raw, sig, pubkeys):
|
||||
return raw
|
||||
return None
|
||||
|
||||
|
||||
def acknowledge_entry(session: ReviewSession, entry_id: str) -> None:
|
||||
ids = {c.entry_id for c in session.changes}
|
||||
if entry_id not in ids:
|
||||
@@ -483,22 +521,53 @@ class SignDecision:
|
||||
def can_sign(session: ReviewSession, current_blob_sha: str) -> SignDecision:
|
||||
"""Whether the Sign button may fire right now.
|
||||
|
||||
Two independent gates, both required:
|
||||
1. TOCTOU: `current_blob_sha` (fetched fresh, immediately before signing)
|
||||
Four independent gates, all required, checked in this order:
|
||||
|
||||
0. The diff must be non-empty. An empty diff historically meant "Sign
|
||||
unlocks instantly" (`set() <= set()` is vacuously True), which is
|
||||
exactly backwards: a vacuously-satisfied gate is worse than no gate
|
||||
at all, because it *manufactures confidence* -- the signature looks
|
||||
identical to one produced by a real review. "Nothing changed" must
|
||||
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.)
|
||||
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
|
||||
remote changed since -- a new commit pushed to the same PR, a
|
||||
force-push, another PR merged in between -- signing is refused and a
|
||||
re-review is forced. This is what makes "signing is the approval act"
|
||||
true rather than aspirational: the signature is bound to the exact
|
||||
reviewed bytes, not to "whatever the file happens to be now".
|
||||
2. Every changed entry in the diff must be individually acknowledged.
|
||||
2. No blocking risk finding may be outstanding on ANY changed entry, full
|
||||
stop -- checked here, not just in the GUI. The GUI additionally
|
||||
disables the acknowledge checkbox for a blocking entry, but that is a
|
||||
UI nicety, not the enforcement point: if this pure gate didn't also
|
||||
check it, a blocking risk would only be stopped by the GUI happening
|
||||
to have wired the checkbox correctly, and nothing would catch a
|
||||
regression in that wiring. The GUI must not be the only thing
|
||||
standing between a blocking risk and a signature.
|
||||
3. Every changed entry in the diff must be individually acknowledged.
|
||||
"""
|
||||
if not session.changes:
|
||||
return SignDecision(
|
||||
False,
|
||||
"Nothing to sign: this review's diff is empty. If you expected "
|
||||
"changes here, you may be diffing the wrong source/ref.",
|
||||
)
|
||||
if current_blob_sha != session.pinned_blob_sha:
|
||||
return SignDecision(
|
||||
False,
|
||||
"The reviewed bytes changed since this review began (blob SHA "
|
||||
"mismatch) -- re-review required before signing.",
|
||||
)
|
||||
blocking_ids = sorted({c.entry_id for c in session.changes if has_blocking_risk(c)})
|
||||
if blocking_ids:
|
||||
return SignDecision(
|
||||
False,
|
||||
"Blocking risk finding(s) outstanding on: "
|
||||
f"{', '.join(blocking_ids)} -- fix the underlying change, do not sign around it.",
|
||||
)
|
||||
if not all_entries_acknowledged(session):
|
||||
pending = sorted({c.entry_id for c in session.changes} - session.acknowledged)
|
||||
return SignDecision(
|
||||
@@ -507,6 +576,28 @@ def can_sign(session: ReviewSession, current_blob_sha: str) -> SignDecision:
|
||||
return SignDecision(True, None)
|
||||
|
||||
|
||||
def sign_precondition(
|
||||
session: ReviewSession, resolve_blob_sha: Callable[[str], str]
|
||||
) -> SignDecision:
|
||||
"""The real Sign-button gate: resolves the current TOCTOU blob SHA from
|
||||
*the ref this session was actually loaded from* (`session.loaded_ref`),
|
||||
never a hardcoded "main", then delegates to can_sign().
|
||||
|
||||
`resolve_blob_sha` is injected so this stays testable without git/Qt --
|
||||
catalog_console.ReviewWindow._on_sign passes a real resolver
|
||||
(fetch_ref + blob_sha_at against self.repo_dir); tests pass a fake
|
||||
dict-backed lookup. This is the fix for issue #68 finding 1's first bug:
|
||||
`_on_sign` used to hardcode `fetch_ref(self.repo_dir, "main")` as the
|
||||
comparison ref, so for any PR review (where `loaded_ref` is the PR's
|
||||
head, not main) the SHAs differed by definition and Sign could never
|
||||
fire -- and the retry path re-called the same hardcoded resolver, so it
|
||||
re-pinned the same wrong value and looped forever instead of forcing a
|
||||
genuine re-review.
|
||||
"""
|
||||
current_blob_sha = resolve_blob_sha(session.loaded_ref)
|
||||
return can_sign(session, current_blob_sha)
|
||||
|
||||
|
||||
def catalog_signing_message(raw_bytes: bytes) -> bytes:
|
||||
"""The exact bytes that get signed: bcc_core's domain-separation prefix
|
||||
(imported, never retyped) + the raw catalog bytes. Using this function
|
||||
|
||||
Reference in New Issue
Block a user