fix: verify signature against cached public key after signing (#2571)#2619
Open
NicoMoli wants to merge 2 commits into
Open
fix: verify signature against cached public key after signing (#2571)#2619NicoMoli wants to merge 2 commits into
NicoMoli wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a lightweight post-signature ed25519 verification guard for cached-public-key signers (Ledger + Secure Store) so the CLI fails locally with a clear error when the cached key drifts from the actual signing key.
Changes:
- Introduces
validation::verify_signature(ed25519-dalek strict verification) plus unit tests. - Wires post-sign verification into
SecureStoreEntry::{sign_tx_hash, sign_payload}when a cached public key is present. - Wires post-sign verification into
LedgerEntry::{sign_tx_hash, sign_payload}for the cached-pubkey path, and plumbs validation errors through signer/ledger error enums.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/soroban-cli/src/signer/validation.rs | Adds signature verification helper, a dedicated error, and unit tests. |
| cmd/soroban-cli/src/signer/mod.rs | Plumbs validation errors and adds post-sign verification for Secure Store signing paths. |
| cmd/soroban-cli/src/signer/ledger.rs | Adds post-sign verification for Ledger signing paths when a cached public key is used. |
Comment on lines
+3
to
+11
| #[derive(thiserror::Error, Debug)] | ||
| pub enum Error { | ||
| #[error( | ||
| "the produced signature does not match the cached public key {public_key}; \ | ||
| the cached key may be stale — check the correct device is connected / the alias's \ | ||
| hd-path is right, or re-add the key" | ||
| )] | ||
| SignatureMismatch { public_key: String }, | ||
| } |
Author
There was a problem hiding this comment.
I have applied a partial improvement for this comment, I believe it is good enough
Comment on lines
+24
to
+30
| use ed25519_dalek::{Signature, VerifyingKey}; | ||
| let mismatch = || Error::SignatureMismatch { | ||
| public_key: format!("{public_key}"), | ||
| }; | ||
| let vk = VerifyingKey::from_bytes(&public_key.0).map_err(|_| mismatch())?; | ||
| let sig = Signature::from_slice(signature).map_err(|_| mismatch())?; | ||
| vk.verify_strict(message, &sig).map_err(|_| mismatch()) |
Author
There was a problem hiding this comment.
Same here, as previous comment
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
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
Add a post-sign ed25519 verification step to the Ledger and Secure Store signing paths. After producing a signature, we verify it against the signer's cached public key and fail locally with a clear error if they don't match. Covers
LedgerEntry::sign_tx_hash,LedgerEntry::sign_payload,SecureStoreEntry::sign_tx_hash, andSecureStoreEntry::sign_payload.The check is gated on a cached public key actually being present, so the cacheless
--sign-with-ledgerflow (which fetches the pubkey live) is unaffected.Closes #2571.
Why
Both
LedgerEntry(added in #2569) andSecureStoreEntrycarry a cached/derived public key alongside the underlying signing primitive. TheSignatureHint(and, for Soroban auth entries, the embeddedpublic_keyScVal) is derived from that cached key, while the actual signature is produced live on the device/keychain.If the cached key ever drifts from what the underlying signer actually produces (corrupted TOML, wrong device plugged in, alias pointing at a different hd-path, schema migration glitch), the transaction carries a hint/pubkey that disagrees with its signature. Today that only surfaces as an opaque Horizon rejection after submission. ed25519 verification is microseconds, so catching this locally with a clear message is essentially free.
SignerKind::LocalandSignerKind::Labare intentionally out of scope: their public key is derived from the same in-process primitive that signs, so drift is not possible.Known limitations
The verification only runs when a cached public key is present. The cacheless
--sign-with-ledgerpath fetches the public key live from the same device that signs, so there is nothing to drift from and no check is needed there.