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

Jump to bottom

Switch to use only the recommended TLS versions without fallback to earlier insecure options #2902

Closed
TheCakeIsNaOH opened this issue Nov 12, 2022 · 2 comments · Fixed by #3123
Closed

Switch to use only the recommended TLS versions without fallback to earlier insecure options #2902

TheCakeIsNaOH opened this issue Nov 12, 2022 · 2 comments · Fixed by #3123
Assignees
@vexx32
Labels
5 - Released Breaking Change
Milestone
2.0.0

Comments

@TheCakeIsNaOH
Copy link
Member

Is Your Feature Request Related To A Problem? Please describe.

Now that Chocolatey CLI is moving to .Net 4.8, there is a redundant fallback and warning message in SecurityProtocol. Since .Net 4.8 always has TLS v1.2 available, there is no need to continue to have this fallback and warning.

choco/src/chocolatey/infrastructure/registration/SecurityProtocol.cs

Lines 43 to 56 in edde979

ServicePointManager.SecurityProtocol = SecurityProtocolType.Tls | SecurityProtocolType.Ssl3;
//todo: #2585 provide this warning with the ability to opt out of seeing it again so we can move it up to more prominent visibility and not just the verbose log
if (provideWarning)
{
"chocolatey".Log().Warn(ChocolateyLoggers.Verbose,
@" !!WARNING!!
Choco prefers to use TLS v1.2 if it is available, but this client is
running on .NET 4.0, which uses an older SSL. It's using TLS 1.0 or
earlier, which makes it susceptible to BEAST and also doesn't
implement the 1/n-1 record splitting mitigation for Cipher-Block
Chaining. Upgrade to at least .NET 4.5 at your earliest convenience.
For more information you should visit https://www.howsmyssl.com/");
}

Describe The Solution. Why is it needed?

At minimum, this specific warning/fallback should be removed.

But it would be better to have a re-evaluation of the status of SSL/TLS in Chocolatey CLI, and best practices for .Net 4.8 programs.

This document recommends that the TLS version should not be explicitly set by programs, and use the OS defaults.
https://learn.microsoft.com/en-us/dotnet/framework/network-programming/tls

However, from what I understand, this may mean that on Windows 7, depending on updates/registry settings then .Net apps may not have TLS 1.2 enabled by default. As per:
https://learn.microsoft.com/en-us/windows/win32/secauthn/protocols-in-tls-ssl--schannel-ssp-#tls-protocol-version-support

So it may make sense to enable TLS 1.1/1.2 if the Windows build/version corresponds to Windows 7/Server 2008 R2, and use the OS default if the OS build is newer?

Additional Context.

N/A

Related Issues
@TheCakeIsNaOH TheCakeIsNaOH mentioned this issue Nov 18, 2022
Closed
@TheCakeIsNaOH TheCakeIsNaOH mentioned this issue Jan 10, 2023
Closed
8 tasks
@TheCakeIsNaOH TheCakeIsNaOH added this to the 2.0.0 milestone Jan 24, 2023
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Mar 21, 2023
@TheCakeIsNaOH
(chocolatey#2902)(chocolatey#2585) Update TLS protocol handling
b1277ce 
This updates the TLS protocols that Chocolatey sets now that Chocolatey
is built with .NET 4.8.

First, this removes the fallback and warning about being unable to set
TLS 1.2, as .NET 4.8 will always have TLS 1.2 available. Second, it
uses the TLS 1.2 definition directly available in the system libraries.
Third, it removes the provideWarning bool in the method definition, as
the warning is no longer applicable.

Finally, a OS version check is added, so the system default will be
used on Windows 8/Server 2012 and newer, as per Microsoft guidance,
but explicitly enabling TLS 1.2 on older operating systems. See code
comments for links to documentation.
@TheCakeIsNaOH TheCakeIsNaOH mentioned this issue Mar 21, 2023
Closed
10 tasks
TheCakeIsNaOH added a commit to TheCakeIsNaOH/choco that referenced this issue Apr 6, 2023
@TheCakeIsNaOH
(chocolatey#2902)(chocolatey#2585) Update TLS protocol handling
3358bf0 
This updates the TLS protocols that Chocolatey sets now that Chocolatey
is built with .NET 4.8.

First, this removes the fallback and warning about being unable to set
TLS 1.2, as .NET 4.8 will always have TLS 1.2 available. Second, it
uses the TLS 1.2 definition directly available in the system libraries.
Third, it removes the provideWarning bool in the method definition, as
the warning is no longer applicable.

Finally, a OS version check is added, so the system default will be
used on Windows 8/Server 2012 and newer, as per Microsoft guidance,
but explicitly enabling TLS 1.2 on older operating systems. See code
comments for links to documentation.
vexx32 added a commit to vexx32/choco that referenced this issue Apr 18, 2023
@vexx32
(chocolatey#2902) ReplaceTLS/HTTPS handling code
29b7b85 
After speaking with the team and reviewing .NET's best practices
documentation on this, this is the solution we've come to. Being overly
specific about TLS versions has its downsides so it's better to leave it
to the OS.

I also replaced the class that handles it with one that has a more
descriptive name and properly made it a static class.
vexx32 added a commit to vexx32/choco that referenced this issue Apr 18, 2023
@vexx32
(chocolatey#2902) ReplaceTLS/HTTPS handling code
c09b27c 
After speaking with the team and reviewing .NET's best practices
documentation on this, this is the solution we've come to. Being overly
specific about TLS versions has its downsides so it's better to leave it
to the OS.

I also replaced the class that handles it with one that has a more
descriptive name and properly made it a static class.
@vexx32 vexx32 mentioned this issue Apr 18, 2023
Merged
10 tasks
vexx32 added a commit to vexx32/choco that referenced this issue Apr 18, 2023
@vexx32
(chocolatey#2902) ReplaceTLS/HTTPS handling code
17bf150 
After speaking with the team and reviewing .NET's best practices
documentation on this, this is the solution we've come to. Being overly
specific about TLS versions has its downsides so it's better to leave it
to the OS.

I also replaced the class that handles it with one that has a more
descriptive name and properly made it a static class.
@vexx32 vexx32 self-assigned this Apr 18, 2023
@vexx32
Copy link
Member

vexx32 commented Apr 18, 2023 •
edited
Loading

Tagging this as a potentially breaking change. It shouldn't break a lot of folks, but it could break folks in some older OSes, and require them to make a few changes to their OS configuration (example) in order to work with the Community Repository for example, so it's worth calling out.

@vexx32 vexx32 changed the title Update or remove setting of enabled security protocols Update handling of HTTPS security protocols Apr 18, 2023
gep13 added a commit that referenced this issue Apr 19, 2023
@gep13
Merge pull request #3123 from vexx32/tls-security-protocols
b18c480 
(#2902) Use .NET recommended practices for TLS protocol handling
@gep13 gep13 closed this as completed Apr 19, 2023
vexx32 added a commit to vexx32/choco that referenced this issue Apr 20, 2023
@vexx32
(chocolatey#2902) Make HttpsSecurity class and method public
e5a8e6a 
This needs to be used in CLE and Agent, so should be public.
@vexx32 vexx32 mentioned this issue Apr 20, 2023
Merged
10 tasks
gep13 added a commit that referenced this issue Apr 20, 2023
@gep13
Merge pull request #3127 from vexx32/2902-tls-public-frown
5cbf023 
(#2902) Make HttpsSecurity class public
@gep13 gep13 changed the title Update handling of HTTPS security protocols Switch to use only the recommended TLS versions without fallback to earlier insecure options Apr 25, 2023
vexx32 added a commit to vexx32/choco that referenced this issue May 11, 2023
@vexx32
(chocolatey#2902) Ensure PowerShell tasks use SystemDefault TLS
5bf790a 
In chocolatey#3123 we changed our default TLS handling to defer to SystemDefault.
Subsequently, we have discovered that some versions of Windows
PowerShell are explicitly setting the TLS settings away from the
SystemDefault. As covered in the previous PR, this is a potential
security hazard, and also complicates matters for users in terms of
being able to communicate with servers that need TLS 1.3 for example.

Rather than tolerate the default from Windows PowerShell, we can inject
our own default setting here for our PowerShell host similar to how we
clear out culture settings before running PowerShell tasks.
@vexx32 vexx32 mentioned this issue May 11, 2023
Merged
10 tasks
@gep13 gep13 mentioned this issue May 18, 2023
Closed
7 tasks
corbob added a commit that referenced this issue May 19, 2023
@corbob
Merge branch 'release/2.0.0'
89c8c9c 
* release/2.0.0: (422 commits)
  (maint) Update Chocolatey.NuGet.Client version
  (build) Update GRM Template to add full stop.
  (build) Update GRM config for BuildAutomation tag
  (#508) Ensure correct configuration object is used
  (tests) Use nuspec for version normalization tests
  (tests) Tag version normalization Pester tests
  (#158) Restore --source in tab completion for list
  (#158) Refactoring based on pairing session
  (#158) Restore --source for choco list
  (maint) Add vscode settings file
  (maint) Changes during pairing session
  (tests) Add non-normalized version Pester tests
  (maint) Update testing vagrant for granular runs
  (maint) Update Pester test for Pester v5 norms
  (#1174) Add non-normalized packages for tests
  (#508) Fix v3 feeds mixed with v2 feeds not finding packages
  (#508) Do case insensitive comparison on package ids
  (maint) Remove prerelease warnings from the nuspec
  (maint/doc) Amend GenerateDocs for list/search split
  (#2902) Ensure PowerShell tasks use SystemDefault TLS
  ...
@choco-bot
Copy link

🎉 This issue has been resolved in version 2.0.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

@gep13 gep13 mentioned this issue Jul 7, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vexx32 vexx32

Labels
5 - Released Breaking Change
Projects
None yet
Milestone
2.0.0
Development

Successfully merging a pull request may close this issue.

(#2902) Use .NET recommended practices for TLS protocol handling vexx32/choco
5 participants
@gep13 @choco-bot @pauby @vexx32 @TheCakeIsNaOH