Skip to content

ML-KEM: X509Certificate2.GetMLKemPublicKey - #114657

Merged
vcsjones merged 5 commits into
dotnet:mainfrom
vcsjones:ml-kem-x509-keys
Apr 15, 2025
Merged

ML-KEM: X509Certificate2.GetMLKemPublicKey#114657
vcsjones merged 5 commits into
dotnet:mainfrom
vcsjones:ml-kem-x509-keys

Conversation

@vcsjones

Copy link
Copy Markdown
Member

This provides an in-box implementation of X509Certificate2.GetMLKemPublicKey().

Originally, this change included the X509CertificateKeyAccessors for Microsoft.Bcl.Cryptography. However, we cannot meaningfully test this until there is a Windows / .NET Framework implementation of MLKem. The unit tests run for either NetCoreAppCurrent, which is .NET 10 and the implementation of X509CertificateKeyAccessors will be "Just call the one on X509Certificate2". The other target for the unit tests is .NET Framework, which would use the downlevel implementation, but we lack a function MLKem on Windows at the moment.

Instead of including a big chunk of untestable code, I decided to hold off on that part, for now.

CopilotAI review requested due to automatic review settings April 14, 2025 21:21
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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-box implementation of X509Certificate2.GetMLKemPublicKey(), exposing the new ML-KEM API and adding the corresponding reflection metadata and unit tests.

  • Implements GetMLKemPublicKey with proper XML documentation and experimental attributes.
  • Updates the reference assembly to expose the new API.
  • Adds unit tests and test data for various ML-KEM certificate scenarios.

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

FileDescription
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/X509Certificate2.csAdds the new GetMLKemPublicKey() implementation with experimental support.
src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.csUpdates the public contract to include GetMLKemPublicKey().
src/libraries/Common/tests/System/Security/Cryptography/X509Certificates/MLKemCertTests.csProvides tests for the new ML-KEM API under different conditions.
src/libraries/Common/tests/System/Security/Cryptography/MLKemTestData.csAdds test data including certificate PEM strings for ML-KEM certificates.
Files not reviewed (1)
  • src/libraries/System.Security.Cryptography/tests/System.Security.Cryptography.Tests.csproj: Language not supported

@vcsjonesvcsjones added this to the 10.0.0 milestone Apr 14, 2025
@vcsjones
vcsjones requested a review from bartonjsApril 14, 2025 21:29

@bartonjsbartonjs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question about a test, otherwise LGTM

@vcsjones
vcsjones merged commit 99a0bd2 into dotnet:mainApr 15, 2025
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 15, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@vcsjones@bartonjs