Skip to content

add TLS 1.3 support to WinHttp #58590

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.

Sign up for GitHub

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

Merged
wfurt merged 4 commits into from
Sep 4, 2021
Merged

add TLS 1.3 support to WinHttp #58590

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6bbe732
add TLS 1.3 support to WinHttp
wfurt Sep 2, 2021
76277bb
remove extra line
wfurt Sep 2, 2021
0ce2c48
fix comment
wfurt Sep 2, 2021
5e33673
add check if WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_3 is supported
wfurt Sep 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libraries/Common/src/Interop/Windows/WinHttp/Interop.winhttp_types.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ internal static partial class WinHttp
public const uint WINHTTP_FLAG_SECURE_PROTOCOL_TLS1 = 0x00000080;
public const uint WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_1 = 0x00000200;
public const uint WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2 = 0x00000800;
public const uint WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_3 = 0x00002000;

public const uint SECURITY_FLAG_IGNORE_UNKNOWN_CA = 0x00000100;
public const uint SECURITY_FLAG_IGNORE_CERT_DATE_INVALID = 0x00002000;
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.SslProtocols.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public async Task GetAsync_AllowedSSLVersion_Succeeds(SslProtocols acceptedProto
#if !NETFRAMEWORK
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;

@karelz karelz Sep 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@ManickaP ManickaP Sep 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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,

@karelz karelz Sep 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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);

#endif
#pragma warning restore 0618
}
Expand Down
9 changes: 6 additions & 3 deletions src/libraries/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1156,6 +1156,7 @@ private void SetSessionHandleConnectionOptions(SafeWinHttpHandle sessionHandle)

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

@karelz karelz Sep 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@wfurt wfurt Sep 3, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

uint optionData = 0;
SslProtocols sslProtocols =
(_sslProtocols == SslProtocols.None) ? SecurityProtocol.DefaultSecurityProtocols : _sslProtocols;
Expand Down Expand Up @@ -1187,10 +1188,12 @@ private void SetSessionHandleTlsOptions(SafeWinHttpHandle sessionHandle)
optionData |= Interop.WinHttp.WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_2;
}

// As of Win10RS5 there's no public constant for WinHTTP + TLS 1.3
// This library builds against netstandard, which doesn't define the Tls13 enum field.
if ((sslProtocols & Tls13) != 0)
{
optionData |= Interop.WinHttp.WINHTTP_FLAG_SECURE_PROTOCOL_TLS1_3;
}

// If only unknown values (e.g. TLS 1.3) were asked for, report ERROR_INVALID_PARAMETER.
// If only unknown values were asked for, report ERROR_INVALID_PARAMETER.
if (optionData == 0)
{
throw WinHttpException.CreateExceptionUsingError(
Expand Down