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

@andreolf, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 43 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 67133f06-b38a-444f-832c-f37e90e9c162

📥 Commits

Reviewing files that changed from the base of the PR and between e4c7458 and 107349e.

📒 Files selected for processing (1)

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

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{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.