[QUIC] Add Windows version check to QUIC initialization #54488

CarnaViire · 2021-06-21T12:46:20Z

I took the version number from our QUIC readme (OS Build 20145.1000)

ghost · 2021-06-21T12:46:27Z

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

Issue Details

I took the version number from our QUIC readme (OS Build 20145.1000)

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

wfurt

LGTM

wfurt · 2021-06-21T13:04:48Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs

@@ -121,6 +123,12 @@ private MsQuicApi(NativeApi* vtable)

        static MsQuicApi()
        {
+            if (OperatingSystem.IsWindows() && !IsWindowsVersionSupported())
+            {
+                IsQuicSupported = false;


Should we add trace message in case we need to debug it the field? Or is this going to be documented when we have QUIC API?

Yes, I guess it makes sense to log current and min supported version here

Hmm I guess there is no reliable way to get current Windows version, that's why #32575 was created in the first place. I will only log min supported version then

Environment.OSVersion was changed to report current OS version as part of .NET 5. Should be safe to use if the desire is still there.

ManickaP

LGTM, thanks!

scalablecory

@nibanks is MsQuic tracking a minimum Windows version?

We should make sure whatever version we set this to, we actually test on. We've probably only been testing with latest dev channel.

scalablecory · 2021-06-22T00:02:36Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs

@@ -9,6 +9,8 @@ namespace System.Net.Quic.Implementations.MsQuic.Internal
 {
    internal unsafe sealed class MsQuicApi
    {
+        private static readonly Version MinWindowsVersion = new Version(10, 0, 20145, 1000);


Not much worth in a field since it is only initialized and accessed once. A property is fine.

Suggested change

private static readonly Version MinWindowsVersion = new Version(10, 0, 20145, 1000);

private static Version MinWindowsVersion => new Version(10, 0, 20145, 1000);

It's actually accessed 4 times on check and 5th time on logging...

scalablecory · 2021-06-22T00:03:56Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Internal/MsQuicApi.cs

@@ -121,6 +123,12 @@ private MsQuicApi(NativeApi* vtable)

        static MsQuicApi()
        {
+            if (OperatingSystem.IsWindows() && !IsWindowsVersionSupported())
+            {
+                IsQuicSupported = false;


Environment.OSVersion was changed to report current OS version as part of .NET 5. Should be safe to use if the desire is still there.

nibanks · 2021-06-22T00:31:48Z

@nibanks is MsQuic tracking a minimum Windows version?

Not specifically we aren't. The TLS dependency is the biggest issue. If you use Schannel, Windows Server 2022 is probably the earliest you can officially support. If you used OpenSSL, you can go back at least as far as Windows Server 2019, likely 2016. There are performance differences as you use newer Windows OS builds (at the UDP layer) but no absolute requirements (beyond TLS).

CarnaViire · 2021-06-22T14:07:09Z

We should make sure whatever version we set this to, we actually test on. We've probably only been testing with latest dev channel.

Yep, I use Insider Preview that is constantly updating - forcibly - so I cannot fix the Windows version on my side. Do you see any other options? @scalablecory

nibanks · 2021-06-22T14:31:34Z

Are you trying to focus on a client or server build of Windows? I believe Insiders is the only option for client right now. For server, 2022 is the way to go.

CarnaViire · 2021-06-23T17:26:50Z

@nibanks Pardon my ignorance, but what would be the difference between Windows client vs server build with respect to QUIC? I would expect it to work on both of them (as Kestrel uses the same library). I also don't know how the versions coincide between client and server builds, API OperatingSystem.IsWindowsVersionAtLeast doesn't seem to distinguish it. Do you think there should be two different versions to check, depending on whether current system is client or server build?

nibanks · 2021-06-23T18:51:00Z

@nibanks Pardon my ignorance, but what would be the difference between Windows client vs server build with respect to QUIC? I would expect it to work on both of them (as Kestrel uses the same library). I also don't know how the versions coincide between client and server builds, API OperatingSystem.IsWindowsVersionAtLeast doesn't seem to distinguish it. Do you think there should be two different versions to check, depending on whether current system is client or server build?

Beyond having different release dates (and therefore one is more recent than the other), they sometimes have different features enabled by default or default tuning constants. Also, Windows Server 2022 is the only existing (almost) released Windows build that could practically be used, and I was making sure that was Ok.

CarnaViire · 2021-06-23T20:01:25Z

It seems that there is no API in .NET to distinguish server vs workstation OS build. We need to agree on what version to put here, given that both server and workstation Windows builds seem to have interchanging numbering. We also need to decide whether we actually want to find the earliest build number QUIC will work on, or for now just to be sure that starting from build X it's definitely ok and what's earlier we don't care. I don't think the first way is worth the effort as all the working builds are previews anyway at this point.

Workstation build I have right now is 21390; I see here https://www.microsoft.com/en-us/software-download/windowsinsiderpreviewserver that server build is 20344.

Options I see for now:

put 20344 as "smallest denominator"
leave 20145.1000 there as it is smaller than both of these and it (probably) made its way to Readme for a reason
don't add any checks at all - wait for public versions to ship - seems to be worse than adding at least some (preview) version check?
anything else I didn't come up with?

cc @nibanks @ManickaP @wfurt @geoffkizer

karelz · 2021-06-24T10:04:36Z

I would use 20145.1000 from these reasons:

It encompasses Server builds we know support Schannel for QUIC.
It was in the Readme for a reason.
If it was wrong for client Insider builds, we would have heard about it by now I hope.
Even if it was wrong and nobody told us, it is unlikely anyone will be using ancient client Insider builds (which are even not officially supported), so all we need to make sure is that recent client Insider builds are higher version than the one we check for ... which they are.

wfurt · 2021-06-24T12:35:50Z

I agree with @karelz. This is minimal check e.g. this does not represent best or supported configuration.

ManickaP

wegylexy · 2021-07-13T00:14:08Z

I read about 10.0.20170 for client is the first version having SChannel TLS 1.3 enabled by default, not sure if there is anything between the 20145 server and that.

wegylexy · 2021-07-13T00:21:43Z

Nevertheless, since the OpenSSL version of msquic.dll may be dropped into the DLL lookup path and worked (until broken by #51015), adding such check prevents earlier versions of Windows to use the OpenSSL alternative.

wfurt · 2021-07-13T00:26:29Z

That could be fixed. But OpenSSL on Windows is not going to be supported configuration @wegylexy. (at least not from .NET prospective)

wegylexy · 2021-07-13T01:34:10Z

How about SChannel with TLS 1.3 in the next Windows 10 public release? Many CPUs are too old to run Windows 11.
e.g. I have TPM 2.0 and fulfil all other requirements, just it's a 7th-gen Intel Core, M$FT says I can only get Insider Preview builds until RTM.

karelz · 2021-07-13T08:53:15Z

@wegylexy that is not a decision we can influence -- I would recommend to follow up with feedback channel for Windows directly.

CarnaViire added 2 commits June 21, 2021 14:13

Add Windows version check to QUIC initialization

29a5e4a

Add min version constant

67efff0

CarnaViire requested review from scalablecory, ManickaP, geoffkizer and wfurt June 21, 2021 12:46

dotnet-issue-labeler bot added the area-System.Net.Quic label Jun 21, 2021

wfurt approved these changes Jun 21, 2021

View reviewed changes

Add log message

d307022

ManickaP approved these changes Jun 21, 2021

View reviewed changes

Merge branch 'main' into quic-win-version-detection

d64f523

scalablecory approved these changes Jun 22, 2021

View reviewed changes

Add current Windows version to log

8904e53

karelz assigned CarnaViire Jun 22, 2021

wfurt approved these changes Jun 24, 2021

View reviewed changes

ManickaP approved these changes Jun 24, 2021

View reviewed changes

nibanks approved these changes Jun 24, 2021

View reviewed changes

CarnaViire merged commit 8ecdf98 into dotnet:main Jun 24, 2021

ManickaP mentioned this pull request Jul 7, 2021

QUIC: Detecting whether QUIC is supported #52145

Closed

CarnaViire deleted the quic-win-version-detection branch July 13, 2021 09:18

karelz added this to the 6.0.0 milestone Jul 15, 2021

ghost locked as resolved and limited conversation to collaborators Aug 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QUIC] Add Windows version check to QUIC initialization #54488

[QUIC] Add Windows version check to QUIC initialization #54488

CarnaViire commented Jun 21, 2021

ghost commented Jun 21, 2021

wfurt left a comment

wfurt Jun 21, 2021

CarnaViire Jun 21, 2021

CarnaViire Jun 21, 2021

scalablecory Jun 22, 2021

ManickaP left a comment

scalablecory left a comment

scalablecory Jun 22, 2021

CarnaViire Jun 22, 2021

scalablecory Jun 22, 2021

nibanks commented Jun 22, 2021

CarnaViire commented Jun 22, 2021

nibanks commented Jun 22, 2021

CarnaViire commented Jun 23, 2021

nibanks commented Jun 23, 2021

CarnaViire commented Jun 23, 2021

karelz commented Jun 24, 2021

wfurt commented Jun 24, 2021

ManickaP left a comment

wegylexy commented Jul 13, 2021

wegylexy commented Jul 13, 2021

wfurt commented Jul 13, 2021

wegylexy commented Jul 13, 2021

karelz commented Jul 13, 2021

	private static readonly Version MinWindowsVersion = new Version(10, 0, 20145, 1000);
	private static Version MinWindowsVersion => new Version(10, 0, 20145, 1000);

[QUIC] Add Windows version check to QUIC initialization #54488

[QUIC] Add Windows version check to QUIC initialization #54488

Conversation

CarnaViire commented Jun 21, 2021

ghost commented Jun 21, 2021

wfurt left a comment

Choose a reason for hiding this comment

wfurt Jun 21, 2021

Choose a reason for hiding this comment

CarnaViire Jun 21, 2021

Choose a reason for hiding this comment

CarnaViire Jun 21, 2021

Choose a reason for hiding this comment

scalablecory Jun 22, 2021

Choose a reason for hiding this comment

ManickaP left a comment

Choose a reason for hiding this comment

scalablecory left a comment

Choose a reason for hiding this comment

scalablecory Jun 22, 2021

Choose a reason for hiding this comment

CarnaViire Jun 22, 2021

Choose a reason for hiding this comment

scalablecory Jun 22, 2021

Choose a reason for hiding this comment

nibanks commented Jun 22, 2021

CarnaViire commented Jun 22, 2021

nibanks commented Jun 22, 2021

CarnaViire commented Jun 23, 2021

nibanks commented Jun 23, 2021

CarnaViire commented Jun 23, 2021

karelz commented Jun 24, 2021

wfurt commented Jun 24, 2021

ManickaP left a comment

Choose a reason for hiding this comment

wegylexy commented Jul 13, 2021

wegylexy commented Jul 13, 2021

wfurt commented Jul 13, 2021

wegylexy commented Jul 13, 2021

karelz commented Jul 13, 2021