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

rustup-init.sh: Add tests for old OSes #1804

Closed
wants to merge 2 commits into from

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Apr 24, 2019

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

At minimum the fix for OSX < 10.13 needs restoring

if check_cmd sw_vers; then
if [ "$(sw_vers -productVersion | cut -d. -f2)" -lt 13 ]; then
# Older than 10.13
echo "Warning: Detected OS X platform older than 10.13"
Copy link
Contributor

Choose a reason for hiding this comment

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

You appear to have removed this test and not put in the equivalent anywhere else. This test is necessary because 10.12's curl claims support for the arguments, but fails in a way which is indistinguishable from a MITM attack if they are used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand. I'd add new commits to test those macOS images listed
in https://docs.travis-ci.com/user/reference/osx/#macos-version.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is also meant to cope for users, so do not do anything travis-specific.

_ok="n"
check_curl_options() {
local flags tls_protocol
for tls in '--tlsv1.2' '--tlsv1.1' '--tlsv1.0' '--tlsv1'; do
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not want to do this. We only want tlsv1.2 if we're specifying a protocol at all. At least that's my understanding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think fallback is better than nothing at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather let the server-side negotiation protect us at that point.

if [ -z "$tls_protocol" ]; then
warn 'cannot use secure protocol to download'
elif [ "$tls_protocol" != "--tlsv1.2" ]; then
warn "fallback to TLS${tls_protocol#--tls}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure there's a need for this fallback, similar reasoning to above.

check_wget_options() {
local flags tls_protocol
if ! (wget --secure-protocol 2>&1 > /dev/null | grep -q unrecognized); then
for tls in TLSv1_2 TLSv1_1 TLSv1; do
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar issue to above.

@bors
Copy link
Contributor

bors commented Apr 27, 2019

☔ The latest upstream changes (presumably #1809) made this pull request unmergeable. Please resolve the merge conflicts.

@tesuji
Copy link
Contributor Author

tesuji commented May 15, 2019

I will close this PR then.

@tesuji tesuji closed this May 15, 2019
@tesuji tesuji deleted the tls-fallback branch May 15, 2019 16:21
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.

3 participants