Runtime: Some System.Security.Principal types are missing members present in Desktop

[danmosemsft]

The following members are on types implemented in .NET Core. However on desktop in 4.5+ they have more members. The members below are missing from Core.

The problem with this situation is

• it defies user expectations that if a type X exists, code from other platforms that use X will work on .NET Core: instead they have to reason about individual members on X
• in some cases it cannot be fixed without servicing -- for instance methods we could supply a package with extension methods, but it would not be binary-compatible as different type identity

I didn't look at usage data. Any that are not be technically possible on Core in ideally would exist but throw PlatformNotSupportedException. Even if we don't have time to implement them, it's potentially valuable to do that (allows compile to succeed, for a runtime path that may not be followed).

This is not a NS2.0 issue. The types are not in NS2.0

MembersMustExist : Member 'System.Security.Principal.IdentityReferenceCollection.IsReadOnly.get()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsIdentity..ctor(System.IntPtr, System.String, System.Security.Principal.WindowsAccountType)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsIdentity..ctor(System.IntPtr, System.String, System.Security.Principal.WindowsAccountType, System.Boolean)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsIdentity..ctor(System.Security.Principal.WindowsIdentity)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsIdentity..ctor(System.String, System.String)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsIdentity.DeviceClaims.get()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsIdentity.Impersonate()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsIdentity.Impersonate(System.IntPtr)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsIdentity.UserClaims.get()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsPrincipal.DeviceClaims.get()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsPrincipal.UserClaims.get()' does not exist in the implementation but it does exist in the contract.

also

MembersMustExist : Member 'System.Security.SecurityContext.SuppressFlow()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.SecurityContext.SuppressFlowWindowsIdentity()' does not exist in the implementation but it does exist in the contract

For completeness, these relevant types are missing:

TypesMustExist : Type 'System.Security.Principal.WindowsAccountType' does not exist in the implementation but it does exist in the contract.
TypesMustExist : Type 'System.Security.Principal.WindowsImpersonationContext' does not exist in the implementation but it does exist in the contract.

[danmosemsft]

These three have significant usage, the rest have little or none. @stephentoub do you know why we implemented RunImpersonated but not Impersonate .. ?

up to 1.4%
M:System.Security.Principal.WindowsIdentity.#ctor(System.IntPtr,System.String,System.Security.Principal.WindowsAccountType,System.Boolean)

up to 6%
M:System.Security.Principal.WindowsIdentity.Impersonate
M:System.Security.Principal.WindowsIdentity.Impersonate(System.IntPtr)

[danmosemsft]

Ping @stephentoub :)

[stephentoub]

@stephentoub do you know why we implemented RunImpersonated but not Impersonate .. ?

I don't. I was going to say that we'd have an issue with Impersonate, in that on desktop the impersonation information is associated with the current ExecutionContext, and that's not the case in coreclr/corefx. But on second thought I realized we have the same problem with RunImpersonated: even though it's scoped due to taking an Action or a Func that's what's run while impersonated, the body of RunImpersonated can still queue work, and that work isn't going to flow the impersonation as it does on desktop. I'm not sure what to do about this.
cc: @kouvel

[danmosemsft]

OK it seems finishing Impersonation is a Future issue that this can track.
@kouvel comment on the possible bug in our existing RunImpersonated?

[kouvel]

@kouvel comment on the possible bug in our existing RunImpersonated?

That's probably the same issue with not flowing the impersonation. I'll take a look at trying that using an AsyncLocal.

[stephentoub]

There are several issues floating around related to this, and I guess I only followed up on the others. My initial concern seems to be mitigated by the fact that we do use AsyncLocal to flow impersonation in the corefx implementation.

[kouvel]

Ah ok, then I guess it's just about bringing back the rest of the APIs

[ericstj]

I found an old comment in email about this from a former owner, quoting as this is not my text.

The reason for the new Run(Un)Imperonated APIs is that the old Impersonate method cannot be implemented correctly without some crazy runtime magic to deal with exception filters. Imagine you have a function A() that impersonates, and then throws for whatever reason:
```C#
static void A()
{
WindowsImpersonationContext context = WindowsIdentity.Impersonate(token);
try
{
throw new Exception();
}
finally
{
context.Undo();
}
}

>Now imagine this is called by B(), which has an exception filter:
```C#
        static void B()
        {
            try
            {
                A();
            }
            catch (Exception e) if (Filter(e))
            {
                ///
            }
        }

        static bool Filter(Exception e)
        {
            // are we impersonating here?
            return false;
        }

Since filters are executed in the first pass of EH, before any finally blocks are executed, the filter ends up running before A’s finally block has a chance to revert the impersonation. So B ends up running a filter in A’s impersonation context, which clearly was not the intent of this code.

The CLR deals with this by building special knowledge of WindowsIdentity into the JIT and the VM’s exception handling code, which detects patterns like our example A() and reverts any impersonation as exceptions pass through in the first pass. As you might imagine, this is not the kind of thing we want to keep doing in new runtimes if we can avoid it. Thus the new APIs, which don’t rely on any special handling in their callers.

[Anipik]

@ericstj should i start working on porting these apis(except the impersonation ones) ?

[ericstj]

Yes, anything that doesn't involve impersonation and looks like a straight forward port.

[Anipik]

@danmosemsft @ericstj do we need to port

MembersMustExist : Member 'System.Security.SecurityContext.SuppressFlow()' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.SecurityContext.SuppressFlowWindowsIdentity()' does not exist in the implementation but it does exist in the contract

?

[ericstj]

WRT to SecurityContext, it looks like that whole type is already stubbed out:
https://github.com/dotnet/corefx/blob/db181f448f3c6bc252ae267025adf7f77f36869c/src/System.Security.Permissions/src/System/Security/SecurityContext.cs#L19-L20

I think this issue predated @danmosemsft 'a commit https://github.com/dotnet/corefx/commit/db181f448f3c6bc252ae267025adf7f77f36869c.

[MarcoRossignoli]

@Anipik is this completed?

[Anipik]

@MarcoRossignoli only the the members that involve impersonation are left.

@ericstj @danmosemsft is there any plan for the members that involve impersonation ?

[danmosemsft]

From talking to @jkotas -- we should not concern ourselves with the exception filter issue, we should just go ahead and port this. Need to find a dev.

[MarcoRossignoli]

Need to find a dev

Poor MS :laughing:
Joking apart I'm not in the office right now...however if this is not urgent tomorrow I'll check the work to do and if I feel ok with it I could tackle it!
Feel free to assign to other!

[ViktorHofer]

@MarcoRossignoli awesome, thanks!

@danmosemsft I don't understand why exactly this set of APIs are part of the WPF/Winforms bring-up? Are such apps heavily relying on these APIs and if yes, is this functionality necessary for preview-1?

[danmosemsft]

@ViktorHofer yes, it's in use by eg., some popular control vendors.

This would be for preview 2 at this point. But, many of the vendors we are working with will be trying "live" builds so relatively soon would be good.

[jkotas]

Note that these vendors are modifying their code to run on .NET Core anyway. If it is ok with them, it would be better to ask them to change their code to use RunImpersonated because of it would avoid the subtle issues with the exception filters mentioned above.

[danmosemsft]

That might affect prioritization, but it seems to me that issue is not serious (most exception filters don't do much work, at least work for which impersonation is relevant?) so porting would still be worthwhile, to reduce paper cuts and not have to revisit this issue periodically 😺 As we know, not everyone can easily change their code.

[MarcoRossignoli]

@danmosemsft I did some check and found this members missing:

MembersMustExist : Member 'System.Security.Principal.WindowsIdentity..ctor(System.String, System.String)' does not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsIdentity.Impersonate()' dos not exist in the implementation but it does exist in the contract.
MembersMustExist : Member 'System.Security.Principal.WindowsIdentity.Impersonate(System.IntPtr)' does not exist in the implementation but it does exist in the contract.
TypesMustExist : Type 'System.Security.Principal.WindowsImpersonationContext' does not exist in the implementation but it does exist in the contract.

Can you confirm that these are the members to implement?Maybe could be better re-launch tools generates list on top.

[danmosemsft]

@MarcoRossignoli looks like

       public WindowsIdentity(IntPtr userToken, string type, WindowsAccountType acctType);
        public WindowsIdentity(IntPtr userToken, string type, WindowsAccountType acctType, bool isAuthenticated);
       public WindowsIdentity(string sUserPrincipalName, string type);
        public virtual WindowsImpersonationContext Impersonate();
        public static WindowsImpersonationContext Impersonate(IntPtr userToken);

Seems part of the code is currently in WindowsIdentity(string sUserPrincipalName). I'm comparing
https://referencesource.microsoft.com/#mscorlib/system/security/principal/windowsidentity.cs,979

[MarcoRossignoli]

@danmosemsft after some code analysis
public WindowsIdentity(string sUserPrincipalName, string type); could be implemented refactoring a method helper like LogonAsUntrustedCaller with code on WindowsIdentity(string sUserPrincipalName) the strange thing is that param type is not used...maybe should be _authType = type;

This methods are implemented here and here:

public WindowsIdentity(IntPtr userToken, string type, WindowsAccountType acctType);
public WindowsIdentity(IntPtr userToken, string type, WindowsAccountType acctType, bool isAuthenticated);

Missing piece WindowsImpersonationContext
I think that to use WindowsIdentity.Impersonate() created from upn we need an impersonation token so maybe we've to import code to try to enable SeTcbPrivilege or we could maintain current behaviour?

[danmosemsft]

Hmm, looks like it's significant work (especially with tests) and requires some knowledge of Windows access control. Also, we will need to complete this relatively soon. Let me know if you'd rather take on something else 😃 But, if you'd like to give it a start and expect to have enough time this week, you're welcome to.

[MarcoRossignoli]

Also, we will need to complete this relatively soon...this week

Sadly I cannot guarantee to be so fast :pensive:...feel free to assign to other...I apologize. Let me know if prioritization change. I could try and have something in a pair of weeks, but maybe is too much.

[danmosemsft]

No need to apologize, you have a day job 😄 Let me see who I can find here.

[MarcoRossignoli]

you have a day job

Yes, I'm very lucky! But working on coreX codebases is very stimulating, thank's to all of you for the opportunity!

[ViktorHofer]

I can't say it enough, thanks for the great work you are doing!

[danmosemsft]

Well, it turns out I couldn't find someone soon enough to do it for preview 1, so we have more time, if you're still interested in the work :)

[MarcoRossignoli]

Sure, I'm trying!

[danmosemsft]

@MarcoRossignoli OK it's all yours -- let us know if you need anything!

[MarcoRossignoli]

let us know if you need anything!

I think we need to create tests with "new windows user" and assert identity...in past when I worked on File.Encrypt/File.Decrypt wasn't a clear and standard way to "create new user" fo CI. What do you think?

[danmosemsft]

TestAccountImpersonator looks promising in src\System.IO.Pipes\tests\NamedPipeTests\NamedPipeTest.CurrentUserOnly.netcoreapp.Windows.cs . Seems like it could move to Common to be reused. In fact looks like maybe the existing tests for RunImpersonated could potentially use improvement.

[MarcoRossignoli]

Thank's!With some amendment(user name cannot be constant to avoid conflicts) should work great! For common you mean https://github.com/dotnet/corefx/tree/master/src/CoreFx.Private.TestUtilities/src/System?

[danmosemsft]

That seems a good place.

[danmosemsft]

BTW I think tests that create users should be outer loop only.

[MarcoRossignoli]

Should I skip FrameSecurityDescriptor and StackCrawlMark https://referencesource.microsoft.com/#mscorlib/system/security/principal/windowsidentity.cs,735, right? FrameSecurityDescriptor contains unsupported CAS logic.
/cc @jkotas @danmosemsft

[danmosemsft]

Yes please remove all that.

[MarcoRossignoli]

this ctor https://docs.microsoft.com/en-us/dotnet/api/system.security.principal.windowsidentity.-ctor?view=netframework-4.7.2#System_Security_Principal_WindowsIdentity__ctor_System_String_System_String_ is This constructor is intended for use only on computers joined to Windows Server 2003 or later domains. any possibility to test?I could manual test inside a domain...but on CI?Conditional with some trick to understand if we'are inside domain?

[danmosemsft]

@MarcoRossignoli we're not going to set up a domain just for that purpose - I would just copy the code - if you have a domain locally you could try - otherwise we can open an issue for one of us to try it locally.

[MarcoRossignoli]

We need to enable SeTcbPrivilege to have Impersonating token. My idea was reuse internal Privilege class, so I moved some source to common...however we cannot do in this way because code catch public PrivilegeNotHeldException exception and I cannot reference System.Security.AccessControl due to circular dependency(SecurityIdentifier). At this point I see two strategy...InternalsVisibleToAttribute but it's not used on codebase, the other is "duplicate" some Privilege code, or is there other standard strategy?

[jkotas]

InternalsVisibleToAttribute but it's not used on codebase,

Right, we are not using InternalsVisibleToAttribute in corefx.

however we cannot do in this way because code catch public PrivilegeNotHeldException

You can create a private copy of PrivilegeNotHeldException in System.Security.Principal.Windows to get around this. Here is an example how a similar situation is solved for Registry: https://github.com/dotnet/corefx/blob/046c0291d0172bf23139cf6d3f856f92364ac94c/src/Common/src/CoreLib/Microsoft/Win32/SafeHandles/SafeRegistryHandle.cs#L15

Another option would be to merge System.Security.Principal.WindowsIndetity and System.Security.AccessControl assemblies. They have pretty significant overlap now.

[MarcoRossignoli]

@jkotas if you agree I would try with private copy strategy to avoid huge PR. There is also other changes I'd like to fix...as said above a "CreateWindowsUser" helper on test utilities and relative fix on named pipe tests, but maybe is better do it in other PR after this one. Do you agree?

[jkotas]

Yes, more smaller self-contained PRs is better.

[MarcoRossignoli]

Question: I found difference between implementation of RunImpersonated from netfx and netcoreapp, netfx use WindowsImpersonationContext and core use ExecutionContext+AsyncLocal to preserve EC and automatically revert impersonation. Why this difference? I don't found this behaviour on docs if someone want to store something on EC during impersonification on netfx could not work anymore, right?Second...I don't have a "delegate" to run on EC to maintain same behaviour on classic WindowsImpersonationContext scenario, so if on netfx use RunImpersonated it's the same as use WindowsImpersonationContext on core we should maintain the behaviour?Or maybe there is a way to switch EC with a set on some property?

[jkotas]

netfx and netcoreapp, netfx use WindowsImpersonationContext and core use ExecutionContext+AsyncLocal to preserve EC and automatically revert impersonation. Why this difference?

The netfx execution context was a fixed hard-coded set: https://referencesource.microsoft.com/#mscorlib/system/threading/executioncontext.cs,493. It included WindowsIdentity as part of SecurityContext.

The fixed hard-coded set was both too much (your app paid to flow things it did not care about or ever used) and too little (your app could not extend the set of things to flow).

AsyncLocals were invented to make this abstract. The execution context in core is a collection of AsyncLocals.

[MarcoRossignoli]

@jkotas thank's a lot for explanation...I've almost done...but I'm not sure if I need to capture EC before impersonation and restore after it, because I don't found a method to restore EC on current thread.
RunImpersonated uses ExecutionContext.Run because there we've a delegate, but here we return WindowsImpersonationContext and if user change EC on Undo() I should restore captured EC to follow RunImpersonated logic. But there isn't a ExecutionContext.Restore(ExecutionContext ec). How I can restore EC? Here the code under consideration.

[jkotas]

The desktop implementation does not saves/restores the whole EC, so I do not think you need to do that in .NET Core implementation of these APIs either.

I understand the implementation of the existing RunImpersonated saves/restores the whole execution context in .NET Core today. This behavior is not compatible with desktop. If you would like to make all impersonation APIs in .NET Core to work consistently, I would rather look at fixing this to match desktop. For example, try the following on desktop and .NET Core - it will print different results today:

using System;
using System.Globalization;
using System.Security.Principal;

class Program
{
    static void Main(string[] args)
    {
        var wi = WindowsIdentity.GetCurrent();        

        WindowsIdentity.RunImpersonated(wi.AccessToken, () =>
        {
            CultureInfo.CurrentCulture = new CultureInfo("cz-cz");
            Console.WriteLine(CultureInfo.CurrentCulture);
        });

        Console.WriteLine(CultureInfo.CurrentCulture);
    }
}

BTW: All ComVisible attributes should be stripped.

[MarcoRossignoli]

BTW: All ComVisible attributes should be stripped.

OK

I would rather look at fixing this to match desktop.

Ok I'll fix code to behave like desktop using WindowsImpersonationContext...do you agree?

[jkotas]

Ok I'll fix code to behave like desktop using WindowsImpersonationContext...do you agree?

I agree with fixing this, but I am not sure whether WindowsImpersonationContext is the right way to fix that. WindowsImpersonationContext feels pretty heavy weight compare to what the current .NET Core implementation does. It may regress something important - you should measure.

[MarcoRossignoli]

I agree with fixing this, but I am not sure whether WindowsImpersonationContext is the right way to fix that. WindowsImpersonationContext feels pretty heavy weight compare to what the current .NET Core implementation does. It may regress something important - you should measure.

Sure.

BTW: All ComVisible attributes should be stripped.

I found attribute on codebase i.e. WindowsAccountType, when we should apply them?

[jkotas]

I found attribute on codebase i.e. WindowsAccountType, when we should apply them?

You can delete them here. These attributes should be only on types that really have to do something with COM Interop.

[MarcoRossignoli]

I did some measure, using WindowsImpersonationContext I think we pay more on IsImpersonating scenario due to token duplication, code here

BenchmarkDotNet=v0.11.2, OS=Windows 10.0.17134.407 (1803/April2018Update/Redstone4)
Intel Core i7 CPU 860 2.80GHz (Nehalem), 1 CPU, 8 logical and 4 physical cores
Frequency=2727540 Hz, Resolution=366.6307 ns, Timer=TSC
.NET Core SDK=2.1.500
  [Host]         : .NET Core 2.1.6 (CoreCLR 4.6.27019.06, CoreFX 4.6.27019.05), 64bit RyuJIT
  CoreFxUpdated  : .NET Core 6e21ac56-683f-4873-bf2a-c24b6887a256 (CoreCLR 4.6.27113.05, CoreFX 4.7.18.57001), 64bit RyuJIT
  CoreFxUpstream : .NET Core fbd256b7-2932-45ab-8245-5e8a7e16aca2 (CoreCLR 4.6.27119.03, CoreFX 4.7.18.57001), 64bit RyuJIT
  Job-YEFMVZ     : .NET Framework 4.7.2 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.3221.0

| Method | Job | Runtime | Toolchain | cycles | Mean | Error | StdDev | Ratio | RatioSD | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|-------------------------------- |--------------- |-------- |---------- |------- |----------:|----------:|----------:|------:|--------:|------------:|------------:|------------:|--------------------:|
| RunImpersonated | CoreFxUpdated | Core | CoreRun | 3000 | 42.01 ms | 0.8308 ms | 1.2934 ms | 1.00 | 0.04 | 166.6667 | 83.3333 | - | 750 KB |
| RunImpersonated | CoreFxUpstream | Core | CoreRun | 3000 | 42.37 ms | 0.8359 ms | 0.8210 ms | 1.00 | 0.00 | 250.0000 | 83.3333 | - | 1171.88 KB |
| RunImpersonated | Default | Clr | Default | 3000 | 58.35 ms | 1.1290 ms | 1.2549 ms | 1.38 | 0.04 | 444.4444 | 222.2222 | - | 2086.31 KB |
| | | | | | | | | | | | | | |
| RunImpersonated_IsImpersonating | CoreFxUpstream | Core | CoreRun | 3000 | 87.08 ms | 1.6784 ms | 2.4072 ms | 1.00 | 0.00 | 500.0000 | 166.6667 | - | 2671.88 KB |
| RunImpersonated_IsImpersonating | CoreFxUpdated | Core | CoreRun | 3000 | 93.97 ms | 1.7148 ms | 1.5202 ms | 1.07 | 0.04 | 333.3333 | 166.6667 | - | 1804.69 KB |
| RunImpersonated_IsImpersonating | Default | Clr | Default | 3000 | 128.98 ms | 2.3061 ms | 2.1571 ms | 1.47 | 0.05 | 1250.0000 | 500.0000 | - | 5158.17 KB |

With some code duplication things goes as expected better

| Method | Job | Runtime | Toolchain | cycles | Mean | Error | StdDev | Ratio | RatioSD | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
|-------------------------------- |--------------- |-------- |---------- |------- |----------:|----------:|----------:|------:|--------:|------------:|------------:|------------:|--------------------:|
| RunImpersonated | CoreFxUpdated | Core | CoreRun | 3000 | 42.02 ms | 0.8216 ms | 0.9132 ms | 0.99 | 0.04 | 76.9231 | - | - | 585.94 KB |
| RunImpersonated | CoreFxUpstream | Core | CoreRun | 3000 | 42.54 ms | 1.0403 ms | 1.1562 ms | 1.00 | 0.00 | 250.0000 | 83.3333 | - | 1171.88 KB |
| RunImpersonated | Default | Clr | Default | 3000 | 59.34 ms | 1.2645 ms | 2.2146 ms | 1.41 | 0.08 | 444.4444 | 222.2222 | - | 2086.31 KB |
| | | | | | | | | | | | | | |
| RunImpersonated_IsImpersonating | CoreFxUpdated | Core | CoreRun | 3000 | 83.72 ms | 1.6326 ms | 2.4436 ms | 0.95 | 0.03 | 285.7143 | 142.8571 | - | 1171.88 KB |
| RunImpersonated_IsImpersonating | CoreFxUpstream | Core | CoreRun | 3000 | 87.99 ms | 0.8963 ms | 0.8384 ms | 1.00 | 0.00 | 500.0000 | 166.6667 | - | 2671.88 KB |
| RunImpersonated_IsImpersonating | Default | Clr | Default | 3000 | 128.55 ms | 1.3502 ms | 1.2630 ms | 1.46 | 0.02 | 1250.0000 | 500.0000 | - | 5158.17 KB |

Is there some rule you apply to decide between pay perf and code duplication?

[jkotas]

These differences do not look too bad to me (with or without code duplication).

I have tried to find out why the ExecutionContext handling in the existing RunImpersonated method was done the way it was done. I am not able to find it - the code came with the initial commit: https://github.com/dotnet/corefx/pull/1638 . @MattGal This was your PR - do you happen the remember?

It may be best to do do any changes for the existing RunImpersonated method as separate issue/PR from this one. It may be the case that the RunImpersonated method was intentionally changed this way, even though it is a behavior change from desktop and we would not want to change it back. I do not know.

[MarcoRossignoli]

It may be best to do do any changes for the existing RunImpersonated method as separate issue/PR from this one. It may be the case that the RunImpersonated method was intentionally changed this way, even though it is a behavior change from desktop and we would not want to change it back. I do not know.

Ok, only to know...can you confirm that today there isn't an API to capture/revert current EC(other than ExecutionContext.Run, something like pseudo ExecutionContext.Restore(ExecutionContext ec))?

[MattGal]

@jkotas there was no intentional change in behavior, if something changed it was a mistake. I was mostly participating in the open-source effort to learn about git, .NET Core, etc. I grabbed a copy of sources from ndpclrsrcBCLSystemSecurityPrincipal from whatever appropriate 4.x branch we were porting from at the time and got it working, not much more thought to it than that unfortunately.

[jkotas]

can you confirm that today there isn't an API to capture/revert current EC

Right

[blowdart]

I was pretty sure in an API design meeting we decided bringing back Impersonate as a method was a bad idea and we'd rather people moved to the much safer new behaviour. @terrajobst I believe this was one of your meetings.

[jkotas]

I agree with @blowdart that adding the Impersonate API to .NET Core for compat feels like a recipe for disaster:

(1) Poorly designed API that is often used incorrectly
(2) Subtle behavior differences between how .NET Framework behaved and how .NET Core would behave: execution context flow or exception filters
(3) Security related API. Bugs caused by combinations of (1) and (2) are security bugs.

[MarcoRossignoli]

FWIW I worked on porting and I agree especially with (1), knowledge to correctly use this API go beyond the very(too) simple syntax.
Also for (3) as I've pointed out here related to the risk to return impersonated thread to thread pool in case of bug(I don't know if there is some mechanism to ensure cleanup before reuse). Maybe PNSE.

[blowdart]

@MarcoRossignoli yes, to be clear, this isn't about your PR, or your code, it's about the API itself, which deserves to vanish and not come back, which I believe had previously been agreed to when we were doing the 3.0 API review.

[blowdart]

To update from an internal discussion we will not bring back .Impersonate due to security concerns about it's function and the api design. Instead RunImpersonatedAsync will be implemented and the RunImpersonated APIs will be the only impersonation APIs we support.

@terrajobst will dig out the results of the API gap meetings we had where we ruled out APIs for security reasons and update the missing API issue to flag the ones that are not returning.

@MarcoRossignoli I apologise for discarding the work you put into this. It was entirely our process failure here, and nothing to do with your code.

[MarcoRossignoli]

I apologise for discarding...

@blowdart understood, no problem!

[danmosemsft]

Ditto, thanks again @MarcoRossignoli

[airbreather]

Just so it's noted, we ran into this when trying to port our WPF application that uses the Enterprise Library Logging Application Block. The compat shims aren't enough here, presumably because of this.

Specifically, the Microsoft.Practices.EnterpriseLibrary.Logging.LogWriter.ProcessLog method tries to revert any impersonation that might be in place before doing its work (and restore it after). There does not appear to be any workaround for it other than to discontinue our use of this logging provider and replace it with a different one (or, of course, for us to modify the source code to remove the unavailable bits and rebuild).

I'm not looking to change any decisions or asking for any help, just pointing this out (maybe it'll find its way into some documentation somewhere, or someone will find it through a search, or something).