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

(#2902) Use .NET recommended practices for TLS protocol handling #3123

Merged
merged 1 commit into from
Apr 19, 2023
Merged

(#2902) Use .NET recommended practices for TLS protocol handling #3123

merged 1 commit into from
Apr 19, 2023

Conversation

vexx32
Copy link
Member

@vexx32 vexx32 commented Apr 18, 2023 •
edited
Loading

Description Of Changes

  • Use SystemDefault protocol setting
  • Moved the SetProtocol method to a more aptly-name static class
  • Deprecated the old SecurityProtocol class
  • Updated existing method calls
  • Lots of comments that will Hopefully(tm) remain relevant in future or at least be legible enough to inform future decisions

Motivation and Context

Looking at the .NET recommendations and best practices for TLS, it seems to be the case that we really shouldn't be explicitly enabling or disabling TLS protocols. I'll enumerate the reasons here, they are also in code comments:

  1. Most supported operating systems default to / require TLS 1.2 anyway.
  2. Explicitly enabling any less secure protocols leaves us open to TLS downgrade attacks
  3. Always requiring TLS 1.2 or higher on any OS version may cause issues with some folks' internal networks which may have older infrastructure that doesn't support it, and they won't have a way to make Choco work with that infrastructure if we force a higher TLS version than they support.
  4. Thus, the only sensible solution (since we don't want to add a configuration value for this) is to take the OS-level setting and use that, because if folks set their system up to use certain TLS versions, we should probably follow suit, regardless of what their OS version may be.

Testing

  1. Ensure you can still choco install things from the CCR
  2. Beyond that, I'm not really sure how much network-layer testing we have the capability to do, or how meaningful it would be?

Operating Systems Testing

Win11

Change Types Made

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • PowerShell code changes.

Change Checklist

  • Requires a change to the documentation.
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v2 compatibility checked?

Related Issue

Fixes #2902

@vexx32 vexx32 requested review from gep13 and AdmiringWorm April 18, 2023 13:50
@vexx32 vexx32 changed the base branch from develop to release/2.0.0 April 18, 2023 13:51
@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 force-pushed the tls-security-protocols branch from c09b27c to 17bf150 Compare April 18, 2023 13:54
@vexx32 vexx32 mentioned this pull request Apr 18, 2023
Closed
10 tasks
gep13
gep13 approved these changes Apr 19, 2023
View reviewed changes
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gep13 gep13 merged commit b18c480 into chocolatey:release/2.0.0 Apr 19, 2023
@gep13
Copy link
Member

gep13 commented Apr 19, 2023

@vexx32 thanks for getting this fixed up, and thanks to @TheCakeIsNaOH for getting us started with this change!

@vexx32 vexx32 deleted the tls-security-protocols branch April 19, 2023 12:50
vexx32 added a commit to vexx32/choco that referenced this pull request 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 pull request May 11, 2023
Merged
10 tasks
@gep13 gep13 mentioned this pull request May 18, 2023
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheCakeIsNaOH TheCakeIsNaOH TheCakeIsNaOH left review comments

@gep13 gep13 gep13 approved these changes

@AdmiringWorm AdmiringWorm Awaiting requested review from AdmiringWorm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Switch to use only the recommended TLS versions without fallback to earlier insecure options
3 participants
@vexx32 @gep13 @TheCakeIsNaOH