Remove SSL feature (and openssl/security-framework dependencies) #985

Closed
seanmonstar opened this Issue Jan 5, 2017 · 11 comments

Comments

Projects
None yet
6 participants
@seanmonstar
Member

seanmonstar commented Jan 5, 2017

The Problem

The current problem is that the openssl crate has released versions 0.9.x, and hyper is depending on 0.7.x. That by itself isn't an issue, but since the openssl crate has openssl-sys as a direct dependency, the old openssl version does become a problem for many people. Cargo only allows one version to exist in the whole dependency graph for any -sys crate. So the real pain comes from when people want to use other libraries that depend on a newer version of openssl, and also depend on hyper. They are then left in a situation where their dependency graph wants both openssl-sys at version 0.9.5 and 0.7.14 (versions as of this writing), and that is illegal.

Attempts to Fix the Things

The suggestion so far has been that people could turn off the ssl feature of hyper, thus removing the openssl optional dependency. If people needed TLS, the suggestion has been to use the better client library, reqwest, which does this exact process: disables hyper/ssl, and provides a client using rust-native-tls. hyper has always been generic over TLS implementations, thanks to SslClient and SslServer traits. Sounds simple, right?

Well, it gets worse. Say you do exactly that: you replace your client usage with reqwest. But say you also have a server component to your app, using some other framework, such as iron, or nickel, or something newer. Those frameworks likely depend on hyper, and did not disable the ssl feature. So now, your dependency graph wants hyper both with and without openssl. And since reqwest (or whichever other library) also depends on openssl, but at 0.9.x, we have the conflict again. And the problem with this conflict, is that it is out of the users' control to fix it, because you cannot alter features of dependencies of dependencies.

Well then, why not just upgrade openssl! Well, there are different problems with doing that. Upgrading openssl will be a breaking change, so the minor version number must increase (while hyper is 0.x, if it were 1.x, then we'd need to go up to 2.0!). Sometimes that is needed, but the cost must always be considered. Every breaking version increase for hyper has meant disruption through the ecosystem. It takes a while for all other libraries to update their internal dependency on hyper. That means a time period when trying to combine some libraries will fail, because rustc will treat the 2 different versions as completely different (as they are).

But so what, why not just upgrade this time? The problem is that every single time openssl has a breaking change, that will require hyper to also have a breaking change. So the disruption will occur to the entire hyper ecosystem each time openssl must break an API. Perhaps that slows down to a crawl at some point, and maybe we reconsider then.

Also, hey! The openssl may not be the best default dependency. In fact, rust-native-tls is probably a better default, since it works better for Windows and macOS, and fixes certificate verification on Windows. However, there's a couple downsides with using that as the default as well. First, it still depends on -sys crates, and young ones at that (@sfackler clarified that on Windows and macOS, there wouldn't be an issue, but there still would be on Linux), so breaking changes in those brings us back to the version bump hell mentioned above. Second, it doesn't yet expose all the things that you can currently do with openssl, such as alter verification rules, adjust cipher suites, etc. Probably fine for a default, but it means people who need those things would need to disable hyper's ssl feature anyways, and then they run into the potentially tricky depedency graph juggling abyss that we're currently in right now.

The Solution

Hm, it seems the root of the problem is having such a fundamental dependency, an http crate, depending on a -sys crate. So, let's rip it out! The SslServer and SslClient traits will stay in, so it is still super easy to plug in some TLS. But this allows hyper to exist peacefully, while the various TLS implementations do their own thing. This allows other libraries to determine for themselves what version and even what implementation of TLS they wish to use. If you need an HTTPS client, you can use reqwest, which easily handles this upgrade (I've already checked locally). Other server frameworks can either make a choice on TLS, or they can punt the decision further up the stack, by also being generic over SslServer.

There already exists hyper-openssl, and I've reserved hyper-tls which will soon provide an excellent-for-most-people non-blocking HTTPS connector and accepter.

The 0.10.x already has this done, and it seems to work fine. I've tested that reqwest easily updates to this (it took no code changes, just a Cargo.toml change).

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jan 5, 2017

Contributor

I think this is a reasonable approach.

One minor nit: the -sys crates used by schannel and security-framework don't have the same limitations as openssl-sys, since they're linking to system libraries and don't need to worry about the double-link issue.

Contributor

sfackler commented Jan 5, 2017

I think this is a reasonable approach.

One minor nit: the -sys crates used by schannel and security-framework don't have the same limitations as openssl-sys, since they're linking to system libraries and don't need to worry about the double-link issue.

@drusellers

This comment has been minimized.

Show comment
Hide comment
@drusellers

drusellers Jan 5, 2017

I appreciate the forward looking nature of this issue.

I appreciate the forward looking nature of this issue.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Jan 5, 2017

I second @drusellers , and I think this is a great idea.

I second @drusellers , and I think this is a great idea.

@mgattozzi mgattozzi referenced this issue in SergioBenitez/Rocket Jan 5, 2017

Closed

Encrypt and sign session cookies #20

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Jan 5, 2017

Contributor

This'd also require dropping the dependency on the cookie crate, right?

Contributor

sfackler commented Jan 5, 2017

This'd also require dropping the dependency on the cookie crate, right?

@zsck

This comment has been minimized.

Show comment
Hide comment
@zsck

zsck Jan 5, 2017

I really like this idea. I think that switching to a design that allows users to supply an SslServer or SslClient of their choosing should alleviate the problems I've had.

I think @seanmonstar has stumbled on something that seems somewhat fundamental to crate design. If you could, it would be a huge help for other crates also using openssl directly to have a detailed reference of the changes that would be made to resolve this issue so that they can emulate the approach.

zsck commented Jan 5, 2017

I really like this idea. I think that switching to a design that allows users to supply an SslServer or SslClient of their choosing should alleviate the problems I've had.

I think @seanmonstar has stumbled on something that seems somewhat fundamental to crate design. If you could, it would be a huge help for other crates also using openssl directly to have a detailed reference of the changes that would be made to resolve this issue so that they can emulate the approach.

@seanmonstar

This comment has been minimized.

Show comment
Hide comment
@seanmonstar

seanmonstar Jan 6, 2017

Member

@sfackler good point, I've now removed the cookie and solicit crates from the 0.10.x branch, since they both had optional dependencies to openssl.

@zsck The excellent hyper-openssl crate documentation shows how simple it is to add in openssl 0.9 support into your crate.

Member

seanmonstar commented Jan 6, 2017

@sfackler good point, I've now removed the cookie and solicit crates from the 0.10.x branch, since they both had optional dependencies to openssl.

@zsck The excellent hyper-openssl crate documentation shows how simple it is to add in openssl 0.9 support into your crate.

@zsck

This comment has been minimized.

Show comment
Hide comment
@zsck

zsck Jan 7, 2017

I didn't mean so much "could you explain how to add openssl support" rather "could you document the design considerations that others could follow to implement a similarly 'generic over SslClient' approach".

zsck commented Jan 7, 2017

I didn't mean so much "could you explain how to add openssl support" rather "could you document the design considerations that others could follow to implement a similarly 'generic over SslClient' approach".

@seanmonstar

This comment has been minimized.

Show comment
Hide comment
@seanmonstar

seanmonstar Jan 10, 2017

Member

I plan on making this release later today. I've tested the 0.10.x branch myself, but if anyone else wants to try it or comment about what will be released, now's the time.

Member

seanmonstar commented Jan 10, 2017

I plan on making this release later today. I've tested the 0.10.x branch myself, but if anyone else wants to try it or comment about what will be released, now's the time.

@seanmonstar

This comment has been minimized.

Show comment
Hide comment
@seanmonstar

seanmonstar Jan 10, 2017

Member

Below is the changelog currently expected for the release:


Features

  • client:
    • change ProxyConfig to allow HTTPS proxies (14a4f1c2)
    • remove experimental HTTP2 support (d301c6a1)
  • lib:
    • remove SSL dependencies (2f48612c)
    • remove serde-serialization feature (7b9817ed)
  • header: remove cookie dependency (f22701f7)

Breaking Changes

  • There is no more hyper::http::h2.

    (d301c6a1)

  • The Cookie and SetCookie headers no longer use the
    cookie crate. New headers can be written for any header, or the ones
    provided in hyper can be accessed as strings.

    (f22701f7)

  • There is no longer a serde-serialization feature.
    Look at external crates, like hyper-serde, to fulfill this feature.

    (7b9817ed)

  • hyper will no longer provide OpenSSL support out of the
    box. The hyper::net::Openssl and related types are gone. The Client
    now uses an HttpConnector by default, which will error trying to
    access HTTPS URLs.

    TLS support should be added in from other crates, such as
    hyper-openssl, or similar using different TLS implementations.

    (2f48612c)

  • Usage of with_proxy_config will need to change to
    provide a network connector. For the same functionality, a
    hyper::net::HttpConnector can be easily created and passed.

    (14a4f1c2)

Member

seanmonstar commented Jan 10, 2017

Below is the changelog currently expected for the release:


Features

  • client:
    • change ProxyConfig to allow HTTPS proxies (14a4f1c2)
    • remove experimental HTTP2 support (d301c6a1)
  • lib:
    • remove SSL dependencies (2f48612c)
    • remove serde-serialization feature (7b9817ed)
  • header: remove cookie dependency (f22701f7)

Breaking Changes

  • There is no more hyper::http::h2.

    (d301c6a1)

  • The Cookie and SetCookie headers no longer use the
    cookie crate. New headers can be written for any header, or the ones
    provided in hyper can be accessed as strings.

    (f22701f7)

  • There is no longer a serde-serialization feature.
    Look at external crates, like hyper-serde, to fulfill this feature.

    (7b9817ed)

  • hyper will no longer provide OpenSSL support out of the
    box. The hyper::net::Openssl and related types are gone. The Client
    now uses an HttpConnector by default, which will error trying to
    access HTTPS URLs.

    TLS support should be added in from other crates, such as
    hyper-openssl, or similar using different TLS implementations.

    (2f48612c)

  • Usage of with_proxy_config will need to change to
    provide a network connector. For the same functionality, a
    hyper::net::HttpConnector can be easily created and passed.

    (14a4f1c2)

@seanmonstar

This comment has been minimized.

Show comment
Hide comment
@seanmonstar

seanmonstar Jan 11, 2017

Member

Release is done, docs up, crate published, changelog at https://github.com/hyperium/hyper/releases/tag/v0.10.0

Member

seanmonstar commented Jan 11, 2017

Release is done, docs up, crate published, changelog at https://github.com/hyperium/hyper/releases/tag/v0.10.0

@frewsxcv

This comment has been minimized.

Show comment
Hide comment
@frewsxcv

frewsxcv Jan 11, 2017

Contributor

Awesome, thanks for your work on this @seanmonstar! 🥇

Contributor

frewsxcv commented Jan 11, 2017

Awesome, thanks for your work on this @seanmonstar! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment