Skip to content

Conversation

@bartonjs
Copy link
Member

@bartonjs bartonjs commented Apr 7, 2025

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

ghost commented Apr 7, 2025

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

ghost commented Apr 7, 2025

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.

Copy link
Contributor

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.

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

bartonjs commented Apr 8, 2025

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

bartonjs commented Apr 8, 2025

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.

3 participants