Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

curl: Add HTTP/2 support with nghttp2 #37979

Closed
wants to merge 1 commit into from

Conversation

felixbuenemann
Copy link
Sponsor Contributor

This forces compilation against openssl if --with-nghttp2 is specified and --with-libressl is omitted, because --with-darwinssl lacks support for ALPN which is necessary to negotiate HTTP/2 over TLS.

Also fixed a problem where --with-libressl was ignored on Lion.

More info:

@felixbuenemann
Copy link
Sponsor Contributor Author

Btw. technically it is possible to compile cURL with HTTP/2 support while using the default Secure Transport, but because most servers only offer HTTP/2 over TLS it would be of very limited use.

@DomT4
Copy link
Member

DomT4 commented Mar 23, 2015

See #36942 for prior discussion. Probably don't want to ohai - It's kind of an extremely rare thing in-formulae for very special cases.

@felixbuenemann
Copy link
Sponsor Contributor Author

Removed the ohai and pulled the accompanying comment up to the dependencies section.

Sorry for duplicating #36942, I had this change hanging around locally and forgot to check for PRs before polishing it up.

@@ -47,7 +51,7 @@ def install
if build.with?("libressl") && build.with?("openssl")
ohai <<-EOS.undent
--with-openssl and --with-libressl are both specified and
curl can only use one at a time; proceeding with openssl.
curl can only use one at a time; proceeding with libressl.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly, this could cause problems. The last three versions of libressl have had some... problems, working with the generated ca certs, which is causing a few connections to fail without any particularly informative message. Spent a good 7 hours chasing it around earlier, which was fun. We need to default to remain openssl, despite my broader desires and admiration of the libressl codebase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, let's default to openssl here.

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but this is only for the case where both --with-openssl and --with-libressl are defined. This makes the logic for deciding which lib to use much more readable and also fixes a bug where openssl was used on pre Mountain Lion even if --with-libresslwas specified. So I really think this is the much better choice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. cURL is rather special in that it explicitly and happily supports Secure Transport, so we default to that to avoid dropping extra mandatory deps on anyone. Not dead-set against changing the default when both are declared as is the situation here, but I fear we may see some problems for end-users in doing so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks.

@DomT4
Copy link
Member

DomT4 commented Mar 23, 2015

Sorry for duplicating #36942, I had this change hanging around locally and forgot to check for PRs before polishing it up.

The other issue has been dead for a good month or so, and a search of the issues & PRs for curl gets you a ton of results, It's understandable. Normally dupes slow things down a bit, but I don't think there's much risk of that here 😸.

if MacOS.version >= :mountain_lion
# HTTP/2 support requires OpenSSL 1.0.2+ or LibreSSL 2.1.3+ for ALPN Support
# which is currently not supported by Secure Transport (DarwinSSL).
if MacOS.version < :mountain_lion || build.with?("nghttp2") && build.without?("libressl")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some parens here for those of us who can't always remember the precedence rules here? Thanks!

Copy link
Sponsor Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Compilation against openssl is forced if `--with-nghttp2` is specified
and `--with-libressl` is omitted, because the default `--with-darwinssl`
(Secure Transport) lacks support for ALPN which is necessary to negotiate
HTTP/2 over TLS encrypted connections.

Also fixed a problem where `--with-libressl` was ignored on Lion.

More info:
* [cURL HTTP/2 Support](http://curl.haxx.se/dev/readme-http2.html)
* [cURL ALPN Support](http://curl.haxx.se/docs/ssl-compared.html)
@felixbuenemann
Copy link
Sponsor Contributor Author

Pushed new commit to add parens and bump the revision, because of the pre-ML libressl fix.

@MikeMcQuaid
Copy link
Member

Thanks again @felixbuenemann!

@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants