Added TLS 1.3 support for PS Core

[variableresistor]

Description

Added support for TLS 1.3

Issues Fixed

Fixes #443

References

Checklist

• You actually ran the code that you just wrote, especially if you did just "one last quick change".
• Comment-based help added/updated, including examples.
• Static analysis is reporting back clean.
• New/changed code adheres to our coding guidelines.
• Formatters were created for any new types being added.
• New/changed code continues to support the pipeline.
• Changes to the manifest file follow the manifest guidance.
• Unit tests were added/updated and are all passing. See testing guidelines. This includes making sure that all pipeline input variations have been covered.
• Relevant usage examples have been added/updated in USAGE.md.
• If desired, ensure your name is added to our Contributors list

[HowardWolosky]

@variableresistor - This is a great idea. I didn't realize that PS Core 7+ supported TLS 1.3.

That being said, I'm not confident with the change.

I opened up a Windows PowerShell console:

C:\Users\howard> $PSVersionTable.PSVersion

Major  Minor  Build  Revision
-----  -----  -----  --------
5      1      26100  2161

C:\Users\howard> $PSVersionTable.PSVersion -lt 7.0.0
False


C:\Users\howard> $PSVersionTable.PSVersion -lt 5.01.26100
False
C:\Users\howard> $PSVersionTable.PSVersion -lt 5.01.26101
False
C:\Users\howard> $PSVersionTable.PSVersion -lt 5.1.26101
False
C:\Users\howard> $PSVersionTable.PSVersion -lt 5.1.26100
False

Looking online, it looks like there's more that needs to be done than what you currently have in this PR.

[variableresistor]

@variableresistor - This is a great idea. I didn't realize that PS Core 7+ supported TLS 1.3.

That being said, I'm not confident with the change.

I opened up a Windows PowerShell console:

C:\Users\howard> $PSVersionTable.PSVersion

Major  Minor  Build  Revision
-----  -----  -----  --------
5      1      26100  2161

C:\Users\howard> $PSVersionTable.PSVersion -lt 7.0.0
False


C:\Users\howard> $PSVersionTable.PSVersion -lt 5.01.26100
False
C:\Users\howard> $PSVersionTable.PSVersion -lt 5.01.26101
False
C:\Users\howard> $PSVersionTable.PSVersion -lt 5.1.26101
False
C:\Users\howard> $PSVersionTable.PSVersion -lt 5.1.26100
False

Looking online, it looks like there's more that needs to be done than what you currently have in this PR.

Thanks @HowardWolosky for the quick reply. I'll check that link and get back with you shortly.

[variableresistor]

Fixed it. To test:

powershell -Command '$PSVersionTable.PSVersion -lt [version]"7.0.0"'
True
pwsh -Command '$PSVersionTable.PSVersion -lt [version]"7.0.0"'
False

[HowardWolosky]

I'm fine with this change I think, but when trying out PS Core 7.4.6 myself, I'm finding that TLS 1.2 is what is default, not 1.3. Am I missing something?

[HowardWolosky]

Suggested change

	if ($PSVersionTable.PSVersion -lt [version]"7.0.0")
	if ($PSVersionTable.PSVersion -lt [Version]'7.0.0')

Prefer single-quotes for non-interpreted strings, and appropriately capitalizing type names

[variableresistor]

Likely it's getting set in some other module. According to Microsoft's documentation SecurityProtocolType Enum, the default is SystemDefault. Specifically in the description, it says:
"Allows the operating system to choose the best protocol to use, and to block protocols that are not secure. Unless your app has a specific reason not to, you should use this value."

[variableresistor]

We COULD add an additional check:

[Net.ServicePointManager]::SecurityProtocol -lt [System.Net.SecurityProtocolType]::Tls12
# So the full check would be:
if ($PSVersionTable.PSVersion -lt [version]'7.0.0' -and [Net.ServicePointManager]::SecurityProtocol -lt [System.Net.SecurityProtocolType]::Tls12)
{
    [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12
}

What do you think?
EDIT: Changed double to single quotes

[HowardWolosky]

We COULD add an additional check:

[Net.ServicePointManager]::SecurityProtocol -lt [System.Net.SecurityProtocolType]::Tls12
# So the full check would be:
if ($PSVersionTable.PSVersion -lt [version]'7.0.0' -and [Net.ServicePointManager]::SecurityProtocol -lt [System.Net.SecurityProtocolType]::Tls12)
{
    [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12
}

What do you think? EDIT: Changed double to single quotes

I don't think that will quite work. The problem I see there is that it will return false when it's set to SystemDefault (since its enum value is 0), which then would ultimately have us back in the original state (from before this change) of setting the value to TLS 1.2.

The confusion that I have is the documentation (thanks for the link) indicates that SystemDefault is intended to allow the system to choose the best protocol to use, but I was previously forced to explicitly set it to TLS 1.2 because the system wasn't making that choice, so I'm not sure how much confidence to put into that SystemDefault value.

[variableresistor]

How about this?

if ($PSVersionTable.PSVersion -lt [version]'7.0.0' -and [Net.ServicePointManager]::SecurityProtocol -lt [System.Net.SecurityProtocolType]::Tls12)
{
  # Must do the comparison this way because of the Tls13 enumeration doesn't exist, then it will blow up
  if ( ([System.Net.SecurityProtocolType] | Get-Member -MemberType Property -Static).Name -contains 'Tls13')
  {
    [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls13
  }
  else
  {
    # Revert to the old behavior
    [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12
  }
}
[Net.ServicePointManager]::SecurityProtocol

Worked on Windows PowerShell for me and
Would it slow things down too much? I figure in-memory comparisons are going to be far faster than disk or network operations. Added comments for us to refer to later.
EDIT: It's a bit slow, even when I try using native .NET methods :-(:

Measure-Command { 'Tls13' -in ([System.Net.SecurityProtocolType] | Get-Member -MemberType Property -Static).Name }
Measure-Command { 'Tls13' -in [System.Net.SecurityProtocolType].GetEnumNames() }

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 8
Ticks             : 88331
TotalDays         : 1.02234953703704E-07
TotalHours        : 2.45363888888889E-06
TotalMinutes      : 0.000147218333333333
TotalSeconds      : 0.0088331
TotalMilliseconds : 8.8331

Days              : 0
Hours             : 0
Minutes           : 0
Seconds           : 0
Milliseconds      : 4
Ticks             : 44498
TotalDays         : 5.15023148148148E-08
TotalHours        : 1.23605555555556E-06
TotalMinutes      : 7.41633333333333E-05
TotalSeconds      : 0.0044498
TotalMilliseconds : 4.4498

[variableresistor]

Again, I appreciate your time and effort on this change. I know it must be a lot of work maintaining this repository. So my team appreciates the work that's been done on this module.
This comparison from your code:

$PSVersionTable.PSVersion -ge [Version]'7.0.0'

Is checking to see if we're running on PowerShell 7 and if we are, to set the protocol. In mine, what I want to to only tinker with the protocol if the user is running on Windows PowerShell.
The justification being that .NET 3.1 and higher already supports TLS 1.3 https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-core-3-0#security (at least on Linux) and PowerShell 7 doesn't even listen to this setting Has there been a change in SSL/TLS backends with PowerShell 7 ?
To also be on the conservative side, we could also check to make sure they're not running on on-prem deployment of GitHub:

(Get-GitHubConfiguration -Name ApiHostName) -eq 'github.com'

I'm open to your thoughts on this.

[variableresistor]

It's tough because Microsoft actually recommends not fiddling with the protocol at all, as the case in our code, for when TLS 1.2 is inevitably exploited in the future Transport Layer Security (TLS) best practices with the .NET Framework

[HowardWolosky]

Ok, this is actually a bit more complicated than I had originally understood. I appreciate the additional context. I misread #443 and misunderstood your PR title, and had thought you were trying to have PS Core users take advantage of TLS 1.3 (I think the PR title and description could likely use a bit more flushing out for intent).

From a little reading here, it looks like .NET 4.7 is when it started to use the system default, but .NET 4.6.2 is the first time that TLS 1.3 got introduced (for Windows 11).

So, I believe the rough logic desired is as follows:

• Use System Default for PowerShell Core (7+)
• Use System Default for Windows PowerShell (< 7) if .NET version >= 4.7 else set to TLS 1.3 (if supported) else set to TLS 1.2.

Checking the .NET version appears to be complicated appears to require a regkey check.

So, mapping that logic to code:

# See https://learn.microsoft.com/en-us/dotnet/framework/migration-guide/how-to-determine-which-versions-are-installed#minimum-version
$installedDotNetVersion = (Get-ItemPropertyValue -LiteralPath 'HKLM:SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full' -Name Release)
$dotNet47 = 460798

$script:PreferredSecurityProtocolType = [Net.SecurityProtocolType]::Tls12
if (($PSVersionTable.PSVersion -ge [version]'7.0.0') -or ($installedDotNetVersion -ge $dotNet47))
{
  # .NET 4.7 (460798) is the first .NET version to respect the system default.
  $script:PreferredSecurityProtocolType = [Net.SecurityProtocolType]::SystemDefault
}
elseif (([System.Net.SecurityProtocolType] | Get-Member -MemberType Property -Static).Name -contains 'Tls13')
{
  $script:PreferredSecurityProtocolType = [Net.SecurityProtocolType]::Tls13
}

Given this increased logic, my suggestion:

1. Wrap that logic inside of a new function (something like Get-PreferredSecurityProtocolType) inside of Helpers.ps1 (but have it return back a value, not modify a static.
2. Add a new read-only script variable to the top of GitHubCore.ps1 (below ValidBodyContainingRequestMethods) and have it's value come by calling that new helper method
3. Update Invoke-GHRestMethod to now use that script variable instead.

Thoughts?

[variableresistor]

Sounds like a plan Howard! Are we concerned about *nix compatibility? If so, checking for the PowerShell version first would short-circuit the if condition before PowerShell tries to check the registry that doesn't exist in Linux:

$dotNet47 = 460798
$script:PreferredSecurityProtocolType = [Net.SecurityProtocolType]::Tls12

if (($PSVersionTable.PSVersion -ge [version]'7.0.0') -or ((Get-ItemPropertyValue -LiteralPath 'HKLM:SOFTWARE\Microsoft\NET Framework Setup\NDP\v4\Full' -Name Release) -ge $dotNet47))
{
  # .NET 4.7 (460798) is the first .NET version to respect the system default.
  $script:PreferredSecurityProtocolType = [Net.SecurityProtocolType]::SystemDefault
}
elseif (([System.Net.SecurityProtocolType] | Get-Member -MemberType Property -Static).Name -contains 'Tls13')
{
  $script:PreferredSecurityProtocolType = [Net.SecurityProtocolType]::Tls13
}

[HowardWolosky]

Good callout. Yes -- this module should be compatible across all PS Core supported platforms.
Your modified update generally looks good...to keep line length under 80 though, you'll likely need to move the ((Get-ItemPropertyValue ... to the subsequent line, indented. Please also retain the comment with the link to the MSDN documentation as well, and remember to update this to be a helper function that ultimately returns a value instead of directly setting the static...have the static set with the resulting value from the function call.