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

Statically link to Rustls instead of using OpenSSL #1214

Merged
merged 2 commits into from
Sep 18, 2022

Conversation

charlespierce
Copy link
Contributor

@charlespierce charlespierce commented Apr 22, 2022

Info

  • Implements the changes described in Switching to a Statically-Linked TLS implementation rfcs#47
  • As discussed in the RFC, while using a statically-linked TLS implementation reduces our need to build for the unique CentOS flavor of OpenSSL, we still need to use an older version of Linux to link against an older glibc version.
  • However, the build tools on CentOS 6 (the baseline for the glibc support that we've maintained) are too old to support building ring (a dependency of rustls).
  • To work around that, I discovered a docker image for Scientific Linux CERN 6, which is based off of RHEL 6 (the same as CentOS).
    • The CERN distro includes its own mirror of the package repositories (in fact, we are already using the cern mirrors for CentOS 6 in our CI right now)
    • It also includes the ability to install more recent build tools, which allows us to compile ring

Changes

  • Updated the dependency on attohttpc to enable the use of rustls instead of native-tls (OpenSSL on Linux)
  • Created a Dockerfile that builds off of Scientific Linux CERN 6, installing updated build tools
  • Updated the release CI workflow to build a single Linux version using the above Docker container.
  • Updated the install script to look for the new single version in the case of Linux

Tested

  • Confirmed locally using this Docker image that we are able to compile and run Volta
  • Verified that the single Linux build artifact from CI runs correctly works on old CentOS Linux, recent Ubuntu, and latest Ubuntu (3 different OpenSSL versions).

Notes

  • This PR changes the file name for the linux build artifact to be volta-{version}-linux.tar.gz, without the openssl-X suffix. This means we probably will need to roll it out in stages:
    • Pin the redirect for get.volta.sh to a commit hash, so that it isn't affected when this is merged. (Done in Pin get.volta.sh installer to current commit hash get#7) (Completed)
    • Merge this and prepare the release (along with any other features that we might want to include)
    • Test by fetching the new script directly to confirm that it works as expected.
    • Cut the release, then un-pin the get.volta.sh redirect so that it points at the updated script.
  • We'll also need to update the "Installing Old Versions" section of the Installers docs to include another special-cased version of the install script, for people who want to install versions prior to the install script change.

@alvarlagerlof
Copy link

Would this mean that you wouldn't need openssl to install volta?

@charlespierce
Copy link
Contributor Author

@alvarlagerlof Yes, exactly! That's actually one of the main motivations for this suggested change (see the linked RFC): Dealing with all of the different OpenSSL versions—trying to detect the appropriate one, fetch the correct binary, etc.—is a major pain point in our Linux installer. This would simplify all of that by statically linking to a Rust-based TLS/SSL implementation and avoiding all of the headaches that come with dynamically-linked OpenSSL.

@ZauberNerd
Copy link

@charlespierce I guess this PR would also allow #1049 to move forward, right?
I'm using volta on an EC2 with the Graviton2 ARM CPU and at the moment have to compile it myself, but would much prefer volta to release prebuilt binaries.

@charlespierce
Copy link
Contributor Author

@ZauberNerd Yes, I believe this would allow that change to move forward as well, since it would remove the OpenSSL issues that ran into. The shift to Scientific Linux CERN 6 may also help alleviate the toolchain issues.

@ZauberNerd
Copy link

@charlespierce that's awesome! Is there something I could do to help get this PR out of the draft state?

@charlespierce
Copy link
Contributor Author

@ZauberNerd I don't think there's anything left on this PR directly (other than resolving the merge conflicts). I'm leaving this in draft for now mostly to make sure that we don't accidentally merge it 😄 There's some infrastructure work we'll need to do before merging to make sure we don't break existing installs while we work towards the actual release of this change.

@chriskrycho
Copy link
Contributor

@charlespierce what (if anything) can I do to unblock this over this coming week? Would love to help get it landed!

@charlespierce
Copy link
Contributor Author

@chriskrycho I think the only thing remaining is landing this PR (and then some testing when we cut the release). My initial thoughts were being a little wary of version churn with the in-progress pnpm implementation, but I think there's enough significant changes between this and Yarn 3 support, plus there are some follow-ups needed for pnpm, so I'd lean towards getting this merged (along with any smaller outstanding PRs), and then cutting it all as version 1.1.

I'll update this to resolve the merge conflicts and also put out a PR to clean up some additional warnings we seem to be getting.

@chriskrycho
Copy link
Contributor

That seems reasonable to me! Let's do it!

Copy link
Contributor

@chriskrycho chriskrycho left a comment

Choose a reason for hiding this comment

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

Other than the one question, this looks good to me.

@charlespierce charlespierce merged commit a125fa8 into volta-cli:main Sep 18, 2022
@charlespierce charlespierce deleted the rustls_centos branch September 18, 2022 03:54
@charlespierce
Copy link
Contributor Author

itshappening.gif

@nifr
Copy link

nifr commented Sep 23, 2022

Wow this is huge! Thanks to everyone involved who helped to solve those openSSL linking issues with this PR.

Is it possible to get an "official" v1.1.0 release after the merge?

@chriskrycho
Copy link
Contributor

That's precisely the plan—this and Yarn 3 support will be the headliner features! (With, hopefully, a 1.2 with pnpm support landing relatively soon after that. 🤞🏼)

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.

6 participants