Fix SPN used for Negotiate authentication #33426

davidsh · 2018-11-12T18:54:49Z

SocketsHttpHandler was not normalizing the DNS name prior to using it for the SPN
(Service Principal Name). So, when using URI's that involve a CNAME, it was using
the CNAME directly and not evaluating it to the normalized FQDN A record of the host.

This change fixes the behavior to match .NET Framework so that CNAMEs are resolved
properly. We can use the standard Dns.GetHostEntryAsync() API to resolve the name.

From a performance perspective, this additional DNS API call is limited to just
the SPN calculation for NT Auth. Calling this API doesn't impact the performance on the
wire since the OS will cache DNS calls. Wireshark confirms that no additional DNS
protocol packets will be sent.

.NET Framework actually caches the normalized DNS resolution on the ServicePoint object
when it opens up a connections. Thus, it doesn't have to call Dns.GetHostEntryAsync()
for the SPN calculation. While a future PR could further optimize SocketsHttpHandler to
also cache this DNS host name, it isn't clear it would result in measurable performance gain.

I tested this change in a separate Enterprise testing environment I set up. I created
a CNAME for a Windows IIS server in a Windows domain-joined environment and demonstrated that
the Negotiate protocol results in a Kerberos authentication (and doesn't fall back to NTLM).

Fixes #32328

SocketsHttpHandler was not normalizing the DNS name prior to using it for the SPN (Service Principal Name). So, when using URI's that involve a CNAME, it was using the CNAME directly and not evaluating it to the normalized FQDN A record of the host. This change fixes the behavior to match .NET Framework so that CNAMEs are resolved properly. We can use the standard Dns.GetHostEntryAsync() API to resolve the name. From a performance perspective, this additional DNS API call is limited to just the SPN calculation for NT Auth. Calling this API doesn't impact the performance on the wire since the OS will cache DNS calls. Wireshark confirms that no additional DNS protocol packets will be sent. .NET Framework actually caches the normalized DNS resolution on the ServicePoint object when it opens up a connections. Thus, it doesn't have to call Dns.GetHostEntryAsync() for the SPN calculation. While a future PR could further optimize SocketsHttpHandler to also cache this DNS host name, it isn't clear it would result in measurable performance gain. I tested this change in a separate Enterprise testing environment I set up. I created a CNAME for a Windows IIS server in a Windows domain-joined environment and demonstrated that the Negotiate protocol results in a Kerberos authentication (and doesn't fall back to NTLM). Fixes #32328

rmkerr · 2018-11-12T18:59:54Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.NtAuth.cs

+                        string spn;
+                        if (authUri.HostNameType == UriHostNameType.IPv6 || authUri.HostNameType == UriHostNameType.IPv4)
+                        {
+                            spn = "HTTP/" + authUri.IdnHost;


Do we need to place brackets around the IPv6 address to work around #28863?

Do we need to place brackets around the IPv6 address to work around #28863?

I don't think we have tested this scenario. Both .NET Framework and .NET Core currently have this as-is. And my PR doesn't change this.

Practically speaking, NT Auth is unlikely to work in a scenario where IP addresses are used in the URI. IP address based SPNs are not registered by default in Kerberos (Windows Active Directory). So, they would have to be custom registered by an Enterprise. And typically that would not include any brackets. I've never seen Enterprises even use IP addresses for SPNs.

The safe thing to do for this PR is to leave the behavior as-is, and not add brackets right now. But we could open up an issue for further investigation if we want.

In that case this seems like a reasonable behavior.

karelz · 2018-11-12T19:32:27Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.NtAuth.cs

@@ -77,7 +78,23 @@ private static async Task<HttpResponseMessage> SendWithNtAuthAsync(HttpRequestMe

                        string challengeData = challenge.ChallengeData;

-                        string spn = "HTTP/" + authUri.IdnHost;
+                        // Need to use FQDN normalized host so that CNAME's are traversed.


Should we move the comment rather into the else-branch?

I think the comment is in the right place since it describes the logic to calculate the SPN. But I will revise the comment because it needs to describe that we skip DNS lookup in cases of IP literals.

stephentoub · 2018-11-12T19:40:52Z

src/System.Net.Http/src/System.Net.Http.csproj

@@ -599,6 +599,9 @@
    <Compile Include="System\Net\Http\HttpClientHandler.Core.cs" />
    <Compile Include="uap\System\Net\HttpClientHandler.cs" />
  </ItemGroup>
+  <ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp'">
+    <Reference Include="System.Net.NameResolution" />
+  </ItemGroup>


We didn't already have this because we were relying on the underlying implementation (e.g. System.Net.Sockets) to do the original Dns call?

Correct. I needed to add it in order to compile.

stephentoub · 2018-11-12T19:42:27Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.NtAuth.cs

-                        string spn = "HTTP/" + authUri.IdnHost;
+                        // Need to use FQDN normalized host so that CNAME's are traversed.
+                        string spn;
+                        if (authUri.HostNameType == UriHostNameType.IPv6 || authUri.HostNameType == UriHostNameType.IPv4)


Nit: can you store HostNameType into a local so that in the common case we only need to access it once rather than twice? It looks like it caches but even then that it's not super-cheap or inlineable.

stephentoub · 2018-11-12T19:42:56Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.NtAuth.cs

+                        }
+                        else
+                        {
+                            IPHostEntry result = await Dns.GetHostEntryAsync(authUri.IdnHost);


.ConfigureAwait(false)

Is it possible this will throw? If it does, is the correct answer to let that exception propagate, or is there a fallback we should use (e.g. do the same thing that's done in the if block)? I don't know the answer, so I'm asking :)

It would be rare for this to fail because it already succeeded prior to this in order to establish the TCP connection in the first place.

It would be better to let any exception propagate. This is similar to other places in this code that would throw exceptions.

.ConfigureAwait(false)

Fixed.

Ok, thanks.

stephentoub · 2018-11-12T19:45:58Z

src/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/AuthenticationHelper.NtAuth.cs

+                        {
+                            IPHostEntry result = await Dns.GetHostEntryAsync(authUri.IdnHost);
+                            spn = "HTTP/" + result.HostName;
+                        }


Nit: maybe instead of doing:

string spn; if (...) { spn = "HTTP/" + authUri.IdnHost; } else { ... spn = "HTTP/" + result.HostName; }

do:

string spn; if (...) { spn = authUri.IdnHost; } else { ... spn = result.HostName; } spn = "HTTP/" + spn;

?

Why is this better? Is seems slower because we are doing two assignments of spn field.

I don't think it is really less lines of IL or memory because the "HTTP/" string should only occur once.

Why is this better? Is seems slower because we are doing two assignments of spn field.

From my perspective it's better because it's centralizing some logic, namely the concatenation of the "HTTP/" prefix. There's also no additional field assignment, as spn here is a local, not a field. And the IL is a bit longer otherwise, as there are two calls to Concat instead of one:
https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQxASwDYB8ACAGAAhwEYBuAWACh8jiA6AOThgsqpwGYiAmAgYQIBvKgVFEuJQgFliACgCqCNAQCuSgJQixwygEgxBRWgASAe1gMIYOABUAngAc4BABYA7GAQC8qpXTMWVraOcKwGBpIEUA5uYeGiaABmBLLunl4+RgEwltb2TnQAkgAKAG4ALARYWK4e3plK2bnBBSWlAGyalPGiOj3h0W7eBABExjY2xQD0IwQA1L5oRQAmbtlx8QC+WvFwGFBwO+F9/WKDw2MT07MLACJuUHQA4szZAKIeCHayakuFq9l1P5zDkghtwttuj0cAB2KIxcGQgxHTi0aTcBRKRZdAw6fRiLIg5r5ZxpYa/YGBPIhcERYiEQa0sRJFJkjKGRpEoIkoplSrVWrpBomLnU1plTpHXFSnrnHwU/5rEFM0RInp7A4y3pagYxYb3R4vGDvT7fBUAkFAppgnVq+Jy0bjSYzebw2I62FuxFUTZAA=

But it's just a small thing. Whatever you prefer is fine.

Ok. I'll make the change.

And I like the https://sharplab.io example! That is a nice website for this kind of analysis.

stephentoub

Thanks.

davidsh · 2018-11-12T20:49:44Z

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test Outerloop OSX x64 Debug Build

wfurt

LGTM.

davidsh · 2018-11-12T22:46:55Z

CI failures unrelated to PR.

SocketsHttpHandler was not normalizing the DNS name prior to using it for the SPN (Service Principal Name). So, when using URI's that involve a CNAME, it was using the CNAME directly and not evaluating it to the normalized FQDN A record of the host. This change fixes the behavior to match .NET Framework so that CNAMEs are resolved properly. We can use the standard Dns.GetHostEntryAsync() API to resolve the name. From a performance perspective, this additional DNS API call is limited to just the SPN calculation for NT Auth. Calling this API doesn't impact the performance on the wire since the OS will cache DNS calls. Wireshark confirms that no additional DNS protocol packets will be sent. .NET Framework actually caches the normalized DNS resolution on the ServicePoint object when it opens up a connections. Thus, it doesn't have to call Dns.GetHostEntryAsync() for the SPN calculation. While a future PR could further optimize SocketsHttpHandler to also cache this DNS host name, it isn't clear it would result in measurable performance gain. I tested this change in a separate Enterprise testing environment I set up. I created a CNAME for a Windows IIS server in a Windows domain-joined environment and demonstrated that the Negotiate protocol results in a Kerberos authentication (and doesn't fall back to NTLM). Fixes #32328

SocketsHttpHandler was not normalizing the DNS name prior to using it for the SPN (Service Principal Name). So, when using URI's that involve a CNAME, it was using the CNAME directly and not evaluating it to the normalized FQDN A record of the host. This change fixes the behavior to match .NET Framework so that CNAMEs are resolved properly. We can use the standard Dns.GetHostEntryAsync() API to resolve the name. From a performance perspective, this additional DNS API call is limited to just the SPN calculation for NT Auth. Calling this API doesn't impact the performance on the wire since the OS will cache DNS calls. Wireshark confirms that no additional DNS protocol packets will be sent. .NET Framework actually caches the normalized DNS resolution on the ServicePoint object when it opens up a connections. Thus, it doesn't have to call Dns.GetHostEntryAsync() for the SPN calculation. While a future PR could further optimize SocketsHttpHandler to also cache this DNS host name, it isn't clear it would result in measurable performance gain. I tested this change in a separate Enterprise testing environment I set up. I created a CNAME for a Windows IIS server in a Windows domain-joined environment and demonstrated that the Negotiate protocol results in a Kerberos authentication (and doesn't fall back to NTLM). Fixes dotnet/corefx#32328 Commit migrated from dotnet/corefx@684114f

davidsh added the area-System.Net.Http.SocketsHttpHandler label Nov 12, 2018

davidsh added this to the 3.0 milestone Nov 12, 2018

davidsh self-assigned this Nov 12, 2018

davidsh requested review from stephentoub, geoffkizer and a team November 12, 2018 18:54

davidsh added the tenet-compatibility Incompatibility with previous versions or .NET Framework label Nov 12, 2018

rmkerr reviewed Nov 12, 2018

View reviewed changes

rmkerr approved these changes Nov 12, 2018

View reviewed changes

karelz approved these changes Nov 12, 2018

View reviewed changes

stephentoub reviewed Nov 12, 2018

View reviewed changes

stephentoub approved these changes Nov 12, 2018

View reviewed changes

Address PR feedback

93d6e81

wfurt approved these changes Nov 12, 2018

View reviewed changes

davidsh merged commit 684114f into dotnet:master Nov 12, 2018

davidsh deleted the spn_fix_30 branch November 12, 2018 22:47

OpenSpacesAndPlaces mentioned this pull request Jan 17, 2020

Support security mode TransportWithMessageCredential dotnet/wcf#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SPN used for Negotiate authentication #33426

Fix SPN used for Negotiate authentication #33426

davidsh commented Nov 12, 2018

rmkerr Nov 12, 2018

davidsh Nov 12, 2018 •

edited

Loading

rmkerr Nov 12, 2018

karelz Nov 12, 2018

davidsh Nov 12, 2018

stephentoub Nov 12, 2018

davidsh Nov 12, 2018

stephentoub Nov 12, 2018

davidsh Nov 12, 2018

stephentoub Nov 12, 2018

stephentoub Nov 12, 2018

davidsh Nov 12, 2018

davidsh Nov 12, 2018

stephentoub Nov 12, 2018

stephentoub Nov 12, 2018

davidsh Nov 12, 2018

stephentoub Nov 12, 2018

davidsh Nov 12, 2018

stephentoub left a comment

davidsh commented Nov 12, 2018

wfurt left a comment

davidsh commented Nov 12, 2018

Fix SPN used for Negotiate authentication #33426

Fix SPN used for Negotiate authentication #33426

Conversation

davidsh commented Nov 12, 2018

Choose a reason for hiding this comment

davidsh Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

davidsh commented Nov 12, 2018

wfurt left a comment

Choose a reason for hiding this comment

davidsh commented Nov 12, 2018

davidsh Nov 12, 2018 •

edited

Loading