Pbkdf2 support #45
Pbkdf2 support #45
Conversation
staking_deposit/cli/generate_keys.py
Outdated
| default=False, | ||
| is_flag=True, | ||
| param_decls='--pbkdf2', | ||
| help='Uses the pbkdf2 encryption method instead of scrypt.', |
There was a problem hiding this comment.
This should be moved into the internalization structure with the load_text function. The message should also be explicit about this being used for the resulting keystore file(s).
README.md
Outdated
| @@ -152,6 +152,7 @@ You can use `new-mnemonic --help` to see all arguments. Note that if there are m | |||
| | `--folder` | String. Pointing to `./validator_keys` by default | The folder path for the keystore(s) and deposit(s) | | |||
| | `--chain` | String. `mainnet` by default | The chain setting for the signing domain. | | |||
| | `--execution_address` (or `--eth1_withdrawal_address`) | String. Eth1 address in hexadecimal encoded form | If this field is set and valid, the given Eth1 address will be used to create the withdrawal credentials. Otherwise, it will generate withdrawal credentials with the mnemonic-derived withdrawal public key in [ERC-2334 format](https://eips.ethereum.org/EIPS/eip-2334#eth2-specific-parameters). | | |||
| | `--pbkdf2` | Flag | Will use pbkdf2 key derivation instead of scrypt as defined in EIP-2335. This can be a good alternative if you intend to work with a large number of keys. | | |||
There was a problem hiding this comment.
The documentation should also be explicit about this being used for the resulting keystore file(s).
README.md
Outdated
| @@ -164,6 +165,7 @@ You can use `existing-mnemonic --help` to see all arguments. Note that if there | |||
| | `--folder` | String. Pointing to `./validator_keys` by default | The folder path for the keystore(s) and deposit(s) | | |||
| | `--chain` | String. `mainnet` by default | The chain setting for the signing domain. | | |||
| | `--execution_address` (or `--eth1_withdrawal_address`) | String. Eth1 address in hexadecimal encoded form | If this field is set and valid, the given Eth1 address will be used to create the withdrawal credentials. Otherwise, it will generate withdrawal credentials with the mnemonic-derived withdrawal public key in [ERC-2334 format](https://eips.ethereum.org/EIPS/eip-2334#eth2-specific-parameters). | | |||
| | `--pbkdf2` | Flag | Will use pbkdf2 key derivation instead of scrypt as defined in EIP-2335. This can be a good alternative if you intend to work with a large number of keys. | | |||
There was a problem hiding this comment.
The documentation should also be explicit about this being used for the resulting keystore file(s).
|
I've updated the wording but was struggling because pbkdf2 is less secure than scrypt and our iteration count of 262,144 is currently below the recommended 600,000. This is a recently updated recommendation which explains the discrepancy from the spec. I'd like to be more specific with pbkdf2 being a more performant but less secure option but I worry about alarms that could raise. |
What about using a good default iteration count and making the iteration count configurable with another flag? If someone really wants speed, he could choose a lower iteration count while making the compromise of less security but it would be an opt-in. |
I worry about the cost-benefit of adding the iteration count parameter. We would definitely want to have some minimum to allow a standard and then it would be directly utilized with the kdf params which is where auditors might be a bit scrupulous. I played around with an increase of the iteration to 2**20 to measure creation time and it becomes slower than scrypt. But you do raise a good point: The original issue was requested for a shorter decryption time. The encryption and decryption time is a function of the iteration count so as that increases the performance of the former decreases. This is where I'd like to ask Carl why PBKDF2 was never included 😁 But in the end, it probably becomes moot due to the password ultimately being the deciding factor. Given the password length requirement of 8 characters, I would imagine that is going to be the weakest link regardless of kdf iteration count. Given that ramble, are you happy with the wording or would you like to make additional changes? |
|
I'm happy with the wording changes and I don't see any other pressing need for this PR. I would accept it as is. |
Adding a flag that will allow keys to be generated using pbkdf2 derivation function instead of scrypt. This is a substantially faster alternative and good option for users if they were going to generate keys in bulk.
We will still default to scrypt but if the flag is specified, it will drill down to the key generation where either the existing
Pbkdf2Keystorewill be used orScryptKeystoreif not specified.When it comes to exiting through the keystore file, there is no need to specify as the
Keystore.decryptfunction will callself.kdfwhich then does a simple check ofreturn scrypt(**kwargs) if 'scrypt' in self.crypto.kdf.function else PBKDF2(**kwargs)Fixes #37