Implement Keystore spec for BLS12-381 secret keys
#777
Conversation
|
Is this ready for review? |
|
Yup it is. Forgot to mark this one as ready. |
| E: de::Error, | ||
| { | ||
| let bytes = hex::decode(v).map_err(E::custom)?; | ||
| Ok(from_slice(&bytes)) |
There was a problem hiding this comment.
The from_slice function will panic unless bytes.len() == IV_SIZE.
I think it would be best to check bytes.len() here and return an error if it's not 16.
There was a problem hiding this comment.
Yep. I guess we can also return an Option from the from_slice function and handle the None case here.
| impl Crypto { | ||
| /// Generate crypto module for `Keystore` given the password, | ||
| /// secret to encrypt, kdf params and cipher params. | ||
| pub fn encrypt(password: String, secret: &[u8], kdf: Kdf, cipher: Cipher) -> Self { |
There was a problem hiding this comment.
When we're dealing with secrets/passwords we should be careful to zeroize them after they're done.
For password it would probably be safest to create a Password type that wraps a string and always calls zeroize on drop. The secret field is fine since we only have a reference to it, but we'd have to pay attention to this in upstream functions.
There was a problem hiding this comment.
Thanks, will add the zeroize calls :)
| Cipher::Aes128Ctr(cipher) => cipher.encrypt(&derived_key[0..16], secret), | ||
| }; | ||
| // Generate checksum | ||
| let mut pre_image: Vec<u8> = derived_key[16..32].to_owned(); |
There was a problem hiding this comment.
It's a little pedantic, but we could avoid a couple of heap allocations here:
let mut pre_image = [0; 32];
pre_image[0..16].copy_from_slice(derived_key[16..32]);
pre_image[16..32].copy_from_slice(&cipher_message);There was a problem hiding this comment.
I think we need a vector here since we don't know the size of the secret in advance.
We can do this if we assume that the secret will always be a secret key of fixed length.
There was a problem hiding this comment.
Oh yeah, you're right sorry! I thought it was producing a HMAC but it's encrypting :)
| /// | ||
| /// An error will be returned if `cipher.message` is not in hex format or | ||
| /// if password is incorrect. | ||
| pub fn decrypt(&self, password: String) -> Result<Vec<u8>, String> { |
validator_client/src/keystore/mod.rs
Outdated
| /// An error is returned if the password provided is incorrect or if | ||
| /// keystore does not contain valid hex strings or if the secret contained is not a | ||
| /// BLS12-381 secret key. | ||
| fn to_keypair(&self, password: String) -> Result<Keypair, String> { |
validator_client/src/keystore/mod.rs
Outdated
|
|
||
| /// Pad 0's to a 32 bytes BLS secret key to make it compatible with the Milagro library | ||
| /// Note: Milagro library only accepts 48 byte bls12 381 private keys. | ||
| fn pad_secret_key(sk: &[u8]) -> [u8; PRIVATE_KEY_BYTES] { |
There was a problem hiding this comment.
We should probably wrap this return value in something so it also gets zeroized.
| @@ -81,6 +105,19 @@ fn load_keypair(base_path: PathBuf, file_prefix: &str) -> Result<Keypair, String | |||
| .map_err(|e| format!("Unable to decode keypair: {:?}", e)) | |||
| } | |||
|
|
|||
| /// Load a `Keystore` from a file. | |||
| fn load_keystore(path: PathBuf, password: String) -> Result<Keystore, String> { | |||
| use std::default::Default; | ||
|
|
||
| // TODO: verify size of salt | ||
| const SALT_SIZE: usize = 32; |
There was a problem hiding this comment.
From some quick reading, RFC 2898 (PBKDF2-SHA-256) specifies an "eight octet" salt (64 bits) whilst RFC 7914 (scrypt) does not specify the salt size.
It seems the test vectors have 32-byte salts, though.
There was a problem hiding this comment.
RFC 2898 specifies that the salt should be atleast 64 bits.
I don't see any harm in reducing to the minimum recommended value. Its only while deriving the key that we use the SALT_SIZE constant. We will still be able to read keystores with larger salt values (for tests etc).
Also, scrypt uses pbkdf2 under the hood, so I guess we should have ultimately have similar salt sizes for both.
There was a problem hiding this comment.
Are you suggesting to drop SALT_SIZE to 8 bytes (64 bits)?
There was a problem hiding this comment.
I checked the other implementations (Go, Python, Typescript). All are using 32 byte salts. I suggest we keep it the same.
paulhauner
added
waiting-on-author
pawanjay176
added
ready-for-review
|
Closing in favour of #1071 which builds upon this work. Thanks @pawanjay176! |
Issue Addressed
N/A
Proposed Changes
Supersedes #596 to maintain clean commit history.