diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ba3ab86..b2cd6c4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -95,12 +95,39 @@ jobs: - name: Install dependencies run: pip install cryptography + # ๐Ÿ”ด TRUST ANCHOR โ€” issue #68 finding 4. + # + # This step used to do `import bcc_core as c` FROM THE CHECKED-OUT PR + # BRANCH and verify the catalog against c.CATALOG_PUBKEYS โ€” i.e. it + # trusted the public key shipped in the very diff it was reviewing. A + # PR that changed data/catalog.json AND bcc_core.CATALOG_PUBKEYS (to + # an attacker key, with a matching signature produced by the attacker's + # matching private key) went green, because there was nothing outside + # the PR's own content to check the key against. The gate's whole + # point is catching a friendly-looking PR the maintainer merges + # without really reading it โ€” and that hole made it a two-file diff. + # + # EXPECTED_CATALOG_PUBKEY_B64 below is hardcoded HERE, in the workflow + # file, independent of whatever bcc_core.py says on the PR branch. It + # is intentionally the only line in this step that matters for + # security review: changing it changes what this gate is willing to + # trust. THIS CONSTANT IS A TRUST ANCHOR. A PR that changes this line + # in the same diff as a catalog change is exactly the attack this gate + # exists to prevent โ€” review a change to this line on its own, + # never bundled with a catalog update. + # + # NOTE for the next key rotation: update EXPECTED_CATALOG_PUBKEY_B64 + # 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=" run: | python - <<'PY' - import pathlib, sys + import base64, os, pathlib, sys import bcc_core as c + expected_pubkey_b64 = os.environ["EXPECTED_CATALOG_PUBKEY_B64"] + raw = pathlib.Path("data/catalog.json").read_bytes() sig_path = pathlib.Path("data/catalog.json.sig") @@ -111,6 +138,26 @@ jobs: if b"\x00" * 32 in c.CATALOG_PUBKEYS: sys.exit("FAIL: CATALOG_PUBKEYS still holds the placeholder key.") + # Trust anchor check FIRST, before verifying anything against + # bcc_core.CATALOG_PUBKEYS: a PR is not allowed to bring its own + # key. CATALOG_PUBKEYS on the checked-out branch must be EXACTLY + # the key(s) this workflow file itself expects -- no more, no + # fewer, no substitutions. + actual_pubkeys_b64 = [base64.b64encode(k).decode() for k in c.CATALOG_PUBKEYS] + if actual_pubkeys_b64 != [expected_pubkey_b64]: + sys.exit( + "FAIL: bcc_core.CATALOG_PUBKEYS on this branch does not match the " + "trust anchor hardcoded in .github/workflows/ci.yml.\n" + f" expected: {[expected_pubkey_b64]}\n" + f" actual: {actual_pubkeys_b64}\n" + "\n" + "This PR is changing (or has changed) the catalog signing key. That " + "change must be reviewed on its own, separately from any catalog " + "content change, and the workflow's EXPECTED_CATALOG_PUBKEY_B64 " + "updated deliberately -- not accepted because it happened to match " + "whatever bcc_core.py says on this branch." + ) + if not c.verify_catalog_signature(raw, sig_path.read_bytes(), c.CATALOG_PUBKEYS): sys.exit( "FAIL: data/catalog.json does NOT match its signature.\n" @@ -124,5 +171,6 @@ jobs: if problems: sys.exit("FAIL: catalog failed validation:\n " + "\n ".join(problems)) - print("OK: catalog signature verifies and the catalog validates clean.") + print("OK: catalog signature verifies, the pubkey matches the CI trust anchor, " + "and the catalog validates clean.") PY diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 616ed94..a65ba33 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -101,9 +101,20 @@ jobs: # signing step โ€” which means a wrong/missing RELEASE_SIGNING_KEY secret # would only be discovered at the worst possible moment: during a real # release. This job signs a throwaway manifest with the secret and verifies - # the result against the PUBLIC key already compiled into bcc_core. + # the result against scripts/sign_checksums.RELEASE_PUBKEYS. # - # It proves the two halves of the keypair actually match, without + # IMPORTANT (issue #68 finding 5): this must verify against the RELEASE + # public key, never bcc_core.CATALOG_PUBKEYS. The catalog key is the + # offline, maintainer-held root of trust for what BCC executes; it must + # NEVER be compared against a value that lives in a CI secret, because + # that comparison is itself a way to smuggle a catalog-trusted key through + # CI review ("does this repo secret match the catalog key" is a question + # this workflow must never even ask). The release key is a SEPARATE + # keypair, generated via `catalog_console.py keygen --release`, that only + # ever signs release SHA256SUMS manifests -- a CI/secret compromise burns + # this key, not the catalog key. + # + # It proves the two halves of the RELEASE keypair actually match, without # publishing anything. Run it from the Actions tab after setting or # rotating the secret. signing-smoke-test: @@ -120,42 +131,54 @@ jobs: - name: Install dependencies run: pip install cryptography - - name: Sign a throwaway manifest and verify against the shipped pubkey + - name: Sign a throwaway manifest and verify against the RELEASE pubkey env: RELEASE_SIGNING_KEY: ${{ secrets.RELEASE_SIGNING_KEY }} run: | if [ -z "$RELEASE_SIGNING_KEY" ]; then echo "FAIL: RELEASE_SIGNING_KEY secret is not set." - echo "Generate it with: python catalog_console.py show-seed-b64" - echo "then add it under Settings -> Actions -> Secrets." + echo "Generate the RELEASE key (NOT the catalog key) with:" + echo " python catalog_console.py keygen --release" + echo "then add its seed under Settings -> Actions -> Secrets, via:" + echo " python catalog_console.py show-seed-b64 --release" exit 1 fi mkdir -p smoke && echo "smoke test payload" > smoke/hello.txt python3 scripts/sign_checksums.py generate smoke --out smoke/SHA256SUMS python3 scripts/sign_checksums.py sign --sums smoke/SHA256SUMS --out smoke/SHA256SUMS.sig python - <<'PY' - import base64, pathlib, sys - import bcc_core as c - from scripts.sign_checksums import verify_checksums + import pathlib, sys + from scripts.sign_checksums import RELEASE_PUBKEYS, verify_checksums_against_any - # The public half that ships inside the binary. If the secret is a - # DIFFERENT key than the one users' copies trust, this fails here -- - # which is the entire point of the job. - pub_b64 = base64.b64encode(c.CATALOG_PUBKEYS[0]).decode() + # Deliberately does NOT import bcc_core / CATALOG_PUBKEYS at all -- + # this smoke test must never be able to compare the CI secret + # against the catalog's root of trust (issue #68 finding 5). Only + # RELEASE_PUBKEYS (scripts/sign_checksums.py) is a legitimate + # target for a CI-resident key. + if not RELEASE_PUBKEYS: + sys.exit( + "FAIL: scripts/sign_checksums.RELEASE_PUBKEYS is empty.\n" + "\n" + "Generate the release keypair with:\n" + " python catalog_console.py keygen --release\n" + "then paste the printed public key into RELEASE_PUBKEYS in\n" + "scripts/sign_checksums.py and commit that change." + ) sums = pathlib.Path("smoke/SHA256SUMS").read_text() sig = pathlib.Path("smoke/SHA256SUMS.sig").read_bytes() - if not verify_checksums(pub_b64, sums, sig): + if not verify_checksums_against_any(RELEASE_PUBKEYS, sums, sig): sys.exit( "FAIL: the signature produced by RELEASE_SIGNING_KEY does NOT verify\n" - "against the public key in bcc_core.CATALOG_PUBKEYS.\n" + "against any key in scripts/sign_checksums.RELEASE_PUBKEYS.\n" "\n" - "The secret and the shipped public key are different keypairs. Users\n" - "would reject every signature this CI produces. Re-copy the seed from\n" - "`catalog_console.py show-seed-b64`, or update CATALOG_PUBKEYS." + "The secret and the shipped release public key are different keypairs.\n" + "Downloaders would reject every signature this CI produces. Re-copy the\n" + "seed from `catalog_console.py show-seed-b64 --release`, or update\n" + "RELEASE_PUBKEYS with the matching public key." ) - print("OK: RELEASE_SIGNING_KEY matches the public key shipped in bcc_core.") + print("OK: RELEASE_SIGNING_KEY matches a key in RELEASE_PUBKEYS.") PY # โ”€โ”€ Create GitHub Release with all three artifacts โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€ @@ -206,9 +229,13 @@ jobs: # checks. It does NOT remove Gatekeeper/SmartScreen warnings. # # The private key is a repo secret (RELEASE_SIGNING_KEY, base64 raw - # Ed25519 seed) generated via the Catalog Console (#62). If it's not - # set, we still publish the release โ€” just without a .sig โ€” rather - # than fail the release outright. + # Ed25519 seed) for the RELEASE key -- a SEPARATE keypair from the + # catalog key, generated via `python catalog_console.py keygen + # --release` (issue #68 finding 5; #62). This key is intentionally + # CI-resident and signs ONLY this checksum manifest; it is never + # trusted to sign data/catalog.json. If it's not set, we still + # publish the release โ€” just without a .sig โ€” rather than fail the + # release outright. - name: Check for signing key id: signing run: | @@ -234,7 +261,7 @@ jobs: - name: Warn โ€” release will be unsigned if: steps.signing.outputs.has_key != 'true' run: | - echo "::warning::RELEASE_SIGNING_KEY secret is not set โ€” this release is being published WITHOUT a signed SHA256SUMS.sig. Add the secret (base64 raw Ed25519 seed, generated via the Catalog Console, #62) before the next tag." + echo "::warning::RELEASE_SIGNING_KEY secret is not set โ€” this release is being published WITHOUT a signed SHA256SUMS.sig. Generate the RELEASE key (python catalog_console.py keygen --release) and add its seed (python catalog_console.py show-seed-b64 --release) as this secret before the next tag." - name: Create GitHub Release uses: softprops/action-gh-release@v2 diff --git a/README.md b/README.md index d28ffd3..c04b894 100644 --- a/README.md +++ b/README.md @@ -36,11 +36,10 @@ are only suppressed by a paid OS-vendor certificate, which this project doesn't have. Verifying checksums is about detecting tampering in transit or on a mirror, not about vouching for the software. -**Release signing public key** (Ed25519, base64, raw 32 bytes): - -``` - -``` +This manifest is signed with BCC's **release key**, which is a different +key from the one that signs the MCP server catalog โ€” see +[Signing keys](#signing-keys) below for why, and for the public key value +to use with `--pubkey-b64` below. ### macOS / Linux @@ -59,7 +58,7 @@ To also verify the manifest's signature (optional, requires Python + ```bash python3 scripts/sign_checksums.py verify \ --sums SHA256SUMS --sig SHA256SUMS.sig \ - --pubkey-b64 "" + --pubkey-b64 "" ``` ### Windows (PowerShell) @@ -78,6 +77,45 @@ release is missing the `.sig` file, the checksums themselves are still valid and safe to check against โ€” the release workflow only skips signing, never checksum generation. +## Signing keys + +BCC uses **two separate Ed25519 keypairs**, deliberately never the same +key, because they protect different things and live in different places: + +| | Catalog key | Release key | +|---|---|---| +| Signs | `data/catalog.json` (the MCP server catalog every user's app trusts) | `SHA256SUMS` (the checksum manifest for release binaries) | +| Verified by | `bcc_core.CATALOG_PUBKEYS` | `scripts/sign_checksums.RELEASE_PUBKEYS` | +| Lives | Offline, passphrase-encrypted, maintainer's machine only (OS keychain or an encrypted file outside the repo โ€” see the [Catalog Console](#files), issue #62) | A Gitea Actions repo secret, `RELEASE_SIGNING_KEY` โ€” **intentionally CI-resident** | +| 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` | + +**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 +and the release-checksum key were the same (as they briefly were โ€” see +[issue #68](../../issues/68)), then anything that can exfiltrate a Gitea +Actions secret (a malicious workflow-file PR, a compromised runner, a leaky +log) could sign a catalog every user's copy of BCC would trust, not just a +checksum manifest. Splitting them means **a CI/secret compromise burns the +release key, never the catalog key** โ€” checksums for a future release could +be forged, which is bad, but no attacker gains the ability to make BCC run +arbitrary commands on installs that trust the catalog. That asymmetry is +the entire point of having two keys instead of one. + +The catalog key is **never** meant to leave the maintainer's machine: it's +generated, stored, unlocked, and used to sign entirely inside the Catalog +Console (`catalog_console.py`), and `catalog_console.py show-seed-b64` +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: + +``` + +``` + ## Run from source ```bash @@ -152,6 +190,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). ## Building from source diff --git a/bcc_core.py b/bcc_core.py index 677faab..38cea9c 100644 --- a/bcc_core.py +++ b/bcc_core.py @@ -2165,6 +2165,30 @@ def restart_claude_desktop() -> RestartResult: # is rejected by validate_catalog() regardless of how plausible it looks. CATALOG_ALLOWED_COMMANDS = frozenset({"npx", "uvx", "docker", "node", "python", "python3"}) +# Env var keys a catalog entry's config.env must never set. Every one of +# these is a loader/interpreter override that lets a value walk straight +# past CATALOG_ALLOWED_COMMANDS and the -e/--eval/-c deny-rule below: e.g. +# NODE_OPTIONS="--require /tmp/x.js" turns an allowlisted `npx` entry into +# arbitrary code execution without ever touching config.args, which is the +# only field the allowlist/deny-rules/ASCII/secret checks used to cover. +# Matched case-insensitively -- env keys are case-sensitive on POSIX, but a +# `node_options` lookalike is exactly the kind of thing this must catch. +CATALOG_DENIED_ENV_KEYS = frozenset( + { + "NODE_OPTIONS", + "PYTHONSTARTUP", + "PYTHONPATH", + "PYTHONHOME", + "LD_PRELOAD", + "LD_LIBRARY_PATH", + "DYLD_INSERT_LIBRARIES", + "DYLD_LIBRARY_PATH", + "BROWSER", + "PATH", + "NODE_REPL_EXTERNAL_MODULE", + } +) + # Ed25519 public keys allowed to sign a catalog, raw 32-byte form. A LIST # (not a single key) so keys can be rotated without bricking installs that # still trust an older key: verify_catalog_signature() accepts a match @@ -2190,6 +2214,36 @@ _CATALOG_SECRET_ARG_RE = re.compile(r"(?i)--api[-_]?key=|--token=|--password=") # -style tokens the GUI must have the user fill in before Save. _PLACEHOLDER_RE = re.compile(r"<[^<>\s]+>") +# A catalog entry's id becomes an mcpServers JSON key AND is interpolated +# into Qt.AutoText widgets (status bar, QMessageBox) -- an id like +# "Verified" renders as markup there. Not RCE, but UI spoofing, so +# ids are constrained to a plain lowercase slug. +_CATALOG_ID_RE = re.compile(r"^[a-z0-9][a-z0-9._-]{0,63}$") + +# Docker flags that consume the next arg as a value (so that value must not +# be mistaken for the image reference when locating it in config.args). +_DOCKER_VALUE_FLAGS = frozenset( + { + "-e", + "--env", + "-v", + "--volume", + "-p", + "--publish", + "--name", + "-w", + "--workdir", + "-u", + "--user", + "--entrypoint", + "--network", + "--platform", + "--add-host", + "-l", + "--label", + } +) + # How many versions a single accepted catalog jump may leap in one go. Bounds # a "freeze" attack: a compromised/leaked signing key claiming an absurd # future version would otherwise permanently outrank every legitimate @@ -2264,6 +2318,121 @@ def _docker_arg_violations(tag: str, args: list[str]) -> list[str]: return problems +def _catalog_package_spec_version(spec: str) -> str | None: + """ + Extract the version pin from an npm-style package spec, or None if the + spec carries no pin. + + Handles unscoped "name@version" and scoped "@scope/name@version" -- + scoped names have a leading "@" that is NOT the version separator, so a + naive split on the first/only "@" misparses "@scope/pkg" (no version) + as pinned to "scope/pkg". Splitting from the right side instead is safe + for both forms because a package name may contain "@" only as the + scope's leading character. + """ + if spec.startswith("@"): + rest = spec[1:] + if "@" not in rest: + return None + _, _, version = rest.rpartition("@") + return version or None + if "@" not in spec: + return None + _, _, version = spec.rpartition("@") + return version or None + + +def _catalog_package_spec_pinned(spec: str) -> bool: + """ + True if `spec` carries an exact version pin. Covers npm's "name@version" + / "@scope/name@version" and uv's documented PyPI pin forms + "name@version" and "name==version". + """ + if "==" in spec: + _, _, version = spec.partition("==") + return bool(version) + return bool(_catalog_package_spec_version(spec)) + + +def _first_catalog_package_spec(args: list[str]) -> str | None: + """ + The first arg that could plausibly BE a package spec: skip flags + (leading "-") and tokens (which can't be validated and + are filled in by the user later, never shipped by the catalog as the + package name itself). Everything after the first hit is ignored -- + trailing flags, paths, and placeholders are not package specs. + """ + for a in args: + if a.startswith("-"): + continue + if _PLACEHOLDER_RE.fullmatch(a): + continue + return a + return None + + +def _catalog_pin_violations(tag: str, command: str, args: list[str]) -> list[str]: + """ + Version-pinning enforcement (finding #3): a catalog PR can otherwise + ship `npx -y @scope/pkg` or `docker run img:latest` and the *next* + resolve of that package/image is whatever the registry serves that day + -- outside review, outside the signature's meaning. This is the only + place that enforces pinning at runtime; catalog_review.py's + risk_unpinned_package() is a maintainer-facing hint, not a gate. + """ + if command in ("npx", "uvx"): + spec = _first_catalog_package_spec(args) + if spec is None: + return [f"{tag}: config.args must include a package spec to pin (e.g. name@1.2.3)."] + if not _catalog_package_spec_pinned(spec): + return [ + f"{tag}: config.args package {spec!r} is not version-pinned; use " + "name@version, @scope/name@version, or name==version." + ] + return [] + + if command == "docker": + image = _docker_image_ref(args) + if image is None: + return [f"{tag}: config.args docker command has no image reference to pin."] + _, sep, image_tag = image.rpartition(":") + if not sep or "/" in image_tag: + return [ + f"{tag}: config.args docker image {image!r} has no explicit tag; " + "pin an exact version (not 'latest', not untagged)." + ] + if image_tag == "latest": + return [ + f"{tag}: config.args docker image {image!r} uses the 'latest' tag, " + "which is not allowed; pin an exact version." + ] + return [] + + return [] + + +def _docker_image_ref(args: list[str]) -> str | None: + """ + Locate the image reference in a `docker run ...` args list: skip the + "run" subcommand and any flags, including ones that consume the next + token as a value (-e, -v, --name, ...) so that value isn't mistaken for + the image. The first remaining positional token is the image. + """ + i = 0 + if i < len(args) and args[i] == "run": + i += 1 + while i < len(args): + a = args[i] + if a.startswith("-"): + if a in _DOCKER_VALUE_FLAGS and "=" not in a: + i += 2 + else: + i += 1 + continue + return a + return None + + def _validate_catalog_config(tag: str, config) -> list[str]: """Validate the `config` block of a basic-tier catalog entry.""" if not isinstance(config, dict): @@ -2284,10 +2453,11 @@ def _validate_catalog_config(tag: str, config) -> list[str]: f"({', '.join(sorted(CATALOG_ALLOWED_COMMANDS))})." ) - args = config.get("args") - if not isinstance(args, list) or not all(isinstance(a, str) for a in args): + raw_args = config.get("args") + args_ok = isinstance(raw_args, list) and all(isinstance(a, str) for a in raw_args) + args = raw_args if args_ok else [] + if not args_ok: problems.append(f"{tag}: config.args must be a list of strings.") - args = [] for a in args: if not a.isascii(): @@ -2306,11 +2476,55 @@ def _validate_catalog_config(tag: str, config) -> list[str]: if command == "docker": problems.extend(_docker_arg_violations(tag, args)) + # Version pinning (finding #3) -- only meaningful once command/args are + # actually well-formed; a malformed args list already got its own + # problem above and has nothing left to pin-check. + if args_ok and command in ("npx", "uvx", "docker"): + problems.extend(_catalog_pin_violations(tag, command, args)) + env = config.get("env") - if env is not None and ( - not isinstance(env, dict) or any(not isinstance(v, str) for v in env.values()) - ): - problems.append(f"{tag}: config.env must be an object of string values.") + if env is not None: + env_ok = isinstance(env, dict) and all( + isinstance(k, str) and isinstance(v, str) for k, v in env.items() + ) + if not env_ok: + problems.append(f"{tag}: config.env must be an object of string values.") + else: + # config.env (finding #2): unlike args, env was previously + # type-checked ONLY -- no allowlist, no deny-rule, no ASCII + # check, no secret check. That made it the single easiest way + # to smuggle a payload past every other guard in this + # function: an allowlisted `command: npx` plus + # NODE_OPTIONS=--require /tmp/x.js in env walks straight past + # the command allowlist AND the -e/--eval/-c deny-rule above, + # because neither of those ever looks at env. + for key, value in env.items(): + if not key.isascii(): + problems.append( + f"{tag}: config.env key {key!r} must be ASCII " + "(non-ASCII code points rejected)." + ) + if key.upper() in CATALOG_DENIED_ENV_KEYS: + problems.append( + f"{tag}: config.env key {key!r} is on the catalog deny-list " + "(interpreter/loader override) and is not allowed." + ) + if not value.isascii(): + problems.append( + f"{tag}: config.env value for {key!r} must be ASCII " + "(non-ASCII code points rejected)." + ) + if _is_secret_value(value): + problems.append( + f"{tag}: config.env[{key!r}] looks like a real secret value; " + "catalog entries must never ship secret values." + ) + if value != "" and not _PLACEHOLDER_RE.fullmatch(value): + problems.append( + f"{tag}: config.env[{key!r}] must be an empty string or a " + "single token -- the catalog declares which env " + "vars a server needs, it never supplies their values." + ) return problems @@ -2330,6 +2544,12 @@ def _validate_catalog_entry(idx: int, entry, seen_ids: set[str]) -> list[str]: tag = f"servers[{idx}] ({entry_id!r})" if not entry_id.isascii(): problems.append(f"{tag}: 'id' must be ASCII (non-ASCII code points rejected).") + elif not _CATALOG_ID_RE.match(entry_id): + problems.append( + f"{tag}: 'id' must match ^[a-z0-9][a-z0-9._-]{{0,63}}$ " + "(it becomes an mcpServers JSON key and is interpolated into " + "Qt.AutoText widgets)." + ) if entry_id in seen_ids: problems.append(f"{tag}: duplicate id.") seen_ids.add(entry_id) @@ -2443,15 +2663,32 @@ def verify_catalog_signature(raw: bytes, sig: bytes, pubkeys: list[bytes]) -> bo return False +def _verify_catalog_candidate(candidate: tuple[bytes, bytes] | None) -> tuple[dict | None, int]: + """Verify+load+validate one (raw, sig) candidate. Returns (None, -1) on any failure.""" + if not candidate: + return None, -1 + raw, sig = candidate + if not verify_catalog_signature(raw, sig, CATALOG_PUBKEYS): + return None, -1 + try: + data = load_catalog(raw) + except (ValueError, TypeError): + return None, -1 + if validate_catalog(data): + return None, -1 + return data, catalog_version(data) + + def resolve_catalog( bundled: tuple[bytes, bytes] | None, cached: tuple[bytes, bytes] | None, remote: tuple[bytes, bytes] | None, + floor: int = 0, ) -> dict: """ - Pick the highest-version catalog among bundled/cached/remote. Each - argument is either None (unavailable) or an (raw_bytes, signature_bytes) - pair. + Pick the highest-version catalog among bundled/cached/remote. Each of + bundled/cached/remote is either None (unavailable) or an (raw_bytes, + signature_bytes) pair. ๐Ÿ”ด SECURITY: every candidate โ€” including `bundled`, the copy frozen into this binary โ€” is verified against CATALOG_PUBKEYS and re-validated from @@ -2462,46 +2699,68 @@ def resolve_catalog( by virtue of being local. Signing (and checking the signature at runtime, every time) closes that. - Anti-rollback: a candidate's version is never accepted if it's lower - than the best verified candidate already found in this same resolution - pass โ€” an attacker replaying an old, since-superseded signed catalog - can't downgrade you. + `floor` is a pure, caller-supplied lower bound (e.g. a persisted + "last accepted version" the GUI can load from disk and pass in) โ€” this + function does no storage of its own. - Anti-freeze: a candidate whose version leaps more than - _CATALOG_MAX_VERSION_JUMP past the current best is also rejected. A - compromised/leaked signing key claiming an absurd future version would - otherwise permanently outrank every legitimate release from then on, - since the resolver always prefers the highest verified version โ€” this - caps how far a single accepted jump can go. + Anti-rollback / anti-freeze, and WHY they apply to every candidate + including the first one evaluated: the previous version of this + function only ran these checks `if best_version >= 0`, i.e. once a + candidate had already been accepted in this pass. That let the FIRST + verified candidate through unconditionally โ€” a signed catalog claiming + version=999999999 sailed straight past both guards if it happened to be + evaluated first, and rollback protection reset on every call anyway + (nothing persisted across restarts). Now both guards are anchored to + something that doesn't depend on iteration order: + + - The anti-freeze cap is measured against the BUNDLED catalog's version + (verified independently, once), not against "whatever was accepted + so far in this loop." Bundled ships inside the binary, so it's the + one candidate that isn't attacker-supplied at resolve time โ€” the + natural trust anchor. If bundled itself doesn't verify, `floor` is + the anchor instead. + - The anti-rollback floor is max(floor, bundled's version), so a + caller that persists `floor` across restarts gets real rollback + protection; a caller that doesn't still gets "never below bundled." + + On a version TIE, the bundled candidate wins over cached/remote (it + previously lost ties to whichever candidate happened to be evaluated + last, silently preferring remote over bundled at equal version). Returns the winning catalog dict, or {} if nothing verified and validated. """ + bundled_data, bundled_version = _verify_catalog_candidate(bundled) + + anchor = bundled_version if bundled_version >= 0 else floor + min_accepted = max(floor, bundled_version if bundled_version >= 0 else 0) + + candidates = ( + ("bundled", bundled_data, bundled_version), + ("cached", *_verify_catalog_candidate(cached)), + ("remote", *_verify_catalog_candidate(remote)), + ) + best: dict = {} best_version = -1 + best_is_bundled = False - for candidate in (bundled, cached, remote): - if not candidate: - continue - raw, sig = candidate - if not verify_catalog_signature(raw, sig, CATALOG_PUBKEYS): - continue - try: - data = load_catalog(raw) - except (ValueError, TypeError): - continue - if validate_catalog(data): + for source, data, version in candidates: + if data is None: continue + if version < min_accepted: + continue # anti-rollback / below the persisted floor + if version > anchor + _CATALOG_MAX_VERSION_JUMP: + continue # anti-freeze, capped against the bundled trust anchor - version = catalog_version(data) - if best_version >= 0: - if version < best_version: - continue # anti-rollback - if version > best_version + _CATALOG_MAX_VERSION_JUMP: - continue # anti-freeze - - best = data - best_version = version + is_bundled = source == "bundled" + better = version > best_version or ( + version == best_version and is_bundled and not best_is_bundled + ) + if better: + best = data + best_version = version + best_is_bundled = is_bundled return best diff --git a/catalog_console.py b/catalog_console.py index 0438379..736d67d 100644 --- a/catalog_console.py +++ b/catalog_console.py @@ -12,7 +12,12 @@ 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 for the rest of this review pass. + 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". 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 @@ -25,20 +30,34 @@ Flow: Load -> Review -> Sign. changed entry must be individually acknowledged (its checkbox ticked) before Sign unlocks. There is no "acknowledge all" -- see catalog_review.py. - 3. Sign -- re-fetches the current blob SHA and refuses to sign unless - it still matches the pinned SHA from step 1 (TOCTOU fix: - catalog_review.can_sign). On success, writes + 3. Sign -- re-resolves the current blob SHA from the SAME ref that was + reviewed (never a hardcoded "main") and refuses to sign + unless it still matches the pinned SHA from step 1, the diff + is non-empty, and no changed entry has an outstanding + 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. + between a catalog merge and its signature. 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. + +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 +CI-resident and signs ONLY the release SHA256SUMS manifest +(scripts/sign_checksums.py) -- `show-seed-b64 --release` is the only +supported way to get a seed out of this tool, and it refuses to run without +`--release` so the catalog seed can never be exported by habit. See the +README's "Signing keys" section. """ from __future__ import annotations import argparse +import base64 import contextlib import getpass import html @@ -69,8 +88,28 @@ SIG_PATH = "data/catalog.json.sig" # maintainer-only tool, so a dotfile under $HOME is an acceptable fallback # when the OS keychain isn't available -- the blob stored there is always # passphrase-encrypted (see catalog_review.encrypt_private_key), never raw. +# +# Two SEPARATE keys are stored here, never conflated (issue #68 finding 5): +# "catalog" -- offline, Console-only. Verified by bcc_core.CATALOG_PUBKEYS. +# Roots of trust for every catalog entry BCC ships. Must +# NEVER leave this machine, never touch CI, never become an +# env var or a repo secret. +# "release" -- CI-resident. Verified by scripts.sign_checksums. +# RELEASE_PUBKEYS. Signs ONLY the release SHA256SUMS +# manifest. Its private seed is deliberately meant to be +# pasted into the RELEASE_SIGNING_KEY Gitea Actions secret +# (via `show-seed-b64 --release`) -- that is its normal, +# intended flow. A CI compromise burns this key, not the +# catalog key: that asymmetry is the whole point of having +# two keys instead of one. KEY_STORAGE_DIR = Path.home() / ".bcc-catalog-console" -KEY_STORAGE_FILE = KEY_STORAGE_DIR / "signing_key.enc" +_KEY_KINDS = ("catalog", "release") + + +def _key_storage_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}.enc" + HTTP_TIMEOUT = 6.0 @@ -97,51 +136,64 @@ def _keyring_module(): _KEYRING_SERVICE = "bcc-catalog-console" -_KEYRING_USERNAME = "signing-key" -def store_encrypted_key(blob: bytes) -> str: +def _keyring_username(kind: str) -> str: + assert kind in _KEY_KINDS, f"unknown key kind {kind!r}, expected one of {_KEY_KINDS}" + return f"signing-key-{kind}" + + +def store_encrypted_key(blob: bytes, kind: str = "catalog") -> str: """Persist an already-encrypted key blob (see - catalog_review.encrypt_private_key). Prefers the OS keychain; falls back - to a file under KEY_STORAGE_DIR (outside the repo) with restrictive + catalog_review.encrypt_private_key) under the given `kind` + ("catalog" or "release" -- see the KEY_STORAGE_DIR comment above; the + two are stored under different keychain entries / filenames so they can + never be loaded interchangeably). Prefers the OS keychain; falls back to + a file under KEY_STORAGE_DIR (outside the repo) with restrictive permissions. Returns a human-readable description of where it went.""" keyring = _keyring_module() if keyring is not None: try: - keyring.set_password(_KEYRING_SERVICE, _KEYRING_USERNAME, blob.hex()) + keyring.set_password(_KEYRING_SERVICE, _keyring_username(kind), blob.hex()) return "OS keychain (via the `keyring` package)" except Exception: pass # fall through to the file-based path KEY_STORAGE_DIR.mkdir(parents=True, exist_ok=True) - KEY_STORAGE_FILE.write_bytes(blob) + key_file = _key_storage_file(kind) + key_file.write_bytes(blob) with contextlib.suppress(OSError): # best-effort on platforms without POSIX perm bits - KEY_STORAGE_FILE.chmod(0o600) - return f"encrypted file at {KEY_STORAGE_FILE}" + key_file.chmod(0o600) + return f"encrypted file at {key_file}" -def load_encrypted_key() -> bytes: - """Load the encrypted key blob from wherever store_encrypted_key() put - it. Raises FileNotFoundError if no key has been generated yet.""" +def load_encrypted_key(kind: str = "catalog") -> bytes: + """Load the encrypted key blob of the given `kind` from wherever + store_encrypted_key() put it. Raises FileNotFoundError if no key of that + kind has been generated yet.""" keyring = _keyring_module() if keyring is not None: try: - hex_blob = keyring.get_password(_KEYRING_SERVICE, _KEYRING_USERNAME) + hex_blob = keyring.get_password(_KEYRING_SERVICE, _keyring_username(kind)) if hex_blob: return bytes.fromhex(hex_blob) except Exception: pass - if not KEY_STORAGE_FILE.exists(): + key_file = _key_storage_file(kind) + if not key_file.exists(): + flag = " --release" if kind == "release" else "" raise FileNotFoundError( - f"No signing key found (checked the OS keychain and {KEY_STORAGE_FILE}). " - "Run `python catalog_console.py keygen` first." + f"No {kind} signing key found (checked the OS keychain and {key_file}). " + f"Run `python catalog_console.py keygen{flag}` first." ) - return KEY_STORAGE_FILE.read_bytes() + return key_file.read_bytes() -def unlock_signing_key(passphrase: str) -> bytes: - """Load + decrypt the signing key seed. Raises ValueError on a wrong - passphrase, FileNotFoundError if no key exists yet.""" - blob = load_encrypted_key() +def unlock_signing_key(passphrase: str, kind: str = "catalog") -> bytes: + """Load + decrypt the signing key seed of the given `kind`. Raises + ValueError on a wrong passphrase, FileNotFoundError if no key of that + kind exists yet. Defaults to "catalog" because that's the key + ReviewWindow._on_sign uses -- the GUI never touches the release key.""" + blob = load_encrypted_key(kind) return review.decrypt_private_key(blob, passphrase) @@ -188,6 +240,49 @@ def read_catalog_at_commit(repo_dir: Path, commit: str) -> tuple[bytes, str]: return blob_bytes(repo_dir, sha), sha +def catalog_blob_history(repo_dir: Path, ref: str, limit: int = 200) -> list[str]: + """Blob SHAs of CATALOG_PATH at each commit that touched it, walking + back from `ref`, most-recent-first. Used by last_signed_catalog_raw() to + find "the version of the catalog the current signature actually covers" + without assuming it's the tip commit.""" + log_out = _git(repo_dir, "log", f"--max-count={limit}", "--format=%H", ref, "--", CATALOG_PATH) + commits = [line for line in log_out.splitlines() if line] + shas: list[str] = [] + for commit in commits: + try: + shas.append(blob_sha_at(repo_dir, commit, CATALOG_PATH)) + except GitError: + continue + return shas + + +def last_signed_catalog_raw(repo_dir: Path, commit: str) -> bytes | None: + """The raw data/catalog.json bytes that verify against + data/catalog.json.sig as of `commit` -- i.e. "the last catalog a + maintainer actually signed", found by walking the catalog's git history + on that ref until a version verifies against the *current* signature. + + This is what source="main (current tip)" diffs against (issue #68 + finding 1): if main's tip catalog.json already matches its .sig, this + returns that same content and the diff is correctly empty (nothing new + to sign). If someone merged a catalog change to main without running it + through the Console -- the exact bypass that produced commit b08cf21 -- + the signature still covers the OLDER content, so this returns that older + version and the diff surfaces exactly what was never actually reviewed. + + Returns None if there's no signature yet, or none of the recent history + verifies against it (caller should treat this as "diff against nothing + ever signed", i.e. every current entry shows as newly added). + """ + try: + sig_sha = blob_sha_at(repo_dir, commit, SIG_PATH) + except GitError: + return None + sig_bytes = blob_bytes(repo_dir, sig_sha) + candidates = [blob_bytes(repo_dir, sha) for sha in catalog_blob_history(repo_dir, commit)] + return review.find_last_signed_catalog_raw(candidates, sig_bytes, core.CATALOG_PUBKEYS) + + def commit_and_push_signed_catalog( repo_dir: Path, raw_bytes: bytes, signature: bytes, *, branch: str = "main" ) -> str: @@ -644,28 +739,50 @@ class ReviewWindow(QMainWindow): row = self.source_list.currentRow() try: if row <= 0: - commit = fetch_ref(self.repo_dir, "main") - old_commit = None # main vs itself has no "old" -- nothing to diff without a base + # 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) + 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] - commit = fetch_ref(self.repo_dir, pr.head_ref) + loaded_ref = pr.head_ref + 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) - - if old_commit: + 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) - else: - old_catalog = new_catalog except (GitError, ValueError) as e: QMessageBox.critical(self, "Load failed", html.escape(str(e))) return self._new_raw = new_raw - self.session = review.start_review(new_blob_sha, old_catalog, new_catalog) + # `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 + ) self._render_cards() def _render_cards(self): @@ -706,20 +823,43 @@ class ReviewWindow(QMainWindow): def _on_sign(self): assert self.session is not None + + 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) + try: - current_sha = blob_sha_at(self.repo_dir, fetch_ref(self.repo_dir, "main"), CATALOG_PATH) + decision = review.sign_precondition(self.session, _resolve_blob_sha) except GitError as e: QMessageBox.critical(self, "Sign failed", html.escape(str(e))) return - decision = review.can_sign(self.session, current_sha) if not decision.ok: QMessageBox.warning(self, "Cannot sign", html.escape(decision.reason or "")) - if decision.reason and "changed" in decision.reason.lower(): - self._on_load() # force a re-review against the new bytes + # 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 - dialog = PassphraseDialog("Enter signing key passphrase:", self) + dialog = PassphraseDialog("Enter CATALOG signing key passphrase:", self) if dialog.exec() != QDialog.DialogCode.Accepted: return try: @@ -744,9 +884,17 @@ class ReviewWindow(QMainWindow): # --------------------------------------------------------------------------- # -def cmd_keygen(_args: argparse.Namespace) -> int: +def cmd_keygen(args: argparse.Namespace) -> int: + # Two SEPARATE keypairs, never conflated (issue #68 finding 5): the + # catalog key is the offline root of trust for what BCC executes and + # must never touch CI; the release key is CI-resident and signs ONLY + # the release SHA256SUMS manifest. Which one this run generates is + # explicit via --release, and the printed instructions differ sharply + # so it's obvious which key is safe to paste into a CI secret (release) + # and which one never is (catalog). + kind = "release" if getattr(args, "release", False) else "catalog" seed, pubkey = review.generate_keypair() - passphrase = getpass.getpass("Choose a passphrase to encrypt the new signing key: ") + passphrase = getpass.getpass(f"Choose a passphrase to encrypt the new {kind} signing key: ") confirm = getpass.getpass("Confirm passphrase: ") if passphrase != confirm: print("error: passphrases did not match", file=sys.stderr) @@ -756,31 +904,67 @@ def cmd_keygen(_args: argparse.Namespace) -> int: return 1 blob = review.encrypt_private_key(seed, passphrase) - where = store_encrypted_key(blob) - pubkey_b64 = __import__("base64").b64encode(pubkey).decode("ascii") + where = store_encrypted_key(blob, kind=kind) + pubkey_b64 = base64.b64encode(pubkey).decode("ascii") - print(f"Private key encrypted and stored in: {where}") + print(f"{kind.capitalize()} private key encrypted and stored in: {where}") print() - print("Public key (base64, paste into bcc_core.CATALOG_PUBKEYS):") - print(f" {pubkey_b64}") - print() - print( - "Also add it as the Gitea repo secret RELEASE_SIGNING_KEY (base64 of the " - "32-byte private seed) used by release.yml -- get that value with:" - ) - print(" python catalog_console.py show-seed-b64 # careful: prints the raw key") + if kind == "catalog": + print( + "This is the CATALOG key. It is the root of trust for every catalog " + "entry BCC ships -- it must stay offline and Console-only. NEVER paste " + "it, its seed, or `show-seed-b64` output into CI, an env var, or a repo " + "secret. (If the key currently in bcc_core.CATALOG_PUBKEYS has ever been " + "pasted into a CI secret, treat it as burned for catalog use -- generate " + "a fresh one with this command and rotate.)" + ) + 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):" + ) + print(f" {pubkey_b64}") + else: + print( + "This is the RELEASE key. It signs ONLY the release SHA256SUMS " + "manifest in CI -- it is intentionally CI-resident and is NOT trusted " + "to sign the catalog (bcc_core.CATALOG_PUBKEYS does not and must not " + "contain it)." + ) + print() + print("1. Public key (base64) -- 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:" + ) + print(" python catalog_console.py show-seed-b64 --release # prints the raw key") return 0 -def cmd_show_seed_b64(_args: argparse.Namespace) -> int: - passphrase = getpass.getpass("Signing key passphrase: ") +def cmd_show_seed_b64(args: argparse.Namespace) -> int: + # Deliberately requires --release: this command's whole purpose is to + # produce a value that gets pasted into a CI secret, and the catalog key + # must NEVER be pasted into CI (issue #68 finding 5 -- that is exactly + # how the catalog key ended up burned in the first place). Refusing to + # run without --release makes "export the catalog seed for CI" a + # structurally different, more deliberate action than a typo away. + if not getattr(args, "release", False): + print( + "error: show-seed-b64 only ever exports the RELEASE key -- it is the " + "only key allowed to leave this machine, for the RELEASE_SIGNING_KEY CI " + "secret. Re-run as `show-seed-b64 --release`. The catalog key must never " + "be exported this way; see issue #68 finding 5.", + file=sys.stderr, + ) + return 1 + passphrase = getpass.getpass("Release signing key passphrase: ") try: - seed = unlock_signing_key(passphrase) + seed = unlock_signing_key(passphrase, kind="release") except (FileNotFoundError, ValueError) as e: print(f"error: {e}", file=sys.stderr) return 1 - import base64 - print(base64.b64encode(seed).decode("ascii")) return 0 @@ -810,11 +994,28 @@ def build_parser() -> argparse.ArgumentParser: p_gui.add_argument("--repo", default=".", help="path to a BCC git checkout (default: cwd)") p_gui.set_defaults(func=cmd_gui) - p_keygen = sub.add_parser("keygen", help="generate a new Ed25519 signing keypair") + p_keygen = sub.add_parser( + "keygen", help="generate a new Ed25519 signing keypair (catalog key by default)" + ) + p_keygen.add_argument( + "--release", + action="store_true", + help=( + "generate the RELEASE key (CI-resident, signs SHA256SUMS only) instead " + "of the CATALOG key (offline, Console-only, signs data/catalog.json -- " + "see issue #68 finding 5)" + ), + ) p_keygen.set_defaults(func=cmd_keygen) p_seed = sub.add_parser( - "show-seed-b64", help="print the base64 private seed (for the RELEASE_SIGNING_KEY secret)" + "show-seed-b64", + help="print a base64 private seed for a CI secret -- RELEASE key only", + ) + p_seed.add_argument( + "--release", + action="store_true", + help="required: only the release key may ever be exported this way", ) p_seed.set_defaults(func=cmd_show_seed_b64) diff --git a/catalog_review.py b/catalog_review.py index 6edf044..c3c5669 100644 --- a/catalog_review.py +++ b/catalog_review.py @@ -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//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 diff --git a/scripts/sign_checksums.py b/scripts/sign_checksums.py index 25b6cb7..d4a5f88 100755 --- a/scripts/sign_checksums.py +++ b/scripts/sign_checksums.py @@ -42,6 +42,25 @@ from pathlib import Path # message signed by the same key. DOMAIN_PREFIX = b"bcc-release-v1|" +# Public half of the RELEASE signing key(s) -- a SEPARATE keypair from +# bcc_core.CATALOG_PUBKEYS (issue #68 finding 5). The catalog key is the +# offline, Console-only root of trust for what BCC executes; this key is +# CI-resident and signs ONLY the release SHA256SUMS manifest, never the +# catalog. Keeping them apart means a CI/repo-secret compromise burns the +# release key -- annoying, but it never lets an attacker sign a catalog a +# user's binary would trust. A LIST (not a single key), mirroring +# CATALOG_PUBKEYS, so the release key can be rotated without invalidating +# 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: +# 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] = [] + CHUNK_SIZE = 1024 * 1024 @@ -135,6 +154,19 @@ def public_key_b64_from_seed(seed_b64: str) -> str: return base64.b64encode(raw).decode("ascii") +def verify_checksums_against_any(pubkeys: list[bytes], sums_text: str, signature: bytes) -> bool: + """Verify `signature` against ANY key in `pubkeys` (each a raw 32-byte + Ed25519 public key). Mirrors bcc_core.verify_catalog_signature's + rotation-friendly "any currently-trusted key" semantics, applied to + RELEASE_PUBKEYS instead of the catalog's key list. Returns False (never + raises) for an empty `pubkeys` list -- fails closed rather than + vacuously verifying against nothing.""" + return any( + verify_checksums(base64.b64encode(pk).decode("ascii"), sums_text, signature) + for pk in pubkeys + ) + + # --------------------------------------------------------------------------- # # CLI # --------------------------------------------------------------------------- # diff --git a/tests/test_catalog_review.py b/tests/test_catalog_review.py index adee2b6..8249d7c 100644 --- a/tests/test_catalog_review.py +++ b/tests/test_catalog_review.py @@ -334,13 +334,29 @@ def test_acknowledge_gating_requires_every_entry(): assert r.all_entries_acknowledged(session) is True -def test_no_acknowledge_all_function_exists(): - """Deliberate: there must be no shortcut to acknowledge every entry at - once. See the comment in catalog_review.py above SignDecision.""" +def test_no_acknowledge_all_shortcut_and_gate_is_real(): + """Two things, both load-bearing (issue #68: the original version of + this test asserted ONLY the first half, and passed the entire time the + gate below it was vacuously satisfiable -- 'no function named + acknowledge_all' is worthless if signing doesn't actually require + acknowledgement in practice). + + 1. No bulk-acknowledge shortcut exists (see the comment in + catalog_review.py above SignDecision -- deliberate friction). + 2. The gate that friction protects is actually enforced: with entries + still unacknowledged, can_sign() must refuse, not just "some GUI + checkbox happens to be unticked". + """ names = [n for n in dir(r) if "acknowledge" in n.lower()] assert "acknowledge_all" not in names assert "acknowledge_all_entries" not in names + session = r.start_review("sha1", _catalog(), _catalog(_entry(id="a"), _entry(id="b"))) + r.acknowledge_entry(session, "a") # only one of two -- not a bulk call + decision = r.can_sign(session, "sha1") + assert decision.ok is False + assert "acknowledged" in decision.reason.lower() + # --------------------------------------------------------------------------- # # can_sign: TOCTOU blob pinning + acknowledge gating combined @@ -377,6 +393,130 @@ def test_can_sign_blob_mismatch_takes_priority_message(): assert "blob" in decision.reason.lower() or "changed" in decision.reason.lower() +def test_can_sign_false_on_empty_changeset(): + """The exact bug behind issue #68 finding 1: 'main' loaded against + itself diffs to [], and an empty changeset used to leave can_sign() + with nothing to refuse on (set() <= set() is vacuously True). Commit + b08cf21 signed 19 entries through precisely this path -- zero of them + were ever reviewed. An empty diff must mean 'nothing to sign', never + 'sign unlocked'.""" + same_catalog = _catalog(_entry()) + session = r.start_review("sha1", same_catalog, same_catalog) + assert session.changes == [] # diff_catalogs(x, x) -> [] + assert r.all_entries_acknowledged(session) is True # vacuously -- this is the trap + decision = r.can_sign(session, "sha1") # blob matches, "everything" acknowledged + assert decision.ok is False + assert "nothing to sign" in decision.reason.lower() + + +def test_can_sign_false_with_outstanding_blocking_risk_even_if_acknowledged(): + """can_sign() must itself refuse a blocking risk finding -- today a + blocking finding only disables the GUI checkbox, so the pure gate must + not simply trust that the caller never acknowledged a blocking entry. + Acknowledge it directly here (bypassing any GUI checkbox-disable logic + entirely) to prove the gate catches it independently of the GUI.""" + session = r.start_review( + "sha1", + _catalog(), + _catalog(_entry(config={"command": "bash", "args": ["-c", "evil"]})), + ) + r.acknowledge_entry(session, "filesystem") + 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() + + +# --------------------------------------------------------------------------- # +# sign_precondition: the ref-resolution seam that used to hardcode "main" +# --------------------------------------------------------------------------- # +def test_sign_precondition_resolves_against_loaded_ref_not_hardcoded_main(): + """The regression test for issue #68 finding 1's first bug: + ReviewWindow._on_sign used to hardcode fetch_ref(repo, "main") as the + TOCTOU comparison ref. For a PR review, _on_load pins the PR HEAD's + blob SHA, so comparing against main's SHA differs by definition and + Sign could never fire on the PR path. + + The fake resolver below returns a DIFFERENT (deliberately wrong) SHA for + "main" than for the PR ref that was actually loaded. If + sign_precondition ever resolves against "main" instead of + session.loaded_ref, this test fails -- both via the recorded `calls` + list and via decision.ok flipping to False. + """ + pr_ref = "refs/pull/42/head" + session = r.start_review("pr-blob-sha", _catalog(), _catalog(_entry()), loaded_ref=pr_ref) + r.acknowledge_entry(session, "filesystem") + + calls: list[str] = [] + + def fake_resolver(ref: str) -> str: + calls.append(ref) + return {"main": "main-blob-sha-WRONG", pr_ref: "pr-blob-sha"}[ref] + + decision = r.sign_precondition(session, fake_resolver) + assert calls == [pr_ref] # never asked the resolver for "main" + assert decision.ok is True + assert decision.reason is None + + +def test_sign_precondition_refuses_when_loaded_ref_blob_moved(): + """Same seam, the negative case: if the loaded ref's blob SHA has moved + since review began (a new commit landed on the reviewed PR/branch), the + resolver reflects that and sign_precondition must refuse -- proving this + isn't just a hardcoded pass-through.""" + pr_ref = "refs/pull/42/head" + session = r.start_review("pr-blob-sha", _catalog(), _catalog(_entry()), loaded_ref=pr_ref) + r.acknowledge_entry(session, "filesystem") + + def fake_resolver(_ref: str) -> str: + return "pr-blob-sha-AFTER-A-NEW-PUSH" + + decision = r.sign_precondition(session, fake_resolver) + assert decision.ok is False + assert "mismatch" in decision.reason.lower() or "changed" in decision.reason.lower() + + +def test_sign_precondition_defaults_to_main_when_loaded_ref_unset(): + """start_review()'s loaded_ref defaults to 'main' for source=main + reviews (and backward-compat with callers that don't pass it).""" + session = r.start_review("sha1", _catalog(), _catalog(_entry())) + assert session.loaded_ref == "main" + r.acknowledge_entry(session, "filesystem") + + def fake_resolver(ref: str) -> str: + assert ref == "main" + return "sha1" + + decision = r.sign_precondition(session, fake_resolver) + assert decision.ok is True + + +# --------------------------------------------------------------------------- # +# find_last_signed_catalog_raw: what source=main diffs against +# --------------------------------------------------------------------------- # +def test_find_last_signed_catalog_raw_returns_matching_candidate(): + """Simulates walking catalog.json's git history: the CURRENT signature + covers an OLDER version of the bytes (a later commit changed + catalog.json without re-signing -- the exact bypass that produced + commit b08cf21). The first candidate that verifies against that + signature is 'the last catalog a maintainer actually signed'.""" + seed, pubkey = r.generate_keypair() + old_raw = b'{"schema":1,"version":1,"servers":[]}' + new_raw = b'{"schema":1,"version":2,"servers":[]}' + sig = r.sign_catalog_bytes(old_raw, seed) # signature covers the OLD bytes + found = r.find_last_signed_catalog_raw([new_raw, old_raw], sig, [pubkey]) + assert found == old_raw + + +def test_find_last_signed_catalog_raw_none_when_nothing_verifies(): + seed, _pubkey = r.generate_keypair() + _other_seed, other_pubkey = r.generate_keypair() + raw = b'{"schema":1,"version":1,"servers":[]}' + sig = r.sign_catalog_bytes(raw, seed) + # Check against a pubkey list that does NOT include the signer's key. + assert r.find_last_signed_catalog_raw([raw], sig, [other_pubkey]) is None + + # --------------------------------------------------------------------------- # # catalog_signing_message: domain separation must match bcc_core exactly # --------------------------------------------------------------------------- # diff --git a/tests/test_core.py b/tests/test_core.py index afeaec5..98562a9 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1690,8 +1690,12 @@ def _minimal_catalog(version: int = 1) -> dict: "official": True, "setup": "basic", "config": { + # Pinned on purpose (issue #68 finding 3): an earlier + # version of this fixture used an unpinned package and + # asserted it validated clean, which enshrined the bug + # instead of catching it. "command": "npx", - "args": ["-y", "widget-mcp"], + "args": ["-y", "widget-mcp@1.0.0"], }, "placeholders": {}, "env_required": {}, @@ -1952,6 +1956,213 @@ def test_validate_catalog_rejects_duplicate_ids(): assert any("duplicate id" in p for p in problems) +# --- validate_catalog: config.env (issue #68 finding 2) -------------------- # +def test_validate_catalog_rejects_each_denied_env_key(): + for key in sorted(c.CATALOG_DENIED_ENV_KEYS): + data = _catalog_with( + {"config": {"command": "npx", "args": ["-y", "widget-mcp@1.0.0"], "env": {key: ""}}} + ) + problems = c.validate_catalog(data) + assert any("deny-list" in p for p in problems), (key, problems) + + +def test_validate_catalog_rejects_denied_env_key_case_insensitively(): + data = _catalog_with( + { + "config": { + "command": "npx", + "args": ["-y", "widget-mcp@1.0.0"], + "env": {"node_options": ""}, + } + } + ) + problems = c.validate_catalog(data) + assert any("deny-list" in p for p in problems) + + +def test_validate_catalog_rejects_nonempty_nonplaceholder_env_value(): + data = _catalog_with( + { + "config": { + "command": "npx", + "args": ["-y", "widget-mcp@1.0.0"], + "env": {"FOO_URL": "https://example.com"}, + } + } + ) + problems = c.validate_catalog(data) + assert any("empty string or a single " in p for p in problems) + + +def test_validate_catalog_accepts_empty_and_placeholder_env_values(): + data = _catalog_with( + { + "config": { + "command": "npx", + "args": ["-y", "widget-mcp@1.0.0"], + "env": {"FOO": "", "BAR_URL": ""}, + } + } + ) + assert c.validate_catalog(data) == [] + + +def test_validate_catalog_rejects_non_ascii_env_key(): + data = _catalog_with( + { + "config": { + "command": "npx", + "args": ["-y", "widget-mcp@1.0.0"], + "env": {"Fร–O": ""}, + } + } + ) + problems = c.validate_catalog(data) + assert any("config.env key" in p and "ASCII" in p for p in problems) + + +def test_validate_catalog_rejects_non_ascii_env_value(): + data = _catalog_with( + { + "config": { + "command": "npx", + "args": ["-y", "widget-mcp@1.0.0"], + "env": {"FOO": ""}, + } + } + ) + problems = c.validate_catalog(data) + assert any("config.env value" in p and "ASCII" in p for p in problems) + + +def test_validate_catalog_rejects_secret_looking_env_value(): + data = _catalog_with( + { + "config": { + "command": "npx", + "args": ["-y", "widget-mcp@1.0.0"], + "env": {"SOME_TOKEN": "ghp_abcdef1234567890"}, + } + } + ) + problems = c.validate_catalog(data) + assert any("real secret value" in p for p in problems) + + +def test_validate_catalog_rejects_node_options_env_walking_past_allowlist(): + # The exact reproduction from issue #68 finding 2: an allowlisted + # `npx` command carrying NODE_OPTIONS in env, which previously passed + # validation and would have flowed straight into the executed + # subprocess via catalog_entry_to_paste_json(). + data = _catalog_with( + { + "config": { + "command": "npx", + "args": ["-y", "widget-mcp@1.0.0"], + "env": {"NODE_OPTIONS": "--require /tmp/payload.js"}, + } + } + ) + problems = c.validate_catalog(data) + assert problems != [] + + +# --- validate_catalog: version pinning (issue #68 finding 3) --------------- # +def test_validate_catalog_rejects_unpinned_npx_package(): + data = _catalog_with({"config": {"command": "npx", "args": ["-y", "widget-mcp"]}}) + problems = c.validate_catalog(data) + assert any("not version-pinned" in p for p in problems) + + +def test_validate_catalog_rejects_unpinned_scoped_npx_package(): + data = _catalog_with({"config": {"command": "npx", "args": ["-y", "@scope/pkg"]}}) + problems = c.validate_catalog(data) + assert any("not version-pinned" in p for p in problems) + + +def test_validate_catalog_accepts_pinned_scoped_npx_package(): + data = _catalog_with({"config": {"command": "npx", "args": ["-y", "@scope/pkg@1.2.3"]}}) + assert c.validate_catalog(data) == [] + + +def test_validate_catalog_rejects_unpinned_uvx_package(): + data = _catalog_with({"config": {"command": "uvx", "args": ["some-tool"]}}) + problems = c.validate_catalog(data) + assert any("not version-pinned" in p for p in problems) + + +def test_validate_catalog_accepts_uvx_at_version_pin(): + data = _catalog_with({"config": {"command": "uvx", "args": ["some-tool@1.0.0"]}}) + assert c.validate_catalog(data) == [] + + +def test_validate_catalog_accepts_uvx_double_equals_pin(): + data = _catalog_with({"config": {"command": "uvx", "args": ["some-tool==1.0.0"]}}) + assert c.validate_catalog(data) == [] + + +def test_validate_catalog_rejects_docker_latest_tag(): + data = _catalog_with({"config": {"command": "docker", "args": ["run", "some/image:latest"]}}) + problems = c.validate_catalog(data) + assert any("'latest'" in p for p in problems) + + +def test_validate_catalog_rejects_docker_untagged_image(): + data = _catalog_with({"config": {"command": "docker", "args": ["run", "some/image"]}}) + problems = c.validate_catalog(data) + assert any("no explicit tag" in p for p in problems) + + +def test_validate_catalog_accepts_pinned_docker_image_with_flags(): + data = _catalog_with( + { + "config": { + "command": "docker", + "args": ["run", "-i", "--rm", "-e", "SOME_TOKEN", "some/image:1.2.3"], + } + } + ) + assert c.validate_catalog(data) == [] + + +def test_validate_catalog_does_not_pin_check_placeholders_flags_or_subcommand(): + # A pinned uvx spec followed by flags and a positional + # must not itself get mistaken for an unpinned package. + data = _catalog_with( + { + "config": { + "command": "uvx", + "args": ["mcp-server-git@2026.7.10", "--repository", ""], + } + } + ) + assert c.validate_catalog(data) == [] + + +# --- validate_catalog: id constraint (issue #68 finding 7) ----------------- # +def test_validate_catalog_rejects_id_with_markup(): + data = _catalog_with({"id": "Verified"}) + problems = c.validate_catalog(data) + assert any("must match" in p for p in problems) + + +def test_validate_catalog_rejects_id_with_uppercase(): + data = _catalog_with({"id": "Widget"}) + problems = c.validate_catalog(data) + assert any("must match" in p for p in problems) + + +def test_validate_catalog_rejects_id_starting_with_dash(): + data = _catalog_with({"id": "-widget"}) + problems = c.validate_catalog(data) + assert any("must match" in p for p in problems) + + +def test_validate_catalog_accepts_valid_slug_id(): + data = _catalog_with({"id": "widget-2.thing-ok"}) + assert c.validate_catalog(data) == [] + + # --- resolve_catalog -------------------------------------------------------- # def test_resolve_catalog_nothing_available_returns_empty_dict(): assert c.resolve_catalog(None, None, None) == {} @@ -2003,13 +2214,93 @@ def test_resolve_catalog_rejects_absurd_version_jump(monkeypatch): pub = priv.public_key().public_bytes_raw() monkeypatch.setattr(c, "CATALOG_PUBKEYS", [pub]) - cached = _signed(_minimal_catalog(version=5), priv) + # The freeze attempt goes FIRST (as `cached`), the legitimate catalog + # SECOND (as `remote`) -- on purpose. Putting the good catalog first + # (as an earlier version of this test did) never exercises the + # vulnerable path: the old implementation only guarded a candidate + # against "the best accepted so far," so whichever candidate was + # evaluated FIRST got in unconditionally, uncapped. Ordering the freeze + # attempt first is what actually proves the cap holds regardless of + # evaluation order. freeze_attempt = _signed(_minimal_catalog(version=999999), priv) + good = _signed(_minimal_catalog(version=5), priv) - result = c.resolve_catalog(None, cached, freeze_attempt) + result = c.resolve_catalog(None, freeze_attempt, good) assert c.catalog_version(result) == 5 +def test_resolve_catalog_caps_first_and_only_candidate(monkeypatch): + # issue #68 finding 6: with no bundled catalog to anchor against, a + # signed catalog claiming an absurd version must still be capped even + # when it is the ONLY candidate resolve_catalog() ever sees -- there is + # no "best so far" for it to be compared against, so the cap has to + # apply unconditionally, not "once something else has already landed." + priv = Ed25519PrivateKey.generate() + pub = priv.public_key().public_bytes_raw() + monkeypatch.setattr(c, "CATALOG_PUBKEYS", [pub]) + + freeze_attempt = _signed(_minimal_catalog(version=999999999), priv) + + result = c.resolve_catalog(None, None, freeze_attempt) + assert result == {} + + +def test_resolve_catalog_anchors_cap_to_bundled_not_a_chained_best(monkeypatch): + # Anti-freeze must be measured against the BUNDLED version specifically, + # not against "whatever the best-so-far happens to be after each + # candidate is accepted" -- a chained anchor lets each accepted + # candidate ratchet the allowed ceiling upward, so a legitimate + # moderate bump (cached) plus a second, much larger jump (remote) can + # each individually look "within _CATALOG_MAX_VERSION_JUMP of the + # previous one" while remote is nowhere near bundled's version. + priv = Ed25519PrivateKey.generate() + pub = priv.public_key().public_bytes_raw() + monkeypatch.setattr(c, "CATALOG_PUBKEYS", [pub]) + + bundled = _signed(_minimal_catalog(version=2), priv) + cached = _signed(_minimal_catalog(version=1000), priv) # within 1000 of bundled + remote = _signed(_minimal_catalog(version=1900), priv) # within 1000 of cached, + # NOT of bundled + + result = c.resolve_catalog(bundled, cached, remote) + assert c.catalog_version(result) == 1000 + + +def test_resolve_catalog_prefers_bundled_on_version_tie(monkeypatch): + # issue #68 finding 6: on a tie the LAST candidate evaluated used to + # win, so remote silently beat bundled at equal version. Bundled -- + # the copy frozen into the binary -- must win ties. + priv = Ed25519PrivateKey.generate() + pub = priv.public_key().public_bytes_raw() + monkeypatch.setattr(c, "CATALOG_PUBKEYS", [pub]) + + bundled_data = _minimal_catalog(version=5) + bundled = _signed(bundled_data, priv) + + remote_data = _minimal_catalog(version=5) + remote_data["servers"][0]["display"] = "Remote Impostor" + remote = _signed(remote_data, priv) + + result = c.resolve_catalog(bundled, None, remote) + assert c.catalog_version(result) == 5 + assert result["servers"][0]["display"] == "Widget" + + +def test_resolve_catalog_floor_rejects_below_persisted_version(monkeypatch): + # `floor` is a pure parameter: the caller (eventually the GUI, from + # persisted storage) can pass a previously-accepted version, and + # nothing below it may be accepted even with no bundled catalog to + # anchor against. + priv = Ed25519PrivateKey.generate() + pub = priv.public_key().public_bytes_raw() + monkeypatch.setattr(c, "CATALOG_PUBKEYS", [pub]) + + stale = _signed(_minimal_catalog(version=3), priv) + + result = c.resolve_catalog(None, None, stale, floor=10) + assert result == {} + + def test_resolve_catalog_malformed_candidate_does_not_raise(monkeypatch): priv = Ed25519PrivateKey.generate() pub = priv.public_key().public_bytes_raw() @@ -2039,15 +2330,32 @@ def test_resolve_catalog_invalid_but_signed_candidate_is_skipped(monkeypatch): def test_catalog_entry_to_paste_json_basic_shape(): entry = _minimal_catalog()["servers"][0] result = c.catalog_entry_to_paste_json(entry) - assert result == {"widget": {"command": "npx", "args": ["-y", "widget-mcp"]}} + assert result == {"widget": {"command": "npx", "args": ["-y", "widget-mcp@1.0.0"]}} def test_catalog_entry_to_paste_json_includes_env_when_present(): + # NOTE: catalog_entry_to_paste_json() is a pure shape-converter for an + # entry that has ALREADY passed validate_catalog() -- it is correct for + # it to carry env through verbatim. The bug (issue #68 finding 2) was + # never in this function; it was that validate_catalog() let entries + # with dangerous/non-placeholder env values reach this function in the + # first place. This test now proves that boundary explicitly: a + # validation-legal env value (a token) survives the + # conversion, and a value validate_catalog() would have rejected is + # confirmed rejected before it ever gets here. entry = _minimal_catalog()["servers"][0] entry["config"]["env"] = {"GRAFANA_URL": ""} result = c.catalog_entry_to_paste_json(entry) assert result["widget"]["env"] == {"GRAFANA_URL": ""} + catalog = _minimal_catalog() + catalog["servers"][0]["config"]["env"] = {"GRAFANA_URL": ""} + assert c.validate_catalog(catalog) == [] + + malicious = _minimal_catalog() + malicious["servers"][0]["config"]["env"] = {"NODE_OPTIONS": "--require /tmp/payload.js"} + assert c.validate_catalog(malicious) != [] + def test_catalog_entry_to_paste_json_seeds_env_required_keys(): # Regression: env_required is where the seed data actually keeps its