Skip to content
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

rcombs
Copy link
Contributor

@rcombs 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 July 11, 2018 10:31
@bagder bagder added the TLS label Jul 11, 2018
Copy link
Member

@nickzman nickzman left a 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.

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

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
Copy link
Member

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.

@rcombs
Copy link
Contributor Author

rcombs commented Jul 14, 2018

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

Copy link
Member

@nickzman nickzman left a 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!

@nickzman nickzman merged commit 092f681 into curl:master Jul 14, 2018
@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.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants