Skip to content
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

Added TLS 1.3 support for PS Core #444

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

Added TLS 1.3 support for PS Core #444

Changes from all commits
Commits
Show all changes
4 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
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
Member

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
Member

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 Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

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
Member

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.

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