-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Move MLDsa Experimental to appropriate members #119289
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
base: main
Are you sure you want to change the base?
Conversation
Not experimental: * MLDsaAlgorithm (whole type) * MLDsa (class name) * MLDsa.GenerateKey * MLDsa.ImportMLDsaPublicKey * MLDsa.ImportMLDsaPrivateKey * MLDsa.ImportMLDsaPrivateSeed * MLDsa.ExportMLDsaPublicKey * MLDsa.ExportMLDsaPrivateKey * MLDsa.ExportMLDsaPrivateSeed * MLDsa.SignData * MLDsa.VerifyData * Any *Core method for the above Experimental: * IETF Formats * MLDsa.ImportFromPem * MLDsa.ImportFromEncryptedPem * MLDsa.ImportPkcs8PrivateKey * MLDsa.ImportEncryptedPkcs8PrivateKey * MLDsa.ImportSubjectPublicKeyInfo * MLDsa.ExportPkcs8PrivateKey * MLDsa.ExportPkcs8PrivateKeyPem * MLDsa.ExportEncryptedPkcs8PrivateKey * MLDsa.ExportEncryptedPkcs8PrivateKeyPem * MLDsa.ExportSubjectPublicKeyInfo * MLDsa.ExportSubjectPublicKeyInfoPem * And any *Core methods powering them * Unsure we have the right shape * MLDsa.SignPreHash * MLDsa.VerifyPreHash * MLDsa.SignMu * MLDsa.VerifyMu * And any *Core methods powering them
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 removes the experimental attribute from the base MLDsa class and MLDsaAlgorithm type while adding the experimental attribute to specific members that are still considered experimental. The primary purpose is to graduate core ML-DSA functionality from experimental status while keeping IETF format methods and certain signing methods as experimental.
- Removes
[Experimental]
attribute from the baseMLDsa
class andMLDsaAlgorithm
type - Adds
[Experimental]
attribute to specific IETF format import/export methods - Adds
[Experimental]
attribute to PreHash and Mu signing/verification methods
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs | Updates reference assembly to move experimental attributes from class level to specific IETF and PreHash/Mu methods |
src/libraries/Common/src/System/Security/Cryptography/MLDsaAlgorithm.cs | Removes experimental attribute from the MLDsaAlgorithm class |
src/libraries/Common/src/System/Security/Cryptography/MLDsa.cs | Removes experimental attribute from MLDsa class and adds it to specific IETF format and PreHash/Mu methods |
src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs
Show resolved
Hide resolved
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs
Show resolved
Hide resolved
I sanity checked this by adding a quick and dirty test to a library that doesn't already have global suppressions enabled: private static byte[] Sign(byte[] key, byte[] message)
{
using (MLDsa mldsa = MLDsa.ImportMLDsaPrivateSeed(MLDsaAlgorithm.MLDsa44, key))
{
return mldsa.SignData(message);
}
} No warnings. - return mldsa.SignData(message);
+ return mldsa.SignMu(message); And SYSLIB5006 was correctly emitted. |
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.
What about the derived types? MLDsaCng
and MLDsaOpenSsl
? When we did MLKem we also removed it from the whole class on the derived types and some of the "helper" stuff like CngAlgorithm and CngAlgorithmGroup.
Here's the MLKem PR for reference. #117831 |
Whoops.
CngAlgorithm and CngAlgorithmGroup I'm pretty solid on. The CngBlobFormats I also did, but I feel less good about... if I were Windows I'd be regretting the "PQ" name and trying to replace it before full GA... but I don't expect them to, so I went ahead and removed it. If anyone wants to be hesitant on the PQ blobs, I'm happy to put it back. They're not really important for mainline usage. |
I paused and thought about it. But yeah I think I am least concerned about these. |
@bartonjs why does this need API review? |
It changes the ref.cs file. And no one is answering email 😄 |
Not experimental:
Experimental: