feat(tbtc/signer): install TBTC_SIGNER_* knobs via init-time FFI config#4037
Conversation
Post-merge follow-up #3 from the June 2026 review stack: shrink the ops/audit surface by letting the host install the signer's operational configuration once at startup (frost_tbtc_init_signer_config) instead of exporting ~40 TBTC_SIGNER_* environment variables. Design (parity by construction): - every operational env read now goes through one chokepoint, engine::signer_env_var; with no config installed it falls through to the process environment, so existing env-driven behavior (and the entire pre-existing test suite) is unchanged - an installed config wins wholesale: the environment is no longer consulted for covered knobs, and an unset field means the built-in default - no per-knob source mixing - typed InitSignerConfigRequest (field = lowercased env suffix) converts to the same canonical strings the existing parsers consume, so every clamp/warn/reject path runs unchanged on identical inputs - deny_unknown_fields: a typo'd knob fails the init instead of silently running on defaults; enforcement-gated policy combinations (admission, firewall, auto-quarantine) are validated at install with rollback - re-init is idempotent for an identical request, rejected on conflict - secrets never ride the config FFI: TBTC_SIGNER_STATE_ENCRYPTION_KEY_HEX stays on the dedicated env/command key-provider channel (deliberate std::env::var exception, commented) - deletes lib.rs's duplicated profile/truthy parsing in favor of the engine's single implementation Verified: fmt --check; clippy --all-targets -D warnings; full suite 235 passed + 1 ignored / 24 / 1 (11 new tests incl. FFI round-trip); --features bench-restart-hook builds; chaos suite and formal_verification_ filter pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Addresses the converged Codex/Gemini review finding plus two findings from my own review of #4037: - candidate configs are now validated through a thread-local resolver override visible only to the validating thread, and published to the global slot only after validation succeeds. Previously the candidate was installed first and rolled back on failure, so a concurrent identical init could report idempotent success while the twin rolled the slot back to None, and concurrent readers could briefly act on a config that never legally installed. Both impossible now: failed init has no observable side effects, and idempotent success is only ever reported against a validated, installed config. - init now validates state_file_path(): a production config (explicit, or by profile-omission default) without state_path fails at init instead of installing and then failing at first state access with an env-var-oriented message; the state-path error now also names the state_path config field. - new end-to-end test: installed config's state_path is honored by run_dkg persistence after a process (re)start, and the existing in-process state-path-switch refusal is pinned as the contract for installing a config after state has been touched. - README: do not inline key material into state_key_command (the command string rides the config FFI); documented the no-side-effects init guarantee and the production state_path requirement. Suite: 237 passed + 1 ignored / 24 / 1; clippy -D warnings, fmt, chaos suite, bench-restart-hook feature build all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review follow-ups addressed in 9226be1 (findings from three reviews: mine, Codex, Gemini — the latter two converged on one root cause):
Note for test authors: configs in tests must now be explicit about Suite: 237 passed + 1 ignored / 24 / 1; clippy |
Addresses Codex's re-review P2: the init validated state_path but not the adjacent key-provider knobs, so a production config that omitted state_key_provider (wholesale-defaulting to the env provider, which production forbids) - or that selected the command provider without a command - installed successfully and then failed at the first state access. - extract the structural head of state_encryption_key_material into resolve_state_key_provider_plan (provider selection, the production env-provider prohibition, command-spec presence; error strings unchanged) and have both the runtime key path and init validation consume it - one source of truth, no drift - init validation rejects: production defaulting to the env provider, command provider without state_key_command, unknown provider values; all WITHOUT reading the secret or executing the key command (pinned by a test whose key command points at a nonexistent binary) - README documents that production configs must carry the command key-provider pair Suite: 241 passed + 1 ignored / 24 / 1 (4 new tests); clippy -D warnings, fmt, chaos suite, bench-restart-hook build all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Re-review responses — Gemini: approved, no action. Codex's P2 was valid and is fixed in The finding was my own state-path pattern applied to a knob family the previous fix only partially covered: a production config omitting Fix: the structural head of Suite: 241 passed + 1 ignored / 24 / 1 (4 new tests); clippy |
Addresses Codex's third-round P2 (the family member my own review had deferred): production forces the provenance gate, so a production config without a complete, verifiable attestation set installed successfully and then failed every protected operation. - validate_candidate_config now runs enforce_provenance_gate(): self- gating (no-op when unenforced, so dev configs are unaffected), reads only candidate values plus local crypto - no secrets, no command execution, no network. Full verification at init (status, payload signature against trust root, runtime-version minimum, TTL); runtime calls still re-check, so an init-time pass does not exempt TTL aging. - production-config tests now carry a complete signed attestation (reusing the existing build_signed_provenance_attestation fixture); new tests pin: production-without-attestation rejected at init, enforced-gate-with-unparseable-trust-root rejected, and a complete production config (state path + command key provider + valid attestation + min version) installs. - README documents the production attestation requirement and the TTL caveat. Suite: 244 passed + 1 ignored / 24 / 1 (3 new tests); clippy -D warnings, fmt, chaos suite all green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Codex round-3 P2 — valid, fixed in
New tests pin: production-without-attestation rejected at init (message names the missing knob), enforced-gate-with-unparseable-trust-root rejected, and a complete production config (state path + command key provider + valid signed attestation + min version) installs — the production-config tests now carry real signed attestations via the existing Suite: 244 passed + 1 ignored / 24 / 1; clippy With this, the install-then-fail-late family is closed for every knob the config can express: state path, key provider/command, admission/firewall/auto-quarantine combos, and the provenance gate. The remaining deliberate-lazy items are external-world checks (KMS reachability, secret presence, TTL aging) that no init-time validation can honestly promise. |
f77154b
into
extraction/frost-signer-mirror-2026-05-26
…ence (#4040) Post-merge follow-up **#4** from the June 2026 review stack (#4028–#4035): replace `json.Marshal` as the canonical signed-bytes encoding for evidence snapshots/bundles — explicitly scheduled to land **before Phase 7 wiring ossifies the format**. (Items 2 and 3 landed as #4036/#4037 on the mirror branch; this is the Go-side sibling on the scaffold branch.) ## Why now The RFC-21 Layer B evidence signatures were computed over canonical JSON. That byte stability is a Go-implementation accident — field-order-stable `encoding/json` output — not a portable contract. The moment Phase 7 wires evidence verification into the Rust signer (or any second implementation appears), every verifier would need to replicate Go's exact JSON emission. No persisted or cross-component evidence exists yet, so the format can still change for free. ## Design: sign what you transmit, verify what you received New `pkg/frost/roast/gen/pb/evidence.proto`: - A snapshot travels as `SignedLocalEvidenceSnapshot{body, operator_signature}` where `body` is the serialized `LocalEvidenceSnapshotBody` — the operator signs those exact bytes. - A transition message travels as `SignedTransitionMessage{body, coordinator_signature}` whose `TransitionMessageBody` embeds every member's signed snapshot envelope **verbatim** (`repeated bytes signed_snapshots`) — the coordinator attests to the exact signed snapshots it assembled, in order. - Producers marshal a body **exactly once**, at signing time, and cache it; parsed messages retain received body/envelope bytes verbatim; verification always runs over exact received bytes. **Nothing in the evidence chain is ever re-encoded**, so signature validity never depends on any serializer's canonical form — across protobuf library versions or across languages. This deliberately sidesteps protobuf's own caveat that deterministic serialization is not canonical across implementations. - `Marshal` of a received message returns the received envelope verbatim — evidence bytes survive re-broadcast, including wire-legal but non-canonical encodings (pinned by a handcrafted reversed-field-order test). - `CanonicalSnapshotBytes`/`CanonicalBundleBytes` → `SignableBytes()` accessors; the coordinator's first-write-wins conflict check now compares exact signed bytes. ## Tests - Existing suite migrated off JSON fixtures: test-only encode helpers bypass production signing so every structural-rejection path (zero sender, bad hash length, unsorted/duplicate entries, oversize caps, bundle ordering/hash-binding) is still exercised at the wire level. - New `wire_test.go` pins the format's core properties: byte-preservation through unmarshal→re-marshal, verbatim snapshot-envelope embedding inside bundle bodies, producer-signed bytes == receiver-verified bytes, non-canonical-encoding survival, tampered-body verification failure. - `go build ./...`, `go vet`, `gofmt` clean; frost + tbtc package tests green. Generated with protoc 33.4 / protoc-gen-go v1.36.3 (matches the go.mod protobuf runtime v1.36.3). ## Docs RFC-21 "Evidence message format" decision rewritten: signed-body protobuf envelopes, with the retirement rationale for canonical JSON recorded. ## Notes for reviewers - The in-memory model types (`LocalEvidenceSnapshot`, `TransitionMessage`) are unchanged apart from two unexported byte caches; all call sites kept their shapes. - Immutability contract: evidence fields must not be mutated after `SignableBytes()` is first computed (documented on the cache fields); the aggregation flow already treats snapshots as immutable post-receipt. - Phase 7 cross-language note: the Rust signer will verify operator/coordinator signatures over `body` bytes and parse them with any protobuf implementation — no canonicalization requirements transfer. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…ence (#4042) Post-merge follow-up **#5** from the June 2026 review stack — the remaining half of #4033's item: cite the exact external audit report and version range covering the pinned FROST stack in the readiness/rollout docs. Also records the attestation-rotation operational requirement that #4037's review surfaced. ## Audit status (researched against upstream sources, 2026-06-12) The new "Cryptographic Dependency Audit Status" section in `roast-phase-5-security-rollout-gates.md` records, with citations: - **NCC Group, "Zcash FROST Security Assessment"** (report 2023-10-20): audited **v0.6.0** (commit `5fa17ed`) of `frost-core` + five ciphersuites — trusted-dealer and DKG key generation plus signing; all findings addressed and re-reviewed. - The upstream README's explicit exclusion, quoted verbatim: *"This does not include frost-secp256k1-tr and rerandomized FROST."* - **Least Authority's Q1 2025 FROST Demo audit** covered `frost-client`/`frostd` tooling only — not the library crates this signer consumes. - No 2.x/3.x release notes mention further audit coverage. **The honest bottom line, now on the record:** the exact ciphersuite this signer uses for production signatures (`frost-secp256k1-tr =3.0.0`, released 2025-04-23) and the v0.6.0 → 3.0.0 evolution of `frost-core` have **no external audit coverage**. Gate 1 sign-off must therefore either commission/await an audit covering that range (the checklist item-8 "audit as ECDSA-retirement merge gate" decision) or record a written, canary-scoped risk acceptance. The section gives that team decision its factual basis instead of letting "FROST was audited" stand unqualified. ## Attestation rotation cadence (runbook prerequisite 6) From #4037's design: init-time config is immutable for the process lifetime and attestation TTL caps at 7 days, so production signers must restart with fresh attestation material within every window — rollout stage scheduling has to absorb that cadence. Live re-attestation without restart is deliberately unsupported (it would need a dedicated narrow FFI; general config mutation reopens the split-brain risk the immutable design closed). Doc-only change; no code. Sources verified via the upstream README, NCC's published report PDF, zfnd.org announcements, and the ZcashFoundation/frost releases page. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Post-merge follow-up #3 from the June 2026 review stack (#4028–#4035; #4036 landed the engine split that stages this change): move the
TBTC_SIGNER_*env-var surface into an init-time FFI config struct, shrinking the ops/audit surface from ~40 scatteredstd::env::varreads to one explicit, validated installation at startup.What this adds
New FFI entry
frost_tbtc_init_signer_config(request_ptr, request_len)taking a typed JSONInitSignerConfigRequest(40 optional fields; field name = lowercasedTBTC_SIGNER_*suffix). The host installs it once at startup.Semantics
deny_unknown_fieldsrejects typo'd knobs; enforcement-gated policy combinations (admission, signing-policy firewall, auto-quarantine) are validated at install by running the same loaders the runtime gates use, with rollback on rejection — a misconfigured signer fails at startup, not at first signing.TBTC_SIGNER_STATE_ENCRYPTION_KEY_HEXis read exclusively from the dedicated env/command key-provider channel even when a config is installed (the one deliberatestd::env::varleft outside the chokepoint, commented at the read).engine::signer_env_varfalls through to the process environment — existing hosts and the entire pre-existing test suite run unchanged; non-development profiles log a one-time warning suggesting the init FFI.Why parity is safe by construction
The typed request converts to the same canonical strings the existing env parsers consume (
"true"/"false", decimal ints, comma-joined identifier lists), and every existing clamp/warn/reject path runs unchanged on identical inputs. The diff swapsstd::env::var(X)→signer_env_var(X)at 31 sites and changes nothing else about how values are interpreted. Also deleteslib.rs's duplicated profile/truthy parsing in favor of the engine's single implementation.Thanks to #4036, this lands as one new ~400-line module (
engine/init_config.rs) plus one-line touches acrossconfig/lifecycle/persistence/policy/provenance/state— not an 18k-line-file churn.engine/config.rsremains the single home of the env-name constants.Verification
cargo fmt --check✅;cargo clippy --all-targets -- -D warnings✅reset_for_testsclearing,deny_unknown_fields, list/bool canonicalization, and an FFI round-trip--features bench-restart-hookbuilds; chaos suite (5/5--exactpaths) andformal_verification_filter passinclude/frost_tbtc.hgains the symbol; README documents the contractNotes for reviewers
🤖 Generated with Claude Code