Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a C# P/Invoke wrapper for HPKE (RFC 9180) plus a console-style test that exercises Base mode seal/open and public-key serialization round-trip.
Changes:
- Added HPKE native imports and managed helper APIs (init, keygen, serialize/deserialize, seal/open, free).
- Added HPKE Base mode test flow to the C# test runner.
- Enabled
HAVE_HPKEin the C# wrapper user settings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| wrapper/CSharp/wolfSSL_CSharp/wolfCrypt.cs | Adds HPKE P/Invoke bindings and managed convenience APIs for base-mode single-shot operations |
| wrapper/CSharp/wolfCrypt-Test/wolfCrypt-Test.cs | Adds an HPKE base-mode functional test (keygen, serialize/deserialize, seal/open) |
| wrapper/CSharp/user_settings.h | Enables HPKE in the C# wrapper build configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using System; | ||
| using System.Collections.Concurrent; | ||
| using System.Runtime.InteropServices; |
There was a problem hiding this comment.
System.Collections.Concurrent is used unconditionally, but this wrapper has #if WindowsCE support throughout the file. ConcurrentDictionary is not available on .NET Compact Framework/Windows CE, so defining WindowsCE will fail to compile. Consider wrapping the HPKE context tracking in #if !WindowsCE (or multi-targeting) and using a simpler Dictionary<IntPtr, HpkeContextState> + lock (or another CF-compatible collection) for WindowsCE builds.
| if (plaintext == null || plaintext.Length == 0) | ||
| { | ||
| log(ERROR_LOG, "HPKE seal base: invalid plaintext"); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
HpkeSealBase currently rejects plaintext.Length == 0, but HPKE (and the native wc_HpkeSealBase API) can validly seal an empty plaintext (ciphertext would be just the AEAD tag). Consider allowing zero-length plaintexts (keep rejecting plaintext == null) so callers can encrypt empty messages and so the wrapper matches the native API semantics.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| uint infoSz = (info != null) ? (uint)info.Length : 0; | ||
| uint aadSz = (aad != null) ? (uint)aad.Length : 0; | ||
| uint ptSz = (uint)plaintext.Length; | ||
|
|
There was a problem hiding this comment.
HpkeSealBase computes sealLen as (int)ptSz + HPKE_Nt without guarding against plaintext.Length > int.MaxValue - HPKE_Nt. For very large inputs this can overflow to a negative length and throw or lead to incorrect buffer sizing. Add an explicit bounds check (similar to HpkeOpenBase) and fail cleanly before allocating arrays.
| if (plaintext.Length > int.MaxValue - HPKE_Nt) | |
| { | |
| log(ERROR_LOG, "HPKE seal base: plaintext too large"); | |
| return null; | |
| } |
| /// <param name="kem">KEM used (needed to free the ephemeral key)</param> | ||
| /// <returns>enc||ciphertext byte array or null on failure</returns> | ||
| public static byte[] HpkeSealBase(IntPtr hpke, IntPtr receiverKey, | ||
| byte[] info, byte[] aad, byte[] plaintext, HpkeKem kem) | ||
| { |
There was a problem hiding this comment.
The convenience overload HpkeSealBase(..., HpkeKem kem) uses the caller-provided kem only to free the internally-generated ephemeralKey in finally. If the caller accidentally passes a mismatched KEM, wc_HpkeFreeKey will free the key with the wrong type (e.g., treating an ecc_key as curve25519_key), which can crash or corrupt memory. Since the wrapper can derive the KEM from the hpke context (it is already stored in HpkeContextState), remove this parameter or validate it against the context before freeing.
| } | ||
|
|
||
| int pubKeySzInt = encCiphertext.Length - sealLen; | ||
| if (pubKeySzInt < 0 || pubKeySzInt > ushort.MaxValue) |
There was a problem hiding this comment.
HpkeOpenBase(..., int ptLen) derives pubKeySz from the remaining bytes and currently allows up to ushort.MaxValue (65KB) before allocating/copying. This should be capped to the expected enc length for the context/KEM (or at least HPKE_Npk_MAX) to avoid unnecessary large allocations and to reject malformed inputs earlier.
| if (pubKeySzInt < 0 || pubKeySzInt > ushort.MaxValue) | |
| if (pubKeySzInt < 0 || pubKeySzInt > HPKE_Npk_MAX) |
Description
Add HPKE (RFC 9180) C# wrapper and tests
Testing
Done with CSharp tests
Checklist