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

Upgrade openssl dependency #907

Closed
arcrose opened this issue Aug 31, 2016 · 22 comments
Closed

Upgrade openssl dependency #907

arcrose opened this issue Aug 31, 2016 · 22 comments

Comments

@arcrose
Copy link

arcrose commented Aug 31, 2016

Currently, Hyper's Cargo.toml specifies using Rust-OpenSSL version 0.7.x. At the time of this writing, the latest release is version 0.8.2, which includes a number of bug fixes that, in some applications has resolved some functional failings.

I'd like to ask for Hyper to adopt these changes and upgrade to the latest version.

@kwaegel
Copy link

kwaegel commented Oct 10, 2016

Upgrading may also fix a build issue on Widows, as I mentioned in the linked bug for rust-rosetta: rust-rosetta/rust-rosetta#566 .

Unfortunately, cookie v0.3.0 also depends on rust-openssl v0.7, so fixing the build issue would require an upgrade in that crate as well. Such a change is already in master, but isn't part of a release yet.

@Thomasdezeeuw
Copy link
Contributor

cookie v0.3.1 now depends on v0.8. Is their anything else that is blocking an update to v0.8?

euclio added a commit to euclio/hyper that referenced this issue Oct 21, 2016
euclio added a commit to euclio/hyper that referenced this issue Oct 21, 2016
This fixes a potential bug in rust-openssl where creating a connection could
segfault.

Closes hyperium#907.
@seanmonstar
Copy link
Member

Bumping to openssl 0.8 is a breaking change, so that'd require bumping hyper as well. openssl is also addinv support for opensslv1.1, which will likely bump it to 0.9. I've been hoping hyper 0.10 would be async hyper.

My plan has been this:

  • hyper v0.10: Be "async" hyper. Utilize tokio-tls.
  • reqwest: Be the convenient, higher level Client crate. This will release very soon, and depend on rust-native-tls. Everyone using hyper for a client should be able to easily switch the reqwest, getting better TLS support immediately, and a (as much as possible) not-breaking API, even as hyper v0.10 comes out with its async Client.

@Thomasdezeeuw
Copy link
Contributor

@seanmonstar that sounds great, looking forward to it. I would offer help, but I started learning Rust last week and I'm not sure I would be any help.

@arcrose
Copy link
Author

arcrose commented Oct 22, 2016

Is there anything I could do to help speed this process along? My project at work has actually had to drop a couple of features because of the OpenSSL version mismatch between a few of our dependencies, and I'd like to be able to help resolve that if possible.

@seanmonstar
Copy link
Member

@zsck which do you need? I'm assuming that the majority of people who need HTTPS are using a Client (since so many terminate TLS with nginx for a Server). In that case, there are issues filed on the reqwest repo. It's also largely dependent on rust-native-tls being published to crates.io, instead of being a git dependency.

@arcrose
Copy link
Author

arcrose commented Oct 23, 2016

In my case, I need the library to just be using OpenSSL 0.8. The problem is that Hyper's requirement for 0.7 conflicts with some of my other dependencies which now use 0.8. Cargo picks up on the fact that I'm using the Hyper Client and resolves the dependencies to using 0.7 everywhere, which breaks another one of my dependencies which needs 0.8.

@seanmonstar
Copy link
Member

@zsck I understand. As i mentioned, openssl will be getting another breaking change (likely to 0.9) very very shortly (see sfackler/rust-openssl#466). When it does, native-tls will be published, and I can publish reqwest.

@steveklabnik
Copy link

steveklabnik commented Nov 12, 2016

update: 0.9 was published six days ago.

@eddyb
Copy link

eddyb commented Nov 13, 2016

As a side note, it's a shame that https://crates.io/crates/request takes the name even though it looks like development stopped before Rust 1.0, and @ghmlee seemed to have dropped off GitHub 😞.

@seanmonstar
Copy link
Member

@eddyb oh i know. I've tried to contact him several different ways for a couple months, and was also reminded about the crates.io policy of not taking a name away from someone, so I've settled on this weird name (requests with an 's' also exists).

@dereckson
Copy link

dereckson commented Nov 22, 2016

This should be prioritized: Hyper crates won't compile out of the box on Debian Sid or Fedora Rawhide, they shipped OpenSSL 1.1 this week, a version only supported in the openssl crate at 0.9.x.

I've given more context on https://users.rust-lang.org/t/openssl-1-1-and-openssl-0-9-api-breaking-changes/8112

@seanmonstar
Copy link
Member

Are there still situations that cannot be taken care with either:

  • use reqwest
  • if on the server, set default-features = false for hyper, which removes the openssl dependency

@Arnavion
Copy link
Contributor

I personally am waiting for (TODO: Customizable redirect policy) to be fixed in reqwest before I can switch my crates to it :)

@seanmonstar
Copy link
Member

@Arnavion mind adding exactly what you'd need to the issue? seanmonstar/reqwest#24

@arcrose
Copy link
Author

arcrose commented Nov 24, 2016

Are there still situations that cannot be taken care with either:

  • use reqwest
  • if on the server, set default-features = false for hyper, which removes the openssl dependency

With regards to switching to reqwest, there's the cost of time to actually become familiar with reqwest and then change all of the code in users of Hyper's codebases. As someone running Rust in production, it's not always easy to come up with time to make all the appropriate changes and sufficiently test that no regressions occurred when there are features and other pressing things to look after.

@seanmonstar Look, hyper is a really popular library. I understand that you may feel there are better alternatives to using the Hyper client today, but asking all of Hyper's users to update their code to an entirely new library is really unfair as one of Hyper's core developers- the amount of time it would take for you to update Hyper and for users to simply switch to the new version totals to far less than what you're suggesting would impose on developers.

@ajdlinux
Copy link

This is blocking me from upgrading other libraries (git2, to be precise). I'm happy to port over to using reqwest, as our requirements are very much appropriate for a higher-level library like that, but as soon as I started looking into it, I hit seanmonstar/reqwest#30.

Until reqwest stabilises and implements at least a few of the more important feature requests that have already been reported, migrating to reqwest isn't going to be feasible just yet. In the meantime an updated hyper would be really helpful!

@ahwatts
Copy link

ahwatts commented Dec 16, 2016

After hunting through the build scripts for the openssl* crates, I found a combination of environment variables which at least got hyper to build on my Mac:

export OPENSSL_INCLUDE_DIR=/usr/local/opt/openssl/include OPENSSL_LIB_DIR=/usr/local/opt/openssl/lib DEP_OPENSSL_INCLUDE=/usr/local/opt/openssl/include

I hope that helps someone.

@SergioBenitez
Copy link

I've submitted #975 to fix this for 0.9.x. Note that this is a breaking, albeit minor, change. It's up to @seanmonstar to accept it now.

@compressed
Copy link

From what I can see, using reqwest does not resolve this issue because reqwest still depends on cookie version 0.2 via its hyper dependency, which brings along openssl 0.7.4.

@shssoichiro
Copy link

@compressed reqwest compiles because it sets default-features = false for hyper. However, this is a conflict if you depend on reqwest and any other crate that depends on hyper with its default feature set (which includes cookie ^0.2). See iron/iron-test#37 as an example.

Interestingly, OS X will successfully compile a crate despite the above version conflict, but Linux will fail the build.

@seanmonstar
Copy link
Member

The full issue was discussed in #985, and the solution there was just released as https://github.com/hyperium/hyper/releases/tag/v0.10.0, so closing.

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 a pull request may close this issue.