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
darwinssl: add support for ALPN negotiation #2731
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks okay, compiles okay, and does make HTTP/2 work as expected. It is important, though, to exclude versions of High Sierra prior to 10.13.4. Thanks for working on this.
lib/vtls/darwinssl.c
Outdated
@@ -1573,6 +1573,35 @@ static CURLcode darwinssl_connect_step1(struct connectdata *conn, | |||
} | |||
#endif /* CURL_BUILD_MAC_10_8 || CURL_BUILD_IOS */ | |||
|
|||
#if (CURL_BUILD_MAC_10_13 || CURL_BUILD_IOS_11) && HAVE_BUILTIN_AVAILABLE == 1 | |||
if(conn->bits.tls_enable_alpn) { | |||
if(__builtin_available(macOS 10.13, iOS 11, *)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throughout this code, we need to be checking for macOS 10.13.4 or later. When 10.13 was first released, it had both ALPN functions defined in the header, but the implementations were actually missing from the framework. Apple fixed that in 10.13.4.
docs/HTTP2.md
Outdated
@@ -63,6 +63,7 @@ the necessary TLS features. Right now we support: | |||
- mbedTLS: ALPN | |||
- SChannel: ALPN | |||
- wolfSSL: ALPN | |||
- Secure Transport: ALPN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add additional spaces to the above eight lines so they all line up.
Not sure if you caught it; I've re-pushed this with your comments addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
Rewrite of #500 using the public APIs introduced in macOS 10.13 and iOS 11.