Improve the Client Certificate Logic

[nibanks]

• Updates the logic to make sure to validate there is a peer certificate if (client || require_client_auth) and cert validation isn't disabled.
• If cert validation is disabled, there is no cert and the app wants a callback, give then a NULL cert callback.

Only Schannel complete. OpenSSL next.

[anrossi]

Going through the OpenSSL certificate validation, I think it already does all the above desired things.
It validates the peer certificate if the recipient is a client, or requires client auth.
It will report validation errors when the ignore_validation and deferred_validation flags are not present.
It will give a NULL certificate if indicate_cert is set and ignore_validation is set.

[ManickaP]

Locally, on a Linux machine, with this change, one of our tests fails (ConnectWithCertificateChain). And based on my debugging, when we receive PEER_CERTIFICATE_RECEIVED, the Chain buffer is empty.

But based on the comment:

Only Schannel complete. OpenSSL next.

I assume this will get resolved eventually. I'm only mentioning it in case this really is an unexpected error.

cc @wfurt

[nibanks]

@anrossi I don't believe this is expected. Please address and make sure we have a test case to cover this apparent gap in our coverage.

[anrossi]

@ManickaP does the test validate the chain sent from the server, or sent from the client? Is the Chain buffer nullptr or just an empty buffer? Also, are you using the QUIC_CREDENTIAL_FLAG_USE_PORTABLE_CERTIFICATES flag or not?

[wfurt]

The chain should be symmetric e.g. when endpoint sends certificate, it should send intermediates without root.
I would expect that what ever is sent on the wire as intermediates would be presented in the chain.
(and on Linux, we always use the PORTABLE_CERTIFICATES.)

The test @ManickaP mentioned does verify server's chain e.g. there is no client cert.
So the question is if the server sends it and we fail to present the certs or if the intermediates are omitted for some reason.

[ManickaP]

@anrossi

does the test validate the chain sent from the server, or sent from the client?

Sent from the server, client blows up.

Is the Chain buffer nullptr or just an empty buffer?

Empty buffer:

Also, are you using the QUIC_CREDENTIAL_FLAG_USE_PORTABLE_CERTIFICATES flag or not?

Yes, we're using it.

BTW, on the client we're setting these flags: INDICATE_CERTIFICATE_RECEIVED, NO_CERTIFICATE_VALIDATION, USE_PORTABLE_CERTIFICATES (on Linux), CLIENT

The test code is here:
https://github.com/dotnet/runtime/blob/6fad9b3c8193b9961ef6e2802759aeadad94ac5a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs#L64-L107

[anrossi]

Thanks @ManickaP and @wfurt; I'll look into this today and make sure we have a test for it too.

[anrossi]

@wfurt and @ManickaP does this issue repro on Windows with Schannel? (Schannel supports portable certificates in user mode).
Do you also receive the leaf certificate? Or are both missing?
I've opened #2732 to track this, so let's continue the discussion there.

[ManickaP]

does this issue repro on Windows with Schannel? (Schannel supports portable certificates in user mode).

No idea. If you want me to test it, I'll have to set up Win machine since I don't have one 😄 I can do it, but it'll take time.

Do you also receive the leaf certificate? Or are both missing?

If you mean PEER_CERTIFICATE_RECEIVED.Certificate, then we get that.

[wfurt]

I can probably try both. Since our goal is SslStream parity I'm probably at best position to do the testing.

[nibanks]

Thanks @wfurt. We appreciate it.