-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
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
|
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered it, but I couldn't think of other uses elsewhere. We can always move it later when we need it.
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
…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. |
Fixes #43879
This PR adds timestamp-based invalidation of
SafeFreeCredentials
inSslSessionCache
. The expiration timestamp is calculated based onNotAfter
fields 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.