Skip to content

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

Merged
merged 4 commits into from
Mar 27, 2025
Merged

Conversation

krwq
Copy link
Member

@krwq krwq commented Mar 21, 2025

Contributes to: #113502

This adds initial OpenSSL based implementation for MLDsa.

Missing:

  • MLDsaOpenSsl class
  • Microsoft.Bcl.Cryptography stuff
  • PKCS8
  • much more extensive tests (only basic test cases were added)

@Copilot Copilot AI review requested due to automatic review settings March 21, 2025 15:49
@ghost
Copy link

ghost commented Mar 21, 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.

@ghost
Copy link

ghost commented Mar 21, 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

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

Copy link
Contributor

@Copilot 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.

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);


int ret = -1;

sigAlg = EVP_SIGNATURE_fetch(libCtx, alg, NULL);
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

@krwq krwq Mar 26, 2025

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

@krwq
Copy link
Member Author

krwq commented Mar 26, 2025

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

@krwq

This comment was marked as off-topic.

This comment was marked as off-topic.

@krwq

This comment was marked as off-topic.

This comment was marked as off-topic.

@krwq

This comment was marked as off-topic.

This comment was marked as off-topic.

@krwq

This comment was marked as off-topic.

This comment was marked as off-topic.

Assert.Equal(privateSeed.Length, mldsaTmp.ExportMLDsaPrivateSeed(privateSeed));
}

using MLDsa mldsa = MLDsa.ImportMLDsaPrivateSeed(algorithm, privateSeed);
Copy link
Member

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.

Copy link
Member Author

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);
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

too many syntaxes 😄

@bartonjs
Copy link
Member

/ba-g Unrelated failure

@bartonjs bartonjs merged commit 2adab65 into dotnet:main Mar 27, 2025
98 of 100 checks passed
@tmds
Copy link
Member

tmds commented Mar 27, 2025

@krwq @bartonjs @vcsjones this is giving an issue compiling in source-build config.

On Fedora 41 (which has OpenSSL 3.2) I get:

  /home/tmds/repos/runtime/src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.c:117:9: error: call to undeclared function 'EVP_PKEY_sign_message_init'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    117 |     if (EVP_PKEY_sign_message_init(ctx, NULL, contextParams) <= 0)
        |         ^
  /home/tmds/repos/runtime/src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.c:117:9: note: did you mean 'EVP_PKEY_sign_init'?
  /usr/include/openssl/evp.h:1913:5: note: 'EVP_PKEY_sign_init' declared here
   1913 | int EVP_PKEY_sign_init(EVP_PKEY_CTX *ctx);
        |     ^
  [  7%] Building C object libs-native/System.Security.Cryptography.Native/CMakeFiles/objlib.dir/pal_pkcs7.c.o
  [  7%] Building C object libs-native/System.Security.Cryptography.Native/CMakeFiles/objlib.dir/pal_ssl.c.o
  /home/tmds/repos/runtime/src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.c:197:9: error: call to undeclared function 'EVP_PKEY_verify_message_init'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    197 |     if (EVP_PKEY_verify_message_init(ctx, NULL, contextParams) <= 0)
        |         ^
  /home/tmds/repos/runtime/src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_ml_dsa.c:197:9: note: did you mean 'EVP_PKEY_verify_recover_init'?
  /usr/include/openssl/evp.h:1923:5: note: 'EVP_PKEY_verify_recover_init' declared here
   1923 | int EVP_PKEY_verify_recover_init(EVP_PKEY_CTX *ctx);
        |     ^
  2 errors generated.

cc @omajid

@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 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