MLDsa initial OpenSSL based implementation#113764
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.
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
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.
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.
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.
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.
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.
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.
Why not signature.AsSpan().Fill(0)? I'm not sure I've ever seen this pattern before.
|
/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: