cryptography icon indicating copy to clipboard operation
cryptography copied to clipboard

[WIP] feat: add kdf_rounds param to encryption algorithm

Open Ajpantuso opened this issue 3 years ago • 3 comments

Summary

Currently the number of kdf_rounds used when encrypting OpenSSH private keys is fixed at the ssh-keygen default of 16. ssh-keygen however parameterizes this value with the -a flag. This PR permits the same parameterization through the BestAvailableEncryption class.

Open Questions

  • [] Should this parameter be passed through BestAvailableEncryption where it is generally not applicable or should another data structure e.g. KDFOptions be created and passed through BestAvailableEncryption to better indicate this is optional and specific behavior?
  • [] It doesn't really make sense to mention the default of 16 at the level of BestAvailableEncryption because it is only applied in the ssh module, but I don't have a better way to express that without discussing interface boundaries.
  • [] Not sure which version this would be a candidate for so version_added is missing from the docs.

Requested originally in https://github.com/ansible-collections/community.crypto/issues/449

Ajpantuso avatar Jul 21 '22 16:07 Ajpantuso

Thank you for working on this! As you've discovered, our API doesn't have a clear place to override any options about how encryption is performed. Unfortunately, simply adding kdf_rounds to BestAvailableEncryption is probably not correct. We'll need to design an API that considers that KDF rounds may not be meaningful for some encryption options, and that it will have different meaning for different algorithms.

If you're interested in doing some of that design work, that's fantastic and we're very much up for working through these issues with you. https://github.com/pyca/cryptography/issues/4272 and https://github.com/pyca/cryptography/pull/5656 and https://github.com/pyca/cryptography/pull/6569 contain some of the background on the challenges here -- though we don't need to solve all the problems discussed in them!

alex avatar Jul 22 '22 00:07 alex

Thanks for the initial review. I'll put some thought into this, but my gut feeling is that OpenSSH private key encryption is not really aligned with the concept of BestAvailableEncryption to begin with i.e. it made reuse of this pattern for programming convenience and not for the sake of a coherent API. That being said to avoid breaking changes it is probably best to not touch that class for the time being so existing behavior remains intact and instead create a new encryption subclass closer to the OpenSSH domain. It may be a little clunky, but ultimately I think it's better than treating OpenSSH like it's OpenSSL.

Ajpantuso avatar Jul 22 '22 01:07 Ajpantuso

Pushed a rough draft of a new KeySerializationEncryption interface which attempts to sprinkle in some polymorphism making new behavior less risky to implement. It does add some unnecessary detail to the abstract interface which could be pulled into a second abstraction layer like PasswordBasedEncryption, but not sure if that additional complexity is valuable at this point. The options property is a little loosely typed for my tastes, but then again adding a new data structure to contain the optional parameters isn't particularly 'pythonic'. Defaulted kwargs should avoid breaks and users shouldn't really instantiate KeySerializationEncryption directly, but it's public so a second set of eyes on that would be nice. Choose not to update docs or address cov report until this draft is reviewed.

Ajpantuso avatar Jul 22 '22 20:07 Ajpantuso