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

Add SSL_CTX_set_min_proto_version/SSL_CTX_set_max_proto_version bindings #985

Merged
merged 8 commits into from Mar 10, 2021

Conversation

mhils
Copy link
Member

@mhils mhils commented Dec 21, 2020

This PR adds the SSL_CTX_set_min_proto_version/SSL_CTX_set_max_proto_version protocol selection mechanisms along with TLS_method/TLS_client_method/TLS_server_method to pyOpenSSL.

depends on pyca/cryptography#5662, fixes #860

@mhils mhils force-pushed the set_proto_version branch 3 times, most recently from 21c68dc to 25ac2b6 Compare December 21, 2020 15:14
@mhils
Copy link
Member Author

mhils commented Dec 21, 2020

This should generally be ready for review, but depends on a new cryptography release (and then a bump of the minimum version here). 😃

@mhils mhils marked this pull request as ready for review February 8, 2021 11:00
@mhils
Copy link
Member Author

mhils commented Feb 8, 2021

Tests for 3.6 and above are passing now. 😃

2.7 and 3.5 are failing because they are not supported by the latest cryptography release anymore. I presume you want to drop them for pyOpenSSL as well soon-ish? :)

@reaperhulk
Copy link
Member

We'll try to keep 2.7 support in pyOpenSSL until the minimum version has to be bumped to support new bindings. These bindings were recently added, but were they in the 3.3 release?

@reaperhulk
Copy link
Member

I could have scrolled up in this PR. I guess we didn't, which means this requires 3.4? 🤣

@mhils
Copy link
Member Author

mhils commented Feb 8, 2021

One could ship a 3.3 release with hardcoded constants if it needs to be, but bumping to 3.4 would be cleaner.

Also - having just taken a quick look at the cryptography tracker - thank you all for the fantastic work! ❤️ All of my projects happily accepted 3.4, everything worked out of the box, and I can't wait to see more memory-safe code being used. :)

@reaperhulk
Copy link
Member

Given pyOpenSSL's common use in more legacy environments I think we probably should just hardcode the constants (with comments that state when we move to 3.4+ of cryptography we can switch to getting them from cryptography) and allow the use of 3.3 still. I'd like to keep 2.7 support here another year if possible.

And thanks for the kind words -- it's going to be a rough couple weeks 😄

@reaperhulk
Copy link
Member

We dropped py3.5 in 3.3 so you'll need to update the CI config to remove the py35 configs. Sorry about that!

@mhils
Copy link
Member Author

mhils commented Feb 8, 2021

No worries, I was just about to ask if you want me to do it. :)

@mhils mhils force-pushed the set_proto_version branch 2 times, most recently from 86e229b to e982888 Compare February 8, 2021 15:07
Base automatically changed from master to main February 13, 2021 17:50
@reaperhulk
Copy link
Member

Also needs a changelog rebase

@reaperhulk reaperhulk added this to the 21.0.0 milestone Feb 28, 2021
@mhils mhils force-pushed the set_proto_version branch 2 times, most recently from cbe1843 to 345dbd2 Compare March 7, 2021 16:32
@reaperhulk reaperhulk closed this Mar 7, 2021
@reaperhulk reaperhulk reopened this Mar 7, 2021
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@mhils mhils force-pushed the set_proto_version branch 3 times, most recently from 0bf9a11 to e0e7d4b Compare March 7, 2021 20:02
@mhils
Copy link
Member Author

mhils commented Mar 7, 2021

Turns out the culprit was that constants require single quotes, not double quotes. 🤷‍♂️

Any hints on the py27 job failing with "ERROR: InterpreterNotFound: python2.7"?

@reaperhulk
Copy link
Member

Looks like https://github.com/pyca/infra/blob/main/runners/debian/Dockerfile#L20 isn't doing its job. Looking inside the container that's because /etc/debian_version no longer contains a name and instead just has a version number. I wonder how long this has been true...

@reaperhulk
Copy link
Member

pyca/infra#340

@mhils
Copy link
Member Author

mhils commented Mar 7, 2021

Ok, removing PIP_NO_BINARY=cryptography in tox.ini fixes the ubuntu-bionic tests:

PIP_NO_BINARY=cryptography

PIP_NO_BINARY=:all: was introduced by @hynek in 8b5ee6f ("wheels make testing of various OpenSSLs on OS X impossible") and then changed to only apply to cryptography in 0c21cbd ("Banning all breaks setuptools installation").

Is there a particular (security) reason why cryptography is not installed as a wheel?

If yes, what approach shall we take for the ubuntu-bionic container build? Without wheels, it errors building cryptography.
If no, this PR should be good to go.

@reaperhulk
Copy link
Member

I believe this was originally done so we could test alternate versions of OpenSSL against pyOpenSSL rather than just the ones our wheels ship. That seems like it's probably still worthwhile since we don't test that all of pyOpenSSL works on the set of OpenSSL versions cryptography tests against in its own matrix. So we should probably just make it so we can build the rust components in the bionic container.

@mhils
Copy link
Member Author

mhils commented Mar 8, 2021

Alright, this was in fact very straightforward in the end! 😃

@reaperhulk reaperhulk merged commit 5dc6988 into pyca:main Mar 10, 2021
@reaperhulk
Copy link
Member

Thanks for working through this @mhils

@mhils mhils deleted the set_proto_version branch March 10, 2021 21:51
@mhils
Copy link
Member Author

mhils commented Mar 10, 2021

Always welcome, thanks y'all! 😊

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

add SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version
2 participants