-
Notifications
You must be signed in to change notification settings - Fork 5k
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
SLH-DSA implementation for OpenSSL #114060
Conversation
Add back APIs depending on PbeParameters for netstandard2.0 Add OpenSsl SafeHandle API Update tests
Note regarding the
|
1 similar comment
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 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
src/libraries/Common/tests/System/Security/Cryptography/MLKemImplementationTests.cs
Outdated
Show resolved
Hide resolved
...Common/tests/System/Security/Cryptography/AlgorithmImplementations/SlhDsa/SlhDsaTestsBase.cs
Show resolved
Hide resolved
...raries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPKey.SlhDsa.cs
Outdated
Show resolved
Hide resolved
...raries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EvpPKey.SlhDsa.cs
Outdated
Show resolved
Hide resolved
.../Common/tests/System/Security/Cryptography/AlgorithmImplementations/SlhDsa/SlhDsaTestData.cs
Outdated
Show resolved
Hide resolved
.../Common/tests/System/Security/Cryptography/AlgorithmImplementations/SlhDsa/SlhDsaTestData.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
...ystem.Security.Cryptography/src/System/Security/Cryptography/SlhDsaImplementation.OpenSsl.cs
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey.c
Outdated
Show resolved
Hide resolved
@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
|
...ystem/Security/Cryptography/AlgorithmImplementations/SlhDsa/SlhDsaImplementationTestsBase.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography/tests/SlhDsaOpenSslConstructionTests.NotSupported.cs
Show resolved
Hide resolved
src/libraries/System.Security.Cryptography/tests/SlhDsaOpenSslConstructionTests.Unix.cs
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_slh_dsa.c
Outdated
Show resolved
Hide resolved
...on/tests/System/Security/Cryptography/AlgorithmImplementations/SlhDsa/SlhDsaPlatformTests.cs
Outdated
Show resolved
Hide resolved
src/native/libs/System.Security.Cryptography.Native/pal_evp_pkey_slh_dsa.c
Outdated
Show resolved
Hide resolved
|
||
if (context) | ||
{ | ||
contextParams[0] = OSSL_PARAM_construct_octet_string(OSSL_SIGNATURE_PARAM_CONTEXT_STRING, (void*)context, Int32ToSizeT(contextLen)); |
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 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)
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 did that in a previous iteration but Jeremy suggested duplicating instead: #114060 (comment)
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'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.
@vcsjones Could you please run the OpenSSL 3.5 CI? @vcsjones-bot test 82c847d with openssl-3.5 |
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, []); |
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.
[] 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.
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.
Instead of a different test, the helper routine could check if length == 0 and then try both the [] and the Empty versions.
...Common/tests/System/Security/Cryptography/AlgorithmImplementations/SlhDsa/SlhDsaTestsBase.cs
Outdated
Show resolved
Hide resolved
…thmImplementations/SlhDsa/SlhDsaTestsBase.cs Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
/ba-g test failures so far look unrelated and the test run before the merge was also the same |
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