Refactor DataProtection trimming by JamesNK · Pull Request #41650 · dotnet/aspnetcore

JamesNK · 2022-05-12T05:34:50Z

Result of the discussion with Levi and Hao about how DataProtection is used:

Custom type names are rare
Registry policy is also rare. Mostly used by administrators who want to override crypto settings machine-wide. Adding a config setting to opt into registry policy isn't ideal for this situation because now the machine admin also needs to know that all apps have the setting enabled.

Since custom type names are rare, this PR removes build warnings. It's possible to get runtime errors, but this change tries to make them useful.

This PR:

Removes trimming warnings from public APIs
Suppresses warnings at a lower level
Provides friendly error message if a custom type name is used and trimmed
Statically references any built-in types, e.g. deserializers, encryptors

HaoK · 2022-05-12T18:09:09Z

Can we add a test somewhere to verify the warning experience, i.e. a trimmed app test that references a type that fails to deserialize?

JamesNK · 2022-05-13T00:08:03Z

Testing trimming is difficult because you need to build a new app, publish it, then run it.

However, I can add a test that verifies a type that can't be found throws the friendly error message.

davidfowl · 2022-05-13T05:07:01Z

src/DataProtection/DataProtection/src/KeyManagement/XmlKeyManager.cs

-        else if (type == typeof(CngCbcAuthenticatedEncryptorDescriptorDeserializer))
+        else if (type == typeof(CngCbcAuthenticatedEncryptorDescriptorDeserializer) && RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
        {
            return _activator.CreateInstance<CngCbcAuthenticatedEncryptorDescriptorDeserializer>(descriptorDeserializerTypeName);
        }
-        else if (type == typeof(CngGcmAuthenticatedEncryptorDescriptorDeserializer))
+        else if (type == typeof(CngGcmAuthenticatedEncryptorDescriptorDeserializer) && RuntimeInformation.IsOSPlatform(OSPlatform.Windows))


Is this netstandard 2.0? These APIs aren't trim friendly.

Yes, it targets netstandard2.0.

What specifically isn't trim-friendly? No warnings are coming from the linker.

My bad, I meant that RuntimeInformation.IsOSPlatform(OSPlatform.Windows) isn't recognized by the trimmer. Those branches will exist when this is trimmed and we're targeting linux.

Got it. I added IsOSPlatform to suppress errors from build, not for trimming.

A couple of types won't matter. I don't think introducing a net6-window target is worth it.

davidfowl · 2022-05-13T07:53:10Z

This might still fail in weird ways. Even if the external type is loaded, it would be missing things that it depends on that weren't preserved. We're going to let that fail randomly in that case right? (TBH, nothing can be done about that...)

JamesNK · 2022-05-13T09:27:09Z

Agreed.

Nice thing is these changes are internal and conservative. If people run into problems we can't revisit them in the future.

khellang · 2024-02-28T08:20:27Z

Hey @JamesNK! 👋🏻

I'm pretty sure these changes broke cross-version forwarding when using Azure.Extensions.AspNetCore.DataProtection.Keys. See Azure/azure-sdk-for-net#42223.

The code in XmlEncryptionExtensions used to call IActivator.CreateInstance directly. Since the KeyVault extensions register their own IActivator, which handles their own forwarding, it worked across versions, but after this change, it tries to call Type.GetType first, which results in the following System.IO.FileLoadException:

at System.RuntimeTypeHandle.GetTypeByName(String name, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMarkHandle stackMark, IntPtr pPrivHostBinder, Boolean loadTypeFromPartialName, ObjectHandleOnStack type)
   at System.RuntimeTypeHandle.GetTypeByName(String name, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMark& stackMark, IntPtr pPrivHostBinder, Boolean loadTypeFromPartialName)
   at System.RuntimeType.GetType(String typeName, Boolean throwOnError, Boolean ignoreCase, Boolean reflectionOnly, StackCrawlMark& stackMark)
   at System.Type.GetType(String typeName, Boolean throwOnError)
   at Microsoft.AspNetCore.DataProtection.XmlEncryption.XmlEncryptionExtensions.CreateDecryptor(IActivator activator, String decryptorTypeName)
   at Microsoft.AspNetCore.DataProtection.XmlEncryption.XmlEncryptionExtensions.DecryptElement(XElement element, IActivator activator)
   at Microsoft.AspNetCore.DataProtection.KeyManagement.XmlKeyManager.Microsoft.AspNetCore.DataProtection.KeyManagement.Internal.IInternalXmlKeyManager.DeserializeDescriptorFromKeyElement(XElement keyElement)
   at Microsoft.AspNetCore.DataProtection.KeyManagement.DeferredKey.<>c__DisplayClass1_0.<GetLazyDescriptorDelegate>g__GetLazyDescriptorDelegate|0()
   at System.Lazy`1.CreateValue()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Lazy`1.get_Value()
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyBase.get_Descriptor()
   at Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption.CngGcmAuthenticatedEncryptorFactory.CreateEncryptorInstance(IKey key)
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyBase.CreateEncryptor()
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRing.KeyHolder.GetEncryptorInstance(Boolean& isRevoked)
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRing.GetAuthenticatedEncryptorByKeyId(Guid keyId, Boolean& isRevoked)
   at Microsoft.AspNetCore.DataProtection.KeyManagement.KeyRingBasedDataProtector.UnprotectCore(Byte[] protectedData, Boolean allowOperationsOnRevokedKeys, UnprotectStatus& status)

Since the old version of the decryptor cannot be found.

I'd send a PR to fix this, but I'm not 100% sure what the intention of the change in this PR was and how to work around it.

JamesNK · 2024-02-28T08:28:06Z

Hi

Create an issue with what you just said. PR welcome. @amcasey is working on data protection recently. Include both of us on the issue and PR.

JamesNK added 2 commits May 12, 2022 10:31

Refactor DataProtection trimming

6bd2407

Resolve trim warnings

658a511

JamesNK added the area-dataprotection Includes: DataProtection label May 12, 2022

JamesNK requested review from HaoK, davidfowl and wtgodbe May 12, 2022 05:34

JamesNK requested a review from Pilchie as a code owner May 12, 2022 05:34

Clean up

d2e7a7e

Fix build and PR feedback

4e4cd18

davidfowl reviewed May 13, 2022

View reviewed changes

davidfowl approved these changes May 13, 2022

View reviewed changes

JamesNK merged commit 2c2074b into main May 13, 2022

JamesNK deleted the jamesnk/dataprotection-trimming-redux branch May 13, 2022 09:31

ghost added this to the 7.0-preview5 milestone May 13, 2022

JamesNK mentioned this pull request May 18, 2022

Blazor Server execption on publish after updating to .net 7 #41712

Closed

JamesNK mentioned this pull request Mar 30, 2023

Only call AddDataProtection in Authentication Services that require it #47410

Open

khellang mentioned this pull request Feb 28, 2024

[BUG] Error when upgrading Azure.Extensions.AspNetCore.DataProtection.Keys Azure/azure-sdk-for-net#42223

Closed

khellang mentioned this pull request Feb 28, 2024

Error when upgrading Azure.Extensions.AspNetCore.DataProtection.Keys #54253

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor DataProtection trimming#41650

Refactor DataProtection trimming#41650
JamesNK merged 4 commits intomainfrom
jamesnk/dataprotection-trimming-redux

JamesNK commented May 12, 2022 •

edited

Loading

Uh oh!

HaoK commented May 12, 2022

Uh oh!

JamesNK commented May 13, 2022

Uh oh!

davidfowl May 13, 2022

Uh oh!

JamesNK May 13, 2022

Uh oh!

davidfowl May 13, 2022

Uh oh!

JamesNK May 13, 2022

Uh oh!

davidfowl commented May 13, 2022 •

edited

Loading

Uh oh!

JamesNK commented May 13, 2022 •

edited

Loading

Uh oh!

khellang commented Feb 28, 2024

Uh oh!

JamesNK commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

JamesNK commented May 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HaoK commented May 12, 2022

Uh oh!

JamesNK commented May 13, 2022

Uh oh!

davidfowl May 13, 2022

Choose a reason for hiding this comment

Uh oh!

JamesNK May 13, 2022

Choose a reason for hiding this comment

Uh oh!

davidfowl May 13, 2022

Choose a reason for hiding this comment

Uh oh!

JamesNK May 13, 2022

Choose a reason for hiding this comment

Uh oh!

davidfowl commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesNK commented May 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khellang commented Feb 28, 2024

Uh oh!

JamesNK commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JamesNK commented May 12, 2022 •

edited

Loading

davidfowl commented May 13, 2022 •

edited

Loading

JamesNK commented May 13, 2022 •

edited

Loading