jaraco / keyrings.alt

MIT License
24 stars 24 forks source link

Fall back to pycryptodome if pycryptodomex is not installed #46

Closed TheChymera closed 2 years ago

TheChymera commented 2 years ago

Pycryptodomex is a compatibility wrapper for pycryptodome, with all code and functionality otherwise being identical.

yarikoptic commented 2 years ago

FWIW, initially was skeptical on "why bother" but checking the description of differences between the two (coming from the same source/project): https://github.com/Legrandin/pycryptodome#pycryptodome realized that indeed there should be no need to favor one over another. But then there should be also tune up to CI testing to ensure that it does work with both where in one setup it would install pycryptodome instead of pycryptodomex . ATM that dependency listed only in extras_require testing, so should be easy to implement.

jaraco commented 2 years ago

Thanks for the contrib. This change adds some complexity asymmetry to the code. If we're going to adopt this change, I'd like to see the selection/switching of backing libraries abstracted (rather than implemented inline with different methods of the keyring).

Before we do that, however, I'd like more background on the value proposition for this change ("why bother"). You've demonstrated that it can be done, but I see no reason why it should be done. Why isn't it sufficient to identify a preferred crypto implementation and use that? Is there some point in the future where one will supersede the other? What happens if a third fork is published - should that one be supported too?

But then there should be also tune up to CI testing to ensure that it does work with both where in one setup it would install pycryptodome instead of pycryptodomex

I noticed you +1 this comment. Are you still planning on adding testing for this mode?

TheChymera commented 2 years ago

a third fork

The issue is that pycryptodome and pycryptodomex are not forks. They are just different package names for the same library (i.e. there are no two different sources). This is explained (if a bit poorly) in the README https://github.com/Legrandin/pycryptodome

The distinction originated from the need to create a separate namespace back when pycrypto (which is now deprecated and of which pycryptodome is a fork) was still a thing and the namespaces would collide.

sufficient to identify a preferred crypto implementation

The different crypto implementations are “pycryptodome” and “cryptography”. The preferred one is pycryptodome, which is published both under its proper name and a historical workaround. In effect there is no need to use the workaround name anymore, but in order to not take anybody by surprise it would make sense to accept both names for the time being.

Are you still planning on adding testing for this mode?

If you think this is required in light of the above, sure, let me know if you're convinced this is a good idea.

TheChymera commented 2 years ago

@jaraco thank you 💛