Hyper: Enabling hostname verification by default was a breaking change

[mhristache]

I am maintaining a tool at my company who's main functionality was silently broken (still compiles but fails at runtime) due to https://github.com/hyperium/hyper/commit/01160abd92956e5f995cc45790df7a2b86c8989f.

Now I get: An error in the OpenSSL library: certificate verify failed. FYI, we are using self signed certificates internally.

This is clearly a breaking change in hyper so I don't understand why the minor release was not bumped. Of course, I can disable the hostname verification but the point is that semver was not followed by hyper.

[pyfisch]

Obviously this was considered to be a bugfix because nowhere hyper said that it didn't check the host.

[mhristache]

Well, it didn't check the host before so I don't really get your point.

Also, there doesn't seem to be an easy way to disable hostname verification, is there? Any suggestion on how to achieve that?

Otherwise I will have to use 0.8 or switch to curl.

Thanks

[mhristache]

Just found out that I can force the previous hyper version by setting "=0.9.3" in the Cargo.toml for the crate where the binary is created, which propagates to all libs using hyper, so I will use that for now.

[tarcieri]

the point is that semver was not followed by hyper.

Please look here:

http://semver.org/

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

Hyper is following semantic versioning just fine. Your expectations aren't, however.

[sfackler]

This was not caused by the addition of hostname verification - if your company's self signed certs are set up in a reasonable way, the hostname check should pass just fine. It was caused by Hyper changing from _completely ignoring the contents of the certificates the server sends_ to actually looking at those and seeing if they're trusted.

I'm not sure I understand why you'd have to switch to curl to fix this - curl requires manual intervention in almost exactly the same way that Hyper does - you'll need to add those self signed certificates to OpenSSL's trusted set with something like http://sfackler.github.io/rust-openssl/doc/v0.7.13/openssl/ssl/struct.SslContext.html#method.set_CA_file for Hyper and --cafile for curl.

[MoSal]

@sfackler

Let me quote my comment from #472

There are use-cases where skipping TLS verification is necessary.

In libcurl, the options CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST are > available and you can set them to 0.

The curl tool has the option -k,--insecure which relies on the library options mentioned above.

[mhristache]

@tarcieri you are correct but so far hyper released a new minor version every time a breaking change was added.

@sfackler As @MoSal mentioned, libcurl provides ways to disable the host verification and I want something similar for hyper. I will try to play with the Openssl context as you suggested in the other thread.

Setting the CA file does not help my use case as I would have to ask the user to provide the path to the CA file which is basically killing the ease of use of my tool for no good reason (it's an internal tool, running in an environment without internet access, all servers are trustable etc).

Thanks for your comments

[tarcieri]

@maximih by default hyper's previous configuration was insecure. That's bad, but it's still in 0.x versions, and now they have shipped a fix so it's now secure-by-default.

SemVer says you should reasonably expect changes at any time, because hyper is still in an initial development stage.

I'm sorry that a change in a 0.x minor version broke your insecure configuration because hyper added security, but at the same time you cited SemVer, and SemVer says they're allowed to do that.

[paragonie-scott]

More directly: Why are you expecting non-stable software's API to be... stable?

[mhristache]

@tarcieri thanks for the clarifications, I already said in my previous comment that you are correct in regards to semver. Also, I would appreciate if you keep the irony out of your comments. I don't find it friendly and I expect only friendliness and love from the rust community :). Thanks

@paragonie-scott I was expecting a minor version bump as was (almost) always the case with hyper breaking changes so far.

[frewsxcv]

Enabling hostname verification is neither a breaking change nor a new feature; it is a bugfix.

Speaking generically, if one's software is relying on a broken feature, any bugfix can be breaking change.

[DemiMarie]

The correct solution for your company is to install the certificates in a place where OpenSSL will find them.

[aidanhs]

I agree that this was a bugfix. However

More directly: Why are you expecting non-stable software's API to be... stable?

and the other comments saying "well it's pre-1.0 so you should be ready for breaking changes every day" aren't really fair in general (this issue aside). Arewewebyet says "Rust has a mature HTTP stack", with a footnote of "getting there, stable but maturing" [1]. Neither the github readme nor hyper.rs mention anything about instability, and so the only hint is the orange badge with the version. If hyper is actively changing and breaking (it is), I don't think it's unreasonable to want it to do a better job of advertising that fact.

The reality is that the nodejs ecosystem (arguably one which contributed significantly to the popularity of semver) has damaged the meaning of pre-1.0 version numbers with a culture of refusing to touch 1.0. It's got better, but for a project with such a key role in the ecosystem like hyper, an explicit note saying "Hyper is pre 1.0 and is still undergoing breaking changes" helps clarify both for people used to old nodejs-semver and who aren't really aware of semver at all.

[1] as an aside, there's a comment on hyper on arewewebyet from feb which says "I don’t see hyper having major breaking changes in the forseeable future and is pretty usable/ready.", which is clearly wrong

[seanmonstar]

I agree it was a breaking change. It's unfortunate that it broke some instances. I'm very sorry about that.

At the same time, it felt like a security fix for a majority of users. It's likely that most people using hyper were just hoping that it does everything correct and securely. And it can take time for frameworks to upgrade to a new minor version of hyper, which would mean those users don't have the security fix quickly.

It was definitely a dilemma for me, and I opted for the decision that helped the majority case. Maybe I chose wrong, I'm certainly still open to hearing that I did.

I'm going to close the issue, since it's not something that can be fixed in the code, but comments are still open.

---

@aidanhs I agree about the nodejs version situation. I'm not afraid of tagging 1.0; I've done so on a few of my other crates. In hyper's case, I truly believe it's not 1.0 yet. Clearly, switching to async IO is a big change that I feel is required for a 1.0. I imagine that after releasing async IO, some feedback will come to make some other tweaks. It'd be nice for hyper 1.0 to also support the major use cases of HTTP2, but if that appeared in hyper 1.1, that'd be fine too.

Also, that quote on arewewebyet is definitely incorrect :)

Perhaps it'd be best to add to the README some text "hyper is still evolving towards 1.0, and may have breaking changes", and add a link to a Milestone page. Perhaps also a Milestone page could be added to the wiki (or maybe the Issues milestones are sufficient?).

[tarcieri]

@aidanhs this was a CVE-worthy security bugfix. Previously Hyper was trivially vulnerable to a MitM attack.

Regardless of version numbers, if fixing CVE-worthy security issues involves breaking changes, IMO those changes should be made. If programs were relying on the old behavior, they are broken and should be fixed.

This is par for the course with security fixes: if new attacks are found, often breaking changes have to be made to fix them. The Internet community has recently "retired" several old, broken algorithms and protocols like RC4, DHE (at least for HTTPS), and SSLv3. Making those changes involved breakages.

We shouldn't continue to support insecure systems for backwards compatibility's sake. Supporting broken crypto for backwards compatibility's sake makes everyone else less secure.

[aidanhs]

@seanmonstar I did consider doing that, but didn't want to be too presumptuous. Since you suggested it, I've made a PR! I think the issues milestone is probably best, since it's most likely to get updated.

@tarcieri in my comment I do say "this issue aside" - I maybe could have emphasised that more.
I was already in agreement with everything you've said in your comment and personally I think "this was a security bugfix" is a fine reason for fixing this in the patch version of a hypothetical post-1.0 project. My comment was more aimed at the general "well you should know what you're getting in for with pre-1.0" feeling I was getting - hyper would be changing even if there weren't critical security issues (I'm a big fan of the direction fwiw) and I think we can do a better job of calling this out, especially given the wider context of nodejs history and hyper being a pretty key project in an important corner of the rust ecosystem. To do something about it I've created https://github.com/bashyHQ/arewewebyet/pull/48 and https://github.com/hyperium/hyper/issues/889.