Add DangerousAcceptAnyServerCertificateValidator property to HttpClient by stephentoub · Pull Request #19908 · dotnet/corefx

stephentoub · 2017-05-17T21:18:51Z

It's a common pattern in certain situations (in particular in tests) to use HttpClient to connect to a server with a certificate that shouldn't be validated, e.g. a self-signed cert. This is commonly achieved with HttpClientHandler by setting the ServerCertificateCustomValidationCallback property to a delegate that always returns true (returning true means that the cert passed validation):

handler.ServerCertificateCustomValidationCallback = delegate { return true; };

However, we can't support this callback on all implementations of HttpClientHandler. For example, on macOS, the default libcurl built against SecureTransport doesn't provide the callback we'd need to properly implement the callback, and as such, today trying to set up such a callback results in a PlatformNotSupportedException. This blocks some important scenarios; that includes testing (e.g. ASP.NET had to move away from HttpClient because of this, aspnet/MetaPackages#100) but also higher level libraries and tools that provide similar functionality (e.g. PowerShell's Invoke-WebRequest -SkipCertificateCheck is blocked by this PowerShell/PowerShell#3648).

This PR adds an approved API to address this case:

public static Func<HttpRequestMessage, X509Certificate2, X509Chain, SslPolicyErrors, bool> DangerousAcceptAnyServerCertificateValidator { get; }

The actual implementation of this property is trivial, simply returning a cached always-return-true delegate:

public static Func<HttpRequestMessage, X509Certificate2, X509Chain, SslPolicyErrors, bool> DangerousAcceptAnyServerCertificateValidator { get; } = delegate { return true; };

so developers can just substitute it for their custom delegate:

handler.ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAnyServerCertificateValidator;

The benefit of that is it gives HttpClientHandler implementations a known object reference identity that can be compared against to understand the developer's intention: if the object stored in ServerCertificateCustomValidationCallback is reference equals to DangerousAcceptAnyServerCertificateValidator, then we know that it will always return true, and on platforms where we'd otherwise throw a PlatformNotSupportedException because we can't hook up a callback, we can still entirely disable validation. This enables such scenarios to work. (As a side benefit, such a property means developers can use this property and give tools an easier opportunity for flagging the danger of such disabling of validation, making it easier for developers to avoid shipping insecure applications.)

This PR:

Adds the property and the singleton delegate implementation.
Adds some tests to ensure it behaves correctly.
On Unix, if we would throw a PNSE because a callback was supplied, we special-case DangerousAcceptAnyServerCertificateValidator to avoid throwing. No changes were made to the implementation on Windows.

In the future, we could choose to go further and avoid hooking up the delegate at all on both Windows and Unix even if callbacks are supported, as doing so may save some overhead. However, as this is not meant to be a common case in production, I've only modified code necessary to get these scenarios working.

Fixes https://github.com/dotnet/corefx/issues/19709
Related to https://github.com/dotnet/corefx/issues/19718

cc: @bartonjs, @davidsh, @CIPop, @Priya91, @wfurt, @geoffkizer

davidsh · 2017-05-17T21:30:24Z

As a side benefit, such a property means developers can use this property and give tools an easier opportunity for flagging the danger of such disabling of validation, making it easier for developers to avoid shipping insecure applications

We should do two things to support the above:

Modify FxCop security rules to flag this API use as insecure.
Modify UWP WACK tools to flag this API and prevent submittal to the Windows App Store.

@morganbr - what is the process for handling FxCop changes?

The UWP WACK tool modification will be handled separately via a Windows Platform change process

stephentoub · 2017-05-17T21:34:33Z

Modify UWP WACK tools to flag this API and prevent submittal to the Windows App Store.

For the record, I still don't think we should do this. All it does is add friction to developers without actually preventing anything. But I'll leave it at that.

davidsh

LGTM

CIPop

Left a few questions.

CIPop · 2017-05-17T21:47:48Z

+                    {
+                        EventSourceTrace("Disabling peer and host verification per {0}", nameof(HttpClientHandler.DangerousAcceptAnyServerCertificateValidator), easy: easy);
+                        easy.SetCurlOption(Interop.Http.CURLoption.CURLOPT_SSL_VERIFYPEER, 0); // don't verify the peer
+                        easy.SetCurlOption(Interop.Http.CURLoption.CURLOPT_SSL_VERIFYHOST, 0); // don't verify the peer cert's hostname


Should CRL also be deactivated here?

It is by default. Nothing need be changed. In fact we throw PNSE if you try to enable it and can't.

CIPop · 2017-05-17T21:53:24Z

+                switch (answer)
+                {
+                    case CURLcode.CURLE_OK:
+                        // We successfully registered.  If we'll be invoking a user-provided callback to verify the server


I'm a bit confused by the comment: I thought the callback will be invoked internally regardless if the user provided a callback or not (if the public API is null) to perform the chain verification then the name verification.

Because if this isn't true: wouldn't we keep the host name validation enabled?

We do the validation in our callback: if libcurl does it and fails, we never get the callback.

That's what I thought. The comment seems out of sync: it should not have that "If".
Maybe something on the lines of "we'll disable libcurl's verification of the host name and perform it within the callback" would be clearer.

I'll fix the comment. Thanks.

CIPop · 2017-05-17T21:56:41Z

        public System.Security.Cryptography.X509Certificates.X509CertificateCollection ClientCertificates { get { throw null; } }
        public System.Net.CookieContainer CookieContainer { get { throw null; } set { } }
        public System.Net.ICredentials Credentials { get { throw null; } set { } }
+        public static System.Func<System.Net.Http.HttpRequestMessage, System.Security.Cryptography.X509Certificates.X509Certificate2, System.Security.Cryptography.X509Certificates.X509Chain, System.Net.Security.SslPolicyErrors, bool> DangerousAcceptAnyServerCertificateValidator { get { throw null; } }


We don't need to add ifdefs here to limit to core right? (I remember a time when we used to do that in ref but I'm not yet caught up with recent changes.)

CIPop · 2017-05-17T21:59:38Z

+        }
+
+        [Theory]
+        [InlineData(SslProtocols.Tls, false)]


I'm not against coverage but I don't understand why this test is varying the protocol. If there is a particular reason a comment would be useful (we should have different tests varying the protocol already).

I can cull it back and add a comment. It was mainly to ensure we're appropriately setting version constraints even when the callback is applied (since I know in the implementation that has to be done in particular places).

morganbr · 2017-05-17T22:08:32Z

@davidsh This could be a good rule for https://github.com/dotnet/roslyn-analyzers . I'm not sure if new FXCop rules are still being added. I wouldn't be in favor of adding it to the WACK, however. The WACK prevents using code that won't function or would be dangerous to UWP sandboxing. It doesn't prevent developers from having poorly chosen patterns.

bartonjs · 2017-05-17T22:04:56Z

  </data>
  <data name="net_http_libcurl_callback_notsupported" xml:space="preserve">
-    <value>The handler does not support custom handling of certificates with this combination of libcurl ({0}) and its SSL backend ("{1}").</value>
+    <value>The handler does not support custom validation of certificates with this combination of libcurl ({0}) and its SSL backend ("{1}"). A libcurl built with OpenSSL is required.</value>


When I made these three messages I was very much explicitly trying to avoid saying OpenSSL. I don't quite remember why, but that was my goal.

(Maybe because #19718 was still several thought experiments away?)

I think we should be explicit about it. Without saying OpenSSL, we don't give the developer guidance on how to avoid the problem. By adding the phrase, we're not only telling them what's not supported and why, we're telling them how to fix it.

Right now on macOS using libcurl+openssl gains nothing. And even if I can get my gedankenexperiment working today the clientcerts message will still be misleading (since it will make callback work, but not clientcerts).

So mainly I'm concerned with macOS users saying "it didn't work, and I followed the exception's instructions, and it gave me the exact same error".

Hmm, ok, that's fair. I guess we could have different messages for macOS and Linux.

bartonjs · 2017-05-17T22:39:51Z

+                    {
+                        EventSourceTrace("Disabling peer and host verification per {0}", nameof(HttpClientHandler.DangerousAcceptAnyServerCertificateValidator), easy: easy);
+                        easy.SetCurlOption(Interop.Http.CURLoption.CURLOPT_SSL_VERIFYPEER, 0); // don't verify the peer
+                        easy.SetCurlOption(Interop.Http.CURLoption.CURLOPT_SSL_VERIFYHOST, 0); // don't verify the peer cert's hostname


darwinssl, at least, uses VERIFYHOST=1 as "should I enable SNI". So it would be good if we can leave this on. But maybe that causes it to still reject the self-signed cert if the hostname doesn't match, in which case... cutting SNI is a sad side effect that we may need to write down. (Though I don't know what the state of SNI is for us on Linux or Windows... so maybe this is "a nice feature of macOS might get turned off"))

Though I don't know what the state of SNI is for us on Linux or Windows

SNI should work on Windows for the client side in all components (SslStream, HttpClient, etc)

I'll double check, but I believe when I tested it with the default of CURLOPT_SSL_VERIFYHOST==1, it still failed if the hostname didn't match, in which case we'll need to set it to 0. If that means SNI doesn't work when this is used, we can doc it.

I tried removing the CURLOPT_SSL_VERIFYHOST=0 setting. With an NSS backend, VERIFYPEER=0 automatically implies VERIFYHOST=0 (and there's no way to override that), so it didn't affect the CentOS/Fedora/RHEL VMs with have in CI, as they have an NSS backend. With a GnuTLS backend on Ubuntu 16.10, and without VERIFYHOST=0, and connecting to a site with an incorrect host name, we get:

[Microsoft-System-Net-Http-12] (65, 64, CurlDebugFunction, CURLINFO_TEXT: subjectAltName does not match wrong.host.badssl.com). [Microsoft-System-Net-Http-12] (65, 64, CurlDebugFunction, CURLINFO_TEXT: SSL: no alternative certificate subject name matches target host name 'wrong.host.badssl.com').

which then fails the request with:

SSL peer certificate or SSH remote key was not OK

Similar error for all of our localhost ssl tests. So I at least need to leave the VERIFYHOST=0 part in for the Linux CurlHandler.SslProvider.cs. However, CI passed on macOS without setting VERIFYHOST, so maybe we could get away without it? But I'm not sure why this didn't fail. With VERIFYHOST at its default value, libcurl should be calling Apple's SSLSetPeerDomainName function:
https://github.com/curl/curl/blob/master/lib/vtls/darwinssl.c#L1536
and that should cause the connection to fail if the hostname doesn't match.

CIPop · 2017-05-17T22:59:45Z

@davidsh This could be a good rule for https://github.com/dotnet/roslyn-analyzers . I'm not sure if new FXCop rules are still being added. I wouldn't be in favor of adding it to the WACK, however. The WACK prevents using code that won't function or would be dangerous to UWP sandboxing. It doesn't prevent developers from having poorly chosen patterns.

@morganbr @davidsh I've too raised some questions about WACK. Wouldn't it be simpler to just continue the discussion in #19709

I've thought the matter was closed and both Dev, API review board and Security signed off on @stephentoub's proposal. My signoff above was made with that assumption.

gkhanna79 · 2017-05-18T14:50:40Z

@stephentoub Can you PTAL at the outerloop failures?

stephentoub · 2017-05-18T15:15:08Z

@gkhanna79, the failure is unrelated: it's #19702, a fix for which is up at #19920. I'm going to run a few more legs, though.

…ntHandler

In situations where setting ServerCertificateValidationCallback would result in a PlatformNotSupportedException, if it was set to DangerousAcceptAnyServerCertificateValidator instead simply disable CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST. This enables a common case of `delegate { return true; }` to work on libcurls with non-SSL backends, such as the default on macOS.

stephentoub · 2017-05-18T18:11:13Z

@dotnet-bot test outerloop netcoreapp Windows_NT Debug
@dotnet-bot test outerloop netcoreapp Ubuntu14.04 Debug
@dotnet-bot test outerloop netcoreapp Ubuntu16.10 Debug
@dotnet-bot test outerloop netcoreapp OSX10.12 Debug
@dotnet-bot test outerloop netcoreapp RHEL7.2 Debug

dnfclas added the cla-already-signed label May 17, 2017

davidsh added the area-System.Net.Http label May 17, 2017

davidsh added this to the 2.0.0 milestone May 17, 2017

stephentoub force-pushed the ssl_callback branch from e596085 to fe0179c Compare May 17, 2017 21:32

davidsh approved these changes May 17, 2017

View reviewed changes

stephentoub force-pushed the ssl_callback branch from fe0179c to c104df0 Compare May 17, 2017 21:57

CIPop reviewed May 17, 2017

View reviewed changes

bartonjs reviewed May 17, 2017

View reviewed changes

CIPop approved these changes May 17, 2017

View reviewed changes

bartonjs approved these changes May 17, 2017

View reviewed changes

karelz assigned stephentoub May 18, 2017

stephentoub force-pushed the ssl_callback branch 2 times, most recently from f32ae4a to 483a9d9 Compare May 18, 2017 12:44

stephentoub added 3 commits May 18, 2017 13:49

Add DangerousAcceptAnyServerCertificateValidator property to HttpClie…

2a94352

…ntHandler

Remove unused resource string

513426e

stephentoub force-pushed the ssl_callback branch from 483a9d9 to 513426e Compare May 18, 2017 18:08

Priya91 approved these changes May 18, 2017

View reviewed changes

stephentoub merged commit ea706ea into dotnet:master May 18, 2017

stephentoub deleted the ssl_callback branch May 18, 2017 19:23

stephentoub mentioned this pull request May 18, 2017

release/2.0: Port "Add DangerousAcceptAnyServerCertificateValidator property to HttpClient" #19941

Merged

karelz modified the milestones: 2.1.0, 2.0.0 May 19, 2017

stephentoub added the netfx-port-consider label Sep 15, 2017

mitch-b mentioned this pull request Mar 21, 2018

Issue with Login API? DotNetDevs/Synology#115

Open

snakefoot mentioned this pull request May 17, 2018

All Splunk logs fail AlanBarber/NLog.Targets.Splunk#10

Closed

sagnihotri-cohesity mentioned this pull request Aug 22, 2018

Project file changes to make Cohesity PowerShell Module work with PS core 6.0 on Mac OS. cohesity/cohesity-powershell-module#7

Merged

This was referenced Dec 9, 2020

CA5359: Apply to HttpClientHandler.ServerCertificateCustomValidationCallback dotnet/roslyn-analyzers#4542

Open

Fix S4830 FN: Match DangerousAcceptAnyServerCertificateValidator SonarSource/sonar-dotnet#3824

Closed

Nisden mentioned this pull request Aug 13, 2021

The remote certificate is invalid [I know its duplicated] Xabaril/AspNetCore.Diagnostics.HealthChecks#804

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DangerousAcceptAnyServerCertificateValidator property to HttpClient#19908

Add DangerousAcceptAnyServerCertificateValidator property to HttpClient#19908
stephentoub merged 3 commits intodotnet:masterfrom
stephentoub:ssl_callback

stephentoub commented May 17, 2017

davidsh commented May 17, 2017 •

edited

Loading

stephentoub commented May 17, 2017

davidsh left a comment

CIPop left a comment

CIPop May 17, 2017

stephentoub May 17, 2017

CIPop May 17, 2017

stephentoub May 17, 2017

CIPop May 17, 2017

stephentoub May 18, 2017

CIPop May 17, 2017

stephentoub May 17, 2017

CIPop May 17, 2017

stephentoub May 18, 2017

morganbr commented May 17, 2017

bartonjs May 17, 2017

bartonjs May 17, 2017

stephentoub May 18, 2017

bartonjs May 18, 2017

stephentoub May 18, 2017

bartonjs May 17, 2017

CIPop May 17, 2017

stephentoub May 18, 2017

stephentoub May 18, 2017

CIPop commented May 17, 2017

gkhanna79 commented May 18, 2017

stephentoub commented May 18, 2017

stephentoub commented May 18, 2017

Labels

9 participants

Conversation

stephentoub commented May 17, 2017

davidsh commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

stephentoub commented May 17, 2017

davidsh left a comment

Choose a reason for hiding this comment

CIPop left a 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

morganbr commented May 17, 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

CIPop commented May 17, 2017

gkhanna79 commented May 18, 2017

stephentoub commented May 18, 2017

stephentoub commented May 18, 2017

Labels

9 participants

davidsh commented May 17, 2017 •

edited

Loading