Skip to content

feat: "Spanify" DataProtector Protect [part 1] #62903

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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

DeagleGross
Copy link
Member

@DeagleGross DeagleGross commented Jul 24, 2025

"Spanify" DataProtector Protect [part 1]

Notes

I've implemented only Encrypt part here in order to not have an enormous PR doing Decryption as well. I am willing to do the Decryption part in a follow-up PR. Let me know if that is OK or we should do it in a single PR already.

Details

Current PR proposes additions to IDataProtector and IAuthenticatedEncryptor interfaces. The APIs are:

interface Microsoft.AspNetCore.DataProtection.IDataProtector
{
+ int GetProtectedSize(ReadOnlySpan<byte> plainText);
+ bool TryProtect(ReadOnlySpan<byte> plainText, Span<byte> destination, out int bytesWritten);
}

and

interface Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption.IAuthenticatedEncryptor
{
+ int GetEncryptedSize(int plainTextLength);
+ bool TryEncrypt(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> additionalAuthenticatedData, Span<byte> destination, out int bytesWritten);
}

These APIs are basically doing what existing Protect() / Unprotect() and Encrypt() / Decrypt() do, but without allocating a result array and instead filling in the Span<byte> destination.

Testing

In order to test the behavior and to not have a lot of changes, I decided to not touch existing APIs, however they also can be calling Try... APIs. This approach also helps to test the changes against existing API - for example we can check if encryption works correctly:

// new API
var tryEncryptResult = encryptor.TryEncrypt(plaintext, aad, cipherTextPooled, out var bytesWritten);
Assert.True(tryEncryptResult);

// existing API
var decipheredTryEncrypt = encryptor.Decrypt(new ArraySegment<byte>(cipherTextPooled, 0, expectedSize), aad);

// verify original plaintext is the same as decrypted text
Assert.Equal(plaintext.AsSpan(), decipheredTryEncrypt.AsSpan());

Fixes #44758

@github-actions github-actions bot added the area-dataprotection Includes: DataProtection label Jul 24, 2025
@DeagleGross DeagleGross changed the title feat: "Spanify" DataProtector Protect/Unprotect feat: "Spanify" DataProtector Protect [part 1] Jul 24, 2025
@DeagleGross DeagleGross marked this pull request as ready for review July 24, 2025 13:54
@Copilot Copilot AI review requested due to automatic review settings July 24, 2025 13:54
@DeagleGross DeagleGross self-assigned this Jul 24, 2025
Copy link
Contributor

@Copilot Copilot AI left a 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 adds new non-allocating "Span-based" APIs to the DataProtection interfaces, specifically implementing the Protect/Encrypt functionality without requiring heap allocations. The new APIs allow callers to get the size of protected data upfront and then encrypt directly into a provided buffer.

Key changes:

  • Added GetProtectedSize() and TryProtect() methods to IDataProtector interface
  • Added GetEncryptedSize() and TryEncrypt() methods to IAuthenticatedEncryptor interface
  • Implemented these methods across all encryptor types (AES-GCM, CBC, CNG variants, Managed implementations)

Reviewed Changes

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

Show a summary per file
File Description
src/DataProtection/Abstractions/src/IDataProtector.cs Added new span-based APIs to the main data protector interface
src/DataProtection/DataProtection/src/AuthenticatedEncryption/IAuthenticatedEncryptor.cs Added new span-based APIs to the authenticated encryptor interface
src/DataProtection/DataProtection/src/KeyManagement/KeyRingBasedDataProtector.cs Core implementation of new APIs with proper header management
src/DataProtection/DataProtection/src/Managed/ManagedAuthenticatedEncryptor.cs Implementation for managed (non-CNG) encryption algorithms
src/DataProtection/DataProtection/src/Managed/AesGcmAuthenticatedEncryptor.cs Implementation for AES-GCM encryption
src/DataProtection/DataProtection/src/Cng/CbcAuthenticatedEncryptor.cs Implementation for CNG-based CBC encryption
src/DataProtection/DataProtection/src/Cng/CngGcmAuthenticatedEncryptor.cs Implementation for CNG-based GCM encryption
src/DataProtection/Extensions/src/TimeLimitedDataProtector.cs Updated time-limited protector to support new APIs
Multiple test files Comprehensive test coverage for the new functionality

var plainTextSpan = plaintext.AsSpan();

#if NET10_0_OR_GREATER
var size = GetEncryptedSize(plainTextSpan.Length);
Copy link
Preview

Copilot AI Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the new span-based TryEncrypt method is already implemented, the allocating Encrypt method should delegate to it instead of duplicating the implementation. This would reduce code duplication and ensure consistency between the two methods.

Copilot uses AI. Check for mistakes.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should update our existing code that calls into IDataProtecter to use the new methods to get good test coverage of the new methods.

/// </summary>
/// <param name="plainText">The plain text that will be encrypted later</param>
/// <returns>The length of the encrypted data</returns>
int GetProtectedSize(ReadOnlySpan<byte> plainText);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's breaking to add methods to an interface without a DIM. Otherwise, you'll get type load errors. If it weren't for that, I'd add the new methods to netstandard as well.

The DIM can throw, but ideally, we'd make it work or at least have a way to detect whether or not the new methods are supporting without try/catch (like with a bool IsSpanSupported property or something like that). Without that, I don't think anyone can safely consume the new interface methods from our own code like cookie auth or antiforgery because we'll have no way of knowing if we can use it safely.

I'd rather have a DIM that just allocated and worked than having to add an IsSupported-style property to IDataProtector, but I don't think it would work well with GetProtectedSize, since you'd be forced to call byte[] Protect(byte[] plaintext) and then throw away the result which seems overly wasteful.

At the point, I wonder if creating a new interface would be the simpler way to detect if an IDataProtector supports spans.

Copy link
Member Author

@DeagleGross DeagleGross Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback Stephen!
I've put the methods into new interfaces then - it makes sense and it is easier to have a new interface than controlling availability of the API via property IMO:

+ public interface IOptimizedDataProtector : IDataProtector
{
+    int GetProtectedSize(ReadOnlySpan<byte> plainText);
+    bool TryProtect(ReadOnlySpan<byte> plainText, Span<byte> destination, out int bytesWritten);
}
+ public interface IOptimizedAuthenticatedEncryptor : IAuthenticatedEncryptor
{
+     int GetEncryptedSize(int plainTextLength);
+     bool TryEncrypt(ReadOnlySpan<byte> plaintext, ReadOnlySpan<byte> additionalAuthenticatedData, Span<byte> destination, out int bytesWritten);
}

note: those are introduced only for NET10 or greater.

Can you please elaborate on

I think we should update our existing code that calls into IDataProtecter to use the new methods to get good test coverage of the new methods.

Do you propose changing the usage across the whole aspnetcore? Should I do it in a follow-up PR probably and after I support decryption flow?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dataprotection Includes: DataProtection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API proposal: Add ReadOnlySpan<byte> to IDataProtector (un)Protect
2 participants