eth-educators / ethstaker-deposit-cli

Secure key generation for deposits
https://eth-educators.github.io/ethstaker-deposit-cli/
Creative Commons Zero v1.0 Universal
3 stars 5 forks source link

Pbkdf2 support #45

Closed valefar-on-discord closed 4 months ago

valefar-on-discord commented 5 months ago

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 Pbkdf2Keystore will be used or ScryptKeystore if not specified.

When it comes to exiting through the keystore file, there is no need to specify as the Keystore.decrypt function will call self.kdf which then does a simple check of return scrypt(**kwargs) if 'scrypt' in self.crypto.kdf.function else PBKDF2(**kwargs)

Fixes #37

valefar-on-discord commented 4 months ago

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.

remyroy commented 4 months ago

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.

valefar-on-discord commented 4 months ago

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. My knowledge of cryptography is very limited but because the params are saved in the file there should be no compatibility concerns if used with any iteration count.

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?

remyroy commented 4 months ago

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.