Skip to content

fix(tbtc/signer): fail closed on unsafe share refresh#4155

Merged
mswilkison merged 3 commits into
extraction/frost-signer-mirror-2026-05-26from
fix/signer-refresh-safety-2026-07-13
Jul 13, 2026
Merged

fix(tbtc/signer): fail closed on unsafe share refresh#4155
mswilkison merged 3 commits into
extraction/frost-signer-mirror-2026-05-26from
fix/signer-refresh-safety-2026-07-13

Conversation

@mswilkison

@mswilkison mswilkison commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Stack

Rust signer review-fix stack 1 of 2.

  • Base: extraction/frost-signer-mirror-2026-05-26 (#4005)
  • Follow-up: #4156 — admission-attestation validation

What changed

  • Retain the RefreshShares symbol but fail closed with terminal code cryptographic_refresh_not_supported until a multi-round, zero-constant FROST refresh protocol exists.
  • Bump the FFI contract from ABI 3.3 to 4.0 so ABI-3 consumers reject the incompatible success-to-error response change during startup negotiation.
  • Remove the SHA-256 pseudo-refresh path, its process-local pending-operation machinery, and tests that encoded invalid refresh-success semantics.
  • Prove rejected refresh requests leave DKG key packages, the public package, lifecycle state, and persisted bytes unchanged.
  • Anchor the first refresh deadline and overdue metric to durable DKG creation time.
  • Treat metadata from the retired synthetic refresh stub as non-authoritative: it cannot postpone cadence, increment valid refresh counts, or claim key continuity.
  • Mark persisted synthetic refresh-only sessions without a DKG anchor immediately overdue in both status and telemetry, including when the system clock saturates at the Unix epoch.

Review findings addressed

  • P1: implement cryptographic refresh instead of hashing shares — the unsafe one-shot operation now rejects and returns no replacement material.
  • P2: anchor the first refresh deadline to DKG creation — status and telemetry share the same durable deadline helper.
  • P2: bump the ABI for the incompatible refresh response — the endpoint now reports ABI 4.0 with its minor version reset to zero.
  • P1: mark unanchored legacy refresh sessions overdue — untrusted refresh artifacts now resolve to an explicit, unconditionally overdue sentinel instead of a sliding deadline.

The C symbol, request shape, response envelope, and reserved response type remain present; the major version makes the changed status and JSON semantics explicit.

Verification

  • cargo fmt --manifest-path pkg/tbtc/signer/Cargo.toml -- --check
  • cargo check --locked --manifest-path pkg/tbtc/signer/Cargo.toml --all-targets
  • cargo clippy --locked --manifest-path pkg/tbtc/signer/Cargo.toml --all-targets -- -D warnings
  • cargo test --locked --manifest-path pkg/tbtc/signer/Cargo.toml
  • cargo test --locked --manifest-path pkg/tbtc/signer/Cargo.toml formal_verification_
  • pkg/tbtc/signer/scripts/run_phase5_chaos_suite.sh
  • git diff --check

The TLA+ runner was not available locally because this machine has no Java runtime; CI remains the authoritative model-check gate.

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 20da1ca8-fc0f-4d8b-9cd0-51c33a0fc653

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/signer-refresh-safety-2026-07-13

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

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

@mswilkison
mswilkison merged commit 43a0d25 into extraction/frost-signer-mirror-2026-05-26 Jul 13, 2026
20 checks passed
@mswilkison
mswilkison deleted the fix/signer-refresh-safety-2026-07-13 branch July 13, 2026 21:41
mswilkison added a commit that referenced this pull request Jul 13, 2026
## Stack

Rust signer review-fix stack 2 of 2.

- Depends on:
[#4155](#4155)
- Ultimate base: `extraction/frost-signer-mirror-2026-05-26`
([#4005](#4005))

## What changed

- Reject an empty or whitespace-only policy
`required_attestation_status` with
`required_attestation_status_missing`.
- Reject an empty or whitespace-only candidate `attestation_status` with
`attestation_status_missing`.
- Compare statuses only after both normalized values are non-empty,
avoiding a misleading mismatch reason for missing data.
- Preserve case-insensitive, whitespace-normalized matching for
legitimate non-empty values.

## Review finding addressed

- P2: reject empty attestation statuses before comparing them — an empty
policy and empty candidate can no longer normalize to equality and pass
admission.

## Verification

- `cargo fmt --manifest-path pkg/tbtc/signer/Cargo.toml -- --check`
- `cargo check --locked --manifest-path pkg/tbtc/signer/Cargo.toml
--all-targets`
- `cargo clippy --locked --manifest-path pkg/tbtc/signer/Cargo.toml
--all-targets -- -D warnings`
- `cargo test --locked --manifest-path pkg/tbtc/signer/Cargo.toml`
- `cargo test --locked --manifest-path pkg/tbtc/signer/Cargo.toml --bin
admission_checker attestation`
- `git diff --check`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant