Verify TAPSIGNER factory certificates

[praveenperera]

Summary

• verify each TAPSIGNER certificate against a Coinkite factory root during connection
• reject development certificates before Cove sends PIN-bearing commands
• keep only verified TAPSIGNER instances in the reader state
• add a counterfeit-card transport test that confirms early rejection

Root cause

Cove converted the unauthenticated card status response into a usable TAPSIGNER and did not call the certificate verification operation provided by rust-cktap. A counterfeit card could therefore reach setup, import, or signing operations.

Impact

Cove now checks card authenticity before it accepts a TAPSIGNER. A card with an invalid, missing, or development certificate cannot receive authenticated commands.

Verification

• just fmt
• just clippy
• counterfeit TAPSIGNER regression test
• git diff --check

Summary by CodeRabbit

• Security

  • TapSigner cards are now authenticated during initialization.
  • Development and counterfeit certificates are rejected before commands are sent.

• Bug Fixes

  • Improved protection against untrusted cards during reader setup.
  • Added verification coverage for certificate rejection.

[coderabbitai]

📝 Walkthrough

Walkthrough

TapSigner reader initialization now verifies card certificates and rejects counterfeit or development factory certificates before use. New tests confirm rejection occurs before authenticated commands, with added development dependencies for certificate test data.

Changes

TapSigner verification

Layer / File(s)	Summary
Verified TapSigner connection rust/src/tap_card/tap_signer_reader.rs	VerifiedTapSigner validates the card type and certificate before TapSignerReader stores the connection.
Counterfeit-card validation rust/src/tap_card/tap_signer_reader.rs, rust/Cargo.toml	Tests verify counterfeit-card rejection after three transport calls and before authenticated commands. Development dependencies support the test data.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TapSignerReader
  participant VerifiedTapSigner
  participant TapSignerCard
  participant CertificateVerifier
  TapSignerReader->>VerifiedTapSigner: connect
  VerifiedTapSigner->>TapSignerCard: request status and certificate
  VerifiedTapSigner->>CertificateVerifier: verify certificate
  CertificateVerifier-->>VerifiedTapSigner: authentication result
  VerifiedTapSigner-->>TapSignerReader: verified signer or error

Loading

Suggested reviewers: sandipmandal25

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: verification of TAPSIGNER factory certificates.
Description check	✅ Passed	The description explains the change, root cause, impact, and verification steps, but omits the template checklist and platform coverage.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch tapsigner-med

---

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.}

[coderabbitai]

🧹 Nitpick comments (1)

rust/src/tap_card/tap_signer_reader.rs (1)

86-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Separate result bindings from control flow.

Insert blank lines after lines 87 and 98. This separates each result binding from the following match or if.

As per coding guidelines, “Add blank lines between logical steps in function bodies, including after setup or result bindings before control flow.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@rust/src/tap_card/tap_signer_reader.rs` around lines 86 - 99, In
TapSignerReader::connect, add blank lines after the transport-to-card result
binding and after the certificate root result binding, separating each setup
step from its following match or if control flow.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@rust/src/tap_card/tap_signer_reader.rs`:
- Around line 86-99: In TapSignerReader::connect, add blank lines after the
transport-to-card result binding and after the certificate root result binding,
separating each setup step from its following match or if control flow.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1cef044d-dd8b-4eac-8c3a-63960e2b9855

📥 Commits

Reviewing files that changed from the base of the PR and between b7aa9fd and ef5c549.

⛔ Files ignored due to path filters (1)

• rust/Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (2)

• rust/Cargo.toml
• rust/src/tap_card/tap_signer_reader.rs

[greptile-apps]

Greptile Summary

The PR authenticates TAPSIGNER cards against Coinkite factory roots during reader construction and rejects development certificates before retaining the card or issuing PIN-bearing commands.

• Introduces a VerifiedTapSigner wrapper that performs certificate verification during connection.
• Keeps only verified TAPSIGNER instances in reader state and preserves existing card-type rejection.
• Adds a counterfeit-card transport regression test and its test-only serialization dependencies.

Confidence Score: 5/5

The PR appears safe to merge and strengthens the TAPSIGNER authentication boundary before sensitive commands are dispatched.

Reader construction now validates the card’s factory certificate, rejects development roots, and stores only the verified wrapper; no concrete changed-code failure remains.

Important Files Changed

Filename	Overview
rust/src/tap_card/tap_signer_reader.rs	Adds certificate-gated TAPSIGNER construction, rejects development roots, and tests counterfeit rejection before authenticated commands.
rust/Cargo.toml	Adds ciborium and serde_bytes as test-only dependencies for the counterfeit transport fixture.
rust/Cargo.lock	Records the two new direct development dependencies without changing the pre-existing advisory-flagged package versions.

Sequence Diagram

sequenceDiagram
  participant UI as Mobile UI
  participant Reader as TapSignerReader
  participant Card as NFC TAPSIGNER
  participant Root as Coinkite Factory Roots
  UI->>Reader: create reader(command)
  Reader->>Card: status
  Card-->>Reader: card type and public key
  Reader->>Card: certificate chain and check challenge
  Card-->>Reader: certificate chain and signature
  Reader->>Root: validate factory certificate
  alt Production certificate is valid
    Root-->>Reader: verified production root
    Reader-->>UI: verified reader
    UI->>Reader: run PIN-bearing command
    Reader->>Card: authenticated command
  else Invalid, missing, or development certificate
    Root-->>Reader: reject
    Reader-->>UI: connection error
  end

Loading

_{Reviews (1): Last reviewed commit: "Verify factory certificate on TapSigner ..." | Re-trigger Greptile}