Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[QUIC] Improve MSQuic OS Version detection #32575

Closed
Tratcher opened this issue Feb 19, 2020 · 11 comments · Fixed by #54488
Closed

[QUIC] Improve MSQuic OS Version detection #32575

Tratcher opened this issue Feb 19, 2020 · 11 comments · Fixed by #54488
Assignees
Milestone

Comments

@Tratcher
Copy link
Member

OperatingSystem ver = Environment.OSVersion;
if (ver.Platform == PlatformID.Win32NT && ver.Version < new Version(10, 0, 19041, 0))

Environment.OSVersion is an unreliable API that reports old versions for compat purposes. It only reports the correct Win10 version if an app manifest is added. VS and the test runner have this manifest but sample apps and customer apps won't.
https://stackoverflow.com/questions/33328739/system-environment-osversion-returns-wrong-version

We're going to need a different way to check for this support. Worst case we look at the registry.

@am11
Copy link
Member

am11 commented Feb 20, 2020

fwiw, dotnet-cli uses this property for dotnet --info, which I think does not rely on the app manifest:

public static string OperatingSystemVersion { get; } = PlatformApis.GetOSVersion();

@scalablecory scalablecory added this to the 5.0 milestone Feb 20, 2020
@scalablecory scalablecory removed the untriaged New issue has not been triaged by the area owner label Feb 20, 2020
@jkotas
Copy link
Member

jkotas commented Feb 20, 2020

Microsoft.DotNet.PlatformAbstractions package is obsolete.

P/Invoking Ntdll.RtlGetVersionEx is your best bet.

@jkotas
Copy link
Member

jkotas commented Feb 20, 2020

It only reports the correct Win10 version if an app manifest is added. VS and the test runner have this manifest but sample apps and customer apps won't.

This is considered to be a feature. It is not necessary something you want to "fix".

@am11
Copy link
Member

am11 commented Feb 20, 2020

Microsoft.DotNet.PlatformAbstractions package is obsolete.

will its sources be removed and dotnet-cli switch to p/invoke?

Edit: nvm, found the issue tracking it - #3470

@Tratcher
Copy link
Member Author

Tratcher commented Feb 20, 2020

This is considered to be a feature. It is not necessary something you want to "fix".

This version check is to see if the OS has the right TLS 1.3 support. Http3 will have other opt-in guards to maintain compat. Once a developer opts in, false negatives from this version check are an unnecessary hurdle and support burden.

@Tratcher
Copy link
Member Author

#33643 fixed Environment.OSVersion so we can depend on that again.

@eerhardt
Copy link
Member

@Tratcher - do you want me to update #36302 to add back the Environment.OSVersion check?

@Tratcher
Copy link
Member Author

@jkotalik have you confirmed which version we need?

@jkotalik
Copy link
Contributor

No, let's just merge what we have in that #36302 for now. I'll need to figure out if the Windows slow preview train has schannel support, which will take a while.

@scalablecory scalablecory modified the milestones: 5.0.0, 6.0.0 Aug 11, 2020
@scalablecory
Copy link
Contributor

I'm thinking it's better to implement this in MsQuic -- microsoft/msquic#92

@ManickaP
Copy link
Member

ManickaP commented Apr 15, 2021

This can be covered by analyzers with SupportedOSPlatform, first crude draft is already in #49261, we just need a complete list of supported OS/versions/distros.

edit:
This will not help with checking the minimal Windows version. However, that should be a one-liner in S.N.Quic initialization: https://docs.microsoft.com/en-us/dotnet/api/system.operatingsystem.iswindowsversionatleast?view=net-5.0.

@CarnaViire CarnaViire self-assigned this Jun 21, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 21, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 24, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.