Runtime: Add asynchronous overload of WindowsIdentity.RunImpersonated
https://github.com/dotnet/corefx/issues/9996#issuecomment-307870746
https://github.com/aspnet/Home/issues/1805#issuecomment-339442334
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
Tratcher
All 69 comments
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
stephentoub
on 31 Oct 2017
Is it true that we cannot expect such a feature in a short period of time?
can you give us a timeframe?
robert
madrianr
on 1 Nov 2017
@Tratcher @stephentoub @madrianr are we discussing Integrated Windows Auth (IWA) ?
brentschmaltz
on 1 Nov 2017
Primarily Kerberos for the delegation capabilities.
Tratcher
on 1 Nov 2017
@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.
brentschmaltz
on 1 Nov 2017
How did that flow to the async actions?
Tratcher
on 1 Nov 2017
@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.
brentschmaltz
on 1 Nov 2017
And threadpool threads? That's where most of this code runs.
Tratcher
on 1 Nov 2017
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.
lakeman
on 1 Dec 2017
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
stephentoub
on 1 Dec 2017
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"?
bartonjs
on 10 Jan 2018
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
on 10 Jan 2018
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
Tratcher
on 6 Jun 2018
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
wpbrown
on 10 Sep 2018
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.
mj1856
on 13 Sep 2018
@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.
mj1856
on 14 Sep 2018
@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.
wpbrown
on 14 Sep 2018
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)
lakeman
on 14 Sep 2018
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
on 15 Sep 2018
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?
mravko
on 21 Oct 2018
@mravko interesting, just right now I have been asked to cost this out.
We will have an answer soon.
brentschmaltz
on 22 Oct 2018
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?
clairernovotny
on 23 Oct 2018
@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?
brentschmaltz
on 23 Oct 2018
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?
lakeman
on 25 Oct 2018
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?
fredrikhr
on 18 Jun 2019
@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.
wpbrown
on 18 Jun 2019
@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.
brentschmaltz
on 19 Jun 2019
@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.
fredrikhr
on 20 Jun 2019
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 https://github.com/aspnet/AspNetWebStack/issues/260. I was hoping @stephentoub can tell us what we are doing wrong.
avivanoff
on 26 Sep 2019
@avivanoff - That repro is using identity.Impersonate(), not RunImpersonated.
mj1856
on 19 Oct 2019
RunImpersonated is a synchronous method, it cannot be used to run asynchronous action/function.
avivanoff
on 19 Oct 2019
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):
C#
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.
davidfowl
on 19 Oct 2019
@davidfowl have you verified that? We've tried exactly that in the past and had it fail miserably.
Tratcher
on 19 Oct 2019
We've tried exactly that in the past and had it fail miserably.
Before or after https://github.com/dotnet/corefx/pull/30346 and https://github.com/dotnet/corefx/pull/30377 ?
stephentoub
on 19 Oct 2019
And it should fail. This usage of asynchronous and synchronous code is incorrect. WindowsIdentity.RunImpersonated is a synchronous method, it runs passed in action/function while impersonating, all of it. If you pass in asynchronous method, only some code until the first async point will run while impersonating. I may be saying obvious things, but I want to make sure we are on the same page.
Consider the following code:
Task ThrowAsync()
{
try
{
return Task.Run(() => throw new InvalidOperationException());
}
catch (InvalidOperationException)
{
return Task.FromResult(true);
}
}
Is InvalidOperationException ever caught? No. This is the same as using asynchronous code in WindowsIdentity.RunImpersonated, where the role of RunImpersonated is played by try/catch. And this is how it should be:
async Task ThrowAsync()
{
try
{
await Task.Run(() => throw new InvalidOperationException())
.ConfigureAwait(false);
}
catch (InvalidOperationException)
{
}
}
await effectively "converts" synchronous try/catch into asynchronous one. Hence the need for RunImpersonatedAsync.
avivanoff
on 20 Oct 2019
It doesn't fail, it works fine.
And it should fail. This usage of asynchronous and synchronous code is incorrect. WindowsIdentity.RunImpersonated is a synchronous method, it runs passed in action/function while impersonating, all of it.
It doesn't.
If you pass in asynchronous method, only some code until the first async point will run while impersonating. I may be saying obvious things, but I want to make sure we are on the same page.
That's not what it does.
davidfowl
on 20 Oct 2019
Ok. Can you please explain in details how RunImpersonated works with asynchronous code? Because in my tests, just like in @Tratcher, it failed miserably.
avivanoff
on 20 Oct 2019
But more importantly, I would like an explanation why this happens.
avivanoff
on 20 Oct 2019
I thought i explained it here https://github.com/dotnet/corefx/issues/24977#issuecomment-544196325.
But more importantly, I would like an explanation why this happens.
Isn't this on .NET Framework?
davidfowl
on 20 Oct 2019
Does it matter? I am looking for an explanation why is it ok to run asynchronous code in synchronous context.
avivanoff
on 20 Oct 2019
Does it matter? I am looking for an explanation why is it ok to run asynchronous code in synchronous context.
Yes it matters, the code is different between the 2. I explained why RunImpersonated works and how it works. Do you have any more specifics on the details you need to explain why the .NET Core implementation doesn't work?
davidfowl
on 20 Oct 2019
- Documentation does not say anything about RunImpersonated being different between full framework and core.
- 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.
avivanoff
on 20 Oct 2019
@avivanoff my reading is that it uses an async local, which flows though async transitions. If it didn't, your concern would exist. https://docs.microsoft.com/en-us/dotnet/api/system.threading.asynclocal-1?view=netframework-4.8
danmosemsft
on 20 Oct 2019
However, an AsyncLocal does not cause the task to revert the identity when it yields to another task, it reacquires the impersonating identity when it is resumed on an async continuation.
Consider the following task hierarchy:
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)
So if I understand this correctly (please correct me if I'm wrong), whenever a task in branch 1 is resumed, the AsyncLocal will restore the current thread identity to the unprivileged user (and the same for branch 2 with the Administrator identity). However, when branch 3 is resumed, the async context won't have an impersonation token to restore to, so the continuation of anything in branch 3, would continue with whatever access token was active during the previously executed task, correct?
Imagine task 3 involves OS operations that are sensitive to the current identity (like accessing an ACL protected file, only an Administrator has access to). So sometimes, this operation will run with the service identity, sometimes with the unprivileged user, and sometimes as an Administrator. It is simply pure chance and depends on what happened before. This is how an unsuspecting part of application suddenly might achieve administrative access, as the administrative access token might have leaked from a previously executed task continuation.
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.
My question, and possible workaround for this: If you have an own dedicated task scheduler (with its own thread pool) for asynchronous impersonated async tasks, do you then avoid this issue? I imagine it would, since the dedicated task scheduler would never schedule a regular task that was not from a RunImpersonated code path, so the sync-context would always be guaranteed to have an access token to restore on continuation.
Likewise, the default task scheduler would never schedule RunImpersonated tasks, so leakage of access tokens would not really be possible, right?
@davidfowl, @stephentoub, can you help me out to fill in the blanks here? Am I missing something? Or am I way off in my understanding?
fredrikhr
on 20 Oct 2019
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?
davidfowl
on 20 Oct 2019
@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.
fredrikhr
on 20 Oct 2019
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.
davidfowl
on 20 Oct 2019
What do you mean “nothing bleeds outside”? You just bled out a Task.
So, I would like to repeat my questions:
- Documentation does not say anything about RunImpersonated being different between full framework and core.
- 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.
avivanoff
on 20 Oct 2019
@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)?
fredrikhr
on 20 Oct 2019
@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:
```C#
public T MyFunction
{
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.
davidfowl
on 20 Oct 2019
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.
weltkante
on 20 Oct 2019
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)
- 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). - main branches to code path
1.RunImpersonatedis called. it pushes the access token of the unprivileged user into the execution context and invokes the callback parameter, branch1.1. - code path
1.1executes under the impersonated identity of the unprivileged user. The code path start an asynchronous execution (e.g.await Task.Delay()). The async continuation of1.1is scheduled, and1.1returns with the return value being a Task representing the furture continuation of execution. - as the invoked code path from step 2 has returned,
RunImpersonatedrestores the thread identity to main's original identity, i.e. the service identity. - The same steps repeat for code branch
2with the identity now being the Administrator. - code branch
3is 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. - Asynchronous operation
1.1completes. Initially, the thread idenity is now still main's origin identity. But as the task is resumed, the impersonation token from branch1, 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. - Async op
2.1completes. 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 branch2is restored, the thread idenity is thus now the Administrator. - Async op
3completes. Again, its thread identity is now the Administrator. When3was originally started, there was no Impersonation going on, therefore there is no access token in the execution context to restore. Therefore, the continuation of task3resumes 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?
fredrikhr
on 20 Oct 2019
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.
weltkante
on 20 Oct 2019
@davidfowl
I don't know what you mean.Task.RunandWindowsIdentity.RunImpersonatedare 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
avivanoff
on 21 Oct 2019
@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).
fredrikhr
on 21 Oct 2019
Task.Run has an
Task.Run(Func)andTask.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.
weltkante
on 21 Oct 2019
Since full framework is not likely to get these overloads, updating documentation is the only option.
avivanoff
on 21 Oct 2019
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.RunImpersonatedinternally 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
Macromullet
on 21 Oct 2019
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.
davidfowl
on 21 Oct 2019
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.
Macromullet
on 21 Oct 2019
@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?
stephentoub
on 21 Oct 2019
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:
C#
public static Task<T> RunImpersonatedAsync<T>(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task<T>> func);
public static Task RunImpersonatedAsync<T>(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task> func);
davidfowl
on 21 Oct 2019
@stephentoub, @davidfowl, what are the chances of this change making it into the full framework?
avivanoff
on 23 Oct 2019
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:
```C#
public static Task
WindowsIdentity.RunImpersonated(safeAccessTokenHandle, func);
public static Task RunImpersonatedAsync
WindowsIdentity.RunImpersonated(safeAccessTokenHandle, func);
```
stephentoub
on 23 Oct 2019
@stephentoub, can we at least get documentation updated that it is OK to use WindowsIdentity.RunImpersonated with asynchronous code?
avivanoff
on 23 Oct 2019
@stephentoub I'm going to mark this API as ready for review.
davidfowl
on 23 Oct 2019
- 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
Asyncsuffix 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.
```C#
nullable enable
namespace System.Security.Principal
{
public partial class WindowsIdentity
{
public static Task
=> RunImpersonated(safeAccessTokenHandle, func);
public static Task RunImpersonatedAsync(SafeAccessTokenHandle safeAccessTokenHandle, Func<Task> func)
=> RunImpersonated(safeAccessTokenHandle, func);
}
}
```
terrajobst
on 26 Nov 2019
@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
on 27 Jan 2020
Documentation has finally been updated.
avivanoff
on 19 Mar 2020
Related issues
nalywa
·
3Comments
jamesqo
·
3Comments
matty-hall
·
3Comments
jzabroski
·
3Comments
chunseoklee
·
3Comments
Most helpful comment
@mravko interesting, just right now I have been asked to cost this out.
We will have an answer soon.