Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

fix(cli): fix Ledger signature algorithm verification #1550

Merged
merged 8 commits into from
Dec 22, 2022
Merged

fix(cli): fix Ledger signature algorithm verification #1550

merged 8 commits into from
Dec 22, 2022

Conversation

0a1c
Copy link
Contributor

@0a1c 0a1c commented Dec 14, 2022

Closes: #XXX

Description

Change the hard-coded Ledger derivation to use ethsecp256k1. This algorithm is not actually used, it's only checked against the SupportedAlgosLedger set. We update it here for clarity, so we can use the ethsecp256k1 key exclusively.

For contributor use:

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@0a1c 0a1c requested a review from a team as a code owner December 14, 2022 16:58
@0a1c 0a1c requested review from hanchon and adisaran64 and removed request for a team December 14, 2022 16:58
@github-actions github-actions bot added the C:CLI label Dec 14, 2022
@0a1c 0a1c mentioned this pull request Dec 14, 2022
Merged
12 tasks
fedekunze
fedekunze approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK. Please add a changelog entry

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 22, 2022
View reviewed changes
client/keys/add.go
interactive, _ := cmd.Flags().GetBool(flagInteractive)
noBackup, _ := cmd.Flags().GetBool(flagNoBackup)
useLedger, _ := cmd.Flags().GetBool(flags.FlagUseLedger)
algoStr, _ := cmd.Flags().GetString(flags.FlagKeyAlgorithm)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack.

Returned error is not propagated up the stack.
client/keys/add.go
interactive, _ := cmd.Flags().GetBool(flagInteractive)
noBackup, _ := cmd.Flags().GetBool(flagNoBackup)
useLedger, _ := cmd.Flags().GetBool(flags.FlagUseLedger)

Check warning

Code scanning / gosec

Returned error is not propagated up the stack.

Returned error is not propagated up the stack.
@fedekunze fedekunze changed the title fix: update Ledger default algorithm to EthSecp256k1 fix(cli): fix Ledger signature algorithm verification Dec 22, 2022
@fedekunze fedekunze enabled auto-merge (squash) December 22, 2022 09:07
@fedekunze fedekunze merged commit fc21e4d into main Dec 22, 2022
@fedekunze fedekunze deleted the austinchandra/fix-ledger-supported-algos branch December 22, 2022 09:11
MalteHerrmann pushed a commit that referenced this pull request Dec 22, 2022
@0a1c @facs95
@fedekunze @MalteHerrmann
fix(cli): fix Ledger signature algorithm verification (#1550)
77edadd 
* fix: update Ledger default algorithm to `EthSecp256k1`

* fix ledger signing algo validation

* changelog

Co-authored-by: Freddy Caceres <facs95@gmail.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
@MalteHerrmann MalteHerrmann mentioned this pull request Dec 22, 2022
Merged
fedekunze added a commit that referenced this pull request Dec 22, 2022
@MalteHerrmann @dependabot
@4rgon4ut @GAtom22 @fedekunze @yihuang @0a1c @facs95
Release v0.20-rc5 (#1569)
f2549a7 
* build(deps): bump github.com/cosmos/cosmos-sdk from 0.46.6 to 0.46.7 (#1551)

Bumps [github.com/cosmos/cosmos-sdk](https://github.com/cosmos/cosmos-sdk) from 0.46.6 to 0.46.7.
- [Release notes](https://github.com/cosmos/cosmos-sdk/releases)
- [Changelog](https://github.com/cosmos/cosmos-sdk/blob/main/CHANGELOG.md)
- [Commits](cosmos/cosmos-sdk@v0.46.6...v0.46.7)

---
updated-dependencies:
- dependency-name: github.com/cosmos/cosmos-sdk
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* build(deps): bump github.com/cosmos/ibc-go/v5 from 5.1.0 to 5.2.0 (#1564)

Bumps [github.com/cosmos/ibc-go/v5](https://github.com/cosmos/ibc-go) from 5.1.0 to 5.2.0.
- [Release notes](https://github.com/cosmos/ibc-go/releases)
- [Changelog](https://github.com/cosmos/ibc-go/blob/v5.2.0/CHANGELOG.md)
- [Commits](cosmos/ibc-go@v5.1.0...v5.2.0)

---
updated-dependencies:
- dependency-name: github.com/cosmos/ibc-go/v5
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* make missing key error message during SendTransaction more verbose (#1563)

Co-authored-by: 4rgon4ut <59182467+4rgon4ut@users.noreply.github.com>

* debug(app): add flag to disable optimized build for remote debugging (#1549)

Co-authored-by: MalteHerrmann <42640438+MalteHerrmann@users.noreply.github.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

* Problem: personal_newAccount don't work (#1561)

fix the internal parameter.

* fix(ante): fix reCheckTx gas wanted (#1566)

* fix(abci): fix reCheckTx gas wanted'

* fix(ante): add changelog entry

* fix(cli): fix Ledger signature algorithm verification (#1550)

* fix: update Ledger default algorithm to `EthSecp256k1`

* fix ledger signing algo validation

* changelog

Co-authored-by: Freddy Caceres <facs95@gmail.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>

* update changelog

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: 4rgon4ut <59182467+4rgon4ut@users.noreply.github.com>
Co-authored-by: Tomas Guerra <54514587+GAtom22@users.noreply.github.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: yihuang <huang@crypto.com>
Co-authored-by: Austin Chandra <austinchandra@berkeley.edu>
Co-authored-by: Freddy Caceres <facs95@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@facs95 facs95 facs95 approved these changes

@fedekunze fedekunze fedekunze approved these changes

@hanchon hanchon Awaiting requested review from hanchon hanchon is a code owner automatically assigned from evmos/core-engineering

@adisaran64 adisaran64 Awaiting requested review from adisaran64 adisaran64 is a code owner automatically assigned from evmos/core-engineering

Assignees
No one assigned
Labels
C:CLI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@0a1c @facs95 @fedekunze