-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reuse OpenSSL::SSL::Context::Client
in HTTP::Client
(12x speedup for HTTPS requests)
#15419
Comments
You can already create a context and tell MY_GLOBAL_CONTEXT = OpenSSL::SSL::Context::Client.new
HTTP::Client.new(host, port, tls: MY_GLOBAL_CONTEXT) |
Thanks for reporting and digging into the issue. Especially the references to related discussions are very valuable. I think it's worth noting though that Still creating explicit I do agree that it would be a very useful feature if Instead of caching a generic, global instance as |
Actually, it appears that for older versions libssl, every SSL client socket may mutate the context for hostname validation 😮 crystal/src/openssl/ssl/socket.cr Lines 8 to 29 in 0da1746
That would suggest that SSL context reuse isn't even concurrency safe when used to connect to different hostnames... When |
There is also an issue with cache invalidation. If we store and reuse an SSL context it won't pick up on changes to the ca store on disk. This is particularly relevant for long-running processes. So we should consider how to handle cache invalidation. For example, libcurl by default uses a CA cache timeout of 24 hours (https://curl.se/libcurl/c/CURLOPT_CA_CACHE_TIMEOUT.html). Another option could be to compare time stampts of the ca files. I'm not sure how feasible this is though, as it would require that information from within the SSL library. |
Agree. A perfect argument to drop support for 1.0.2. Even Ubuntu 18.04 had 1.1.1 so that would likely break nothing.
Good point. We'll need a timeout, but it would have to be configurable somehow... Note: curl applies the timeout per CA (not globally), and also checks if the CA changed to invalidate the cache. See https://github.com/curl/curl/blob/553248f501762735c6aa5531f5748e88aefb5314/lib/vtls/openssl.c#L3515-L3560 |
Hum, we should dig into It seems curl is doing its own cache for some reason —maybe because it supports many libraries? |
https://docs.openssl.org/3.4/man3/SSL_CTX_new/#description
|
That doesn't mean that OpenSSL itself can't safely maintain internal caches in the context. |
I don't think SSL context even remembers where certs are loaded from. We probably need to take care of this ourselves. For a default context this shouldn't be too difficult though. We can use the |
Oof, good catch. 😢 https://linux.die.net/man/3/ssl_ctx_set_cert_verify_callback says:
so if we wanted to continue supporting older versions, we might do something like: @context_mutex.synchronize do
context.set_cert_verify_callback(hostname)
@ssl = LibSSL.ssl_new(context)
context.clear_cert_verify_callback # LibSSL.set_cert_verify_callback(@handle, nil)
end Yes, once I discovered this issue, I made a global shared For our monitoring use-case, we are intentionally trying to exercise the full network stack on every request (to make sure the customer's DNS / TCP / load balancer / SSL / application full-stack is all behaving as expected), so we do use I also dug in and I do think this is (at least partially) an OpenSSL 3.x issue.
In the last of these, the requests 2.32.0 changelog says:
Given that this is possibly mostly a performance regression that may eventually get fixed upstream in OpenSSL, and that an end-user workaround is easy by creating my own |
Maybe OpenSSL might be able to improve the performance of loading certificates. But loading them for every request is always going to be slower than not loading them. And I don't think it's actually very complex, at least not for the default of |
TLDR:
HTTP::Client.get
HTTPS request in 16.92 ms 🐢 vs 1.43 ms 🐇The bug:
HTTP::Client
creates a newOpenSSL:SSL::Context::Client
for everyhttps://...
connection.Desired fix: use a global, default
OpenSSL::SSL::Context::Client
in HTTP::ClientWe have a sharded cluster of Crystal processes doing a large number of HTTPS requests per second at Heii On-Call because we're continuously monitoring our customers' API endpoints and websites. I'm embarrassed that I've only just discovered that we're burning an order of magnitude more CPU time on HTTPS requests than we need to be, all because
HTTP::Client
is creating a newOpenSSL:SSL::Context::Client
for every new connection.Creating a new SSL_CTX is slow because of loading CA certificates
OpenSSL::SSL::Context::Client.new
callsOpenSSL::SSL::Context#set_default_verify_paths
, which callsLibSSL.ssl_ctx_set_default_verify_paths
. This loads all of the system CA certificates:Here's a quick benchmark:
(Note that this issue may vary based on OpenSSL versions. It's possibly a performance regression that is being tracked openssl/openssl#20286 .)
SSL_CTX is designed to be set up once and reused
HTTP::Client.get benchmark
(
SSLTEST_URL
is an internal HTTPS service running on the same machine.)Possible resolution
In
src/openssl/ssl/context.cr
, I could imagine:class_getter(default) { new }
and then in
src/http/client.cr#initialize
replacing the call toOpenSSL::SSL::Context::Client.new
withOpenSSL::SSL::Context::Client.default
.I've tested it -- this works and fixes the performance issue.
However, in #2689 I saw that @jhass removed a
OpenSSL::SSL::Context.default
so as not to expose a global mutable default, which makes sense.Is there a good solution here? It would be great to use a single global default
SSL_CTX
for performance, but it seems like leaking the potentially-mutable context is inevitable. For example, could we create aReadOnlyContext
that didn't allow mutation?The text was updated successfully, but these errors were encountered: