[QUIC] Fix flags usage on Linux by ManickaP · Pull Request #69874 · dotnet/runtime

ManickaP · 2022-05-26T18:54:42Z

ghost · 2022-05-26T18:54:49Z

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

Issue Details

Author:	ManickaP
Assignees:	-
Labels:	`area-System.Net.Quic`
Milestone:	-

wfurt · 2022-05-26T19:06:44Z

I'm not sure it is needed. My understanding was that the flags may be used on Windows. I did tests on Windows and all tests passes after updating msquic manually.

anrossi · 2022-05-26T19:10:37Z

On Schannel, USE_SUPPLIED_CREDENTIALS forces the client to send an empty certificate_list back to the server when the server requests client authentication and the client has no certificate to send, so the server may decide whether to terminate the connection or not.
Without USE_SUPPLIED_CREDENTIALS, Schannel client will end the handshake if it cannot find a certificate to send to the server when the server requests client authentication.

wfurt · 2022-05-26T19:12:16Z

It is unfortunate that the flag is platform specific. It feels like it would be better to ignore it on Linux if Linux can provide same functionality.

rzikm

LGTM, thanks!

anrossi · 2022-05-26T19:44:49Z


-            return Create(options, QUIC_CREDENTIAL_FLAGS.CLIENT | QUIC_CREDENTIAL_FLAGS.USE_SUPPLIED_CREDENTIALS, certificate: certificate, certificateContext: null, options.ClientAuthenticationOptions?.ApplicationProtocols, options.ClientAuthenticationOptions?.CipherSuitesPolicy);
+            QUIC_CREDENTIAL_FLAGS flags = QUIC_CREDENTIAL_FLAGS.CLIENT;
+            if (OperatingSystem.IsWindows())


as long as .NET doesn't use OpenSSL on Windows, this is fine.

Do we have any way to query what backend the library uses? Otherwise having the flag produce errors with OpenSSL makes it a bit inconvenient.

I don't think we have a way to query which crypto library is in use. @ThadHouse @nibanks, do you know of something?

No, we don't currently expose a way to do this. We could via a global GetParam option easily enough. If you need this @rzikm please open a MsQuic issue for it.

I still feel that making sure the flag does not break OpenSSL would be better option e.g. less platform code to maintain elsewhere.

.NET uses the SChannel version on Windows, so unless we care that the user may somehow provide a different dll (possibly built with OpenSSL), then we don't need it.

We have had asks to support OpenSSL MsQuic on Windows with .NET, so would prefer not to make assumptions of using Schannel on Windows.

agreed. While we may not support it we may take community contributions. Requirement for S2022 or W11 seems pretty steep for many users.

We also tie to OperatingSystem.IsWindows() (non) usage of portable certs. I assume that is also rather thing of OpenSSL and following would not work with it:

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs

Lines 178 to 183 in cb1fd54

if (OperatingSystem.IsWindows())

{

config.Type = QUIC_CREDENTIAL_TYPE.CERTIFICATE_CONTEXT;

config.CertificateContext = (void*)certificate.Handle;

status = MsQuicApi.Api.ApiTable->ConfigurationLoadCredential(configurationHandle.QuicHandle, &config);

}

Unless, I'm mistaken here, I wouldn't put too much effort into distinguishing Windows from SChannel atm.

I filed microsoft/msquic#2761, it is not urgent, but it would be nice to have it in place when somebody eventually asks for support of OpenSSL-MsQuic on Windows.

ManickaP · 2022-05-27T08:03:15Z

Failing QUIC test is System.Net.Quic.Functional.Tests: [Long Running Test] 'System.Net.Quic.Tests.MsQuicTests.ConnectWithClientCertificate', Elapsed: 00:02:05
Which is cover by #69792

Fix flags usage on Linux

5628b98

ghost added the area-System.Net.Quic label May 26, 2022

ghost assigned ManickaP May 26, 2022

ManickaP requested review from rzikm and wfurt May 26, 2022 18:55

rzikm approved these changes May 26, 2022

View reviewed changes

anrossi reviewed May 26, 2022

View reviewed changes

ManickaP merged commit ecb4e6c into dotnet:main May 27, 2022

ManickaP deleted the mapichov/quic-fix-load-cert branch May 27, 2022 08:05

rzikm mentioned this pull request May 27, 2022

Allow applications to query which TLS backend MsQuic uses. microsoft/msquic#2761

Closed

ghost locked as resolved and limited conversation to collaborators Jun 26, 2022

karelz added this to the 7.0.0 milestone Jul 19, 2022

	if (OperatingSystem.IsWindows())
	{
	config.Type = QUIC_CREDENTIAL_TYPE.CERTIFICATE_CONTEXT;
	config.CertificateContext = (void*)certificate.Handle;
	status = MsQuicApi.Api.ApiTable->ConfigurationLoadCredential(configurationHandle.QuicHandle, &config);
	}

Uh oh!

Conversation

ManickaP commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 26, 2022

Uh oh!

wfurt commented May 26, 2022

Uh oh!

anrossi commented May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wfurt commented May 26, 2022

Uh oh!

rzikm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManickaP May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManickaP commented May 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ManickaP commented May 26, 2022 •

edited

Loading

anrossi commented May 26, 2022 •

edited

Loading

ManickaP May 26, 2022 •

edited

Loading