Cleanup for cert accessors#118116
Merged
PranavSenthilnathan merged 2 commits intoJul 28, 2025
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR performs cleanup for certificate accessor methods following a previous change. The main purpose is to optimize memory usage by replacing byte array allocations with pooled memory for public key comparisons, standardize platform checks, and add proper argument validation with consistent documentation.
Key changes:
- Replace byte array allocations with
CryptoPoolLeasefor improved memory management in key comparison operations - Consolidate platform detection logic into a centralized
Helpers.IsOSPlatformWindowsproperty - Add comprehensive argument validation and XML documentation to public API methods
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| X509Certificate2.cs | Replace byte array allocations with pooled memory for ML-KEM, ML-DSA, and SLH-DSA key comparisons |
| CertificatePal.Windows.PrivateKey.cs | Remove extra blank line for code formatting consistency |
| PrivateKeyAssociationTests.cs | Add comprehensive argument validation tests for post-quantum cryptography methods |
| X509CertificateKeyAccessors.cs | Add argument validation, XML documentation, and replace allocations with pooled memory |
| MLKemImplementation.Windows.cs | Consolidate platform checks using centralized helper and remove unused imports |
| MLKemCng.cs | Consolidate platform checks using centralized helper and remove unused imports |
| MLDsaImplementation.Windows.cs | Consolidate platform checks using centralized helper and remove unused imports |
| MLDsaCng.cs | Consolidate platform checks using centralized helper and remove unused imports |
| Helpers.cs | Add centralized IsOSPlatformWindows property for consistent platform detection |
| CompositeMLDsaImplementation.Windows.cs | Consolidate platform checks using centralized helper and remove unused imports |
| Interop.Properties.cs | Update platform check to use centralized helper and remove unused imports |
PranavSenthilnathan
force-pushed
the
crypto-cleanup
branch
from
4b9abef to
a21c4b2
Compare
bartonjs
reviewed
Jul 28, 2025
bartonjs
approved these changes
Jul 28, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #117907
Fixes #118066