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.

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

Enable TLS 1.2 in core.ps1 #2074

Merged
merged 1 commit into from
Mar 2, 2018
Merged

Conversation

masaeedu
Copy link
Contributor

@masaeedu masaeedu commented Mar 1, 2018

Deal with https://github.com/blog/2507-weak-cryptographic-standards-removed by enabling TLS 1.2 in addition to existing encryption schemes.

Fixes #2040

Deal with https://github.com/blog/2507-weak-cryptographic-standards-removed by enabling TLS 1.2 in addition to existing encryption schemes.
@jordanbtucker
Copy link

This fix should be used in install.ps1 too. See #2040 (comment)

@masaeedu
Copy link
Contributor Author

masaeedu commented Mar 1, 2018

@jordanbtucker Are you referring to lib/install.ps1 or bin/install.ps1? If the latter (which is what get.scoop.sh resolves to), I'm assuming the fact that it downloads and evaluates core.ps1 should be sufficient. See this bit.

@jordanbtucker
Copy link

@masaeedu Oh, I see. One of the scripts refers to installing scoop, and the other refers to installing apps with scoop. Thanks.

@masaeedu
Copy link
Contributor Author

masaeedu commented Mar 1, 2018

Of course, that only works so long as the problem is limited to api.github.com and not raw.github.com (from which we're downloading core.ps1 in the first place), as you mentioned in your comment. Perhaps I should copy paste this into install.ps1 anyway, just to be safe.

@r15ch13 r15ch13 merged commit 039f28b into ScoopInstaller:master Mar 2, 2018
@jordanbtucker
Copy link

jordanbtucker commented Mar 2, 2018

I just noticed that this has the side effect of changing ServicePointManager.SecurityProtocol until you close your PowerShell session.

@masaeedu
Copy link
Contributor Author

masaeedu commented Mar 2, 2018

@jordanbtucker If it's working correctly, it should only be adding TLS 1.2 and leaving your existing protocols intact. Please let me know if it's not doing that.

@tresf
Copy link
Contributor

tresf commented Mar 2, 2018

I just noticed that this has the side effect of changing ServicePointManager.SecurityProtocol until you close your PowerShell session.

Good point. It would be less intrusive to cache it, set it, use it, reset it.

@jordanbtucker
Copy link

@masaeedu Yes, that is what it is doing. The default protocols are Ssl3 and Tls. This PR adds Tls12 to the list, however it doesn't remove Tls12 after scoop has finished running.

PS > [Net.ServicePointManager]::SecurityProtocol
Ssl3, Tls

PS > scoop update
Updating Scoop...
Scoop was updated successfully!

PS > [Net.ServicePointManager]::SecurityProtocol
Ssl3, Tls, Tls12

It's probably not that big of a deal, but I just wanted to document that it has side effects.

@masaeedu
Copy link
Contributor Author

masaeedu commented Mar 2, 2018

@tresf If we unset it, we need to figure out all the locations in the code where we access the relevant URLs and wrap them individually, which is error prone. The approach here is how the problem was solved in Powershell codebase.

@tresf
Copy link
Contributor

tresf commented Mar 2, 2018

@masaeedu yeah, I agree. 99.9% of the time, it's better to simply have it residual, that way the environment is consistent. Although to that point, one may prefer to simply go big or go home. Reason being is sporadically altering user space causes intermittent side-effects, versus predictable side-effects. Anyway, this nice, in no way am I trying to be overly critical. The fix paramount regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants