Skip to content

[release/5.0] make async name resolution working from impersonificated context #46897

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

Merged
merged 1 commit into from
Jan 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 33 additions & 12 deletions src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs
Original file line number Diff line number Diff line change
Expand Up @@ -464,15 +464,22 @@ private static Task GetHostEntryOrAddressesCoreAsync(string hostName, bool justR
{
ValidateHostName(hostName);

Task? t;
if (NameResolutionTelemetry.Log.IsEnabled())
{
return justAddresses
? (Task)GetAddrInfoWithTelemetryAsync<IPAddress[]>(hostName, justAddresses)
: (Task)GetAddrInfoWithTelemetryAsync<IPHostEntry>(hostName, justAddresses);
t = justAddresses
? (Task?)GetAddrInfoWithTelemetryAsync<IPAddress[]>(hostName, justAddresses)
: (Task?)GetAddrInfoWithTelemetryAsync<IPHostEntry>(hostName, justAddresses);
}
else
{
return NameResolutionPal.GetAddrInfoAsync(hostName, justAddresses);
t = NameResolutionPal.GetAddrInfoAsync(hostName, justAddresses);
}

// If async resolution started, return task to user. otherwise fall back to sync API on threadpool.
if (t != null)
{
return t;
}
}

Expand All @@ -481,20 +488,34 @@ private static Task GetHostEntryOrAddressesCoreAsync(string hostName, bool justR
RunAsync(s => GetHostEntryCore((string)s), hostName);
}

private static async Task<T> GetAddrInfoWithTelemetryAsync<T>(string hostName, bool justAddresses)
private static Task<T>? GetAddrInfoWithTelemetryAsync<T>(string hostName, bool justAddresses)
where T : class
{
ValueStopwatch stopwatch = NameResolutionTelemetry.Log.BeforeResolution(hostName);
ValueStopwatch stopwatch = ValueStopwatch.StartNew();
Task? task = NameResolutionPal.GetAddrInfoAsync(hostName, justAddresses);

T? result = null;
try
if (task != null)
{
result = await ((Task<T>)NameResolutionPal.GetAddrInfoAsync(hostName, justAddresses)).ConfigureAwait(false);
return result;
return CompleteAsync(task, hostName, stopwatch);
}
finally

// If resolution even did not start don't bother with telemetry.
// We will retry on thread-pool.
return null;

static async Task<T> CompleteAsync(Task task, string hostName, ValueStopwatch stopwatch)
{
NameResolutionTelemetry.Log.AfterResolution(stopwatch, successful: result is not null);
_ = NameResolutionTelemetry.Log.BeforeResolution(hostName);
T? result = null;
try
{
result = await ((Task<T>)task).ConfigureAwait(false);
return result;
}
finally
{
NameResolutionTelemetry.Log.AfterResolution(stopwatch, successful: result is not null);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ public static unsafe string GetHostName()
return new string((sbyte*)buffer);
}

public static unsafe Task GetAddrInfoAsync(string hostName, bool justAddresses)
public static unsafe Task? GetAddrInfoAsync(string hostName, bool justAddresses)
{
GetAddrInfoExContext* context = GetAddrInfoExContext.AllocateContext();

Expand All @@ -166,7 +166,18 @@ public static unsafe Task GetAddrInfoAsync(string hostName, bool justAddresses)
SocketError errorCode = (SocketError)Interop.Winsock.GetAddrInfoExW(
hostName, null, Interop.Winsock.NS_ALL, IntPtr.Zero, &hints, &context->Result, IntPtr.Zero, &context->Overlapped, s_getAddrInfoExCallback, &context->CancelHandle);

if (errorCode != SocketError.IOPending)

if (errorCode == SocketError.TryAgain)
{
// WSATRY_AGAIN indicates possible problem with reachability according to docs.
// However, if servers are really unreachable, we would still get IOPending here
// and final result would be posted via overlapped IO.
// synchronous failure here may signal issue when GetAddrInfoExW does not work from
// impersonated context.
GetAddrInfoExContext.FreeContext(context);
return null;
}
else if (errorCode != SocketError.IOPending)
Copy link
Member

Choose a reason for hiding this comment

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

Did we / could we add any tests that validate name resolution in an impersonated context?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I think this should work. I will explore that and create PR for master.

{
ProcessResult(errorCode, context);
}
Expand Down