feat(frost): fail closed on an incompatible libfrost_tbtc FFI contract version#4105
Merged
mswilkison merged 4 commits intoJun 22, 2026
Conversation
…t version The Go-side companion to the mirror's frost_tbtc_abi_version export. The bridge and the lib are linked at runtime via dlsym; a symbol that resolves but changed MEANING is silent corruption. This adds a fail-closed preflight that asserts the linked lib's FFI contract version before any contract-sensitive call. - Pure, fully-unit-tested rule (native_tbtc_signer_abi_version.go, untagged): lib abi_major must EQUAL requiredTBTCSignerABIMajor (1) and abi_minor must be >= requiredTBTCSignerABIMinMinor (0). Default-build tests cover every branch (wrong major either direction, too-old minor, higher-minor-additive-ok) via a parameterized helper. - cgo preflight (native_tbtc_signer_abi_version_frost_native.go): fetches the version via the new frost_tbtc_abi_version FFI, runs ONCE per process (sync.Once - the lib is process-global, not hot-swapped) before the first engine op. MISSING symbol keeps ErrNativeCryptographyUnavailable in the chain (explicit message; lib predates ABI versioning) so it SKIPS in dev and is FATAL under the require-cgo gate like any stale lib; PRESENT-but-wrong (malformed/wrong-major/too-old-minor) is ErrTBTCSignerABIIncompatible and fails loudly ALWAYS. - Gate: callBuildTaggedTBTCSignerOperation (the single chokepoint every request-taking engine op funnels through) runs the preflight first. The no-arg version/abi-version calls bypass it, so no recursion. - Bumps ci/frost-signer-pin.env to the mirror commit that adds frost_tbtc_abi_version (25e4f27), and adds that symbol to the CI gate's verified set, so the gate builds a lib whose ABI the preflight accepts. Design via Codex consult (major.minor; exact major + min minor; init-time fail-closed; explicit missing-symbol error; no upper bound; frozen {abi_major,abi_minor} surface). Builds across tag combos; untagged rule tests + cgo preflight test pass; require-cgo full run green against the ABI lib; no-lib skips; default suite unchanged; vet + gofmt clean. Co-Authored-By: Claude Opus 4.8 <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 |
Codex #4105 P2: the ABI preflight failed closed at the FFI op level, but the coarse engine path (signWithTBTCSignerCoarseEngine) treats engine-op errors as ordinary tbtc-signer failures and calls fallbackTBTCSignerLegacySigning - which, if the material carries a legacy private key share, signs via legacy tECDSA anyway. So an incompatible lib was NOT actually failing closed end to end: the ABI error got swallowed into a legacy signature. Fix at the single chokepoint - the top of signWithTBTCSignerCoarseEngine, before any fallback: a guard that fails closed on ErrTBTCSignerABIIncompatible (a PRESENT-but-wrong lib). A MISSING/absent lib is ErrNativeCryptographyUnavailable, NOT this sentinel, and still falls back to legacy as the transitional design intends - only a responding-but- incompatible lib is refused. The check is a build-split helper (tbtcSignerABIIncompatibility: real cgo preflight when linked, no-op otherwise), indirected through a var (tbtcSignerABIIncompatibilityCheck) so the no-fallback behavior is testable in the frost_native build. New test (mirrors _InvalidAttemptPolicy_DoesNotFallback): injects an incompatible ABI verdict + a valid attempt + a legacy-key-share payload, asserts Sign returns ErrTBTCSignerABIIncompatible, no signature, and ZERO fallback events. Bumps the signer pin to the mirror tip (6e3718b, which also adds the header decl). Builds across all tag combos; new test passes; existing fallback tests unchanged; cgo vet + gofmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The P2-2 fail-closed test (a present-but-incompatible lib must not fall back to legacy signing) is frost_native-tagged and uses the test seam, so it does not need an actually-incompatible lib - but it was matched by neither standard CI (no frost_native tags) nor the cgo gate's TestRealCgoInteractiveSigning -run filter, so its proof ran only locally. This gate is the only CI that builds these tags; broaden -run to include it so the no-fallback guarantee is enforced in CI alongside the real-crypto suite. Verified locally: the broadened -run matches both groups and all pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codex #4105 re-review P2: Go's json.Unmarshal silently zero-fills a missing field, so a present-but-partial frost_tbtc_abi_version response like {"abi_major":1} left abi_minor at 0 - and since the required minimum minor is also 0, the compatibility check ACCEPTED it. A broken/partial lib could thus bypass the fail-closed guard. Extract the decode into a pure parseTBTCSignerABIVersion (untagged, unit-testable in the default build) that uses pointer fields to distinguish "absent" from a legitimate zero and rejects a payload missing abi_major and/or abi_minor as ErrTBTCSignerABIIncompatible. Malformed JSON is rejected too; extra/unknown fields are still tolerated (an additive minor bump may add fields old bridges ignore). The cgo assertTBTCSignerABICompatible now calls it instead of a lenient inline Unmarshal. Tests: parser accepts valid (incl. present zero minor) + extra fields; rejects missing abi_minor, missing abi_major, empty object, malformed json, non-object. Builds across tag combos; cgo preflight still passes against the real lib; gofmt + vet clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mswilkison
merged commit 402911a
into
feat/frost-schnorr-migration-scaffold
16 of 17 checks passed
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.
What
The Go-side companion to the mirror's
frost_tbtc_abi_versionexport (#4104). The bridge andlibfrost_tbtcare linked at runtime viadlsym; a symbol that resolves but changed meaning is silent corruption. This adds a fail-closed preflight that asserts the linked lib's FFI contract version before any contract-sensitive call.Design (Codex-validated)
major.minor): libabi_majormust equal the bridge's required major (a higher major broke something newer; a lower major is too old), and libabi_minormust be >= the required minimum minor (additive is backward-compatible; higher minor's extras are ignored). Required =1.0.ErrNativeCryptographyUnavailablein the chain (explicit message): skips in dev, fatal under the require-cgo gate — same as any stale lib.ErrTBTCSignerABIIncompatible, fails loudly always. A node must not sign through a lib whose contract diverges.sync.Once; the lib is process-global, not hot-swapped), gated atcallBuildTaggedTBTCSignerOperation— the single chokepoint every request-taking engine op funnels through. The no-arg version calls bypass it (no recursion).Tests
🤖 Generated with Claude Code