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

[release/6.0] add TLS 1.3 support to WinHttp #58718

Merged
merged 4 commits into from
Sep 7, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 6, 2021

Backport of #58590 to release/6.0

WinHttp now has and the flag and it seems to work right on Windows 11

#define WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_3 0x00002000

.NET Framework 4.8 now supports Tls13 as well somebody can use it with casting int like the test change.
It really does not matter as long as running on supported OS and WinHttp as all the work is done by native code.

Fixes #58587

/cc @karelz @wfurt

Customer Impact

Users of WinHttpHandler cannot use TLS1.3 on Win11+ (where the OS supports it).
We would need to release WinHttpHandler NuGet package patch (via servicing of .NET 6) once Win11 is public.

Testing

TODO: Win11 test run - @wfurt

Risk

Low. The component only sim-ships with .NET 6 as a standalone NuGet package.
The change only sets a flag on WinHttp (if it is supported).

@karelz karelz added this to the 6.0.0 milestone Sep 6, 2021
Copy link
Member

@karelz karelz left a comment

Choose a reason for hiding this comment

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

Pending green CI & manual Win11 verification.

@karelz
Copy link
Member

karelz commented Sep 6, 2021

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@karelz
Copy link
Member

karelz commented Sep 7, 2021

Unrelated infra problems:

  Discovering: System.ComponentModel.Composition.Registration.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.ComponentModel.Composition.Registration.Tests (found 98 of 130 test cases)
  Starting:    System.ComponentModel.Composition.Registration.Tests (parallel test collections = on, max threads = 4)
  Finished:    System.ComponentModel.Composition.Registration.Tests
=== TEST EXECUTION SUMMARY ===
   System.ComponentModel.Composition.Registration.Tests  Total: 98, Errors: 0, Failed: 0, Skipped: 0, Time: 1.505s
/private/tmp/helix/working/AFBC09DD/w/A0980903/e
----- end Mon Sep 6 07:26:42 PDT 2021 ----- exit code 0 ----------------------------------------------------------
exit code 0 means Exited Successfully
... (more lines in the log) ...
  Discovering: System.Net.Http.Unit.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Net.Http.Unit.Tests (found 892 test cases)
  Starting:    System.Net.Http.Unit.Tests (parallel test collections = on, max threads = 4)
(end of log)
  Discovering: System.Text.Encodings.Web.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Text.Encodings.Web.Tests (found 212 test cases)
  Starting:    System.Text.Encodings.Web.Tests (parallel test collections = on, max threads = 4)
(end of log)

@dotnet/dncenghot is it a known infra problem?

Re-running tests ...

@greenEkatherine
Copy link
Contributor

It is on OSX.1013.Amd64.Open
@premun did you see it before?

@greenEkatherine
Copy link
Contributor

all these failures happened on dci-mac-build-086.local, I put it offline

@premun
Copy link
Member

premun commented Sep 7, 2021

I haven't but this is not related to Apple/Android so I wouldn't know.

There were some issues around OSX machines getting their jobs killed - https://github.com/dotnet/core-eng/issues/14085

@greenEkatherine
Copy link
Contributor

I haven't but this is not related to Apple/Android so I wouldn't know.

There were some issues around OSX machines getting their jobs killed - dotnet/core-eng#14085

I see, I will raise ticket for DDFUN to fix that machine then

@karelz
Copy link
Member

karelz commented Sep 7, 2021

On re-run we got 3 different test suites stuck on the same machine dci-mac-build-086.local:

Given all the above, the CI results should be considered passing.
@danmoseley it is ready for your approval & merge.

@danmoseley
Copy link
Member

Approved - localized, low risk change to fix mainstream scenario regressing on Windows 11.

@danmoseley danmoseley added the Servicing-approved Approved for servicing release label Sep 7, 2021
@danmoseley danmoseley merged commit 8482854 into release/6.0 Sep 7, 2021
@danmoseley danmoseley deleted the backport/pr-58590-to-release/6.0 branch September 7, 2021 17:46
@danmoseley
Copy link
Member

@karelz not sure when the Windows 11 pipelines will be up. Will someone in your team run all your tests, including Outerloop, with 6.0 bits with this change in place? (I know @wfurt ran many of them in main)

@karelz
Copy link
Member

karelz commented Sep 7, 2021

Yes, we plan to run Win11 manually -- I actually wanted to make it testing prereq prior to merging (as todo in top post suggests), but I forgot about it and pinged you for approval prematurely ;( ... sorry for that!
I'll make sure we do it in next few days (@wfurt)

@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants