Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/5.0] Support Async DNS lookup on older Windows from impersonificated conte… #47657

Merged
merged 2 commits into from
Feb 11, 2021

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jan 29, 2021

Fixes #45165

Technically this is continuation of #29935 which we fixed in 5.0.3 (PR #46897) and turned out to be only partial fix. This issue is specific to Win8 and Win 2012 (as reported in #45165).
The original issue #29935 didn't have test (because we didn't know how), but we came up with idea later. The test discovered the same problem on Win8 and Win 2012 as the reported issue #45165.

Customer Impact

Asynchronous name resolution and services depending on that (like HttpClient) do not work when running from impersonate context on Win8 and Win 2012 (ie . user other than owner of the process)
At least 1 customer hit this on Win 2012 in #45165.
The issue has been present for some time, but is now blocking migration from Framework to .NET Core.

Regression?

Yes. This works in .NET Framework and was broken in .NET Core 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.
We worked around the OS bug in #29935, but we missed the case for Win8 and Win 2012, which we are fixing in this PR.

Testing

Added 2 automated tests to cover NameResolution from impersonificated context.

Performance

This only adds one more error code check to existing code. No neccesary.

Risk

Small. The fix is to retry synchronous API on thread pool if async resolution fails with certain error codes.
The logic already exists, we are just using it for more OS error codes.

…xt (dotnet#47435)

* test WindowsIdentityImpersonatedTests runs

* fix win8

* add test variant

* final cleanup
@wfurt wfurt added Servicing-consider Issue for next servicing release review area-System.Net labels Jan 29, 2021
@wfurt wfurt added this to the 5.0.x milestone Jan 29, 2021
@wfurt wfurt requested review from stephentoub and a team January 29, 2021 23:23
@wfurt wfurt self-assigned this Jan 29, 2021
@ghost
Copy link

ghost commented Jan 29, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

This if follow-up on #46897 approved for 5.0 and fix for #45165.
When original fix was submitted I was not aware that #45165 and #29935 are essentially same issue since the manifestation was different. I also was not aware of fact that we can create ad-hoc accounts in CI until @stephentoub pointed to me to right direction. So this also brings test I promised in #46897.

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)
The issue has been present for some time, but is now blocking several large customers from migrating from Framework to .NET Core.

This fixes variant for Windows 8 and Server 2012

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

I did manual testing on Windows 8 as well I added two automated tests to cover NameResolution from impersonificated context.

Performance

This only adds one more error code check to existing code.

Risk

Small. The fix is to retry synchronous API on thread pool if async resolution fails with certain error code.
The logic already exists.

Author: wfurt
Assignees: wfurt
Labels:

Servicing-consider, area-System.Net

Milestone: 5.0.x

@karelz
Copy link
Member

karelz commented Feb 2, 2021

@danmosemsft do you think the above is reasonable to bring to shiproom?
In nutshell we found out Win8 and Win2012 gives different error code which we have to react to as well to make previous fix #29935 work well on these platforms.

@danmoseley
Copy link
Member

I would take it for a bar check . They may defer it waiting for more reports, but since it's really follow on from the last change, very low risk and it's easy to understand the change and the impact, it might go through.

@danmoseley
Copy link
Member

We added tests in the original change right -- how come they didn't break -- do we not run on 2021 or Win 8? Should we?

@wfurt
Copy link
Member Author

wfurt commented Feb 2, 2021

no, tests were added in the follow-up e.g. #47435 I'm trying to port to 5.0. I was not aware that we can create ad-hoc users on CI machine as this is my first impersoniofication fix.

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 4, 2021
@leecow leecow modified the milestones: 5.0.x, 5.0.4 Feb 4, 2021
@Anipik
Copy link
Contributor

Anipik commented Feb 10, 2021

@stephentoub @dotnet/ncl can you please review this pr, we need a code approval here to merge the change

@Anipik Anipik merged commit 8d42bce into dotnet:release/5.0 Feb 11, 2021
Copy link
Member

@karelz karelz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ghost ghost locked as resolved and limited conversation to collaborators Mar 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants