Skip to content

Conversation

@rzikm
Copy link
Member

@rzikm rzikm commented May 20, 2022

Fixes #57308, #64944

This PR needs to wait until we consume newer MsQuic with microsoft/msquic#2732 fixed.

@ghost ghost added the area-System.Net.Quic label May 20, 2022
@ghost ghost assigned rzikm May 20, 2022
@ghost
Copy link

ghost commented May 20, 2022

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

Issue Details

Fixes #57308.

This PR needs to wait until we consume newer MsQuic with microsoft/msquic#2732 fixed.

Author: rzikm
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

if (serverAuthenticationOptions.ClientCertificateRequired)
{
flags |= QUIC_CREDENTIAL_FLAGS.REQUIRE_CLIENT_AUTHENTICATION | QUIC_CREDENTIAL_FLAGS.INDICATE_CERTIFICATE_RECEIVED | QUIC_CREDENTIAL_FLAGS.NO_CERTIFICATE_VALIDATION;
flags |= QUIC_CREDENTIAL_FLAGS.REQUIRE_CLIENT_AUTHENTICATION | QUIC_CREDENTIAL_FLAGS.INDICATE_CERTIFICATE_RECEIVED | QUIC_CREDENTIAL_FLAGS.NO_CERTIFICATE_VALIDATION | QUIC_CREDENTIAL_FLAGS.DEFER_CERTIFICATE_VALIDATION;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need both, I thought it's either or:

When QUIC_CREDENTIAL_FLAG_NO_CERTIFICATE_VALIDATION or QUIC_CREDENTIAL_FLAG_DEFER_VALIDATION are present

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The handshake fails without it, I must have left it there when I was debugging the code (MsQuic uses it in its unit tests). I will check with @anrossi

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow, it seems to work with the updated MsQuic, so I removed the DEFER_VALIDATION flag

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe @wfurt fixed MsQuic's OpenSSL layer so that it works with NO_VALIDATION as expected.

@rzikm
Copy link
Member Author

rzikm commented May 24, 2022

updated fedora image to one built in dotnet/dotnet-buildtools-prereqs-docker#618

@rzikm rzikm marked this pull request as ready for review May 24, 2022 15:29
@rzikm rzikm requested a review from ManickaP May 25, 2022 06:56
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
BTW, you removed several ActiveIssues attributes, are all of them fixed by this? If so, this PR should also mention them as "fixed".

}

return Create(options, QUIC_CREDENTIAL_FLAGS.CLIENT, certificate: certificate, certificateContext: null, options.ClientAuthenticationOptions?.ApplicationProtocols, options.ClientAuthenticationOptions?.CipherSuitesPolicy);
return Create(options, QUIC_CREDENTIAL_FLAGS.CLIENT | QUIC_CREDENTIAL_FLAGS.USE_SUPPLIED_CREDENTIALS, certificate: certificate, certificateContext: null, options.ClientAuthenticationOptions?.ApplicationProtocols, options.ClientAuthenticationOptions?.CipherSuitesPolicy);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, USE_SUPPLIED_CREDENTIALS is only used for Schannel. It should cause an error to use it with OpenSSL.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's strange, I didn't observe any test failures on Linux

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still planning to finish testing on Linux & Windows but got distracted with my trip. I should be able to finish testing tomorrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails on my machine, I'll put up a PR to fix this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support optional client certificates

5 participants