[release/10.0] Fix ManagedAuthenticatedEncryptor calculations (#65890)#65934
[release/10.0] Fix ManagedAuthenticatedEncryptor calculations (#65890)#65934wtgodbe merged 2 commits intorelease/10.0from
Conversation
There was a problem hiding this comment.
Pull request overview
Backports the fix for ManagedAuthenticatedEncryptor MAC computation/validation on pre-.NET 5 targets, and expands DataProtection test coverage to include .NET Framework to prevent regressions in the future.
Changes:
- Fix
ManagedAuthenticatedEncryptor.CalculateAndValidateMacto hash the correct payload region and compare the computed MAC correctly on NETFX. - Multi-target
Microsoft.AspNetCore.DataProtection.Testsfor$(DefaultNetFxTargetFramework)and adjust tests/code to compile/run across TFMs. - Add/adjust conditional compilation and minor test compatibility changes (e.g., temp dir creation, UnixFileMode test gating, span assertions).
Reviewed changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/DataProtection/DataProtection/src/Managed/ManagedAuthenticatedEncryptor.cs | Fixes MAC calculation/validation offsets and correct hash comparison behavior for NETFX. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Microsoft.AspNetCore.DataProtection.Tests.csproj | Multi-targets netcore + netfx; conditions Hosting reference for netcore-only. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Managed/ManagedAuthenticatedEncryptorTests.cs | Test compatibility updates (adds usings; adjusts byte comparison). |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingBasedDataProtectorTests.cs | Adjusts assertions for cross-TFM compatibility (span/array comparisons). |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Repositories/FileSystemXmlRepositoryTests.cs | NET-only gating for UnixFileMode test; Guid.Parse overload adjustment. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Repositories/RegistryXmlRepositoryTests.cs | Guid.Parse overload adjustment for netfx compatibility; adds explicit usings. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/HostingTests.cs | Wraps Hosting tests in #if NET to avoid netfx build issues. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Internal/KeyManagementOptionsPostSetupTest.cs | Replaces Directory.CreateTempSubdirectory with netfx-compatible temp dir creation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/XmlEncryption/XmlEncryptionExtensionsTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/XmlEncryption/NullXmlEncryptionTests.cs | Adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/XmlEncryption/EncryptedXmlDecryptorTests.cs | Adds explicit usings / normalizes header for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/XmlEncryption/DpapiXmlEncryptionTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/XmlEncryption/DpapiNGXmlEncryptionTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/XmlEncryption/CertificateXmlEncryptionTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/XmlAssert.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/TypeForwardingActivatorTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/StringLoggerFactory.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/ServiceCollectionTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/SecretTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/SecretAssert.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/SP800_108/SP800_108Tests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Repositories/EphemeralXmlRepositoryTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/RegistryPolicyResolverTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/XmlKeyManagerTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyRingProviderTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/KeyEscrowServiceProviderExtensionsTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DeferredKeyTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/DefaultKeyResolverTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/CacheableKeyRingTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/KeyManagement/AdditionalAuthenticatedDataTemplateTests.cs | Adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Internal/KeyManagementOptionsSetupTest.cs | Adds explicit usings / normalizes header for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Internal/Int7BitEncodingUtilsTests.cs | Adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/EphemeralDataProtectionProviderTests.cs | Adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/DataProtectionUtilityExtensionsTests.cs | Fixes usings ordering and adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/ContainerUtilsTests.cs | Adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Cng/GcmAuthenticatedEncryptorTests.cs | Adds explicit usings and adjusts byte comparison for multi-target compatibility. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Cng/CngAuthenticatedEncryptorBaseTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/Cng/CbcAuthenticatedEncryptorTests.cs | Adds explicit usings and adjusts byte comparison for multi-target compatibility. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ManagedAuthenticatedEncryptorFactoryTest.cs | Normalizes header and adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/ManagedAuthenticatedEncryptorDescriptorTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/ManagedAuthenticatedEncryptorDescriptorDeserializerTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/ManagedAuthenticatedEncryptorConfigurationTests.cs | Adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/CngGcmAuthenticatedEncryptorDescriptorTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/CngGcmAuthenticatedEncryptorDescriptorDeserializerTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/CngGcmAuthenticatedEncryptorConfigurationTests.cs | Adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/CngCbcAuthenticatedEncryptorDescriptorTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/CngCbcAuthenticatedEncryptorDescriptorDeserializerTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/CngCbcAuthenticatedEncryptorConfigurationTests.cs | Adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/AuthenticatedEncryptorDescriptorTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/ConfigurationModel/AuthenticatedEncryptorDescriptorDeserializerTests.cs | Adds explicit usings for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/CngGcmAuthenticatedEncryptorFactoryTest.cs | Normalizes header and adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/AuthenticatedEncryption/CngCbcAuthenticatedEncryptorFactoryTest.cs | Normalizes header and adds explicit Xunit using for multi-target compilation. |
| src/DataProtection/DataProtection/test/Microsoft.AspNetCore.DataProtection.Tests/ActivatorTests.cs | Adds explicit usings for multi-target compilation. |
| @@ -518,11 +517,11 @@ private void CalculateAndValidateMac( | |||
| #else | |||
| // if validationSubkey is stackalloc'ed, there is no way we avoid an alloc here | |||
| validationAlgorithm.Key = validationSubkeyArray ?? validationSubkey.ToArray(); | |||
| correctHashArray = validationAlgorithm.ComputeHash(payloadArray, macOffset, eofOffset - macOffset); | |||
| correctHash = validationAlgorithm.ComputeHash(payloadArray, ivOffset, macOffset - ivOffset); | |||
| #endif | |||
There was a problem hiding this comment.
In the NET<10 branch, correctHash is initialized via stackalloc/array, but then immediately reassigned to the array returned by ComputeHash. This makes the initial stackalloc path redundant (and the stack buffer won’t get cleared because the span reference is overwritten). Consider restructuring so the Span<byte> is only used in the NET10_0_OR_GREATER path, and the NET<10 path uses a separate byte[]/ReadOnlySpan<byte> variable for the computed hash.
| byte[] decipheredtext = encryptor.Decrypt(new ArraySegment<byte>(ciphertext), aad); | ||
|
|
||
| // Assert | ||
| Assert.Equal(plaintext.AsSpan(), decipheredtext.AsSpan()); | ||
| Assert.Equal(plaintext.AsSpan().ToArray(), decipheredtext.AsSpan().ToArray()); | ||
| } |
There was a problem hiding this comment.
decipheredtext is already a byte[], so decipheredtext.AsSpan().ToArray() allocates a copy unnecessarily. You can compare against the expected bytes without cloning the actual value (e.g., compare decipheredtext directly to the expected array, or use SequenceEqual on spans).
| .Returns<ArraySegment<byte>, ArraySegment<byte>>((actualPlaintext, actualAad) => | ||
| { | ||
| Assert.Equal(expectedPlaintext, actualPlaintext.AsSpan()); | ||
| Assert.Equal(expectedAad, actualAad.AsSpan()); | ||
| Assert.Equal(expectedPlaintext, actualPlaintext.AsSpan().ToArray()); | ||
| Assert.Equal(expectedAad, actualAad.AsSpan().ToArray()); | ||
| return new byte[] { 0x23, 0x29, 0x31, 0x37 }; // ciphertext + tag |
There was a problem hiding this comment.
These assertions allocate via AsSpan().ToArray() just to compare bytes. Since ArraySegment<byte>.AsSpan() supports SequenceEqual, you can avoid allocations by comparing spans directly (and apply the same pattern to the other similar assertions in this file).
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework> | ||
| <TargetFrameworks>$(DefaultNetCoreTargetFramework);$(DefaultNetFxTargetFramework)</TargetFrameworks> |
There was a problem hiding this comment.
Are there any other tests projects we should add NetFx targets to?
There was a problem hiding this comment.
Yep, but not in a patch or this PR.
* Update Helix queue * Update SkipOnHelix queues for StartupTests * Update SkipOnHelix queue for Http2Tests * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Backport of #65890
Fix ManagedAuthenticatedEncryptor calculations
Description
Code targeting <.NET5 and using the 10.0+ version of the DataProtection nuget package would not be able to use Dataprotection due to every Decrpyt operation throwing.
Fixes #65889
Customer Impact
DataProtection not usable. No customers explicitly noticed this since it's not a core scenario, however it's still supported and a regression.
Regression?
10.0
Risk
Fixed test coverage issue and reverted broken code to 9.0 version.
Verification
Packaging changes reviewed?