Improve usage of Type.GetType when activating types in data protection

[khellang]

Fixes #54253
Fixes #48910

There are some documented cases where Type.GetType(<name>, throwOnError: false) can still throw:

The throwOnError parameter specifies what happens when the type is not found, and also suppresses certain other exception conditions, as described in the Exceptions section. Some exceptions are thrown regardless of the value of throwOnError. For example, if the type is found but cannot be loaded, a System.TypeLoadException is thrown even if throwOnError is false.

This happens in the case where a decryptorType of previous version of an assembly has been used and prevents a custom IActivator implementation from doing proper forwarding.

This PR handles this case by catching errors from Type.GetType and letting the activator handle it instead.

Another way of solving this could be to make TypeForwardingActivator public and expose a way to register other namespaces to forward. Today, the DecryptorTypeForwardingActivator type in Azure.Extensions.AspNetCore.DataProtection.Keys is more or less an outdated copy of the TypeForwardingActivator in this repo.

// @JamesNK @amcasey

[amcasey]

LGTM, but let's give @JamesNK time to comment. Thanks!

[JamesNK]

I worry about static state like this impacting other unit tests. It's not being reset so unit tests run afterwards would run this func. And if it was reset, there is still the possibility of concurrent tests.

Is it necessary? Can a value be specified that would trigger TypeLoadException without having to override how the GetType call worked.

[khellang]

I did try to find a way to produce a FileLoadException or TypeLoadException by modifying the type, namespace, assembly, version etc. of the decryptorType, but wasn't able to; no matter what I did, it kept returning null instead of throwing.

We're probably seeing this more because we're using .NET Framework with (semi-)manual binding redirects, while .NET (Core) seems much more forgiving.

If you have any suggestions, I'm all ears 😅

[JamesNK]

Not having a unit test is one solution. However, when I wrote this code, I didn't consider that an error could be thrown here. Catching it is a band aid.

Maybe the solution here is to match to these known types from a string without using Type.GetType. If the type name and assembly match, then we go directly to the known types. We'd be getting into the type name parsing business though, which is scary.

[khellang]

.NET Core probably changed the behavior so that throwOnError was always respected.

It's still documented to throw under the same circumstances, but .NET (Core) is much more resilient when it comes to resolving multiple versions of the same assemblies.

Not having a unit test is one solution. However, when I wrote this code I didn't consider that an error could be thrown here. Catching it is a band aid.

Not necessarily - you're accepting a string "user input" and passing it to a function that could throw in some documented cases. I think it's Totally Fine™ to catch the exception and have a fallback behavior.

Maybe the solution here is to match to these known types from a string without using Type.GetType. If the type name and assembly match, then we go directly to the known types. We'd be getting into the type name parsing business though, which is scary.

Yeah, why were the type checks introduced in the first place? I see there's a similar issue in XmlKeyManager.CreateDeserializer.

I have a crazy idea; what about an internal (or maybe even public?) interface

internal interface ITypeForwardingActivator : IActivator
{
    bool TryForwardType(string originalTypeName, [NotNullWhen(true)] out Type? type);
}

which could be checked by pattern matching

private static IXmlDecryptor CreateDecryptor(IActivator activator, string decryptorTypeName)
{
    if (activator is ITypeForwardingActivator typeForwarder && typeForwarder.TryForwardType(decryptorTypeName, out var type))
    {
        if (type == typeof(DpapiNGXmlDecryptor))
        {
            return activator.CreateInstance<DpapiNGXmlDecryptor>(decryptorTypeName);
        }
        else if (type == typeof(DpapiXmlDecryptor))
        {
            return activator.CreateInstance<DpapiXmlDecryptor>(decryptorTypeName);
        }
        else if (type == typeof(EncryptedXmlDecryptor))
        {
            return activator.CreateInstance<EncryptedXmlDecryptor>(decryptorTypeName);
        }
        else if (type == typeof(NullXmlDecryptor))
        {
            return activator.CreateInstance<NullXmlDecryptor>(decryptorTypeName);
        }
    }

    return activator.CreateInstance<IXmlDecryptor>(decryptorTypeName);
}

This would be implemented by the default TypeForwardingActivator and reuse the same existing logic.

Then the unit test could provide its own faked/mocked activator that returns false from TryForwardType through the IActivator argument?

[khellang]

I've pushed a commit to show what it would look like 😄

[khellang]

But the "what are we even testing at this point?" question is becoming more and more relevant 😆

[khellang]

@JamesNK What do you think about the approach? Keep it or just keep it simple and remove the test altogether?

[JamesNK]

Sorry for the delay. I looked at the latest updates.

A problem I see is that custom activators won't have ITypeForwardingActivator. It will be a behavior change for them.

Another problem is type names get resolved over and over which is bad if Type.GetType throws an expensive exception. I want to match the old behavior without a regression in perf.

I made some changes that I think solves these two issues. Can you check out: https://github.com/dotnet/aspnetcore/compare/jamesnk/check-type-name-before-resolve

Changes:

• Before calling Type.GetType, the name is checked whether it is a known type. There is still a try/catch to handle an error, but this should reduce or eliminate exceptions from custom implementations.
• Added ITypeNameResolver interface for resolving type names. If the IActivator instance implements it then it then the passed in implementation is used, otherwise it falls back to a default implementation. Only used in tests.

What do you think? If you're happy with the changes I can start a new PR or you're welcome to update this one.

[khellang]

What do you think? If you're happy with the changes I can start a new PR or you're welcome to update this one.

LGTM. I'm most concerned about getting the bug patched 😄 I've rebased the branch, picked up your commit and removed a now-unnecessary suppression 👍

[JamesNK]

Do you need this patched in a 8.0 servicing release?

@amcasey Can you take another look.

[khellang]

Do you need this patched in a 8.0 servicing release?

That would be awesome. Otherwise we'd have to wait until november? 😅

[amcasey]

Generally looks good, but I had some questions.

[JamesNK]

@amcasey PR feedback applied.

[khellang]

I see there's an 8.0.3 version of the data protection packages out. Was this ever backported?

[JamesNK]

This missed the deadline for 8.0.4. We'll see about getting it into 8.0.5.

There is a long lead time for servicing.

[JamesNK]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/8429820737