basic certificate handling for quic #50613

wfurt · 2021-04-01T21:37:17Z

With microsoft/msquic#1411 support for STUB Tls was removed and we only have real option left.

This change adds server support for normal X509Certificate and plus usual verification callback so the fate of the handshake can be decided from .NET.

Right now, this is supported only ion Windows, Linux & macOS coming (hopefully) soon.

This is also missing support for custom certificate chains, SslStreamCertificateContext, and client certificates.

cc: @nibanks

ghost · 2021-04-01T21:37:35Z

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

Issue Details

With microsoft/msquic#1411 support for STUB Tls was removed and we only have real option left.

This change adds server support for normal X509Certificate and plus usual verification callback so the fate of the handshake can be decided from .NET.

Right now, this is supported only ion Windows, Linux & macOS coming (hopefully) soon.

This is also missing support for custom certificate chains, SslStreamCertificateContext, and client certificates.

cc: @nibanks

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

wfurt · 2021-04-02T01:29:13Z

Basic certificates should now work on Linux & macOS well.
Some networking tests are failing like this on macOS

System.Net.Quic.Tests.QuicConnectionTests_MsQuicProvider.TestConnect [FAIL]
        Assert.Equal() Failure
        Expected: 127.0.0.1:63445
        Actual:   [::]:63445

I will investigate in separate effort.

...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs

nibanks · 2021-04-02T03:19:21Z

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

+            }
+            else
+            {
+                flags |= QUIC_CREDENTIAL_FLAGS.INDICATE_CERTIFICATE_RECEIVED | QUIC_CREDENTIAL_FLAGS.NO_CERTIFICATE_VALIDATION;


You never want to let the TLS stack do the certificate validation?

That how SslStream works now. Long term, we may check if custom callback is present or now. But that is still problematic (at least on Linux) as X509Chain would consider user stores while OS does not have any clue about them. So we would end up in situation when SslStream works and Quick does not or vice versa.

nibanks · 2021-04-02T03:27:37Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

+
+            try
+            {
+                if (certificate == null)


I don't expect this to ever happen. We shouldn't be calling you with "Peer Certificate Received" if we didn't have a certificate. Personally, I'd say you could just assert here or something.

The null makes perfect sense for server side where client's certificate is optional. I just need to complete that part but the validation callback will be shared.

nibanks · 2021-04-02T05:06:51Z

This is supposed to match our maybe header, which uses uint32_t. @danmoseley commented on this pull request.

________________________________ In src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fruntime%2Fpull%2F50613%23discussion_r606071720&data=04%7C01%7Cnibanks%40microsoft.com%7Cd5cbe3864a3543a2cd6108d8f593ce93%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637529362500387665%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=SDie88n2VuWx%2FT8DBVDVmRCmxJ46jcvTdMv6FUomGR4%3D&reserved=0>:

@@ -272,6 +275,17 @@ internal struct CredentialConfigCertificateCertificateFileProtected

internal string PrivateKeyPassword; } + [StructLayout(LayoutKind.Sequential)] + internal struct CredentialConfigCertificatePkcs12 + { + internal IntPtr Asn1Blob; + + internal int Asn1BlobLength; We mainly use uint when necessary to match native headers.

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicEnums.cs

...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs

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

ManickaP · 2021-04-06T14:16:12Z

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

+                        byte[] asn1 = certificate.Export(X509ContentType.Pkcs12);
+                        unsafe
+                        {
+                            fixed (void* ptr = asn1)


I think this is the only occurrence of fixed in here. I'm not an interop expert, but should we rather use GCHandle as we do in other places? Is there any difference in those two? If not should we rather stick to the same approach throughout?
cc: @scalablecory

You need to allocate GCHandle to preserve object for longer time. This basically says "don't move asn1 while I'm in this block" and it is much simpler. (and does not work for the other cases.

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

…Cert

scalablecory · 2021-04-08T13:32:12Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

+
+            if (!OperatingSystem.IsWindows())
+            {
+                // TODO fix validation with OpenSSL


What is remaining to do here? Is there an issue?

The issue is that we don't have good way how to get certificate chain right now with OpenSSL.
There is not specific issue AFAIK. I mentally do this as contributions to #49574.
I will follow-up if/when microsoft/msquic#1450 (+implementation) is ready.

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs

wfurt · 2021-04-08T18:06:42Z

all the concerns should be addressed. Following outstanding issues will be follow-up changes:

certificate chain handling
client certificates
certificate validation with OpenSSL

JamesNK · 2021-04-08T23:24:23Z

I created a number of TLS issues for QUIC. It would be good to know which ones your PR makes progress on.

For #49574, it looks like this checks off RemoteCertificateValidationCallback and CertificateRevocationCheckMode.

Does this PR start to unblock #50156?

wfurt · 2021-04-08T23:29:16Z

With this, self-signed (and other certificates) will work on Windows @JamesNK. Linux/macOS is getting closer.
I'm planning to go through the open issues once we have platform parity.

basic certificate handling for quic

1912525

wfurt added the area-System.Net.Quic label Apr 1, 2021

wfurt added 2 commits April 1, 2021 18:16

fix linux

2ff0772

fix macOS

0e33435

nibanks reviewed Apr 2, 2021

View reviewed changes

...es/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/MsQuicNativeMethods.cs Outdated Show resolved Hide resolved

nibanks reviewed Apr 2, 2021

View reviewed changes

ManickaP reviewed Apr 6, 2021

View reviewed changes

wfurt added 2 commits April 6, 2021 10:30

feedback from review

cf0ea98

Merge branch 'quicCert' of https://github.com/wfurt/runtime into quic…

0d27128

…Cert

scalablecory reviewed Apr 8, 2021

View reviewed changes

scalablecory approved these changes Apr 8, 2021

View reviewed changes

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicConnection.cs Outdated Show resolved Hide resolved

feedback from review

7a374f7

nibanks approved these changes Apr 8, 2021

View reviewed changes

This was referenced Apr 8, 2021

Runtime jobs being abandoned due to infra #50746

Closed

Runtime jobs being abandoned due to infra #50747

Closed

wfurt closed this Apr 8, 2021

wfurt reopened this Apr 8, 2021

wfurt merged commit 01b7e73 into dotnet:main Apr 9, 2021

wfurt deleted the quicCert branch April 9, 2021 16:29

This was referenced Apr 14, 2021

Update msquic revision in docs after certificate work. #51230

Merged

[QUIC] Use client TLS options in QUIC #32069

Closed

ghost locked as resolved and limited conversation to collaborators May 9, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic certificate handling for quic #50613

basic certificate handling for quic #50613

wfurt commented Apr 1, 2021

ghost commented Apr 1, 2021

wfurt commented Apr 2, 2021

nibanks Apr 2, 2021 •

edited

Loading

wfurt Apr 2, 2021

nibanks Apr 2, 2021

wfurt Apr 2, 2021

nibanks commented Apr 2, 2021 via email •

edited by wfurt

Loading

ManickaP Apr 6, 2021

wfurt Apr 6, 2021

scalablecory Apr 8, 2021

wfurt Apr 8, 2021

wfurt commented Apr 8, 2021

JamesNK commented Apr 8, 2021

wfurt commented Apr 8, 2021

basic certificate handling for quic #50613

basic certificate handling for quic #50613

Conversation

wfurt commented Apr 1, 2021

ghost commented Apr 1, 2021

wfurt commented Apr 2, 2021

nibanks Apr 2, 2021 • edited Loading

Choose a reason for hiding this comment

wfurt Apr 2, 2021

Choose a reason for hiding this comment

nibanks Apr 2, 2021

Choose a reason for hiding this comment

wfurt Apr 2, 2021

Choose a reason for hiding this comment

nibanks commented Apr 2, 2021 via email • edited by wfurt Loading

ManickaP Apr 6, 2021

Choose a reason for hiding this comment

wfurt Apr 6, 2021

Choose a reason for hiding this comment

scalablecory Apr 8, 2021

Choose a reason for hiding this comment

wfurt Apr 8, 2021

Choose a reason for hiding this comment

wfurt commented Apr 8, 2021

JamesNK commented Apr 8, 2021

wfurt commented Apr 8, 2021

nibanks Apr 2, 2021 •

edited

Loading

nibanks commented Apr 2, 2021 via email •

edited by wfurt

Loading