iqlusioninc / tmkms

Tendermint KMS: Key Management System for Tendermint Validators
https://tendermint.com/
Apache License 2.0
335 stars 121 forks source link

Allow restore from 4-character mnemonics #741

Open greg-szabo opened 1 year ago

greg-szabo commented 1 year ago

Summary

User story: For easier restore, let TMKMS expand the whole passphrase from the 4-char words. For example: tmkms yubihsm setup --restore --short-words would check for 4-char words and restore them before generating the password.

Proposal

The solution boils down around the Phrase::new method in the hkd32 crate from the iqlusioninc/crates repository. This method converts a string containing the mnemonics passphrase into a Phrase struct.

Option 1: Expand the new method looping through the string's content and try to expand any words if they are four characters. This would not break the method implementation but it would accept input that is not strictly a 24-word BIP39 passphrase as correct input. (aban abandon aban === abandon abandon abandon)

Option 2: Expand the new method input parameters with a boolean that indicates if the submitted string contains short-word mnemonics or full-word mnemonics. This would break implementations of this function elsewhere but it would keep the strict word checking.

Option 3: Add a method to the Phrase struct that allows converting a short-word list to a full mnemonic-list or return with an error if it is invalid. This would not break the current implementations, it could enable strict checking, but it requires the developer to make an extra call to get the full mnemonics, before new is called. It also requires a similar method in the Language struct.

Implementation

I went ahead and implemented Option 3, because I didn't want to break current implementations but I wanted to keep strict checking. I actually like Option 1 more, that's why I need feedback in this issue.

In the hkd32 crate:

After this, in tmkms, I can call Phrase::expand_short, before Phrase::new, if the --short-words input parameter was set.

What do you think, which option would be best?

tony-iqlusion commented 1 year ago

Option 3 seems the best to me, as it's purely additive and doesn't encumber the existing functionality with extra baggage around a niche use case