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

gh-79846: Make ssl.create_default_context() ignore invalid certificates #91740

Merged
merged 6 commits into from
Aug 7, 2024

Conversation

pukkandan
Copy link
Contributor

@pukkandan pukkandan commented Apr 20, 2022

Fixes #79846, fixes #89475

Currently, when loading certificates from the Windows certificate store, error in any one certificate causes ssl.create_default_context() to crash. This causes issues in systems that have certificates that are not quite to-spec. A primary culprit for this is "MUPCA Root", which (despite being is technically invalid) is essential for citizens of Serbia

See the conversations under the linked issues for more details

I believe it makes sense for create_default_context to ignore any invalid certificates in the system store. An existing comment in the related code seems to agree with me on this:

cpython/Lib/ssl.py

Lines 772 to 774 in 8497514

# no explicit cafile, capath or cadata but the verify mode is
# CERT_OPTIONAL or CERT_REQUIRED. Let's try to load default system
# root CA certificates for the given purpose. This may fail silently.

This issue can be solved by loading each certificate one by one and ignoring any SSLErrors. I had outlined the idea for this patch in the above-mentioned issue, but never recieved any reply on whether this is acceptable. Hopefully, this PR receives better attention

An error in one certificate should not cause the whole thing to crash

Fixes python#79846, python#89475
@ghost
Copy link

ghost commented Apr 20, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Every change to Python requires a NEWS entry.

Please, add it using the blurb_it Web app or the blurb command-line tool.

@pukkandan pukkandan changed the title gh-79846: Load CA certificates one by one in ssl.create_default_context() gh-79846: Make ssl.create_default_context() ignore invalid certificates Apr 20, 2022
@serhiy-storchaka
Copy link
Member

The issues was closed and I cannot reproduce the original problem.

@ideasman42
Copy link
Contributor

This issue still seems to be a problem.
While I don't use MS-Windows, multiple Blender users look to have run into this with Blender 4.2, using Python 3.11.
See: https://projects.blender.org/blender/blender/issues/124731#

Lib/ssl.py Outdated
if trust is True or purpose.oid in trust:
certs.extend(cert)
try:
self.load_verify_locations(cadata=cert)
Copy link
Member

Choose a reason for hiding this comment

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

This loads the certificate unconditionally, unlike the current code.

@serhiy-storchaka serhiy-storchaka requested review from sethmlarson and a team July 30, 2024 09:47
hubot pushed a commit to blender/blender that referenced this pull request Jul 30, 2024
Workaround: `[ASN1] nested asn1 error` error when making HTTPS
connections on systems with certificates that OpenSSL cannot parse
are installed.

This is a general issue with Python, resolve by applying a proposed
fix [0] to the extensions Python process at run-time.
(this doesn't impact Blender's Python run-time).

The down side is HTTPS connections will only work for extensions
on systems with this problem so this needs to be resolved by Python
long term.

While any changes to Python's SSL checks is worth avoiding,
this simply skips SSL certificates in the windows store that OpenSSL
can't parse instead of failing all SSL connections.

See related issues:

- python/cpython#79846
- openssl/openssl#25023

[0]: python/cpython#91740

Ref !124943.
@zooba
Copy link
Member

zooba commented Aug 6, 2024

Is calling load_verify_locations multiple times okay? It doesn't replace the old loaded certs in any OpenSSL versions, right?

hubot pushed a commit to blender/blender that referenced this pull request Aug 6, 2024
Workaround: `[ASN1] nested asn1 error` error when making HTTPS
connections on systems with certificates that OpenSSL cannot parse
are installed.

This is a general issue with Python, resolve by applying a proposed
fix [0] to the extensions Python process at run-time.
(this doesn't impact Blender's Python run-time).

The down side is HTTPS connections will only work for extensions
on systems with this problem so this needs to be resolved by Python
long term.

While any changes to Python's SSL checks is worth avoiding,
this simply skips SSL certificates in the windows store that OpenSSL
can't parse instead of failing all SSL connections.

See related issues:

- python/cpython#79846
- openssl/openssl#25023

[0]: python/cpython#91740

Ref !124943.
@serhiy-storchaka
Copy link
Member

There are tests that call load_verify_locations() multiple times. For example, test_load_verify_cadata checks that calling load_verify_locations() multiple times with different certificates increases some counter, but calling it with already loaded certificate does not. Also, calling it with concatenated certificates has the same effect on this counter as calling it separately for every certificate.

I have no plausible scenario in which it would replace the old loaded certificates, but can detect duplicates in statistics. Looking at the code of cert_store_stats(), it requests statistic from the library, not from Python-level cache.

@zooba
Copy link
Member

zooba commented Aug 6, 2024

If it's tested, then that's fine. This change LGTM

@serhiy-storchaka serhiy-storchaka merged commit 9e551f9 into python:main Aug 7, 2024
33 of 34 checks passed
@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Aug 7, 2024
@miss-islington-app
Copy link

Thanks @pukkandan for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @pukkandan for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 7, 2024
…ificates (pythonGH-91740)

An error in one certificate should not cause the whole thing to fail.

(cherry picked from commit 9e551f9)

Co-authored-by: pukkandan <pukkandan.ytdlp@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 7, 2024
…ificates (pythonGH-91740)

An error in one certificate should not cause the whole thing to fail.

(cherry picked from commit 9e551f9)

Co-authored-by: pukkandan <pukkandan.ytdlp@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Aug 7, 2024

GH-122768 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Aug 7, 2024
@bedevere-app
Copy link

bedevere-app bot commented Aug 7, 2024

GH-122769 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Aug 7, 2024
terryjreedy pushed a commit that referenced this pull request Aug 9, 2024
…tificates (GH-91740) (#122769)

An error in one certificate should not cause the whole thing to fail.

(cherry picked from commit 9e551f9)

Co-authored-by: pukkandan <pukkandan.ytdlp@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
…ificates (pythonGH-91740)

An error in one certificate should not cause the whole thing to fail.

Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Yhg1s pushed a commit that referenced this pull request Sep 2, 2024
…tificates (GH-91740) (#122768)

gh-79846: Make ssl.create_default_context() ignore invalid certificates (GH-91740)

An error in one certificate should not cause the whole thing to fail.

(cherry picked from commit 9e551f9)

Co-authored-by: pukkandan <pukkandan.ytdlp@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants