[release/5.0] make async name resolution working from impersonificated context#46897
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsThis is port of #45816 to fix #29935 Customer ImpactThis is pretty old issue where asynchronous name resolution and services depending on that (like HttpClient) do not work when running from impersonificated context on Windows e.g. user other than owner of the process. This is blocking several large customers from migrating from Framework to .NET Core Regression?Yes. This works in .NET Framework and got broken 2.1 by dotnet/corefx#26850 TestingTesting was manual as we lack infrastructure for enterprise authentication. PerformanceThere may be some small impact for cases when running in imperonificated context as we will try async resolution and we will fall back to old method if that fails. This seems like a problem with underlying OS and it was brought to attention of responsible team. If this is ever fixed in Windows, this will basically become noop. RiskSmall. The fix is to retry synchronous API on thread pool if async resolution fails with certain error code.
|
| GetAddrInfoExContext.FreeContext(context); | ||
| return null; | ||
| } | ||
| else if (errorCode != SocketError.IOPending) |
There was a problem hiding this comment.
Did we / could we add any tests that validate name resolution in an impersonated context?
There was a problem hiding this comment.
not easily as it would require separate account on the machine and password for it. So it is going to be problematic from security prospective. I was thinking about it and it seems like it would be best to address this as part of the "Enterprise authentication testing" project - if we ever decide to fund it. for that, we will need special setup and we will need to work out the password management.
I could also add test conditional on environment variables (e.g. run if user & password is provided via environment) but that will not run during regular CI.
There was a problem hiding this comment.
We have tests at https://github.com/dotnet/runtime/blob/master/src/libraries/System.Security.Principal.Windows/tests/WindowsIdentityImpersonatedTests.netcoreapp.cs for WindowsIdentity that do impersonation. Can any of that be used?
There was a problem hiding this comment.
yes, I think this should work. I will explore that and create PR for master.
leecow
added
Servicing-approved
|
cc @dotnet/ncl for review |
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
The product change LGTM. But I would feel better about it, especially for servicing, if we had some tests.
|
I will work that out in master @stephentoub and I'll port it here once ready. I think test-only changes are TELL mode, right? |
correct |
ghost
locked as resolved and limited conversation to collaborators
4 participants
This is port of #45816 to fix #29935
cc: @karelz @samsp-msft
Customer Impact
Asynchronous name resolution and services depending on that (like HttpClient) do not work when running from impersonate context on Windows (ie . user other than owner of the process)
At least 4 customers reported this in #29351
The issue has been present for some time, but is now blocking several large customers from migrating from Framework to .NET Core.
Regression?
Yes. This works in .NET Framework and was broken in 2.1 by dotnet/corefx#26850 when we began supporting async name resolution. As part of this we changed to use GetAddrInfoExW which has a bug in that it does not flow the impersonated token. The fix we made is to fall back to the sync API if it fails.
Testing
Tested manually (we lack infrastructure for automated tests using enterprise authentication)
All existing automated tests are still passing.
Performance
There may be some small impact for cases when running in impersonated context as we first will try async resolution and we will fall back to old method if that fails. However that's an improvement over not working. This seems like a problem with underlying OS and it was brought to attention of responsible team. If this is ever fixed in Windows, this will basically become noop.
Risk
Small. The fix is to retry synchronous API on thread pool if async resolution fails with certain error code.