feat: "Spanify" DataProtector with IBufferWriter<byte> #64262

DeagleGross · 2025-11-06T19:51:21Z

Please, review with caution! This is cryptography and failures here can be pretty dramatic...

This is a 2nd iteration over spanification of DataProtection. 1st iteration used a different pattern: GetSize() followed by actual operation (protection or unprotection). Here we introduce IBufferWriter<byte> as destination parameter.

The following idea came up due to the internals of DataProtection: it may happen (though it's rare) that key changes during the user's interaction with DataProtection API, and suddenly the destination buffer can be of an insufficient size. Consider:

var protectBufferSize = dataProtector.GetSize();
var destination = ArrayPool<byte>.Shared.Rent(protectBufferSize);
// >>> IMAGINE key is changed under the hood, and now destination buffer will be twice bigger <<<
if (dataProtector.TryProtect(plainText, destination))
{
...
}

However, we can simply solve it by using IBufferWriter<byte> - since it has an ability to expand its size on-the-fly.

We have experimented with the API form a lot, but decided to go with such API:

+ public interface ISpanDataProtector : IDataProtector
{
+     void Protect<TWriter>(ReadOnlySpan<byte> plaintext, ref TWriter destination)
+         where TWriter : IBufferWriter<byte>
+ #if NET
+         , allows ref struct
+ #endif
+        ;

+     void Unprotect<TWriter>(ReadOnlySpan<byte> protectedData, ref TWriter destination)
+         where TWriter : IBufferWriter<byte>
+ #if NET
+         , allows ref struct
+ #endif
+         ;
}

and

+public interface ISpanAuthenticatedEncryptor : IAuthenticatedEncryptor
{
+     void Encrypt<TWriter>(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> additionalAuthenticatedData, ref TWriter destination)
+         where TWriter : IBufferWriter<byte>
+ #if NET
+         , allows ref struct
+ #endif
+         ;

+     void Decrypt<TWriter>(ReadOnlySpan<byte> ciphertext, ReadOnlySpan<byte> additionalAuthenticatedData, ref TWriter destination)
+         where TWriter : IBufferWriter<byte>
+ #if NET
+         , allows ref struct
+ #endif
+         ;
}

New API has a few benefits and intentions:

As described above, passing IBufferWriter<byte> avoids an undeterministic scenario where buffer may be not of enough size
Usage is simplier (no need for doing GetSize() then Protect()/Unprotect() with not 100% guarantee that works, which means a fallback to allocatey API is required).
It has a generic parameter TWriter with a constraint to be IBufferWriter<byte> to allow passing in structs for even further optimization (reduction of allocation of a ref-type for buffer instance)
It has an anti-constraint and passes the buffer with ref to allows passing in a mutable ref struct. That should help in most of the cases DataProtection was designed to be used for - like Antiforgery where the input is quite small (under 100 bytes for instance), and buffer for encryption can be stackalloc'ed (which is probably the most performant mechanism to operate with a temporary buffer)

Artificial benchmarks show expected results: ref struct which uses stackalloc'ed buffer is the fastest way to operate.

That is also shown in the real benchmarks I've done on windows machine comparing original implementation, passing in a buffer which is a class, and a buffer whcih is a ref struct. With ref struct we get the most perf and use the least allocations.

Method	Job	Toolchain	RunStrategy	PlaintextLength	Mean	Error	StdDev	Median	Op/s	Gen 0	Gen 1	Gen 2	Allocated
ByteArray_ProtectUnprotectRoundtrip	Job-REJWKR	.NET Core 10.0	Throughput	80	4.039 us	0.0805 us	0.2078 us	3.939 us	247,555.9	-	-	-	512 B
PooledWriter_ProtectUnprotectRoundtrip	Job-REJWKR	.NET Core 10.0	Throughput	80	3.809 us	0.0751 us	0.0835 us	3.783 us	262,504.4	-	-	-	224 B
RefWriter_ProtectUnprotectRoundtrip	Job-REJWKR	.NET Core 10.0	Throughput	80	3.718 us	0.0711 us	0.0665 us	3.717 us	268,936.9	-	-	-	160 B

Fixes #44758

This reverts commit 11a249f.

Copilot

Pull Request Overview

This PR introduces span-based APIs for DataProtection using IBufferWriter<byte> to improve performance and reduce memory allocations. The implementation adds ISpanDataProtector and ISpanAuthenticatedEncryptor interfaces that enable zero-allocation scenarios through generic type parameters with allows ref struct constraints.

Key changes:

New span-based interfaces ISpanDataProtector and ISpanAuthenticatedEncryptor with IBufferWriter<byte> destination parameters
Implementations across all encryptors (Managed, CNG CBC, CNG GCM, AES GCM)
Two buffer writer implementations: PooledArrayBufferWriter<T> (class) and RefPooledArrayBufferWriter<T> (ref struct)
Comprehensive test coverage and benchmarking infrastructure

Reviewed Changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/Shared/PooledArrayBufferWriter.cs	Adds class-based pooled buffer writer implementation with validation logic issue
src/Shared/Buffers/RefPooledArrayBufferWriter.cs	Adds ref struct pooled buffer writer with similar validation issue
src/DataProtection/Abstractions/src/ISpanDataProtector.cs	Defines span-based data protector interface
src/DataProtection/DataProtection/src/AuthenticatedEncryption/ISpanAuthenticatedEncryptor.cs	Defines span-based authenticated encryptor interface
src/DataProtection/DataProtection/src/KeyManagement/KeyRingBasedSpanDataProtector.cs	Implements span-based data protector using key ring
src/DataProtection/DataProtection/src/Managed/ManagedAuthenticatedEncryptor.cs	Adds span API implementations with unnecessary using directive
src/DataProtection/DataProtection/src/Managed/AesGcmAuthenticatedEncryptor.cs	Implements span-based GCM encryption
src/DataProtection/DataProtection/src/Cng/*.cs	Implements span-based APIs for CNG encryptors
src/DataProtection/samples/KeyManagementSimulator/Program.cs	Updates mock encryptor with typo in parameter name
src/DataProtection/DataProtection/test/*/.cs	Adds comprehensive roundtrip tests
src/DataProtection/benchmarks/**	Adds benchmarking project showing performance improvements

Copilot · 2025-11-11T00:17:28Z

src/Shared/PooledArrayBufferWriter.cs

+        if (sizeHint <= 0)
+        {
+            throw new ArgumentOutOfRangeException(nameof(sizeHint), actualValue: sizeHint, $"{nameof(sizeHint)} ('{sizeHint}') must be a non-negative value.");
+        }


The condition sizeHint <= 0 on line 175 is incorrect. It should check if sizeHint < 0 (negative) instead. The next condition on line 180 checks sizeHint == 0, which means the first condition will always throw for sizeHint == 0 before reaching the second condition. This creates unreachable code.

Copilot · 2025-11-11T00:17:28Z

src/Shared/Buffers/RefPooledArrayBufferWriter.cs

+        if (sizeHint <= 0)
+        {
+            throw new ArgumentOutOfRangeException(nameof(sizeHint), actualValue: sizeHint, $"{nameof(sizeHint)} ('{sizeHint}') must be a non-negative value.");


The same issue exists here: checking sizeHint <= 0 followed by sizeHint == 0 creates unreachable code. The condition should check sizeHint < 0 instead to allow the zero case to be handled by the subsequent condition.

Copilot · 2025-11-11T00:17:28Z

src/DataProtection/samples/KeyManagementSimulator/Program.cs

-    byte[] IAuthenticatedEncryptor.Encrypt(ArraySegment<byte> plaintext, ArraySegment<byte> _additionalAuthenticatedData) => plaintext.ToArray();
+    public byte[] Encrypt(ArraySegment<byte> plaintext, ArraySegment<byte> _additionalAuthenticatedData) => plaintext.ToArray();
+
+    public void Encrypt<TWriter>(ReadOnlySpan<byte> plainttext, ReadOnlySpan<byte> additionalAuthenticatedData, ref TWriter destination) where TWriter : System.Buffers.IBufferWriter<byte>, allows ref struct


Typo in parameter name: should be plaintext instead of plainttext (double 't').

Copilot · 2025-11-11T00:17:29Z

src/DataProtection/DataProtection/src/Managed/ManagedAuthenticatedEncryptor.cs

 using System;
 using System.Buffers;
 using System.Diagnostics;
+using System.Drawing;


Unnecessary using directive System.Drawing was added. This namespace is not used in the file and should be removed.

Suggested change

using System.Drawing;

halter73 · 2025-11-13T01:54:53Z

src/DataProtection/Abstractions/src/ISpanDataProtector.cs

+    void Protect<TWriter>(ReadOnlySpan<byte> plaintext, ref TWriter destination)
+        where TWriter : IBufferWriter<byte>
+#if NET
+        , allows ref struct


Would it be better to drop the netstandard support for this high-performance (perhaps unwieldy) API limit ISpanDataProtector and ISpanAuthenticatedEncryptor to .NET 11 and above?

halter73 · 2025-11-13T02:05:52Z

src/Shared/Buffers/RefPooledArrayBufferWriter.cs

+
+    private void CheckAndResizeBuffer(int sizeHint)
+    {
+        if (sizeHint <= 0)


Suggested change

if (sizeHint <= 0)

if (sizeHint < 0)

Good catch @copilot. We should add a test case where sizeHint is zero.

I guess I never should have @-mentioned copilot. Of course, it opened a PR because of this 😆

halter73 · 2025-11-13T02:10:05Z

src/Shared/Buffers/RefPooledArrayBufferWriter.cs

+
+            var previousBuffer = oldBuffer.AsSpan(0, _index);
+            previousBuffer.CopyTo(_rentedBuffer);
+            previousBuffer.Clear();


Suggested change

previousBuffer.Clear();

Do we need to clear here? Presumably this will all be overwritten anyway. If we did want to clear, we should probably pass clearArray: true to ArrayPool<byte>.Return right below, but I don't think that's necessary.

halter73 · 2025-11-13T02:22:57Z

src/Shared/Buffers/RefPooledArrayBufferWriter.cs

+            // Optionally clear the buffer before returning to pool (security)
+            // Uncomment if needed for sensitive data:
+            // _buffer.AsSpan(0, _index).Clear();
+
+            ArrayPool<T>.Shared.Return(_rentedBuffer, clearArray: false);
+            _buffer = null!;


No reason to leave the dead code around or to be explicit about clearArray when not clearing is the well-known default. But if we did want to reduce risk, we could create an ArrayPool using ArrayPool<byte>.Create(). that's used exclusively for DataProtection rather than use the shared one. This would also give us more control over the max size of pooled arrays and the maximum number of arrays to pool.

It's also unclear what nulling out the buffer does. Presumably the stack frame with the ref struct will be popped shortly after this will be disposed.

Suggested change

// Optionally clear the buffer before returning to pool (security)

// Uncomment if needed for sensitive data:

// _buffer.AsSpan(0, _index).Clear();

ArrayPool<T>.Shared.Return(_rentedBuffer, clearArray: false);

_buffer = null!;

ArrayPool<T>.Shared.Return(_rentedBuffer);

halter73 · 2025-11-13T02:24:27Z

src/Shared/Buffers/RefPooledArrayBufferWriter.cs

+    }
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    private void ThrowIfDisposed()


Do we ever give user code access to this IBufferWriter<T>? If not, the most efficient thing would probably be to replace this with a Debug.Assert.

halter73 · 2025-11-13T02:49:10Z

src/Shared/Buffers/RefPooledArrayBufferWriter.cs

+    public void Advance(uint count)
+        => Advance((int)count);
+


Suggested change

public void Advance(uint count)

=> Advance((int)count);

Other than tests, don't we always have to call the IBufferWriter overload that takes int anyway? I see a bunch of calls to both Advance(checked((int) and just Advance((int). We should probably be more consistent with it. Since we reject any count below zero, I think unchecked conversions from uint are fine.

halter73 · 2025-11-13T02:56:12Z

src/Shared/Buffers/RefPooledArrayBufferWriter.cs

+        // no rented buffer initially - only if we need to grow over the limits of initialBuffer
+        _rentedBuffer = null!;
+
+        _buffer = initialBuffer;
+        _index = 0;


I don't think this comment is necessary once we properly mark the field as nullable.

Suggested change

// no rented buffer initially - only if we need to grow over the limits of initialBuffer

_rentedBuffer = null!;

_buffer = initialBuffer;

_index = 0;

_buffer = initialBuffer;

halter73 · 2025-11-13T02:58:26Z

src/DataProtection/DataProtection/src/Cng/CbcAuthenticatedEncryptor.cs

+                    var buffer = destination.GetSpan(checked((int)dwRequiredSize));
+
+                    // Clone IV again for the actual decryption call
+                    byte* pbClonedIV2 = stackalloc byte[checked((int)_symmetricAlgorithmBlockSizeInBytes)];


This is the second call to checked((int)_symmetricAlgorithmBlockSizeInBytes). While checked operations aren't that expensive, it's probably good to reuse the operation or just do the unchecked cast.

Suggested change

byte* pbClonedIV2 = stackalloc byte[checked((int)_symmetricAlgorithmBlockSizeInBytes)];

byte* pbClonedIV2 = stackalloc byte[(int)_symmetricAlgorithmBlockSizeInBytes];

halter73 · 2025-11-13T03:01:14Z

src/DataProtection/DataProtection/src/Cng/CbcAuthenticatedEncryptor.cs

-        CryptoUtil.Assert(dwActualDecryptedByteCount <= dwEstimatedDecryptedByteCount, "dwActualDecryptedByteCount <= dwEstimatedDecryptedByteCount");
-        if (dwActualDecryptedByteCount == dwEstimatedDecryptedByteCount)
+#else
+        var pooledArrayBuffer = new PooledArrayBufferWriter<byte>(outputSize);


Is there any reason we cannot use using for these?

Suggested change

var pooledArrayBuffer = new PooledArrayBufferWriter<byte>(outputSize);

using var pooledArrayBuffer = new PooledArrayBufferWriter<byte>(outputSize);

halter73 · 2025-11-13T03:07:31Z

src/DataProtection/DataProtection/src/Cng/CbcAuthenticatedEncryptor.cs


-        // Assumption: pbCipherText := { keyModifier | IV | encryptedData | MAC(IV | encryptedPayload) }
+#if NET
+        byte[] rentedBuffer = null!;


As a general rule, we shouldn't use the null-forgiving operator when initializing variables/fields/properties, unless we're intentionally lying about the nullability of a public property that's always non-null by the time the user sees it, and even that's super questionable. It's better to save the null-forgiving until later in the code when we know for sure the variable/field/property must not be null. In this case, the only other place we use it we have a if (rentedBuffer is not null) anyways.

Suggested change

byte[] rentedBuffer = null!;

byte[]? rentedBuffer = null;

This is really making me wish C# had macros though. I wonder if we should just have another overload to RefPooledArrayBufferWriter that takes an initial size other than a stackalloced Span. Not only would it simplify a lot of this repeated code a little bit, this would ensure the RefPooledArrayBufferWriter is always be responsible for renting and returning arrays to the pool.

Copilot · 2025-11-13T03:09:00Z

@halter73 I've opened a new pull request, #64339, to work on those changes. Once the pull request is ready, I'll request review from you.

DeagleGross added 30 commits July 15, 2025 21:40

implement encrypt size calculation

b1a4b40

implement geteencyrptedsize

aecf589

try encrypt

198501e

simplify net10 impl

a4e3ca4

simplify tests and cbc

8d49973

cnggcm

ae6fb76

aes

9caa72e

something

b9b45b2

key ring based part

b3e5b42

Merge branch 'main' into dmkorolev/dataprotector-spans

4eb96e3

refactor

afd344c

finally passed!

fd7f929

generate test for plain text

afbad47

correct empty plain text scenario

34d52e7

minor improvements

18eec02

hide in internal + docs

9e4a634

to public api - its a nightmare to implement otherwise

8829566

fix build

5f240c2

wip

8d275ac

re-review

8f5b762

implement timelimitedDataProtector

efd4f79

introduce IOptimizedDataProtector

6142309

introduce optimized IAuthenticatedEncryptor

ae5bb82

fix dataprotector usage

0646d1a

fix build?

3d37955

move to a separate ISpanAuthenticatedEncryptor

bbfce53

refactor to ISpan interfaces

b0fc525

correct span allocation

d4b1fc9

use TryEncrypt from Encrypt()

146af98

fix

630e451

DeagleGross added 12 commits October 9, 2025 20:41

fix warnings

e6d0d0a

fix warnings in benchmarks

41c2163

fix build?

bf9537a

rollback benchmark changes

77dd694

init + cbc + mock

97db08b

cng gcm

edada0c

aes gcm

1c16e20

encryptors

cef12f0

change

96b8d93

fixes / tests / etc

400ec6d

nit

d309725

reimplement with "ref"

05e3dde

github-actions bot added the area-dataprotection Includes: DataProtection label Nov 6, 2025

build-analysis bot mentioned this pull request Nov 6, 2025

Known Build Error: error : Cannot create a file when that file already exists. #61178

Open

diff!

ccb4bc8

build-analysis bot mentioned this pull request Nov 7, 2025

Known Test Repository Issue Sometimes Misses Test Failures dotnet/dnceng#6411

Open

3 tasks

DeagleGross added 6 commits November 7, 2025 14:15

fix AES-GCM encrypt with pre-post buffers

72418b6

fix reading spans

6b4823b

rewrite refPooledArrayBufferWriter

d300807

setup numbers using stackalloc

4d2a8b4

improve CBC authenticated encryptor

11a249f

Revert "improve CBC authenticated encryptor"

7601537

This reverts commit 11a249f.

DeagleGross marked this pull request as ready for review November 11, 2025 00:07

Copilot AI review requested due to automatic review settings November 11, 2025 00:07

Copilot started reviewing on behalf of DeagleGross November 11, 2025 00:07 View session

Copilot finished reviewing on behalf of DeagleGross November 11, 2025 00:08

Copilot AI reviewed Nov 11, 2025

View reviewed changes

halter73 reviewed Nov 13, 2025

View reviewed changes

Copilot AI mentioned this pull request Nov 13, 2025

[WIP] Add test case for zero sizeHint in DataProtector #64339

Closed

	byte* pbClonedIV2 = stackalloc byte[checked((int)_symmetricAlgorithmBlockSizeInBytes)];
	byte* pbClonedIV2 = stackalloc byte[(int)_symmetricAlgorithmBlockSizeInBytes];

	var pooledArrayBuffer = new PooledArrayBufferWriter<byte>(outputSize);
	using var pooledArrayBuffer = new PooledArrayBufferWriter<byte>(outputSize);

feat: "Spanify" DataProtector with IBufferWriter<byte> #64262

Are you sure you want to change the base?

feat: "Spanify" DataProtector with IBufferWriter<byte> #64262

Conversation

DeagleGross commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 11, 2025

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Nov 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DeagleGross commented Nov 6, 2025 •

edited

Loading