BCrypt Composite ML-DSA#129612
Conversation
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
There was a problem hiding this comment.
Pull request overview
This PR wires up Windows support for Composite ML-DSA via the BCrypt “single key”/PQDSA blob mechanism (including new parameter-set identifiers and blob magic values), and updates platform-support expectations in tests.
Changes:
- Add Composite ML-DSA PQDSA blob encode/decode support and parameter-set mapping for Windows composite algorithms.
- Implement Windows
CompositeMLDsaoperations (keygen/import/export/sign/verify) using BCrypt Composite-ML-DSA provider and PQDSA blobs. - Update Windows algorithm identifiers / magic numbers and adjust support-detection tests accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj | Removes managed Composite ML-DSA sources from the Windows build item group. |
| src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/CompositeMLDsa/CompositeMLDsaFactoryTests.cs | Updates platform support expectations for IsAlgorithmSupported on Windows. |
| src/libraries/Common/src/System/Security/Cryptography/PqcBlobHelpers.cs | Adds composite parameter-set strings plus composite PQDSA blob encode/decode helpers; adjusts stackalloc threshold. |
| src/libraries/Common/src/System/Security/Cryptography/CompositeMLDsaImplementation.Windows.cs | Replaces Windows PNSE stub with a BCrypt-backed implementation for composite keygen/import/export/sign/verify. |
| src/libraries/Common/src/System/Security/Cryptography/CompositeMLDsa.cs | Adds an algorithm support check during PKCS#8 private-key import. |
| src/libraries/Common/src/Interop/Windows/BCrypt/Interop.Blobs.cs | Adds composite ML-DSA public/private magic numbers for PQDSA blobs. |
| src/libraries/Common/src/Interop/Windows/BCrypt/Interop.BCryptSignHash.cs | Changes PQC “pure” signing helper to return bytesWritten and relaxes debug assertion. |
| src/libraries/Common/src/Interop/Windows/BCrypt/Cng.cs | Adds BCrypt algorithm name constant for Composite-ML-DSA. |
fadf2c4 to
b94a7dc
Compare
b94a7dc to
f7063c5
Compare
| int written = Interop.BCrypt.BCryptSignHashPqcPure(_key, data, context, destination); | ||
| Debug.Assert(written == destination.Length); |
| if (!CompositeMLDsa.IsSupported || | ||
| CompositeMLDsa.IsAlgorithmSupported(CompositeMLDsaAlgorithm.MLDsa87WithEd448)) | ||
| throw new SkipTestException("Algorithm is supported on this platform."); |
There was a problem hiding this comment.
This looks odd. Consider using braces. I generally don't recommend we use braceless ifs if the if condition itself spans multiple lines. (Repeat feedback to other places)
| internal const string BCRYPT_MLKEM_PARAMETER_SET_768 = "768"; | ||
| internal const string BCRYPT_MLKEM_PARAMETER_SET_1024 = "1024"; | ||
|
|
||
| internal const string BCRYPT_COMPOSITE_MLDSA_PARAMETER_SET_44_ECDSA_P256_SHA256 = "44-ECDSA-P256-SHA256"; |
There was a problem hiding this comment.
This basically hardcodes our list of supported composite algorithms. I think that's fine, but just noting that if Windows gets around to adding other things like RSA then we will have to do work to enable them.
| return ExportKey( | ||
| Interop.BCrypt.KeyBlobType.BCRYPT_PQDSA_PRIVATE_BLOB, | ||
| destination); |
There was a problem hiding this comment.
This doesn't have to be wrapped and can be a single line.
| throw new PlatformNotSupportedException(); | ||
| } | ||
|
|
||
| internal static bool TryGetCompositeMLDsaParameterSet(CompositeMLDsaAlgorithm algorithm, [NotNullWhen(true)] out string? parameterSet) |
There was a problem hiding this comment.
This method signature should wrap.
| throw new PlatformNotSupportedException(); | ||
| } | ||
|
|
||
| internal static bool TryGetCompositeMLDsaParameterSet(CompositeMLDsaAlgorithm algorithm, [NotNullWhen(true)] out string? parameterSet) |
There was a problem hiding this comment.
Should the parameterSet just be a nullable property on CompositeMLDsaAlgorithm like MaxPublicKeySizeInBytes? That way this doesn't really even need to exist.
CompositeMLDsaAlgorithm alg = GetTheAlg();
if (alg.CngParameterSetName is null)
{
throw new NotSupportedException();
}
string parameterSet = alg.CngParameterSetName;
Windows Insider builds support Composite ML-DSA in BCrypt. Documentation of public API can be found here.
Note this changes the default implementation on Windows from managed to BCrypt. On versions that support Composite ML-DSA, this reduces the number of supported algorithms to the 4 listed in the docs above. And for Windows versions that don't support Composite ML-DSA, our default implementation also doesn't support it now.
Fixes #116999