add TLS 1.3 support to WinHttp #58590

wfurt · 2021-09-02T21:54:15Z

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.

blocking #58570
fixes #58587

ghost · 2021-09-02T21:54:21Z

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

Issue Details

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.

blocking #58570
fixes #58587

Author:	wfurt
Assignees:	wfurt
Labels:	`area-System.Net.Http`, `os-windows`
Milestone:	-

wfurt · 2021-09-02T21:56:02Z

BTW Here is local test run:

=== TEST EXECUTION SUMMARY ===
     System.Net.Http.WinHttpHandler.Functional.Tests  Total: 1134, Errors: 0, Failed: 0, Skipped: 25, Time: 122.556s

and 4.8 Framework

  ----- start Thu 09/02/2021 14:39:14.05 ===============  To repro directly: =====================================================
  pushd C:\Users\test\github\wfurt-runtime\artifacts\bin\System.Net.Http.WinHttpHandler.Functional.Tests\net48-windows-Debug\
  xunit.console.exe System.Net.Http.WinHttpHandler.Functional.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing -verbose -parallel none
  popd
  ===========================================================================================================
    Discovering: System.Net.Http.WinHttpHandler.Functional.Tests (app domain = on [no shadow copy], method display = ClassAndMethod, method display options = None)
    Discovered:  System.Net.Http.WinHttpHandler.Functional.Tests (found 171 of 293 test cases)
    Starting:    System.Net.Http.WinHttpHandler.Functional.Tests (parallel test collections = off, max threads = 4)

…

=== TEST EXECUTION SUMMARY ===
     System.Net.Http.WinHttpHandler.Functional.Tests  Total: 558, Errors: 0, Failed: 0, Skipped: 25, Time: 17.612s
  ----- end Thu 09/02/2021 14:39:33.08 ----- exit code 0 ----------------------------------------------------------

all tests are passing on Windows 11.

ManickaP

LGTM

karelz · 2021-09-03T08:48:58Z

src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.SslProtocols.cs

                    handler.SslProtocols = SslProtocols.Tls | SslProtocols.Tls11 | SslProtocols.Tls12 | SslProtocols.Tls13;
 #else
-                    handler.SslProtocols = SslProtocols.Tls | SslProtocols.Tls11 | SslProtocols.Tls12;
+                    handler.SslProtocols = SslProtocols.Tls | SslProtocols.Tls11 | SslProtocols.Tls12 | (SslProtocols)12288;


I think the reason for 12288 is that's what we use in the definition:

runtime/src/libraries/System.Net.Primitives/ref/System.Net.Primitives.cs

Line 524 in 9f6da55

Tls13 = 12288,

Ah, that is rather ugly. I wonder why? Perhaps it was auto-generated at some point?
I would still consider that hex values are more developer-friendly for reading ...

BTW: Definition is technically hex-defined:

runtime/src/libraries/System.Net.Primitives/src/System/Net/SecureProtocols/SslEnumTypes.cs

Line 18 in 57bfe47

Tls13 = Interop.SChannel.SP_PROT_TLS1_3,

runtime/src/libraries/Common/src/Interop/Windows/SChannel/Interop.SchProtocols.cs

Lines 31 to 33 in 57bfe47

public const int SP_PROT_TLS1_3_SERVER = 0x00001000;

public const int SP_PROT_TLS1_3_CLIENT = 0x00002000;

public const int SP_PROT_TLS1_3 = (SP_PROT_TLS1_3_SERVER | SP_PROT_TLS1_3_CLIENT);

karelz · 2021-09-03T08:49:11Z

src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs


        private void SetSessionHandleTlsOptions(SafeWinHttpHandle sessionHandle)
        {
+            const SslProtocols Tls13 = (SslProtocols)12288; // enum is missing in .NET Standard


Suggested change

const SslProtocols Tls13 = (SslProtocols)12288; // enum is missing in .NET Standard

const SslProtocols Tls13 = (SslProtocols)0x3000; // enum is missing in .NET Standard

I took the value from documentation. But I don't really care. It seems like the SP_PROT is platform specific mapping that happens to be the same on Windows.
It feels like this is really matter of personal preference.

karelz · 2021-09-03T08:56:15Z

Related test failures:

    System.Net.Http.Functional.Tests.PlatformHandler_HttpClientHandler.GetAsync_TrailingHeaders_Ignored(includeTrailerHeader: False) [FAIL]
      System.Net.Http.HttpRequestException : An error occurred while sending the request.
      ---- System.Net.Http.WinHttpException : Error 87 calling WinHttpSetOption, 'The parameter is incorrect.'.

I assume we need to check OS version if the option is supported or not, prior to setting it.

Win7 - 127 failures
Win8.1 - 129 failures
Win10 - 137 failures

karelz · 2021-09-03T08:56:54Z

Once we have it working, we should consider porting it to 6.0 -- that way we will avoid backporting it to servicing once Win11 ships officially.

karelz · 2021-09-06T10:00:29Z

/backport to release/6.0

github-actions · 2021-09-06T10:00:44Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1205541997

wfurt added 3 commits September 2, 2021 14:44

add TLS 1.3 support to WinHttp

6bbe732

remove extra line

76277bb

fix comment

0ce2c48

wfurt added area-System.Net.Http os-windows labels Sep 2, 2021

wfurt requested a review from a team September 2, 2021 21:54

wfurt self-assigned this Sep 2, 2021

ManickaP approved these changes Sep 3, 2021

View reviewed changes

karelz approved these changes Sep 3, 2021

View reviewed changes

karelz added this to the 7.0.0 milestone Sep 3, 2021

add check if WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_3 is supported

5e33673

wfurt merged commit 2e36b60 into dotnet:main Sep 4, 2021

wfurt deleted the winhttp_58587 branch September 4, 2021 02:23

github-actions bot mentioned this pull request Sep 6, 2021

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

Merged

karelz mentioned this pull request Sep 15, 2021

WinHttp tests with Tls 1.3 are failing #58587

Closed

ghost locked as resolved and limited conversation to collaborators Oct 6, 2021

	handler.SslProtocols = SslProtocols.Tls \| SslProtocols.Tls11 \| SslProtocols.Tls12 \| (SslProtocols)12288;
	handler.SslProtocols = SslProtocols.Tls \| SslProtocols.Tls11 \| SslProtocols.Tls12 \| (SslProtocols)0x3000;

	public const int SP_PROT_TLS1_3_SERVER = 0x00001000;
	public const int SP_PROT_TLS1_3_CLIENT = 0x00002000;
	public const int SP_PROT_TLS1_3 = (SP_PROT_TLS1_3_SERVER \| SP_PROT_TLS1_3_CLIENT);

	const SslProtocols Tls13 = (SslProtocols)12288; // enum is missing in .NET Standard
	const SslProtocols Tls13 = (SslProtocols)0x3000; // enum is missing in .NET Standard

add TLS 1.3 support to WinHttp #58590

add TLS 1.3 support to WinHttp #58590

Uh oh!

Conversation

wfurt commented Sep 2, 2021

Uh oh!

ghost commented Sep 2, 2021

Uh oh!

wfurt commented Sep 2, 2021

Uh oh!

ManickaP left a comment

Choose a reason for hiding this comment

Uh oh!

karelz Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

ManickaP Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

karelz Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

karelz Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

wfurt Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

karelz commented Sep 3, 2021

Uh oh!

karelz commented Sep 3, 2021

Uh oh!

karelz commented Sep 6, 2021

Uh oh!

github-actions bot commented Sep 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants