gh-80192: Use windows api if ssl cert verification fails #127622

tobil4sk · 2024-12-05T00:47:15Z

This avoids rejecting certificates that should be accepted, see #80192.

Here is the relevant openssl documentation: https://docs.openssl.org/master/man3/SSL_CTX_set_verify/.

Also, here are the windows api docs:

I've been testing manually by deleting DigiCert Global Root G2 from Third Party Root Certificates in the user certificate store (this is safe, the certificate will be added back automatically), and running the following code:

import http.client
conn = http.client.HTTPSConnection("www.verisign.com")
conn.request("GET", "/")
conn.close()

The main branch errors with:

ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1025)

Whereas with this patch, the certificate is verified and the connection is established.

Issue: ssl - tls verify on Windows fails #80192

This avoids rejecting certificates that should be accepted, see python#80192

ghost · 2024-12-05T00:47:18Z

All commit authors signed the Contributor License Agreement.

asvetlov · 2024-12-05T10:44:48Z

Such change requires some tests at least I guess.

ZeroIntensity

Yeah, this needs a test. FYI, test_ssl is failing.

ZeroIntensity · 2024-12-05T15:42:26Z

Misc/NEWS.d/next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst

@@ -0,0 +1 @@
+Fixed valid ssl certificates being rejected.


Use a reference:

Suggested change

Fixed valid ssl certificates being rejected.

Fixed valid :mod:`ssl` certificates being rejected.

Reference added

I would see a broader text, e.g. the PR verifies certifications rejected by OpenSSL by additional calls to Windows specific API functions.
The exact proposed text is definitely not fine, but you have got my idea.

ZeroIntensity · 2024-12-05T15:44:15Z

Modules/_ssl.c

+        return NULL;
+    }
+
+    cert_bytes = PyMem_RawMalloc(cert_bytes_length);


Is it possible to use pymalloc (PyMem_Malloc) here? Typically, we only need RawMalloc if we plan on using it inside Py_BEGIN_ALLOW_THREADS areas.

Using PyMem_Malloc I got the following error in my sample program:
Fatal Python error: _PyMem_DebugMalloc: Python memory allocator called without holding the GIL

I figured. It might be worth adding a comment mentioning that the GIL isn't held (so no Python APIs can get called).

Modules/_ssl.c

ZeroIntensity · 2024-12-05T15:50:23Z

Modules/_ssl.c

+    case CERT_E_WRONG_USAGE:
+    case CERT_E_CRITICAL:
+    case CERT_E_PURPOSE:


There's a chance we need to use _Py_FALLTHROUGH here, but it looks like build isn't complaining. If it does start failing feel free to use that.

CERT_E_UNTRUSTEDROOT: A certification chain processed correctly but terminated in a root certificate that is not trusted by the trust provider. X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY: The issuer certificate could not be found: this occurs if the issuer certificate of an untrusted certificate cannot be found. - https://docs.openssl.org/master/man3/X509_STORE_CTX_get_error/#error-codes - https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/ns-wincrypt-cert_chain_policy_status#members

Modules/_ssl.c

tobil4sk · 2024-12-05T17:58:45Z

Such change requires some tests at least I guess.

Ok, I'll have to look into how to create a sample certificate chain that reproduces the problem.

zooba · 2025-03-07T11:10:59Z

I don't think you can reliably test this other than by deleting a root CA, at least not for the default behaviour. You might need to refactor things to be able to call the verification function directly (from tests), and also to validate that it has been set on SSLContext objects.

Also, just FYI, historically our SSL experts have roundly rejected overriding the verification callback, which is why we haven't done this (it's not like we were unaware of this approach 😉 ). I don't recall the details, but it might be worth someone doing the archaeology on GitHub and perhaps the old email archives at mail.python.org to see what the specifics were.

For my part, I know that many projects have documented the OpenSSL environment variables that affect certificate validation. If possible, we should try and ensure those work (possibly just by checking if they're set and if so, don't override the callback).

zooba · 2025-03-07T11:13:29Z

Misc/NEWS.d/next/Windows/2024-12-05-00-21-11.gh-issue-80192.28PTj4.rst

@@ -0,0 +1 @@
+Fixed valid :mod:`ssl` certificates being rejected.


Agree with Andrew's comment - this needs to be significantly expanded, because this is a very impactful change.

We also would need a section in Doc/whatsnew/3.14.rst explaining the impact and options for workarounds.

We might also need to use a deprecation period to turn this on by default, though part of me would prefer not to.

zooba · 2025-03-07T11:15:15Z

I wonder if @sethmlarson has thoughts?

sethmlarson · 2025-03-11T15:45:39Z

@zooba I think the latest discussion on the topic was this thread on DPO (cc @woodruffw). In general I'm in favor of moving to using system certificate stores by default, but I think it should happen through the PEP process to get the API right.

woodruffw · 2025-03-11T16:03:48Z

I'm in favor of moving to using system certificate stores by default, but I think it should happen through the PEP process to get the API right.

Agreed with this.

(Tangentially, I think the approach in this patch is unfortunately hard to follow -- if the goal is to inject the Windows cert store into the path validation flow, then IMO that should be done by loading the store and injecting it into the OpenSSL equivalent, rather than adding a verification callback and calling the equivalent WinCrypt APIs in situ. The latter effectively means that two path validation flows are running at the same time!)

zooba · 2025-03-11T17:31:07Z

if the goal is to inject the Windows cert store into the path validation flow, then IMO that should be done by loading the store and injecting it into the OpenSSL equivalent, rather than adding a verification callback and calling the equivalent WinCrypt APIs in situ.

Unfortunately, this is precisely what doesn't work, because Windows will update the store dynamically based on validation requests. So you can't just read it into OpenSSL and be done (or this wouldn't continue being raised as a problem for the last 15+ years).

IMHO, I'd love to drop OpenSSL on Windows entirely in favour of the native TLS support (unless you specifically want it, in which case you can use the native TLS support to install it). I suspect I'm the only person who thinks in those terms though, and everyone else assumes that OpenSSL is inevitable. I don't have the bandwidth outside of work to build the library, unfortunately, and I don't have support from work to release it publicly, so probably people should stop asking my opinion about this, since they never seem to like it (and in exchange, I'll stop paying attention to users who hit this issue 😉 )

woodruffw · 2025-03-11T18:34:21Z

because Windows will update the store dynamically based on validation requests. So you can't just read it into OpenSSL and be done (or this wouldn't continue being raised as a problem for the last 15+ years).

Oh, huh. Is this because they're doing a variant of the Chrome intermediate cache? I wasn't aware any OS-native TLS stacks were doing that!

Given that, I suspect there are two slightly different goals here: there's "use the OS root of trust so that Python and PyPI don't operate informal root programs anymore," and there's "use the OS root of trust so that Python's HTTPS behavior aligns more closely with browser agents in terms of fixing real-world broken chains."

IMHO, I'd love to drop OpenSSL on Windows entirely in favour of the native TLS support (unless you specifically want it, in which case you can use the native TLS support to install it). I suspect I'm the only person who thinks in those terms though, and everyone else assumes that OpenSSL is inevitable.

I'm a big fan of this! I'd really love it if PEP 748 (or a replacement) could fit this exactly, although my colleagues and I are unfortunately in a similar position around bandwidth.

sethmlarson · 2025-03-11T18:35:55Z

because Windows will update the store dynamically based on validation requests. So you can't just read it into OpenSSL and be done (or this wouldn't continue being raised as a problem for the last 15+ years).

Oh, huh. Is this because they're doing a variant of the Chrome intermediate cache? I wasn't aware any OS-native TLS stacks were doing that!

This is the reason why Truststore doesn't simply load certificates into an OpenSSL store, we use Windows' validation. Doesn't macOS do the same?

woodruffw · 2025-03-11T20:21:28Z

This is the reason why Truststore doesn't simply load certificates into an OpenSSL store, we use Windows' validation. Doesn't macOS do the same?

I'm not sure -- I'm aware of this technique in the browser world for handling broken/misconfigured HTTPS servers, but I wasn't aware of it being employed within the OS validator itself (I was under the impression that Chrome et al. did it by maintaining a browser-side intermediate cache, which they then fed into the system APIs, with the latter remaining stateless).

Looking at this some more, I'm not actually sure that's what's happening in the Windows case: I think the Windows system validator might be doing AIA chasing instead, i.e. fetching the intermediate from the AuthorityInformationAccess endpoint instead of from a local intermediate cache. That would make a lot more sense than a global browser-style intermediate cache at the OS level, but someone who knows these platform verifiers better than I do could answer definitively 😅

zooba · 2025-03-11T23:50:52Z

Looking at this some more, I'm not actually sure that's what's happening in the Windows case: I think the Windows system validator might be doing AIA chasing instead, i.e. fetching the intermediate from the AuthorityInformationAccess endpoint instead of from a local intermediate cache.

Windows used to distribute root CA updates through Windows Update, so my assumption is that it's a replica of that process, just more finely grained. Probably also works as an alternative to CRLs.

The Windows cert store isn't just used for TLS, so the design is going to be less biased towards online scenarios, as it has to include software and driver certifications as well. Though it wouldn't surprise me if most code signing CAs are included out of the box, or else OEMs/users would have a terrible time adding drivers.

Use windows api if ssl cert verification fails

9d28198

This avoids rejecting certificates that should be accepted, see python#80192

bedevere-app bot added the awaiting review label Dec 5, 2024

bedevere-app bot mentioned this pull request Dec 5, 2024

ssl - tls verify on Windows fails #80192

Open

Use const where appropriate

d3da5df

ZeroIntensity added the OS-windows label Dec 5, 2024

ZeroIntensity reviewed Dec 5, 2024

View reviewed changes

tobil4sk added 4 commits December 5, 2024 17:05

Refactor error translation into separate function

b8f5bd5

Fix PEP 7 violations

c9ad0a3

Add reference to news entry

80b21bd

tobil4sk commented Dec 5, 2024

View reviewed changes

Modules/_ssl.c Show resolved Hide resolved

zooba reviewed Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-80192: Use windows api if ssl cert verification fails #127622

gh-80192: Use windows api if ssl cert verification fails #127622

tobil4sk commented Dec 5, 2024 •

edited

Loading

ghost commented Dec 5, 2024 •

edited by ghost

Loading

asvetlov commented Dec 5, 2024

ZeroIntensity left a comment

ZeroIntensity Dec 5, 2024

tobil4sk Dec 5, 2024

asvetlov Dec 5, 2024 •

edited

Loading

ZeroIntensity Dec 5, 2024

tobil4sk Dec 5, 2024

ZeroIntensity Dec 5, 2024

ZeroIntensity Dec 5, 2024

tobil4sk commented Dec 5, 2024

zooba commented Mar 7, 2025

zooba Mar 7, 2025

zooba commented Mar 7, 2025

sethmlarson commented Mar 11, 2025

woodruffw commented Mar 11, 2025 •

edited

Loading

zooba commented Mar 11, 2025

woodruffw commented Mar 11, 2025

sethmlarson commented Mar 11, 2025

woodruffw commented Mar 11, 2025

zooba commented Mar 11, 2025

	Fixed valid ssl certificates being rejected.
	Fixed valid :mod:`ssl` certificates being rejected.

		@@ -0,0 +1 @@
		Fixed valid :mod:`ssl` certificates being rejected.

gh-80192: Use windows api if ssl cert verification fails #127622

Are you sure you want to change the base?

gh-80192: Use windows api if ssl cert verification fails #127622

Conversation

tobil4sk commented Dec 5, 2024 • edited Loading

ghost commented Dec 5, 2024 • edited by ghost Loading

asvetlov commented Dec 5, 2024

ZeroIntensity left a comment

Choose a reason for hiding this comment

ZeroIntensity Dec 5, 2024

Choose a reason for hiding this comment

tobil4sk Dec 5, 2024

Choose a reason for hiding this comment

asvetlov Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

ZeroIntensity Dec 5, 2024

Choose a reason for hiding this comment

tobil4sk Dec 5, 2024

Choose a reason for hiding this comment

ZeroIntensity Dec 5, 2024

Choose a reason for hiding this comment

ZeroIntensity Dec 5, 2024

Choose a reason for hiding this comment

tobil4sk commented Dec 5, 2024

zooba commented Mar 7, 2025

zooba Mar 7, 2025

Choose a reason for hiding this comment

zooba commented Mar 7, 2025

sethmlarson commented Mar 11, 2025

woodruffw commented Mar 11, 2025 • edited Loading

zooba commented Mar 11, 2025

woodruffw commented Mar 11, 2025

sethmlarson commented Mar 11, 2025

woodruffw commented Mar 11, 2025

zooba commented Mar 11, 2025

tobil4sk commented Dec 5, 2024 •

edited

Loading

ghost commented Dec 5, 2024 •

edited by ghost

Loading

asvetlov Dec 5, 2024 •

edited

Loading

woodruffw commented Mar 11, 2025 •

edited

Loading