Skip to content

Make CertificateRequest et al work with ML-DSA - #114357

Closed
bartonjs wants to merge 2 commits into
dotnet:mainfrom
bartonjs:mldsa_certreq
Closed

Make CertificateRequest et al work with ML-DSA#114357
bartonjs wants to merge 2 commits into
dotnet:mainfrom
bartonjs:mldsa_certreq

Conversation

@bartonjs

@bartonjs bartonjs commented Apr 7, 2025

Copy link
Copy Markdown
Member

A few things happen in this PR:

  • CRL Builder Tests now run for every supported signing algorithm
  • CopyWithPrivateKey was under-tested for all algorithms (particularly around the exception model), that has been rectified
  • Our test CertificateAuthority class gained algorithm agility
    • On my system where MLDsa.IsSupported reports true, I changed the default key algorithm from RSASSA-2048+SHA-2-256 to ML-DSA-65, and everything passed.

Those pieces could be broken out into a pure test enhancement change for cleanliness.

The impetus of the change, however, was to add first-class support for ML-DSA to CertificateRequest.

  • Add ctors to CertificateRequest
  • Enlighten CertificateRequest that future signing algorithms might not require a HashAlgorithmName
  • Add support to CertificateRequestListBuilder
  • Add cert.GetMLDsaPublicKey/GetMLDsaPrivateKey/CopyWithPrivateKey to power the above.

Contributes to #113502

@bartonjs bartonjs added this to the 10.0.0 milestone Apr 7, 2025
@bartonjs bartonjs self-assigned this Apr 7, 2025
Copilot AI review requested due to automatic review settings April 7, 2025 22:51
@ghost

ghost commented Apr 7, 2025

Copy link
Copy Markdown

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.

1 similar comment
@ghost

ghost commented Apr 7, 2025

Copy link
Copy Markdown

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 26 out of 29 changed files in this pull request and generated 2 comments.

Files not reviewed (3)
  • src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.csproj: Language not supported
  • src/libraries/System.Security.Cryptography/src/Resources/Strings.resx: Language not supported
  • src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj: Language not supported

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@bartonjs

bartonjs commented Apr 8, 2025

Copy link
Copy Markdown
Member Author

Looks like I forgot to try to build System.Net.Security.Tests, which also includes CertificateAuthority.cs


private SafeX509Handle _cert;
private SafeEvpPKeyHandle? _privateKey;
private MLDsa? _mldsaPrivateKey;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should this be added to Dispose like _privateKey?


[Experimental(Experimentals.PostQuantumCryptographyDiagId)]
[UnsupportedOSPlatform("browser")]
public MLDsa? GetMLDsaPublicKey()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

XML docs?

}

[Experimental(Experimentals.PostQuantumCryptographyDiagId)]
public static X509SignatureGenerator CreateForMLDsa(MLDsa key)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Docs would be good.

@bartonjs

bartonjs commented Apr 8, 2025

Copy link
Copy Markdown
Member Author

The longer the PR has been up, the more dirty I feel about a huge passel of test/infrastructure changes and the "enable ML-DSA" part. Partially because the "enable ML-DSA" part is supposed to serve as a template for SLH-DSA and CompositeMLDsa... so I think I'm going to break this into two pieces, even though that's more work.

@github-actions github-actions Bot locked and limited conversation to collaborators May 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants