SSL/TLS dependency

[lberezy]

In cases where the crate is being used only for insecure connections to IMAP server, there is no need to build with SSL/TLS support.

I think the correct way to handle this would be to put all SSL/TLS related features behind a "ssl" compile-time feature. This feature would be included in the default features so that forgoing TLS usage is opt-out. This is how other libraries dealing with this typically implement this.

I've got a PR in the works that does this.

[jonhoo]

I'm somewhat disinclined to make SSL/TLS support optional, because I think it should be very rare these days that SSL/TLS is not what you want. Of course, there may be smaller exceptions, like if you already have a secured channel, but those are very niche. What is the use-case you have in mind for this?

[lberezy]

I agree that TLS should be more than strongly recommended, however I think it is still important to give people the option not to pay for something they do not need, especially if this has no impact for the general case (feature on by default).

My scenario is as your said, one that is protected by other transport level protections. The other libraries I am using in my application all have opt-out SSL, but my final binary still depends on a bunch of SSL here. This complicates deployment dependencies and ease of cross-compilation (some SSL crates have special build requirements, like requiring a C compiler be installed).

[brycefisher]

I'm running into the same issue here with cross compilation specifically.

I would propose supporting a pure Rust TLS crate in addition to native_tls as my preferred option to (1) keep TLS support and (2) ease cross complication significantly.

[jonhoo]

@brycefisher I'd be happy to review a PR adding support for a pure Rust TLS crate!

[brycefisher]

Ok! I'll see if I can put one together. I'm experimenting with rustls on raspberrypi now. If that works, I'll open a PR here. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[brycefisher]

So, without looking too deeply into the source of this crate, it appears that the openssl-sys crate is mandatory requirement. If I'm able to get rustls working, both myself and @lberezy would want to opt out of openssl-sys. I would probably do that by creating a cargo feature for openssl that is enabled by default, and then users wanting to cross-compile and rely on rustls would simply disable all features.

The main wrinkle I see with this plan is that the easiest way I can see to do this would allow users to opt out of TLS support altogether by simply using connect_insecure and disabling the default features. I only mention this because @jonhoo specifically stated a desire not to provide a means of disabling TLS completely.

Assuming I (or someone more capable / determined) is able to provide a PR or documentation of how to rustls can work without making the openssl-sys crate necessary, are you still willing to review a PR like this @jonhoo?

[hpk42]

On Sat, Sep 14, 2019 at 21:27 -0700, Bryce Fisher-Fleig wrote: So, without looking too deeply into the source of this crate, it appears that the openssl-sys crate is mandatory requirement. If I'm able to get `rustls` working, both myself and @lberezy would want to opt out of openssl-sys. I would probably do that by creating a cargo feature for openssl that is enabled by default, and then users wanting to cross-compile and rely on rustls would simply disable all features. The main wrinkle I see with this plan is that the easiest way I can see to do this would allow users to opt out of TLS support altogether by simply using `connect_insecure` and disabling the default features. I only mention this because @jonhoo specifically stated a desire not to provide a means of disabling TLS completely.

sidenote: please don't totally prohibit disabling TLS. There are setups (eg someone using a localhost imap server, then connecting to it from Delta Chat) where it would complicate matters.

[jonhoo]

Ah, sorry, to clarify, I would not want to remove connect_insecure at all — it is very handy for things like test deployments where you don't have certs set up. My argument was more for making it hard to disable SSL/TLS support on the crate as a whole, though it may be that that is foolish on my part. I think one tricky thing we might run into is that cargo features must be additive, which may be tricky if we wish to provide two alternative TLS implementations. I actually think my preference might be to one support rusttls. Thoughts?

[brycefisher]

My thought is that in an ideal world there would be an agreed upon TLS trait which offered a standard interface so that how TLS support was implemented would not be the concern of other projects (like the imap crate) depending on TLS support. Sadly that's not yet the world we're in AFAIK.

The way I think about openssl support in this crate is that:

• Openssl is currently the default -- we don't want to change that, so add a default feature which includes openssl support for backwards compatibility
• Openssl is hardcoded into the codebase -- we do want to change this to support cross compilation by allowing users to disable the default feature
• The Client::new() fn appears to allow us to support a different transport or any other TLS implementation

Given those 3 constraints, its not required there's a new cargo feature to support rustls. People using this library could manually integrate that themselves. In an ideal world the imap crate would at least include an integration test / example code for support of rustls. I think this is a good 80/20 solution that allows developers to have TLS & cross compile, but doesn't clutter up the current (very sensible) API you've built for this crate.

All that to say, we don't need to abuse cargo features to support the cross-compilation + rustls use case. How does this plan sound to you?

[brycefisher]

Okay, so without changing any code in this repo, I was able to get this working with rustls using the rustls-connector crate. Here's a POC:
https://github.com/brycefisher/imap-rustls

@jonhoo -- so sorry to bombard you with notifications! If you like the idea of using rustls, I'd like to open a PR that does that following:

1. Adds a default cargo feature which includes native-tls as a dependency, which can be disabled to remove native-tls from the codebase at compile time
2. Include a new example which is more-or-less the same as my POC repo above and ensure this example is tested in CI
3. Perhaps add a note in the crate documentation somewhere that points to this example

If this doesn't feel right and you'd rather provide more ergonomic and better tested integration with rustls, let me know and I can come up with a more robust plan, but it will probably take me a bit longer to complete the work (which is ok with me!)

[jonhoo]

@brycefisher Ah, yes, I like that plan! And don't worry -- that's what happens when you maintain an open-source library that has users, and it is a good problem to have.

That plan seems great. I think the documentation would fit well on Client::new, and maybe also in the crate-level docs under its own heading. I do think pointing at Client::new from connect_insecure might also be a good idea. I have half a mind to get rid of connect_insecure entirely though, and say that if you want an insecure connection, you must go through Client::new -- thoughts?

[brycefisher]

Great! Thanks for being a good sport :-) I'll get to work on that plan. Yeah, removing connect_insecure in favor of Client::new() seems like a reasonable option. Would that be considered a breaking change and major version bump? -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[jonhoo]

Yes, it would be, but I'm okay with that. We already have some backwards-incompatible changes coming due to #133 (see #120 (comment)) and probably #130.

[brycefisher]

@jonhoo I've got a working PR that includes native_tls by default, but can be opted out of:
#140

I started stripping out connect_insecure, but I see this test for connect_insecure_then_secure(). Does that functionality still make sense given we're removing connect_insecure? I'm not quite familliar enough with the context and motivation of that feature to have a sense of whether being able to call secure() to "upgrade" an insecure connection makes sense any more or not.

[brycefisher]

@jonhoo -- you rock! Thank you so much for creating a positive experience for me in contributing to this repo, and teaching me about additive features. Woot!

@lberezy -- should we close out your earlier PR #124 ?

[lberezy]

Sounds like a good idea.

…

On Wed, 25 Sep 2019 at 11:42, Bryce Fisher-Fleig ***@***.***> wrote: @jonhoo <https://github.com/jonhoo> -- you rock! Thank you so much for creating a positive experience for me in contributing to this repo, and teaching me about additive features. Woot! @lberezy <https://github.com/lberezy> -- should we close out your earlier PR #124 <#124> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#123?email_source=notifications&email_token=ABHXKV7M3JV3PX6WG4UWHTLQLK6MTA5CNFSM4HK4XTKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7QJY6I#issuecomment-534813817>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABHXKV4MD6ZPMGNAYT6ELT3QLK6MTANCNFSM4HK4XTKA> .

[jonhoo]

@brycefisher Glad you had a good experience! 🎉

Fixed by #140.

[641i130]

I'm sorry if I'm asking in the wrong place, but I felt this had the most context in case the answer to my question was what I feared.

Does this IMAP crate have support for NON-TLS/SSL connections? I have an obscure reason that I need to use plain text authentication to access an IMAP server (this is all local of course, it's for a university class).

[jonhoo]

Yep, you can use it with plain connections too :)

[641i130]

From my understanding, there is no way to make a client for plain text authentication use. Is this true @jonhoo ?
Hang on, I might've figured it out. Making a 'naked TCP' connection as the contributor recommended. I'll push an example probably for others struggling with this.
I figured it out and made a pull request!
#248