Add support for RemoteCertificateValidationCallback with ClientWebSocket

[BravoTango86]

There is no way of overriding the validation of server certificates on an SslStream utilised by ClientWebSocket.

Could this be exposed through ClientWebSocketOptions?

EDIT 3/5/2018 by @stephentoub. Added proposal:

public sealed class ClientWebSocketOptions
{
    public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get; set; }
    ...
}

[davidsh]

cc: @CIPop @stephentoub

[CIPop]

Could this be exposed through ClientWebSocketOptions?

I think that's the right place to put the new API too. The API should be identical to one on HttpClient:

        public System.Func<System.Net.Http.HttpRequestMessage, System.Security.Cryptography.X509Certificates.X509Certificate2, System.Security.Cryptography.X509Certificates.X509Chain, System.Net.Security.SslPolicyErrors, bool> ServerCertificateCustomValidationCallback { get { throw null; } set { } }

For the API proposal please follow https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/adding-api-guidelines.md.
If anyone picks this item, please focus on the ManagedWebSocket implementation initially.

[qmfrederik]

@stephentoub I've again hit a use case where I'm wanting to use a ClientWebSocket with client certificates, and where the client certificates come from a private CA (a Kubernetes cluster in this case).

I don't necessarily want to add that CA to the list of trusted CAs, but rather manage the validation in code.

In https://github.com/dotnet/corefx/issues/5120#issuecomment-353613941 you said to open an API issue to support certificate validation in ClientWebSocket; it looks like this issue is just about that.

So, could this issue serve as the API proposal issue? The API proposal from @CIPop seems to make sense:

class ClientWebSocket
{
+       public Func<HttpRequestMessage, X509Certificate2, X509Chain, SslPolicyErrors, bool> ServerCertificateCustomValidationCallback { get; set; }
}

[stephentoub]

We can use this issue for it, but the proposed API would actually need to be on ClientWebSocketOptions rather than on ClientWebSocket. I also think it would behoove us to instead consider something like https://github.com/dotnet/corefx/issues/26567 rather than adding lots of individual properties as we find more and more areas of SslStream configuration that's desirable to be exposed.

[tintoy]

@stephentoub - if I'm reading the documentation correctly, the way to proceed with a proposal is to fork the corefx repo and actually make the proposed changes so they can be reviewed and discussed? Have I got that right?

If so, I'd be willing to put my hand up to have a go at helping to move this forward :-)

[stephentoub]

the way to proceed with a proposal is to fork the corefx repo and actually make the proposed changes so they can be reviewed and discussed? Have I got that right?

That's not necessary; we just need a solid proposal for what the API would look like.

[tintoy]

Ok I'm still happy to pitch in FWIW (I can look at older proposals for guidance). Just want to check that you're ok with me making the attempt (if someone else is already working on it I wouldn't want to double up on effort is all).

[stephentoub]

Actually, I take back what I said about using SslClientAuthenticationOptions:

1. The ClientWebSocketOptions already exposes ClientCertificates, and it'd be a bit strange to have both that and an SslOptions that also had ClientCertificates (though we could smooth over that a bit by just having the former delegate to the latter).
2. ClientWebSocket isn't necessarily tied in implementation to System.Net.Security and SslStream, so it could be strange in the public API to use a type that is.

So, I suggest we just keep this simple and add:

public sealed class ClientWebSocketOptions
{
    public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get; set; }
    ...
}

It'd be nice to get this into 2.1 if possible.
cc: @geoffkizer, @davidsh

[davidsh]

I agree with this API approach with just adding this API to the existing ClientWebSocketOptions class:

public RemoteCertificateValidationCallback RemoteCertificateValidationCallback { get; set; }

[brendandburns]

Would love to see this get added!

[stephentoub]

@davidsh, I just looked at the WinRT implementation, and it appears to be based on MessageWebSocket, which has an API surface that I'm not sure could support this. Any concerns?

[davidsh]

In that case, it would have to throw PNSE on UAP for now. Long-term we should consider moving the UAP version to use the "managed" implementation of ClientWebSocket, same as the rest of CoreFx.

[karelz]

@stephentoub can you please ping shiproom with brief description of the API addition? Given we are quite past API freeze, we should do it.

[stephentoub]

Given we are quite past API freeze, we should do it.

What is the purpose of the API freeze? a) This was approved before it. b) This is making something public that was already implemented before it.