Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pin maximum version cryptography to prevent breaking changes #1143

Closed
thesuperzapper opened this issue Sep 13, 2022 · 6 comments
Closed

pin maximum version cryptography to prevent breaking changes #1143

thesuperzapper opened this issue Sep 13, 2022 · 6 comments

Comments

@thesuperzapper
Copy link

Currently we pin cryptography>=37.0.2 in our install_requires, which will no longer work as of the release of version 38.0.0 of cryptography, as there was a breaking change specifically, the removal of X509_V_FLAG_CB_ISSUER_CHECK.

This issue has been raised numerous times across the internet, so we should push out a fix ASAP:


To resolve this issue (and prevent it from happening in the future), I propose that update our pin to cryptography>=37.0.2,<38.0.0, and manually verify each major version bump of cryptography going forwards.

@thesuperzapper
Copy link
Author

@alex @mhils @reaperhulk, sorry for the ping, but I think this is pretty important, as currently, pyOpenSSL will not work if a user pip installs without already having an older version of cryptography installed.

@mhils
Copy link
Member

mhils commented Sep 13, 2022

I don't see X509_V_FLAG_CB_ISSUER_CHECK being used in pyOpenSSL 22.0 (it was removed in #982). Are you using an older version of pyOpenSSL?

@alex
Copy link
Member

alex commented Sep 13, 2022

Yes, the root cause here is older pyOpenSSLs with brand new cryptographys.

On the one hand pinning would prevent this case. It'd also require us to do a fresh release of pyopenssl for every cryptography release. Maybe that makes sense? I'm loath to do even more pyopenssl work, but maybe the simple trade-off is best.

The other potential solution would be a new pip/packaging feature to allow packages to declare constraints that are not dependencies. Then cryptography releases could say "not compatible with pyopenssl x" and pip would take it into account, but not create a dependency on pyopenssl if the user wasn't using it. But that'd be a bunch of work.

@thesuperzapper
Copy link
Author

@alex maybe I am confused, but I thought that pyOpenSSL depends on cryptography, not the other way around?

If so, it makes sense to expect that sometimes a release of cryptography will include breaking changes that the current version of pyOpenSSL will be compatible with.

Pip only has one method for defining dependencies, and that is install_requires.

Note, we don't have to release pyOpenSSL for every cryptography release if we trust cryptography to only make breaking changes when they bump major versions.

If we don't do this, and continue only using a cryptography>=X.X.X pin, we will inevitably end up in situations where pip install pyOpenSSL will install an incompatible version of cryptography if the user has not already got cryptography installed (which is the situation we are in right now).

@thesuperzapper
Copy link
Author

Note, when I say "which is the situation we are in right now", I am referring to the fact that every release of pyOpenSSL before version 22.0.0 is incompatible with cryptography versions from 38.0.0, but pip does not know this, so can't correctly resolve the dependency issue.

Ideally, we would want the following to be true:

  1. When running pip install cryptography==38.0.0 (while already having pyOpenSSL older than 22.0.0 installed), pip should know to upgrade the installed pyOpenSSL to at least 22.0.0
  2. When running pip install pyOpenSSL==21.0.0 (while already having cryptography newer than 38.0.0 installed), pip should know to downgrade the installed cryptography to 37.0.4

With pip's current dependency resolution, using cryptography>=X.X.X,<Y.Y.Y as our pin will result in the above behavior for new releases of pyOpenSSL, but will still not fix older releases like 21.0.0, because pip will think everything is a-OK and happily allow cryptography newer than 38.0.0 to be installed alongside it.

So we are in a very difficult state to resolve, and probably a warning needs to be put in the docs that all versions of pyOpenSSL from before we introduce the pin (so, 22.0.0 and older) will require you to explicitly install a compatible version of cryptography.

@alex
Copy link
Member

alex commented Sep 15, 2022

After spending some time mulling this over, I'm convinced that pinning the cryptography dep, and then bumping + release on any major cryptography release is the way to go.

simondeziel added a commit to simondeziel/charm-lxd that referenced this issue Oct 6, 2022
This ensures the OS provided python3-openssl package won't be used
as it conflicts with a newer cryptography module that is installed
through pip. See pyca/pyopenssl#1143 for
details.

Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
simondeziel added a commit to simondeziel/charm-lxd that referenced this issue Oct 6, 2022
This ensures the OS provided python3-openssl package won't be used
as it conflicts with a newer cryptography module that is installed
through pip. See pyca/pyopenssl#1143 for
details.

Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

3 participants