Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(crypto): error if incorrect ledger public key #19691

Merged
merged 6 commits into from
Mar 13, 2024
Merged

fix(crypto): error if incorrect ledger public key #19691

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b607615
fix: error if incorrect ledger public key
rootulp Mar 7, 2024
c7ccfba
test: SignWithLedger
rootulp Mar 11, 2024
9592b02
fix: lint
rootulp Mar 11, 2024
f1d03e1
docs: changelog entry
rootulp Mar 11, 2024
1e2a1c9
Merge branch 'main' into rp/19690
rootulp Mar 11, 2024
23e2364
Merge branch 'main' into rp/19690
tac0turtle Mar 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crypto/keyring/keyring.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,6 +634,14 @@ func SignWithLedger(k *Record, msg []byte, signMode signing.SignMode) (sig []byt
if err != nil {
return nil, nil, err
}
ledgerPubKey := priv.PubKey()
pubKey, err := k.GetPubKey()
if err != nil {
return nil, nil, err
}
if !pubKey.Equals(ledgerPubKey) {
return nil, nil, fmt.Errorf("the public key that the user attempted to sign with %v does not match the public key on the ledger device %v", pubKey.String(), ledgerPubKey.String())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added check in the SignWithLedger function correctly addresses the issue of mismatched public keys during the signing process with a Ledger device. This change enhances security and user experience by providing immediate feedback on the mismatch. However, consider the following improvements for clarity and maintainability:

  • Extract the public key comparison logic into a separate, well-named function to improve readability and testability.
  • Use sdkerrors.Wrapf for error handling to maintain consistency with the rest of the Cosmos SDK error handling practices.
  • Include more detailed error messages that guide users on how to resolve the mismatch issue.
- return nil, nil, fmt.Errorf("the public key that the user attempted to sign with %v does not match the public key on the ledger device %v", pubKey.String(), ledgerPubKey.String())
+ return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidPubKey, "Public key mismatch: attempted to sign with %s, but Ledger device contains %s. Please ensure the correct Ledger device is used or the --from flag is set correctly.", pubKey.String(), ledgerPubKey.String())
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
ledgerPubKey := priv.PubKey()
pubKey, err := k.GetPubKey()
if err != nil {
return nil, nil, err
}
if !pubKey.Equals(ledgerPubKey) {
return nil, nil, fmt.Errorf("the public key that the user attempted to sign with %v does not match the public key on the ledger device %v", pubKey.String(), ledgerPubKey.String())
}
ledgerPubKey := priv.PubKey()
pubKey, err := k.GetPubKey()
if err != nil {
return nil, nil, err
}
if !pubKey.Equals(ledgerPubKey) {
return nil, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidPubKey, "Public key mismatch: attempted to sign with %s, but Ledger device contains %s. Please ensure the correct Ledger device is used or the --from flag is set correctly.", pubKey.String(), ledgerPubKey.String())
}


switch signMode {
case signing.SignMode_SIGN_MODE_TEXTUAL:
Expand Down
Loading