fix(crypto): error if incorrect ledger public key

[rootulp]

Closes #19690

Summary by CodeRabbit

• Bug Fixes
  • Enhanced security by ensuring the public key used for signing transactions matches the public key on the ledger device. Now throws an error if there's a mismatch.
• Refactor
  • Improved organization and efficiency by moving functions and interfaces to more appropriate packages within the types module and removing deprecated functions.
• Chores
  • Implemented various updates and fixes across different modules to enhance configuration, server context, and calculation methods.

[coderabbitai]

Walkthrough

Walkthrough

The update enhances the SignWithLedger function in keyring.go by adding a validation check to ensure the public key used for signing transactions aligns with the public key stored on the Ledger device. This improvement directly tackles the issue where transactions could be signed with an incorrect Ledger without alerting the user, thereby bolstering security and user experience by preventing mismatches and potential errors in the multi-signature process.

Changes

File Path	Change Summary
crypto/keyring/keyring.go	Added a validation check to ensure the public key used for signing matches the public key on the Ledger device.
types/types.go	Moved functions and interfaces to different packages and removed deprecated functions.
config/config.go server/context.go calculation/methods.go	Various updates and fixes related to configuration, server context, and calculation methods.

Assessment against linked issues

Objective	Addressed	Explanation
Bug Identification and Expected Behavior (#19690)	✅
Detailed Reproduction Steps (#19690)	❌	The PR focuses on code changes rather than including reproduction steps.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

---

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit-tests for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit tests for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit tests.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

[coderabbitai]

Review Status

Actionable comments generated: 1

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between a1e3a85 and b607615.

Files selected for processing (1)

• crypto/keyring/keyring.go (1 hunks)

[coderabbitai]

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())
	}

[sontrinh16]

side question, in #14460, why aren't we verifying the signature against the pubkey getting from Record

[julienrbrt]

Can we get a changelog?

[rootulp]

side question, in #14460, why aren't we verifying the signature against the pubkey getting from Record

Great question! I have no idea why that doesn't use the pubkey from Record. I think it should. I haven't manually tested Zaki's PR but after re-reading it I expect it wouldn't catch the scenario where a user attempts to sign with a pub key that doesn't belong to the plugged in Ledger.

Note for reviewers: I have manually tested this PR and confirmed it catches the scenario where a user attempts to sign with a pub key that doesn't belong to the plugged in Ledger. IMO we shouldn't merge this until we have a unit test.

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between b607615 and c7ccfba.

Files selected for processing (2)

• crypto/keyring/keyring.go (1 hunks)
• crypto/keyring/keyring_ledger_test.go (2 hunks)

Files skipped from review as they are similar to previous changes (1)

• crypto/keyring/keyring.go

Additional comments: 2

crypto/keyring/keyring_ledger_test.go (2)

• 11-17: The addition of imports for testify/assert, cosmos-sdk/crypto/keys/secp256k1, cosmos-sdk/crypto/ledger, and cosmos-sdk/crypto/types is appropriate for the new test function TestSignWithLedger. These imports are necessary for assertions, handling cryptographic keys, and interacting with Ledger devices within the test.
• 142-202: The TestSignWithLedger function introduces comprehensive test cases to validate the signing process with Ledger devices. It correctly sets up two distinct Ledger records (recordA and recordB) to simulate different scenarios, including a successful signing operation and an error case where the public key does not match the Ledger device's public key.

Suggestions for Improvement:

1. Error Message Verification: While the test case for expecting an error when the public key does not match is well-implemented, it's crucial to ensure that the error message is precise and user-friendly. The current error message, "the public key that the user attempted to sign with does not match the public key on the ledger device," is clear, but always consider if there's room for making it more informative or actionable for the user.

2. Test Coverage: Ensure that all possible error paths and edge cases are covered by the tests. This includes scenarios where the Ledger device might be unavailable, the signing process is interrupted, or other exceptional conditions that could arise during interaction with Ledger devices.

3. Mocking Ledger Devices: The tests appear to rely on the actual implementation of ledger.NewPrivKeySecp256k1 and NewLedgerRecord. For unit tests, consider mocking these dependencies to isolate the test environment from external factors and make the tests more deterministic. This approach also allows for testing error conditions more thoroughly.

4. Code Documentation: Adding comments to the test cases and critical sections of the test code can enhance readability and maintainability. While the test function names and structure are clear, brief comments explaining the purpose of each test case and the significance of the setup steps can be beneficial for future maintainers or contributors.

Overall, the test implementation is solid and addresses the key functionality introduced in the PR. These suggestions aim to refine the tests further and ensure they are as robust and maintainable as possible.

[rootulp]

I just tried to manually verify #14460 but it didn't throw an error after signing a transaction with the incorrect Ledger so IMO it doesn't work as expected.

[rootulp]

I'm going to ignore this CI failure b/c it seems unrelated to this PR.

[sontrinh16]

I just tried to manually verify #14460 but it didn't throw an error after signing a transaction with the incorrect Ledger so I think it doesn't work as expected.

yeah i think we would use this and remove the old check and back port to older releases ? wdyt @julienrbrt

[coderabbitai]

Review Status

Actionable comments generated: 0

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between c7ccfba and 9592b02.

Files selected for processing (1)

• crypto/keyring/keyring_ledger_test.go (2 hunks)

Files skipped from review as they are similar to previous changes (1)

• crypto/keyring/keyring_ledger_test.go

[coderabbitai]

Review Status

Actionable comments generated: 1

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between 9592b02 and 23e2364.

Files selected for processing (1)

• CHANGELOG.md (2 hunks)

Additional comments: 1

CHANGELOG.md (1)

• 147-147: The changes listed under the types module are significant and could impact developers relying on the previous structure. Ensure that the release notes or documentation clearly communicate these changes as potentially breaking.

[coderabbitai]

The entry for PR #19691 could be more descriptive to highlight the key improvement. Consider rephrasing to:
"Fix in the crypto module to throw an error for key mismatches when an incorrect Ledger device is used for signing transactions, enhancing security and user experience."