[Foundation] Add support to the ServerCertificateValidationCallback on the NSUrlSessionHandler

[mandel-macaque]

This changes allows to use the service point manager callback within the
NSUrlSessionHanlder.

The logic is as follows:

1. Check that the challange is about the ssl cert.
2. Convert the SecTrust info to the managed objects.
3. Call the users cb.
4. If user returns true, continue, else, cancel and create an exception
   similar to the one used in the managed code.

Fixes #4170

[monojenkins]

Build failure
❌ Build was aborted

[monojenkins]

Build failure
❌ Build was aborted

[baulig]

Well, I really don't like this at all for two main reasons:

1. We should not use ServicePointManager here because it is going away, see my explanation below.
2. Let's please not have custom certificate evaluation code all over the place but reuse the existing one (I can help you with that). However, that's mostly cosmetic and I don't care too much about it.

Now, let me explain the ServicePointManager issue.

In .NET Core, .NET Standard and .NET Framework >= 4.71.1 there is a new property HttpClientHandler.ServerCertificateCustomValidationCallback (see https://docs.microsoft.com/en-us/dotnet/api/system.net.http.httpclienthandler.servercertificatecustomvalidationcallback?view=netframework-4.7.2) for certificate validation.

We do not support this in our old HttpClient code (and please don't add it there because that code is going away very shortly), but the new CoreFX based implementation does fully support it.

Once we have switched all over mobile profiles over to the new code, then HttpClient won't be using the web stack anymore at all (and therefore not using ServicePointManager). So we shouldn't get used to using ServicePointManager in the context of HttpClient as that won't work anymore. This new behavior is consistent with .NET Core (which does not even have the ServicePointManager).

Since the full roll-out of the new code is going to be finished before we branch for 2019-04 (and might actually still happen as part of 2019-02 for XI), I don't see much value in having a temporary solution in the meantime.

Specifically regarding HttpClient for Xamarin.iOS and NSUrlSessionHandler - this was actually planned to land during this sprint, but got delayed until the 2019-02 integration gets to a point where it builds and can run tests.

Hooking up the certificate validator in NSUrlSessionHandler is also part of my todo list for landing.

So if this it not too urgent and can wait until next sprint, then just leave it to me and I'll take care of everything.

[monojenkins]

Build success
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (no change)
✅ Generator Diff (no change)
✅ Test run succeeded

[spouliot]

@baulig makes good points about how it should be done and what are the API that could be used.

However there's another, more basic, reason for the current situation. This is exactly the approach we decided not to take when we adapted ModernHttpClient's handler code, i.e. the original implementation has similar code and we removed it.

Using the native handler(s) is promoted as a way to get better, faster and smaller applications. The current PR breaks the later (for sure, maybe the first one too) even when the new feature is not used. Why ? the linker can't eliminate X509Certificate[2] (and friends) and that brings nearly all the BCL crypto code as a side effect (which can't be fixed since it's by design / required).

This means any recompiled application gains megabytes of code that is never used (zero benefit).

We quickly chatted about this some weeks ago, but the right (or better) approach is to only expose native API (e.g. SecTrust) in the public API. That will greatly limit the impact of the size increase as p/invokes are a lot smaller than the crypto code IL (and the AOT code it generates directly and indirectly by referencing mode BCL code).

As a (significant) bonus this can be done more safely, i.e. @baulig earlier comment, in the sense that we won't be shipping certificate validation code (a huge plus security wise). Only the OS and, optionally the developer code, would be handling the decisions.

So what we need is to define a different API that, if used/implemented, allow developers to override the OS decision, where all the inputs must be native types (minimal size impact).

From that new API developers can reconstruct their own logic (which can be simpler that the one in the PR for simply pinning), including one that call .NET logic (we can document the conversion, there's already method helpers). If they do the later then only their application (and not every recompiled apps) would be paying the "size increase" price (which can range from small-to-large depending if the managed crypto code is already used inside their application).

[mandel-macaque]

Awesome feedback! Thanks for or the concerns!

[mandel-macaque]

@baulig I don't mind you taking over the implementation, but if you are happy with it, I would not mind taking a look on how to hook it up to the new way. I used the ServicePointManager because it was mentioned in the issue, I am more than happy to be ready to the new way to do things, and the more code we reuse the better.

@spouliot I was expecting something of the kind, I created the PR to start the conversation and try to move this forward.. The only issues we have to be worried about are the current way in which we are dealing with the authentication. But this can be quite simple since we just need to check the ProtectionSpace.AuthenticationMethod and branch there. We can expose the SecTrust to the users, and then do the correct thing with the completion handler. So we can either expose a callback to the NSUrlSessionHandler (similar to what is done). But I would wait for that approach until we see what @baulig is planning, since having a central way to do it and shared between the implementations would me a nicer approach, meaning that the users won't have to add too much custom code per implementation.

PS: I should have used a draft PR, I keep forgetting about that feature, again, I wanted to get the conversation started so that users can see that we do have planes for this.

[baulig]

Let me clarify one thing here about what is meant by "the new code".

There are two pieces to it: the code in the System.Net.Http and System.Net.Http.Headers namespace that we're switching over to the CoreFX implementation - and the actual SocketsHttpHandler.

All profiles will receive the former (so that we could completely nuke the System.Net.Http and System.Net.Http.Headers directories in mcs/class/System.Net.Http) - but you can still use the native handlers without the sockets handler (which can either be removed by the linker or "physically" omitted in the first place like on web assembly).

As part of that work, I will go over all the native handlers (like CFNetworkHandler, NSUrlSessionHandler etc.) and hook up the new certificate validator. Let's also keep in mind that the new sockets handler does not have any dependencies on the web stack anymore - so ideally, the linker should be able to completely eliminate the web stack.

In addition to what @spouliot pointed out regarding X509Certificate2, this is also an expensive operation if the X509Certificate2 instance is not properly constructed from a native handle.

@mandel-macaque No worries, I have already started work on this last week, but then got distracted by build issues in the mono-2019-02 branch (related to Mono.Native). There is also a draft PR: mono/mono#13339.

[spouliot]

Superseded by #6103