Support OpenSSL3 providers / cert store URLs #8
Conversation
- Add serf_ssl_cert_uri_set(), a callback to set the URL of a certificate store. - Use the OSSL_STORE API from OpenSSL to read certificates and keys. Certs and keys are read from a URL instead of a file path. The default URL scheme is file:. - Keep fallback support for the existing serf_ssl_client_cert_provider_set() callback, which reads exclusively from a local PKCS12 file. - Support full intermediate certificate handling. Previously whatever was in the PKCS12 file was blindly passed to the the server on the assumption the administrator had pre-done the work constructing the certificate chain. Now we make no assumption as to the size of the certificate store, if a Windows personal certificate store of a MacOS keychain is used, we search for the most appropriate leaf certificate that matches what is requested by the server. - Update test cases to handle both URIs and PKCS12 files. Note: tests will fail on modern unix until reference to now-removed MD5 is fixed. This test failure is unrelated.
- Adds the config option ssl-client-cert-uri to specify the URI of a certificate store. - OpenSSL URIs point at files, or pkcs11 smartcards, or TPMs, or native platform certificate stores. - Maintains existing ssl-client-cert-file as fallback. - Depends on apache/serf#8 - Example configuration: [apachegroup] ssl-trust-default-ca = yes ssl-client-cert-uri = /home/minfrin/.my-cert.p12 ssl-client-cert-password = supersecret
CMakeLists.txt
Outdated
| CheckNotFunction("ASN1_STRING_get0_data" "NULL" "SERF_NO_SSL_ASN1_STRING_GET0_DATA" | ||
| "openssl/asn1.h" "${OPENSSL_INCLUDE_DIR}" | ||
| ${OPENSSL_LIBRARIES} ${SERF_STANDARD_LIBRARIES}) | ||
| CheckFunction("OSSL_STORE_open_ex" "NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL" "SERF_HAVE_OSSL_STORE_OPEN_EX" |
There was a problem hiding this comment.
Please try to stay below 80-columns (not just here).
| * | ||
| */ | ||
|
|
||
| static int ssl_x509_ex_data_idx = -1; |
buckets/ssl_buckets.c
Outdated
| #if defined(SERF_HAVE_OSSL_STORE_OPEN_EX) | ||
|
|
||
| if (ssl_x509_ex_data_idx < 0) { | ||
| ssl_x509_ex_data_idx = X509_get_ex_new_index(0, NULL, NULL, NULL, NULL); |
There was a problem hiding this comment.
This is not thread safe, nor are the other two uses of this variable. Serf's pipeline may be single-threaded asynchronous, but the restriction is only that a particular serf context may not be used in multiple threads. Access to the static variable must still be serialized. Could you use apr_atomic for this? Or maybe put this into the Serf context? Does it have to be a singleton per process?
There was a problem hiding this comment.
The variable is a process wide singleton.
X509_get_ex_new_index is a macro that calls this:
From https://docs.openssl.org/3.1/man3/CRYPTO_get_ex_new_index/
"Exdata types are identified by an index, an integer guaranteed to be unique within structures for the lifetime of the program. Applications using exdata typically call CRYPTO_get_ex_new_index at startup, and store the result in a global variable, or write a wrapper function to provide lazy evaluation."
Is there a once-off-init location in serf that this can be moved to?
There was a problem hiding this comment.
Moved to the init section.
git-svn-id: https://svn.apache.org/repos/asf/serf/branches/PR-8@1926893 13f79535-47bb-0310-9956-ffa450edef68
| ssl_x509_ex_data_idx = X509_get_ex_new_index(0, NULL, NULL, NULL, NULL); | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
This looks good to me now, even if X509_get_ex_new_index() isn't thread safe, have_init_ssl should make sure it can't be run by several threads at the same time.
However, do we need to CRYPTO_free_ex_index() the index, for example in cleanup_ssl?
|
As you have probably seen, I've created an SVN branch for this PR: https://svn.apache.org/repos/asf/serf/branches/PR-8. It builds and tests out ok, so I think it should be good to go. Except possibly the question of calling CRYPTO_free_ex_index() in case the library is unloaded. (Brane commented on the mailing list that the CRYPTO_locking_* functions doesn't exist on modern OpenSSL so here is no ssl_pool anymore and thus cleanup_ssl doesn't exist either - so much for an easy place to put this.) @brainy Any further comments from you? @minfrin Can you double check that my branch above matches what you expect? Would you like to do the honors of merging this to trunk? |
|
Only that the CRYPTO_locking_* stuff doesn't exist in OpenSSL 1.1.1, either. So it's probably been a long time since that code was actually used. (I haven't checked 1.1.0, but these versions are all dead anyway). |
One of the big things that OpenSSL3 fixed was cleanup. In earlier versions, you had to call all sorts of init functions, and at the end all sorts of cleanup functions, and this blew up horribly when openssl was linked to an application twice (think httpd+lotsamodules). To fix this, openssl deprecated and removed all the init and cleanup stuff, handling everything internally with proper reference counting. Long story short, openssl cleans up after itself, we mustn't touch anything, or things will break. |
Review is hugely appreciated, thank you - committed in r1926950. |
3 participants
Add serf_ssl_cert_uri_set(), a callback to set the URL of a certificate store.
Use the OSSL_STORE API from OpenSSL to read certificates and keys. Certs and keys are read from a URL instead of a file path. The default URL scheme is file:.
Keep fallback support for the existing serf_ssl_client_cert_provider_set() callback, which reads exclusively from a local PKCS12 file.
Support full intermediate certificate handling. Previously whatever was in the PKCS12 file was blindly passed to the the server on the assumption the administrator had pre-done the work constructing the certificate chain. Now we make no assumption as to the size of the certificate store, if a Windows personal certificate store of a MacOS keychain is used, we search for the most appropriate leaf certificate that matches what is requested by the server.
Update test cases to handle both URIs and PKCS12 files.
Note: tests will fail on modern unix until reference to now-removed MD5 is fixed. This test failure is unrelated.