| msg281230 - (view) |
Author: Steve Dower (steve.dower) *  |
Date: 2016-11-19 21:36 |
| As a prerequisite for fixing issues such as (dynamic download/update of CAs and CRLs), we really need to be able to plug into the certificate verification function for OpenSSL. This patch adds SSLContext._set_cert_verify_callback, which will allow Python code to inject its own verification function. No other functionality is added, but I have proof-of-concept code that uses this patch to delegate all certificate handling to Windows and it works beautifully (better than I expected :) ). If possible, I'd like to get this into Python 3.6. I intend to turn that proof-of-concept into an actual released library and would like to be able to do it sooner rather than later. Targeting 3.6 is the main reason I named the function with an underscore, but I'd be happy to drop it. |
|
|
| msg281231 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2016-11-19 21:37 |
| Patch attached with code changes and basic tests. Doc changes to follow. |
|
|
| msg281233 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2016-11-19 21:39 |
| Oh, I also made the SSL module chain exceptions properly. That's probably the biggest and scariest part of the change, but it can't have been overwriting exceptions before anyway (because it calls back into Python code to instantiate SSLError), so it's only going to chain in the new case of the callback function raising. |
|
|
| msg281235 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2016-11-19 23:08 |
| Hi Steve, there is a better approach to fix . The verify callback is not the correct API, because it is called too late. We want to hook into the cert resolution mechanism of OpenSSL and get trust anchors and CRLs in before OpenSSL builds the verification chain. Instead of a verify cb we have to implement a X509_LOOKUP_METHOD with a get_by_subject(). The function looks up X509_LU_CRL or X509_LU_X509 by X509_NAME. The other lookups functions (fingerprint, issuer) aren't used to look up root CAs. Then use some CAPI function like CertFindCertificateInStore() with CERT_FIND_SUBJECT_NAME to look up the cert, convert it to OpenSSL X509 object, copy the additional trust flags from Windows' cert type to the X509_CERT_AUX member of OpenSSL's X509 type. |
|
|
| msg281237 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2016-11-20 00:44 |
| When I was stepping through, this callback avoided all of those lookups, so I don't understand how it's being called too late? This approach basically takes the entire raw certificate and lets the OS do whatever it needs. OpenSSL doesn't ever have to crack it open at all (or at least when it does, it can assume the whole chain is trusted). What am I missing here? I've got no doubt I'm missing something, as OpenSSL is possibly the most complicated code I've ever seen :) |
|
|
| msg281247 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2016-11-20 04:14 |
| Few patch updates - better tests and some docs. |
|
|
| msg281260 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2016-11-20 10:52 |
| IMHO SSL CTX set cert verify callback() is the wrong approach. Your are completely bypassing cert validation checks of OpenSSL. The callback has to build the chain and perform all checks on its own. By all checks I literally mean *all*, https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_cert_verify_callback(3)#WARNINGS Basically you want to replace OpenSSL's X509 verification with Windows' cert validation and just leave the handshake and encryption to OpenSSL? |
|
|
| msg281262 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2016-11-20 12:53 |
| > Basically you want to replace OpenSSL's X509 verification with Windows' cert validation and just leave the handshake and encryption to OpenSSL? Yep. (See WinVerifyTrust for the Windows API I'm using.) |
|
|
| msg281318 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2016-11-21 02:30 |
| Should have assigned this to me, as I expect I'll be the one to apply it. Christian - I need to look to you for whether I've exposed the right function here and it's not adding security risk (obviously excluding a broken callback implementation). I *think* it's okay, but I trust your greater experience here. 3.6.0b4 is being tagged tomorrow and I think this is worth getting in - hence why I added Ned. There's no added functionality and no impact if the API isn't used. The latest patch removes the '_' prefix but happy to put it back and leave it as undocumented. If neither of you have any concerns, I'll check it in. As I mentioned, at some point early in Python 3.6's release I should have a library available that lets the OS do certificate validation, but it needs this callback exposed. |
|
|
| msg281324 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2016-11-21 06:34 |
| With the patch (_2), clang (and gcc 4.2) on macOS warn: ./Modules/_ssl.c:3968:7: warning: assigning to 'unsigned char *' from 'char *' converts between pointers to integer types with different sign [-Wpointer-sign] p = PyBytes_AS_STRING(enc_cert); ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
|
|
| msg281325 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2016-11-21 06:45 |
| And, as it stands, the tests fail (at least on macOS): ====================================================================== ERROR: test_set_cert_verify_callback (test.test_ssl.SimpleBackgroundTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/py/dev/36/root/fwd_macports/Library/Frameworks/pytest_10.12.framework/Versions/3.6/lib/python3.6/test/test_ssl.py", line 1782, in test_set_cert_verify_callback ctx._set_cert_verify_callback(callback) AttributeError: 'SSLContext' object has no attribute '_set_cert_verify_callback' ====================================================================== ERROR: test_set_cert_verify_callback_error (test.test_ssl.SimpleBackgroundTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/py/dev/36/root/fwd_macports/Library/Frameworks/pytest_10.12.framework/Versions/3.6/lib/python3.6/test/test_ssl.py", line 1805, in test_set_cert_verify_callback_error ctx._set_cert_verify_callback(raise_error) AttributeError: 'SSLContext' object has no attribute '_set_cert_verify_callback' ====================================================================== ERROR: test_set_cert_verify_callback_suppress_error (test.test_ssl.SimpleBackgroundTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/py/dev/36/root/fwd_macports/Library/Frameworks/pytest_10.12.framework/Versions/3.6/lib/python3.6/test/test_ssl.py", line 1827, in test_set_cert_verify_callback_suppress_error ctx._set_cert_verify_callback(raise_error) AttributeError: 'SSLContext' object has no attribute '_set_cert_verify_callback' ---------------------------------------------------------------------- Ran 130 tests in 27.416s FAILED (errors=3, skipped=8) |
|
|
| msg281326 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2016-11-21 06:50 |
| We are two weeks from producing the release candidate for 3.6.0. I don't think we should be rushing to add a new security-critical API which, IIUC, won't be used in the initial release anyway. Let's target it for 3.7 after proper review and then we can decide whether it makes sense to backport to a 3.6.x maint release if the security issues it may solve warrant it. |
|
|
| msg281382 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2016-11-21 16:48 |
| Whoops, that's what I get for renaming something. Easily fixed, but I'm happy to aim for 3.7. |
|
|
| msg281390 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2016-11-21 20:22 |
| For the sake of review, I fixed the patch and rebased it on default. |
|
|
| msg283517 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2016-12-17 20:51 |
| The current _3.patch builds on default without warning and the tests pass (_2.patch is the one Ned tried). Any objections to committing this into 3.7? What about 3.6.1? As an additive and easy to detect API, I think it's suitable, and I will certainly use it (right now my library's setup.py depends on having each libeay.lib from each original CPython build handy to get some of the functions out of it - these files are about 50MB each, so it's a little painful). If it helps, I'm happy to add a warning to the docs that setting the callback may cause a loss of security if the callback does not properly validate the certificate (or some wording to that effect). Personally I think that's fairly well implied though, as there isn't any other obvious use for the callback. |
|
|
| msg283519 - (view) |
Author: Ned Deily (ned.deily) * |
Date: 2016-12-17 21:33 |
| From a release manager perspective, I'm OK in principle with adding it to 3.6.1 as long as the ssl experts are OK with it. |
|
|
| msg284207 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2016-12-28 23:22 |
| Any comment from the SSL experts? |
|
|
| msg309594 - (view) |
Author: Steve Dower (steve.dower) * |
Date: 2018-01-07 01:46 |
| The change to make OpenSSL a separate DLL (on Windows, at least, which is all I really care about) means this function is available via ctypes. That's good enough for me, so I'll close this. |
|
|
| msg349645 - (view) |
Author: Marcelo Salhab Brogliato (msbrogli) |
Date: 2019-08-14 05:37 |
| Exposing SSL_CTX_set_cert_verify_callback is useful when we want to use a Public Key Authentication, like it is done in the SSH Protocol. Do you know any other way to use Public Key Authentication besides using SSL_CTX_set_cert_verify_callback? |
|
|