Add TLS support #81

compressed · 2017-01-19T22:05:23Z

So here we go... TLS support take two!

The Options change and recent refactoring made this change much easier to make. I didn't need to modify the macro at all. I was able to re-use all the tests as well with some refactoring. I added the t! macro there in testing (it's common in a few other libraries), mainly because it provides better errors (line numbers) on mac. I can change those to unwraps though or change everything to t! if you want.

I think most of the changes are straight-forward. The only tricky part I ran into was that Options is not Send, so I needed to pull out the related TLS part of Options on both the client and server in order to make the compiler happy.

Note: I did a merge because the changes in both this codebase and my branch were so much that the rebase was turning out to be quite ugly. I believe you can do a merge and squash and GitHub though.

Fix #77

When `-- features tls` is specified for tarpc, RPC communication can also occur over a `TlsStream<TcpStream>` instead of a `TcpStream`. The functional tests have been refactored to use a common set of functions for constructing the client and server structs so that all the tests are shared across non-tls and tls test runs.

A new version of `native-tls` has been released so we don’t need to depend on the git version anymore.

tikue · 2017-01-20T03:44:52Z

Thanks again for doing this! Hopefully I'll get through it all by the end of the weekend.

tikue

I haven't looked at the tests yet, but everything else looks great. Thank you so much.

I'd like @shaladdle to do a once-over, as well.

tikue · 2017-01-20T03:46:05Z

.travis.yml

@@ -21,7 +21,8 @@ before_script:

 script:
 - |
-    travis-cargo build && travis-cargo test
+    travis-cargo build && travis-cargo test &&
+    travis-cargo build -- --features tls && travis-cargo test -- --features tls

 after_success:
 - travis-cargo coveralls --no-sudo


How does this work with features?

These run just like cargo build --features tls. If you pass -- to travis-cargo then whatever follows is passed to cargo.

What about coveralls? Looks like it needs to have the tls feature enabled.

Ideally yes... However, coveralls is broken already in travis-cargo. I think there are a few PRs to fix it in their repo. Maybe we should have a separate PR just to remove travis-cargo. It may be easier than using that tool.

tikue · 2017-01-20T03:52:18Z

README.md

+    let (acceptor, client_cx) = tls_context();
+    HelloServer.listen(addr, server::Options::default()
+                                 .handle(core.handle())
+                                 .tls(acceptor)).wait().unwrap();


Building the client_cx seems pretty straightforward; maybe we could just inline it as .tls(TlsClientContext::new("foobar.com"))

tikue · 2017-01-20T04:00:42Z

src/client.rs

+
+    /// Provides the connection Fn impl for Tls
+    #[doc(hidden)]
+    pub struct ConnectFn {


I could be mistaken, but I have a feeling this can be private?

Oh yes, awesome!

Rust was originally complaining about private type in a public interface, but that was with an older version of the Connect trait. Making this private seems okay now. I like that it can be private again as well.

tikue · 2017-01-20T04:01:17Z

src/server.rs

+                inner: future::Either::B(future::ok(listen_with(new_service,
+                                                                addr,
+                                                                handle,
+                                                                acceptor))),
            }
        }
    }
 }
 /// Spawns a service that binds to the given address using the given handle.
 #[doc(hidden)]


No longer needed.

tikue · 2017-01-20T04:05:18Z

src/client.rs

+    use native_tls::TlsConnector;
+
+    /// TLS context
+    pub struct TlsClientContext {


I don't feel super strongly about this, but since it's already in the client::tls module, we might consider just calling it Context and people can alias it if they so choose. Thoughts? Maybe the tls module should be top-level and it should be tls::client::Context?

cc @shaladdle

Yeah, I prefer client::tls::Context. I prefer client/server to be top level, since those seem like core concepts that you'll always use with tarpc.

Yeah, I actually was considering that as well. I didn't particularly like putting Client in the name, but thought it would be useful if it was available at the root namespace; ::TlsContext wouldn't be very descriptive.

I like the idea of a root tls module.

Spoke a bit with Tim. I think he and I are on the same page now. Can we make the crate structure like this:

root

tls

client

server

The client and server mods can conditionally generate tls-specific methods or options, and the tls module can have types that are used in those methods/options. Does that seem reasonable?

tikue · 2017-01-20T04:14:29Z

src/client.rs

    {
+        #[cfg(not(feature = "tls"))]
+        inner:
+            future::Either<


Can we add a type alias for part of this so we don't have to rewrite the entire thing in both branches of the cfg?

tikue · 2017-01-20T04:36:09Z

src/lib.rs

+        extern crate tokio_tls;
+        pub extern crate native_tls;
+
+        pub use client::tls::TlsClientContext;


Related to my other comment, I'm starting to feel more strongly that there should be a mod tls at the crate root, within which is a client module.

Yes, I like this approach too. I will make those changes.

tikue · 2017-01-20T04:39:30Z

src/macros.rs

    use util::FirstSocketAddr;
    extern crate env_logger;

+    macro_rules! t {


This is basically unwrap on steroids, right? Is there a mnemonic to remember that?

Ha yes. I'm not sure on the history behind the naming though, other than it is short to type...

Short names are nice, but can we name it something that indicates what it does?

We could just call it unwrap! I guess.

tikue · 2017-01-20T04:42:03Z

src/lib.rs

+cfg_if! {
+    if #[cfg(feature = "tls")] {
+        extern crate tokio_tls;
+        pub extern crate native_tls;


Does this have to be public?

Well we need to expose the TlsAcceptor type in the builder for the server options. Another option here though instead of just re-exporting the whole crate would be to just re-export the TlsAcceptor type itself.

I was also thinking on the client side it could take an option TlsConnector if people want to override the default. So we could get away with just re-exporting TlsConnector and TlsAcceptor from native_tls instead of the whole crate; that's probably a better idea.

tikue · 2017-01-20T04:44:22Z

src/server.rs

 /// Additional options to configure how the server operates.
-#[derive(Clone, Default)]
+#[cfg_attr(feature = "tls", derive(Default))]
+#[cfg_attr(not(feature = "tls"), derive(Clone, Default))]


Same comment as above: I'd rather impl the intersection of possible traits for each feature.

tikue

One general comment: there are a lot of cfgs now. It's a little scary from a maintainability perspective, but I'm not sure if there's really a good way around it.

tikue · 2017-01-20T05:03:22Z

src/client.rs

    use tokio_proto::BindClient;
+    #[cfg(feature = "tls")]


Can we use a cfg_if! to group all the tls imports together?

+1, reducing the number of pieces sprayed around the code with cfg(feature..) is very desirable. If they could somehow just be confined to a single file/module, that would be even better, though I know that might be hard.

Yup, will do.

shaladdle

Sending these now so they're not stale.

shaladdle · 2017-01-20T06:00:58Z

Cargo.toml

@@ -33,7 +36,12 @@ env_logger = "0.3"
 futures-cpupool = "0.1"
 clap = "2.0"

+[target.'cfg(target_os = "macos")'.dev-dependencies]


Does this stuff work on osx/linux/windows? Is OSX the only one that needs an extra dependency like this?

This line will make this dependency apply only for osx. For linux, it internally uses the openssl crate, but we can make required data for testing purposes without needing to bring in the openssl crate.

For windows, I didn't write the test case since I don't actually have access to a windows machine and it seemed a little complicated based on the tests I read in the native-tls and tokio-tls crates.

shaladdle · 2017-01-20T06:02:05Z

README.md

+## Example: Futures + TLS
+
+By default, tarpc uses a `TcpStream` for communication, however you can also opt to use a
+`TlsStream<TcpStream>` when the `tls` feature of `tarpc` is enabled.


I think a sentence or two about what TLS is would be useful here (similar to the beginning of the readme that says what an RPC is).

I've added some additional context now. Let me know how this looks.

shaladdle · 2017-01-20T06:03:51Z

README.md

+
+When using TLS, some additional information is required. You will need to make [`TlsAcceptor`] and
+`TlsClientContext` structs; `TlsClientContext` requires a [`TlsConnector`]. The [`TlsAcceptor`] and
+[`TlsConnector`] structs are defined in the [native-tls] crate which is exposed by `tarpc`.


This seems like implicit advice to use these structs as exported by tarpc rather than having the user add the native-tls crate as a dependency. Is that the intention? If so is that approach used by any other libraries? I know we re-export some crates already, but my understanding is that those are there to make the macro standalone.

Yes, that's the direction I was going. I've seen libraries that have public interfaces which require structs from other crates generally do this. You can see in reqwest and hyper. I think the best approach would be to just re-export only the external types we need, not the whole crate. I'll make that change when I do my updates.

shaladdle · 2017-01-20T06:04:49Z

README.md

+[native-tls]: https://github.com/sfackler/rust-native-tls
+
+Enabling the `tls` feature does not require you to use TLS streams. You can still
+use TCP streams as well or a combination of both stream types.


Please clarify that you mean it's ok/possible to use both types of stream in the same binary, rather than being able to use either TLS or non-TLS stream types for the same server.

shaladdle · 2017-01-20T06:09:04Z

src/client.rs

+    use native_tls::TlsConnector;
+
+    /// TLS context
+    pub struct TlsClientContext {


Yeah, I prefer client::tls::Context. I prefer client/server to be top level, since those seem like core concepts that you'll always use with tarpc.

shaladdle · 2017-01-20T06:09:14Z

src/client.rs

+    impl TlsClientContext {
+        /// Try to make a new `TlsClientContext`, providing the domain the client will
+        /// connect to.
+        pub fn try_new<S: Into<String>>(domain: S)


shaladdle · 2017-01-20T06:12:29Z

src/client.rs

@@ -91,9 +119,12 @@ impl<Req, Resp, E> fmt::Debug for Client<Req, Resp, E>
 }

 /// Additional options to configure how the client connects and operates.
-#[derive(Clone, Default)]
+#[cfg_attr(feature = "tls", derive(Default))]
+#[cfg_attr(not(feature = "tls"), derive(Clone, Default))]


shaladdle · 2017-01-20T06:13:11Z

src/client.rs

    use tokio_proto::BindClient;
+    #[cfg(feature = "tls")]


+1, reducing the number of pieces sprayed around the code with cfg(feature..) is very desirable. If they could somehow just be confined to a single file/module, that would be even better, though I know that might be hard.

compressed · 2017-01-23T00:22:42Z

Thanks for all the comments. I'll work on these tomorrow!

shaladdle

Ok, got through everything :). Thanks so much for the PR. Overall it looks good. Two requests:

Can you add one test that instantiates both a tls and non-tls server and uses both independently? This is mostly meant as a sanity check that something weird doesn't happen when using both.

I'm also curious about error cases related specifically to TLS. What is supposed to happen when a non-TLS client tries to connect to a TLS server? What about the opposite? Can we have tests for these cases?

shaladdle · 2017-01-22T23:38:12Z

src/macros.rs

    use util::FirstSocketAddr;
    extern crate env_logger;

+    macro_rules! t {


Short names are nice, but can we name it something that indicates what it does?

shaladdle · 2017-01-22T23:46:59Z

src/client.rs

+    use native_tls::TlsConnector;
+
+    /// TLS context
+    pub struct TlsClientContext {


Spoke a bit with Tim. I think he and I are on the same page now. Can we make the crate structure like this:

root

tls

client

server

The client and server mods can conditionally generate tls-specific methods or options, and the tls module can have types that are used in those methods/options. Does that seem reasonable?

shaladdle · 2017-01-23T00:15:24Z

src/client.rs

            let rx = match options.reactor {
                Some(Reactor::Handle(handle)) => {
-                    let tcp = TcpStream::connect(&addr, &handle).map(MultiplexConnect::new(handle));


Instead of reproducing these three lines twice, can we only build the ConnectFn using cfg, and do the rest unconditionally?

Good idea. I've implemented that.

shaladdle · 2017-01-23T00:15:42Z

src/client.rs

@@ -227,6 +352,7 @@ pub mod future {
            ConnectFuture { inner: future::Either::B(rx.map_err(panic as fn(_) -> _).flatten()) }
        }
    }
+


Is this blank line intentional?

Good catch. I have removed it.

shaladdle · 2017-01-23T00:22:08Z

src/lib.rs

@@ -59,6 +60,53 @@
 //! }
 //! ```
 //!
+//! Example usage with TLS:
+//!
+#![cfg_attr(feature = "tls", doc = " ```rust,no_run")]


What do these cfgs do?

These provide some configurability for what runs during rustdoc's test.

So for these two:

#![cfg_attr(feature = "tls", doc = " ```ignore")] #![cfg_attr(not(feature = "tls"), doc = " ```")]

It's interpreted something like this:

if feature != tls { return //! ``` }; if feature == tls { return //! ```ignore };

ignore just means don't run this test case at all. no_run is going to compile the test, but not actually run it. We want this for the tls use case because we need to add extra stuff like in the test cases to add the certs to the cert chain, which we don't need to here in the rustdoc.

For the tcp side actually we can run that now even with tls enabled, so the first set of conditions can be removed.

shaladdle · 2017-01-23T05:49:50Z

src/server.rs

@@ -50,34 +75,48 @@ pub fn listen<S, Req, Resp, E>(new_service: S, addr: SocketAddr, options: Option
          Resp: Serialize + 'static,
          E: Serialize + 'static
 {
+    // similar to the client, since `Options` is not `Send`, we take the `TlsAcceptor` when it is


nit: Capitalize

shaladdle · 2017-01-23T05:52:21Z

src/server.rs

+fn listen_with<S, Req, Resp, E>(new_service: S,
+                                addr: SocketAddr,
+                                handle: Handle,
+                                _acceptor: Acceptor)


Why _? Is this unused with tls off or something?

Yes, that's correct. We don't use acceptor in the tcp workflow.

tikue · 2017-01-23T18:38:17Z

I can't figure out how to reply to one of @shaladdle's comments, so I'm just writing it here. It would be great if a non-tls client trying to connect to a tls server receives a specific tls-related error, like Unauthenticated. Is that possible?

compressed · 2017-01-23T23:36:02Z

I've made updates to incorporate most of the comments (and also updated to latest master).

Here are the outstanding items/discussion points (I think!):

Test cases / error conditions: I added one test case before that tests both tcp and tls where they are configured properly, so that functionality is validated. However, I didn't add a test case for misconfigured clients. If you use a TLS client on a TCP server, the client connection just waits indefinitely, probably because the client is waiting for the tls handshake to begin. If you use a TCP client on a TLS server, the server logs an error when the client makes an RPC call and the client waits indefinitely (ERROR:tarpc::server: While processing incoming connections: connection closed via error, from here: https://github.com/google/tarpc/blob/master/src/server.rs#L98). Ideally, it would be best to catch these misconfigurations at compile time, but I'm not sure the best way to do that. I was thinking we could require some reference from the server to make the client, but even that won't work because the server struct itself could be used to make both a tcp and a tls server... Not sure here.
TlsClientContext name? I kept it as-is for now. Since we don't have tls::client etc., I thought we should keep client in the name. It also goes along with the other tls types: TlsAcceptor, TlsConnector, but open to change.
Lots of cfgs. I'm not sure what to do about this either. The only way to eliminate most of them would be to make the interfaces generic over all stream types but we tried that before in the earlier versions of this PR, but then macro itself becomes quite unwieldly which I think is even worse.

tikue · 2017-01-24T05:33:21Z

TlsClientContext name? I kept it as-is for now. Since we don't have tls::client etc., I thought we should keep client in the name. It also goes along with the other tls types: TlsAcceptor, TlsConnector, but open to change.

I still think it should be named Context. It can be used as client::tls::Context or aliased as ClientTlsContext or TlsClientContext. Also, I don't think there are good reasons to reexport it from the crate root.

compressed · 2017-01-24T14:07:08Z

Okay thanks! Works for me. I've made that change now too.

The `cfg_attr` logic caused many false warnings from clippy.

shaladdle

I think we're nearing the end here. Just (hopefully) some small things.

shaladdle · 2017-01-27T03:43:11Z

src/lib.rs

+        extern crate tokio_tls;
+        extern crate native_tls;
+
+        /// Re-exported TLS-related types


Why do these need to be re-exported?

Because we have public interfaces now that use all these types, except Pkcs12, but you need that type to make the TlsAcceptor itself.

shaladdle · 2017-01-27T03:57:40Z

src/client.rs

+    }
+
+    impl Context {
+        /// Try to construct a new `Context`, providing the domain the client will


s/providing/provided?

Good point, actually this comment is wrong. It's the domain used for validation, not connection. I will update that.

compressed · 2017-01-27T18:50:53Z

Great! I've updated the comments for domain in the most recent commit to be more accurate.

tikue · 2017-01-30T20:23:46Z

Relevant issue

tikue · 2017-01-31T01:25:43Z

src/lib.rs

+
+        /// Re-exported TLS-related types
+        pub mod tls {
+            pub use native_tls::Error as NativeTlsError;


what about renaming the module native_tls and then pub use native_tls::Error? (and then just add it to the below pub use statement)

Done. Used extern crate native_tls as native_tls_inner to avoid the name spacing problem.

tikue · 2017-01-31T01:30:51Z

src/macros.rs

+            const DOMAIN: &'static str = "foobar.com";
+
+            use client::tls::Context;
+            use ::native_tls::{Pkcs12, TlsAcceptor, TlsConnector};


I think the :: prefix is redundant?

tikue

Thanks again, @compressed, this is an incredibly important addition!

compressed · 2017-01-31T02:17:47Z

You're welcome! I'm excited for it to land. Thanks for all the comments and working with me on it.

I just made one more minor change to add Error to the use above this line: https://github.com/compressed/tarpc/blob/c6bc6a871be294303942f7d70980f49bbdc811ab/src/client.rs#L46 so then the type can just be Error not ::native_tls::Error in the new fn so that's why it's building again.

shaladdle · 2017-01-31T05:03:05Z

+1, this is great, @compressed. Thanks so much for contributing :)

compressed and others added 15 commits January 12, 2017 14:32

Work around some type issues

5fa4359

Remove stream type param

6fc9469

Update pre-push to test tls

31b993f

chore(deps): use crates.io versions for tls dependencies

7e4257f

A new version of `native-tls` has been released so we don’t need to depend on the git version anymore.

chore(error): native2io -> native_to_io

31c2c88

docs: update examples

9a21c25

test: add tls and tcp at the same time test case

f296219

chore: Merge branch 'google/master' into tls-support-v2

49d19a8

chore: cleanup

ce132a6

chore: update travis-cargo command

a26805a

docs(README): formatting for TLS section

87a21ee

chore: rm unneeded comment

d7c2c33

chore: correct client visibility

65c68be

docs(client): add comment for mod tls

c5a5dd6

compressed force-pushed the tls-support-v4 branch from e0f27cb to c5a5dd6 Compare January 19, 2017 22:20

tikue requested changes Jan 20, 2017

View reviewed changes

shaladdle reviewed Jan 22, 2017

View reviewed changes

shaladdle requested changes Jan 23, 2017

View reviewed changes

compressed added 3 commits January 23, 2017 17:08

refactor: address review comments

16625f5

chore: Merge branch 'master' into tls-support-v4

64c87ca

chore: rustfmt

61acb5f

compressed added 2 commits January 23, 2017 19:42

chore: rm extra newline

6e4c22b

chore: use re-exported NativeTlsError type

e9653f5

refactor: TlsClientContext -> tls::client::Context

ce5cbca

compressed added 3 commits January 24, 2017 09:30

chore: clippy

e1311e6

docs(example): update formatting

4831c27

docs(TLS): set the TLS doctest to ignore

9f358a8

The `cfg_attr` logic caused many false warnings from clippy.

shaladdle requested changes Jan 27, 2017

View reviewed changes

docs(client): correct comment about tls::Context.domain's usage

f0dd32e

tikue requested changes Jan 31, 2017

View reviewed changes

shaladdle approved these changes Jan 31, 2017

View reviewed changes

tikue approved these changes Jan 31, 2017

View reviewed changes

compressed force-pushed the tls-support-v4 branch from a35a3e3 to c6bc6a8 Compare January 31, 2017 02:09

fix(tls): change tls to native_tls for re-exports

7c9cca8

compressed force-pushed the tls-support-v4 branch from c6bc6a8 to 7c9cca8 Compare January 31, 2017 13:47

tikue added 4 commits January 31, 2017 09:55

Merge branch 'master' into tls-support-v4

8b3f8d5

Fix merge fallout

bec30af

Fix merge fallout

c2bcc88

Fix merge fallout.

ac353e0

tikue merged commit fafe569 into google:master Jan 31, 2017

Add TLS support #81

Add TLS support #81

Conversation

compressed commented Jan 19, 2017

tikue commented Jan 20, 2017

tikue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tikue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaladdle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compressed commented Jan 23, 2017

shaladdle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compressed Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tikue commented Jan 23, 2017

compressed commented Jan 23, 2017

tikue commented Jan 24, 2017

compressed commented Jan 24, 2017

shaladdle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compressed commented Jan 27, 2017

tikue commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compressed Jan 23, 2017 •

edited

Loading