Skip to content

Conversation

Ajpantuso
Copy link
Contributor

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 ansible-collections/community.crypto#449

@Ajpantuso Ajpantuso force-pushed the ajpantuso/add_kdf_rounds_encryption_param branch 4 times, most recently from 7660598 to e6ea365 Compare July 21, 2022 18:21
@Ajpantuso Ajpantuso force-pushed the ajpantuso/add_kdf_rounds_encryption_param branch from e6ea365 to 7361099 Compare July 21, 2022 18:27
@alex
Copy link
Member

alex commented Jul 22, 2022

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. #4272 and #5656 and #6569 contain some of the background on the challenges here -- though we don't need to solve all the problems discussed in them!

@Ajpantuso
Copy link
Contributor Author

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 Ajpantuso changed the title feat: add kdf_rounds param to encryption algorithm [WIP] feat: add kdf_rounds param to encryption algorithm Jul 22, 2022
@Ajpantuso
Copy link
Contributor Author

Ajpantuso commented Jul 22, 2022

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.

@alex alex added this to the Thirty Eighth Release milestone Aug 4, 2022
@Ajpantuso Ajpantuso deleted the ajpantuso/add_kdf_rounds_encryption_param branch August 22, 2022 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants