Implement X25519DiffieHellmanCng by vcsjones · Pull Request #127924 · dotnet/runtime

vcsjones · 2026-05-07T16:32:36Z

Contributes to #126206

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

dotnet-policy-service · 2026-05-07T16:34:25Z

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

vcsjones · 2026-05-07T16:33:36Z

+        internal static bool TryDeriveKeyMaterialTruncate(
+            SafeNCryptSecretHandle secretAgreement,
+            SecretAgreementFlags flags,
+            Span<byte> destination,


The current ones all return an allocation, this introduces a real span-writing one so the span-writing key agreement can use it without an intermediate allocation.

vcsjones · 2026-05-07T16:35:27Z

+                validOids,
+                source,
+                keyReader,
+                static (key, kr, in algId, out ret) => kr(key, algId, out ret),


The goal with the changes on KeyFormatHelpers is to introduce overloads of PKCS#8 reading to accept a state. The existing overloads use the new state overflows and just erase the concept of a state.

In this case the non-state callback is the state. That allows us to avoid any captures.

The goal was to make it so the PKCS#8 readers could accept a Span<byte> as state so that they can write to it. Using TOut would force an allocation.

vcsjones · 2026-05-07T16:37:20Z

+
+namespace System.Security.Cryptography
+{
+    internal static class X25519WindowsHelpers


This file is just extracting common things out of X25519DiffieHellmanImplementation.Windows into a shared location so that the ncrypt one can use it. There were some changes in API shape to make the sharing easier but overall the same spirit.

vcsjones · 2026-05-07T16:40:47Z

No functional changes in this file aside from adapting to the extracted helpers.

vcsjones · 2026-05-07T16:42:44Z

+{
+    internal static class CngHelpers
+    {
+        public static CryptographicException ToCryptographicException(this Interop.NCrypt.ErrorCode errorCode)


In order to import CngKey's that are X25519, I needed to pull in quite a bit of p/invokes because CngKey.Import can't do it.

This extension method is needed by the p/invoke helpers but it is defined in a different helpers file that would require pulling in a bunch of other things I didn't want to pull in, so it's just defined stand-alone here.

Copilot

Pull request overview

This PR adds a Windows CNG-backed implementation of X25519DiffieHellman (X25519DiffieHellmanCng) to enable interop with CNG keys/providers, and factors shared Curve25519/CNG-blob handling into a reusable Windows helper.

Changes:

Introduces X25519DiffieHellmanCng (Windows implementation + public surface + non-Windows thrower).
Refactors Windows X25519 key import/export and public-key reduction logic into X25519WindowsHelpers.
Adds CNG-focused test coverage and adjusts base tests to account for platforms that can’t roundtrip “unclamped” private scalars.

Show a summary per file

File	Description
src/libraries/System.Security.Cryptography/tests/X25519DiffieHellmanCngTests.cs	Adds CNG-specific tests for construction, export policies, and non-exportable behavior.
src/libraries/System.Security.Cryptography/tests/X25519DiffieHellmanBaseTests.cs	Adds `CanRoundTripKeys` and shared helper logic for clamped/unclamped private key expectations.
src/libraries/System.Security.Cryptography/tests/System.Security.Cryptography.Tests.csproj	Includes new test and required Windows/CNG helper sources for the test assembly.
src/libraries/System.Security.Cryptography/tests/CngHelpers.cs	Adds a small test-only helper for mapping NCrypt error codes to `CryptographicException`.
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X25519DiffieHellmanImplementation.Windows.cs	Switches Windows BCrypt implementation to use the shared helper for reduction/blob operations.
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X25519DiffieHellmanCng.Windows.cs	Implements the Windows CNG key agreement + export logic for `X25519DiffieHellmanCng`.
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/X25519DiffieHellmanCng.cs	Adds the public `X25519DiffieHellmanCng` API (Windows-only) and docs.
src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/Cng.NotSupported.cs	Adds non-Windows stubs for `X25519DiffieHellmanCng` that throw `PlatformNotSupportedException`.
src/libraries/System.Security.Cryptography/src/System.Security.Cryptography.csproj	Wires new source files into the product build and links the shared Windows helper.
src/libraries/System.Security.Cryptography/src/Resources/Strings.resx	Adds a new resource string for invalid CNG X25519 key usage.
src/libraries/System.Security.Cryptography/ref/System.Security.Cryptography.cs	Adds the new public ref surface for `X25519DiffieHellmanCng`.
src/libraries/Common/src/System/Security/Cryptography/X25519WindowsHelpers.cs	New shared helper for public-key reduction + CNG blob create/export + scalar fixup/refix.
src/libraries/Common/src/System/Security/Cryptography/KeyFormatHelper.Encrypted.cs	Adds stateful overloads to avoid captures/allocations (supports ref-struct state).
src/libraries/Common/src/System/Security/Cryptography/KeyFormatHelper.cs	Adds a stateful `KeyReader` delegate + overload to pass state into PKCS#8 readers.
src/libraries/Common/src/System/Security/Cryptography/ECCng.ImportExport.NamedCurve.cs	Adds an optional `flags` parameter to `ImportKeyBlob` to support import flags.
src/libraries/Common/src/Interop/Windows/NCrypt/Interop.NCryptDeriveKeyMaterial.cs	Adds a Span-based P/Invoke overload and `TryDeriveKeyMaterialTruncate` helper.
src/libraries/Common/src/Interop/Windows/NCrypt/Interop.Keys.cs	Adds a Span-based `NCryptExportKey` overload for allocation-free export.

Copilot's findings

Files reviewed: 17/17 changed files
Comments generated: 9

bartonjs · 2026-05-08T17:44:47Z

+
+namespace System.Security.Cryptography
+{
+    internal static class X25519WindowsHelpers


Does this need to be in common instead of just under S.S.C? I know that a lot of the older algorithms are here because of how the implementation libraries were split, and the PQC ones are here because of M.B.C, but X25519 seems contained to S.S.Cryptography...

(I haven't confirmed yet, but I'm starting to suspect the answer is "it's shared in the tests")

I haven't confirmed yet, but I'm starting to suspect the answer is "it's shared in the tests"

Yes, that. We don't have many good examples of tests pulling in directly from src but lots from Common, so I stuck with Common.

bartonjs · 2026-05-08T18:03:19Z

+
+            // Assume that if key agreement is being done between two different X25519DiffieHellmanCng instances then
+            // the Provider is the same. It's the caller's responsibility to ensure the providers match.
+            if (otherParty is X25519DiffieHellmanCng x25519Cng)


We don't have the ECDH "well, we shipped it years ago... and... compat..." problem here, and I sort of feel like this block will have zero coverage outside of unit tests. I think we should just skip the "it's already an NCRYPT_KEY_HANDLE" optimization path and always do the else path.

Unless I'm forgetting some scenario that this is actually important to enable?

(I just really expect any public key to be a BCRYPT_KEY_HANDLE, rather than doing x25519 between two persisted keys)

It was mostly perf, but it is also a way to "get out of the way". If you are using CNG APIs you are already doing something "interesting" and I wanted this to be an "as close as possible to using NCrypt APIs" as reasonable. We can still yank it out though.

bartonjs · 2026-05-08T19:11:22Z

+        public override X25519DiffieHellmanCng ImportPrivateKey(ReadOnlySpan<byte> source)
+        {
+            using CryptoPoolLease lease = X25519WindowsHelpers.CreateCngBlob(source, true, out _);
+            using SafeNCryptKeyHandle keyHandle = ECCng.ImportKeyBlob(


If this is the reason we need the giant pile of P/Invokes in the tests... I believe that current versions of Windows (all that we test on) support doing a CngKey.Import with a NCRYPT_ECC_CURVE_NAME_PROPERTY ("ECCCurveName") property specified.

The NCRYPTBUFFER_DESC variant I think was required on older OSes, so the runtime code still needs them, but we can probably skip it in tests.

If this is the reason we need the giant pile of P/Invokes in the tests

Yes.

CngKey.Import doesn't allow you pass any properties during import, and it's too late to set the property after the key has been finalized, and we have no way of passing DO_NOT_FINALIZE.

According to... me... in https://stackoverflow.com/questions/50856998/can-someone-explain-c-sharp-cngkey-import-please/50861324, CngKey.Create can be coerced into doing an import by specifying the blob type as a property name. Using magic powers of observation, I believe BCRYPT_ECCPRIVATE_BLOB to be such a supported value. (CngKeyBlobTypes.EccPrivateKey.Format as the property name)

So that'd be something like

CngKeyCreateParameters cngParams = new(); cngParams.Properties.Add(new CngProperty(CngKeyBlobTypes.EccPrivateKey.Format, keyBlob, CngPropertyOptions.None)); cngParams.Properties.Add(new CngProperty("ECCCurveName", BytesOf("curve25519"), 0)); CngKey key = CngKey.Create(CngAlgorithm.ECDiffieHellman, null, cngParams);

Yes, that is Create, which I did exactly what you said for GenerateKey. We need Import though.

Oh I misunderstood. You are using Create as import. Okay, let me try that.

Actually it still doesn't matter - we need to pass NCRYPT_NO_KEY_VALIDATION to dwFlags of NCryptImportKey. I do not see a way to do NCRYPT_NO_KEY_VALIDATION with public CngKey APIs.

Implement X25519DiffieHellmanCng

bf68812

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 7, 2026 16:32

Copilot started reviewing on behalf of vcsjones May 7, 2026 16:32 View session

github-actions Bot added the area-System.Security label May 7, 2026

dotnet-policy-service Bot assigned vcsjones May 7, 2026

vcsjones commented May 7, 2026

View reviewed changes

Copilot AI reviewed May 7, 2026

View reviewed changes

Copilot feedback

defee89

build-analysis Bot mentioned this pull request May 8, 2026

Unable to pull image from mcr.microsoft.com #117164

Open

bartonjs reviewed May 8, 2026

View reviewed changes

Conversation

vcsjones commented May 7, 2026

Uh oh!

dotnet-policy-service Bot commented May 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vcsjones May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vcsjones May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vcsjones May 8, 2026 •

edited

Loading

vcsjones May 8, 2026 •

edited

Loading