Add ML-KEM/ML-DSA support for C# wrapper #9040
Conversation
|
🛟 Devin Lifeguard found 3 likely issues in this PR
@tamasan238 |
|
Add _new/_delete API for ML-KEM/ML-DSA Use case: Add C# examples using ML-KEM/ML-DSA |
|
Retest this please Jenkins: Codespell warning |
There was a problem hiding this comment.
Pull Request Overview
This PR implements ML-KEM (post-quantum key encapsulation mechanism) and ML-DSA (post-quantum digital signature algorithm) support for the C# wrapper of wolfSSL. The implementation adds comprehensive cryptographic APIs for both quantum-resistant algorithms.
- Adds ML-KEM key generation, encapsulation/decapsulation, and key encoding/decoding functions
- Implements ML-DSA key generation, signing, verification, and key import/export operations
- Includes comprehensive test coverage for both ML-KEM and ML-DSA functionality
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| wrapper/CSharp/wolfSSL_CSharp/wolfSSL.cs | Adds key share support and named group enums for post-quantum cryptography |
| wrapper/CSharp/wolfSSL_CSharp/wolfCrypt.cs | Implements complete ML-KEM and ML-DSA cryptographic APIs with P/Invoke declarations |
| wrapper/CSharp/wolfCrypt-Test/wolfCrypt-Test.cs | Adds comprehensive test cases for ML-KEM and ML-DSA functionality |
| wrapper/CSharp/README.md | Documents how to enable PQC support through compile-time flags |
Comments suppressed due to low confidence (5)
wrapper/CSharp/wolfCrypt-Test/wolfCrypt-Test.cs:770
- The function call should be prefixed with 'wolfcrypt.' to match the pattern used elsewhere in the test file.
ret = DilithiumExportPrivateKey(key, out privateKey);
wrapper/CSharp/wolfCrypt-Test/wolfCrypt-Test.cs:775
- The function call should be prefixed with 'wolfcrypt.' to match the pattern used elsewhere in the test file.
ret = DilithiumExportPublicKey(key, out publicKey);
wrapper/CSharp/wolfCrypt-Test/wolfCrypt-Test.cs:784
- The function call should be prefixed with 'wolfcrypt.' to match the pattern used elsewhere in the test file.
ret = DilithiumImportPrivateKey(privateKey, key);
wrapper/CSharp/wolfCrypt-Test/wolfCrypt-Test.cs:789
- The function call should be prefixed with 'wolfcrypt.' to match the pattern used elsewhere in the test file.
ret = DilithiumImportPublicKey(publicKey, key);
wrapper/CSharp/wolfCrypt-Test/wolfCrypt-Test.cs:792
- The error message is inconsistent - it says 'DilithiumImportPrivateKey failed' but the actual call on line 789 was DilithiumImportPublicKey. The error message should be 'DilithiumImportPublicKey failed'.
throw new Exception("DilithiumImportPrivateKey failed");
| ret = wolfcrypt.MlKemEncapsulate(keyA, out cipherText, out sharedSecretA); | ||
| if (ret != 0) | ||
| { | ||
| throw new Exception("Failed to encalsulate."); |
There was a problem hiding this comment.
The word "encalsulate" is misspelled. It should be "encapsulate".
| throw new Exception("Failed to encalsulate."); | |
| throw new Exception("Failed to encapsulate."); |
wrapper/CSharp/README.md
Outdated
| mono server.exe -S | ||
| ``` | ||
|
|
||
| ## PQC Suppport |
There was a problem hiding this comment.
The word "Suppport" is misspelled. It should be "Support".
| ## PQC Suppport | |
| ## PQC Support |
| [DllImport(wolfssl_dll, CallingConvention = CallingConvention.Cdecl)] | ||
| private static extern int wc_MlKemKey_Delete(IntPtr key, ref IntPtr key_p); | ||
| [DllImport(wolfssl_dll, CallingConvention = CallingConvention.Cdecl)] | ||
| private static extern int wc_MlKemKey_Init(IntPtr key, int type, IntPtr heap, int devId); |
There was a problem hiding this comment.
The parameter order for wc_MlKemKey_Init is inconsistent between WindowsCE and non-WindowsCE versions. The WindowsCE version has (int type, IntPtr key, IntPtr heap, int devId) while the non-WindowsCE version has (IntPtr key, int type, IntPtr heap, int devId).
| private static extern int wc_MlKemKey_Init(IntPtr key, int type, IntPtr heap, int devId); | |
| private static extern int wc_MlKemKey_Init(int type, IntPtr key, IntPtr heap, int devId); |
There was a problem hiding this comment.
@tamasan238 thanks! Looks good for the most part. Please double check the flagged WINCE build comment.
@gojimmypi could you take a look at this PR having C# dev fresh on your mind?
|
Hi @tamasan238 - Looks like some nice PQ updates for wolfSSL C#. Thank you! I'm seeing some runtime errors: I followed the instructions in the README update: As seen here: 
It appears 
Unrelated WarningsThere are some warnings that seem to have snuck in over time, unrelated to this PR: I'm addressing those warnings in #8946 |
|
Also, I checked with anhu & confirmed additional macros need to be defined. For instance, check out this section of the |
|
Thank you for your review! |
tamasan238
force-pushed
the
for-pr-2
branch
2 times, most recently
from
7d1e654 to
f9e935b
Compare
|
1️⃣ 1. typo
2. WinCE Error
3. configuration options added some options for wolfSSL(wolfCrypt). 2️⃣
I would appreciate it if you could check it after referring to the above. |
|
Hi @tamasan238 - looks like you are making good progress on the C# library and examples! I do have a question: why is the PQ code only conditionally compiled in? The current conditional code is primarily platform-based, no? (e.g. decorators specific to Windows CE). Do you think it would be better if the PQ code was always on, always available, with no need for manually defined macros? |
|
Thank you for your review! Yes, I think it is better to be available without any macros. If some PQC APIs are unavailable in wolfSSL, the C# wrapper test program( However, I believe it is also important to keep the code base and build procedures as simple as possible. |
|
Hi @tamasan238 Yes, I agree that building the native C wolfSSL DLL would need attention. As the PQ code there is gated with macros defined in the You could leave a brief comment around the C# declarations indicating which macros are assumed to have been enabled in the external wolfSSL DLL. |
|
Oh, I see! That is the best plan. |
|
I applied them. Is this style correct? // Build test will fail now because PR #9039 has not yet been merged. |
|
Hi @tamasan238 I fetched your latest code from this pr2 branch, and applied the patch from pr1. I see there's a section added in the wrapper/CSharp/user_settings.h: /* Enable ML-KEM, ML-DSA */
#define HAVE_MLKEM
#define WOLFSSL_WC_MLKEM
#define WOLFSSL_HAVE_MLKEM
#define WOLFSSL_DTLS_CH_FRAG
#define HAVE_DILITHIUM
#define WOLFSSL_WC_DILITHIUM
#define WOLFSSL_SHAKE128
#define WOLFSSL_SHAKE256This only enables ML-KEM/ML-DSA support, eh? The reason I ask, is the out-of-the-box experience is such that my C# client in this PR will not connect to my unmodified listener. I see this error for the C# Here's the message I see at the server (a stand-alone RPi with clean wolfssl): My server was build like this: and I'm running it like this The failure occurs in if (con.Send(msg, 0, msg.Length, SocketFlags.None) == 0 && sz != 0)Can you please confirm what you see when running the examples? Any special instructions needed? |
|
This is strange behavior. I will check it right away. |
|
I could reproduce that issue with unmodified wolfssl v5.8.2. Communication between Server/Client under the C# wrapper directory should work fine. Examples of ML-KEM/ML-DSA usage can be found here. |
|
jenkins retest this please |
|
Hi @tamasan238 I'm glad to see you were able to reproduce the connection problem that I encountered. I don't recall seeing & resolving that issue, but it does not occur on my WIP #8946 btw: Note your failing haproxy Test / v3.1.0 (pull_request) is resolved with #9061 Perhaps if you refresh from upstream to fix HAProxy, it will also resolve the C# connection error? I didn't dig into the root cause. I'll take a closer look at the ML-KEM/ML-DSA examples soon. As for the code review: I see you followed the existing pattern in the I would have implemented something differently: one that completed all the tests, showed results, then threw a single error at the end, or even just gracefully exit with a non-zero exit code. Refactoring that is well beyond the scope of the PR though, and I suspect someone had a specific objective in mind there. Perhaps you can find out why that pattern was chosen? In any case: thank you for your work on the C# PQ. It will be a nice addition! Cheers |
|
Thank you for your confirmation!
I still haven't figured out the root cause of this issue.
Indeed, even if a test fails, subsequent tests should still be executed. #9039 should be merged soon. |
|
jenkins retest this please |
|
The API addition PR(#9039) has been merged!
Could you please re-check or/and merge this PR? |
|
I've confirmed the ML-KEM / ML-DSA test app runs successfully in VS 2022. I didn't get a chance today to test client / server apps, nor a specific code review yet. |
tamasan238
force-pushed
the
for-pr-2
branch
3 times, most recently
from
a77c646 to
a56f605
Compare
|
jenkins retest this please |
tamasan238
force-pushed
the
for-pr-2
branch
from
a56f605 to
2bfcc47
Compare
|
jenkins retest this please |
|
@gojimmypi I fixed mistakes. Could you please check them? |
| Console.WriteLine("Generate Key Pair A..."); | ||
| keyA = wolfcrypt.MlKemMakeKey(type, heap, devId); | ||
| if (keyA == IntPtr.Zero) | ||
| { |
There was a problem hiding this comment.
Is there a reason why ret = -1 (or some failure) is not set here?
| keyB = wolfcrypt.MlKemMakeKey(type, heap, devId); | ||
| if (keyB == IntPtr.Zero) | ||
| { | ||
| Console.Error.WriteLine("Failed to generate key pair B."); |
There was a problem hiding this comment.
Same as keyA check, above
| Console.Error.WriteLine("Failed to generate key pair B."); | ||
| } | ||
|
|
||
| Console.WriteLine("ML-KEM Key Generation test passed."); |
There was a problem hiding this comment.
What if either keyA == IntPtr.Zero or keyB == IntPtr.Zero ?
| { | ||
| Console.Error.WriteLine("DilithiumMakeKey failed"); | ||
| } | ||
| Console.WriteLine("ML-DSA Key Generation test passed."); |
There was a problem hiding this comment.
what if key == IntPtr.Zero ?
| /* Cleanup */ | ||
| if (keyA != IntPtr.Zero) | ||
| { | ||
| ret = wolfcrypt.MlKemFreeKey(keyA); |
There was a problem hiding this comment.
The soft errors tracked by ret values (above) is good.
At the end here, the prior value of ret is lost. This may be important for test monitoring.
If there are any external tests such as GitHub workflows that expect error codes upon failure, it would be lost here, no?
Although it is much better to not throw many exceptions for each failure, there's also the desire to match existing code patterns. Since the test does not return a success / fail (it should, but beyond the scope of this PR) ... do you think it would be best to put just one final throw exception at the end here?
Having just one exception will make future refactoring easier.
| try | ||
| { | ||
| ret = wc_MlKemKey_PublicKeySize(key, ref pubLen); | ||
| if (pubLen == 0) |
There was a problem hiding this comment.
same as above: what if wc_MlKemKey_PublicKeySize failed and returned a negative value but pubLen != 0 ?
| int ret; | ||
| ss = null; | ||
| uint ssLen = 0; | ||
|
|
There was a problem hiding this comment.
Consider checking the parameters before proceeding.
| ret = wc_dilithium_delete(key, ref heap); | ||
| key = IntPtr.Zero; | ||
| } | ||
| return ret; |
There was a problem hiding this comment.
what if key == IntPtr.Zero ?
| int ret = 0; | ||
| int privLen = 0; | ||
| uint outLen; | ||
|
|
There was a problem hiding this comment.
Consider checking parameters before proceeding.
| { | ||
| Console.Error.WriteLine("Failed to encapsulate."); | ||
| } | ||
| Console.WriteLine("ML-KEM Encapsulation test passed."); |
There was a problem hiding this comment.
Is this message always printed? Same question with others, below.
|
Thank you for checking. |
3 participants
Description
Implementing ML-KEM and ML-DSA APIs in the C# wrapper.
For this PR to work, you need to merge other PRs that you will add in comments.
Testing
Tested.
Checklist