-
Notifications
You must be signed in to change notification settings - Fork 5k
MLDsa initial OpenSSL based implementation #113764
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
Conversation
Note regarding the
|
Note regarding the
|
Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones |
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 the initial OpenSSL‐based implementation for ML‑DSA along with updates to the public API. Key changes include:
- Adding the OpenSSL interop implementation for ML‑DSA operations (signing, verification, exporting keys).
- Replacing use of the internal ParameterSetInfo with direct use of the public MLDsaAlgorithm type and its properties.
- Updating tests and NotSupported implementations to reflect the refactored API and method signatures.
Reviewed Changes
Copilot reviewed 14 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/libraries/Common/src/System/Security/Cryptography/MLDsaImplementation.OpenSsl.cs | Implements core ML‑DSA functionality via OpenSSL interop. |
src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPKey.MLDsa.cs | Provides native interop for various ML‑DSA operations. |
src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/MLDsa/MLDsaTests.cs | Adds tests to verify ML‑DSA key generation, signing, and verification (with and without context). |
src/libraries/Common/src/Interop/Unix/System/Security/Cryptography.Native/Interop.EvpPkey.cs | Introduces ExportKeyContents to standardize key exports. |
src/libraries/Common/src/System/Security/Cryptography/MLDsaAlgorithm.cs | Makes MLDsaAlgorithm public and adds OID mapping along with property definitions. |
src/libraries/Common/src/System/Security/Cryptography/MLDsa.cs | Refactors the ML‑DSA base class to use MLDsaAlgorithm for size and OID lookups, removing internal ParameterSetInfo usage. |
Other NotSupported and platform‑specific implementation files | Update method signatures to use MLDsaAlgorithm in place of ParameterSetInfo. |
Files not reviewed (9)
- src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj: Language not supported
- src/libraries/System.Security.Cryptography/tests/System.Security.Cryptography.Tests.csproj: Language not supported
- src/native/libs/System.Security.Cryptography.Native/CMakeLists.txt: Language not supported
- src/native/libs/System.Security.Cryptography.Native/configure.cmake: Language not supported
- src/native/libs/System.Security.Cryptography.Native/entrypoints.c: Language not supported
- src/native/libs/System.Security.Cryptography.Native/opensslshim.h: Language not supported
- src/native/libs/System.Security.Cryptography.Native/pal_crypto_config.h.in: Language not supported
- src/native/libs/System.Security.Cryptography.Native/pal_evp_kem.c: Language not supported
- src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c: Language not supported
Comments suppressed due to low confidence (1)
src/libraries/Common/src/System/Security/Cryptography/MLDsa.cs:762
- Ensure that MLDsaAlgorithm.GetMLDsaAlgorithmFromOid correctly handles all expected OIDs. If an unknown OID is received, the exception path should be validated with corresponding tests to avoid unhandled cases.
MlkDsaAlgorithm algorithm = MLDsaAlgorithm.GetMLDsaAlgorithmFromOid(spki.Algorithm.Algorithm);
...braries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPKey.MLDsa.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/MLDsaImplementation.OpenSsl.cs
Outdated
Show resolved
Hide resolved
...braries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPKey.MLDsa.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/MLDsaImplementation.OpenSsl.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/MLDsaImplementation.OpenSsl.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/MLDsaImplementation.OpenSsl.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/MLDsaImplementation.OpenSsl.cs
Outdated
Show resolved
Hide resolved
src/libraries/Common/src/System/Security/Cryptography/MLDsaImplementation.OpenSsl.cs
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.c
Outdated
Show resolved
Hide resolved
|
||
int ret = -1; | ||
|
||
sigAlg = EVP_SIGNATURE_fetch(libCtx, alg, NULL); |
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.
It feels weird to me that we need to touch the EVP_SIGNATURE at all. Like, we have a pkey, why do we need to ask it what algorithm it is so we can tell OpenSSL? Shouldn't it already know?
Aside from commenting that it's weird, I wonder if there's a different pattern we can do here to avoid binding EVP_SIGNATURE_fetch at all.
Unless this is related to "make smartcards work" and getting the externalized alg?
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.
Turns out I can just pass in NULL
(and looking at OpenSSL implementation it handles it) but I don't see it documented anywhere other than the mention about implicit fetching but it doesn't say NULL (or at least I can't find it). Not sure about smart cards because it will be a while until I can test that but I think it will do the same thing with NULL
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.
This is addressed in the latest iteration but leaving open in case we're not ok with NULL
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.c
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.c
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.h
Outdated
Show resolved
Hide resolved
...braries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPKey.MLDsa.cs
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native/entrypoints.c
Outdated
Show resolved
Hide resolved
Had to squash changes because of the merge conflict. I started hitting #34649 after the rebase so not sure if there aren't any errors - will fix incrementally if needed |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.c
Outdated
Show resolved
Hide resolved
Assert.Equal(privateSeed.Length, mldsaTmp.ExportMLDsaPrivateSeed(privateSeed)); | ||
} | ||
|
||
using MLDsa mldsa = MLDsa.ImportMLDsaPrivateSeed(algorithm, privateSeed); |
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.
This pattern where you have one block-bodied key lifetime followed by an implicit-scope lifetime feels weird to me. Not necessarily worth making a commit to update, but it definitely bothers me. If a third thing were to appear then all of a sudden this would want an explicit block... so it should have just had one the whole time.
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 mostly rewrite the tests and apply this feedback, these were only to have a proof of concept
|
||
Assert.True(mldsa.VerifyData(data, signature)); | ||
|
||
((Span<byte>)signature).Fill(0); |
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.
Why not signature.AsSpan().Fill(0)
? I'm not sure I've ever seen this pattern before.
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.
too many syntaxes 😄
/ba-g Unrelated failure |
@krwq @bartonjs @vcsjones this is giving an issue compiling in source-build config. On Fedora 41 (which has OpenSSL 3.2) I get:
cc @omajid |
Contributes to: #113502
This adds initial OpenSSL based implementation for MLDsa.
Missing: