OpenSSL

[uiri]

Close #5

Large refactoring to wrap TcpStreams in SslStreams. Added associated parameters to the config.

I feel like there are probably a lot of idiomatic issues lurking around.

---

This change is 

[indiv0]

Something came up while I was reviewing this PR so I didn't get around to the last file. I'll try to finish the review tonight but I'll publish what I've already reviewed.

---

Reviewed 6 of 7 files at r1. Review status: 6 of 7 files reviewed at latest revision, 8 unresolved discussions.

---

src/server/config.rs, line 20 at r1 (raw file):

    // Host on which to listen
    pub host: String,
    // Plaintext Port on which to listen for LMTP

Incorrect capitalization in a comment? PR rejected! (also occurs below)

---

src/server/config.rs, line 74 at r1 (raw file):

    pub fn get_ssl_keys(&self) -> Result<ParsedPkcs12, PkcsError> {
        let mut buf = vec![];

Usually I do this with Vec:new(), but I've seen both and I think it's just personal preference. You can leave this if you like.

---

src/server/config.rs, line 76 at r1 (raw file):

        let mut buf = vec![];
        match File::open(&self.pkcs_file) {
            Err(e) => Err(PkcsError::Io(e)),

These kinds of match statements can be replaced by implementing From<IoError> on PkcsError. Then you can just do let file = File::open(&self.pkcs_file)?;. The ? operator replace the old try! macro, so in effect that statement will attempt to unwrap the Result of File::open, and if it's an IoError it will use the From<IoError> trait to automatically wrap the IoError in a PkcsError and then return it. We should probably take advantage of where we can because it's idiomatic and concise.

It would let you reduce this whole block to (approximately):

let mut file = File::open(&self.pkcs_file)?;
file.read_to_end(&mut buf)?;
let p = Pkcs12::from_der(&buf)?;
p.parse(&self.pkcs_pass).map_err(PkcsError::from)

---

src/server/config.rs, line 97 at r1 (raw file):

            lmtp_port: 3000,
            imap_port: 10000,
            lmtp_ssl_port: 0,

Is this supposed to be 0?

---

src/server/mod.rs, line 26 at r1 (raw file):

        match *self {
            Stream::Ssl(ref mut s) => s.write(buf),
            Stream::Tcp(ref mut s) => s.write(buf)

I think there should be some way to implement these traits without these duplicated lines. Maybe implementing Deref on Stream instead of these traits would work? Then rust would automatically deref Stream to SslStream or TcpStream as necessary and use their existing trait impls. Then again I believe Deref has to return a concrete type, which would render this method impossible. Hmm.

---

src/server/mod.rs, line 63 at r1 (raw file):

        // Load the user data from the specified user data file.
        let users = load_users(&conf.users)?;
        let ssl_acceptor = if let Ok(identity) = conf.get_ssl_keys() {

This block can also be reduced by implementing the necessary From traits on Error and using ?.

---

src/server/mod.rs, line 90 at r1 (raw file):

            Some(TcpListener::bind((&self.conf.host[..], port)))
        } else {
            None

Is returning None here an error case or acceptable behaviour? If it's an error case, we should probably just return a custom error type here.

---

src/user/session.rs, line 78 at r1 (raw file):

        return_on_err!(stream.flush());
        loop {
            let mut command = String::new();

We probably wanna remove this allocation outside the loop and just clear it as necessary (String::truncate?) to avoid allocating with every line read. Then we could also make this a while let loop, I think.

---

Comments from Reviewable

[indiv0]

Reviewed 1 of 7 files at r1. Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

src/user/session.rs, line 87 at r1 (raw file):

                    }

                    let mut args = command.trim().split(' ');

I've looked around, and it looks like the only reason we don't collect() this iterator immediately is because we call args.next() a few lines down.

Instead we could just do something like:

let mut args = comment.trim().split(' ');
let opt_tag = args.next();
let args = args.collect::<Vec<&str>>();

and then just pass around the Vec<&str> around after that, instead of the Split<char> iterator, which we then collect() in every match arm below.

---

src/user/session.rs, line 98 at r1 (raw file):

                        Some(tag) => {
                            let mut bad_res = tag.to_string();
                            bad_res.push_str(inv_str);

You could make these lines into an inline function (i.e. a function in this funtion). Then just call bad_res(tag, inv_str) when you need to return a bad_res. Will let us avoid another unnecessary allocation on every line read. This might be overeager premature optimization, but it's also an easy fix.

Alternatively, we could make some types (e.g. Response) to help us build responses, rather than having them be "stringly-typed". Probably could even use the builder pattern to specify the tag and content portions separately, and then just build the final response once.

---

src/user/session.rs, line 104 at r1 (raw file):

                                None => bad_res,
                                Some(c) => {
                                    warn!("Cmd: {}", command.trim());

I think that these kinds of general purpose log statements should probably use info!(). I realize you didn't add this line in this PR, but I've noticed this before and everything we log being "WARN" seems silly to me.

---

src/user/session.rs, line 107 at r1 (raw file):

                                    match &c.to_ascii_lowercase()[..] {
                                        // STARTTLS is handled here because it modifies the stream
                                        "starttls" => {

I'm not a fan of the fact that we're doing some manual (if simple) command parsing here to check if this is a STARTTLS command, then doing more interpretation later. I'd rather parse the entire command into a rust struct and then work with that afterwards.

That said, you can leave this as-is and I'll add the parsing for starttls commands later and re-organize this method as necessary.

---

src/user/session.rs, line 135 at r1 (raw file):

                    if starttls {
                        if let Ok(Stream::Tcp(tcp_stream)) = stream.into_inner() {

If you change the handle function to return -> Result<(), ImapError>, then you can also get rid of these blocks with the From/? method mentioned earlier.

In fact, it might be a good idea to open an issue and get rid of this everywhere possible in the codebase.

---

src/user/session.rs, line 167 at r1 (raw file):

                let mut res = tag.to_string();
                res.push_str(" OK NOOP\r\n");
                res

This can be one-lined as let res = format!("{} OK NOOP\r\n", tag);, or (preferably) as

let mut res = tag.to_string();
res += " OK NOOP\r\n";

Because rust supports string concatenation as long as it's String + &str.

That said, this is another reason to have a proper response builder.

---

Comments from Reviewable

[uiri]

Review status: all files reviewed at latest revision, 14 unresolved discussions.

---

src/server/config.rs, line 20 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

Incorrect capitalization in a comment? PR rejected! (also occurs below)

Done.

---

src/server/config.rs, line 76 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

These kinds of match statements can be replaced by implementing `From` on `PkcsError`. Then you can just do `let file = File::open(&self.pkcs_file)?;`. The `?` operator replace the old `try!` macro, so in effect that statement will attempt to unwrap the `Result` of `File::open`, and if it's an `IoError` it will use the `From` trait to automatically wrap the `IoError` in a `PkcsError` and then return it. We should probably take advantage of where we can because it's idiomatic and concise. It would let you reduce this whole block to (approximately): ```rs let mut file = File::open(&self.pkcs_file)?; file.read_to_end(&mut buf)?; let p = Pkcs12::from_der(&buf)?; p.parse(&self.pkcs_pass).map_err(PkcsError::from) ```

Done. Added more of the SslAcceptor logic.

---

src/server/config.rs, line 97 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

Is this supposed to be `0`?

Yep. I'm using 0 to disable that particular service listening on a port.

---

src/server/mod.rs, line 26 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

I think there *should* be some way to implement these traits without these duplicated lines. Maybe implementing `Deref` on `Stream` instead of these traits would work? Then rust would automatically deref `Stream` to `SslStream` or `TcpStream` as necessary and use their existing trait impls. Then again I believe `Deref` has to return a concrete type, which would render this method impossible. Hmm.

I took this basically verbatim from the rust-openssl source since their own MaybeSslStream is no longer pub for some reason.

---

src/server/mod.rs, line 63 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

This block can also be reduced by implementing the necessary `From` traits on `Error` and using `?`.

Totally refactored this section into config. Changed get_ssl_keys to get_ssl_acceptor.

---

src/server/mod.rs, line 90 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

Is returning `None` here an error case or acceptable behaviour? If it's an error case, we should probably just return a custom error type here.

This is not an error which is why I went through the trouble of wrapping it up in an Option, as awkward as that may be. None signifies that the service is disabled and not listening on a port.

---

src/user/session.rs, line 78 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

We probably wanna remove this allocation outside the loop and just clear it as necessary (`String::truncate`?) to avoid allocating with every line read. Then we could also make this a `while let` loop, I think.

Done.

---

src/user/session.rs, line 87 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

I've looked around, and it looks like the only reason we don't `collect()` this iterator immediately is because we call `args.next()` a few lines down. Instead we could just do something like: ```rs let mut args = comment.trim().split(' '); let opt_tag = args.next(); let args = args.collect::>(); ``` and then just pass around the `Vec<&str>` around after that, instead of the `Split` iterator, which we then `collect()` in every match arm below.

The issue is that we also need to call args.next() for UID commands. If that were not the case, I would agree with you. I considered passing a Vec<&str> but settled on this as it was a simpler fix for the issue at hand.

---

src/user/session.rs, line 98 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

You could make these lines into an inline function (i.e. a function in this funtion). Then just call `bad_res(tag, inv_str)` when you need to return a `bad_res`. Will let us avoid another unnecessary allocation on every line read. This might be overeager premature optimization, but it's also an easy fix. Alternatively, we could make some types (e.g. `Response`) to help us build responses, rather than having them be "stringly-typed". Probably could even use the builder pattern to specify the `tag` and content portions separately, and then just build the final response once.

I'm applying the same += syntactic sugar as done below for noop. Tag length may vary, and the tag goes at the front of the buffer, so avoiding an allocation on each command here seems difficult.

I'm against creating a typedef for Response to String. I think anything else is overkill - the IMAP protocol is basically built up off of the strings "OK", "NO" and "BAD". I think the data types should flow from the needs of the protocol.

---

src/user/session.rs, line 104 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

I think that these kinds of general purpose log statements should probably use `info!()`. I realize you didn't add this line in this PR, but I've noticed this before and everything we log being "WARN" seems silly to me.

It is silly for software which is production ready... which this is no where near. It is still on version 0.0.1 for a reason :wink:

---

src/user/session.rs, line 107 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

I'm not a fan of the fact that we're doing some manual (if simple) command parsing here to check if this is a STARTTLS command, then doing more interpretation later. I'd rather parse the entire command into a rust struct and then work with that afterwards. That said, you can leave this as-is and I'll add the parsing for starttls commands later and re-organize this method as necessary.

For the love of all that is holy... please no. I realize that this code is now an ugly hacky mess instead of the clean loop we had before but I think that that also describes bolting TLS onto a plaintext protocol in the manner of STARTTLS.

interpret is now basically one gigantic match on cmd. The only reason that this branch of the match isn't inside of interpret is that I determined this partial rewrite of the logic was cleaner than changing the return type of interpret so that a boolean could be passed out.

The key issue is that the response to STARTTLS must be sent before the TLS handshake can take place since otherwise the client is still waiting on a response to its request.

---

src/user/session.rs, line 135 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

If you change the `handle` function to return `-> Result<(), ImapError>`, then you can also get rid of these blocks with the `From/?` method mentioned earlier. In fact, it might be a good idea to open an issue and get rid of this everywhere possible in the codebase.

When handle returns, the child session thread dies. There's nothing outside of it to grab and handle an error. I elected to instead move this inside of Server::starttls since the logic can be represented there more simply.

---

src/user/session.rs, line 167 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

This can be one-lined as `let res = format!("{} OK NOOP\r\n", tag);`, or (preferably) as ```rs let mut res = tag.to_string(); res += " OK NOOP\r\n"; ``` Because rust supports string concatenation as long as it's `String + &str`. That said, this is another reason to have a proper response builder.

Done.

---

Comments from Reviewable

[indiv0]

Reviewed 3 of 3 files at r2. Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

src/server/config.rs, line 97 at r1 (raw file):

Previously, uiri (Will Pearson) wrote…

Yep. I'm using 0 to disable that particular service listening on a port.

This seems like this should be an Option<usize> then, I think. Otherwise users are forced to include it in their TOML file to explicitly disable the service. If it was Option<usize>, None could also be used to represent the "disabled" case (or you could continue using 0).

---

src/server/mod.rs, line 63 at r1 (raw file):

Previously, uiri (Will Pearson) wrote…

Totally refactored this section into `config`. Changed `get_ssl_keys` to `get_ssl_acceptor`.

Looks good. You can simplify the remaining line down to let ssl_acceptor = conf.get_ssl_acceptor().ok();

---

src/server/mod.rs, line 131 at r2 (raw file):

        if let Ok(Stream::Tcp(stream)) = inner_stream {
            if let Some(ref ssl_acceptor) = self.ssl_acceptor {
                return Some(ssl_acceptor.accept(stream).unwrap());

Is there a way to avoid this unwrap() here? Or are we supposed to panic in this case?

---

src/user/session.rs, line 98 at r1 (raw file):

Previously, uiri (Will Pearson) wrote…

I'm applying the same `+=` syntactic sugar as done below for `noop`. Tag length may vary, and the tag goes at the front of the buffer, so avoiding an allocation on each command here seems difficult. I'm against creating a typedef for `Response` to `String`. I think anything else is overkill - the IMAP protocol is basically built up off of the strings "OK", "NO" and "BAD". I think the data types should flow from the needs of the protocol.

OK. I had more of a proper response builder type in mind rather than a typedef, but if it's really that simple then yeah we can stick with strings.

---

src/user/session.rs, line 104 at r1 (raw file):

Previously, uiri (Will Pearson) wrote…

It is silly for software which is production ready... which this is no where near. It is still on version 0.0.1 for a reason :wink:

OK, fair enough. I'm going to open an issue about it so I don't forget though.

---

src/user/session.rs, line 107 at r1 (raw file):

Previously, uiri (Will Pearson) wrote…

For the love of all that is holy... please no. I realize that this code is now an ugly hacky mess instead of the clean loop we had before but I think that that also describes bolting TLS onto a plaintext protocol in the manner of STARTTLS. `interpret` is now basically one gigantic match on `cmd`. The only reason that this branch of the `match` isn't inside of `interpret` is that I determined this partial rewrite of the logic was cleaner than changing the return type of `interpret` so that a boolean could be passed out. The key issue is that the response to STARTTLS must be sent _before_ the TLS handshake can take place since otherwise the client is still waiting on a response to its request.

OK, that makes sense then and I completely agree with you.

---

Comments from Reviewable

[uiri]

Review status: 5 of 7 files reviewed at latest revision, 3 unresolved discussions.

---

src/server/config.rs, line 97 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

This seems like this should be an `Option` then, I think. Otherwise users are forced to include it in their TOML file to explicitly disable the service. If it was `Option`, `None` could also be used to represent the "disabled" case (or you could continue using `0`).

Done.

---

src/server/mod.rs, line 63 at r1 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

Looks good. You can simplify the remaining line down to `let ssl_acceptor = conf.get_ssl_acceptor().ok();`

Done.

---

src/server/mod.rs, line 131 at r2 (raw file):

Previously, indiv0 (Nikita Pekin) wrote…

Is there a way to avoid this `unwrap()` here? Or are we supposed to panic in this case?

Done. The unwrap() in imap_ssl a few lines above this is unavoidable because stream gets borrowed only to be used later if it fails.

---

Comments from Reviewable

[indiv0]

---

Reviewed 2 of 2 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable