OpenVPN
ssl_openssl: Fix some CRL mixups
Hi all! I tried to send this to openvpn-devel but it looks like it might not have gotten through? (I'm probably confused or just did something wrong.) It sounds like you all do look at GitHub though, so I figure I'll start here and we can figure out the mailing list step afterwards?
I've tested this by making sure things build and that the tests pass. However, it looks like some tests were skipped because they require manually configuring some things in ways I hadn't figured out. It's also unclear to me how well-tested CRL support is. There's no mention of CRLs in the tests directory. I'm not sure how you all test CRL-related changes.
There are two ways to load CRLs in OpenSSL. They can be loaded at the X509_STORE, shared across verifications, or loaded per verification at the X509_STORE_CTX.
OpenVPN currently does the former. However, it also supports CRL reloading, and tries to reload the CRL file before each connection. OpenSSL does not really have a good way to unload objects from an X509_STORE. OpenVPN currently does it by grabbing the STACK_OF(X509_OBJECT) out of the X509_STORE and manually deleting all the CRLs from it.
This mutates an OpenSSL internal object which bumps into problems if OpenSSL ever switches to a more efficient representation. See https://github.com/openssl/openssl/pull/28599
(It's also not thread-safe, though it doesn't look like that impacts OpenVPN? Actually even reading that list doesn't work. See CVE-2024-0397. This OpenSSL API was simply broken.)
Additionally, this seems to cause two OpenVPN features to not work together. I gather backend_tls_ctx_reload_crl is trying to clear the CRLs loaded from last time it ran. But tls_ctx_load_ca with a ca_file can also load CRLs. tls_ctx_load_ca with ca_path will also pick up CRLs and backend_tls_ctx_reload_crl actually ends up clobbering some state X509_LOOKUP_hash_dir internally maintains on the X509_STORE. Likewise, tls_verify_crl_missing can get confused between backend_tls_ctx_reload_crl's crl_file-based CRLs and CRLs from tls_ctx_load_ca.
Avoid all this by tracking the two CRLs separately. crl_file-based CRLs now go onto a STACK_OF(X509_CRL) tracked on the tls_root_ctx. Now this field can be freely reloaded by OpenVPN without reconfiguring OpenSSL. Instead, pass the current value into OpenSSL at verification time. To do so, we need to use SSL_CTX_set_cert_verify_callback, which allows swapping out the X509_verify_cert call, and also tweaking the X509_STORE_CTX configuration before starting certificate verification.
Context: SSL_CTX_set_cert_verify_callback and the existing verify_callback are not the same. SSL_CTX_set_cert_verify_callback wraps the verification while verify_callback is called multiple times throughout verification. It's too late to reconfigure X509_STORE_CTX in verify_callback. verify_callback is usually not what you want. Sometimes current_cert and error_depth don't quite line up, and cert_hash_remember may end up called multiple times for a single certificate.
I suspect some of the other verify_callback logic would also be better done in the new callback, but I've left it alone to keep this change minimal. verify_callback is really only usable for suppressing errors. Application bookkeeping is better done elsewhere.
The preferred way for patch submission (these days) is our gerrit, https://gerrit.openvpn.com.
The second best way is the mailing list, but it has been a bit unreliable in the last weeks. Also, you need to be subscribed to be allowed to post (but my colleagues should see the moderation request and permit it).
Github MRs will not be merged - they can serve as a starting point for the discussion (and we do use GH issues), but in the end the final patch needs to be taken to the list or gerrit - one of us can help with it, if needed.
Now, for the issue at hand, I defer to people who understand OpenSSL :-)
I'm guessing that was meant to be https://gerrit.openvpn.net. The other URL took me somewhere confusing. :-) If I followed the links right, looks Gerrit uses the wiki credentials and I need to ask IRC to get an account? I've gone and done that.
(There's no mention of the Gerrit at https://github.com/OpenVPN/openvpn/blob/master/CONTRIBUTING.rst by the way. Maybe worth updating?)
To close the loop, the conclusion from IRC seems to have been that Gerrit accounts are only given to long-time contributors and that, for a one-off fix, GitHub was probably best?
I'll defer to you all on how you wish to do this. For now, I'm assuming that this is the right place and that this PR is just awaiting review. Let me know if you all would prefer another venue change.
Gerrit accounts should be also given to other contributers from my understanding. We only automatically run the tests automatically for long time contributers/trusted users.
@schwabe is right. Sorry @davidben for the back and forth. Any chance you could create an account at https://gerrit.openvpn.net/pwm/public/newuser and push the patch to gerrit? There it'll be easier to review and comment.
I... attempted to make an account but I keep getting "The browser session is invalid or has expired. Please try again" in the after the first page. Sometimes it breaks on the first page too. I tried both Chrome and Safari.
I... attempted to make an account but I keep getting "The browser session is invalid or has expired. Please try again" in the after the first page. Sometimes it breaks on the first page too. I tried both Chrome and Safari.
@davidben it looks like we have an issue with captcha. To speedup the process I created an account for you with the username and email you tried. Could you please try to log in by resetting the password via https://gerrit.openvpn.net/pwm/public/forgottenpassword
Sorry, this got buried in my inbox. Thanks! I got the account working and pushed the change to https://gerrit.openvpn.net/c/openvpn/+/1289
(It took me a bit to figure out the git URL to push to. I couldn't find it documented and had to guess URLs until I found it. Might be worth documenting that somewhere, unless I just missed it.)
Sorry, this got buried in my inbox. Thanks! I got the account working and pushed the change to https://gerrit.openvpn.net/c/openvpn/+/1289
(It took me a bit to figure out the git URL to push to. I couldn't find it documented and had to guess URLs until I found it. Might be worth documenting that somewhere, unless I just missed it.)
Actually that's not the URL you should be using (and I'm surprised that this actually works). But it seems Gerrit is smart enough to take incoming commits on any URL...
The documentation for uploading is here (linked from the "documentation" tab on top of gerrit): https://gerrit.openvpn.net/Documentation/user-upload.html
I think we can close the PR, plus the private-issue report, and focus on the gerrit? Having this in only one place simplifies keeping track on open issues (and poking fellow developers to un-forget things .-) ).
Sure. Whatever works for you all. Any things for me to do on the Gerrit? Seems to have been quiet.
Sure. Whatever works for you all. Any things for me to do on the Gerrit? Seems to have been quiet.
We've been distracted by a heap of other reports... so, "deal with things in gerrit" is our responsibility - there might be questions asked or changes requested, but until then, you do not have to do anything. If it takes too long, feel free to remind us (on IRC or by mail).
I'm not the one who understands OpenSSL innards, so I can't deal with the actual changes - I'm just doing the janitor, trying to clean our backlog ;-)