Thread.CurrentPrincipal does not flow with ExecutionContext

[AndreasHeisel]

We have a problem with System.Threading.ExecutionContext and System.Threading.Thread.CurrentPrincipal on .NET Core 2.1.

We want to implement a custom asynchronous point. According to docs.microsoft.com System.Threading.ExecutionContext is the way to go:

The ExecutionContext class provides the functionality for user code to capture and transfer this context across user-defined asynchronous points.

Test:

using System;
using System.Security.Claims;
using System.Threading;
using System.Threading.Tasks;

class Program
{
	static void Main()
	{
		var principal = new ClaimsPrincipal();

		Thread.CurrentPrincipal = principal;
		var context = ExecutionContext.Capture();
		Thread.CurrentPrincipal = null;

		ExecutionContext.Run
		(
			context,
			state =>
			{
				Console.WriteLine("Thread.CurrentPrincipal: {0}", Thread.CurrentPrincipal);
			},
			null
		);
	}
}

Using .NET Framework 4.7.1 I get:
Thread.CurrentPrincipal: System.Security.Claims.ClaimsPrincipal
Wich is the expected behavior, according to the documentation

The ExecutionContext class provides a single container for all information relevant to a logical thread of execution. This includes security context, call context, and synchronization context.

Using .NET Core 2.1 I get:
Thread.CurrentPrincipal:

I tested the behavior with the runtime provided asynchronoues point "await" and "Task.Run". In both cases it worked as expected.

Is this behavioral change intended?

[MarcoRossignoli]

I think so. AFAIK in .NET core context is a list of AsyncLocal<T> added through Value set
I don't see any code on Thread.CurrentPrincipal to flow it to context.
On full .NET Principal is added to context https://referencesource.microsoft.com/#mscorlib/system/threading/thread.cs,1369
Here one sample of AsyncLocal used for impersonation https://github.com/dotnet/corefx/blob/master/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs#L715
Maybe guide should be updated https://docs.microsoft.com/en-us/dotnet/api/system.threading.executioncontext?view=netcore-2.2#remarks

/cc @stephentoub

[MarcoRossignoli]

I tested the behavior with the runtime provided asynchronoues point "await" and "Task.Run". In both cases it worked as expected.

I did some test and on simple console app 2.2 principal seems not flow(maybe I misunderstood):

 internal class Program
    {
        private static async Task Main(string[] args)
        {
            await Context1();
            Console.WriteLine("Test end");
            Console.ReadKey();
        }

        public static async Task Context1()
        {
            ClaimsPrincipal principal = new ClaimsPrincipal();

            Debug.Assert(Thread.CurrentPrincipal == null);
            Thread.CurrentPrincipal = principal;
            Debug.Assert(Thread.CurrentPrincipal != null);
            Debug.Assert(Thread.CurrentPrincipal is ClaimsPrincipal);

            Console.WriteLine(Thread.CurrentPrincipal);
            Console.WriteLine(Thread.CurrentThread.ManagedThreadId);

            await Task.Run(async () => await Context2());
        }

        public static async Task Context2()
        {
            Debug.Assert(Thread.CurrentPrincipal == null); // empty Thread.CurrentPrincipal
            Console.WriteLine(Thread.CurrentPrincipal);
            Console.WriteLine(Thread.CurrentThread.ManagedThreadId);
        }
    }

[AndreasHeisel]

You are right, my tests were wrong. Await and Task.Run also doen't work on .NET Core.

This is a big issue for us, and it doesn't answer the question if it is a bug or a feature. Other contextual data like CurrentCulture and CurrentUICulture flow as they were ported to AsyncLocal.

[MarcoRossignoli]

I think this is a feature...context flow is a key piece of async/await machinery...however I cannot answer to this we need to wait team guys /cc @stephentoub @jkotas

[jkotas]

If the CurrentPrincipal was part of ExecutionContext on desktop, it makes sense to flow it in .NET Core as well for better compatibility.

It should be done lazily - create the AsyncLocal only once somebody actually sets the principal.

[jkotas]

cc @danmosemsft

[AndreasHeisel]

It's good to see this on the to do list.

@jkotas This is not only for completeness and compatibility. There is a real need for the feature. It enables authorisation downstream in an asynchronous path if you don't control all the code on the way.

[MarcoRossignoli]

I think this is a feature...context flow is a key piece of async/await machinery

I'm surprised that this issue doesn't come out before.

[benaadams]

cc @blowdart

[jkotas]

I'm surprised that this issue doesn't come out before.

It has come up a few times before, e.g. aspnet/Security#322 or dotnet/aspnetcore#481. Depending on ambient statics for security-related stuff is not very robust pattern as multiple people pointed out in the linked issues. It is easy to make a subtle mistake and get these ambient statics mixed up or escape unexpectedly.

[MarcoRossignoli]

Thank's Jan for pointing issues....my "surprise" come right from "web apps"

[jkotas]

@MarcoRossignoli Interested in grabbing this one? This one should be pretty straightforward and non-controversial.

[MarcoRossignoli]

Sure @jkotas I will grab this ASAP, thank you for your trust.

[gulbanana]

This change is a bit worrying. What if people have existing .NET Core ambient-security code which really does expect Thread.CurrentPrincipal to mean the thread's principal, rather than the principal of an execution context? This situation can happen when you dispatch work into some sort of pool of worker threads, or when you lazily set up per-thread security contexts.

[jkotas]

Right, any change has potential to break somebody. We try to make sure that benefits outweigh the risks.

@gulbanana Do you have a real example of a program that would be broken by this change?

This property is unlikely to be used in .NET Core code written from scratch. It is not used by ASP.NET Core. This property is most likely going to be used in code being ported from .NET Framework where it was flowing with the execution context.

[gulbanana]

I do work on a security framework which is used in (but predates) ASP.NET Core, and tries to do its own management of threading/async context. Going through the code, when operating with ambient sec it doesn't use that method - only stuff like WindowsUser.GetCurrent(). Someone else might have written similar code that happens to make use of the method in question, but I don't know of any that actually exists :)

[MarcoRossignoli]

@gulbanana when you say "thread's principal" are you meaning underneath OS principal object like windows user?
Guide says that Thread.CurrentPrincipal should be used for "Role based security" https://docs.microsoft.com/en-us/dotnet/api/system.threading.thread.currentprincipal?view=netcore-2.2. I mean also on netfx is not coupled with thread's principal.
AFAIK this object should rapresent "security logic object" of an app, so if we'are in an ambient coupled with OS security object the correct way should be flow also plat security token with impersonation.
(@jkotas default on full framework is empty GenericPrincipal but is null on .net core, maybe also this could/should be fixed. )

[MarcoRossignoli]

What if people have existing .NET Core ambient-security code which really does expect Thread.CurrentPrincipal to mean the thread's principal, rather than the principal of an execution context?

IMO the consequence of Thread.CurrentPrincipal use should be very clear 😃 and used/documented(app context) very well.

[jkotas]

default on full framework is empty GenericPrincipal but is null on .net core

The default principal policy on .NET Core is PrincipalPolicy.NoPrincipal. It is intentional. It is one-liner for the apps to change the default if they actually want a different default.

[MarcoRossignoli]

The default principal policy on .NET Core is PrincipalPolicy.NoPrincipal. It is intentional. It is one-liner for the apps to change the default if they actually want a different default.

I didn't know thank's!

[AndreasHeisel]

@gulbanana Thread.CurrentPrincipal is part of .NET standard. An API standard can only be useful, if it does not only define an API technically - in this case: "Has a property named CurrentPrincipal of type IPrincipal but also the semantics. So IMHO there are two options:

• Make it work (as it had worked for nearly two decades)
• Make it throw PlattformNotSupportedException

Bring the API but let it work different is a non-option with respect to the standard. If one want's to introduce new behavior, he should invent a new API like the identity team did with ClaimsPrincipal.Current.

[MarcoRossignoli]

@jkotas Can I go on with this or it needs of some kind of review?

[jkotas]

This looks like a pretty straightforward compat bug fix to me. I do not think this needs any special review.

[gulbanana]

the .net standard argument is pretty convincing. even if it's a change to the .net core behaviour, behaving differently on .net core and .net framework is worth fixing.

[MarcoRossignoli]

behaving differently on .net core and .net framework is worth fixing.

I asked because "it depends", after my recent experience 😄 dotnet/corefx#33645 (comment) https://github.com/dotnet/corefx/issues/17164#issuecomment-454143649

[blowdart]

ASP.NET Core is always going to consider anything .Current (including ClaimsPrincipal.Current) as an anti-pattern. We purposely avoided it, and we won't be setting this either :)

[MarcoRossignoli]

@blowdart do you have some security concern with this compat fix?(only to have double green light)

[blowdart]

It's already mentioned;

What if people have existing .NET Core ambient-security code which really does expect Thread.CurrentPrincipal to mean the thread's principal, rather than the principal of an execution context?

People have expectations of this, and they're usually wrong;

aspnet/Security#322
dotnet/aspnetcore#481

I'd rather it threw to be honest.

[MarcoRossignoli]

I'd rather it threw to be honest.

@jkotas @blowdart I don't have "double green light" 😅 thoughts?

[blowdart]

@MarcoRossignoli Sorry we had an internal discussion and I assumed one of the core engineering folks would respond. Don't let my griping stop fixing Thread.CurrentPrincipal. However .Impersonate is dead in the water, and I'll respond on that thread.

[jkotas]

Both me and @stephentoub have given green light to fixing Thread.CurrentPrincipal to be more compatible with .NET Framework in the discussion we had about this.

[MarcoRossignoli]

ok thank's guys!