Audit hardening: validation, gate alignment, and operational fixes for the ceremony - #1
Merged
Merged
Conversation
mellowcroc
force-pushed
the
audit/ed25519-key-validation
branch
2 times, most recently
from
August 14, 2026 06:01
dbbfb84 to
4932d2b
Compare
Identity.Validate checked only that ed25519_public_key_hex decoded to 32 bytes and that public_key_fingerprint matched. It never checked that the bytes are a usable curve point, so a roster could enrol a small-order key. Ed25519 verification computes [-k]A + [S]B and compares the result to R. When A has small order that equation collapses: the signature R = identity, S = 0 verifies against every message. Anyone can then forge signatures for that identity without holding a private key, which voids every signature-based control for whichever role holds it. A coordinator who authors participants.json could plant such a key for a public witness and manufacture the receipts that exist to detect coordinator equivocation. Validate now rejects three cases: bytes that are not a curve point, non-canonical encodings, and points of small order. The canonical check matters on its own because identity uniqueness across the definition is enforced on public_key_fingerprint, a hash of these exact bytes, so two encodings of one point would otherwise register as two distinct identities. The guard lives in Identity.Validate, so it also covers the roles enrolled outside the ceremony definition: EnrollmentRecord, PublicWitnessReceipt and ImmutableMirrorReceipt each validate their embedded Identity.
Three layers disagreed on how many audits a ceremony may have. A definition may enrol two or more auditors (definition.go). SignRelease accepts two or more signed passing reports (audit.go). ProductionDecision demanded exactly two. A ceremony that enrolled three auditors, which is permitted and strictly more conservative, could therefore produce a valid signed release that could never be recorded in a valid decision. The failure surfaces at final GO signing, after the ceremony is complete and nothing can be redone. Both audit lists now require at least two rather than exactly two, and every site that consumed them follows. - Validate: distinctness of auditor key ids and external signer fingerprints moves from comparing elements 0 and 1 to a set check across the whole slice. - requiredDecisionSigners: every named auditor must sign, not just the first two. An auditor whose report is bound into the decision but whose consent is not required would otherwise be recorded as having reviewed the release without agreeing to it, and a signature from them was rejected as falling outside the required threshold. - verifyProductionRelease: expectedAuditRefs is built from the full slice, so a release binding three audits coheres with its final transcript, which already accepted two or more. - allLocatedArtifacts: every audit's record and signature enters URI conflict detection and the located-artifact digest sweep. releaseChecksumNames and verifyReleaseTreeExact were already count-derived.
A K=21 phase close replays every accepted contribution before it writes anything, which runs for hours and produced no output. An operator could not tell a running replay from a hung one, and could not measure how long a close takes on their hardware. That measurement is not a convenience. The closure commits to a future drand round, and choosing a round far enough ahead requires knowing how long the replay will take. Misjudging it is what caused the 2026-07-24 closure-timing incident. The current code fails loudly in that case rather than publishing an invalid closure, but the operator still burns the attempt with no better information for the retry. internal/mpcceremony deliberately has no logger: it handles signing keys and secret contribution state, and having no output path is stronger than having a careful one. A callback preserves that. ReplayProgress carries a phase, a one-based index and a total, never a path or key material, and rendering is the caller's business. PhaseTranscriptPaths carries the optional callback, which reaches every replay site already threaded through that struct. The CLI writes to stderr, never stdout, which is reserved for the result contract. Single head loads pass nil because they read one record rather than replaying.
Records the findings behind the preceding commits, plus the items that are not code changes, so a reviewer can see what was checked and what was left open. Each entry cites the file and line that establishes it, and separates verified findings from proposals and from items that were named but not investigated. The local runbook documents how to build the tool and stand up a ceremony on one machine. It is orientation and rehearsal only. The production procedure is docs/mpc-ceremony-runbook.md, which is absent from main and survives only in refs/pull/34/head of the upstream repository (item B1). It also records the two roots of trust, the coordinator public key and the binary, which must arrive over channels the reader already trusts. scripts/mpc-demo-init.sh runs the documented init end to end. It builds with go build rather than go run, because go run omits the VCS metadata that software.go requires, and it reads the coordinator key id back from participants.json rather than hardcoding it.
ContributionEnvironment.OS/.Architecture and audit findings used a plain `== ""` presence check, so a single space satisfied "must not be empty" and flowed into signed attestations and records. Require the trimmed, non-empty form, matching the convention already used for Identity.DisplayName and (in e9a789f) artifact names.
mellowcroc
force-pushed
the
audit/ed25519-key-validation
branch
from
August 14, 2026 06:26
c747d26 to
a37c27f
Compare
gnark's mpcsetup APIs mutate their arguments, and this package's discipline is to streamClone before any call that does. Three sites did not follow it. VerifyAndAcceptContribution verified the candidate it retains. Phase1.Verify and Phase2.Verify write next.Challenge, and the same candidate pointer is re-serialized into the authoritative transcript further down the function. Today the write is value-identical because the challenge-equality guard runs first, so nothing is corrupted, but the archived object is handed to a mutating API and stays correct only by coincidence. Both arms now verify a throwaway clone. That clone costs a second copy of the contribution state for the duration of the verify: roughly 576 MiB at K=21 for Phase 1, and the circuit-dependent equivalent for Phase 2. Acceptance already holds the predecessor and the candidate simultaneously, so this raises the peak by one state rather than changing the order of magnitude. Paying it buys the guarantee that no gnark call ever receives a pointer the transcript depends on. sealReplayedPhase1Head returns commons that alias the head it consumed. Seal returns p.parameters by value, and those slice headers point at the head's backing arrays rather than at copies, so mutating or re-sealing the head afterwards would corrupt commons already returned to the caller. The doc comment now says so, and both callers that keep the head in scope past the seal drop their reference at the call site, which makes reuse structurally impossible rather than merely discouraged. Phase2.Seal retains evals.G1.CKK and evals.G1.VKK in the keys it produces. Comments at the seal call site and at replayPhase2State's return record that evaluations must stay per-call, since a cached or shared Phase2Evaluations would leave two key sets aliasing one set of commitment arrays.
mellowcroc
force-pushed
the
audit/ed25519-key-validation
branch
from
August 16, 2026 15:47
bea5180 to
a37c27f
Compare
Identity.Validate checked display_name only for trimming and UTF-8 validity, so there was no length bound and interior ANSI escapes, bidi overrides, and zero-width characters reached signed records, transcripts, and logs. validateArtifactName was hardened earlier but shared the same blind spot: it screens with unicode.IsControl, which reports Unicode category Cc, while every bidi and zero-width character is category Cf and passed through. Both validators now share rejectDeceptiveRunes, which rejects control characters, the bidi formatting set, and U+200B. validateDisplayName adds a 256-byte cap. The bidi and zero-width sets are listed explicitly instead of rejecting all of category Cf, because U+200C separates Persian and Indic letterforms and U+200D joins emoji sequences; a blanket ban would make legitimate names unwritable. A test asserts those stay accepted. Nothing here was forgeable. display_name is never read for a decision and identity is keyed on id, key id, and public key fingerprint. The target is the human review that the audit and release stages depend on: a value stored as U+202E followed by "ecila" displays as "alice", so a reviewer approves one string while the transcript records another. That is the Trojan Source technique applied to attested names rather than source code.
An inventory of the deliberate defenses in the ceremony code, each mapped to the attack it counters with a file:line citation, plus the gaps found during the audit and their current state. It was written against the tree rather than committed with it, so it has been sitting untracked. That also blocks a production ceremony: Go stamps vcs.modified from git status, which counts untracked files, and a production definition requires a clean checkout.
A phase close names its beacon round up front, then replays the entire accepted phase, then stamps closed_at and checks the round is still in the future with the signed witness lead intact. At domain 2^21 that replay runs for hours, so naming the round first asks the coordinator to predict their own hardware. Guess low and the whole replay is discarded. This is what caused the 2026-07-24 closure-timing incident, and it recurred on 2026-08-16 during a production-mode run that chose the round from the signed lead plus a margin, which is the only rule written down anywhere. The signed minimum_witness_lead_seconds states how long witnesses need; it says nothing about how long this host takes to replay. Those quantities are unrelated and only the first is recorded in the ceremony. Add --beacon-round-lead as an alternative to --beacon-round, deriving the round from closed_at plus the larger of the requested lead and the signed minimum, plus the publication safety margin that validateCloseCommitTime re-checks against a second clock sample. FirstQuicknetRoundAfter inverts QuicknetRoundTime; rounds are arithmetic from the pinned genesis, so this needs no network access. Deriving later commits to nothing sooner. The round is not published, signed, or observable until the closure record is written at the end, so the choice is indistinguishable to every observer, and under either ordering the round is in the future and its randomness does not yet exist. The derivation cannot live in the CLI. Only the package knows when the replay finished, and closed_at is sampled inside publishReplayedPhaseClose; a CLI deriving beforehand would be making the same blind guess. Two checks assumed an explicit round and are narrowed rather than removed. Retry recovery compares a published closure's round against the requested one, which a derived round has no operator intent to contradict, so it now applies only when a round was named; the existing record is authenticated and fully revalidated either way. The phase 2 round-reuse check runs before the replay, so a derived round is checked for reuse after derivation.
Replay progress was added on PhaseTranscriptPaths, which reaches every command whose paths come from the CLI's transcriptPaths helper: contribute, verify, close. The seal was missed. Its options carry a bare transcript root and it builds its own PhaseTranscriptPaths internally, so there was no Progress field to populate and the callback had nowhere to attach. The seal replays the entire phase and then applies the beacon contribution, so it does strictly more work than a close. On a production-mode K=21 run the close reported three progress lines and finished in 1h40m33s while the seal ran silently past 2h25m, which left the longest operation in the ceremony as the only long one that said nothing. SealPhase1FilesOptions now carries Progress and threads it into the paths it constructs, and the CLI attaches the same stderr reporter it already uses. The workflow integration helper asserts the callback fires during a seal so the wiring cannot be dropped again unnoticed. RecordBeaconFiles and InitializePhase2Files also take a bare transcript root but perform no replay, so they need nothing.
With the seal covered, phase 2 initialization was still silent past 2h20m on a production-mode K=21 run. This one is not a plumbing omission. InitializePhase2Files performs no replay, so the per-contribution callback has nothing to count: it verifies the sealed phase 1 commons, transforms them into circuit-specific parameters across the whole 2^21 domain, and publishes the result. The transform is a single monolithic computation inside gnark that exposes no progress of its own. ReplayProgress cannot describe that, and a fabricated percentage would be worse than silence. Add StageProgress, which reports entry into a named stage with a one-based index and a total, and report the three stages above. This is coarser than an index into work completed, deliberately. The expensive stage is opaque, so the honest signal is which stage is running rather than an invented fraction of it. It still separates running from hung and names what the operator is waiting on. Like ReplayProgress it carries no secret material and does not print; the CLI renders it to stderr, never stdout. The workflow integration helper asserts all three stages arrive in order.
mpc-finalization-evidence derived its credential at account 3, role 2, but PublicFinalizationEvidence.Validate accepts only the credential pinned in GoldenPublicCredentialHex, which is account 0, role 0. The two constants were added in the same commit and never agreed, so the command could not produce evidence any ceremony would accept: error: public evidence does not use the exact repository golden public vector This is on the only path to a finished ceremony. finalize complete requires the evidence, the evidence requires this command, and the failure is reachable only after finalize prepare has replayed both phases to derive the keys. On a K=21 production run that is over thirty hours before the mismatch surfaces. Every other reference in the tree already agrees on account 0, role 0: cmd/api, cmd/proof-tool, cmd/bench-native-prove, internal/verifier, the committed Plutus fixtures, and the pinned constant itself. The generator was the sole outlier. Correct the path, and name the master key, path and destination as constants instead of inlining them, so a test can assert they derive to the pinned golden vector. The drift was possible because two files held the same value independently with nothing comparing them.
mellowcroc
changed the title
Two of the three gaps recorded for the CLI redaction blocklist: writeDiagnostic previously performed no redaction, so only the error paths that remembered to call redactCLIError were covered and a new diagnostic call site could echo a command-line value silently. writeDiagnostic now takes argv and redacts the formatted message itself; there is no unredacted stderr outlet left to forget. Redaction is idempotent, so already-redacted messages pass through unchanged. Short argument values previously blanked matching substrings of unrelated numbers and words (a participant count of 3 blanked every digit 3 in the message). Values shorter than four characters are now replaced only as whole tokens. A plain length floor was tried before and reverted because validateID permits one-character key ids and skipping them entirely leaked the id verbatim; token matching keeps those redacted while leaving longer tokens that merely contain the short value readable.
The failover drill instructs the operator to run a read-only inspection and compare the derived next participant and index with the primary run card, but no such command existed; the only "inspect" was a rehearsal-script stage reading its own step markers rather than the signed chain. mpc-ceremony inspect reports ceremony identity and mode, per-phase accepted count and head record, the next scheduled participant and index (a pure function of the signed chain and the frozen policy order), closure, beacon, and seal state, and which referenced artifacts are present. It requires no signing key, writes nothing, and never replays contributions. Two depths, and the output states which one ran: the default verifies signatures and structure and checks artifact presence by size in seconds; --full additionally re-verifies every payload digest, attestation, erasure, and coordinator verification record through the same loaders the operational commands use. Neither depth re-runs the gnark replay; that remains the job of contribute, verify, and audit. Unlike every other command, inspect discovers the highest published chain file per phase. That is safe only because inspection is read-only: its output feeds no signing or verification decision, every discovered file is authenticated against the out-of-band trust anchor before being reported, and the chain filename index must equal the signed record count. The workflow helper exercises both depths at end of lifecycle from a real built binary so the running-software gate is the production gate.
The proposed-changes document was a working audit log; its open items are now tracked in the pull request description, and the fixed items are the pull request's own commits. Rewrite the two runbook references that pointed into it so no dangling links remain.
The signed minimum witness lead is measured from two different anchors: ValidateClose measures roundTime-closedAt and accepted equality, while witness receipts measure roundTime-observedAt with observedAt strictly after closedAt. A production close at exactly the signed minimum — the value the close help text sanctions — therefore left public witnesses a window of seconds (or zero, with an explicit round) in which a valid receipt could exist, and the contradiction surfaced only when the operational evidence bundle was assembled at release, with the round already pinned inside the signed closure and the phase unrecoverable. Production closes must now reserve ProductionWitnessObservationWindowSeconds (one hour) on top of the signed minimum, enforced consistently in ValidateClose, in the derived-round computation, and in the pre-publication commit-time guard via a single requiredCloseLead helper. Rehearsals are exempt: their leads are minutes and their witness receipts are same-host fixtures.
Three more instances of the audit-count defect class: a limit asserted in one layer that another layer exceeds, fail-closed but discovered only at release or decision time, after the work is complete. - Auditors: the final transcript stores audits in a list capped at 20, but enrollment, release, and decision accepted any count >= 2. A ceremony with 21 auditors completed every audit and then could not bundle them, and the dropped auditor was barred from the GO decision. Enrollment, definition validation, and the CLI now enforce 2..MaxAuditors, matching the transcript. - Audit order: release bundled audit reports in --audit-report flag order and froze that order into the transcript ID, while the decision requires its audits ascending by auditor ID and the transcript refs to match that order exactly. Reports passed in any other order signed a release for which no valid decision could exist. bundleAuditArtifacts now sorts by the auditor ID each record names before bundling. - Release tree ceiling: the decision capped the pinned artifact list at 4096 files while the bundle layers permit roughly four times that from governance evidence alone, so a thoroughly documented ceremony could sign a release the decision then rejected. The ceiling is now 32768, derived in a comment from the bundle layers' own maxima. Also corrects documentation drift from the earlier >= 2 auditors fix: the decision help no longer says "the two auditors" or shows exactly four signature flags, and the wrong-signer error no longer says "either audit".
The gate label "two-independent-audits" said "two" while the rule it names now accepts two or more. The label is part of the signed decision schema, so it is only renamable while no signed decision record exists — none does, in this tree or any published artifact. Rename it to "independent-audits" now, before the first production decision makes it permanent.
mellowcroc
changed the title
Fold the audit's fixes into the defense list, replace the stale known-gaps section with the four items actually open, and cut the prose to anchors: one line per defense, section intros dropped, fixed items collapsed into a single list. 634 lines to under 200.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit hardening of the ceremony package (
internal/mpcceremony) and its CLI. No cryptographic break was found; these changes close validation gaps, operational failure modes, and mutation-aliasing hazards surfaced by the audit.Identity and input validation
filippo.io/edwards25519, require canonical encoding, reject small-order points. A small-order key verifies signatures for any message, voiding every signature-based control for that identity; non-canonical encodings would also evade fingerprint-based duplicate detection.ContributionEnvironment.OS/.Architectureand audit findings used plain== "", so a single space satisfied "must not be empty."Fixes that keep a valid ceremony from stranding
--beacon-round-leadderives the committed round after the hours-long close replay instead of forcing the operator to predict replay duration (the 2026-07-24 and 2026-08-16 closure-timing failures). Not weaker: the round is unobservable until the closure record is written, and is still in the future under either ordering.inspectcommand — the failover drill referenced an inspection step that did not exist.inspectreports ceremony identity/mode, per-phase accepted count and head record, next scheduled participant and index, closure/beacon/seal state, and artifact presence. No signing key, no writes, no replay; two depths (metadata seconds /--fullre-verifies every record and digest), and the output states which ran.closed_atat close but fromobserved_at(strictly later) in witness receipts, so a close at exactly the sanctioned minimum left witnesses seconds — or zero — in which a valid receipt could exist, discovered only at release with the round pinned. Production closes now reserve a one-hour observation window on top of the minimum, enforced identically inValidateClose, the derived-round computation, and the pre-publication guard."two-independent-audits"said "two" while the rule now accepts two or more; renamed to"independent-audits"while no signed decision record exists to be invalidated by the change.writeDiagnosticnow redacts against argv itself, so no stderr outlet can leak a command-line value by omission; short values are replaced only as whole tokens, so a short argument no longer blanks unrelated digits while a short key id echoed verbatim is still caught (the plain length floor tried earlier leaked and was reverted).Mutation-aliasing hazards (gnark mpcsetup)
VerifyAndAcceptContributionpassed the retained candidate toVerify, which mutatesnext.Challenge, and later re-serialized that same object into the authoritative transcript; it now verifies a throwaway clone. Documented thatPhase1.Seal's returned commons alias the spent head's arrays and thatPhase2.Seal's returned keys retain evaluation slices, and stopped retaining the spent head in both seal callers.Docs
docs/mpc-ceremony-local-runbook.md) — build, rehearsal init, artifact anatomy, trust roots, beacon precedent survey.Known open items (tracked here, not in the tree)
ReclaimDeploymentScripts.hs:47, regression-tested), callers derive both from the same bytes, and the deployed manifest's hash checks out numerically. Residual: nothing bindsreclaim_global.script_hashto the VK at validation or runtime — only theformal/scriptsrecompile-and-compare do, and they are Preprod-pinned, so mainnet has no recompile gate. Fix location:ValidateReclaimDeployment, or lift the Preprod-only guard."two-independent-audits"was renamed to"independent-audits"while no signed decision record exists to freeze it.