Native SASL support

[dequbed]

Hi! ^^

This PR adds native SASL support using rsasl. I've been testing usability of the rsasl API by patching existing crates to use rsasl and this PR is basically this testing code for rust-imap.

So in essence this is an issue asking if this is someting that you would be interested in merging, but already coming with some code attached ^^

Why would we want this?

While with the current Authenticator system most of SASL can be implemented there are a few advantages in using an external crate.

For one, features like SASL security layers and channel bindings aren't really doable with the current approach. The latter specifically is something that would want explicit support by this crate as the strongest channel binding — tls-exporter — needs access to the TLS state otherwise managed by rust-imap. Mechanism selection is another thing that could be implemented somewhat easier upstream as an user of the crate doesn't have access to capability without authenticating first.

Lastly it would allow us to drop the direct dependency on base64 (although rsasl itself depends on the very same again) ^^

What's the cost?

Of course this will result in an additional dependency, but I designed rsasl to make heavy use of feature unification to make it as cheap as possible when not used — when I checked for lettre it added 2.2KiB in (pre-LTO) binary size and less than a second in compile time.

---

This change is 

[jonhoo]

Sorry for the slow reply!

This seems like a totally reasonable thing to add, though I'd want to add it behind a feature flag. I also think we could do it in an additive way. That is, keep the current Authenticate-based API, and then provide a SASL-authenticate method (e.g., Client::sasl_auth) when the SASL feature is enabled that does auth through SASL. Authenticate may still be useful on its own for users who are working with relatively simple (or experimental) mechanisms, such as during testing.

One thing that surprised me when glancing over the changes was the need for Arc-wrapping the config — can that be avoided? Why is that required by the SASLClient API?

[jonhoo]

Oh, also, we'd want to make sure by the time this lands that we can take a regular crates.io dependency on rsasl rather than a git dependency, so that we can in turn continue to publish imap on crates.io.

[dequbed]

Sorry for the slow reply!

Hey no worries, we're all doing unpaid volunteer work here — I don't mind waiting a few days, your README already states you don't have much time for this project. :)

This seems like a totally reasonable thing to add, though I'd want to add it behind a feature flag. I also think we could do it in an additive way. That is, keep the current Authenticate-based API, and then provide a SASL-authenticate method (e.g., Client::sasl_auth) when the SASL feature is enabled that does auth through SASL. Authenticate may still be useful on its own for users who are working with relatively simple (or experimental) mechanisms, such as during testing.

Sure thing! I'd like to note that I designed rsasl specifically to be extensible so this would still allow experimental / testing mechanisms to be used, just like with Authenticate, without further changes to rsasl or rust-imap being required. The main reason I removed that method was really ~~because I like deleting code~~ since it means less code this library has to worry about and maintain, but I'm very much not set on that.

One thing that surprised me when glancing over the changes was the need for Arc-wrapping the config — can that be avoided? Why is that required by the SASLClient API?

The main reason for this is that SASLConfig is an unsized type, specifically a trait object. Since the config comes from user code and not from either rsasl or libraries using rsasl, having the config be a generic instead of a trait object would quickly lead to a generic explosion as every code path handling SASL would have to be generic over a type they never interact with directly. Arc specifically was chosen because I wanted to have one config for both server- and client-side and especially on the former a global config being used by multiple threads simultaniously is quite likely. So of the typical DST containers Box, Rc and Arc I considered Arc to be the best option, and it is not much more expensive than Box when used only by one thread.

That all being said, I am debating making SASLConfig (and related) generic types in a future version of rsasl, especially now that GATs become an option. However, that's somewhat further in the future and I do think it can be done in a backwards-compatible manner.

Oh, also, we'd want to make sure by the time this lands that we can take a regular crates.io dependency on rsasl rather than a git dependency, so that we can in turn continue to publish imap on crates.io.

Oh yeah of course! I was only using the git dependency as I was trying out a changed API that I did not yet release, as I said I am doing this PR partly to make sure that my API makes sense — and I did in fact find an issue ^^

[dequbed]

I pushed a new version of this PR with the changes you requested so far. There are still a few to-dos I'd like to implement in rust-imap for the rsasl feature, but most of them can be added in a future PR too if we want to get this feature into a release sooner.

Specifically, channel binding and SASL security layers would be nice features to implement, but I'm not happy with how rsasl currently handles the latter (and security layers are rare, and IMO should not be used at all), and I need to write better utilities for the former.

[dequbed]

Hi!

I was linked your stream VoD recently and I noticed that I could have explained myself a bit better, especially since most people won't have deep familarity with SASL, rsasl, and all that surrounds it ^^'

So to answer some questions you asked into the ether in that video:

A first thing; I'm currently working on an updated version of rsasl (hey, just like you with imap 3.0.0-alpha.*! ;p) that is written entirely in Rust and doesn't depend on an external library like the previous version did. So the default docsrs link will not help you with most of the types as this is in essence a full rewrite. The version you'd want to look at is the 2.0.0-rc.* (https://docs.rs/rsasl/2.0.0-rc.3/rsasl/index.html), and its API is exactly what I'm trying to test with these PRs ;)

"Channel Binding" in one sentence is taking some unmistakable information from an underlying secure transport (so e.g. TLS in the common IMAP case) and including it in the authentication exchange in a way that will make the authentication fail if the information doesn't match up. It only works for a limited number of SASL mechanisms but it can help detect some cases of active MitM that has intercepted the TLS stream using e.g. a faked certificate. It's a fringe feature as most servers don't offer the mechanisms required and even then Channel Binding support is somewhat brittle as it doesn't have good negotiation.

SASL authentication using e.g. client certificates or other contextual information is a specific SASL mechanism called EXTERNAL.

Also, the magic method of rsasl is Session::step64, it takes base64 data, does the authentication magic part, and puts base64-encoded data into a provided writer ^^

[jonhoo]

That's helpful, thank you!

[codecov[bot]]

Codecov Report

Merging #243 (09d3ead) into main (41c5597) will decrease coverage by 1.1%. The diff coverage is 32.0%.

Additional details and impacted files

Impacted Files	Coverage Δ
src/error.rs	25.8% <ø> (ø)
src/client.rs	90.9% <27.2%> (-2.3%)	:arrow_down:
src/types/capabilities.rs	55.1% <53.3%> (-0.7%)	:arrow_down:

[dequbed]

EHLO!

With rsasl now being fully released as 2.0.0 I cleaned this PR up and squashed it down somewhat. It also depends on the commit in #255 which we said would potentially be better as an entirely separate PR.

[jonhoo]

Thanks! I'll hold off on reviewing this until #255 lands :+1: