Skip to content

Use more restrictive options to make curl safer #136

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

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

Leonidas-from-XIV
Copy link
Contributor

Inspired by @smorimoto's #126 I think these options make sense to add in the script:

  • --tlsv1.2 will require TLS 1.2+ (but also allow 1.3) thus prevents it from using older, less secure TLS versions
  • --proto '=https' will prevent redirecting to HTTP. Since we download from GitHub and where GitHub redirects to is outside of our control I think there is a point in making sure it will fail if it redirects to a HTTP location.

Both our server and Github support TLSv1.2, the question is more whether our users have curl that support these options. My curl has them and the one in Alpine as well, so maybe this is fine without a feature check?

WDYT?

@Leonidas-from-XIV Leonidas-from-XIV changed the title Use more restrictive options to make curl safer Use more restrictive options to make curl safer Nov 15, 2024
@Leonidas-from-XIV Leonidas-from-XIV added the script Related to the script provided by the website label Nov 15, 2024
@smorimoto
Copy link
Contributor

Both features have been available in curl over a decade ago and should be fine.

Copy link
Contributor

@gridbugs gridbugs left a comment

Choose a reason for hiding this comment

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

Lgtm! These flags are also part of the install command on https://rustup.rs.

@Leonidas-from-XIV Leonidas-from-XIV merged commit ca6a04d into ocaml-dune:main Nov 18, 2024
2 checks passed
@Leonidas-from-XIV Leonidas-from-XIV deleted the safer-curl branch November 18, 2024 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
script Related to the script provided by the website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants