Skip to content

Support optional client certificates #57308

Closed

Description

@Tratcher

Description

SslServerAuthenticationOptions.ClientCertificateRequired = true is needed to prompt the client to send a certificate. SslStream will invoke RemoteCertificateValidationCallback with the result, even if it's null, and the server can choose if it wants to accept the connection. QuicListener however will terminate the connection without invoking RemoteCertificateValidationCallback. This is feature gap for quic that should be filled if possible.

            var listenerOptions = new QuicListenerOptions()
            {
                MaxBidirectionalStreams = 100,
                MaxUnidirectionalStreams = 100,
                ServerAuthenticationOptions = new SslServerAuthenticationOptions()
                {
                    ServerCertificate = TestResources.GetTestCertificate(),
                    ApplicationProtocols = new List<SslApplicationProtocol>() { new SslApplicationProtocol("h3") },
                    ClientCertificateRequired = true,
                    RemoteCertificateValidationCallback = RemoteCertificateValidationCallback,
                },
                ListenEndPoint = new IPEndPoint(IPAddress.Loopback, 0),
            };
            var listener = new QuicListener(listenerOptions);
            var acceptConnection = listener.AcceptConnectionAsync();

            var clientOptions = new QuicClientConnectionOptions
            {
                MaxBidirectionalStreams = 200,
                MaxUnidirectionalStreams = 200,
                RemoteEndPoint = listener.ListenEndPoint,
                ClientAuthenticationOptions = new SslClientAuthenticationOptions
                {
                    ApplicationProtocols = new List<SslApplicationProtocol>
                    {
                        new SslApplicationProtocol("h3")
                    },
                    RemoteCertificateValidationCallback = RemoteCertificateValidationCallback
                }
            };
            var testCert = TestResources.GetTestCertificate();
            // clientOptions.ClientAuthenticationOptions.ClientCertificates = new X509CertificateCollection { testCert };

            using var clientConnection = new QuicConnection(clientOptions);
            await clientConnection.ConnectAsync();

            var serverConnection = await acceptConnection;

            var clientStreamAccept = clientConnection.AcceptStreamAsync();

            var serverStream = serverConnection.OpenUnidirectionalStream();
            await serverStream.WriteAsync(TestData);

            var clientStream = await clientStreamAccept;

            static bool RemoteCertificateValidationCallback(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
            {
                return true;
            }

The client's call to ConnectAsync fails with:

    | System.Net.Quic.QuicException: Connection has been shutdown by transport. Error Code: 0x80410100
    |    at System.Net.Quic.Implementations.MsQuic.MsQuicConnection.HandleEventShutdownInitiatedByTransport(State state, ConnectionEvent& connectionEvent)
    |    at System.Net.Quic.Implementations.MsQuic.MsQuicConnection.NativeCallbackHandler(IntPtr connection, IntPtr context, ConnectionEvent& connectionEvent)

I can't tell if the termination is happening on the client or server. The server logs show it did accept the connection before failing (see #57246).

Recommendation: .NET 7

Activity

  1. ghost added
    untriagedNew issue has not been triaged by the area owner
    on Aug 12, 2021
  2. wfurt commented on Aug 12, 2021

    @wfurt
    Member

    It seems like in that case we don't even get callback from MsQuic and it fails internally. I will take a look to at least understand.

  3. removed
    untriagedNew issue has not been triaged by the area owner
    on Aug 13, 2021
  4. added this to the 7.0.0 milestone on Aug 13, 2021
  5. added
    enhancementProduct code improvement that does NOT require public API changes/additions
    and removed on Nov 16, 2021
  6. karelz commented on Nov 16, 2021

    @karelz
    Member

    Triage: We need to double check if msquic now supports all we need.
    Consistency with HTTP/2 and HTTP/1.1

  7. ManickaP commented on Feb 8, 2022

    @ManickaP
    Member

    This should functionally work. But we do have duplicated logic for building the chain and evaluating client cert validity from SslStream in MsQuicConnection. We might consider some refactorings.
    However, I don't think we're missing any functionality atm, @wfurt ?

  8. wfurt commented on Feb 8, 2022

    @wfurt
    Member

    this was failing within msquic last time I check @ManickaP e.g. if certificate was not provided it would always fail.

  9. ManickaP commented on Feb 8, 2022

    @ManickaP
    Member

    So need to retest and potentially file an issue in msquic?

  10. wfurt commented on Feb 8, 2022

    @wfurt
    Member

    yes. possibly related to microsoft/msquic#1249

  11. ManickaP commented on Feb 8, 2022

    @ManickaP
    Member

    I guess that's related to missing implementation for LocalCertificateSelectionCallback?

  12. ManickaP commented on Feb 16, 2022

    @ManickaP
    Member

    I retested this, we don't get any callback from MsQuic when client sends NULL certificate.

  13. wfurt commented on Feb 16, 2022

    @wfurt
    Member

    We may be able to fix it up if we get handshake complete even without certificate callback.

    BTW did you test this on Windows? The issue I was talking while back was Schannel specific.

    [Theory]
    [InlineData(true)]
    // [InlineData(false)] [ActiveIssue("https://github.com/dotnet/runtime/issues/57308")]
    [ActiveIssue("https://github.com/dotnet/runtime/issues/64944", TestPlatforms.Windows)]
    public async Task ConnectWithClientCertificate(bool sendCerttificate)

    Can this pass now both options? #64944 is very recent and I don't know if that is just flaky on Server 2022

  14. ManickaP commented on Feb 17, 2022

    @ManickaP
    Member

    No, I don't have any Win 11 machine atm. It reproduced on Linux.

  15. ManickaP commented on Feb 22, 2022

    @ManickaP
    Member

    Triage: we cannot work around by not setting ClientCertificateRequired since that will not prompt the client to send the cert. We still want the option to keep the connection open even if client disobeys and doesn't send the certificate.
    This should work, we'll provide pseudo code description for msquic to test and see what happens. Based on msquic they have ability to not to kill the connection.

    msquic issue microsoft/msquic#1953

  16. added
    tracking-external-issueThe issue is caused by external problem (e.g. OS) - nothing we can do to fix it directly
    on Mar 8, 2022
  17. Tratcher commented on May 19, 2022

    @Tratcher
    MemberAuthor
  18. wfurt commented on May 20, 2022

    @wfurt
    Member

    maybe. I know @ManickaP was planning to do more testing. We should enable our tests on all platforms before we claim victory.

  19. ManickaP commented on May 20, 2022

    @ManickaP
    Member

    ATM, we cannot update msquic due to this: microsoft/msquic#2732, so we cannot consume it yet. However, we might test it out locally and do some fix ups if necessary; otherwise this should just work (tm).

    @rzikm is this something you could look into?

  20. rzikm commented on May 20, 2022

    @rzikm
    Member

    @ManickaP just curious, is that bug you mentioned something that was broken recently (and why we can't update)?

    I tested the latest MsQuic locally and it will need some fixes from our side (namely passing QUIC_CREDENTIAL_FLAG_USE_SUPPLIED_CREDENTIALS for clients). Don't ask how long it took me to debug this :D

  21. self-assigned this
    on May 20, 2022
  22. ghost added
    in-prThere is an active PR which will close this issue when it is merged
    on May 20, 2022
  23. ManickaP commented on May 20, 2022

    @ManickaP
    Member

    It's portable certificates specific (in our case on Linux), it's been introduced in microsoft/msquic#2419. Some more details are in that PR.

  24. ghost removed
    in-prThere is an active PR which will close this issue when it is merged
    on May 25, 2022
  25. ghost locked as resolved and limited conversation to collaborators on Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area-System.Net.QuicenhancementProduct code improvement that does NOT require public API changes/additionstracking-external-issueThe issue is caused by external problem (e.g. OS) - nothing we can do to fix it directly

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions