Scaffold CompositeMLDsa and CompositeMLDsaAlgorithm #116926

PranavSenthilnathan · 2025-06-23T18:55:18Z

This PR adds the public API skeleton for CompositeMLDsa and CompositeMLDsaAlgorithm. The goal of this PR is to get a consensus on src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs so we can start adding implementations.

It also has a rough (and probably buggy) implementation of the API to show its feasibility, but the intent is that all this code is subject to change in future PRs when we actually start thorough implementation and testing. All entry points will eventually throw PNSE to ensure this class can't be used yet, but I'm also fine with removing the extra implementation code if we want to keep the PR scoped to just the API.

Copilot

Pull Request Overview

This PR introduces the initial public API and skeleton implementation for CompositeMLDsa and CompositeMLDsaAlgorithm to support future post-quantum cryptography work. Key changes include adding new API definitions in the reference assembly, providing rough implementation stubs that throw PNSE, and adding associated tests and resource strings.

Reviewed Changes

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

Show a summary per file

File	Description
src/libraries/System.Security.Cryptography/tests/System.Security.Cryptography.Tests.csproj	Added compile entry for CompositeMLDsaFactoryTests
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/AsymmetricAlgorithmHelpers.Der.cs	Removed implementation of DER helper methods; these are now provided from the common shared source
src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj	Added compile entries for new CompositeMLDSA files
src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs	Added public API signatures for CompositeMLDsa and CompositeMLDsaAlgorithm
src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/CompositeMLDsa/CompositeMLDsaFactoryTests.cs	Added tests to verify that CompositeMLDsa APIs are not yet supported
src/libraries/Common/src/System/Security/Cryptography/CompositeMLDsaAlgorithm.cs	Introduced the CompositeMLDsaAlgorithm class with several static instances
src/libraries/Common/src/System/Security/Cryptography/CompositeMLDsaImplementation.cs	Added stub implementations for CompositeMLDsa methods that throw PNSE
src/libraries/Common/src/System/Security/Cryptography/AsymmetricAlgorithmHelpers.Der.cs	Added shared DER helper methods

Comments suppressed due to low confidence (1)

src/libraries/Common/src/System/Security/Cryptography/CompositeMLDsaAlgorithm.cs:63

The algorithm name for MLDsa44WithRSA2048Pkcs15 appears to be incorrect; the string literal should likely be "MLDSA44-RSA2048-PKCS15-SHA256" to match the variable name and intended algorithm.

        public static CompositeMLDsaAlgorithm MLDsa44WithRSA2048Pkcs15 =        new("MLDSA44-RSA2048-PSS-SHA256",

dotnet-policy-service · 2025-06-23T18:55:54Z

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

src/libraries/Common/src/System/Security/Cryptography/AsymmetricAlgorithmHelpers.Der.cs

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

krwq · 2025-06-24T13:35:35Z

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

+    {
+        private static readonly string[] s_knownOids =
+        [
+            Oids.MLDsa44WithRSA2048PssPreHashSha256,


I hate how spec asks us to treat Composite ML-DSA as a single algo but then it treats each variation separately - it feels like this should be a combo of 3 Oids which probably already exist (but then existing specs would need to handle that). Once they add support for couple more sizes of ML-DSA or combination with other PQC this list will grow quickly which doesn't seem like a good idea. They could have at least have the same prefix i.e. a.b.c.mldsacompositeid.rsapssid.sha256id

Yeah I think they want to be interoperable with the existing x509 cert specs. If they have multiple separate oids they'd need to specify where to encode them in the actual cert and ensure that it doesn't break people expecting the old format.

They could have at least have the same prefix i.e. a.b.c.mldsacompositeid.rsapssid.sha256id

The OIDs are actually not finalized. These are the placeholders defined in the current spec but IANA will assign new ones. But.. I expect they won't do anything fancy and will just keep the sequential numbering.

If they have multiple separate oids they'd need to specify where to encode them in the actual cert and ensure that it doesn't break people expecting the old format.

EC-DSA works by saying "I'm EC-DSA" (well, actually "this is an EC Public Key"), then in the public key parameters it has a second OID for what curve the key is on. So it makes it easy to route for a key importer, but you can't look at just the one OID to say whether or not it's supported.

With the 2025 wave of algorithms, they're saying that publicKey.parameters always get terrible support, so let's not use them. Anything that would be in parameters needs to be pre-defined and keyed by OID... so, lots of OIDs.

It's sort of just 6 of one, half-dozen of the other.

But this list starting to get long means we might want to consider changing our plumbing to take this as a HashSet, or SearchValues, or something. Nothing important right now, but just thinking for the future.

krwq · 2025-06-24T13:42:09Z

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

+        ///   The platform does not support Composite ML-DSA. Callers can use the <see cref="IsSupported" /> property
+        ///   to determine if the platform supports Composite ML-DSA.
+        /// </exception>
+        public static CompositeMLDsa GenerateKey(CompositeMLDsaAlgorithm algorithm)


I feel like we should also have:

public static CompositeMLDsa UnsafeConstructKey(MLDsa, RSA, HashAlgorithmName) public static CompositeMLDsa UnsafeConstructKey(MLDsa, ECDsa, HashAlgorithmName)

but apparently spec says to not allow that for some reason (therefore Unsafe prefix proposed)

Just putting the spec comments here for reference:

Compliant parties MUST NOT use, import or export component keys that are used in other contexts, combinations, or by themselves as keys for standalone algorithm use.

Some more context: https://datatracker.ietf.org/doc/html/draft-ietf-lamps-pq-composite-sigs-06#section-10.3

You mentioned offline that hardware keys could benefit from this API so it might be worth considering.

MUST NOT is pretty strong. I still have doubts as this being the right thing to do: think of i.e. TPM or keyvault keys - @bartonjs @vcsjones thoughts?

"Less is more". We can add the ability to bring your own pieces into the managed compositor later, but if we add it we can't take it away.

Bringing your own pieces also potentially carries future FIPS (etc) risk, since if the composite algorithms make it into the CAVP program then the managed compositor will be uncertified.

Double-and: CompositeMLDsa isn't a sealed type, someone could extend it to offer this functionality.

So, in net: Doesn't belong in the base class, at least at this time.

krwq · 2025-06-24T13:44:25Z

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

+            }
+            finally
+            {
+                writer.Reset();


why is the reset needed? (here and other similar places)

For the Clear since the writer buffer is on the heap. We're not super consistent about it elsewhere, but it's easy enough to do in new code. Arguably the encrypted data isn't sensitive but there are a couple of places where we use well known placeholder passwords, so it's effectively as secure as plaintext.

If this is required shouldn't that be part of Dispose or GC? In that case we should use using pattern? If we don't want to enforce that on everyone perhaps at least make syntax less ugly:

using (writer.WithMemoryClear) { DoSomeStuffWithWriter(writer); } public partial class AsnWriter { public MemoryClearScope WithMemoryClear => new MemoryClearScope(this); public struct MemoryClearScope : IDisposable { private readonly AsnWriter _writer; internal MemoryClearScope(AsnWriter writer) => _writer = writer; public void Dispose() => _writer.Clear(); } }

If this is required shouldn't that be part of Dispose or GC?

AsnWriter isn't disposable. And the GC will eventually cause memory to be cleared, but "eventually" isn't good enough for some of our customer('s auditor)s.

This is the pattern that we already use in the other algorithms.

public struct AsnWriter.MemoryClearScope

AsnWriter is a public type, so that'd have to go through API Review. I don't expect it would succeed.

We could do something like that as a non-public wrapper type, but I don't think it should be introduced into this PR (if the PR was already trying it as an experiment, sure, but let's not sidetrack it with this now that it's already up).

krwq · 2025-06-24T13:46:23Z

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

+            int minimumPossiblePkcs8Key = int.MaxValue;
+
+            if (destination.Length < minimumPossiblePkcs8Key)
+            {
+                bytesWritten = 0;
+                return false;
+            }


Do we need the shortcut at all? This seems like non-trivial computation and we shouldn't be optimizing for fail paths

RSAPrivateKey has room for variation, ECPrivateKey has room for variation... the only ones that are predictable are ML-DSA-* (32 bytes) and Ed25519 (32 bytes). So 64 is the minimum.

Trying to avoid the Core dispatch with bad values has virtue, but I think I agree that the pre-filter here isn't doing much, and can probably be cut.

krwq · 2025-06-24T13:47:22Z

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

+            ThrowIfDisposed();
+
+            // TODO Pick a good estimate for the initial size of the buffer.
+            int initalSize = 1;


should this be something along the lines of:

Suggested change

int initalSize = 1;

int initalSize = Algorithm.MaxPublicKeySizeInBytes;

There's no fixed max size so we probably don't want to expose that publicly (RSA exponent can be arbitrarily big). But in practice all of them are bounded so yeah we can have an internal property with an appropriate value.

krwq · 2025-06-24T13:49:50Z

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

+
+                // TODO verify overhead
+
+                // The ASN.1 overhead of a SubjectPublicKeyInfo encoding a public key is ___ bytes.


Suggested change

// The ASN.1 overhead of a SubjectPublicKeyInfo encoding a public key is ___ bytes.

// TODO: The ASN.1 overhead of a SubjectPublicKeyInfo encoding a public key is ___ bytes.

I'm certain it will be easy to miss otherwise

The true overhead is going to change across the algorithms, because of the size of the value. But if you check the size of the SPKI vs the natural public key format for id-MLDSA87-RSA4096-PSS-SHA512, that'll tell you what the upper bound is. (It's the biggest "raw" public key, by far).

And, of course, the overhead may be different for future composites if they use OIDs under a different arc.

krwq · 2025-06-24T13:52:36Z

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

+        /// <value>
+        ///   An ML-DSA algorithm identifier for the ML-DSA-44 and 2048-bit RSASSA-PSS with SHA256 algorithm.
+        /// </value>
+        public static CompositeMLDsaAlgorithm MLDsa44WithRSA2048Pss =           new("MLDSA44-RSA2048-PSS-SHA256",


Should HashAlgorithmName/HashAlgorithmName? be an arg somewhere?

krwq · 2025-06-24T13:52:52Z

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

+        /// <value>
+        ///   An ML-DSA algorithm identifier for the ML-DSA-44 and 2048-bit RSASSA-PSS with SHA256 algorithm.
+        /// </value>
+        public static CompositeMLDsaAlgorithm MLDsa44WithRSA2048Pss =           new("MLDSA44-RSA2048-PSS-SHA256",


nit: the space after = seems a bit weird

Yeah, the alignment seems pretty unstable.

I'd do

public static CompositeMLDsaAlgorithm MLDsa44WithRSA2048Pss = new("MLDSA44-RSA2048-PSS-SHA256", ...);

Or

public static CompositeMLDsaAlgorithm MLDsa44WithRSA2048Pss = new( "MLDSA44-RSA2048-PSS-SHA256", ...);

if you're worried it'll gain more parameters (right now they look like a reasonable single-line length)

I'd also declare them as properties.

If they're actually intended to be fields, they should be readonly, but I think the others are all properties.

krwq · 2025-06-24T13:56:19Z

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

@@ -0,0 +1,46 @@
+// Licensed to the .NET Foundation under one or more agreements.


consider adding NotSupported in the name somewhere

PranavSenthilnathan · 2025-06-25T06:29:08Z

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

+
+            ThrowIfDisposed();


Suggested change

ThrowIfDisposed();

bartonjs · 2025-06-25T21:25:28Z

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

+        /// </remarks>
+        public int SignData(ReadOnlySpan<byte> data, Span<byte> destination, ReadOnlySpan<byte> context = default)
+        {
+            if (destination.Length < Algorithm.MLDsaAlgorithm.SignatureSizeInBytes)


We should consider exposing a minimum size for each algorithm, along with the maximum. For RSA the signature size is fixed, so Min==Max.

For EC-DSA under the X.509 format, the minimum is 6 (which is utterly unachievable, but smaller than 6 is actually impossible), and the maximum is (we have a formula).

So... we could do better for the minimum here without dispatching to the Core method... but maybe it's not important.

bartonjs · 2025-06-25T21:27:38Z

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

+        /// <exception cref="CryptographicException">
+        ///   An error occurred while signing the data.
+        /// </exception>
+        protected abstract int SignDataCore(ReadOnlySpan<byte> data, ReadOnlySpan<byte> context, Span<byte> destination);


Because of the variable length from EC-DSA in the X.509 format, the abstract method probably wants to be protected abstract bool TrySignDataCore(data, context, destination, out int bytesWritten).

Otherwise the Core method needs to throw an ArgumentException that destination was too small.

Similarly, we will want a public TrySignData. Thanks, IETF.

bartonjs · 2025-06-25T21:46:02Z

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

+        ///   Imports a Composite ML-DSA key from an encrypted RFC 7468 PEM-encoded string.
+        /// </summary>
+        /// <param name="source">
+        ///   The PEM text of the encrypted key to import.</param>


Suggested change

/// The PEM text of the encrypted key to import.</param>

/// The PEM text of the encrypted key to import.

/// </param>

bartonjs · 2025-06-25T21:47:03Z

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

+        /// <exception cref="CryptographicException">
+        ///   An error occurred while importing the key.
+        /// </exception>
+        /// <exception cref="PlatformNotSupportedException">


What exception are you intending for "I support composites... but not this composite."?

bartonjs · 2025-06-25T21:50:53Z

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

+
+

Suggested change

bartonjs · 2025-06-25T21:57:30Z

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

+        ///     An error occurred while importing the key.
+        ///   </para>
+        /// </exception>
+        public static CompositeMLDsa ImportPublicKey(CompositeMLDsaAlgorithm algorithm, ReadOnlySpan<byte> source)


I've been trying to avoid methods named "ImportPublicKey" and "ImportPrivateKey", because the SPKI-formatted version of the public key is "a" public key, and similar for PKCS#8.

KEM's keys were called Encapsulation and Decapsulation, so they didn't need to redundantly say "MLKem" in the name. But this should include the word "Composite", if not "CompositeMLDsa".

There's also the debate of what they called the non-public key.

The spec uses sk, which is expanded sometimes to "secret key":

KeyGen() -> (pk, sk): A probabilistic key generation algorithm
which generates a public key pk and a secret key sk. Some
cryptographic modules may also expose a KeyGen(seed) -> (pk, sk),
which generates pk and sk deterministically from a seed. This
specification assumes a seed-based keygen for ML-DSA.

Sign(sk, M) -> s: A signing algorithm which takes as input a
secret key sk and a message M, and outputs a signature s. Signing
routines may take additional parameters such as a context string
or a hash function to use for pre-hashing the message.

But in other places they call "private key"

3. Output the composite public and private keys pk = SerializePublicKey(mldsaPK, tradPK) sk = SerializePrivateKey(mldsaSeed, tradSK) return (pk, sk)

bartonjs · 2025-06-25T21:58:08Z

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

+        }
+
+        /// <summary>
+        ///   Imports a Composite ML-DSA public key from its private seed value.


Composites don't, to my recollection, have seeds.

And, also, this is the public key import 😄

bartonjs · 2025-06-25T22:50:13Z

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

+        }
+
+        /// <summary>
+        ///   Exports the current key in the FIPS 205 secret key format.


Not quite :)

Scaffold CompositeMLDsa and CompositeMLDsaAlgorithm

18d4c5d

PranavSenthilnathan added this to the 10.0.0 milestone Jun 23, 2025

PranavSenthilnathan requested review from vcsjones, krwq and bartonjs June 23, 2025 18:55

PranavSenthilnathan self-assigned this Jun 23, 2025

Copilot AI review requested due to automatic review settings June 23, 2025 18:55

PranavSenthilnathan added the area-System.Security label Jun 23, 2025

Copilot AI reviewed Jun 23, 2025

View reviewed changes

Fix algorithm id

6755011

krwq reviewed Jun 24, 2025

View reviewed changes

src/libraries/Common/src/System/Security/Cryptography/AsymmetricAlgorithmHelpers.Der.cs Show resolved Hide resolved

krwq reviewed Jun 24, 2025

View reviewed changes

src/libraries/Common/src/System/Security/Cryptography/CompositeMLDsa.cs Show resolved Hide resolved

krwq reviewed Jun 24, 2025

View reviewed changes

PranavSenthilnathan commented Jun 25, 2025

View reviewed changes

bartonjs reviewed Jun 25, 2025

View reviewed changes

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

Comment on lines +866 to +867

Copy link

Member

bartonjs Jun 25, 2025

Choose a reason for hiding this comment

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

Suggested change

bartonjs reviewed Jun 25, 2025

View reviewed changes

	int initalSize = 1;
	int initalSize = Algorithm.MaxPublicKeySizeInBytes;


		// TODO verify overhead

		// The ASN.1 overhead of a SubjectPublicKeyInfo encoding a public key is ___ bytes.

	// The ASN.1 overhead of a SubjectPublicKeyInfo encoding a public key is ___ bytes.
	// TODO: The ASN.1 overhead of a SubjectPublicKeyInfo encoding a public key is ___ bytes.

		@@ -0,0 +1,46 @@
		// Licensed to the .NET Foundation under one or more agreements.

	/// The PEM text of the encrypted key to import.</param>
	/// The PEM text of the encrypted key to import.
	/// </param>

Scaffold CompositeMLDsa and CompositeMLDsaAlgorithm #116926

Are you sure you want to change the base?

Scaffold CompositeMLDsa and CompositeMLDsaAlgorithm #116926

Conversation

PranavSenthilnathan commented Jun 23, 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 Jun 23, 2025

Uh oh!

Uh oh!

Uh oh!

krwq Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

krwq Jun 24, 2025 •

edited

Loading

krwq Jun 24, 2025 •

edited

Loading