FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-107361: strengthen default SSL context flags by woodruffw · Pull Request #112389 · python/cpython · GitHub

/ cpython Public

gh-107361: strengthen default SSL context flags - #112389

Merged
gpshead merged 19 commits into
python:mainfrom
woodruffw-forks:default-ssl-verify-flags
Mar 6, 2024
Merged

gh-107361: strengthen default SSL context flags#112389
gpshead merged 19 commits into
python:mainfrom
woodruffw-forks:default-ssl-verify-flags

Conversation

woodruffw commented Nov 25, 2023
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

See #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.

CC @sethmlarson


📚 Documentation preview 📚: https://cpython-previews--112389.org.readthedocs.build/

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.
sethmlarson added the type-security A security issue label Nov 25, 2023
gpshead self-assigned this Nov 29, 2023
Comment thread Doc/library/ssl.rst

bedevere-app Bot commented Nov 29, 2023

Copy link
Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

pitrou commented Nov 29, 2023

Copy link
Copy Markdown
Member

Side note: perhaps we mark all PEM files as generated?

Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>
Signed-off-by: William Woodruff <william@yossarian.net>

Copy link
Copy Markdown
Contributor Author

Side note: perhaps we mark all PEM files as generated?

Done: I've marked certdata/*.{pem,0} as generated (the .0 files should also really be .pem I think -- I can do some cleanup there in a follow-up PR, if there's interest).

Comment thread Doc/whatsnew/3.13.rst

Copy link
Copy Markdown
Member

In practice, this should have no effect on >99% of users: most root programs use 5280 as their baseline, and the Web PKI's CABF rules are also built on 5280.

Your PR required to regenerate all PEM test certificates. Does it mean that they belong to the 1% of affected users?

Copy link
Copy Markdown
Contributor Author

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 VERIFY_X509_STRICT. The fact that the PEMs had to be regenerated is more a testament to how lax OpenSSL is by default than any actual common practice 🙂

Or put another way: most users are not manually generating certs with openssl req or, if they are, they're massaging them into RFC 5280 compliance because other ecosystems (like Go, Java, and OSes/major software) are much more strict.

Additional datapoints:

(I picked these semi-arbitrarily from https://pkic.org/ltl/)

sethmlarson left a comment
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

pitrou commented Dec 1, 2023

Copy link
Copy Markdown
Member

Since OpenSSL will likely be the way folks are generating certificates

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)?

is there any value in providing guidance on how one does generate a certificate with OpenSSL that works with VERIFY_X509_STRICT?

I don't think writing an OpenSSL command line tutorial is really a responsibility for CPython maintainers.

Copy link
Copy Markdown
Contributor Author

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?

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).

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

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.

Copy link
Copy Markdown
Member

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 ssl._create_unverified_context() or a custom one to the same effect for those.

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.

Copy link
Copy Markdown
Contributor Author

Gentle ping on this! I've deconflicted the changelog again.

gpshead added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 6, 2024

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @gpshead for commit cef6950 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Mar 6, 2024

gpshead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM - I'm just running a round of buildbot tests to look for any surprises before merging.

…flags

Signed-off-by: William Woodruff <william@yossarian.net>
gpshead merged commit 0876b92 into python:main Mar 6, 2024
woodruffw deleted the default-ssl-verify-flags branch March 6, 2024 21:47
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
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 <william@yossarian.net>
Co-authored-by: Victor Stinner <vstinner@python.org>

Copy link
Copy Markdown
Contributor

@woodruffw Looks like this change breaks trustme generated certificates, see python-trio/trustme#642

Copy link
Copy Markdown
Contributor Author

@woodruffw Looks like this change breaks trustme generated certificates, see python-trio/trustme#642

Thanks for sharing! Looking now.

Copy link
Copy Markdown
Member

@woodruffw Looks like this change breaks trustme generated certificates, see python-trio/trustme#642

If an application is affected by these changes, they can "just" disable the two options, no?

Using ctx.verify_flags &= ~(ssl.VERIFY_X509_STRICT | ssl.VERIFY_X509_PARTIAL_CHAIN):

$ ./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>

Copy link
Copy Markdown
Contributor Author

If an application is affected by these changes, they can "just" disable the two options, no?

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.

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
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 <william@yossarian.net>
Co-authored-by: Victor Stinner <vstinner@python.org>

This comment was marked as abuse.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type-security A security issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants


Back | FazBrowse Home | New Git URL