-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Limit OpenSSL TLS Session cache for client SSL_CTX instances #110154
Conversation
@wfurt we've talked about this recently, so I put it up lest I forget, but this change might not be as useful once we figure out the bug causing the "leak" in the linked issue. |
private const string TlsCacheSizeCtxName = "System.Net.Security.TlsCacheSize"; | ||
private const string TlsCacheSizeEnvironmentVariable = "DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE"; | ||
private const int DefaultTlsCacheSizeClient = 500; // since we keep only one TLS Session per hostname, 500 should be enough to cover most scenarios | ||
private const int DefaultTlsCacheSizeServer = -1; // use implementation default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we have two different values I'm wondering if we should also split the variables...? The 500 for client seems like good default to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that as well, but we either would have to
- rename the existing configuration knob (which could be argued to be a breaking change)
- introduce two more knobs to kontrol client/server separately
- introduce one new with a server/client suffix and reuse the existing one for the other role
and neither of the above is a clear winner so I wanted to not complicate things needlessly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm fine with keeping it as it is. In most cases the workloads would be separated anyway and the once where they don't may simply share the same value as they do today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Test failure possibly related
|
Only partially related. in TLS 1.3, OpenSSL may keep the session in the cache even if the callback rejects it. Since we reject sessions without target hostname, then when the cached item is evicted from the internal cache, we hit the assert from previous comment. (and we hit it in this PR because we lowered the limit, so the eviction happens earlier). This does not cause any problems in the production because we still check against null later in the code. The scenario which triggers this is when IP address is used instead of the target host. Then we would (wrongly) enable session tracking, but (correctly) still would not resume sessions, because we look for only non-ip target host names in the cache. I fixed this by not enabling session tracking in this uncommon scenario. |
/azp run runtime-libraries-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
This pull request limits the OpenSSL TLS Session cache for client SSL_CTX instances to 500, which should be enough for most use cases and should save some memory resources for clients.
Exceptional use cases (web scrapers?) can increase the cache size via AppCtx switch or environmental variable.
We should still investigate issues like #109600 to make sure we are not needlessly overpopulating the sessions cache.