Added TLS 1.3 support and fixed strict-mode variable errors in tests

[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

[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.

[variableresistor]

Verified that this works in regular Windows PowerShell by running:

# This is returned from Get-PreferredSecurityProtocolType
[Net.ServicePointManager]::SecurityProtocol

Invoke-RestMethod https://github.com
Get-PreferredSecurityProtocolType
# Returns SystemDefault

No error.

I fixed an error about a missing repo variable that is due to strict mode being set as well. There were a few errors other I got, but were unrelated to my change:

[-] Should have the expected type and additional properties 1.29s (1.28s|10ms)
  Expected strings to be the same, but they were different.
  Expected length: 17
  Actual length:   52
  Strings differ at index 0.
  Expected: 'GitHub.GistCommit'
  But was:  'Selected.System.Management.Automation.PSCustomObject'
               ^
  at $commit.PSObject.TypeNames[0] | Should -Be 'GitHub.GistCommit', 
C:\Repos\PowerShellForGitHub\Tests\GitHubGists.tests.ps1:163
  at <ScriptBlock>, C:\Repos\PowerShellForGitHub\Tests\GitHubGists.tests.ps1:163

[-] Should have fewer results with using the since parameter 470ms (468ms|2ms)
  Expected the actual value to be greater than 0, but got $null.
  at $sinceGists.Count | Should -BeGreaterThan 0, C:\Repos\PowerShellForGitHub\Tests\GitHubGists.tests.ps1:254
  at <ScriptBlock>, C:\Repos\PowerShellForGitHub\Tests\GitHubGists.tests.ps1:254

  [-] Should throw the correct exception 580ms (575ms|5ms)
   Expected like wildcard '*The remote server returned an error: (401) Unauthorized*' to match 'The remote server returned an error: (403) Forbidden.
   API rate limit exceeded for 172.117.119.161. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.) | https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting
   RequestId: C0D8:2809CF:B467BE:B96E51:67912352', but it did not match.
   at $Error[0].Exception.Message | Should -BeLike $exceptionMessage1, C:\Repos\PowerShellForGitHub\Tests\GitHubGraphQl.Tests.ps1:156
   at <ScriptBlock>, C:\Repos\PowerShellForGitHub\Tests\GitHubGraphQl.Tests.ps1:156

[-] Should throw the correct exception 684ms (680ms|4ms)
  Expected like wildcard '*Parse error on "InvalidQuery"*' to match 'GraphQl Error: Expected one of SCHEMA, SCALAR, TYPE, ENUM, INPUT, UNION, INTERFACE, actual: IDENTIFIER ("InvalidQuery") at [1, 1]
  RequestId: C20C:3D330C:B18DD3:B6944B:67912353', but it did not match.
  at $Error[0].Exception.Message | Should -BeLike "*Parse error on ""$invalidQuery""*", C:\Repos\PowerShellForGitHub\Tests\GitHubGraphQl.Tests.ps1:201
  at <ScriptBlock>, C:\Repos\PowerShellForGitHub\Tests\GitHubGraphQl.Tests.ps1:201

[variableresistor]

Also on the topic of Helpers.ps1, can I just get rid of the Ensure-Directory function? It's not referenced anywhere and doesn't use an approved verb.