Skip to content

wallet: check min ledger app version for shielded key derivation#4358

Merged
mergify[bot] merged 3 commits intomainfrom
tomas/check-ledger-app-version
Feb 17, 2025
Merged

wallet: check min ledger app version for shielded key derivation#4358
mergify[bot] merged 3 commits intomainfrom
tomas/check-ledger-app-version

Conversation

@tzemanovic
Copy link
Copy Markdown
Collaborator

Describe your changes

closes #4327

Checklist before merging

  • If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
    • This will require 2 reviewers to approve the changes
  • If this PR requires changes to the docs or specs, a corresponding PR is opened in the namada-docs repo
    • Relevant PR if applies:
  • If this PR affects services such as namada-indexer or namada-masp-indexer, a corresponding PR is opened in that repo
    • Relevant PR if applies:

@tzemanovic tzemanovic requested a review from murisi February 10, 2025 13:30
tzemanovic added a commit that referenced this pull request Feb 10, 2025
@tzemanovic tzemanovic marked this pull request as ready for review February 10, 2025 13:32
@tzemanovic tzemanovic added wallet backport-libs-0.251 Backport libraries to 0.251 maintenance branch labels Feb 10, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.17%. Comparing base (b81f0ca) to head (314a1e5).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4358      +/-   ##
==========================================
- Coverage   74.17%   74.17%   -0.01%     
==========================================
  Files         343      343              
  Lines      110663   110663              
==========================================
- Hits        82082    82081       -1     
- Misses      28581    28582       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Copy Markdown
Collaborator

@murisi murisi left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks! Maybe we could also consider a general warning to upgrade the Ledger app to a version >= v3.0.0 (and move their funds to a key derived with the new modified ZIP 32 algorithm) whenever using an old version of the app to sign a shielded transaction... As it stands, augment_masp_hardware_keys will silently derive viewing keys using whatever ZIP 32 derivation algorithm the hardware wallet provides in the process of checking that at a given path, the software wallet key matches that of the hardware wallet.

@tzemanovic
Copy link
Copy Markdown
Collaborator Author

Looks good to me, thanks! Maybe we could also consider a general warning to upgrade the Ledger app to a version >= v3.0.0 (and move their funds to a key derived with the new modified ZIP 32 algorithm) whenever using an old version of the app to sign a shielded transaction... As it stands, augment_masp_hardware_keys will silently derive viewing keys using whatever ZIP 32 derivation algorithm the hardware wallet provides in the process of checking that at a given path, the software wallet key matches that of the hardware wallet.

Sounds good, will add this!

Copy link
Copy Markdown
Collaborator

@murisi murisi left a comment

Choose a reason for hiding this comment

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

Thanks for adding warning during transaction construction. Everything looks good to me!

@tzemanovic tzemanovic added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Feb 14, 2025
@murisi murisi removed the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Feb 17, 2025
@murisi murisi force-pushed the tomas/check-ledger-app-version branch from a13ce58 to 314a1e5 Compare February 17, 2025 05:24
@murisi murisi added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Feb 17, 2025
mergify bot added a commit that referenced this pull request Feb 17, 2025
mergify bot added a commit that referenced this pull request Feb 17, 2025
@mergify mergify bot merged commit cd3180c into main Feb 17, 2025
25 checks passed
@mergify mergify bot deleted the tomas/check-ledger-app-version branch February 17, 2025 06:55
mergify bot pushed a commit that referenced this pull request Feb 17, 2025
(cherry picked from commit 2bd7779)
mergify bot added a commit that referenced this pull request Feb 17, 2025
wallet: check min ledger app version for shielded key derivation (backport #4358)
tzemanovic added a commit that referenced this pull request Mar 12, 2025
* maint-libs-0.48:
  Changelog: Release libs 0.48.0
  Namada libs 0.48.0
  Make sure to reset masp_index counter to zero whenever new block is encountered.
  Correct the masp_index once it has been found.
  Reduce cloning of state by updating only the Merkle tree when searching for correct ordering.
  Update commitment tree anchors in benchmarks.
  Order transactions in the cache according to the MASP index instead of the block index.
  Assume that Transactions from the indexer are already ordered.
  Compute the transaction order even when needs_witness_map_update capability is disabled.
  Added a changelog entry.
  Factor out the Transaction order search into a separate function.
  Optimize transaction order search.
  Make client try all Transaction permutations.
  Instrumented code with commitment anchor existence checks.
  Changelog: Release libs 0.47.3
  Namada libs 0.47.3
  changelog: add #4403
  wallet: fix build with "migrations" feature
  gov: fix build with "testing" feature on
  PoS: fix build with "testing" feature on
  ci/benches: increase timeout and use bigger machine
  test: sa003
  test+bench: sa002
  test: SA001
  fix to write toml files
  changelog: add #4284
  sdk: fix breaking change in getrandom wasm build
  deps: cargo update
  deps: move all non-workspace deps into workspace and update
  deps: update wasmparser to wasmer matching version
  deps: update toml
  deps: update sysinfo
  deps: update sha2
  deps: update rpassword
  deps: update rlimit
  deps: update reqwest
  deps: update num256
  clippy: reinstate disallow unwrap_or_default
  deps: update itertools
  deps: update byte-unit
  dep: update bech32
  dep: update base64
  update non-breaking deps
  deps: fix crate vulnerabilities
  changelog: add #4371
  add VERSIONING.md
  Modifies `PartialEq` for `Signer`. `Authorization`s account for the signer when compared
  Fixes bugs in `PartialEq` implementations for `SigningTxData` and `Authorization`
  Changelog #4230
  Removes duplicated sections from transactions before signing or dumping
  Custom implementaion of `PartialEq` for `Authorization`
  Update `PartialEq` for `SigningTxData`
  CI: update the Ledger app version to 3.0.1
  Added a changelog entry.
  Print the transaction's chain ID in expert mode.
  client: warn about min ledger app version when using shielded keys
  changelog: add #4358
  wallet: check min ledger app version for shielded key derivation
  Added changelog entry.
  Update hardware genesis files to reflect shielded keys derived using new algorithm.
  changelog
  revert hermes version for CI
  upstream hermes
  test/e2e: fix the test with HW wallet automation
tzemanovic added a commit that referenced this pull request Mar 12, 2025
* maint-libs-0.48:
  Changelog: Release libs 0.48.0
  Namada libs 0.48.0
  Make sure to reset masp_index counter to zero whenever new block is encountered.
  Correct the masp_index once it has been found.
  Reduce cloning of state by updating only the Merkle tree when searching for correct ordering.
  Update commitment tree anchors in benchmarks.
  Order transactions in the cache according to the MASP index instead of the block index.
  Assume that Transactions from the indexer are already ordered.
  Compute the transaction order even when needs_witness_map_update capability is disabled.
  Added a changelog entry.
  Factor out the Transaction order search into a separate function.
  Optimize transaction order search.
  Make client try all Transaction permutations.
  Instrumented code with commitment anchor existence checks.
  Changelog: Release libs 0.47.3
  Namada libs 0.47.3
  changelog: add #4403
  wallet: fix build with "migrations" feature
  gov: fix build with "testing" feature on
  PoS: fix build with "testing" feature on
  ci/benches: increase timeout and use bigger machine
  test: sa003
  test+bench: sa002
  test: SA001
  fix to write toml files
  changelog: add #4284
  sdk: fix breaking change in getrandom wasm build
  deps: cargo update
  deps: move all non-workspace deps into workspace and update
  deps: update wasmparser to wasmer matching version
  deps: update toml
  deps: update sysinfo
  deps: update sha2
  deps: update rpassword
  deps: update rlimit
  deps: update reqwest
  deps: update num256
  clippy: reinstate disallow unwrap_or_default
  deps: update itertools
  deps: update byte-unit
  dep: update bech32
  dep: update base64
  update non-breaking deps
  deps: fix crate vulnerabilities
  changelog: add #4371
  add VERSIONING.md
  Modifies `PartialEq` for `Signer`. `Authorization`s account for the signer when compared
  Fixes bugs in `PartialEq` implementations for `SigningTxData` and `Authorization`
  Changelog #4230
  Removes duplicated sections from transactions before signing or dumping
  Custom implementaion of `PartialEq` for `Authorization`
  Update `PartialEq` for `SigningTxData`
  CI: update the Ledger app version to 3.0.1
  Added a changelog entry.
  Print the transaction's chain ID in expert mode.
  client: warn about min ledger app version when using shielded keys
  changelog: add #4358
  wallet: check min ledger app version for shielded key derivation
  Added changelog entry.
  Update hardware genesis files to reflect shielded keys derived using new algorithm.
  changelog
  revert hermes version for CI
  upstream hermes
  test/e2e: fix the test with HW wallet automation
tzemanovic added a commit that referenced this pull request Mar 14, 2025
Namada libs-v0.48.0 major release.

* tag 'libs-v0.48.0': (153 commits)
  Changelog: Release libs 0.48.0
  Namada libs 0.48.0
  Make sure to reset masp_index counter to zero whenever new block is encountered.
  Correct the masp_index once it has been found.
  Reduce cloning of state by updating only the Merkle tree when searching for correct ordering.
  Update commitment tree anchors in benchmarks.
  Order transactions in the cache according to the MASP index instead of the block index.
  Assume that Transactions from the indexer are already ordered.
  Compute the transaction order even when needs_witness_map_update capability is disabled.
  Added a changelog entry.
  Factor out the Transaction order search into a separate function.
  Optimize transaction order search.
  Make client try all Transaction permutations.
  Instrumented code with commitment anchor existence checks.
  Changelog: Release libs 0.47.3
  Namada libs 0.47.3
  changelog: add #4403
  wallet: fix build with "migrations" feature
  gov: fix build with "testing" feature on
  PoS: fix build with "testing" feature on
  ci/benches: increase timeout and use bigger machine
  test: sa003
  test+bench: sa002
  test: SA001
  fix to write toml files
  changelog: add #4284
  sdk: fix breaking change in getrandom wasm build
  deps: cargo update
  deps: move all non-workspace deps into workspace and update
  deps: update wasmparser to wasmer matching version
  deps: update toml
  deps: update sysinfo
  deps: update sha2
  deps: update rpassword
  deps: update rlimit
  deps: update reqwest
  deps: update num256
  clippy: reinstate disallow unwrap_or_default
  deps: update itertools
  deps: update byte-unit
  dep: update bech32
  dep: update base64
  update non-breaking deps
  deps: fix crate vulnerabilities
  changelog: add #4371
  add VERSIONING.md
  Modifies `PartialEq` for `Signer`. `Authorization`s account for the signer when compared
  Fixes bugs in `PartialEq` implementations for `SigningTxData` and `Authorization`
  Changelog #4230
  Removes duplicated sections from transactions before signing or dumping
  Custom implementaion of `PartialEq` for `Authorization`
  Update `PartialEq` for `SigningTxData`
  CI: update the Ledger app version to 3.0.1
  Added a changelog entry.
  Print the transaction's chain ID in expert mode.
  client: warn about min ledger app version when using shielded keys
  changelog: add #4358
  wallet: check min ledger app version for shielded key derivation
  Added changelog entry.
  Update hardware genesis files to reflect shielded keys derived using new algorithm.
  changelog
  revert hermes version for CI
  upstream hermes
  test/e2e: fix the test with HW wallet automation
  Changelog: Release libs 0.47.2
  Namada libs 0.47.2
  move version detection build script from apps_lib into apps
  keep a separate commit for release changelogs
  apps_lib: output artifacts from build script into OUT_DIR
  tx: build tonic artifacts into OUT_DIR
  changelog: add #4347
  sdk/tx: print tx events if any
  declare internal dev-dependencies via path
  changelog: release Namada libs 0.47.1
  Namada libs 0.47.1
  Revert "Merge pull request #4332 from anoma/murisi/test-vectors-with-chain-ids"
  changelog: add #4283
  PoS: emit token transfer event on claim rewards tx
  Added a changelog entry.
  Print the transaction's chain ID in expert mode.
  Rename test_inverse_conversions to make it clear that it now tests something else.
  Added changelog entry.
  Remove the ability to specify which historical epoch amounts are exchanged to.
  Do not query for the inverse conversion when converting to the latest epoch.
  Added changelog entry.
  Update inverse conversion test.
  Now parameterize query_allowed_conversion with the target epoch.
  fixup! deps: use internal crates defined in workspace
  deps: update non-lib crates Cargo.toml to use workspace defs
  config/release: allow to publish released crates
  deps: fix ignored and rm unused default-features flags
  deps: use internal crates defined in workspace
  specify all internal lib crates in workspace
  deps: rm unused borsh-ext
  fixup! changelog: add #4307
  fixup! wallet: change the derivation path for modified ZIP32
  wallet: update the rustdoc warning about modified ZIP32
  changelog: add #4307
  wallet: change the derivation path for modified ZIP32
  Added a changelog entry.
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-libs-0.251 Backport libraries to 0.251 maintenance branch merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass wallet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

check and enforce Ledger app version with a matching modified ZIP32 derivation path

2 participants