Clarify default credentials usage for HttpClientHandler on UAP #21672

davidsh · 2017-06-28T18:51:40Z

This PR clarifies how credential handling and specifically default
logged-on credentials actually works on the UAP platform.

First, "default credentials" are the current credentials of the
logged-on user, typically on an enterprise network joined to an
Active Directory domain. When these credentials are sent to a
proxy or server, only Negotiate, Kerberos, or NTLM schemes are
used. Those credentials can never be sent via other schemes
like basic or digest.

.NET Framework traditionally has two ways for developers to specify
using default logged-on credentials to send to the server. Either
set the .UseDefaultCredentials property to true or specify
CredentialCache.DefaultCredentials to the .Credentials property.

For proxy credentials, the developer could pass DefaultCredentials
via the custom IProxy object or via the .DefaultProxyCredentials
property.

.NET Framework would send default credentials anywhere (intranet
or internet) if specified by the developer.

The UAP platform implementation of System.Net.Http uses a different
set of rules for sending default credentials. It is based on whether
the app has Enterprise Authentication capability as well as if the
endpoint is specified in an intranet zone. This design is part of
the base native WinInet and WinHTTP stacks. It was a design choice
to provide optimum security as well as convenience for developers
writing apps for enterprise scenarios.

This PR does a few things. First, it keeps the .NET ICredentials
properties stored separately so that they correctly round-trip
on get and set. .NET NetworkCredential has 3 parameters (domain,
user, password). But WinRT PasswordCredential only has 2 parameters
(user, password). We now convert to the WinRT objects only when
we need to use them.

Second, this PR clarifies that the .UseDefaultCredentials property
is basically a no-op because the API models of default credentials
handling is different on the UAP platform as described above.

These platform differences will be documented here:
https://github.com/dotnet/corefx/wiki/ApiCompat

Fixes #19642
Fixes #21510

This PR clarifies how credential handling and specifically default logged-on credentials actually works on the UAP platform. First, "default credentials" are the current credentials of the logged-on user, typically on an enterprise network joined to an Active Directory domain. When these credentials are sent to a proxy or server, only Negotiate, Kerberos, or NTLM schemes are used. Those credentials can never be sent via other schemes like basic or digest. .NET Framework traditionally has two ways for developers to specify using default logged-on credentials to send to the server. Either set the .UseDefaultCredentials property to true or specify CredentialCache.DefaultCredentials to the .Credentials property. For proxy credentials, the developer could pass DefaultCredentials via the custom IProxy object or via the .DefaultProxyCredentials property. .NET Framework would send default credentials anywhere (intranet or internet) if specified by the developer. The UAP platform implementation of System.Net.Http uses a different set of rules for sending default credentials. It is based on whether the app has Enterprise Authentication capability as well as if the endpoint is specified in an intranet zone. This design is part of the base native WinInet and WinHTTP stacks. It was a design choice to provide optimum security as well as convenience for developers writing apps for enterprise scenarios. This PR does a few things. First, it keeps the .NET ICredentials properties stored separately so that they correctly round-trip on get and set. .NET NetworkCredential has 3 parameters (domain, user, password). But WinRT PasswordCredential only has 2 parameters (user, password). We now convert to the WinRT objects only when we need to use them. Second, this PR clarifies that the .UseDefaultCredentials property is basically a no-op because the API models of default credentials handling is different on the UAP platform as described above. These platform differences will be documented here: https://github.com/dotnet/corefx/wiki/ApiCompat Fixes #19642 Fixes #21510

davidsh · 2017-06-28T18:52:12Z

cc: @karelz

DavidGoll · 2017-06-28T22:58:17Z

src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs

+        {
+            // The WinRT PasswordCredential object does not have a special credentials value for "default credentials".
+            // In general, the UWP HTTP platform automatically manages sending default credentials, if no explicit
+            // credential was specificed, based on if the app has EnterpriseAuthentication capability and if the endpoint


specificed [](start = 30, length = 10)

typo: specificed

I will fix that typo in the next PR this week since I'm editing the same file.

CIPop · 2017-06-29T00:08:06Z

These platform differences will be documented here:
https://github.com/dotnet/corefx/wiki/ApiCompat

Is there a doc bug tracking the changes?

davidsh · 2017-06-29T00:12:04Z

Is there a doc bug tracking the changes?

There is no doc bug because I plan to update that document right after I merge the PR. That is what I did for the last few PRs. The document is a wiki and can be edited by anyone on GitHub.

davidsh · 2017-06-29T00:13:14Z

@mmitche @stephentoub I noticed that the Windows runs have been running 5+ hours now. Is there something wrong with CI?

CIPop · 2017-06-29T00:09:27Z

src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs

-        {
-            // We don't support changing the proxy settings in the NETNative version of HttpClient since it's layered on
-            // WinRT HttpClient. But we do support passing in explicit proxy credentials, if specified, which we can
-            // get from the specified or default proxy.


Doesn't this remove the " But we do support passing in explicit proxy credentials, if specified, which we get from the specified or default proxy" functionality?

I don't understand your comment. Are you saying I should put back the comment or revise it in some way?

Sorry, I forgot to remove this from the review: I did see that the code handling explicit proxy credentials has been moved (not removed as I've originally thought).

CIPop · 2017-06-29T00:12:17Z

src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs

@@ -167,99 +168,48 @@ public bool PreAuthenticate

        public bool UseDefaultCredentials
        {
-            get { return Credentials == null; }
+            get { return false; }


I thought that if the app has Enterprise Auth it will use default credentials and credentials haven't been set. Can we somehow implement capability detection and return the correct state?

Enterprise Auth is not the only thing that determines if default credentials are used. It also depends on the endpoint itself and whether or not it is listed in the intranet zones.

CIPop · 2017-06-29T00:13:16Z

src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs

-                }
-                else if (_rtFilter.ServerCredential == null)
-                {
-                    // The only way to disable default credentials is to provide credentials.


Wasn't this comment true? If yes, it would allow apps to disable DefaultCredentials.

The comment was not true which is why it is being removed.

CIPop · 2017-06-29T00:16:55Z

src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs

+            // empty values for username and password) indicates that there is no explicit credential. And that means
+            // that the default logged-on credentials might be sent to the endpoint.
+            //
+            // There is currently no WinRT API to turn off sending default credentials other than the capability


See other comment: isn't setting user/pass credentials going to disable default credentials?

Yes, in general setting a non-null credential will always prevent default credentials from being sent since there is an explicit credential. However, having a "null" credential assigned to the properties doesn't mean "no credential". It simply means "no explicit credential" and the end result can be either no credential is sent, or a default logged-on credentials is sent.

I see: so basically we cannot implement DefaultCredentials=false because it needs to send at least some fake, non-null user-name.

so basically we cannot implement DefaultCredentials=false because it needs to send at least some fake, non-null user-name.

Correct. And although I thought about doing that (creating a "fake" username, i.e. guid-string), it would actually break app-compat behavior with prior UWP .NET Core (1.0, 1.1, etc.). It is better to keep the behavior and document the platform differences.

CIPop · 2017-06-29T00:19:02Z

src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs

+                // with empty strings for username and password inside the object. However, one can't assign
+                // empty strings to those properties; otherwise, it will throw an error.
+                RTPasswordCredential rtCreds = new RTPasswordCredential();
+                if (!string.IsNullOrEmpty(networkCred.UserName))


Instead of calling multiple COM pinvokes why not just decide first if the user/pwd are empty and then call the right RTPasswordCredential ctor?

I'll look into doing that.

See comment above.

CIPop · 2017-06-29T00:20:13Z

src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs

+                NetworkCredential networkCred = (NetworkCredential)creds;
+
+                // Creating a new WinRT PasswordCredential object with the default constructor ends up
+                // with empty strings for username and password inside the object. However, one can't assign


According to docs this shouldn't be true: only the user must be not null: https://docs.microsoft.com/en-us/uwp/api/windows.security.credentials.passwordcredential

So, I actually re-tested this. The documentation is wrong. That is why I coded it that way in the first place.

Using a null for either the username or password parameter in the constructor or setting the property gives:

"System.ArgumentNullException: Value cannot be null."

Using an empty string for either the username or password parameter in the constructor or setting the property gives:

"System.ArgumentException: The parameter is incorrect."

So, that is why I create the RTPasswordCredential with default constructor first. And then assign the properties only if non-null and non-empty.

You should open a bug or correct the docs then.

Opened internal bug: 12569816.

CIPop · 2017-06-29T00:21:04Z

src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs

+
+        private void SetFilterProxyCredential()
+        {
+            // We don't support changing the proxy settings in the NETNative version of HttpClient since it's layered on


Nit: it's the UAP version as well. NetNative shouldn't have any differences from UAP.

good point. I can revise that comment.

I may revise that comment in the next PR depending on if I can get this PR merged since CI is acting weird.

CIPop

There may be one potential perf improvement when initializing the PasswordCredential.

LGTM

davidsh · 2017-06-29T01:14:06Z

6 hours and counting:
https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/554/
https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netcoreapp+CGroup_Release+AGroup_x86+TestOuter_false_prtest/554/

davidsh · 2017-06-29T01:14:28Z

@dotnet-bot Test Windows x64 Debug Build
@dotnet-bot Test Windows x86 Release Build

davidsh · 2017-06-29T02:21:17Z

Updated https://github.com/dotnet/corefx/wiki/ApiCompat

mmitche · 2017-06-29T15:05:37Z

@dotnet/dnceng PTAL. This might be related to the scale set issues from yesterday?

sunandabalu · 2017-06-29T19:20:39Z

Scale sets have been updated and redeployed. Please retry failed/blocked build.

Clarify default credentials usage for HttpClientHandler on UAP Commit migrated from dotnet/corefx@d3f9f21

davidsh added area-System.Net.Http os-windows-uwp labels Jun 28, 2017

davidsh added this to the UWP6.0 milestone Jun 28, 2017

davidsh self-assigned this Jun 28, 2017

davidsh requested review from stephentoub, danmoseley, CIPop and DavidGoll June 28, 2017 18:51

dnfclas added the cla-already-signed label Jun 28, 2017

stephentoub approved these changes Jun 28, 2017

View reviewed changes

DavidGoll reviewed Jun 28, 2017

View reviewed changes

DavidGoll approved these changes Jun 28, 2017

View reviewed changes

CIPop reviewed Jun 29, 2017

View reviewed changes

karelz modified the milestones: UWP6.0, 2.1.0 Jun 29, 2017

CIPop approved these changes Jun 29, 2017

View reviewed changes

davidsh merged commit d3f9f21 into dotnet:master Jun 29, 2017

davidsh deleted the sdk_uwp branch June 29, 2017 02:13

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#21672 from davidsh/sdk_uwp

e9fe6aa

Clarify default credentials usage for HttpClientHandler on UAP Commit migrated from dotnet/corefx@d3f9f21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify default credentials usage for HttpClientHandler on UAP #21672

Clarify default credentials usage for HttpClientHandler on UAP #21672

davidsh commented Jun 28, 2017

davidsh commented Jun 28, 2017

DavidGoll Jun 28, 2017

davidsh Jun 29, 2017

CIPop commented Jun 29, 2017

davidsh commented Jun 29, 2017

davidsh commented Jun 29, 2017

CIPop Jun 29, 2017

davidsh Jun 29, 2017

CIPop Jun 29, 2017

CIPop Jun 29, 2017

davidsh Jun 29, 2017

CIPop Jun 29, 2017

davidsh Jun 29, 2017

CIPop Jun 29, 2017

davidsh Jun 29, 2017

CIPop Jun 29, 2017

davidsh Jun 29, 2017

CIPop Jun 29, 2017

davidsh Jun 29, 2017

davidsh Jun 29, 2017

CIPop Jun 29, 2017

davidsh Jun 29, 2017 •

edited

CIPop Jun 29, 2017

davidsh Jun 29, 2017

CIPop Jun 29, 2017

davidsh Jun 29, 2017

davidsh Jun 29, 2017

CIPop left a comment

davidsh commented Jun 29, 2017

davidsh commented Jun 29, 2017

davidsh commented Jun 29, 2017

mmitche commented Jun 29, 2017

sunandabalu commented Jun 29, 2017

Clarify default credentials usage for HttpClientHandler on UAP #21672

Clarify default credentials usage for HttpClientHandler on UAP #21672

Conversation

davidsh commented Jun 28, 2017

davidsh commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CIPop commented Jun 29, 2017

davidsh commented Jun 29, 2017

davidsh commented Jun 29, 2017

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

davidsh Jun 29, 2017 • edited

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

CIPop left a comment

Choose a reason for hiding this comment

davidsh commented Jun 29, 2017

davidsh commented Jun 29, 2017

davidsh commented Jun 29, 2017

mmitche commented Jun 29, 2017

sunandabalu commented Jun 29, 2017

davidsh Jun 29, 2017 •

edited