-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
gh-107361: strengthen default SSL context flags #112389
gh-107361: strengthen default SSL context flags #112389
Conversation
See python#107361: this adds `VERIFY_X509_STRICT` to make the default SSL context perform stricter (per RFC 5280) validation, as well as `VERIFY_X509_PARTIAL_CHAIN` to enforce more standards-compliant path-building behavior. As part of this changeset, I had to tweak `make_ssl_certs.py` slightly to emit 5280-conforming CA certs. This changeset includes the regenerated certificates after that change.
Misc/NEWS.d/next/Library/2023-11-24-23-40-00.gh-issue-107361.v54gh46.rst
Show resolved
Hide resolved
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be poked with soft cushions! |
Side note: perhaps we mark all PEM files as generated? |
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Done: I've marked |
Your PR required to regenerate all PEM test certificates. Does it mean that they belong to the 1% of affected users? |
I made the numbers up for emphasis, but I'll stand by this having no effect on the vast majority of users: the vast majority use case is the Web PKI, which is significantly stricter than even Or put another way: most users are not manually generating certs with Additional datapoints:
(I picked these semi-arbitrarily from https://pkic.org/ltl/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ++ on the partial chain changes, my comments below apply to VERIFY_X509_STRICT
:
Since OpenSSL will likely be the way folks are generating certificates and like you said, OpenSSL allows you to do things that isn't acceptable for Web PKI, is there any value in providing guidance on how one does generate a certificate with OpenSSL that works with VERIFY_X509_STRICT
?
Also since we're recommending folks down a new code path to restore old behavior, should we create a new test which exercises that code path and verifies that "bad" certificates would be accepted?
What about non-Web PKI, does this change have any affect for certificates in that domain? I am less versed for this, so am relying a bit on others' experience.
Is that still the case? Aren't most users relying on something like Let's Encrypt nowadays (at least for Web certificates, but also perhaps e-mail)?
I don't think writing an OpenSSL command line tutorial is really a responsibility for CPython maintainers. |
To clarify: I think most users won't be using OpenSSL to generate certs -- I think the majority will be getting their certs from a CA service (like Let's Encrypt), with a very small minority doing bespoke things. Putting an OpenSSL example in the docs might cause additional confusion, and IMO won't be too helpful anyways (since the context here is verification, in which case the user might not have any control over the certs they're being presented with).
Nope, this should be wholly compatible with those -- "strict" validation for OpenSSL means "approximate RFC 5280," and the Web PKI profiles are a superset of RFC 5280 🙂 In other words: all public web certificates should be compatible with these changes. |
These are the checks that OpenSSL applies when VERIFY_X509_STRICT is set: https://www.openssl.org/docs/man3.0/man1/openssl-verification-options.html#x509_strict (search for -x509_strict in case the link doesn't take you there directly) These checks all make sense for certs generated with a trusted CA, e.g. Let's Encrypt. The other common use case is having self-signed certs (e.g. for intranet/VPC devices). Those will already fail to verify with the current standard settings, since self-signed certs are rejected by OpenSSL's verify. You have to use the Note: The POP/FTP/IMAP and SMTP modules use the unverified context, since intranet/VPC mail and FTP servers will often not use certs created by a trusted CA for various reasons or they are referenced using IP addresses, which don't work with certs at all. |
@@ -2945,6 +2956,36 @@ def test_ecc_cert(self): | |||
cipher = s.cipher()[0].split('-') | |||
self.assertTrue(cipher[:2], ('ECDHE', 'ECDSA')) | |||
|
|||
def test_verify_strict(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sethmlarson This test provides a backstop check on VERIFY_X509_STRICT
, PTAL!
Sorry for the delay here! I've updated this PR to include a testcase for strict verification, including a "backstop" test that ensures that disabling |
OpenSSL is not consistent about error types between versions.
I'm a little mystified by this error, which only happens on OpenSSL 1.1.1w in CI:
AFAICT, the strict flag exists in that version and has the same semantics as in OpenSSL 3.0 and 3.1, which both pass. Edit: This is probably why: openssl/openssl@1e41dad (and openssl/openssl#10688) |
This should be good to go again: I've added a "backstop" test that confirms that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, apologies for the delay!
It's "only" 3 months late on my end, so no hard feelings 😉 |
050b4eb
to
1a3e037
Compare
…flags Signed-off-by: William Woodruff <[email protected]>
Gentle ping on this! I've deconflicted the changelog again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I'm just running a round of buildbot tests to look for any surprises before merging.
…flags Signed-off-by: William Woodruff <[email protected]>
This adds `VERIFY_X509_STRICT` to make the default SSL context perform stricter (per RFC 5280) validation, as well as `VERIFY_X509_PARTIAL_CHAIN` to enforce more standards-compliant path-building behavior. As part of this changeset, I had to tweak `make_ssl_certs.py` slightly to emit 5280-conforming CA certs. This changeset includes the regenerated certificates after that change. Signed-off-by: William Woodruff <[email protected]> Co-authored-by: Victor Stinner <[email protected]>
@woodruffw Looks like this change breaks trustme generated certificates, see python-trio/trustme#642 |
Thanks for sharing! Looking now. |
If an application is affected by these changes, they can "just" disable the two options, no? Using $ ./python
Python 3.13.0a5+ (heads/main:b44898299a2, Mar 28 2024, 09:01:21) [GCC 13.2.1 20240316 (Red Hat 13.2.1-7)] on linux
>>> import ssl
>>> ctx=ssl.create_default_context()
>>> ctx.verify_flags & ssl.VERIFY_X509_STRICT
<VerifyFlags.VERIFY_X509_STRICT: 32>
>>> ctx.verify_flags & ssl.VERIFY_X509_PARTIAL_CHAIN
<VerifyFlags.VERIFY_X509_PARTIAL_CHAIN: 524288>
>>> ctx.verify_flags &= ~(ssl.VERIFY_X509_STRICT | ssl.VERIFY_X509_PARTIAL_CHAIN)
>>> ctx.verify_flags & ssl.VERIFY_X509_STRICT
<VerifyFlags.VERIFY_DEFAULT: 0>
>>> ctx.verify_flags & ssl.VERIFY_X509_PARTIAL_CHAIN
<VerifyFlags.VERIFY_DEFAULT: 0> |
Yep, correct. I think the above is also generating certs, so they wanted to get it right more generally, so that each Python downstream consumer doesn't have to also download the strict option. |
This adds `VERIFY_X509_STRICT` to make the default SSL context perform stricter (per RFC 5280) validation, as well as `VERIFY_X509_PARTIAL_CHAIN` to enforce more standards-compliant path-building behavior. As part of this changeset, I had to tweak `make_ssl_certs.py` slightly to emit 5280-conforming CA certs. This changeset includes the regenerated certificates after that change. Signed-off-by: William Woodruff <[email protected]> Co-authored-by: Victor Stinner <[email protected]>
See #107361: this adds
VERIFY_X509_STRICT
to make the default SSL context perform stricter (per RFC 5280) validation, as well asVERIFY_X509_PARTIAL_CHAIN
to enforce more standards-compliant path-building behavior.As part of this changeset, I had to tweak
make_ssl_certs.py
slightly to emit 5280-conforming CA certs. This changeset includes the regenerated certificates after that change.CC @sethmlarson
ssl.create_default_context()
: addVERIFY_X509_STRICT
andVERIFY_X509_PARTIAL_CHAIN
to the defaultverify_flags
#107361📚 Documentation preview 📚: https://cpython-previews--112389.org.readthedocs.build/