Skip to content
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

perf: improve ManagedAuthenticatedEncryptor Decrypt() and Encrypt() flow #59424

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

Conversation

DeagleGross
Copy link
Contributor

@DeagleGross DeagleGross commented Dec 10, 2024

The goal of PR is to bring linux performance closer to windows performance for DataProtection scenario. Below is the picture of Antiforgery benchmarks on win vs lin machines.
image

The benchmark I am relying on to show the result numbers is here, which is basically building default ServiceProvider, adding DataProtection via .AddDataProtection() and calling IDataProtector.Protect() or IDataProtector.Unprotect().

Results:

net Code Method Mean Error StdDev Gen0 Allocated
net10 main branch Unprotect 9.706 µs 0.0143 µs 0.0134 µs 0.1068 3.34 KB
net10 changed Unprotect 8.421 µs 0.0252 µs 0.0236 µs 0.2747 1.75 KB
net10 main branch Protect 10.88 µs 0.015 µs 0.014 µs 0.6256 3.91 KB
net10 changed Protect 9.791 µs 0.0259 µs 0.0242 µs 0.3662 2.33 KB

Optimization details

I looked into Unprotect method for ManagedAuthenticatedEncryptor and spotted MemoryStream usage and multiple Buffer.BlockCopy usages. Also I saw that there is some shuffling of byte[] data, which I think can be skipped and performed in such a way, that some allocations are skipped.

In order to be as safe as possible, I created a separate DataProtectionPool which provides API to rent and return byte arrays. It is not intersecting with ArrayPool<byte>.Shared.

  1. ManagedSP800_108_CTR_HMACSHA512.DeriveKeys is changed to explicit usage ManagedSP800_108_CTR_HMACSHA512.DeriveKeysHMACSHA512, because _kdkPrfFactory is anyway hardcoded to use HMACSHA512. There is a static API allowing to hash without allocating kdk byte[] which is rented from the buffer: HMACSHA512.TryHashData(kdk, prfInput, prfOutput, out _);

  2. Avoided usage of DeriveKeysWithContextHeader which allocates a separate intermediate array for contextHeader and context. Instead passing the spans operationSubkey and validationSubkey directly into ManagedSP800_108_CTR_HMACSHA512.DeriveKeys

  3. ManagedSP800_108_CTR_HMACSHA512.DeriveKeysHMACSHA512 had 2 more arrays (prfInput and prfOutput), which now I am renting (via DataProtectionPool) or even stackalloc'ing. They are returned to the pool with clearArray: true flag to make sure key material is removed from the memory after usage.

  4. In Decrypt() flow I am again using HashAlgorithm.TryComputeHash overload, which works based on the Span<byte> types, compared to previously used HashAlgorithm.ComputeHash

  5. In Decrypt() flow changed usage to SymmetricAlgorithm.DecryptCbc() instead of CryptoTransform.TransformBlock() with same idea to use Span<byte> API instead of another byte[] allocation.

  6. Encrypt() flow is reusing №1, №2 and №3 optimizations as well

  7. Encrypt() before was relying on the MemoryStream and CryptoStream to write data in the result buffer, but I am pre-calculating the length, and then doing a single allocation of result array: var outputArray = new byte[keyModifierLength + ivLength + cipherTextLength + macLength]; All required data is copied into the outputArray via APIs supporting Span<byte>.

All listed optimizations are included in the net10 TFM, but only some (№ 2, №3 and №6) are used in netstandard2.0 and netFx TFMs which DataProtection also targets.

Related to #59287

@DeagleGross DeagleGross changed the title [DRAFT] perf: improve ManagedAuthenticatedEncryptor.Decrypt flow [DRAFT] perf: improve ManagedAuthenticatedEncryptor Decrypt() and Encrypt() flow Dec 17, 2024
@DeagleGross DeagleGross marked this pull request as ready for review December 19, 2024 14:31
@DeagleGross DeagleGross changed the title [DRAFT] perf: improve ManagedAuthenticatedEncryptor Decrypt() and Encrypt() flow perf: improve ManagedAuthenticatedEncryptor Decrypt() and Encrypt() flow Dec 19, 2024
@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Dec 19, 2024

Code review notes

  1. It's generally not advisable to pool buffers for sensitive cryptographic operations, such as those which perform key storage or manipulation. This tends to increase the attack surface of the application and should only be performed if it's absolutely required to meet some performance goal. If you absolutely must use pooled buffers, use different pooled buffers for key material specifically vs. (all other data). In dataprotection, key material would be the KDK, KEK, and individual decryption + validation keys.

  2. This PR introduces a call to the SymmetricAlgorithm.Key property setter. The original code intentionally avoided calling this property setter because it duplicates the sensitive key material in such a way that the caller has no control over the new lifetime, and this undermines other protections present in the system (the use of the Secret type and the widespread use of pinning within the core crypto logic). Of course, the EG can always say that the new behavior is preferred over the old behavior, but there needs to be an explicit acknowledgement that there is a security tradeoff here. The tradeoff shouldn't be a mere side effect that is likely to go unnoticed.

    It's possible we can make changes to the underlying SymmetricAlgorithm type to improve the perf without reducing the security stance, but this would require new API to be exposed within corelib. Since you're targeting these changes for net10 that's probably acceptable? Work with Jeff's team to give them your requirements and they can add the work to the backlog.

  3. The pattern if (len < CONST) { foo = stackalloc[len]; } else { foo = Rent(len).Slice(len); } is typically considered an antipattern. Prefer a pattern like if (len < CONST) { foo = stackalloc[CONST]; } else { foo = Rent(len); } foo = foo.Slice(len); instead. (Basically, the stackalloc should be a const, not variable-length.)

Scenario notes

Is the goal to improve the performance of the [real-world?] AntiForgery benchmark or to improve the performance of DataProtection in a standalone benchmark? The PR description (and attached graph) make it sound like improving the performance of the crank-based benchmark is the goal, but no throughput measurement is provided for the changes in this PR. Please provide that graph. It would supply evidence that these changes have real-world impact and aren't just microbenchmark improvements.

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.

3 participants