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

bpo-31429: Define TLS cipher suite on build time #3532

Merged
merged 1 commit into from Jan 29, 2018

Conversation

tiran
Copy link
Member

@tiran tiran commented Sep 13, 2017

Until now Python used a hard coded white list of default TLS cipher
suites. The old approach has multiple downsides. OpenSSL's default
selection was completely overruled. Python did neither benefit from new
cipher suites (ChaCha20, TLS 1.3 suites) nor blacklisted cipher suites.
For example we used to re-enable 3DES.

Python now defaults to OpenSSL DEFAULT cipher suite selection and black
lists all unwanted ciphers. Downstream vendors can override the default
cipher list with --with-ssl-default-suites.

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue31429

@tiran tiran added type-feature A feature request or enhancement type-security A security issue labels Sep 13, 2017
@encukou
Copy link
Member

encukou commented Jan 18, 2018

Is there any way I can help move this forward?

@tiran
Copy link
Member Author

tiran commented Jan 18, 2018

I'm not yet sure this patch can land in 3.7. LibreSSL is messing up my plans.

@tiran tiran changed the title bpo-31429: [WIP] --with-ssl-default-suites bpo-31429: Define TLS cipher suite on build time Jan 20, 2018
@tiran
Copy link
Member Author

tiran commented Jan 20, 2018

This PR can be merged after #3462 has landed or we forcefully unsupport OpenSSL < 1.0.2.

@tiran tiran force-pushed the bpo-31429-cipher-suite branch 2 times, most recently from fddbdb2 to 236097d Compare January 28, 2018 21:20
@tiran tiran requested review from alex, njsmith and pitrou January 28, 2018 21:21
@tiran
Copy link
Member Author

tiran commented Jan 28, 2018

@ned-deily Fedora upstream would love have this PR in 3.7. It's also going to make your life as 3.7 security maintainer easier.

@alex @njsmith @pitrou Please review

Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok minus these comments, but I'm not really qualified to review configure changes.

Lib/ssl.py Outdated
@@ -531,7 +489,7 @@ def create_default_context(purpose=Purpose.SERVER_AUTH, *, cafile=None,
context.verify_mode = CERT_REQUIRED
context.check_hostname = True
elif purpose == Purpose.CLIENT_AUTH:
context.set_ciphers(_RESTRICTED_SERVER_CIPHERS)
pass
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just remove this elif block entirely :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1 @@
[WIP] Add --with-ssl-default-suites
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably needs the WIP removed :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I forgot to update the old blurb. I have created a new one and also added 3.7 whatsnew.

Modules/_ssl.c Outdated
@@ -74,6 +74,10 @@ static PySocketModule_APIObject PySocketModule;
# endif
#endif

#if OPENSSL_VERSION_NUMBER < 0x1000200fL
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed after the other change to require 1.0.2?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, I added it for doubleplusgood paranoia reasons and just removed it.

@tiran tiran force-pushed the bpo-31429-cipher-suite branch 2 times, most recently from ed16875 to 724de80 Compare January 28, 2018 21:41
Until now Python used a hard coded white list of default TLS cipher
suites. The old approach has multiple downsides. OpenSSL's default
selection was completely overruled. Python did neither benefit from new
cipher suites (ChaCha20, TLS 1.3 suites) nor blacklisted cipher suites.
For example we used to re-enable 3DES.

Python now defaults to OpenSSL DEFAULT cipher suite selection and black
lists all unwanted ciphers. Downstream vendors can override the default
cipher list with --with-ssl-default-suites.

Signed-off-by: Christian Heimes <christian@python.org>
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks both fine and useful to me.

@tiran tiran merged commit 892d66e into python:master Jan 29, 2018
@bedevere-bot
Copy link

@tiran: Please replace # with GH- in the commit message next time. Thanks!

@tiran tiran deleted the bpo-31429-cipher-suite branch January 29, 2018 13:10
sfackler added a commit to sfackler/rust-openssl that referenced this pull request Feb 21, 2018
Based off of python/cpython#3532, we use OpenSSL's default cipher list
and turn of things we don't like. This can't be used with 1.0.1,
however, which had a poor default set. There, we use the old defaults,
with the bits that aren't implemented in 1.0.1 removed (namely TLSv1.3
suites and ChaCha).
sfackler added a commit to sfackler/rust-openssl that referenced this pull request Feb 21, 2018
Based off of python/cpython#3532, we use OpenSSL's default cipher list
and turn of things we don't like. This can't be used with 1.0.1,
however, which had a poor default set. There, we use the old defaults,
with the bits that aren't implemented in 1.0.1 removed (namely TLSv1.3
suites and ChaCha).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement type-security A security issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants