gh-80192: Use windows api if ssl cert verification fails #127622
Conversation
This avoids rejecting certificates that should be accepted, see python#80192
|
All commit authors signed the Contributor License Agreement. |
|
Such change requires some tests at least I guess. |
| @@ -0,0 +1 @@ | |||
| Fixed valid ssl certificates being rejected. | |||
There was a problem hiding this comment.
Use a reference:
| Fixed valid ssl certificates being rejected. | |
| Fixed valid :mod:`ssl` certificates being rejected. |
There was a problem hiding this comment.
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.
| return NULL; | ||
| } | ||
|
|
||
| cert_bytes = PyMem_RawMalloc(cert_bytes_length); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Outdated
| case CERT_E_WRONG_USAGE: | ||
| case CERT_E_CRITICAL: | ||
| case CERT_E_PURPOSE: |
There was a problem hiding this comment.
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
Ok, I'll have to look into how to create a sample certificate chain that reproduces the problem. |
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 G2fromThird Party Root Certificatesin the user certificate store (this is safe, the certificate will be added back automatically), and running the following code:The main branch errors with:
Whereas with this patch, the certificate is verified and the connection is established.