-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add an in-memory cache for CRLs on Linux #123562
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
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
|
Testing with a cert that has a large CRL, revocation-enabled chain building is significantly reduced. This is from building a chain 5 times in a row, the last build: Before: After: |
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.
Pull request overview
This PR introduces an in-memory LRU cache layer for Certificate Revocation Lists (CRLs) on Linux to improve performance by reducing disk I/O. The cache sits between the caller and the existing disk cache, with a fixed capacity of 30 entries that evicts least-recently-used items when full. The implementation uses a GC-based pruning mechanism similar to ArrayPool, where a finalizable sentinel object triggers periodic cache cleanup to manage memory pressure.
Changes:
- Added 5 new event source events for tracking in-memory cache hits, misses, expiration, pruning, and capacity events
- Updated existing event messages to clarify they refer to disk cache operations
- Implemented MruCrlCache class with thread-safe LRU eviction, reference counting via DangerousAddRef/DangerousRelease, and GC-triggered pruning
- Refactored CRL loading to check in-memory cache first, then disk cache, then download
- Added proper SafeHandle management to prevent premature finalization during cache operations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| OpenSslX509ChainEventSource.cs | Added 5 new event IDs and methods for in-memory cache telemetry; updated existing event messages to distinguish disk cache operations |
| OpenSslCrlCache.cs | Added MruCrlCache class implementing thread-safe LRU cache with GC-based pruning; refactored CRL loading logic to check memory cache before disk; added proper SafeHandle reference management |
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslCrlCache.cs
Show resolved
Hide resolved
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslCrlCache.cs
Show resolved
Hide resolved
...ryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainEventSource.cs
Outdated
Show resolved
Hide resolved
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslCrlCache.cs
Outdated
Show resolved
Hide resolved
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslCrlCache.cs
Outdated
Show resolved
Hide resolved
...ryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainEventSource.cs
Outdated
Show resolved
Hide resolved
...ryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainEventSource.cs
Outdated
Show resolved
Hide resolved
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslCrlCache.cs
Outdated
Show resolved
Hide resolved
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslCrlCache.cs
Outdated
Show resolved
Hide resolved
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslCrlCache.cs
Show resolved
Hide resolved
Might be simpler to check directly with |
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslCrlCache.cs
Show resolved
Hide resolved
...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslCrlCache.cs
Show resolved
Hide resolved
|
|
||
| using (SafeBioHandle bio = Interop.Crypto.BioNewFile(crlFile, "wb")) | ||
| { | ||
| if (bio.IsInvalid || Interop.Crypto.PemWriteBioX509Crl(bio, crl) == 0) |
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.
In general what is to stop concurrent writes on these CRL files? I don't believe the file mode here will have any kind of exclusivity on the file. Can two processes race and corrupt each other's CRL cache?
Should we instead do some kind of atomic writing pattern?
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.
Yeah, they can stomp, which just causes the next load to fail and and fall back to fetching from the web again. We can certainly improve the resilience, but that feels a bit unrelated (again, mostly because I expect this to get a backport request)
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.
Yeah I understand, I just wonder now that we see the ecosystem shifting to relying on CRLs now if this is going to be a "problem" that is hit more frequently. If it's corrupt, is it self-correcting? (Does the web fetch overwrite the bad one at least?)
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.
@manuelpuyol this is the PR I was mentioning.
|
|
||
| private void PruneForGC() | ||
| { | ||
| if (DateTime.Now < _firstPurge) |
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.
Should we using Stopwatch.GetTimestamp or something monotonic? What happens when the system moves its clock around for DST changes?
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.
That's a really good point. One I hope I would have made had I not written the code.
I'll probably take the suggestion of switching to GC.GetGeneration to just filter out G0/G1 finalizations of this detector.
Introduce an extra layer of caching for CRLs.
This implementation ignores any finalizations that happen in the first minute of the sentinel object's life, to allow the object a chance to get into Gen2 before hyper-eagerly evicting things.