hyper: Enabling hostname verification by default was a breaking change
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.
About this issue
- Original URL
- State: closed
- Created 8 years ago
- Comments: 16 (6 by maintainers)
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?).
@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.
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
--cafilefor curl.