Resolve SSL_PRIVATE_METHOD and certificate slots functionality #2429
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2429 +/- ##
=======================================
Coverage 78.89% 78.89%
=======================================
Files 621 621
Lines 108613 108666 +53
Branches 15414 15421 +7
=======================================
+ Hits 85685 85736 +51
- Misses 22258 22259 +1
- Partials 670 671 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
skmcgrail
force-pushed
the
ssl-priv-method-fixes
branch
from
2fafbeb to
438bc26
Compare
skmcgrail
force-pushed
the
ssl-priv-method-fixes
branch
from
438bc26 to
5d10331
Compare
| cert_pkey->privatekey = UpRef(privkey); | ||
| cert->key_method = privkey_method; | ||
|
|
||
| if(privkey != nullptr) { |
There was a problem hiding this comment.
Just confused about the logic here. If privkey is null, then we should set privkey_method. But if it's not null, then reset it and don't set the privkey_method?
It looks like before, regardless of whether cert_pkey-> privatekey was null, it would be set to privkey. And privkey_method would also be assigned. Why this distinction now?
There was a problem hiding this comment.
I think this got answered for you in the other comment, but the two are mutually exclusive and only one of them can be passed to this function. So this just ensures that state is kept and reflected correctly to avoid confusion or future bugs around the state of the CERT type instance.
| @@ -104,6 +104,7 @@ static bool ssl_set_pkey(CERT *cert, EVP_PKEY *pkey) { | |||
|
|
|||
| // Update certificate slot index once all checks have passed. | |||
| cert->cert_private_keys[idx].privatekey = UpRef(pkey); | |||
| cert->key_method = nullptr; // key_method should be cleared since we've set a private key | |||
There was a problem hiding this comment.
I guess same question here as above, what is the reason behind key_method being set to Null if a privkey has been set?
There was a problem hiding this comment.
You can't use both features at the same time, and if a slot private key is set, then that will be used if not NULL, otherwise it will use key_method. They are mutually exclusive features so it doesn't really make sense to keep a state where both are present. IMO
There was a problem hiding this comment.
Ahhh okay got it, so they do get used exclusively but for whatever reason our code didn't set them exclusively previously?
Thanks!
## What's Changed * Fix prefix build when path has spaces by @justsmth in #2400 * Prepare v1.51.2 by @justsmth in #2401 * Set OPENSSL_NO_EXTERNAL_PSK_TLS13 to indicate lack of TLS 1.3 PSK by @WillChilds-Klein in #2399 * BIO datagram functions by @justsmth in #2321 * Reject NewSessionTicket messages with empty tickets in TLS 1.3 by @justsmth in #2367 * Ensure that AVX512 is not used on macOS by @justsmth in #2363 * Fix socket test issues by @torben-hansen in #2404 * Remove python CI patch for main by @WillChilds-Klein in #2407 * Remove xmlsec patch by @smittals2 in #2405 * Fix clang tidy ci by @justsmth in #2375 * Mark fallible container operations as `nodiscard` by @justsmth in #2366 * Remove extra va_end in err_add_error_vdata by @justsmth in #2364 * Check for QUIC in SSL_process_quic_post_handshake by @justsmth in #2365 * Add missing symbols for Unbound by @nhatnghiho in #2352 * Update mlkem-native by @hanno-becker in #2406 * CI for iOS by @justsmth in #2389 * Squelch clang-tidy by @justsmth in #2414 * Clang-tidy is still noisy by @justsmth in #2417 * Add back two rules for clang-tidy by @smittals2 in #2418 * Implement BIO_dump by @kingstjo in #2331 * Make ASN1_get_object a direct call by @samuel40791765 in #2332 * Add Python 3.9 CI patch by @WillChilds-Klein in #2415 * Rework memory BIOs and implement BIO_seek by @nhatnghiho in #2380 * ML-DSA: ASN.1 Module - add parsing of BOTH private key format by @jakemas in #2416 * Detection of unused results by @justsmth in #2411 * Fix gtest_util.sh failure detection by @justsmth in #2423 * Remove unused docs/configs by @torben-hansen in #2427 * ML-DSA: Add ML-DSA keyGen to break-kat.go by @jakemas in #2422 * Fix CI for mingw by @justsmth in #2428 * Bump AWSLC_API_VERSION for X509_STORE_CTX_set_verify_crit_oids by @samuel40791765 in #2426 * Revert "Rework memory BIOs and implement BIO_seek (#2380)" by @samuel40791765 in #2432 * Resolve SSL_PRIVATE_METHOD and certificate slots functionality by @skmcgrail in #2429 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
Description of changes:
There was an issue with trying to use
SSL_PRIVATE_KEY_METHODfunctionality that was working for the most part, but had some edge scenarios that had stopped working. For example setting theprivkey_methodargument when callingSSL_CTX_set_chain_and_keyorSSL_set_chain_and_key. Originally I thought this feature had been in a complete broken state, but upon further investigation realized that the bssl runner framework was using this functionality for testing async and hints functionality. The caveat was the framework was using alternative functions to directly set the certificate and private key method without using this entrypoint. In theory this entrypoint should work as it is for the most part coded correctly, but had a bug in that is tried to use the private key to determine the slot index when it could possibly be null. This corrects that behavior to correctly use the public key, to allow the chain to be set correctly, but still delegate the private key operations to the registeredSSL_PRIVATE_KEY_METHODprovided.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.