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

Thread.CurrentPrincipal does not flow with ExecutionContext #34489

Open
AndreasHeisel opened this Issue Jan 10, 2019 · 32 comments

Comments

7 participants
@AndreasHeisel
Copy link

AndreasHeisel commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 10, 2019

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

This comment has been minimized.

Copy link

AndreasHeisel commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Member

jkotas commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Member

jkotas commented Jan 10, 2019

@AndreasHeisel

This comment has been minimized.

Copy link

AndreasHeisel commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Collaborator

benaadams commented Jan 11, 2019

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 11, 2019

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 aspnet/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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 11, 2019

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

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 12, 2019

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

@MarcoRossignoli

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 12, 2019

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

@gulbanana

This comment has been minimized.

Copy link

gulbanana commented Jan 13, 2019

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

This comment has been minimized.

Copy link
Member

jkotas commented Jan 13, 2019

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

This comment has been minimized.

Copy link

gulbanana commented Jan 13, 2019

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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 13, 2019

@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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 13, 2019

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

This comment has been minimized.

Copy link
Member

jkotas commented Jan 13, 2019

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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 13, 2019

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

This comment has been minimized.

Copy link

AndreasHeisel commented Jan 14, 2019

@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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 14, 2019

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

@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jan 14, 2019

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

@gulbanana

This comment has been minimized.

Copy link

gulbanana commented Jan 15, 2019

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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 15, 2019

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

I asked because "it depends", after my recent experience 😄 #33645 (comment) #17164 (comment)

@blowdart

This comment has been minimized.

Copy link
Contributor

blowdart commented Jan 15, 2019

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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 15, 2019

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

@blowdart

This comment has been minimized.

Copy link
Contributor

blowdart commented Jan 15, 2019

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
aspnet/AspNetCore#481

I'd rather it threw to be honest.

@MarcoRossignoli

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 16, 2019

I'd rather it threw to be honest.

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

@blowdart

This comment has been minimized.

Copy link
Contributor

blowdart commented Jan 16, 2019

@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

This comment has been minimized.

Copy link
Member

jkotas commented Jan 16, 2019

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

This comment has been minimized.

Copy link
Collaborator

MarcoRossignoli commented Jan 16, 2019

ok thank's guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment