Add asynchronous overload of WindowsIdentity.RunImpersonated

[Tratcher]

https://github.com/dotnet/corefx/issues/9996#issuecomment-307870746
dotnet/aspnetcore#1805 (comment)

ASP.NET Core is getting many requests for users that want to impersonate the authenticated user of the request and perform actions on their behalf. The main problem is that many of these actions are asynchronous, such as database access, but RunImpersonated is synchronous. Users have repeatedly made the mistake of starting an asynchronous operation within RunImpersonated and then returning the Task. Exiting RunImpersonated revokes the impersonation and can cause errors or reverting to the base identity in the Task.

Proposal:
public static Task RunImpersonatedAsync(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task> func)
public static Task<T> RunImpersonatedAsync<T>(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task<T>> func)

@brentschmaltz

[stephentoub]

Note that ExecutionContext in .NET Core does not flow impersonation. The requested functionality could hopefully be achieved with an AsyncLocal and a valueChangedHandler that took care of impersonating and reverting the impersonation, but it would need to be prototyped.
cc: @kouvel

[madrianr]

Is it true that we cannot expect such a feature in a short period of time?
can you give us a timeframe?

robert

[brentschmaltz]

@Tratcher @stephentoub @madrianr are we discussing Integrated Windows Auth (IWA) ?

[Tratcher]

Primarily Kerberos for the delegation capabilities.

[brentschmaltz]

@Tratcher in other runtimes, we set the thread identity and ran the entire call graph under that identity. Then when exiting that call, we reverted the threads identity.
Don't know if that model works in core.

[Tratcher]

How did that flow to the async actions?

[brentschmaltz]

@Tratcher within a process (or appdomain) the new thread has the identity associated with the thread that spawned it. Cross thread or appdomain requires serializing. Cross process shouldn't be used because the NTTOKEN is just a handle and the values are only unique within a process. A windowsIdentity hydrated could point a different user. Very unlikely as the handle would have to be a handle to an nttoken.

[Tratcher]

And threadpool threads? That's where most of this code runs.

[lakeman]

ExecutionContext in dotnet core does seem to flow Impersonation (#25627). ExecutionContext also does not have an explicit lifetime, while token handle's do.

Instead of adding a RunImpersonatedAsync method, I'd like to propose an alternative api;

public IDisposable BeginImpersonation(SafeAccessTokenHandle token)

Which would internally still use an AsyncLocal & ExecutionContext to flow the impersonation across async continuations. While also ensuring that the impersonation cannot outlive the lifetime of the returned IDisposable.

Explicitly documenting that any task that escapes the lifetime of the disposable will still execute, but no longer be impersonating that user.

[stephentoub]

ExecutionContext in dotnet core does seem to flow Impersonation

It doesn't, at least not explicitly, i.e. there's no SecurityContext stored on the ExecutionContext as there is in netfx. But (and I didn't realize this was already done, hence my earlier comment), it appears it's trying to simulate it using AsyncLocal, similar to what I suggested earlier:
https://github.com/dotnet/corefx/blob/9a2e5d8f0e7f816f983bce75f5bba22811df4994/src/System.Security.Principal.Windows/src/System/Security/Principal/WindowsIdentity.cs#L640

[bartonjs]

Is this a "this works on NetFx but is broken on CoreFx", or a "now that async is so easy we see a feature hole"?

[Tratcher]

NetFx had Impersonate() and would flow the impersonated identity across async operations via the ExecutionContext (?). CoreFx doesn't have this API or implicit flow.

[Tratcher]

ping people keep doing this wrong and are set up for failure.
https://github.com/lilpug/ASP.NET-Core-Impersonation/blob/96917b51c578aa7d18c9636a4a58f4571822890f/lib/middleware/pipeline.cs#L46-L49

[wpbrown]

NetFx had Impersonate() and would flow the impersonated identity across async operations via the ExecutionContext (?). CoreFx doesn't have this API or implicit flow.

As @lakeman said, it seems CoreFX is flowing the impersonation. RunImpersonated creates an ExecutionContext and sets that AsyncLocal s_currentImpersonatedToken from within. It then uses the AsyncLocal ThreadContextChanged event to maintain the correct impersonation on the threads the task runs on.

What would an async verions of RunImpersonatedInternal do differently? It could potentially await the Action, but currently nothing happens after the action is called.

Instead of adding a RunImpersonatedAsync method, I'd like to propose an alternative api;

Wouldn't BeginImpersonation essentially just be Impersonate from NetFX? Why wasn't it brought over?

Perhaps @Tratcher's suggestion will provide more implementation flexibility for corefx devs in the future to manage the SafeAccessToken lifetime more appropriately without holding it in an AsyncLocal.

Below is a quick program to demonstrate the impersonation 'flowing' on Tasks returned from RunImpersonated:

using Microsoft.Win32.SafeHandles;
using System;
using System.Diagnostics;
using System.Linq;
using System.Runtime.InteropServices;
using System.Security.Principal;
using System.Threading;
using System.Threading.Tasks;

namespace TestImpersonate
{
    class Program
    {
        [DllImport("advapi32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
        public static extern bool LogonUser(String lpszUsername, String lpszDomain, String lpszPassword,
            int dwLogonType, int dwLogonProvider, out SafeAccessTokenHandle phToken);

        const int LOGON32_PROVIDER_DEFAULT = 0;
        const int LOGON32_LOGON_NETWORK = 3;
        static readonly ThreadLocal<Random> random = new ThreadLocal<Random>(() => new Random(Thread.CurrentThread.ManagedThreadId));

        static void Main(string[] args)
        {
            string user = Environment.UserName;
            LogonUser("test1", "localhost", "test1pass", LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT, out SafeAccessTokenHandle ath1);
            LogonUser("test2", "localhost", "test2pass", LOGON32_LOGON_NETWORK, LOGON32_PROVIDER_DEFAULT, out SafeAccessTokenHandle ath2);
            Console.WriteLine($"Main Begin: {Environment.UserName}");
            var tasks = Enumerable.Range(1, 50).Select(x => {
                var token = x % 2 == 0 ? ath1 : ath2;
                var name = x % 2 == 0 ? "test1" : "test2";
                return WindowsIdentity.RunImpersonated(token, async () => await TestRun(x, name));
            });
            Task.Delay(500).ContinueWith(async t => await TestRun(0, user));
            Task.WaitAll(tasks.ToArray());
            Console.WriteLine($"Main End: {Environment.UserName}");
        }

        static async Task TestRun(int test, string expectUser)
        {
            TestUser(test, expectUser);
            Thread.Sleep(random.Value.Next(5, 50));
            await Task.Delay(random.Value.Next(5,100));
            TestUser(test, expectUser);
            Thread.Sleep(random.Value.Next(5, 50));
            await Task.Delay(random.Value.Next(5,50));
            TestUser(test, expectUser);
        }

        static void TestUser(int test, string expectUser)
        {
            var actualUser = Environment.UserName;
            Console.WriteLine($"[{test}] Actual: {actualUser} Expect: {expectUser} Thread: {Thread.CurrentThread.ManagedThreadId}");
            Trace.Assert(actualUser == expectUser);
        }
    }
}

Program Output

Main Begin: will
[1] Actual: test2 Expect: test2 Thread: 1
[2] Actual: test1 Expect: test1 Thread: 1
[1] Actual: test2 Expect: test2 Thread: 4
[3] Actual: test2 Expect: test2 Thread: 1
[4] Actual: test1 Expect: test1 Thread: 1
[5] Actual: test2 Expect: test2 Thread: 1
[6] Actual: test1 Expect: test1 Thread: 1
[7] Actual: test2 Expect: test2 Thread: 1
[3] Actual: test2 Expect: test2 Thread: 5
[2] Actual: test1 Expect: test1 Thread: 4
[1] Actual: test2 Expect: test2 Thread: 6
[8] Actual: test1 Expect: test1 Thread: 1
[6] Actual: test1 Expect: test1 Thread: 7
[9] Actual: test2 Expect: test2 Thread: 1
[3] Actual: test2 Expect: test2 Thread: 6
[5] Actual: test2 Expect: test2 Thread: 9
[10] Actual: test1 Expect: test1 Thread: 1
[2] Actual: test1 Expect: test1 Thread: 7
[4] Actual: test1 Expect: test1 Thread: 8
[6] Actual: test1 Expect: test1 Thread: 9
[11] Actual: test2 Expect: test2 Thread: 1
[5] Actual: test2 Expect: test2 Thread: 4
[7] Actual: test2 Expect: test2 Thread: 6
[8] Actual: test1 Expect: test1 Thread: 5
[9] Actual: test2 Expect: test2 Thread: 8
[4] Actual: test1 Expect: test1 Thread: 4
[12] Actual: test1 Expect: test1 Thread: 1
[13] Actual: test2 Expect: test2 Thread: 1
[11] Actual: test2 Expect: test2 Thread: 4
[9] Actual: test2 Expect: test2 Thread: 6
[10] Actual: test1 Expect: test1 Thread: 4
[8] Actual: test1 Expect: test1 Thread: 5
[11] Actual: test2 Expect: test2 Thread: 5
[7] Actual: test2 Expect: test2 Thread: 6
[14] Actual: test1 Expect: test1 Thread: 1
[12] Actual: test1 Expect: test1 Thread: 8
[13] Actual: test2 Expect: test2 Thread: 4
[10] Actual: test1 Expect: test1 Thread: 5
[15] Actual: test2 Expect: test2 Thread: 1
[0] Actual: will Expect: will Thread: 6
[12] Actual: test1 Expect: test1 Thread: 8
[16] Actual: test1 Expect: test1 Thread: 1
[14] Actual: test1 Expect: test1 Thread: 9
[13] Actual: test2 Expect: test2 Thread: 8
[17] Actual: test2 Expect: test2 Thread: 1
[14] Actual: test1 Expect: test1 Thread: 5
[18] Actual: test1 Expect: test1 Thread: 1
[17] Actual: test2 Expect: test2 Thread: 5
[0] Actual: will Expect: will Thread: 7
[15] Actual: test2 Expect: test2 Thread: 6
[16] Actual: test1 Expect: test1 Thread: 5
[19] Actual: test2 Expect: test2 Thread: 1
[0] Actual: will Expect: will Thread: 6
[17] Actual: test2 Expect: test2 Thread: 6
[20] Actual: test1 Expect: test1 Thread: 1
[19] Actual: test2 Expect: test2 Thread: 5
[16] Actual: test1 Expect: test1 Thread: 6
[15] Actual: test2 Expect: test2 Thread: 6
[21] Actual: test2 Expect: test2 Thread: 1
[18] Actual: test1 Expect: test1 Thread: 4
[22] Actual: test1 Expect: test1 Thread: 1
[19] Actual: test2 Expect: test2 Thread: 7
[20] Actual: test1 Expect: test1 Thread: 4
[23] Actual: test2 Expect: test2 Thread: 1
[18] Actual: test1 Expect: test1 Thread: 7
[24] Actual: test1 Expect: test1 Thread: 1
[22] Actual: test1 Expect: test1 Thread: 5
[20] Actual: test1 Expect: test1 Thread: 6
[25] Actual: test2 Expect: test2 Thread: 1
[21] Actual: test2 Expect: test2 Thread: 8
[26] Actual: test1 Expect: test1 Thread: 1
[23] Actual: test2 Expect: test2 Thread: 8
[27] Actual: test2 Expect: test2 Thread: 1
[22] Actual: test1 Expect: test1 Thread: 9
[24] Actual: test1 Expect: test1 Thread: 6
[25] Actual: test2 Expect: test2 Thread: 7
[28] Actual: test1 Expect: test1 Thread: 1
[21] Actual: test2 Expect: test2 Thread: 9
[26] Actual: test1 Expect: test1 Thread: 11
[29] Actual: test2 Expect: test2 Thread: 1
[23] Actual: test2 Expect: test2 Thread: 11
[26] Actual: test1 Expect: test1 Thread: 5
[28] Actual: test1 Expect: test1 Thread: 7
[25] Actual: test2 Expect: test2 Thread: 9
[24] Actual: test1 Expect: test1 Thread: 11
[30] Actual: test1 Expect: test1 Thread: 1
[27] Actual: test2 Expect: test2 Thread: 7
[29] Actual: test2 Expect: test2 Thread: 5
[31] Actual: test2 Expect: test2 Thread: 1
[28] Actual: test1 Expect: test1 Thread: 5
[29] Actual: test2 Expect: test2 Thread: 7
[32] Actual: test1 Expect: test1 Thread: 1
[31] Actual: test2 Expect: test2 Thread: 5
[33] Actual: test2 Expect: test2 Thread: 1
[30] Actual: test1 Expect: test1 Thread: 8
[27] Actual: test2 Expect: test2 Thread: 6
[34] Actual: test1 Expect: test1 Thread: 1
[31] Actual: test2 Expect: test2 Thread: 8
[32] Actual: test1 Expect: test1 Thread: 5
[35] Actual: test2 Expect: test2 Thread: 1
[33] Actual: test2 Expect: test2 Thread: 7
[30] Actual: test1 Expect: test1 Thread: 6
[34] Actual: test1 Expect: test1 Thread: 6
[36] Actual: test1 Expect: test1 Thread: 1
[32] Actual: test1 Expect: test1 Thread: 4
[33] Actual: test2 Expect: test2 Thread: 4
[37] Actual: test2 Expect: test2 Thread: 1
[38] Actual: test1 Expect: test1 Thread: 1
[36] Actual: test1 Expect: test1 Thread: 8
[34] Actual: test1 Expect: test1 Thread: 9
[39] Actual: test2 Expect: test2 Thread: 1
[37] Actual: test2 Expect: test2 Thread: 8
[40] Actual: test1 Expect: test1 Thread: 1
[35] Actual: test2 Expect: test2 Thread: 7
[36] Actual: test1 Expect: test1 Thread: 9
[37] Actual: test2 Expect: test2 Thread: 8
[35] Actual: test2 Expect: test2 Thread: 8
[41] Actual: test2 Expect: test2 Thread: 1
[40] Actual: test1 Expect: test1 Thread: 5
[39] Actual: test2 Expect: test2 Thread: 7
[38] Actual: test1 Expect: test1 Thread: 11
[42] Actual: test1 Expect: test1 Thread: 1
[43] Actual: test2 Expect: test2 Thread: 1
[44] Actual: test1 Expect: test1 Thread: 1
[40] Actual: test1 Expect: test1 Thread: 8
[41] Actual: test2 Expect: test2 Thread: 8
[45] Actual: test2 Expect: test2 Thread: 1
[42] Actual: test1 Expect: test1 Thread: 7
[44] Actual: test1 Expect: test1 Thread: 4
[43] Actual: test2 Expect: test2 Thread: 5
[38] Actual: test1 Expect: test1 Thread: 8
[39] Actual: test2 Expect: test2 Thread: 9
[46] Actual: test1 Expect: test1 Thread: 1
[43] Actual: test2 Expect: test2 Thread: 4
[41] Actual: test2 Expect: test2 Thread: 7
[47] Actual: test2 Expect: test2 Thread: 1
[42] Actual: test1 Expect: test1 Thread: 4
[44] Actual: test1 Expect: test1 Thread: 8
[48] Actual: test1 Expect: test1 Thread: 1
[47] Actual: test2 Expect: test2 Thread: 4
[45] Actual: test2 Expect: test2 Thread: 7
[49] Actual: test2 Expect: test2 Thread: 1
[47] Actual: test2 Expect: test2 Thread: 7
[46] Actual: test1 Expect: test1 Thread: 9
[48] Actual: test1 Expect: test1 Thread: 4
[50] Actual: test1 Expect: test1 Thread: 1
[45] Actual: test2 Expect: test2 Thread: 7
[46] Actual: test1 Expect: test1 Thread: 7
[50] Actual: test1 Expect: test1 Thread: 4
[49] Actual: test2 Expect: test2 Thread: 7
[48] Actual: test1 Expect: test1 Thread: 5
[50] Actual: test1 Expect: test1 Thread: 7
[49] Actual: test2 Expect: test2 Thread: 7
Main End: will

[mattjohnsonpint]

FYI, users of my SimpleImpersonation library are also encountering this, as I use WindowsIdentity.RunImpersonated internally (paired with a P/Invoke to LogonUser for the value add, similar to the above code). A recommended workaround would be useful, especially if I could apply it without changing my public API. Thanks.

[mattjohnsonpint]

@wpbrown - I'm confused a bit from your program output. All the actuals match all the expecteds. Were you trying to show where it fails? If so, I don't see it.

[wpbrown]

@mj1856 I'm trying to show that it doesn't fail. I'm curious how others are making it fail?

I'm having a number of problems actually using impersonation, but the impersonation token flowing to the thread executing a task created with the existing method isn't one. I believe the assertion that there is no "implicit flow" isn't currently true. I'm still trying to figure out if there is a good existing issue to share the other problems with impersonation in CoreFX that I don't see in NetFX.

[lakeman]

Database connection pooling & impersonation is what lead me down this rabbit hole (see dotnet/corefx#25477 which was very difficult to diagnose, tracked down to dotnet/SqlClient#60 which I mentioned earlier)

[mattjohnsonpint]

Just to be clear, only one user of my library has reported impersonation failures with async code. They said it works fine for some time, and then fails intermittently after about 5 minutes of activity. I have not been able to reproduce the failure.

[mravko]

It's been an year. Is there any progress with this, I see it is still open. Should we expect it soon, or should we go for another approach for solving async calls to db under impersonated context?

[brentschmaltz]

@mravko interesting, just right now I have been asked to cost this out.
We will have an answer soon.

[clairernovotny]

Seems like this is relevant with WinForms/WPF apps on .NET Core 3, many of them which may need to call services async using windows auth?

[brentschmaltz]

@onovotny the code seems simple, there may be more to it. Thanks for the mention of WinForms/WPF as what I need to understand is the scenario where we need the identity to flow within an async pattern.

Any other pattern?

[lakeman]

Also intranet web apps connecting to an internal database as the end user.

My main concern with the current implementation is that this "works" (or at least fails gracefully at some point);

WindowsIdentity.RunImpersonated(token, () =>token.Close());

But this hits Environment.FailFast and crashes the process;

await WindowsIdentity.RunImpersonated(token, async () =>
    {
        await Task.Yield();
        token.Close();
    }
);

Can we implement an Async API so that failure exceptions propagate somewhere sane so they can be handled?

[fredrikhr]

So, since I need WindowsImpersonation (in some special cases) in an ASP.NET Core application, so I have been trying to come up with a good design that would allow for async impersonation.

First of all we'd need to do some AsyncLocal to store the identity to impersonate. And the asynclocal should be initialized with a callback that re-establishes the identity impersonation on context change. How? Is setting the current Thread principal enough? Or should I just call impersonate again, and keep a list of impersonations that are all going to get disposed at the end of the operation?

AFAIK impersonation is done on per-thread-basis on Windows, correct? What happens if async execution that is doing impersonation yields its thread execution to a task that is not doing impersonation. Since the other task has no knowledge (and no AsyncLocal) of the impersonation, wouldn't it now still run under the impersonated context?

What if I create a custom TaskScheduler class that keeps a dedicated pool of threads. When the impersonation starts, I can schedule the execution delegate to run on a task off that custom impersonation TaskScheduler. All async continuations within that task should now also be scheduled off that custom schedule instead of the default one, right? Wouldn't that effectively separate the impersonated tasks from the default (non-impersonated ones)? And because all tasks within that custom task scheduler would orginally be started with an AsyncLocal that re-establishes the impersonation, the impersonated principal could not leak to other async tasks?

[wpbrown]

@couven92 You should test out your scenario, because there already is an AysncLocal that does restore the impersonation on thread changes. You can try running the code in my comment above to see async impersonation working. Under this issue there are some edge cases that need investigation, but for your use it may already be sufficient.

[brentschmaltz]

@couven92 you are correct that a thread running on windows contains an 'windows identity'. This identity governs what 'windows protected resources' that thread can access. For example: files, database connections, etc.
This is separate from any 'user-mode' identities that are built from OIDC (id_token), etc.

What you say makes sense to me in that WindowsIdentity.RunImpersontedAsync by itself doesn't make much sense.

I would expect that when yielding to an different thread, the 'yielded' thread would NOT change it's identity. I am not sure how to think about a thread reading a file at that time should work. Say the file is already open.

[fredrikhr]

@wpbrown @brentschmaltz I have implemented my own (very simple) custom TaskScheduler now to run async impersonated tasks on dedicated threads. I'll do some more testing and report my findings here, but thus far it looks like that aproach does not leak impersonated principals.

@brentschmaltz As far as I have seen, the permissions for File Access are only checked/enforced when the FileHandle is first created. If you have an open File Handle to do I/O operations with, changes in the Thread (or even process) principal should not affect access rights. (Doing so would enormously expensive, that's why you have the handle in the first place)

With my own custom task scheduler I have thought of a possible issue, though: The custom task scheduler does not actually prevent the leakage of an impersonated principal. It just assumes every scheduled task it executes uses an AsyncLocal that re-establishes the impersonation context. So, if you were to schedule a non-impersonating task on my custom scheduler you'd have the exact same issue as with the default task scheduler. I made my custom scheduler internal, so that no one else can schedule non-impersonating tasks on it by accident.

You can take a look at THNETII.Security.WindowsImpersonation if you want to see how I did this.

[avivanoff]

FYI, users of my SimpleImpersonation library are also encountering this, as I use WindowsIdentity.RunImpersonated internally (paired with a P/Invoke to LogonUser for the value add, similar to the above code). A recommended workaround would be useful, especially if I could apply it without changing my public API. Thanks.

Just to be clear, only one user of my library has reported impersonation failures with async code. They said it works fine for some time, and then fails intermittently after about 5 minutes of activity. I have not been able to reproduce the failure.

@mj1856 I can easily reproduce this. See aspnet/AspNetWebStack#260. I was hoping @stephentoub can tell us what we are doing wrong.

[mattjohnsonpint]

@avivanoff - That repro is using identity.Impersonate(), not RunImpersonated.

[davidfowl]

Point of this is: If you have an asynchronous RunImpersonated call in one call path in your application, you'll now suddenly have to guard every asynchronous code path that might be sensitive to an OS access token.

I don’t understand this reasoning. The above logic you have seems logical. Outside of the RunImpersonated calls things are as they were before running impersonation.

What are you trying to work around or enable? What code do you want to write in an ideal world?

[fredrikhr]

@davidfowl

I don’t understand this reasoning. The above logic you have seems logical. Outside of the RunImpersonated calls things are as they were before running impersonation.

How? If main (0) first runs code path 1, then 2 and then 3. And each of the code paths start their respective tasks, then when task 3 continues after it has yielded (e.g. because of I/O), it might get the access token from one of the other task continuations, or the one from main if you're lucky.

Because, task 3 would not know what access token to restore to, right? Without the call to RunImpersonated, there is no async local in its sync context that would restore the access token, so it does not restore anything, and thus just keeps continuing with the access token from the previous task execution.

What are you trying to work around or enable? What code do you want to write in an ideal world?

An ASP.NET website that supports Windows Authentication. For one request path, e.g. /files, it like to call RunImpersonated, and call app.UseFileServer(). By that I hope achieve that different users will have access to different files, as controlled by the ACL of the file system. The rest of the website does not need Impersonation, however, so for all other request paths, I'd not need RunImpersonated, and use Application defined Authorisation instead.

[davidfowl]

How? If main (0) first runs code path 1, then 2 and then 3. And each of the code paths start their respective tasks, then when task 3 continues after it has yielded (e.g. because of I/O), it might get the access token from one of the other task continuations, or the one from main if you're lucky.

No, that's not how it works. Nothing bleeds outside of the RunImpersonated call.

Because, task 3 would not know what access token to restore to, right? Without the call to RunImpersonated, there is no async local in its sync context that would restore the access token, so it does not restore anything, and thus just keeps continuing with the access token from the previous task execution.

Not the previous task, the caller's ExecutionContext, in the case of task 3, Main's context.

[avivanoff]

What do you mean “nothing bleeds outside”? You just bled out a Task.

So, I would like to repeat my questions:

1. Documentation does not say anything about RunImpersonated being different between full framework and core.
2. I was thought that asynchronous code cannot be used like this. See my try/catch example above. So I need to understand why RunImpersonated is an exception.

[fredrikhr]

@davidfowl Where would the access token in main's context come from? Without main ever calling RunImpersonated, how would its access token ever end up in main's context? Or does the runtime actually store the initial process access token at startup in main's context? If that is so, what if you changed the principal identity of the main thread (through other means than RunImpersonated)?

[davidfowl]

@avivanoff

What do you mean “nothing bleeds outside”? You just bled out a Task.

The async local was set in the callback, the callback returned a Task as a result, any future async thread hops do the right thing. The caller's ExecutionContext doesn't change as RunImpersonated preserves the caller's execution context and restores it before the method exits.

In asynchronous methods, when you await a task, the ExecutionContext is preserved before the await and restored after the continuation runs. Since the ExecutionContext wasn't modified, it shouldn't affect future calls on that Task's continuation.

the TL;DR would be, the intent is that code outside the RunImpersonated isn't impersonated.

Documentation does not say anything about RunImpersonated being different between full framework and core.

I'm sure the intent was to make it behave the same. The difference might be bug fixes and cadence at which .NET Core can update vs .NET Framework.

I was thought that asynchronous code cannot be used like this. See my try/catch example above. So I need to understand why RunImpersonated is an exception.

I don't know what you mean. Task.Run and WindowsIdentity.RunImpersonated are both examples of methods that take callbacks but they do completely different things. I could tell you to look at the implementation of each but I'm not exactly sure what you're looking for.

Here's an example of what RunImpersonated does and maybe it'll help clear up some syntax things:

public T MyFunction<T>(Func<T> callback)
{
       return callback();
}

public async Task Main()
{
    Task task = MyFunction(async () => {  await Task.Delay(1000);  });
    await task;
}

The generic function is returning any T and in this case T is just a Task so I can pass an async delegate which ends up being Func<Task>. Hope that helps.

[weltkante]

I am looking for an explanation why is it ok to run asynchronous code in synchronous context.

Synchronous code that gets passed an asynchronous delegate can still observe when the delegate enters/leaves execution, by installing an AsyncLocal to track control flow.

You might want to review how AsyncLocal works, in particular when it has a callback registered the callback will be notified whenever control flow enters/leaves the code. This allows to remove impersonation when leaving the delegate due to an await, and restore the impersonation once control flow enters again when the await is resumed. This is very different from control flow with exceptions, exceptions are desired to be propagated up to the caller/awaiter while ExecutionContext is used for isolating a delegate (or rather, its AsyncLocal values) from its caller.

Note that the implementation using AsyncLocal is specific to .NET Core, Desktop Framework uses an entirely different implementation for tracking when control flow enters/leaves code (on the lowest level its also based on ExecutionContext but uses a lot of hardwired classes and logic which doesn't exist in .NET Core)

So while the documentation doesn't point you to the implementation details of how it works, any bug reports necessarily have to be examined in context of the framework where they are observed, since the implementation how impersonation is flown is entirely different.

[fredrikhr]

Back to my diagram, assume this sequence of events:

0: main (running as a Service identity)
├ 1: RunImpersonated(Some unprivileged user)
|    ├ 1.1: Async operation
|    └ 1.2: Async operation
├ 2: RunImpersonated(Administrator)
|    ├ 2.1: Async operation
|    ├ 2.2: Async operation
└ 3: Async operation (without call to RunImpersonated)

1. main starts. There is no access token in its execution context. The thread identity is equal to the process identity, which for this purpose is a service idenity (e.g. LOCAL SERVICE).
2. main branches to code path 1. RunImpersonated is called. it pushes the access token of the unprivileged user into the execution context and invokes the callback parameter, branch 1.1.
3. code path 1.1 executes under the impersonated identity of the unprivileged user. The code path start an asynchronous execution (e.g. await Task.Delay()). The async continuation of 1.1 is scheduled, and 1.1 returns with the return value being a Task representing the furture continuation of execution.
4. as the invoked code path from step 2 has returned, RunImpersonated restores the thread identity to main's original identity, i.e. the service identity.
5. The same steps repeat for code branch 2 with the identity now being the Administrator.
6. code branch 3 is executed. Once again, the thread idenity has been restored to the service idenity. I.e. there is no impersonation going on here, there is therefore no access token to restore in the execution context. Similar to step 3, an asynchronous operation is started, and its continuation is scheduled.
7. Asynchronous operation 1.1 completes. Initially, the thread idenity is now still main's origin identity. But as the task is resumed, the impersonation token from branch 1, i.e. the unprivileged user token, is restored from the execution context of the async operation. The continuation resumes with the thread identity being that of the unprivileged user.
8. Async op 2.1 completes. Initially, the thread identity is now the one from setp 7, i.e. the access token of the unprivileged user. The task is resumed, the access token from branch 2 is restored, the thread idenity is thus now the Administrator.
9. Async op 3 completes. Again, its thread identity is now the Administrator. When 3 was originally started, there was no Impersonation going on, therefore there is no access token in the execution context to restore. Therefore, the continuation of task 3 resumes in the context of the Administrator.

This comes from the fact that AsyncLocal is only restored on continuation, and its applied effects are not reversed on yielding.

The on-continue action of the AsyncLocal is applied in steps 7 and 8, because RunImpersonated pushed an AsyncLocal into the task's respective execution contexts. In step 9 however, there is no AsyncLocal in the execution context, as RunImpersonation never was called in this code path. Therefore, there is no on-continue action being called, and therefore whatever access token that was applied in the step before remains applied.

@davidfowl Where am I wrong?

[weltkante]

This comes from the fact that AsyncLocal is only restored on continuation, and its applied effects are not reversed on yielding.

You are wrong here, superficially described AsyncLocal value switches when something else executes on the thread. It doesn't matter whether you enter or leave an ExecutionContext, as soon as it changes the callback is invoked.

Creating a dummy AsyncLocal just for tracking the events is enlightening here. Make sure you experiment both with changing the AsyncLocals value on an existing ExecutionContext, and creating an isolated ExecutionContext like RunImpersonated does (both behave differently, mostly because the isolated ExecutionContext is not shared with the caller, so it prevents leaking AsyncLocal values)

In step 9 however, there is no AsyncLocal in the execution context, as RunImpersonation never was called in this code path. Therefore, there is no on-continue action being called

When you leave step 8 you leave the ExecutionContext, the AsyncLocal switches to NULL, the callback is invoked. This switch is hidden in the async/await machinery and not visible in plain sight, whoever is resuming the async method at begin of step 8 will trigger this machinery and force a hidden ExecutionContext.Run so the async method resumes on the correct ExecutionContext.

Basically the steps you listed are not fine grained enough, you are omitting the steps when something leaves an ExecutionContext. Note that you do not "set an ExecutionContext" - you do "ExecutionContext.Run" which always has a defined entry/exit point. The async/await machinery will restore the old ExecutionContext when leaving and this will trigger an AsyncLocal value switch. It will restore the delegates ExecutionContext when resuming, which will trigger another AsyncLocal value switch.

[avivanoff]

@davidfowl
I don't know what you mean. Task.Run and WindowsIdentity.RunImpersonated are both examples of methods that take callbacks but they do completely different things. I could tell you to look at the implementation of each but I'm not exactly sure what you're looking for.

Comparing Task.Run and WIndowsIdentity.RunImpersonated is a bad example. Task.Run has an Task.Run(Func) and ask.Run(Func<Task>) overloads. if WindowsIdentity.RunImpersonated had similar overloads, I would have no questions.

[fredrikhr]

@weltkante AAAH! :) Okay, this definitely clarifies everything! I see now that I missed the part of how the execution context worked and how it resets to null.

This is because is the impersonated token AsyncLocal is a static field in WindowsIdentity, correct? So the AsyncLocal is always there, its value is just null most of the time, and the callback of the AsyncLocal always invokes RevertToSelf before attempting to invoke ImpersonateLoggedOnUser if there is a token to impersonate (i.e. one that is non-null and valid).

[weltkante]

Task.Run has an Task.Run(Func) and Task.Run(Func<Task>) overloads.

Just for record, these overloads need to do different things, otherwise Run(Func<Task>) wouldn't exist either.

if WindowsIdentity.RunImpersonated had similar overloads, I would have no questions.

Personally I think its a documentation problem and can be solved as such. Though its still an option to add a no-op overload just forwarding to the synchronous version, to make it easier to "fall into the pit of success" if no easy-to-understand way to document this can be found.

[avivanoff]

Since full framework is not likely to get these overloads, updating documentation is the only option.

[Macromullet]

This API actually already works for both sync and async calls, the problem is the way it works makes it hard for the caller to understand how to use it so it ends up being a "pit of failure" API. The correct way to use this API in an async context is like so (ASP.NET middleware example):

app.Use(async (context, next) =>
{
    await WindowsIdentity.RunImpersonated(someToken, () => next());
});

The WindowsIdentity.RunImpersonated internally uses an async local to track thread changes and calls the appropriate win32 APIs to make sure the current thread is being impersonated. It also makes sure that execution context changes (e.g. the async locals) don't leak out of the call to RunImpersonated.

Does this contradict this article?
https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#windowsidentityrunimpersonated

[davidfowl]

Yes and honestly there were bugs that prevented it from working like this in the early days. The API is still a pit of failure IMO.

[Macromullet]

Can we get the docs updated? I consider the above kind of the gold standard that I share, and you wrote it, so thanks for getting the ball rolling on that stuff. I agree about the usage aspect. Any API that requires you to use it one way and one way only is doomed to fail. It's not dissimilar to the issues you found with HttpContextAccessor. It's better than it was with HttpContext.Current, but still has issues.

[stephentoub]

@davidfowl, can you elaborate on the pit-of-failure-ness? If there were a public static Task<T> RunImpersonatedAsync<T>(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task<T>> func), it would behave exactly the same way. I get the concern that the current signature is non-obvious, but is that the concern you're referring to, or something else?

[davidfowl]

I get the concern that the current signature is non-obvious, but is that the concern you're referring to, or something else?

That's the main concern. I think adding the following methods(s) would suffice:

public static Task<T> RunImpersonatedAsync<T>(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task<T>> func);
public static Task RunImpersonatedAsync<T>(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task> func);

[avivanoff]

@stephentoub, @davidfowl, what are the chances of this change making it into the full framework?

[stephentoub]

what are the chances of this change making it into the full framework?

What change? New overloads? Close to zero. But as noted, these overloads are also completely implementable as literal one-liners on top of the existing API surface area:

public static Task<T> RunImpersonatedAsync<T>(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task<T>> func) =>
    WindowsIdentity.RunImpersonated(safeAccessTokenHandle, func);

public static Task RunImpersonatedAsync<T>(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task> func) =>
    WindowsIdentity.RunImpersonated(safeAccessTokenHandle, func);

[avivanoff]

@stephentoub, can we at least get documentation updated that it is OK to use WindowsIdentity.RunImpersonated with asynchronous code?

[davidfowl]

@stephentoub I'm going to mark this API as ready for review.

[terrajobst]

Video

• These APIs can be simply implemented over the existing one, thanks for generics.
• However, it's not obvious that is in fact supported. Adding explicit overload that are named with the Async suffix would make sense
• We considered adding a cancellation token, but given callers likely have to allocate a closure anyway, this doesn't save anything and impersonation is already expensive.

#nullable enable
namespace System.Security.Principal
{
    public partial class WindowsIdentity
    {
        public static Task<T> RunImpersonatedAsync<T>(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task<T>> func)
            => RunImpersonated(safeAccessTokenHandle, func);

        public static Task RunImpersonatedAsync(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task> func)
            => RunImpersonated(safeAccessTokenHandle, func);    
    }
}

[avivanoff]

@stephentoub, I am still trying to get a clear answer whether this works on full framework. Related issue has been sitting there for a while, and documentation has not been updated, either.

[avivanoff]

Documentation has finally been updated.