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.

[jonhoo]

I generally like this a lot! Left another few notes.

[jonhoo]

I wonder if we can pull out some of the code here into a helper function that can be used both by the existing authenticate and by the new SASL functionality. After all, they're doing mostly the same thing.

[jonhoo]

It's a little weird to me to take &Mechname here, for a couple of reasons.

First, it seems like rsals actually wants &[Mechname], so shouldn't we take that?
Second, if it's possible to not provide an initial mechanism, should we make that possible?
And third, if Mechname is Copy (which its name makes it sound like it is), why the &?

[dequbed]

Yeah this was actually something I wanted to ask about but this time it was me who responded late ^^'

So the way rsasl is designed is that it tries to limit the required interactions with an user. So the idea is an user provides a protocol crate with a SASLConfig (potentially as early as in a Client builder, long before a connection is established!) and then doesn't have to care about authentication at all anymore, with a protocl crate being in essence able to do every part of an authentication exchange using that config and not needing to involve the user again.

So in this specific situation the way rsasl would like authentication to be handled is by the imap crate knowing the list of mechanisms supported by the server and then calling SASLClient::start_suggested with that list. Selection of the preferred mechanism is then done by the config based on how the user configured it.

However with imap this presents the problem that a client has to request that list of available mechanisms via CAPABILITY, and your crate doesn't do that by itself.

I see a few solutions present themselves:

• Make the Client be able to cache capabilities returned by the server and a call to sasl_auth does a CAPABILITY if required. The upside is that an user doesn't have to worry about details, the downside with that is that the Client allocates outside of the users control.
• The second option is for sasl_auth to take a &Capabilities and have it figure out the rest from there. That is still a rather manual process but I means the user is in full control of allocation of that data and most importantly can deallocate that data easily when it's no longer required.
• The third option is your idea of having sasl_auth take a &[&Mechname]. This is definitely the most manual approach, but also one of the easiest for the imap crate.

My main gripe with the last option is that I'm currently working on allowing SASLClient::start_suggested to take an Iterator instead of a slice, meaning allocations can be again reduced. However of course that could just translate to sasl_auth too, by it taking an impl Iterator<Item=&Mechname> ^^

Regarding your third point, Mechname isn't Copy. It's a newtype wrapper around [u8] (i.e. it's defined as struct Mechname([u8]);) so it's an unsized type and can't be passed by value. Its main purpose is to allow me to enforce validity checking on mechanism names using the type system, meaning I am able to assume a few properties to hold.

[jonhoo]

I think it's actually entirely reasonable for Client to start auto-issuing a call to gather the server's capabilities at the start of every session and storing the result. I'd be happy to land that, though I think we should land it in a separate PR maybe? And then you could make use of those stored capabilities here and get rid of the &[Mechname] argument entirely! Or, I suppose, we could make it an Option<&[Mechname]> so that users can override if they want, but can also say "no, just choose based on server capabilities".

[jonhoo]

How do you feel about also adding #[deprecated] to fn authenticate and point people at this instead?

[dequbed]

I think that's a good idea ^^

[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]

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%)	⬇️
src/types/capabilities.rs	55.1% <53.3%> (-0.7%)	⬇️

[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 👍