Skip to content

Improving DataProtection linkability #41411

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

Closed
HaoK opened this issue Apr 27, 2022 · 2 comments · Fixed by #41650
Closed

Improving DataProtection linkability #41411

HaoK opened this issue Apr 27, 2022 · 2 comments · Fixed by #41650
Labels
area-dataprotection Includes: DataProtection breaking-change This issue / pr will introduce a breaking change, when resolved / merged. linker-friendliness Tracking linker friendliness Needs: Design This issue requires design work before implementating.

Comments

@HaoK
Copy link
Member

HaoK commented Apr 27, 2022

After 33ea420 this causes linkability warnings everywhere that uses data protection (cookies/identity)

The default

internal sealed class RegistryPolicyResolver : IRegistryPolicyResolver
supports arbitrary types.

But by default there are only two built in types that we need support would be:

Microsoft.AspNetCore.DataProtection.AuthenticatedEncryption.ConfigurationModel.AuthenticatedEncryptorDescriptorDeserializer, Microsoft.AspNetCore.DataProtection, Version=42.42.42.42

Microsoft.AspNetCore.DataProtection.XmlEncryption.DpapiXmlDecryptor, Microsoft.AspNetCore.DataProtection, Version=42.42.42.42

The high level idea would be we just swap out the default implementation to a new implementation store that only supports these same two decryptors but is trimmable and can read the old format, and will throw an error telling them how to swap back to the old store if they use any other type. This would allow us to Apply the trimmable warning to whatever the API is new flag (maybe on DataProtectionOptions.EnableLegacyPolicyResolver?)

@HaoK HaoK added area-dataprotection Includes: DataProtection linker-friendliness Tracking linker friendliness labels Apr 27, 2022
@JamesNK
Copy link
Member

JamesNK commented Apr 28, 2022

Aren't there more places in DataProtection where types are used by default? (I only looked at DP source briefly so correct me if I'm wrong)

For example, the default IKeyManager is XmlKeyManager. The source of the XML often requires configuration, but there are times when DP will configure itself automatically. e.g. DP will detect that it's running in an Azure Website, and then automatically load XML from a known location disk. When given XML, the XML key manager will look for and load arbitrary types in the XML elements. For example, deserializerType can customize IAuthenticatedEncryptorDescriptorDeserializer.

Just fixing IRegistryPolicyResolver isn't enough.

The high level idea would be we just swap out the default implementation to a new implementation store that only supports these same two decryptors but is trimmable and can read the old format, and will throw an error telling them how to swap back to the old store if they use any other type.

I don't think we necessarily need a new IRegistryPolicyResolver here. An improvement for the average use case is just hardcoding the check for the two type names you mentioned and returning them. Then add additional logic when falling back to Type.GetType.

(maybe on DataProtectionOptions.EnableLegacyPolicyResolver?)

I like this idea. Could a flag like this be extended to all places where types are loaded from XML/registry? (or we add two flags: one for policy resolver, one for XmlKeyManager)

The flag would be disabled by default, meaning a user-defined algorithms/deserializers/providers in XML throws an error. An app can set the flag to true to allow custom types to be loaded, but there will be a trimming warning on the property. Anyone who sets it will need to think about the trimming warning. That allows the warning to be removed from IDataProtector and avoid warnings leaking out to places that use DP like auth and session.

Breaking change obviously. Some apps that update will need to add startup configuration to continue running. The question is how many people it would impact.

@davidfowl @GrabYourPitchforks @pranavkm

@ghost
Copy link

ghost commented May 4, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@adityamandaleeka adityamandaleeka added breaking-change This issue / pr will introduce a breaking change, when resolved / merged. Needs: Design This issue requires design work before implementating. labels May 4, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-dataprotection Includes: DataProtection breaking-change This issue / pr will introduce a breaking change, when resolved / merged. linker-friendliness Tracking linker friendliness Needs: Design This issue requires design work before implementating.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants