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

improve Tls12 detection on Windows7 #67935

Merged
merged 3 commits into from
Apr 13, 2022
Merged

improve Tls12 detection on Windows7 #67935

merged 3 commits into from
Apr 13, 2022

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Apr 12, 2022

Protocol detection is trickier on Window7. The Enabled is not sufficient and recent Helix updated confused our platform detection.

To make Tls12 working DisabledByDefault also must be set to 0.

We will need another Helix change to get Tls12 truly working. I did private test run with DisabledByDefault=0 and all tests could pass (including outer loop)

fixes #67687 and replaces #67904
fixes #67712

@wfurt wfurt added area-System.Net.Security test-enhancement Improvements of test source code labels Apr 12, 2022
@wfurt wfurt requested review from rzikm and a team April 12, 2022 23:53
@wfurt wfurt self-assigned this Apr 12, 2022
@ghost
Copy link

ghost commented Apr 12, 2022

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

Issue Details

Protocol detection is trickier on Window7. The Enabled is not sufficient and recent Helix updated confused our platform detection.

To make Tls12 working DisabledByDefault also must be set to 0.

We will need another Helix change to get Tls12 truly working. I did private test run with DisabledByDefault=0 and all tests could pass (including outer loop)

fixes #67687 and replaces #67904
fixes #67712

Author: wfurt
Assignees: wfurt
Labels:

area-System.Net.Security, test-enhancement

Milestone: -

return GetProtocolSupportFromWindowsRegistry(SslProtocols.Tls11, defaultProtocolSupport);
if (IsWindows7)
{
return GetProtocolSupportFromWindowsRegistry(SslProtocols.Tls11, false, true);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can you name these bools? It's hard to tell without that what they mean.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated. I also updated many comments as the logic is getting more complicated.

@wfurt
Copy link
Member Author

wfurt commented Apr 13, 2022

btw we will need to pick this if we see Windows 7 failures in release branches @carlossanlop

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM

@wfurt
Copy link
Member Author

wfurt commented Apr 13, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wfurt wfurt merged commit 16b7255 into dotnet:main Apr 13, 2022
@wfurt wfurt deleted the win7 branch April 13, 2022 21:30
rzikm pushed a commit to rzikm/dotnet-runtime that referenced this pull request Apr 21, 2022
carlossanlop pushed a commit that referenced this pull request May 3, 2022
* Resolve System.Net.Security.Tests.LoggingTest SkipTestException failure (#65322)

* improve Tls12 detection on Windows7 (#67935)

* disable Tls 1.0 and 1.1 tests on new Windows (#68083)

* Don't throw from RemoteExecutor on SkipTestExceptions (#65105)

* update SSL tests to deal better with disabled protocols (#65120)

* update SSL tests to deal better with disabled protocols

* Improve detection of Null encryption on Windows

* update expectation for Mismatched protocols

* update detection

* wrap win32 exception

* update ProtocolMismatchData sets

* remove debug print

* final cleanup

* generate mismatch data

* avoid SslProtocols.Default

Co-authored-by: Miha Zupan <[email protected]>
Co-authored-by: Tomas Weinfurt <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators May 14, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Security test-enhancement Improvements of test source code
Projects
None yet
4 participants