ML-KEM: ImportFrom{Encrypted}Pem by vcsjones · Pull Request #114155 · dotnet/runtime

vcsjones · 2025-04-02T14:43:26Z

This adds ImportFromPem and ImportFromEncryptedPem to MLKem.

Contributes to #113508

ghost · 2025-04-02T14:43:32Z

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.

ghost · 2025-04-02T14:43:34Z

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.

Copilot

Pull Request Overview

This pull request adds ImportFromPem functionality to MLKem, expanding the API to enable importing keys from RFC 7468 PEM-encoded strings.

Updates PemKeyHelpers to be a partial class to enable splitting of functionality
Adds ImportFromPem overloads in both the reference assembly and core implementation
Introduces a suite of tests to validate proper PEM import behavior and exception handling

Reviewed Changes

Copilot reviewed 8 out of 13 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/PemKeyHelpers.cs	Changed to partial class to support new PEM import functionality
src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs	Added ImportFromPem overloads in the reference API
src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/PemEncoding.cs	Added a downlevel PEM encoding implementation
src/libraries/Common/tests/System/Security/Cryptography/MLKemTests.cs	Extended tests covering various ImportFromPem scenarios including ambiguous inputs and roundtrip validations
src/libraries/Common/tests/System/Security/Cryptography/MLKemNotSupportedTests.cs	Added tests to ensure proper exception throwing in unsupported scenarios
src/libraries/Common/src/System/Security/Cryptography/PemKeyHelpers.Factory.cs	Introduced a supporting factory method for PEM import processing
src/libraries/Common/src/System/Security/Cryptography/MLKem.cs	Implemented ImportFromPem methods wrapping the new factory and PEM import logic

Files not reviewed (5)

eng/Versions.props: Language not supported
src/libraries/Microsoft.Bcl.Cryptography/src/Microsoft.Bcl.Cryptography.csproj: Language not supported
src/libraries/Microsoft.Bcl.Cryptography/src/Resources/Strings.resx: Language not supported
src/libraries/Microsoft.Bcl.Cryptography/tests/Microsoft.Bcl.Cryptography.Tests.csproj: Language not supported
src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj: Language not supported

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

dotnet-policy-service · 2025-04-02T14:44:02Z

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

vcsjones · 2025-04-02T14:44:55Z

src/libraries/Common/src/System/Security/Cryptography/MLKem.cs

+        /// <exception cref="ArgumentNullException">
+        ///   <paramref name="source" /> is <see langword="null" />
+        /// </exception>
+        public static MLKem ImportFromPem(string source)


This was not in the initial API design, and we don't have it on other types like RSA. However, I think it is useful for .NET Framework. In .NET Framework, there is no implicit conversion from string to ReadOnlySpan<char>. So even C# developers will be forced to use the AsSpan extension method, unless we give them a nice overload to use.

vcsjones · 2025-04-02T14:46:17Z

src/libraries/Common/src/System/Security/Cryptography/PemKeyHelpers.Factory.cs

+        internal delegate TAlg ImportFactoryKeyAction<TAlg>(ReadOnlySpan<byte> source);
+        internal delegate ImportFactoryKeyAction<TAlg>? FindImportFactoryActionFunc<TAlg>(ReadOnlySpan<char> label);
+
+        internal static TAlg ImportFactoryPem<TAlg>(ReadOnlySpan<char> source, FindImportFactoryActionFunc<TAlg> callback) where TAlg : class


This is largely based on the already existing ImportPem from PemKeyHelpers. We need a new import method because the new PQC types have a different API shape than the ones on AsymmetricAlgorithm (they are static, and they don't out the number of bytes read).

I tried de-duping the logic between the two but it got real messy because the delegates are different types and we need type parameters in this one.

vcsjones · 2025-04-02T14:48:07Z

src/libraries/Microsoft.Bcl.Cryptography/src/System/Security/Cryptography/PemEncoding.cs

+{
+    // Downlevel implementation of PemEncoding. This was originally taken from the .NET 5 implementation of PemEncoding
+    // https://github.com/dotnet/runtime/blob/4aadfea70082ae23e6c54a449268341e9429434e/src/libraries/System.Security.Cryptography.Encoding/src/System/Security/Cryptography/PemEncoding.cs
+    internal static class PemEncoding


This is basically a direct clone of the .NET 5 implementation from PemEncoding, with the exception of being internal the XML doc comments are stripped out, and not having the Write methods (yet).

vcsjones · 2025-04-02T14:49:15Z

src/libraries/Microsoft.Bcl.Cryptography/tests/PemEncodingFindTests.cs

+    // Tests for Downlevel implementation of PemEncoding. This was originally taken from the .NET 5 implementation of
+    // PemEncodingFindTests.
+    // https://github.com/dotnet/runtime/blob/4aadfea70082ae23e6c54a449268341e9429434e/src/libraries/System.Security.Cryptography.Encoding/tests/PemEncodingFindTests.cs
+    public abstract class PemEncodingFindTests


Likewise this is a copy from the .NET 5 unit tests, with some changes for visibility purposes.

vcsjones · 2025-04-02T14:50:56Z

src/libraries/Microsoft.Bcl.Cryptography/tests/Microsoft.Bcl.Cryptography.Tests.csproj

  <ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
    <Compile Include="$(CoreLibSharedDir)System\Runtime\CompilerServices\IsExternalInit.cs"
             Link="Common\System\Runtime\CompilerServices\IsExternalInit.cs" />
+    <Compile Include="$(LibrariesProjectRoot)\Microsoft.Bcl.Cryptography\src\System\Security\Cryptography\PemEncoding.cs"


Since our downlevel PemEncoding implementation is internal we can only test it by including it in the test project, and I don't want it untested. And we can only test in in the .NET Framework test target, since the netcoreapp ones will have the in-box one already.

src/libraries/Microsoft.Bcl.Cryptography/src/Microsoft.Bcl.Cryptography.csproj

src/libraries/Common/tests/System/Security/Cryptography/MLKemTests.cs

bartonjs

LGTM, aside from the fact that all builds are on the floor.

vcsjones · 2025-04-03T22:14:56Z

Looks like build was fixed in #114233.

vcsjones · 2025-04-04T01:27:17Z

@vcsjones-bot test f7a9f18 with openssl-3.5

vcsjones added 5 commits April 2, 2025 09:29

Checkpoint before I try something dumb

3cf7d37

Yep it was dumb keep doing not-dumb things

abd7cdb

Implement more PEM loading tests

dcc21e4

Add string ImportFromPem

535afd2

Fix dup test

97f0961

vcsjones added the area-System.Security label Apr 2, 2025

vcsjones added this to the 10.0.0 milestone Apr 2, 2025

vcsjones requested a review from bartonjs April 2, 2025 14:43

vcsjones self-assigned this Apr 2, 2025

Copilot AI review requested due to automatic review settings April 2, 2025 14:43

ghost added the new-api-needs-documentation label Apr 2, 2025

Copilot AI reviewed Apr 2, 2025

View reviewed changes

vcsjones commented Apr 2, 2025

View reviewed changes

src/libraries/Microsoft.Bcl.Cryptography/src/Microsoft.Bcl.Cryptography.csproj Outdated Show resolved Hide resolved

vcsjones added 3 commits April 2, 2025 11:49

Don't require a new dependency

031379c

Fix some indentation while we are here

a97245a

Add ImportFromEncryptedPem to MLKem

cc79a5b

vcsjones changed the title ~~ML-KEM: ImportFromPem~~ ML-KEM: ImportFrom{Encrypted}Pem Apr 2, 2025

vcsjones added 2 commits April 2, 2025 18:59

Fix .NET Framework build

7e67dab

Merge branch 'main' into ml-kem-import-pem

b5a801a

kg mentioned this pull request Apr 3, 2025

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts #114241

Closed

bartonjs reviewed Apr 3, 2025

View reviewed changes

src/libraries/Common/tests/System/Security/Cryptography/MLKemTests.cs Outdated Show resolved Hide resolved

bartonjs reviewed Apr 3, 2025

View reviewed changes

src/libraries/Common/tests/System/Security/Cryptography/MLKemTests.cs Outdated Show resolved Hide resolved

Code review feedback

d45b3cb

bartonjs approved these changes Apr 3, 2025

View reviewed changes

This was referenced Apr 3, 2025

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Closed

CI flakiness: Mac builds failing with SDK errors #114235

Closed

Merge branch 'main' into ml-kem-import-pem

f7a9f18

vcsjones merged commit 6dc03c2 into dotnet:main Apr 4, 2025
86 of 88 checks passed

vcsjones deleted the ml-kem-import-pem branch April 4, 2025 03:30

github-actions bot locked and limited conversation to collaborators May 4, 2025

Conversation

vcsjones commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 2, 2025

Uh oh!

ghost commented Apr 2, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

dotnet-policy-service bot commented Apr 2, 2025

Uh oh!

vcsjones Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

vcsjones Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

vcsjones Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

vcsjones Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

vcsjones Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

vcsjones Apr 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bartonjs left a comment

Choose a reason for hiding this comment

Uh oh!

vcsjones commented Apr 3, 2025

Uh oh!

vcsjones commented Apr 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vcsjones commented Apr 2, 2025 •

edited

Loading