Skip to content

SLH-DSA implementation for OpenSSL #114060

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 13 commits into from
Apr 18, 2025

Conversation

PranavSenthilnathan
Copy link
Member

@PranavSenthilnathan PranavSenthilnathan commented Mar 31, 2025

Adds the OpenSSL implementation for SLH-DSA. This mostly uses ML-DSA and ML-KEM as a reference and consolidates common APIs where applicable.

Fixes #114665

Add back APIs depending on PbeParameters for netstandard2.0
Add OpenSsl SafeHandle API
Update tests
@ghost
Copy link

ghost commented Mar 31, 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 Mar 31, 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 OpenSSL implementation for SLH-DSA by consolidating common APIs and aligning test infrastructure with the new changes.
  • Key changes include:
    • Replacing explicit OpenSSL version checks with a new boolean property (IsOpenSsl3_5) in test code.
    • Refactoring SLH-DSA test infrastructure by replacing “TestImplementation” with “MockImplementation” and adding abstract methods to standardize key-generation and import/export APIs.
    • Updating test vectors and adjusting interop/dynamic linking files; documentation updates reflect native build target path changes.

Reviewed Changes

Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.

File Description
src/libraries/Common/tests/System/Security/Cryptography/MLKemImplementationTests.cs Simplifies platform check using IsOpenSsl3_5.
src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/SlhDsa/SlhDsaTestsBase.cs Adds abstract methods for key generation and import/export in test base.
src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/SlhDsa/SlhDsaMockImplementation.cs Renames test implementation class for clarity.
docs/workflow/building/libraries/README.md Updates build command documentation for native libraries.
Comments suppressed due to low confidence (1)

src/libraries/Common/tests/System/Security/Cryptography/AlgorithmImplementations/SlhDsa/SlhDsaMockImplementation.cs:11

  • Renaming from 'SlhDsaTestImplementation' to 'SlhDsaMockImplementation' improves clarity; ensure that related documentation and comments are updated accordingly.
internal sealed class SlhDsaMockImplementation : SlhDsa

@vcsjones
Copy link
Member

vcsjones commented Apr 2, 2025

@PranavSenthilnathan The build is failing here because Linux is a case-sensitive file system and your csproj file and the file name differ by case

Interop.EvpPkey.SlhDsa.cs vs Interop.EvpPKey.SlhDsa.cs (The k vs K)


if (context)
{
contextParams[0] = OSSL_PARAM_construct_octet_string(OSSL_SIGNATURE_PARAM_CONTEXT_STRING, (void*)context, Int32ToSizeT(contextLen));
Copy link
Member

@krwq krwq Apr 16, 2025

Choose a reason for hiding this comment

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

I think this entire method is identical to ML-DSA minus one assert, we might want to consider unifying the core part (and same for sign)

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 did that in a previous iteration but Jeremy suggested duplicating instead: #114060 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I'm open to squeezing it down to shared things, but as a followup. Naming and shaping being the hard parts, of course 😄. The earlier attempt didn't feel right, and just having an entirely separate function felt/feels like the best short term answer, rather than ratholing on the shape and blocking other progress.

@PranavSenthilnathan
Copy link
Member Author

@vcsjones Could you please run the OpenSSL 3.5 CI?

@vcsjones-bot test 82c847d with openssl-3.5

@vcsjones
Copy link
Member

@vcsjones Could you please run the OpenSSL 3.5 CI?

https://github.com/vcsjones/runtime-ci/actions/runs/14499783145

@bartonjs also has the power to do this.

The bot still doesn't allow anyone other than me (the trouble with the bot is it does other things than .NET Runtime related tasks and I need to figure out a good auth model for the whole thing).

byte[] signature = new byte[mldsa.Algorithm.SignatureSizeInBytes];
Assert.Equal(signature.Length, mldsa.SignData([], signature));

ExerciseSuccessfulVerify(mldsa, [], signature, []);
Copy link
Member

Choose a reason for hiding this comment

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

[] and ReadOnlySpan.Default sometimes P/Invoke differently. We probably want a specific test for the span-based methods that ensure that ReadOnlySpan.Default works. That can be done as a followup.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of a different test, the helper routine could check if length == 0 and then try both the [] and the Empty versions.

PranavSenthilnathan and others added 2 commits April 17, 2025 09:45
…thmImplementations/SlhDsa/SlhDsaTestsBase.cs

Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
@PranavSenthilnathan
Copy link
Member Author

/ba-g test failures so far look unrelated and the test run before the merge was also the same

@PranavSenthilnathan PranavSenthilnathan merged commit 15bcd62 into dotnet:main Apr 18, 2025
87 of 99 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 18, 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.

ML-DSA: Assert signing empty span
4 participants