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.

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: add hidden types and seed words to key manager #4925

Merged

Conversation

jorgeantonio21
Copy link
Contributor

@jorgeantonio21 jorgeantonio21 commented Nov 15, 2022

Description

We refactor the key_manager crate so that hides sensitive data such as mnemonics, encryption and mac keys, etc.

Motivation and Context

Tackle issue #4861 and a portion of #4846.

Fixes #4861

How Has This Been Tested?

Add unit tests for SeedWords. Also generated need wallet with visible seed words, to the user.

stringhandler
stringhandler previously approved these changes Nov 16, 2022
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Looks great

@sdbondi sdbondi added P-conflicts Process - The PR has merge conflicts that need to be resolved A-security Area - Security related A-wallet Area - related to the wallet labels Nov 18, 2022
@sdbondi sdbondi removed the P-conflicts Process - The PR has merge conflicts that need to be resolved label Nov 21, 2022
@jorgeantonio21 jorgeantonio21 marked this pull request as ready for review November 21, 2022 10:56
sdbondi
sdbondi previously approved these changes Nov 21, 2022
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Awesome

The cli and seed work prompt still leak but these can be fixed in another PR(s) as both of them require a little refactoring

base_layer/key_manager/src/cipher_seed.rs Outdated Show resolved Hide resolved
base_layer/key_manager/src/cipher_seed.rs Outdated Show resolved Hide resolved
base_layer/key_manager/src/cipher_seed.rs Outdated Show resolved Hide resolved
base_layer/key_manager/src/cipher_seed.rs Outdated Show resolved Hide resolved
base_layer/key_manager/src/cipher_seed.rs Outdated Show resolved Hide resolved
@@ -100,7 +105,8 @@ impl MnemonicLanguage {
return Err(MnemonicError::UnknownLanguage);
},
Ordering::Greater => {
for word in words {
for word_ind in 0..words.len() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it worth it to have SeedWords implement an iterator that provides word references? Then you don't need to mess around with indexes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say that it would be nice to have it. But honestly, to implement Iterator one needs to make some lifetime algebra, which adds unnecessary complexity to the SeedWords type. Since these are the only instances in which we need to traverse the SeedWords type, I think it might be not worth the extra complexity. But happy to refactor if you think otherwise :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I hadn't considered that lifetimes would be problematic when writing this. As you say, it's probably not worth it for the two times when it would be used, and the bounds checks are correct as implemented anyway.

base_layer/key_manager/src/mnemonic.rs Show resolved Hide resolved
base_layer/key_manager/src/mnemonic.rs Outdated Show resolved Hide resolved
base_layer/key_manager/src/mnemonic.rs Outdated Show resolved Hide resolved
base_layer/key_manager/src/mnemonic.rs Show resolved Hide resolved
@stringhandler stringhandler merged commit 0bdb568 into tari-project:development Nov 22, 2022
sdbondi added a commit to sdbondi/tari that referenced this pull request Nov 23, 2022
* development:
  fix: add hidden types and seed words to key manager (tari-project#4925)
  feat: timestamp validation (tari-project#4887)
  fix: deleted_txo_mmr_position_to_height_index  already exists error (tari-project#4924)
  feat: add default grpc for localnet (tari-project#4937)
  first commit (tari-project#4926)
  v0.40.2
  fix(dht): use limited ban period for invalid peer (tari-project#4933)
  feat: upgrade tari_crypto sign api (tari-project#4932)
  v0.40.1
  chore: fix depreciated timestamp clippy (tari-project#4929)
  chore: fix naming of ffi functions and comments  (tari-project#4930)
  fix: set wallet start scan height to birthday and not 0 (see issue tari-project#4807) (tari-project#4911)
  v0.40.0
  chore: remove unused methods (tari-project#4922)
  fix: updates for SafePassword API change (tari-project#4927)
stringhandler pushed a commit that referenced this pull request Nov 30, 2022
Description
---
The goal of this PR is to zeroize sensitive data content across the Tari repo.  We pay special attention to kdf key data and other occurrences of sensitive data. For more details, we refer to issue #4846. Moreover, this PR is a companion to #4953 and #4925

Motivation and Context
---
Tackle issue #4846.

How Has This Been Tested?
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-security Area - Security related A-wallet Area - related to the wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mnemonic::to_mnemonic should wrap the seed words in Zeroizing
4 participants