Only call AddDataProtection in Authentication Services that require it

[eerhardt]

In .NET 8, we have a goal to enable JWT authentication with Native AOT. See Stage 2.a in #45910.

In order to use JWT authentication, the app needs to call builder.Services.AddAuthentication(). When bringing in AddAuthentication(), we are getting trimming / NativeAOT warnings from System.Security.Cryptography.Xml. System.Security.Cryptography.Xml is not currently trimming / NativeAOT compatible. See dotnet/runtime#73432. It also appears to be a major amount of work to make it compatible, possibly with many "gotchas".

The reason System.Security.Cryptography.Xml is brought into the app is because this line:

aspnetcore/src/Security/Authentication/Core/src/AuthenticationServiceCollectionExtensions.cs

Line 24 in 9c38b37

services.AddDataProtection();

DataProtection brings in the dependency on System.Security.Cryptography.Xml.

However, to enable JWT bearer authentication, it doesn't require DataProtection. Other types of authentication services do, for example:

• Cookies
• OpenIdConnect
• OAuth
• WsFederation
• Twitter
• etc

So it made sense originally to add DataProtection in a common place, and if the app didn't use it - no big deal. But now with NativeAOT and trimming, it does affect the app because the unused code can't be trimmed from the app - making it bigger unnecessarily.

To solve both the size issue (being able to trim the unused DataProtection code) and the fact that System.Security.Cryptography.Xml is not compatible with NativeAOT/trimming, we should remove AddDataProtection() from AddAuthentication() and instead move the calls to all the specific authentication services that require it.

Note that this would be a breaking change because an app could just call AddAuthentication(), without calling one of the built-in auth services, and then try to get DataProtection services, it will fail (since they aren't registered).

Alternatives

One alternative is to create a new AddAuthenticationCore() method that doesn't call AddDataProtection(), but does everything else AddAuthentication() does today.

cc @halter73 @davidfowl @JamesNK @DamianEdwards

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 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.

[Tratcher]

Yes, this would be breaking for most 3rd party auth providers.

AddAuthenticationCore would avoid the break, but would have to be used directly in program.cs and the template.

[eerhardt]

@Tratcher - do you have an opinion on which approach we should take?

[Tratcher]

AddAuthenticationCore has discoverability issues. How about this:

public static class AuthenticationServiceCollectionExtensions
{
    public static AuthenticationBuilder AddAuthentication(this IServiceCollection services)
    public static AuthenticationBuilder AddAuthentication(this IServiceCollection services, string defaultScheme)
    public static AuthenticationBuilder AddAuthentication(this IServiceCollection services, Action<AuthenticationOptions> configureOptions)
+   public static AuthenticationBuilder AddAuthentication(this IServiceCollection services, bool withDataProtection)
+   public static AuthenticationBuilder AddAuthentication(this IServiceCollection services, bool withDataProtection, string defaultScheme)
+   public static AuthenticationBuilder AddAuthentication(this IServiceCollection services, bool withDataProtection, Action<AuthenticationOptions> configureOptions)
}

I'd also set a bool on AuthenticationBuilder bool HasDataProtection { get; } that callers like AddOpenIdConnect would check and throw if it wasn't enabled. That way they fail at startup instead of on individual requests.

[eerhardt]

Unfortunately that proposal is not trim friendly. The trimmer is not able to see that withDataProtection is only ever passed false. See dotnet/linker#1868.

[Tratcher]

Darn. So we end up with:

public static class AuthenticationServiceCollectionExtensions
{
    public static AuthenticationBuilder AddAuthentication(this IServiceCollection services)
    public static AuthenticationBuilder AddAuthentication(this IServiceCollection services, string defaultScheme)
    public static AuthenticationBuilder AddAuthentication(this IServiceCollection services, Action<AuthenticationOptions> configureOptions)
+   public static AuthenticationBuilder AddAuthenticationCore(this IServiceCollection services)
+   public static AuthenticationBuilder AddAuthenticationCore(this IServiceCollection services, string defaultScheme)
+   public static AuthenticationBuilder AddAuthenticationCore(this IServiceCollection services, Action<AuthenticationOptions> configureOptions)
}

I'd still want that bool on AuthenticationBuilder bool HasDataProtection { get; } that callers like AddOpenIdConnect would check and throw if it wasn't enabled.

[eerhardt]

that callers like AddOpenIdConnect would check and throw if it wasn't enabled.

Why wouldn't AddOpenIdConnect just call AddDataProtection() if it needs it? Why would it require someone else to do it?

[Tratcher]

Good point 😆.

[davidfowl]

Unfortunately that proposal is not trim friendly. The trimmer is not able to see that withDataProtection is only ever passed false. See dotnet/linker#1868.

• It tightly couples the method to the implementation
• It doesn’t scale well for other things we may want to exclude

[eerhardt]

I just remembered there already is an AddAuthenticationCore method:

aspnetcore/src/Http/Authentication.Core/src/AuthenticationCoreServiceCollectionExtensions.cs

Line 19 in 3265dc6

public static IServiceCollection AddAuthenticationCore(this IServiceCollection services)

The issue with it and JwtBearer authentication is that JwtBearer also needs an ‘IAuthenticationConfigurationProvider’, which is only added by AddAuthentication.

So other options would be:

• move the default IAuthenticationConfigurationProvider service registration from AddAuthentication to AddAuthenticationCore.
• Have AddJwtBearer add the default IAuthenticationConfigurationProvider service if it hasn’t already been registered.

[danmoseley]

Aside - why do we consider System.Security.Cryptography.Xml legacy/not recommended given so many mainstream auth scenarios apparently depend on it?
Cc @vcsjones

[eerhardt]

why do we consider System.Security.Cryptography.Xml legacy/not recommended

@danmoseley - I’m not sure where you got that impression. Can you post a link?

The core of this issue is that:

1. The goal for .Net 8 is to enable JWT token auth.
2. JWT token auth doesn’t need System.Security.Cryptography.Xml
3. There’s currently no way to cut the System.Security.Cryptography.Xml code out.
4. System.Security.Cryptography.Xml has not been made compatible with trimming and native AOT. Because it is mostly by design not trim friendly. See Annotate System.Security.Cryptography.Xml for trimming runtime#73432 (comment)

[eerhardt]

why do we consider System.Security.Cryptography.Xml legacy/not recommended

Check out https://github.com/dotnet/runtime/tree/main/src/libraries/System.Security.Cryptography.Xml#readme

[JamesNK]

I'm guessing the problem with System.Security.Cryptography.Xml is there can be type names embedded in XML. For example, Type.GetType(typeNameFromXml) then is destined to fail.

DataProtection has the same kind of issues. The initial trimming annotations basically made the whole thing unsafe. Because of how fundamental DataProtection is, I refactored DataProtection annotations to a more pragmatic approach. The library works out of the box with all the applicable built-in .NET types. If there is a custom type defined in XML and it can't be found, then the error message mentions that it could have been trimmed. It didn't feel like a huge amount of work and it looks like it was successful. At least some people have been using trimming with it (they logged a bug about one place I missed 😬).

IMO System.Security.Cryptography.Xml should be annotated the same way. It works for the vast majority of people and for those doing custom things and AOT (another small number), then provide a good runtime error experience.

I brought this up before, but I think it is better to just fix the underlying cause here. At the very least, look at what is involved in fixing the underlying cause, and estimate its cost vs the cost of workaround it.

DataProtection isn't going anywhere, and for AOT to be a real thing for ASP.NET Core (i.e. generally work and not just a couple of scenarios), we'll need it to work.

[vcsjones]

Looks like @eerhardt found the most relevant link. A more opinionated answer is found here: dotnet/runtime#28599 (comment)

System.Security.Cryptography.Xml is in a state of "as dead as can be while still being technically supported". Further, the W3C shut down the XML Security WG in 2016.

I'm guessing the problem with System.Security.Cryptography.Xml is there can be type names embedded in XML

Kind of. S.S.C.Xml relies heavily on CryptoConfig, which basically maps URLs and URNs that are identifiers to types. CryptoConfig is known not to be trimming friendly:

https://github.com/dotnet/runtime/blob/ef15cfbbcebbfd8bcef2664bd6dad5c34ce66108/src/libraries/System.Security.Cryptography/src/System/Security/Cryptography/CryptoConfig.cs#L334

[davidfowl]

I brought this up before, but I think it is better to just fix the underlying cause here. At the very least, look at what is involved in fixing the underlying cause, and estimate its cost vs the cost of workaround it.
DataProtection isn't going anywhere, and for AOT to be a real thing for ASP.NET Core (i.e. generally work and not just a couple of scenarios), we'll need it to work.

Also, our new token story is currently dependent on data protection so...... +1 to what James said.

[DamianEdwards]

@JamesNK @davidfowl, to be clear, are you advocating we don't do this layering/dependencies fix for Data Protection and instead make Data Protection work for trimming/native AOT? In the case of JWT it still has size implications (JWT doesn't need it).

[JamesNK]

Yes. Put a pin on new work optimizing publish size and focus on AOT functionality.

[eerhardt]

Maybe another question to ask here (and maybe the one that @danmoseley is asking), why is DataProtection using a component that is in a state of "as dead as can be while still being technically supported"? Should we be re-thinking how DataProtection is designed?

[eerhardt]

At the very least, look at what is involved in fixing the underlying cause, and estimate its cost vs the cost of workaround it.

@jeffhandley - is it possible to get an estimate of its cost?

[eerhardt]

@JamesNK, I took a first crack at annotating the library:

dotnet/runtime@main...eerhardt:runtime:AnnotateSSCXml

Basically, almost all the APIs have added [RequiresUnreferencedCode] on them. This will let any developer who calls these APIs know that they may not (probably won't) work in a trimmed app.

There is more work to be done in that change:

• Make a good warning message
• Annotate Microsoft.Extensions.Configuration.Xml as RequiresUnreferencedCode, since it uses these APIs
• (potentially) update all the places that call CryptoHelpers.CreateFromName and throw an exception to say something like "if you are trimming an application, ensure the necessary algorithms are preserved in the app". Note there are a couple places that call CreateFromName, but just return false if null was returned. Those won't be possible to update an exception message.

What would you suggest to do with the warnings that will now be emitted from the ASP.NET code? For example:

aspnetcore/src/DataProtection/DataProtection/src/XmlEncryption/EncryptedXmlDecryptor.cs

Lines 46 to 68 in 0b10e13

	public XElement Decrypt(XElement encryptedElement)
	{
	ArgumentNullThrowHelper.ThrowIfNull(encryptedElement);
	// <EncryptedData Type="http://www.w3.org/2001/04/xmlenc#Element" xmlns="http://www.w3.org/2001/04/xmlenc#">
	// ...
	// </EncryptedData>
	// EncryptedXml works with XmlDocument, not XLinq. When we perform the conversion
	// we'll wrap the incoming element in a dummy <root /> element since encrypted XML
	// doesn't handle encrypting the root element all that well.
	var xmlDocument = new XmlDocument();
	xmlDocument.Load(new XElement("root", encryptedElement).CreateReader());
	// Perform the decryption and update the document in-place.
	var encryptedXml = new EncryptedXmlWithCertificateKeys(_options, xmlDocument);
	_decryptor.PerformPreDecryptionSetup(encryptedXml);
	encryptedXml.DecryptDocument();
	// Strip the <root /> element back off and convert the XmlDocument to an XElement.
	return XElement.Load(xmlDocument.DocumentElement!.FirstChild!.CreateNavigator()!.ReadSubtree());
	}

is now going to warn because it is using EncryptedXml.DecryptDocument() which is marked as RequiresUnreferencedCode. Are we just going to suppress these warnings in ASP.NET? It would either be that or re-mark AddDataProtection as RequiresUnreferencedCode again.

[JamesNK]

Marking AddDataProtection with RequiresUnreferencedCode isn't a good solution. It's used in so many places that huge parts of ASP.NET Core would also need to be annotated with RequiresUnreferencedCode: session, most (all?) of auth, Blazor, host builder (I believe the create default method uses it).

People have been using trimming with DataProtection in .NET 7. Whatever we do today works.

Can situations like the example below be turned into runtime errors? If DeformatterAlgorithm can't be found, then throw an error saying the name might be wrong or the type might have been trimmed.

#if NETCOREAPP
        [RequiresUnreferencedCode("CreateDeformatter is not trim compatible because the algorithm implementation referenced by DeformatterAlgorithm might be removed.")]
#endif
        public sealed override AsymmetricSignatureDeformatter CreateDeformatter(AsymmetricAlgorithm key)
        {
            var item = (AsymmetricSignatureDeformatter)CryptoConfig.CreateFromName(DeformatterAlgorithm!)!;
            item.SetKey(key);
            item.SetHashAlgorithm(DigestAlgorithm!);
            return item;
        }

This is what I did in DataProtection:

aspnetcore/src/DataProtection/DataProtection/src/TypeExtensions.cs

Lines 30 to 41 in 4190655

	[UnconditionalSuppressMessage("Trimmer", "IL2057", Justification = "Unknown type names are rarely used by apps. Handle trimmed types by providing a useful error message.")]
	public static Type GetTypeWithTrimFriendlyErrorMessage(string typeName)
	{
	try
	{
	return Type.GetType(typeName, throwOnError: true)!;
	}
	catch (TypeLoadException ex)
	{
	throw new InvalidOperationException($"Unable to load type '{typeName}'. If the app is published with trimming then this type may have been trimmed. Ensure the type's assembly is excluded from trimming.", ex);
	}
	}

I'd like a situation like DataProtection where all the built-in crypto types always work and then provide runtime errors if someone customizes the crypto types and they enable trimming.

[eerhardt]

People have been using trimming with DataProtection in .NET 7. Whatever we do today works.

We aren’t doing full trimming in ASP.NET apps. By default it uses “partial” in .NET 7.

Can situations like the example below be turned into runtime errors?

This isn’t the intent of how we are enabling AOT and trimming. If the apps behavior can change after trimming, our intention is to give the dev a warning to let them know. There are some situations where the behavior is edge case and it would be too hard (like the DI case), but here the whole library’s design is incompatible. So suppressing these warnings in dotnet/runtime isn’t going to be approved.

Would it be possible to remove EncryptedXml in DataProtection when in trimmed / NativeAOT? We could use a feature switch which devs could turn back on to add it back (with a single warning)? Or is EncryptedXml used too often for this approach to be feasible?

[davidfowl]

Would it be possible to remove EncryptedXml in DataProtection when in trimmed / NativeAOT? We could use a feature switch which devs could turn back on to add it back (with a single warning)? Or is EncryptedXml used too often for this approach to be feasible?

It's currently fundamental to the implementation. I agree with James that DataProtection needs to work with trimming. It seems like we need to take a step back and figure out exactly how much of this needs to work. This is one of the only subsystems that does things like "loads types from configuration", so it'll require more special treatment.

[eerhardt]

It seems like we need to take a step back and figure out exactly how much of this needs to work.

For our current goals for .NET 8, which is just JWT Bearer token authentication, none of it needs to work. The issue is that just calling AddAuthentication() pulls DataProtection in, and then you get warnings - in code your app doesn’t use.

[mcallaghan-geotab]

What is the workaround here? For non-AOT builds, that ONLY use JWT for authentication, on .NET8.

We have log spam with this warning

warn: Microsoft.AspNetCore.DataProtection.Repositories.FileSystemXmlRepository[60]
      Storing keys in a directory '/home/<user>/.aspnet/DataProtection-Keys' that may not be persisted outside of the container. Protected data will be unavailable when container is destroyed. For more information go to https://aka.ms/aspnet/dataprotectionwarning

[martincostello]

One workaround would be to set the log level of Microsoft.AspNetCore.DataProtection.Repositories.FileSystemXmlRepository to Error.

[mcallaghan-geotab]

One workaround would be to set the log level of Microsoft.AspNetCore.DataProtection.Repositories.FileSystemXmlRepository to Error.

True (definitely valid point) - that should work;

I was thinking about a mechanism to disable DataProtection entirely via an explicit call to AddDataProtection in DI with options to disable.

[eerhardt]

@amcasey - any thoughts on the above scenario and warning?

[amcasey]

I can't tell what the goal is. If you just don't want to get the log message, I'd agree with @martincostello that you can just disable it. If your goal is to make it not run, you'd have to experiment, since there's no designed/recommended way to do that. For example, you could register your own dummy repository and encryptor that no-op or throw, depending on your scenario. Alternatively, you might be able to go upstream and replace the data protection provider.

[mcallaghan-geotab]

I will ask plainly perhaps: why did we change the DI to add a dependency that spams the logs with a warning when configured using a very common authentication method (JwtBearer) when the log message isn't even applicable?

That warning message is very confusing and at first seems very concerning until we spend time to understand what is going on (since .NET8) and eventually find this upstream issue. Of course the warning MAY actually be important for other use cases (but it is difficult to know if it is relevant).

Hiding the log message is a workaround for most just to keep production systems ticking without raising false alarm bells in operations. (and it poses the risk that if some point in the future DataProtection is actually put into use (explicitly or implicitly) we wouldn't get any other warnings from that).

As I understand it, the purpose of this ticket is to come up with a solution that allows either better control over the DI (either implicit dependencies or configurability perhaps?).

[amcasey]

Sorry, @mcallaghan-geotab, I didn't mean to imply that your question was unclear - just that I was coming in late without full context. Thanks for clarifying!

That log message has been produced by data protection for many years, so something has probably changed in the way data protection is being configured. I think I understand from your description that your app worked properly (i.e. without logging) on a previous version of aspnetcore (6.0? 7.0?) and, after upgrading to 8.0 - without code changes - has started producing this confusing log message. Is that correct? If so, my guess would be that some change within 8.0, perhaps to improve AOT/trimming support - removed a data protection configuration call that used to make that log warning unnecessary (e.g. setting a different storage location).

To provide a little context, that log message is intended to be displayed (primarily) when the app is running in a docker container and the keys are stored in a location that won't survive container restarts (e.g. a directory that isn't mounted from the host).

[mcallaghan-geotab]

No problem @amcasey :) - yes you have roughly described the situation. To add:

• k8s application (yes runs in a docker container)
• using only JwtBearer authentication
• previously fine through .NET6, .NET7
• warning logs started after .NET8 upgrade

[amcasey]

So, from Data Protection's perspective, everything is (and was) working correctly - the app is running in a container and the storage folder isn't on a mounted file system, so it logs a warning. Obviously, it has no way to know that its keys will never actually be used.

That doesn't mean we can't improve the user experience though. I looked through some of the ways to explicitly configure data protection to not store keys in a persistent manner, but they basically exist as fallbacks, so they all log warnings. It's not hard to configure your own dummy storage system, but I agree that that shouldn't be necessary.

@eerhardt Do you already know what changed in 8.0 in this area? My guess would be that we used to explicitly configure some other xml repository and now we don't? Having said that, it's not really clear to me what that configuration could have been - where would you store the keys that you know, a priori, is persistent?

[amcasey]

@mcallaghan-geotab Would you be able to collect trace-level logs for Microsoft.AspNetCore.DataProtection from your pre-8.0 app? That would tell us where the keys are being stored.

[amcasey]

FWIW, this is the log statement.

[eerhardt]

@eerhardt Do you already know what changed in 8.0 in this area?

Not off the top of my head, no. #48082 is the change I made to make DataProtection as trim-compatible as I could.

[amcasey]

@eerhardt Do you already know what changed in 8.0 in this area?

Not off the top of my head, no. #48082 is the change I made to make DataProtection as trim-compatible as I could.

Those look like changes to data protection and they look fine. I was wondering about changes to how data protection is called.

[eerhardt]

I was wondering about changes to how data protection is called.

I didn't make any changes like that in .NET 8. I wanted to (which is why I opened this issue), but we never agreed on how to design it.

[amcasey]

Yeah, the current state seems expected to me - it's the fact that there was apparently no log message in 7.0 that seems strange. This seems like the next step, if you can spare the time, @mcallaghan-geotab.

[mcallaghan-geotab]

@mcallaghan-geotab Would you be able to collect trace-level logs for Microsoft.AspNetCore.DataProtection from your pre-8.0 app? That would tell us where the keys are being stored.

we're not storing any keys for anything (I dug deeply to confirm this - which led to the conclusion that we can omit/hide the warning logs for now)

Correction: It IS possible actually that the logs started in .NET7; (several software systems at play)

[amcasey]

we're not storing any keys for anything (I dug deeply to confirm this - which led to the conclusion that we can omit/hide the warning logs for now)

I think it will generate one on your behalf and log where it puts it, but I'm not certain.

Correction: It IS possible actually that the logs started in .NET7; (several software systems at play)

No worries - apples to apples comparisons across upgrades are virtually impossible.

If I can summarize then: your user experience is suboptimal but probably not a regression and you have a workaround? This issue tracks making a real fix, which will probably involve making it possible to omit data protection entirely in scenarios like this.

Assuming I've got that correct, can you please thumbs-up the description of this issue? That makes it easier for us to assess the user impact of issues like these and prioritize them appropriately.

Thanks for your patience and sorry for the wasted time!

[amcasey]

I've seen this question several times, so we should revisit it during 10 planning.