(feature) Allow opt-out from `native_tls` crate #140

brycefisher · 2019-09-20T07:00:39Z

Ready for review! @jonhoo

Establishes conditional compilation for all integration with the native_tls crate. Since native_tls has been deeply integrated into this crate for a long time, we want to maintain backwards compatibility by making this feature part of the default.

For a consumer of this crate to "opt-out", include this in cargo.toml:

[dependencies.imap]
version = "0.16.0"        # Replace this with the correct version
default-features = false

See the conversation for details on this approach:
#123

In order to discourage developers from avoiding TLS, we're also simultaneously removing the connect_insecure method. However, it is still possible to use an unsecured TCP connection for an imap session by using imap::Client::new() and providing a naked TcpStream.

.cirrus.yml

azure-pipelines.yml

src/client.rs

Cargo.toml

jonhoo · 2019-09-21T15:32:44Z

src/error.rs

@@ -22,8 +25,10 @@ pub enum Error {
    /// An `io::Error` that occurred while trying to read or write to a network stream.
    Io(IoError),
    /// An error from the `native_tls` library during the TLS handshake.
+    #[cfg(feature = "default")]


So, this is actually pretty problematic, because it makes features non-additive. I think for this to be okay, we'll need rust-lang/rust#44109, which hasn't landed yet. One possible solution would be to add a variant to the end of this enum like this:

#[doc(hidden)] __Nonexhaustive,

That prevents people from matching on Error, and thus makes the features additive again.

jonhoo · 2019-09-21T15:36:16Z

I also just saw #123 (comment), and I think secure does still make sense. It should be generic though; currently it is under impl Client<TcpStream>, but I think it should be possible to secure any transport that is Read + Write.

jonhoo · 2019-09-21T15:36:38Z

As for that test, I would re-write it to manually establish the TcpStream, then use Client::new + secure.

brycefisher · 2019-09-21T22:45:25Z

Np! Will do tonight -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

brycefisher · 2019-09-21T22:51:34Z

I'll not super experienced with cargo features, but when writing tests and trying to prepare this PR, I found that compiling with different sets of features results in the compiler type-checking different sets of code. To ensure that both default and no default compiles, I _think_ it is necessary to at least run a cargo check this way. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

brycefisher · 2019-09-21T22:53:22Z

I see! Ok, so we probably do want the check with --no-default-features in Azure pipelines, but its probably overkill here. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

brycefisher · 2019-09-21T22:54:10Z

Right! Or rather feature = "tls" as you mentioned earlier -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

brycefisher · 2019-09-21T22:54:18Z

Love it! -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

brycefisher · 2019-09-21T22:55:20Z

I'm very happy to make that change, but I don't fully understand the problem. Does the link you shared explain this issue in more depth? -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

brycefisher · 2019-09-21T22:55:56Z

That makes a ton of sense to me. I'll see how far I can get on this!! -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

jonhoo · 2019-09-22T15:32:45Z

So, with regards to Azure Pipelines, the current CI already checks with --no-default-features, so it should already include all the steps you had there :)

As for additive features, consider if one crate, A, has:

[dependencies]
imap = { version = "...", default-features = false }

And another, B, has:

[dependencies]
imap = "..." # so with the tls feature

If you have a crate C that depends on both A and B, then cargo will only compile imap once with all the features enabled. However, consider if A contained something like:

match imap::connect(...) {
  Ok(c) => ...,
  Err(imap::Error::Io(...)) => ...,
  Err(imap::Error::Bad(...)) => ...,
  Err(imap::Error::No(...)) => ...,
  ...
}

That compiles just fine if the tls feature is not enabled, but would not compiled if the tls feature is enabled (it would complain that you are missing the variants TlsHandshake and Tls). This is what we mean by features having to be "additive" — adding a feature can never remove something or otherwise cause something that compiled to no longer compile, because cargo will frequently add features to combine dependencies.

By adding the proposed hidden variant:

#[doc(hidden)]
__Nonexhaustive,

Now it'd be pretty clear to whoever wrote crate A that they should never be trying to match on all the variants of Error, because new variants may be added.

brycefisher · 2019-09-23T18:11:55Z

I see! Okay, got it.

brycefisher · 2019-09-24T05:10:55Z

secure does still make sense. It should be generic though; currently it is under impl Client, but I think it should be possible to secure any transport that is Read + Write.

I'm happy to tackle this! However, I'm seeing what looks like a lot of refactoring work here to nudge the type signatures. In particular the Error::TlsHandshake also knows about the underlying type of the transport. Making this enum generic will probably make a lot of other things more generic, and may make the Error type itself a wee bit unergonomic to users of the library.

I spent a few minutes trying to think of how to change the Error type without being so generic. The only decent idea I came up with was trying to immediately pull out the values of the TlsHandshakeError as strings and storing those on the Error::TlsHandshake. It feels a bit unidiomatic, but on the other hand, the any Error trait object should be easy to pull strings from.

jonhoo · 2019-09-24T13:17:05Z

I think the __Nonexhaustive change is still missing?

jonhoo · 2019-09-24T13:18:05Z

Ah, that's a shame about .secure, but it's probably okay for now. It should be a backwards-compatible change to change it later anyway.

brycefisher · 2019-09-24T15:11:29Z

Oh good catch! I wrote this code last night but somehow forgot to push it.

Establishes conditional compilation for all integration with the native_tls crate in this crate. Since native_tls has been deeply integrated into this crate for a long time, we want to maintain backwards compatibility by making this feature part of the default. For a consumer of this crate to "opt-out", including this in cargo.toml: ``` [dependencies.imap] version = 0.16.0 # Replace this with the correct version default-features = false ``` See the conversation on Github for details on this approach: #123

In order to discourage folks from connecting securely, we're removing the convenience method imap::connect_insecure. Fear not\! For those who manage security in another way (aka a private network or similar measures), it is still possible to connect without TLS by using the imap::Client::new() method. See that method for examples of how to do this.

Also point out the examples/rustlts.rs file for pure Rust TLS goodness

jonhoo · 2019-09-24T23:20:41Z

Looks great, thank you!

jonhoo · 2020-02-20T18:08:40Z

Published in 2.0.0 🎉

Unfortunatly, this implies removing support of unsecured IMAP connection due to evolutions of the IMAP library See jonhoo/rust-imap#140

* Updating non-conflicting deps * Updating atom is not so straightforward * reqwests will be hard to migrate * Upgraded IMAP. Unfortunatly, this implies removing support of unsecured IMAP connection due to evolutions of the IMAP library See jonhoo/rust-imap#140 * Migrating reqwest was just a little harder : I had to get the dependency with the correct feature, then use the blocking part everywhere

brycefisher mentioned this pull request Sep 21, 2019

SSL/TLS dependency #123

Closed

brycefisher changed the title ~~WIP add cargo feature~~ (feature) Allow opt-out from native_tls crate Sep 21, 2019