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

Merged
merged 1 commit into from Jul 14, 2018

Conversation

Projects
None yet
3 participants
@rcombs
Contributor

rcombs commented Jul 11, 2018

Rewrite of #500 using the public APIs introduced in macOS 10.13 and iOS 11.

@bagder bagder requested a review from nickzman Jul 11, 2018

@bagder bagder added the SSL/TLS label Jul 11, 2018

@nickzman

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.

@@ -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, *)) {

This comment has been minimized.

@nickzman

nickzman Jul 12, 2018

Collaborator

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.

@@ -63,6 +63,7 @@ the necessary TLS features. Right now we support:
- mbedTLS: ALPN
- SChannel: ALPN
- wolfSSL: ALPN
- Secure Transport: ALPN

This comment has been minimized.

@nickzman

nickzman Jul 12, 2018

Collaborator

Let's add additional spaces to the above eight lines so they all line up.

@rcombs rcombs force-pushed the rcombs:alpn-darwinssl branch from 4336fbe to 65f3696 Jul 12, 2018

@rcombs

This comment has been minimized.

Contributor

rcombs commented Jul 14, 2018

Not sure if you caught it; I've re-pushed this with your comments addressed.

@nickzman

Thanks for working on this!

@nickzman nickzman merged commit 092f681 into curl:master Jul 14, 2018

5 checks passed

LGTM analysis: C/C++ No alert changes
Details
LGTM analysis: Python No alert changes
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 75.565%
Details

@lock lock bot locked as resolved and limited conversation to collaborators Oct 13, 2018

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