-
Notifications
You must be signed in to change notification settings - Fork 42
Conversation
11a424e
to
6ccdd8f
Compare
6ccdd8f
to
294ea5c
Compare
I've confirmed this PR works live against a YubiHSM2 (although I have a few local changes to the Here is the profile it configures at the moment:
To break it down: id: 0x0001, type: authentication-key, sequence: 1This is the replacement admin key. It has a sequence ID of 1 because it replaces the original default admin key. It has access to all domains and all capabilities.
id: 0x0001, type: wrap-key, sequence: 0This is the initial wrap key generated by the setup process. It has full delegated capabilities, i.e. any object exported under it can be imported with any capabilities. We might want to restrict that in the future (e.g. by scoping this key and any intentionally exportable keys to specific domains).
id: 0x0002, type: authentication-key, sequence: 0This is the initial
id: 0x0003, type: authentication-key, sequence: 0This is the initial
id: 0x0004, type: authentication-key, sequence: 0This is the initial
id: 0xfffe, type: opaque, sequence: 0This is a report of the provisioning process. It's serialized as a JSON document:
If there's anything else anyone thinks should be included in this report, let me know. |
src/commands/yubihsm/setup.rs
Outdated
kdf.expand(HKDF_MNEMONIC_INFO, &mut okm).unwrap(); | ||
ikm.zeroize(); | ||
|
||
// TODO(tarcieri): support other languages? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In gaia we currently also only support english.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! I'll read up on the yubiHSM documentation and give this a 2nd pass whenever this isn't WIP anymore.
c172e7d
to
7371c82
Compare
See detailed description at: #180
7371c82
to
ece511d
Compare
I'm removing WIP because it's functionally complete, except there are a few little details like the prompt to erase your device does not yet actually prompt you, and the report it prints when it's complete is raw debug output. Otherwise, this PR now implements the key derivation and provisioning plan described above, and testing out the various roles so far they appear to be suitable for these particular functions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments. I think we should add the few lines that read the user input and prevent accidentally re-initing the HSM. Other than that, awesome work!
println!("validator (key 4): {}", validator_password.as_str()); | ||
|
||
println!("\nAre you SURE you want erase your HSM and reinitialize it? (y/N)"); | ||
// TODO(tarcieri): prompt that they actually want to continue!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we worried that without this prompt, users might accidentally erase their HSM? I think we should prevent this from happening in this PR.
We could add the simples possible input reading like simply use use std::io;
and
// TODO(tarcieri): prompt that they actually want to continue!!! | |
let mut choice_in = String::new(); | |
io::stdin() | |
.read_line(&mut choice_in) | |
.expect("Failed to read user input"); | |
let choice = choice_in.trim(); | |
if choice == "y" || choice == "Y" { | |
println!("Reinitializing HSM ..."); | |
} else if choice == "n" || choice == "N" { | |
println!("Stopping reinitialization ..."); | |
process::exit(0); | |
} else { | |
println!("Invalid user input. Expected y/N."); | |
process::exit(1); | |
} |
/// These are both used as input key material (IKM) for a key derivation | ||
/// function (HKDF) in order to derive the recovery passphrase, which ideally | ||
/// ensures that the passphrase will be securely random so long as at least | ||
/// one of the two inputs is secure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool idea to split up the randomness source between the OS and the HSM!
} | ||
|
||
/// Derive secrets from the given BIP39 `Mnemonic` ala a BIP32 (hardened) | ||
/// derivation hierarchy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We should probably document how exactly the derivation works.
/// Derive a role password from the given BIP39 `Mnemonic` | ||
pub fn derive_from_mnemonic(mnemonic: &Mnemonic, role_name: &str) -> Self { | ||
let mut secret_key = | ||
derive_secret_from_mnemonic(mnemonic, &[b"role", role_name.as_bytes()]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think we should better document the derivation paths.
} | ||
|
||
child_key | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very similar to bip32 derivation but much simpler. Should I spend some time to properly understand the subtle differences if any?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two main differences from BIP32-style hardened derivation at the moment are:
- No toplevel domain separation string (BIP32 uses
Bitcoin seed
). Instead it only includes a BIP43-style "purpose", which is set to1
. Including this might be a good idea, as it would potentially allow people to cleanly reuse these words for other things if need be. - Raw bytestring-based API instead of BIP32's child number system, which I consider to be incidental complexity arising from combining a symmetric key derivation scheme with a key blinding scheme.
Regarding the first issue, BIP32 curiously uses a fixed string as the HMAC key. This is fine owing to the unique properties of HMAC as a construction, but bad hygiene for PRFs in general (the key should be uniformly random, with the fixed strings used as PRF inputs). Minor nit in the construction of BIP32 in general.
Would there be merit to just using BIP32 outright? Maybe. Personally one of the things I like the most about the YubiHSMs is the ability to generate encrypted backups of keys generated within the HSMs, such that if a validator's "24 words" are ever compromised, they do not immediately give an attacker the ability to calculate a validator's private key. Instead, an attacker will need both that and an encrypted/wrapped backups of the private keys.
The other difficult thing about BIP32 is if we do plan to use it, I think it should probably be used correctly within a properly registered BIP43/BIP44 scheme (otherwise what's the point? and see also previous unpleasantness about registering numbers within derivation paths). That said, if people want to use BIP32 for something in the future, there's no reason they couldn't use it in parallel with the same mnemonic.
Personally I'm happy with something that looks enough like BIP32 to make it unconcerning from a security analysis POV, and simple enough it can be implemented in ~25 lines of code.
I'll merge this for now. It seems like we are aware that the user prompt is not happening (will submit a follow-ip PR) and I trust Tony with regard to the key-derivation method. |
This commit addresses a few of the remaining deficiencies from #180 - Actually prompts the user before erasing their device!!! - Adds a `-r` option to write the provisioning report to a file - Formatting cleanups for password provisioning lists - Prints the device serial number and includes it in the report - Bumps to `yubihsm` crate v0.21.0 final (which adds serial# to report)
This commit addresses a few of the remaining deficiencies from #180 - Actually prompts the user before erasing their device!!! - Adds a `-r` option to write the provisioning report to a file - Formatting cleanups for password provisioning lists - Prints the device serial number and includes it in the report - Bumps to `yubihsm` crate v0.21.0 final (which adds serial# to report)
This commit addresses a few of the remaining deficiencies from #180 - Actually prompts the user before erasing their device!!! - Adds a `-r` option to write the provisioning report to a file - Formatting cleanups for password provisioning lists - Prints the device serial number and includes it in the report - Bumps to `yubihsm` crate v0.21.0 final (which adds serial# to report)
This commit addresses a few of the remaining deficiencies from #180 - Actually prompts the user before erasing their device!!! - Adds a `-r` option to write the provisioning report to a file - Formatting cleanups for password provisioning lists - Prints the device serial number and includes it in the report - Bumps to `yubihsm` crate v0.21.0 final (which adds serial# to report)
This commit addresses a few of the remaining deficiencies from #180 - Actually prompts the user before erasing their device!!! - Adds a `-r` option to write the provisioning report to a file - Formatting cleanups for password provisioning lists - Prints the device serial number and includes it in the report - Bumps to `yubihsm` crate v0.21.0 final (which adds serial# to report)
This commit addresses a few of the remaining deficiencies from #180 - Actually prompts the user before erasing their device!!! - Adds a `-r` option to write the provisioning report to a file - Formatting cleanups for password provisioning lists - Prints the device serial number and includes it in the report - Bumps to `yubihsm` crate v0.21.0 final (which adds serial# to report)
This commit addresses a few of the remaining deficiencies from #180 - Actually prompts the user before erasing their device!!! - Adds a `-r` option to write the provisioning report to a file - Formatting cleanups for password provisioning lists - Prints the device serial number and includes it in the report - Bumps to `yubihsm` crate v0.21.0 final (which adds serial# to report)
Adds a
tmkms yubihsm setup
command which will erase / factory reset a YubiHSM2 and then set it up with the roles described in #174, using the provisioning process added to theyubihsm
crate in tendermint/yubihsm-rs#174Relevant background for YubiHSM2 concepts can be found on Yubico's web site:
Roles
I would suggest avoiding too much customization of these roles to ensure future role-specific features added to the KMS can assume a standard profile.
Key / Password Derivation
This PR derives entropy for a BIP39 mnemonic (24-words, 256-bits, ala Ledger Nano) using randomness from the host OS as well as the YubiHSM2. Both inputs are concatenated and used as HKDF input key material to derive a 256-bit master secret which is encoded as 24 BIP39 words.
This master secret is used directly (i.e. after Yubico's PBKDF2) as the administrator password for the device, conferring all authority.
It is also used as input to a simplified BIP32-like algorithm for deriving symmetric secrets, including the passwords for all non-admin roles. Presently these passwords are encoded using Bech32 with unique HRPs (both so they're shorter and easily distinguishable).
Additionally, it derives an initial wrap key. This is the key to be used for making encrypted backups of objects generated within the HSM. The wrap key's derivation path is personalized with the key's object ID, with the idea that several wrap keys can be derived from the same master secret in the event one of them is exposed.
All of this ensures the "24 words" are all that's needed to initialize several YubiHSM2s as identical copies of each other (sans importing backups of generated keys).
Here is an example of the output from the presently implemented (WIP) setup process: