The RemoteCertificateValidationCallback is not respected when configured via a ServerOptionsSelectionCallback

[halter73]

Description

We're starting to use SslStream's ServerOptionsSelectionCallback AuthenticateAsServerAsync overload in Kestrel (see dotnet/aspnetcore#24286), and I've noticed this issue when we set ClientCertificateRequired = true and RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true in the callback.

Here's a test method that could be added to Kestrel's HttpsConnectionMiddlewareTests after dotnet/aspnetcore#24286 is merged that demonstrates the issue:

[Fact]
public async Task ClientCertificateRequiredConfiguredInCallbackContinuesWhenNoCertificate()
{
    void ConfigureListenOptions(ListenOptions listenOptions)
    {
        listenOptions.UseHttps((connection, stream, clientHelloInfo, state, cancellationToken) =>
            new ValueTask<SslServerAuthenticationOptions>(new SslServerAuthenticationOptions
            {
                ServerCertificate = _x509Certificate2,
                // From the API Docs: "Note that this is only a request --
                // if no certificate is provided, the server still accepts the connection request."
                // Not to mention this is equivalent to the test above.
                ClientCertificateRequired = true,
                RemoteCertificateValidationCallback = (sender, certificate, chain, sslPolicyErrors) => true,
                CertificateRevocationCheckMode = X509RevocationMode.NoCheck
            }), state: null, HttpsConnectionAdapterOptions.DefaultHandshakeTimeout);
    }

    await using (var server = new TestServer(context =>
        {
            var tlsFeature = context.Features.Get<ITlsConnectionFeature>();
            Assert.NotNull(tlsFeature);
            Assert.Null(tlsFeature.ClientCertificate);
            return context.Response.WriteAsync("hello world");
        }, new TestServiceContext(LoggerFactory), ConfigureListenOptions))
    {
        var result = await server.HttpClientSlim.GetStringAsync($"https://localhost:{server.Port}/", validateCertificate: false);
        Assert.Equal("hello world", result);
    }
}

Notice that the client is also using validateCertificate: false. Internally this uses (sender, certificate, chain, sslPolicyErrors) => true as the client SslStream ctor's userCertificateValidationCallback argument and passes checkCertificateRevocation: false to AuthenticateAsClientAsync.

On Windows, both the client and the server are still able to complete their calls to AuthenticateAsClient/ServerAsync. The problem is the client later throws the following from SslStream.ReadAsync() in the tests where it doesn't send a client cert:

System.IO.IOException : The decryption operation failed, see inner exception.
  ---- System.ComponentModel.Win32Exception : An unknown error occurred while processing the certificate.
Stack Trace:
  SslStream.ReadAsyncInternal[TIOAdapter](TIOAdapter adapter, Memory`1 buffer)
  StreamReader.ReadBufferAsync(CancellationToken cancellationToken)
  StreamReader.ReadToEndAsyncInternal()

When the client does provide a cert on Windows, it throws a similar exception also from SslStream.ReadAsync():

System.IO.IOException : The decryption operation failed, see inner exception.
  ---- System.ComponentModel.Win32Exception : The certificate chain was issued by an authority that is not trusted.
Stack Trace: 
  SslStream.ReadAsyncInternal[TIOAdapter](TIOAdapter adapter, Memory`1 buffer)
  StreamReader.ReadBufferAsync(CancellationToken cancellationToken)
  StreamReader.ReadLineAsyncInternal()

On Linux, we get the same Exception both when the client does and doesn't provide a cert, but this time it's thrown on the server from the new AuthenticateAsServerAsync overload (though the stack frame appears to be hidden).

System.Security.Authentication.AuthenticationException: The remote certificate is invalid according to the validation procedure.
   at System.Net.Security.SslStream.SendAuthResetSignal(ProtocolToken message, ExceptionDispatchInfo exception)
   at System.Net.Security.SslStream.ForceAuthenticationAsync[TIOAdapter](TIOAdapter adapter, Boolean receiveFirst, Byte[] reAuthenticationData, Boolean isApm)
   at Microsoft.AspNetCore.Server.Kestrel.Https.Internal.HttpsConnectionMiddleware.OnConnectionAsync(ConnectionContext context)

According to the API docs, despite the name, setting ClientCertificateRequired to true does not indicate the the client is required to send a certificate. That ultimately should be left up to the RemoteCertificateValidationCallback. And indeed that's been the case when we've used the AuthenticateAsServerAsync overload that takes SslServerAuthenticationOptions directly instead of the callback.

// Summary:
//     Gets or sets a value that specifies whether the client is asked for a certificate
//     for authentication. Note that this is only a request -- if no certificate is
//     provided, the server still accepts the connection request.
public bool ClientCertificateRequired

I've also tried using the SslStream's userCertificateValidationCallback constructor parameter instead of SslServerAuthenticationOptions.RemoteCertificateValidationCallback like we did previously when using the AuthenticateAsServerAsync overload that takes SslServerAuthenticationOptions directly, but that didn't make any difference. Even if that did work, it wouldn't allow Kestrel to use a different RemoteCertificateValidationCallback per server name and we do want Kestrel to be able to do that.

Configuration

Windows:

.NET SDK (reflecting any global.json):
 Version:   5.0.100-preview.7.20330.3
 Commit:    eeb77e1a55

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.20180
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   F:\dev\aspnet\AspNetCore\.dotnet\sdk\5.0.100-preview.7.20330.3\

Host (useful for support):
  Version: 5.0.0-rc.1.20370.4
  Commit:  0e0e648770

Linux:

.NET SDK (reflecting any global.json):
 Version:   5.0.100-preview.7.20330.3
 Commit:    eeb77e1a55

Runtime Environment:
 OS Name:     ubuntu
 OS Version:  18.04
 OS Platform: Linux
 RID:         ubuntu.18.04-x64
 Base Path:   /home/halter73/dev/dotnet/aspnetcore/.dotnet/sdk/5.0.100-preview.7.20330.3/

Host (useful for support):
  Version: 5.0.0-rc.1.20370.4
  Commit:  0e0e648770

Regression?

This is a new API, so technically no. It's a gap that's preventing Kestrel from using the new API effectively though.

@wfurt

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

[wfurt]

I did more testing and the documentation is misleading IMHO. I did testing with 3.1 and the negation fails unless you provide override. With 'ClientCertificateRequired` and client not providing certificate, negations may succeed. I'll try to update docs to make that more clear.

The missing callback was fixed as part of #40110 (since that was pending and touching relevant area)