Skip to content

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

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

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

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f3be482
Added TLS 1.3 support for PS Core
variableresistor Jan 8, 2025
a53d6c1
Fixed PowerShell version comparison
variableresistor Jan 8, 2025
6f1b15c
Changed double quotes to single-quotes
variableresistor Jan 8, 2025
e8356b1
Added proper spacing
variableresistor Jan 8, 2025
f42326d
Implemented TLS 1.3 change. Fixed tests.
variableresistor Jan 22, 2025
3f71e21
Made repo variable retrieval call consistent
variableresistor Jan 22, 2025
32e93d9
Fixed variable missing error in test
variableresistor Jan 22, 2025
0ef46ba
Fixed more strict mode variable errors
variableresistor Jan 22, 2025
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
10 changes: 8 additions & 2 deletions GitHubCore.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,10 @@ function Invoke-GHRestMethod
# Disable Progress Bar in function scope during Invoke-WebRequest
$ProgressPreference = 'SilentlyContinue'

[Net.ServicePointManager]::SecurityProtocol=[Net.SecurityProtocolType]::Tls12
if ($PSVersionTable.PSVersion -lt [version]"7.0.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@variableresistor variableresistor Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@variableresistor variableresistor Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

{
[Net.ServicePointManager]::SecurityProtocol=[Net.SecurityProtocolType]::Tls12
}

$result = Invoke-WebRequest @params

Expand Down Expand Up @@ -582,7 +585,10 @@ function Invoke-GHRestMethod
}
finally
{
[Net.ServicePointManager]::SecurityProtocol = $originalSecurityProtocol
if ($PSVersionTable.PSVersion -lt [version]"7.0.0")
{
[Net.ServicePointManager]::SecurityProtocol = $originalSecurityProtocol
}
}
}

Expand Down