fix(core): ignore invalid signatures in satisfies_threshold (#349)

[andreolf]

Summary

RefUpdateCert::satisfies_threshold failed closed on a single malformed or invalid signature entry, so appending junk signatures denied an otherwise valid certificate. It now ignores invalid entries and counts only the valid signers. Fixes #349.

Motivation & context

Closes #349

satisfies_threshold delegated to verify_all, which uses ? on four fallible steps per entry (DID → key, base64 decode, 64-byte length, signature verify). Signature entries live outside the signed body (RefUpdateBody::to_signing_bytes), so any third party can append an entry with no key material. One junk entry made the whole threshold check error out — a denial-of-service on a cert that actually has enough valid signatures.

Follows the maintainer guidance on the issue: fix it in satisfies_threshold, not verify_all, to preserve the public API (and its tampered_signature_fails_verify_all test). This mirrors the sibling handling in gitlawb-attest's verifier.

Kind of change

• Bug fix

What changed

Crate touched: gitlawb-core (src/cert.rs).

• Added a private valid_signers() helper that iterates signature entries and skips any that fail to resolve a key, decode, reach 64 bytes, or verify — returning only the DIDs that actually validate.
• satisfies_threshold now counts distinct valid signers from valid_signers() instead of verify_all. A forged entry naming a real maintainer DID with a bad signature is skipped, so it cannot inflate the count either.
• verify_all is unchanged (still fail-closed); expanded its docstring to state it is strict and to point threshold callers at satisfies_threshold.

How a reviewer can verify

cargo test -p gitlawb-core --lib cert
cargo clippy -p gitlawb-core --all-targets -- -D warnings

Two new tests:

• satisfies_threshold_ignores_appended_junk_signatures — a valid 2-of-2 cert with appended unparseable/short entries still satisfies a 2-of-2 threshold. Fails on the pre-fix code (the junk made verify_all error).
• satisfies_threshold_does_not_count_a_forged_maintainer_signature — an appended entry under a real maintainer DID but with a non-verifying signature is not counted: 2-of-2 fails, 1-of-2 holds.

Before you request review

• Scope is one logical change; no unrelated churn
• cargo test --workspace passes locally (ran gitlawb-core: 89 tests pass)
• New behavior is covered by tests
• cargo fmt --all and cargo clippy --workspace --all-targets -- -D warnings are clean
• Commit titles use Conventional Commits (fix(core): ...)
• Docs / .env.example updated if behavior or config changed (N/A)
• Checked existing PRs so this isn't a duplicate

Protocol & signing impact

• Touches DID / did:key, Ed25519 / RFC 9421 signatures, UCAN, ref certs, or P2P wire formats — ref-update cert threshold verification. No wire-format or signature-scheme change.
• Backward-compatible with existing nodes and previously signed history: for a cert with only valid signatures, behavior is identical; the change only affects how appended invalid entries are treated (ignored instead of erroring). Fails closed on forged entries (they are not counted).

Notes for reviewers

• Companion to fix(attest): make verify_all lenient under RequireAll (#359) #365, which fixes the same short-circuit pattern in gitlawb-attest's verify_all. Kept as a separate PR per crate.

[coderabbitai]

Warning

Review limit reached

Next included review available in 20 minutes.

View limit details

Limit details: You’ve used the included review currently available.

This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 79b54953-3193-4d86-86bc-59201c2a6c41

📥 Commits

Reviewing files that changed from the base of the PR and between bfc44f9 and 7c4daa2.

📒 Files selected for processing (1)

• crates/gitlawb-core/src/cert.rs

---

_{Comment @coderabbitai help to get the list of available commands.}

[beardthelion]

The leniency is the right call, and the two new tests bind real behavior: gutting the delegation to valid_signers turns both red, so the premise is load-bearing rather than carried by volume. Trust anchoring is sound too, since the signer DID comes off the entry but only counts after passing the caller's maintainer set. Three asks.

Reachability first, so the severities read right: satisfies_threshold has no caller outside this module yet, so none of this is live, and I have rated it accordingly. The first finding would be a P1 the moment a caller lands.

Findings

• [P2] Bound the signature-entry count before verifying any of them
  crates/gitlawb-core/src/cert.rs:154
  Entries sit outside the signed body, which is this fix's whole premise, so their count is attacker-controlled and each one now costs a full Ed25519 verify. Measured on a release build: 50,000 appended entries put satisfies_threshold at 8.70s, where pre-fix verify_all shed the same input in 359µs by short-circuiting. Nothing bounds it, since validate_structure only rejects the empty list and satisfies_threshold never calls it. Rejecting an oversized list ahead of any crypto took a 5,000-entry case from 3.86s to 76.9µs when I tried it, and it mirrors MAX_METHOD_ID_LEN in to_verifying_key, which bounds an untrusted method-id ahead of a quadratic decode for the same reason. The constant is yours to pick; the ask is that the bound sits before the loop.

• [P2] Do not let an unresolvable maintainer DID read as a silent threshold miss
  crates/gitlawb-core/src/cert.rs:186
to_verifying_key() errors on any method other than did:key, so a did:web or did:gitlawb entry in maintainers can never be counted. Pre-fix that surfaced as Err("expected did:key, got did:web"). Now the entry is skipped and a 2-of-2 returns Ok(false), with nothing separating a misconfigured maintainer set from a cert that genuinely lacks signatures. The maintainer set is trusted input, so the leniency belongs to the untrusted appended entries only: resolving each maintainer DID before the count preserves the #349 behavior and makes the misconfiguration diagnosable.

• [P2] Add the mirror of #365's malformed-required-type assertion
  crates/gitlawb-core/src/cert.rs:160
  The suite pins the deny-the-DoS direction but not the don't-count-it direction. Making each skip arm push its signer instead of skipping leaves the base64, length, and DID-resolve arms green. satisfies_threshold_ignores_appended_junk_signatures cannot catch it, because the junk it appends names DIDs that already signed, so counting the junk signer only re-adds a DID the set already holds. #365 covers exactly this direction with require_all_rejects_when_only_a_malformed_required_type_is_present; the equivalent here is to sign with kp1 only, append an unparseable-base64 and a short-base64 entry both naming kp2, then assert 2-of-2 fails and 1-of-2 holds.

Not an ask: valid_signers restates all four of verify_all's per-entry checks. They agree today, so this is a maintenance note. If you want it, folding them into one per-entry helper (strict caller propagates, lenient caller discards) is behavior-preserving here, and it buys something real: a defect introduced in the shared verify step then fails both tampered_signature_fails_verify_all and the new threshold test, instead of needing to be introduced twice to be caught twice. Fine as a follow-up.

CI has not actually run on this head. PR Checks is at action_required awaiting approval, so the single green check is only the triage job. Worth a rerun when you push.

[jatmn]

I found an issue that needs to be addressed before this is ready.

Findings

• [P2] Bound the tolerant signature scan before doing cryptographic work
  crates/gitlawb-core/src/cert.rs:159
  Signature entries are outside the signed body, so a peer can append an arbitrary number of syntactically valid did:key / 64-byte entries with bogus signatures. valid_signers now resolves, decodes, and verifies every one of them (including entries after the threshold is already met), while neither this path nor validate_structure limits the entry count or encoded signature size. This replaces the old first-invalid-entry exit with attacker-controlled CPU and allocation work whenever a received certificate is threshold-checked. Reject an oversized signature list and oversized encoded signatures before the loop (and, if restructuring the threshold path, stop once the result is known) so the DoS fix does not create an unbounded verification path.

[jatmn]

lgtm

[beardthelion]

The round-2 bound is the right shape and the cap is a defensible call. Three things to fix, one of which is that the regression test for #349 no longer tests #349.

Findings

• [P2] Move the junk entries ahead of the real signatures in the regression test for #349
  crates/gitlawb-core/src/cert.rs:466
satisfies_threshold_ignores_appended_junk_signatures appends its two junk entries after the two real ones, so with threshold 2 the early return at :221 fires at index 1 and neither junk entry is ever examined. I mutated the length gate at :207 from continue to return Err and that test stayed green; the only test that reddened was satisfies_threshold_skips_an_oversized_encoded_signature. Reordering so the junk sits ahead of the real entries (I used the two existing ones plus a bogus 86-character entry) makes the same mutation turn it red, and it is green unmutated.

• [P3] Hoist the threshold == 0 fast path above the entry-count check
  crates/gitlawb-core/src/cert.rs:190
  A 65-entry certificate with threshold 0 returns Err today, which contradicts threshold_zero_is_always_satisfied one screen down. Swapping the two blocks is the whole fix. With that and the reorder applied together, on the pinned toolchain: rustup run 1.91 cargo clippy -p gitlawb-core --all-targets -- -D warnings clean, the crate suite green, cargo fmt --check clean.

• [P3] Correct the "concedes nothing" line on MAX_SIGNATURE_ENTRIES
crates/gitlawb-core/src/cert.rs:39
  Executed on this head, a valid 2-of-2 cert with 65 appended junk entries returns Err(RefCert("certificate carries 67 signature entries (max 64)")). Round 1 of this PR returned Ok(true) for that input, so the loud rejection does concede something relative to where the branch stood. I am not asking you to change the design: an attacker who can append that many entries in transit can also drop the certificate, so the ceiling is fine. The sentence just needs to say the denial exists above the cap instead of saying it does not.

Everything else held up. The cap guard and the maintainer pre-filter are both load-bearing, and satisfies_threshold still has no production caller, so none of this is reachable yet.

Stale: reviewed 107349e, superseded by the review on the current head.