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

Add a fallback to wget if curl is not available #1913

Merged
merged 3 commits into from
May 21, 2024
Merged

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Feb 20, 2024

This allows using the install script on Alpine containers and other busybox-based systems with no additional dependencies.

The script attempts to use --https-only --secure-protocol=TLSv1_3 --no-verbose if available, otherwise falls back to a portable wget call.

Additionally, error on invalid arguments (currently syntax errors silently continue).

This is based on top of #1912, which should be merged first. The main change for this PR is in the second commit.

Tested on ubuntu (wget is GNU and sh is Dash, no pipefail) and alpine (wget and sh are both alpine, yes pipefail)

@tgross35 tgross35 force-pushed the add-wget branch 3 times, most recently from 159400d to 6be3097 Compare February 20, 2024 02:57
Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me! There are conflicts because along with your shellcheck PR, I also added a check on CI which runs shellcheck, but they should to be easy to merge.

www/install.sh Outdated
if command -v curl > /dev/null; then
curl --proto =https --tlsv1.2 -sSfL "$url" "-o$output"
else
# Some wget implementations support these flags, but busybox
Copy link
Owner

Choose a reason for hiding this comment

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

How about trying wget with all options and then falling back to using none of the options if that fails? We might try the download twice, if there's some other error, like the network not being up, but that might be better than checking each flag individually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds reasonable, I updated to do this and tested with both ubuntu and alpine. I had to suppress stderr from the first run so it doesn't print the help message on Alpine, but that should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually... maybe we don't want to just try twice? With the change, if HTTPS fails or TLS is outdated then it will just rerun without the flags :)

I don't think the flags actually add much since the URL is https, your call for what to do

@tgross35 tgross35 force-pushed the add-wget branch 4 times, most recently from 76d7388 to 8d8c32e Compare February 20, 2024 06:03
This allows using the install script on Alpine containers and other
busybox-based systems with no additional dependencies.

The script attempts to use `--https-only --secure-protocol=TLSv1_3
--no-verbose` if available, otherwise falls back to a portable wget call.

Additionally, error on invalid arguments.
@casey
Copy link
Owner

casey commented May 15, 2024

Is it just old versions of wget which don't support those flags? I think perhaps we should always use those flags, since I'm a little bit wary of not using them just because they aren't supported.

Copy link
Owner

@casey casey left a comment

Choose a reason for hiding this comment

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

I removed the fallback to insecure wget. This fallback would have triggered if there was a cert error, so a MITM could potentially have intentionally downgraded the connection to insecure wget.

If it's necessary to support boxes that don't have a secure curl or wget, we should add a --insecure flag download with forcing TLS.

@casey casey enabled auto-merge (squash) May 21, 2024 00:19
@casey casey merged commit 77f343e into casey:master May 21, 2024
5 checks passed
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.

2 participants