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

[WIP] Support ALPN on Security Framework #100

Closed
wants to merge 1 commit into from

Conversation

kzys
Copy link

@kzys kzys commented Jun 29, 2018

This change exposes security-framework's ALPN methods through
native-tls.

security-framework 0.2.1 doesn't have ALPN methods yet.
So the build wouldn't succeed now.

This change exposes security-framework's ALPN methods through
native-tls.

security-framework 0.2.1 doesn't have ALPN methods yet.
So the build wouldn't succeed now.
@kzys
Copy link
Author

kzys commented Jun 29, 2018

This branch is still work-in-progress. I'm unsure how to provide the feature on platforms that don't support ALPN (e.g. older OS X versions)

Copy link
Owner

@sfackler sfackler left a comment

Choose a reason for hiding this comment

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

My current thinking on how we would deal with ALPN on unsupported platforms (e.g. OSX 10.12, OpenSSL 1.0.1, Windows Vista) is that the APIs would always be present, but ALPN would just not run when it isn't supported.

For Security.framework we'll probably have to implement this by not actually using the security-framework but by instead using dlsym to dynamically load the symbols at runtime if present.

@@ -628,6 +641,11 @@ impl<S: io::Read + io::Write> TlsStream<S> {
Ok(self.0.tls_server_end_point()?)
}

/// Returns the negotiated protocols
pub fn negotiated_protocols(&self) -> Result<Vec<String>> {
Copy link
Owner

Choose a reason for hiding this comment

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

ALPN only allows a single protocol to be negotiated, so this should return a Result<Option<String>>.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. Will update the method.

@kzys
Copy link
Author

kzys commented Jul 1, 2018

Would it make sense to return Result<(), E> to tell the caller that the APIs are no-op? Ignoring the call implicitly would confuse developers.

Regarding dlsym, how about having that in security-framework? First calling dlsym here may need security-framework to expose some guts such as SSLContext. Second people who use security-framework may want to have the dynamic-switch behavior.

@sfackler
Copy link
Owner

Returning an error is a possibility, yeah.

SSLContext is already exposed through the TCFType impl.

@kzys
Copy link
Author

kzys commented Jul 10, 2018

Well, while SSLContext is available as a type, getting that from secure_transport::SslStream may need some changes in native-tls. Wouldn't it be?

@sfackler
Copy link
Owner

@kzys
Copy link
Author

kzys commented Jul 11, 2018

Thanks. I'm trying to implement the suggested dlopen-based method on native-tls side.

SSLSetALPNProtocols must be called before/during handshake(), against the context. However I think security-framework currently doesn't provide hook points to do that.

https://github.com/sfackler/rust-native-tls/blob/master/src/imp/security_framework.rs#L305

@sfackler
Copy link
Owner

We could skip the ClientBuilder and manage the SSLContext directly but there's quite a bit of logic there...

@kzys
Copy link
Author

kzys commented Jul 11, 2018

Yep. Then again, wouldn't be easier to have dlopen in security-framework? security-framework is currently just a simple one-to-one wrapper for Security.framework. Having dlopen would complicate the crate though.

@kzys
Copy link
Author

kzys commented Jul 25, 2018

I've opened a new pull request on rust-security-framework, that replaces ALPN-related methods with dlopen-based implementation.

kornelski/rust-security-framework#68

@kzys
Copy link
Author

kzys commented Apr 13, 2019

Let me close this PR for a moment, since I haven't been working on that for more than a month.

@seanmonstar did similar work for OpenSSL, but we need to make some progress on steffengy/schannel-rs#49 first to have ALPN on rust-native-tls.

@kzys kzys closed this Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants