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

CI matrix: Add windows 20H2 - fix TLS1.3 issue #62435

Closed
wants to merge 9 commits into from

Conversation

aik-jahoda
Copy link
Contributor

@aik-jahoda aik-jahoda commented Dec 6, 2021

When employ Windows server 20H2 into CI we got buch of TLS 1.3 errors:

System.ComponentModel.Win32Exception : The client and server cannot communicate, because they do not possess a common algorithm.

Usually it means the server/client doesn't support TLS 1.3

https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-heads-revert-62098-c4c66b16e5dd4304a8/System.Net.Security.Tests/1/console.77465e3d.log?%3F%253Fsv%253D2019-07-07%2526se%253D2021-12-21T09%25253A56%25253A18Z%2526sr%253Dc%2526sp%253Drl%2526sig%253D6ptrdfy%25252FY91GLeQ4bm8kc%25252FtyN0EXeNccWwtsbnhIAIw%25253D

replace #60054

part of #57947

@ghost
Copy link

ghost commented Dec 6, 2021

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

Issue Details

null

Author: aik-jahoda
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@aik-jahoda
Copy link
Contributor Author

Based on https://docs.microsoft.com/en-us/windows/win32/secauthn/protocols-in-tls-ssl--schannel-ssp- the TLS 1.3 is not supported (not explicitly, but there is no support for Windows 20H2)

Probably there is an issue with Windows version check.
/cc @wfurt

@@ -431,6 +432,8 @@ private static bool GetTls13Support()
{
client = Registry.GetValue(clientKey, "Enabled", null);
server = Registry.GetValue(serverKey, "Enabled", null);
Console.WriteLine($"client {client}");
Console.WriteLine($"client {client}");
Copy link
Member

Choose a reason for hiding this comment

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

Should be server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it served its purpose for getting the CI machine info. Reverting...

@wfurt
Copy link
Member

wfurt commented Dec 7, 2021

The support check will respect registry. Based on the output somebody/something enabled TLS 1.3 even if not officially supported. (The code was there for a while just not enabled by default)
My recommendation would be to find what enables it and remove that code.

@wfurt
Copy link
Member

wfurt commented Dec 7, 2021

BTW this may go back to base Helix image. I don't know who and how created that.
cc: @MattGal

@MattGal
Copy link
Member

MattGal commented Dec 7, 2021

BTW this may go back to base Helix image. I don't know who and how created that. cc: @MattGal

This is Helix-Server-DataCenter-20h2-11-09-2021, created I believe by @ChulHul . If you can list a specific modification it needs, I'm happy to help but that's unclear from this issue.

@wfurt
Copy link
Member

wfurt commented Dec 7, 2021

Thanks for quick update @MattGal.
It seems like from the debug and side conversion with @aik-jahoda KEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\SecurityProviders\SCHANNEL\Protocols\TLS 1.3\Client\Enabled and KEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\SecurityProviders\SCHANNEL\Protocols\TLS 1.3\Server\Enabled are set , forcing TLS 1.3 on platform that officially does not support it.

Can we either flip them to 0 or simply leave them empty?

@aik-jahoda
Copy link
Contributor Author

The Windows version is 10.0.19042 , however the check in Platform detection https://github.com/dotnet/runtime/blob/main/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs#L413
assumes the TLS 1.3 is available from 10.0.19041.

@wfurt, shouldn't be safer to change the IsWindows10Version2004OrGreater (20H1) to is 20H2 or greater (10.0.19044)? It would prevent this error when the registry are set but the Windows doesn't support it

@aik-jahoda
Copy link
Contributor Author

New helix machine will be available 12/15

@aik-jahoda
Copy link
Contributor Author

@ericstj
Copy link
Member

ericstj commented Jan 10, 2022

@safern do we need to rerun this to make sure the test results are still OK from outerloop or is it OK to merge?

@wfurt
Copy link
Member

wfurt commented Jan 10, 2022

The failing tests were innerloop in the past. We can run outer loop but I don't think it is necessary.

@danmoseley danmoseley closed this Jan 10, 2022
@danmoseley danmoseley reopened this Jan 10, 2022
@danmoseley
Copy link
Member

No harm in re-running at least the regular checks since it's been a while..

@ericstj
Copy link
Member

ericstj commented Feb 28, 2022

Closing this since the PR is now stale and the overall change is tracked by #57947

@ericstj ericstj closed this Feb 28, 2022
@jkotas jkotas deleted the jajahoda/cimatrix20h2 branch March 4, 2022 16:08
@ghost ghost locked as resolved and limited conversation to collaborators Apr 3, 2022
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.

6 participants