Runtime: Proposal: HttpClientHandler.DangerousAcceptAnyServerCertificateValidator property

[stephentoub]

We have what in my mind is a big problem with HttpClient in 2.0: to use HttpClient with self-signed certs (very common in testing environments), you need to hook up a ServerCertificateCustomValidationCallback; that will override the default verification and can be made to always return true. We do this in all of our HTTP testing in corefx, ASP.NET did it in theirs, etc.

Unfortunately, this doesn't work on macOS nor on several Linux distros. The problem is that libcurl only supports the necessary callbacks to enable this when using an OpenSSL backend, and if such a backend isn't in use, we can't enable this delegate callback and thus throw a PlatformNotSupportedException when trying to hook up the delegate.

As it stands today, basically none of our https testing for HttpClient is running on macOS/CentOS/RHEL/Fedora, ASP.NET recently had to switch away from HttpClient for their testing because of this, etc.

Proposal:
We add the following property to HttpClientHandler:
public class HttpClientHandler
{
public bool ValidateServerCertificate { get; set; } = true;
...
}
The default behavior is as things are today. But if you set ValidateServerCertificate to false, we disable verification, both built-in and custom via the callback. We should be able to do that on all Unix's via libcurl's https://curl.haxx.se/libcurl/c/CURLOPT_SSL_VERIFYPEER.html, and we can do so on Windows simply by substituting a delegate { return true; } delegate (or tweak a setting if there is one).

Updated proposal (5/13/2017):
We add the following property to HttpClientHandler:
```C#
public class HttpClientHandler
{
public static Func DangerousAcceptAllCertificatesValidator { get; } = delegate { return true; };
...
}

A developer can use this delegate with 
```C#
handler.ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAllCertificatesValidator;

For platforms that already support the callback, nothing need change, although they could be optimized to specially recognize this delegate instance to avoid the callback needing to be invoked. For platforms that don't support the callback, we can use object equality to check for this delegate instance: if any other instance is supplied, we continue to throw a PNSE, but if this instance is supplied, we flip the appropriate options so that certificates are not validated and all are accepted.

cc: @bartonjs, @davidsh, @cesarbs, @CIPop

[blowdart]

ValidateServerCertificate to false, we disable verification, both built-in and custom via the callback

Oh heck no. This seems a nightmare scenario, because people are just going to turn it on and not think, and deploy like that.

Surely, rather than use this nuclear option, we could, when a validation callback is configured, we could build the chain manually, even if it is slow

[stephentoub]

when a validation callback is configured, we could build the chain manually

When? We don't even get a chance; libcurl doesn't call back to us as part of the handshake.

people are just going to turn it on and not think, and deploy like that

How is that any different than someone using delegate { return true; } today on Windows?

[blowdart]

It's equally as bad in implementation, but now you've added two ways to completely bypass security, instead of one, which is well know.

This is a stack overflow cut and paste problem. People search for "My self signed cert doesn't validate" on stack overflow, paste the delegate in, then deploy to live. Now you've made it worse in that there are two loaded guns, rather than one.

The "right" way to go about this is find a way to fix the call backs, rather than say "Sod security, let's just turn it off". If it's impossible in curl, then maybe it's time to look at other options.

[stephentoub]

If it's impossible in curl, then maybe it's time to look at other options.

We are for post-2.0. In the meantime, there's no way. And this is a huge gap in functionality. I see this as a shipblocker. And I don't agree with the two-ways-is-worse-than-one argument, nor do I believe we should hold back important functionality because we're afraid folks may misuse it.

[blowdart]

I'll be honest I'd consider adding this a security blocking item for shipping too.

[blowdart]

Are there any numbers on customer demand? The justification right now is "So we can have unit tests work", which isn't enough for a dangerous feature.

[stephentoub]

Are there any numbers on customer demand?

winhttp supports this via the WINHTTP_OPTION_SECURITY_FLAGS option.
libcurl supports this via the CURLOPT_SSL_VERIFYPEER option.
golang supports this via the InsecureSkipVerify option.
python supports this via _create_unverified_https_context.
And so on.

A quick search on GitHub for CURLOPT_SSL_VERIFYPEER returns ~710K hits, and from quick glance, most of them are setting it to 0/false (which makes sense, given that the default is 1/true).

The justification right now is "So we can have unit tests work", which isn't enough for a dangerous feature.

Not just "we." But, sure, in a world of microservices, who cares about testing.

[bartonjs]

@blowdart See feedback on dotnet/runtime#17723. Granted, some of them want the callback to actually work, which we can't do without the non-existent libcurl callback.

[stephentoub]

I'll be honest I'd consider adding this a security blocking item for shipping too.

You're kidding, right? This is fine:
handler.ServerCertificateCustomValidationCallback = delegate { return true; }
but this is somehow a security shipstopper:
handler.ValidateServerCertificate = false;
even though they have the exact same impact? Come on.

[blowdart]

The call back has an actual purpose, on platforms where it works.

Making it equivalent to your new flag for this argument is disingenuous. Your boolean flag is "Turn off all security on HTTPS and get up to ramming speed", and once added it'll never go away.

[stephentoub]

Making it equivalent to your new flag for this argument is disingenuous.

Search for ServerCertificateValidationCallback on GitHub, and you'll find a majority of the ~55K hits setting it to the equivalent of delegate { return true; }. Can it be used for other things? Of course. But it's often used for this.

[blowdart]

And that use introduces security vulnerabilities. You're in agreement on that, right? As well as in agreement stack overflow hardly illustrates best practice in a lot of cases.

Validation callbacks are a crowbar, they have legitimate uses, your new flag is, well, a big spiky club where the possibility of hurting people with it has gone up by an order of magnitude because it's inherently unsafe.

[stephentoub]

And that use introduces security vulnerabilities. You're in agreement on that, right?

Inherent security vulnerabilities? No, I don't agree. When used to disable security when it shouldn't be disabled? Sure. But that's not a flaw in the framework, that's a flaw in the app explicitly doing something it shouldn't be doing, and sometimes you need to give users that club along with a warning that it's important they know what they're doing. We can document it to high heaven, similar or more so than, for example, libcurl's docs do at https://curl.haxx.se/libcurl/c/CURLOPT_SSL_VERIFYPEER.html.

[blowdart]

Like people read docs :)

How about doing this another way round? How about giving a static validation callback, "DontCheckCertificateValidityBecauseIHateSecurity()", which just returns true as you say everyone does. Then that, because it's static, could be looked, and the horrible, screw you settings, enabled on platforms that don't do callbacks.

That way there's not another API for people to look for in a security code review, and which we won't remove, because we don't want to break people, and, when, post 2.0, you get callbacks working as expected on all platforms, everyone is happy. It protects the folks who want callbacks for actual checking, and for those who want to just turn everything off, because ramming speed.

[stephentoub]

How about doing this another way round?

I'd be ok with that, basically:
```C#
public class HttpClientHandler
{
public static Func ConsiderAllCertificatesValidFunction { get; } = delegate { return true; };
...
}

which could be used like:
```C#
handler.ServerCertificateCustomValidationCallback = HttpClientHandler.ConsiderAllCertificatesValidFunction;

The implementation can use the object identity of that delegate as an indication that it always returns true and do the equivalent of what I proposed with handler.ValidateServerCertificate = false;.

[blowdart]

Yup, because this has the advantage that because it's using the ValidiationCallback API. This API is already considered dangerous, and usage is caught by a bunch of automating code review tools. So those tools will flag it for review anyway, as opposed to lagging behind and giving a false sense of security because a new API appeared they don't know about.

[bartonjs]

I think I like the object identity on the delegate more than the additional property, since it makes there be fewer places we have to update. (Okay, so it only eliminates one of the 3 PAL implementations...).

I'll let you guys fight over the name. I'm tempted to try for a cautious name like DangerousAllowAllCertificates, but that just makes it sound like "this one's bad, write your own 'return true'".

[stephentoub]

DangerousAllowAllCertificates sounds fine to me, though I think we probably need some suffix as well, otherwise it sounds like a settable property. Maybe DangerousAllowAllCertificatesCallback, DangerousAllowAllCertificatesFunction, etc. (I shy away from Delegate as a suffix, as that's often used on delegate types.)

[blowdart]

I'm told I'm not allowed to swear in framework naming, so I can't get what I really want :)

InsecureAllCertificatesAreValidValidator? :)

[stephentoub]

I'm told I'm not allowed to swear in framework naming

You can just store it into your own "appropriately" named static when you need it in your own code ;)

Validator also sounds like a reasonable suffix.

[blowdart]

We used validator as the suffix in asp.net when we were experimenting with cert pinning.

[CIPop]

I don't like the original approach as it will complicate the exiting APIs for the benefit of a single platform.

We have considered this approach during WinHttpHandler design for performance reasons (the current callback will unnecessarily slow down execution compared with having the policy applied by the native code).

I will keep my old recommendation: OS-specific APIs should be added to os-specific handlers such as WinHttpHandler and CurlHandler. Make these handlers public would also eliminate the need of polluting the netstandard APIs in future cases as OSs will evolve and potentially diverge in API patterns.

For the common netstandard API I like @stephentoub 's new idea of delegate identity comparison: achieves the goal as well as simplifies the existing test scenario.

@blowdart An interesting side-effect of having the named delegate is that it makes apps more secure: as static analysis tools can now scan and issue warnings if it's used in a release build. (There are several security-valid uses for the callback such as cert pinning.)

For naming I believe that the API should have a clear "test-only" designation on the lines of: UnsafeDisableSecurityValidator or UnsafeDisableTlsAuthenticationValidator.

[hallatore]

The name should be descriptive, so people can understand the outcome without needing to read the docs. People will find this method on stack overflow, and they need to understand it by the name alone.

I really like the way react.js handles raw html, as in how the name describes what it does. dangerouslySetInnerHTML={{ __html: "<div>Hello</div>" }}

Unsafe can be confused with the unsafe compile flag. It's not necessarily a bad thing in the .net world.
And "SecurityValidator" or "AuthenticationValidator" doesn't really tell the person that https cert checks are turned off.

Something along the lines of DangerouslyOverrideCertificatesValidator maybe.

[blowdart]

DangerousDisabledCertificationValidator perhaps?

[grahamehorner]

or UsingSelfSignDevelopmentCertificateValidator ? 😜 which can only be used with on release builds 😉

[stephentoub]

I've updated the proposal.

[stephentoub]

For reference, here's approximately what the change would look like:
https://github.com/stephentoub/corefx/commit/eb6e15d664b75ada7037d31fcf87b49d5ca4c002

[davidsh]

@stephentoub Is this new API being proposed for NetStandard2.0 as well? If so, this new API will need to be done for .NET Framework vNext as well (inbox System.Net.Http.dll). @AlexGhiondea FYI.

[karelz]

I don't think it should go into .NET Standard 2.0, just into .NET Core 2.0.

[stephentoub]

I'm proposing it for core, for now at least.

[CIPop]

I'm nitpicky about the name: DangerousAcceptAllCertificatesValidator doesn't feel very descriptive to me.
In general, the server sends a chain of certificates (and in this case that's not what we mean by "all certificates"). If those certificates are valid (chaining, expiration, they match the local trusted roots, etc) we will accept them without this set.

I would think that the API needs to describe what part of the TLS protocol it disables: TLS authentication of the server. It's also important that _client_ certificate authentication or all other kinds of authentications such as Basic, Digest, Negotiate will not be disabled. In other words, setting this will send your (cached in certain cases) credentials in clear text to a potentially rogue server.

This is why I think the name should be more ominous and to the point. Here are some names I'm proposing: Disable[Tls|Https]Authentication, Disable[Tls|Https]Security, Disable[Tls|Https]Privacy

[davidsh]

This is why I think the name should be more ominous and to the point. Here are some names I'm proposing: Disable[Tls|Https]Authentication , Disable[Tls|Https]Security , Disable[Tls|Https]Privacy

I think we need to add something about the "Server" in this name. We're not disabling all Tls security, i.e. if the server wants client certificates then a client cert would potentially be sent and the server would decide itself whether to continue the Tls connection.

So, I would want to change DangerousAcceptAllCertificatesValidator to something like DangerousAcceptAnyServerCertificateValidator

[stephentoub]

So, I would want to change DangerousAcceptAllCertificatesValidator to something like DangerousAcceptAnyServerCertificateValidator

DangerousAcceptAnyServerCertificateValidator sounds good to me.

[bartonjs]

I agree with @davidsh. This doesn't "disable" privacy (it might still be there), or security (it might still be there), or authentication (TLS authentication I'd see as "mutual authentication", so it's misleading; and HTTPS authentication could mean a cookie).

DangerousAcceptAnyServerCertificateValidator sounds good to me.

[blowdart]

Tagging @srivatsn for roslyn-analyzers and @h3xstream for roslyn-security-guard - folks you may want to consider flagging usage of this as a bad idea in your security rules once you update for 2.0.

[CIPop]

@bartonjs How can privacy, security or authentication still be there if you're allowing your client to connect to a random machine on the network (WiFi included!) which can decode all transmissions to their original, "clear-text" form?

Take for example a rogue machine acting as the destination server. It will have access to clear-text user/pwd (Basic/Digest), cookies as well as to your identity (your client certificate, other info in the request, etc). There's nothing the client app can do to prevent this rogue MITM machine from reusing the credentials and obtain access to the real server. This scenario proves that privacy (data has been decoded, identity has been compromised), security (secrets are exposed) as well as authentication (client trusts a rogue machine) are compromised.

TLS authentication I'd see as "mutual authentication"

Most scenarios do not involve mutual authentication. Clients usually connect to HTTPS servers without any TLS credentials then use other forms of auth (usually sending clear-text user/pwd to the server).

We're not disabling all Tls security

@davidsh We are definitely disabling all TLS security: if the client connects to a random server on the network, does it matter that it correctly authenticates itself anymore?

@blowdart @morganbr Given the answers above I'm now starting to understand your original concerns of misuse. Is there a way to add a warning in release builds? We should also have this on the deny list for store apps (WACK).

[stephentoub]

We should also have this on the deny list for store apps (WACK).

This doesn't make sense to me. If you put this on the deny list, people are just going to change their code to use delegate { return true; } instead.

[blowdart]

We're nitpicking now :) So, it doesn't disable all TLS protection because the data over the wire would still be encrypted, it's just you can no longer rely on what's at the other end. So, strictly you get the transport security, just now you're open to MITM.

[srivatsn]

Tagging @jinujoseph for the roslyn-analyzers suggestion.

[CIPop]

If you put this on the deny list, people are just going to change their code to use delegate { return true; } instead.

Why would anyone with good intentions do that?

WACK is enforced only as a gate to the Store. The API can still be used during development and testing (apps can be built and installed w/o running WACK). A Release warning would also not prevent compilation, would only notify the developer that he may have forgotten test-only code enabled.

For anyone reading the thread in the future: obtaining correctly signed certificates nowadays is almost 0 cost. When possible, my recommendation is to use something like https://letsencrypt.org/ and avoid this API entirely.

[davidsh]

For anyone reading the thread in the future: obtaining correctly signed certificates nowadays is almost 0 cost. When possible, my recommendation is to use something like https://letsencrypt.org/ and avoid this API entirely.

In addition to getting a public trusted certificate, UWP developers can ship their own trusted roots with the UWP app and they get installed into the app-container certificate store. This allows UWP apps to still use secure Tls connections. The trustworthy-ness of those certificates is then simply about the trustworthy-ness of the app itself which is vetted on the Windows App Store. So, there is always a 0-cost solution for certificates for UWP apps.

So, having app developers rely on using this new API shouldn't be needed to ship an app.

[davidsh]

@stephentoub What is the next step for this? Are you driving API review with @terrajobst et. al.? The dev cost of implementing is low at this point since you have the work already prepared.

[stephentoub]

Why would anyone with good intentions do that?

All of the same reasons people disable compiler warnings, disable fxcop rules warnings, disable Roslyn analyzer warnings, etc. I'm simply saying if someone thinks they need it, if they're blocked from using it they're going to go in search of the quickest and easiest workaround, which already exists and is only a few characters.

What is the next step for this? Are you driving API review with @terrajobst et. al.?

Yes

[CIPop]

I'm simply saying if someone thinks they need it, if they're blocked from using it they're going to go in search of the quickest and easiest workaround

I thought we've agreed that this property is for Debug/Test time only. In https://github.com/dotnet/corefx/issues/19709#issuecomment-301880839 I've already explained that these two scenarios will not be impacted at all (no warnings, app works as expected). Compilation will also not be impacted if the build is a "Debug" build (no warning for a Debug build, just for a Release one).

I'm still missing the point: workaround to which problem exactly?

[blowdart]

You can't limit the property to debug/test though can you? I'm not sure how useful that would be, given people can approach this in another way anyway.

[stephentoub]

You can't limit the property to debug/test though can you? I'm not sure how useful that would be, given people can approach this in another way anyway.

Right.

In any event, that discussion isn't really relevant to the API itself, so it can be tabled and had by whoever is responsible for what the WACK validates.

[karelz]

Reopening to track porting to 2.0 branch.

[stephentoub]

@karelz, it was already ported: https://github.com/dotnet/corefx/issues/19941

[Jonathan34]

I am facing the same issue on osx. I want to temporarily disable ssl check for centos deployment and I develop using macosx.
This feature allows me to disable ssl check for macosx but i am wondering if this is the final solution? it s not really pretty to have this in your code ...

handler.ServerCertificateCustomValidationCallback = HttpClientHandler.DangerousAcceptAllCertificatesValidator;

[vyomdixit19]

Can anyone look into this also, as we are facing same issue for WebSocket, while running a dotnetcore console app on Mac Sierra (osx.10.12-x64) ?
https://github.com/ImoutoChan/websocket-sharp-core/issues/3