Add timestamp-based expiration to cached SafeFreeCredentials #66334
Conversation
|
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsFixes Issue #43879 This PR adds timestamp-based invalidation of
|
rzikm
changed the title
| @@ -18,8 +19,13 @@ internal abstract class SafeFreeCredentials : DebugSafeHandle | |||
| internal abstract class SafeFreeCredentials : SafeHandle | |||
| { | |||
| #endif | |||
| internal DateTime _expiry; | |||
There was a problem hiding this comment.
I know this is tricky but AFAIK Unix does not have the same issue as Windows. I'm wondering if we should (need to?) replicate the logic.
There was a problem hiding this comment.
I wasn't sure how to do this so that the code compiles for both platforms, since the code for SslSessionsCache where I access the Expiry property is not platform specific. However, it would be possible to always leave MaxValue on non-windows platforms and set the expiry in SslStreamPal.Windows.cs
| cred._expiry = GetExpiryTimestamp(certificateContext); | ||
| } | ||
|
|
||
| static DateTime GetExpiryTimestamp(SslStreamCertificateContext certificateContext) |
There was a problem hiding this comment.
We can possibly also add it to SslStreamCertificateContext itself. It may be used multiple times but I guess the difference would be probably very small.
There was a problem hiding this comment.
I considered it, but I couldn't think of other uses elsewhere. We can always move it later when we need it.
…66334) * Add Expiry timestamp on SafeFreeCredentials handle * Recalculate expiration timestamp based on CertificateContext * Fix case when user provides CertificateContext
|
More possible regressions here: dotnet/perf-autofiling-issues#4279 |
|
that should be tracked by #68408 @AndyAyersMS. Basically all expired certificates will be more expensive now regardless of key size or negotiated protocol. |
ghost
locked as resolved and limited conversation to collaborators
Fixes #43879
This PR adds timestamp-based invalidation of
SafeFreeCredentialsinSslSessionCache. The expiration timestamp is calculated based onNotAfterfields of the certificates in theSslAuthenticationOptions.CertificateContext(both the actual certificates and the intermediate certs in the chain).Since this PR does not add any
X509Chain.Build()calls to the hot path, I believe there should not be a significant perf hit from this change. I can run benchmarks next week once I have access to my desktop machine.Also to consider: does it make sense to try to create a test for this? It would probably go to OuterLoop since it would run around 2 minutes, but since the repro requires admin privileges (at least on Windows), I am not sure how viable it is. Maybe running the test on Linux would be sufficient.