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

build: use rustls for TLS instead of relying on openssl #754

Closed
wants to merge 3 commits into from
Closed

build: use rustls for TLS instead of relying on openssl #754

wants to merge 3 commits into from

Conversation

joshuarli
Copy link
Contributor

@joshuarli joshuarli commented May 29, 2020

It'd be nice to ship more static binaries. Currently, release binaries for Linux are linked to various openssl versions, which can make it a little harder to add Volta to CI pipelines. Travis Xenial, for example, has openssl 1.0, so if for some reason someone wanted 1.1, they'd have to build the SO themselves and do some ld env var tweaking.

You can ignore this for now, I'm trying to see if I can update the lockfile to only see the added rustls and removed openssl-sys somehow without upgrading all transient deps (removing Cargo.lock and regenerating it).

I think in theory simply adding the tls-rustls feature and rebuilding would have updated the lockfile as expected, but it doesn't seem to be the case... perhaps the old tls feature is still there? Not sure right now, will look into it later.

Even if I regenerated the lockfile, it still tries to build openssl-sys.

@joshuarli
Copy link
Contributor Author

joshuarli commented May 29, 2020

Oh there we go, I had to set default-features to false.

So I'm not sure if this is possible, but if for some reason you'd like to continue to produce builds linked to openssl, I'm wondering if it can be put behind a Volta feature.

Rustls definitely simplifies the build - it doesn't require pkg-config and of course, openssl dev packages to be installed in the linux distribution. Apparently it's also faster.

@joshuarli joshuarli changed the title build: WIP use rustls for TLS instead of relying on openssl build: use rustls for TLS instead of relying on openssl May 29, 2020
@joshuarli
Copy link
Contributor Author

joshuarli commented May 29, 2020

Aw, I see some nontrivial build failures regarding building assembly in ring. FWIW I built this on my own linuces and it works.

@charlespierce If you like this PR, I can take a further look.

@charlespierce
Copy link
Contributor

Thanks for starting to look into this @joshuarli! OpenSSL compatibility was a big pain to figure out, so moving away from that is definitely interesting to prevent having to revisit that whole situation again. That said, when we've evaluated in the past, there are a couple of concerns that we have:

  1. Security. Despite it's many security issues, OpenSSL is well-tested and industry-standard. Rustls, while it comes with the guarantees of Rust's borrow checker, it still hasn't been vetted as heavily. There is an open issue to get a security audit (Get a security audit done rustls/rustls#189), and the latest comment is that may be happening Soon(TM), but until that is done there's a question mark around the security.

  2. Upgrades. By dynamically linking to OpenSSL, we provide users with the ability to get updates to OpenSSL (e.g. security fixes) without having to get a new version of Volta. If we start to statically link (whether that is using rustls or just statically linking against OpenSSL), then users would need to update Volta in order to get the advantages of those updates in the underlying library.

So for those reasons, we haven't yet committed to a statically linked approach, though I'd be more than happy to have an RFC / Issue discussion around the pros and cons.

@joshuarli
Copy link
Contributor Author

joshuarli commented May 29, 2020

Yup, I'm aware that rustls doesn't have the "in-the-wild" experience and industry backing as OpenSSL. Looking forward to that security audit, I think Rust in general will benefit greatly with a solid TLS alternative that can be statically included.

Let's let this PR stew for a while, I'm unsure if there are many other Volta users right now who care about this.

And haha yeah, I can see the pain in your CI configs.

@joshuarli
Copy link
Contributor Author

@charlespierce Looks like the audit was just completed, refer to the issue you linked for updates. Results seem very positive. That said, it seems to be mostly a 1-person project, so I would hold out on it a little longer to see how healthy maintainership is.

Or, I'm happy to take a stab at providing CI configurations to optionally build, test and release rustls versions behind a volta feature flag.

@charlespierce charlespierce changed the base branch from master to main June 15, 2020 18:04
@charlespierce
Copy link
Contributor

@joshuarli I think a Rust feature flag makes sense. I'm not sure of how we would deploy it as a built, but since (I believe) switching mainly involves changing a feature flag in the attohttpc dependency, having our own feature flag would be a clean approach. That way, if people want / need to have a RusTLS / statically-linked build, they can create one fairly easily themselves.

A step beyond that, if you are interested, would be to build a rustls-backed version in the Production CI branch and include it in releases as an additional asset. It wouldn't be used by the default installers, but having the binaries zipped up would be an even easier case for people who want a static build. Then at some point we can figure out how to incorporate it into the installers.

@joshuarli
Copy link
Contributor Author

I'm not sure of how we would deploy it as a built

I think you answered that yourself in the 2nd paragraph? Build + test + release in CI.

Sounds like this is welcome, I'll toss a PR your way sometime soon.

@charlespierce
Copy link
Contributor

FYI: I changed the default branch for this repo from master to main. This PR has already been updated to point at main, however if you need to rebase for any reason, you will want to make sure to rebase against the main branch.

@joshuarli
Copy link
Contributor Author

Looking at this more @charlespierce, my current knowledge of Cargo manifests falls short on how to implement this. I think this issue (rust-lang/cargo#5954) accurately describes the blocker.

@charlespierce
Copy link
Contributor

@joshuarli In this case, since we already depend on attohttpc, I don't think we need custom dependencies per feature. However, it is complicated by the fact that attohttpc uses OpenSSL in its default features. So to make everything work, I think we'll need a few extra steps:

  1. Create two features (maybe native and rustls) in Volta's root Cargo.toml to represent the two cases.
  2. Make native a default feature in the root Cargo.toml
  3. Create the same features in every sub-crate that actually depends on attohttpc (which at the moment I believe is volta-core and archive, but you'd want to double-check that.
  4. Make the root features activate the same-named feature in each sub-crate
  5. In each sub-crate, import attohttpc with default-features = false and features = ["compress"].
  6. In each sub-crate, have the native feature activate attohttpc/tls and the rustls feature activate attohttpc/tls-rustls.

If that all works the way I think it should, then running cargo build will use the default features (the current OpenSSL implementation), while running cargo build --no-default-features --features rustls would build with RusTLS instead.

@joshuarli
Copy link
Contributor Author

Closing; see #809.

@joshuarli joshuarli closed this Sep 12, 2020
@joshuarli joshuarli deleted the build/use-rustls branch September 12, 2020 20:11
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