-
Notifications
You must be signed in to change notification settings - Fork 5.1k
ML-DSA + COSE improve tests #116543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ML-DSA + COSE improve tests #116543
Conversation
There was a problem hiding this comment.
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 ML‑DSA support into existing COSE tests by updating helper methods, test files, and the project file to include ML‑DSA test data. Key changes include:
- Adding ML‑DSA test data files and a new define constant in the project file.
- Changing method signatures and generic constraints from AsymmetricAlgorithm to IDisposable to accommodate ML‑DSA.
- Updating helper methods (e.g. GetExpectedAlgorithmStringValue) and key conversion logic to support the new ML‑DSA algorithms.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
System.Security.Cryptography.Cose.Tests.csproj | Added compilation for ML‑DSA test data and defined constant for PEM encoding exclusion. |
CoseTestHelpers.cs | Introduced ML‑DSA-specific algorithm string conversion and updated key conversion logic and parameter types. |
CoseSign1MessageTests.Sign.cs, CoseMultiSignMessageTests.Sign.cs, and related test files | Updated generic constraints and method signatures to use IDisposable instead of AsymmetricAlgorithm. |
MLDsaTestsData.cs and MLDsaTestsData.Ietf.cs | Updated with required modifiers and conditional PEM encoding compilation. |
src/libraries/System.Security.Cryptography.Cose/tests/CoseTestHelpers.cs
Show resolved
Hide resolved
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
...mon/tests/System/Security/Cryptography/AlgorithmImplementations/MLDsa/MLDsaTestsData.Ietf.cs
Outdated
Show resolved
Hide resolved
...s/Common/tests/System/Security/Cryptography/AlgorithmImplementations/MLDsa/MLDsaTestsData.cs
Outdated
Show resolved
Hide resolved
...m.Security.Cryptography.Cose/tests/CoseMultiSignMessageTests.Sign.CustomHeaderMaps.Stream.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseTestHelpers.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography.Cose/tests/CoseTestHelpers.cs
Outdated
Show resolved
Hide resolved
...aries/System.Security.Cryptography.Cose/tests/System.Security.Cryptography.Cose.Tests.csproj
Outdated
Show resolved
Hide resolved
[ThreadStatic] | ||
private static MLDsa? t_mldsa44Key; | ||
[ThreadStatic] | ||
private static MLDsa? t_mldsa44KeyWithoutPrivateKey; | ||
[ThreadStatic] | ||
private static MLDsa? t_mldsa65Key; | ||
[ThreadStatic] | ||
private static MLDsa? t_mldsa65KeyWithoutPrivateKey; | ||
[ThreadStatic] | ||
private static MLDsa? t_mldsa87Key; | ||
[ThreadStatic] | ||
private static MLDsa? t_mldsa87KeyWithoutPrivateKey; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider, as a followup, introducing a private nested class to hold all of these ThreadStatic values, e.g.
[ThreadStatic]
private static KeyRing t_threadKeys;
private static KeyRing ThreadKeys => (t_threadKeys ??= new KeyRing());
internal static MLDsa MLDsa44Key => ThreadKeys.MLDsa44Key;
...
private sealed class KeyRing
{
internal MLDsa MLDsa44 { get => field ??= CreateMLDsa(MLDsaAlgorithm.MLDsa44, true); private set; }
...
}
It's not that what's here is bad, it's just that it's a lot of individual ThreadStatic values, and over time it just feels like it's increasing the risk of missing one and having parallelism where we don't expect it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add this and the other change in the next PR
case CoseAlgorithm.MLDsa65: | ||
case CoseAlgorithm.MLDsa87: | ||
return MLDsa.IsSupported; | ||
default: return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit (not worth respinning for, but if there's any other edit, please fix).
default: return true; | |
default: | |
return true; |
Failures seem unrelated |
This includes ML-DSA into existing COSE tests (part of the feedback from #115158)
This is not 100% done yet as some of the new tests might be possible to be removed now but I'm not yet sure if we're actually testing all new CoseKey overloads.