Skip to content

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

Merged
merged 3 commits into from
Jun 24, 2025
Merged

ML-DSA + COSE improve tests #116543

merged 3 commits into from
Jun 24, 2025

Conversation

krwq
Copy link
Member

@krwq krwq commented Jun 11, 2025

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.

@Copilot Copilot AI review requested due to automatic review settings June 11, 2025 16:30
@krwq krwq requested a review from bartonjs June 11, 2025 16:31
Copy link
Contributor

@Copilot Copilot AI left a 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.

Copy link
Contributor

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

Comment on lines +504 to +515
[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;
Copy link
Member

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.

Copy link
Member Author

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;
Copy link
Member

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).

Suggested change
default: return true;
default:
return true;

@krwq
Copy link
Member Author

krwq commented Jun 24, 2025

Failures seem unrelated

@krwq krwq merged commit 491d96e into dotnet:main Jun 24, 2025
80 of 86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants