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.

[karelz]

Good questions, I believe the main desire to track what's being added at this point - from partner impact point of view (e.g. Xamarin) and risk assessment & focus. Double check with them. The API was approved after API freeze, which was on 3/1 😉.

[qmfrederik]

From a partner impact point of view, the .NET client for Kubernetes (see @brendandburns 's message earlier) would benefit hugely from this still making it to 2.1; the alternative is duplicating the managed WebSocket client in the Kubernetes client which is not a very attractive option.

[karelz]

I've send email to shiproom, let's see if we get it approved.

[analogrelay]

FYI, this is affecting SignalR customers as well. We'd definitely like this fix as soon as possible.

[karelz]

We've got shiproom approval, to get it in 2.1 - @stephentoub do you want to reopen your PR and push it through?

[karelz]

FYI @marek-safar API changes after API freeze, not sure if it impacts you ...

[karelz]

Thanks @stephentoub for pushing it through and thanks everyone for impact details, it's appreciated.

[gcadmes]

So after reading through this thread, It's safe to assume that the new API will be available in netstandard2.1, yes? May I ask when that might go primetime?

[mitchcapper]

It is merged into master at this point so yes I am quite confident it should be in 2.1. There should be a new beta of 2.1 any day now, depending when they flighted it it might be in that beta. Official 2.1 release I think it slated for H1.
(Comment edited as was originally replying thinking it was on another project about this issue), sorry.

[gcadmes]

Thank you Mitch for a speedy reply. Looking forward to the beta and release.

[karelz]

Just to be clear: We don't ship netstandard21, only netcore21. The API will be available only on .NET Core at this moment (and potentially Xamarin if they use our source code for this specific library).

[ygoe]

@karelz So if there's no netstandard21, unfortunately this new feature isn't accessible to my library. Should I use reflection to see if the runtime has that property, and set it?

[danmoseley]

@ygoe more robust might be to cross target your library ie build two binaries into your package.

[ygoe]

@danmosemsft Will there be a netstandard21 or something that includes this feature sometime? Or is this cross-targeting meant for a longer time?

[danmoseley]

@ygoe there will certainly be more versions of .NET Standard. We announced there will be an update next year aligned with .NET Core 3.0.

When we update it depends on how quickly other platforms (mainly Mono/Xamarin) pick up new API's. There's more discussion here

In short, multitargeting should be temporary as this API should end up in the next .NET Standard. There will always be periods, though, when a new API is in .NET Core (or possibly Mono) but not yet .NET Standard.

[ygoe]

Just a side note, Visual Studio team has just decided that multi-targeting is not a priority and that my bad experience with it is acceptable. So I have decided that multi-targeting is not an option. My reflection-based implementation seems to do the job without all the hassle.

[lanatmwan]

@ygoe can you share your implementation?

[ygoe]

@lanatmwan It's been a while, but this should be what you're looking for:

webSocket = new ClientWebSocket();
// TODO: Set up server certificate validation, only available in .NET Core 2.1 or newer
// (https://github.com/dotnet/corefx/issues/12038)
//webSocket.Options.RemoteCertificateValidationCallback = ...;
if (RemoteCertificateValidationCallback != null)
{
    var prop = webSocket.Options.GetType().GetProperty("RemoteCertificateValidationCallback");
    if (prop == null)
        throw new InvalidOperationException("RemoteCertificateValidationCallback is not supported on this platform. Use .NET Core 2.1 or newer.");
    prop.SetValue(webSocket.Options, RemoteCertificateValidationCallback);
}

[lanatmwan]

Oh, I thought you were saying you had a way to make it work on 2.0 using reflection. Guess I am stuck using the UWP api. Thanks for the quick response though!

On Wed, Mar 18, 2020 at 3:39 AM Yves Goergen notifications@github.com wrote:

@lanatmwan https://github.com/lanatmwan It's been a while, but this should be what you're looking for:

webSocket = new ClientWebSocket(); // TODO: Set up server certificate validation, only available in .NET Core 2.1 or newer // (https://github.com/dotnet/corefx/issues/12038) //webSocket.Options.RemoteCertificateValidationCallback = ...; if (RemoteCertificateValidationCallback != null) { var prop = webSocket.Options.GetType().GetProperty("RemoteCertificateValidationCallback"); if (prop == null) throw new InvalidOperationException("RemoteCertificateValidationCallback is not supported on this platform. Use .NET Core 2.1 or newer."); prop.SetValue(webSocket.Options, RemoteCertificateValidationCallback); }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dotnet/runtime/issues/18696#issuecomment-600548143, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA22NWQK3MCZNE52WCXJN7LRICQGFANCNFSM4LN5PO3Q .