A significant clean-up of the server authentication phase, with a basic test.

The prior implementation involved some (moderately awkward) layered
`match` expressions; this version skips those in favor of `?`. The cost
is a little less detail about (for example) when serialization errors
happen. On the bright side, it gains us some huge improvements in code
clarity. We might be able to get back the tracing later, with other
library support.

In addition, we know have a couple tests: one to make sure we choose the
authentication method appropriately, and a very basic handshake test for
when we're not actually negotiation a username and password.

src/client.rs

@@ -1,11 +1,11 @@
 use crate::errors::{DeserializationError, SerializationError};
 use crate::messages::{
-    AuthenticationMethod, ClientConnectionCommand, ClientConnectionRequest, ClientGreeting,
+    AuthenticationMethod, ClientGreeting, ClientUsernamePassword, ServerAuthResponse, ServerChoice,
-    ClientUsernamePassword, ServerAuthResponse, ServerChoice, ServerResponse, ServerResponseStatus,
+    ServerResponseStatus,
 };
 use crate::network::generic::Networklike;
 use futures::io::{AsyncRead, AsyncWrite};
-use log::{warn, trace};
+use log::{trace, warn};
 use thiserror::Error;
 
 #[derive(Debug, Error)]
@@ -30,13 +30,28 @@ where
     N: Networklike,
 {
     _network: N,
-    stream: S,
+    _stream: S,
 }
 
 pub struct LoginInfo {
     pub username_password: Option<UsernamePassword>,
 }
 
+impl LoginInfo {
+    /// Turn this information into a list of authentication methods that we can handle,
+    /// to send to the server. The RFC isn't super clear if the order of these matters
+    /// at all, but we'll try to keep it in our preferred order.
+    fn acceptable_methods(&self) -> Vec<AuthenticationMethod> {
+        let mut acceptable_methods = vec![AuthenticationMethod::None];
+
+        if self.username_password.is_some() {
+            acceptable_methods.push(AuthenticationMethod::UsernameAndPassword);
+        }
+
+        acceptable_methods
+    }
+}
+
 pub struct UsernamePassword {
     username: String,
     password: String,
@@ -50,23 +65,18 @@ where
     /// Create a new SOCKSv5 client connection over the given steam, using the given
     /// authentication information.
     pub async fn new(_network: N, mut stream: S, login: &LoginInfo) -> Result<Self, SOCKSv5Error> {
-        let mut acceptable_methods = vec![AuthenticationMethod::None];
+        let acceptable_methods = login.acceptable_methods();
-
+        trace!(
-        if login.username_password.is_some() {
-            acceptable_methods.push(AuthenticationMethod::UsernameAndPassword);
-        }
-
-        println!(
             "Computed acceptable methods -- {:?} -- sending client greeting.",
             acceptable_methods
         );
-        let client_greeting = ClientGreeting { acceptable_methods };
 
+        let client_greeting = ClientGreeting { acceptable_methods };
         client_greeting.write(&mut stream).await?;
         trace!("Write client greeting, waiting for server's choice.");
         let server_choice = ServerChoice::read(&mut stream).await?;
-
         trace!("Received server's choice: {}", server_choice.chosen_method);
+
         match server_choice.chosen_method {
             AuthenticationMethod::None => {}
 
@@ -75,7 +85,7 @@ where
                     trace!("Server requested username/password, getting data from login info.");
                     (linfo.username.clone(), linfo.password.clone())
                 } else {
-                    warn!("Server requested username/password, but we weren't provided one.");
+                    warn!("Server requested username/password, but we weren't provided one. Very weird.");
                     ("".to_string(), "".to_string())
                 };
 
@@ -99,6 +109,9 @@ where
         }
 
         trace!("Returning new SOCKSv5Client object!");
-        Ok(SOCKSv5Client { _network, stream })
+        Ok(SOCKSv5Client {
+            _network,
+            _stream: stream,
+        })
     }
 }

src/errors.rs

@@ -2,6 +2,8 @@ use std::io;
 use std::string::FromUtf8Error;
 use thiserror::Error;
 
+use crate::network::SOCKSv5Address;
+
 /// All the errors that can pop up when trying to turn raw bytes into SOCKSv5
 /// messages.
 #[derive(Error, Debug)]
@@ -164,3 +166,23 @@ impl PartialEq for AuthenticationDeserializationError {
         }
     }
 }
+
+/// The errors that can happen, as a server, when we're negotiating the start
+/// of a SOCKS session.
+#[derive(Debug, Error)]
+pub enum AuthenticationError {
+    #[error("Firewall disallowed connection from {0}:{1}")]
+    FirewallRejected(SOCKSv5Address, u16),
+    #[error("Could not agree on an authentication method with the client")]
+    ItsNotUsItsYou,
+    #[error("Failure in serializing response message: {0}")]
+    SerializationError(#[from] SerializationError),
+    #[error("Failed TLS handshake")]
+    FailedTLSHandshake,
+    #[error("IO error writing response message: {0}")]
+    IOError(#[from] io::Error),
+    #[error("Failure in reading client message: {0}")]
+    DeserializationError(#[from] DeserializationError),
+    #[error("Username/password check failed (username was {0})")]
+    FailedUsernamePassword(String),
+}

src/lib.rs

@@ -4,3 +4,38 @@ pub mod messages;
 pub mod network;
 mod serialize;
 pub mod server;
+
+#[cfg(test)]
+mod test {
+    use crate::client::{LoginInfo, SOCKSv5Client};
+    use crate::network::generic::Networklike;
+    use crate::network::testing::TestingStack;
+    use crate::server::{SOCKSv5Server, SecurityParameters};
+    use async_std::task;
+
+    #[test]
+    fn unrestricted_login() {
+        task::block_on(async {
+            let mut network_stack = TestingStack::default();
+
+            // generate the server
+            let security_parameters = SecurityParameters::unrestricted();
+            let default_port = network_stack.listen("localhost", 9999).await.unwrap();
+            let server =
+                SOCKSv5Server::new(network_stack.clone(), security_parameters, default_port);
+
+            let _server_task = task::spawn(async move { server.run().await });
+
+            let stream = network_stack.connect("localhost", 9999).await.unwrap();
+            let login_info = LoginInfo {
+                username_password: None,
+            };
+            let client = SOCKSv5Client::new(network_stack, stream, &login_info).await;
+
+            if let Err(e) = &client {
+                println!("client result: {:?}", e);
+            }
+            assert!(client.is_ok());
+        })
+    }
+}

src/messages/server_auth_response.rs

@@ -14,6 +14,14 @@ pub struct ServerAuthResponse {
 }
 
 impl ServerAuthResponse {
+    pub fn success() -> ServerAuthResponse {
+        ServerAuthResponse { success: true }
+    }
+
+    pub fn failure() -> ServerAuthResponse {
+        ServerAuthResponse { success: false }
+    }
+
     pub async fn read<R: AsyncRead + Send + Unpin>(
         r: &mut R,
     ) -> Result<Self, DeserializationError> {

src/messages/server_choice.rs

@@ -17,6 +17,18 @@ pub struct ServerChoice {
 }
 
 impl ServerChoice {
+    pub fn rejection() -> ServerChoice {
+        ServerChoice {
+            chosen_method: AuthenticationMethod::NoAcceptableMethods,
+        }
+    }
+
+    pub fn option(method: AuthenticationMethod) -> ServerChoice {
+        ServerChoice {
+            chosen_method: method,
+        }
+    }
+
     pub async fn read<R: AsyncRead + Send + Unpin>(
         r: &mut R,
     ) -> Result<Self, DeserializationError> {

src/network.rs

@@ -6,9 +6,5 @@ pub mod standard;
 pub mod stream;
 pub mod testing;
 
-use crate::messages::ServerResponseStatus;
 pub use crate::network::address::SOCKSv5Address;
 pub use crate::network::standard::Builtin;
-use async_trait::async_trait;
-use futures::{AsyncRead, AsyncWrite};
-use std::fmt;

src/server.rs

@@ -1,7 +1,7 @@
-use crate::errors::{DeserializationError, SerializationError};
+use crate::errors::{AuthenticationError, DeserializationError, SerializationError};
 use crate::messages::{
     AuthenticationMethod, ClientConnectionCommand, ClientConnectionRequest, ClientGreeting,
-    ClientUsernamePassword, ServerChoice, ServerResponse, ServerResponseStatus,
+    ClientUsernamePassword, ServerAuthResponse, ServerChoice, ServerResponse, ServerResponseStatus,
 };
 use crate::network::address::HasLocalAddress;
 use crate::network::generic::Networklike;
@@ -29,6 +29,21 @@ pub struct SecurityParameters {
     pub connect_tls: Option<fn(GenericStream) -> Option<GenericStream>>,
 }
 
+impl SecurityParameters {
+    /// Generates a `SecurityParameters` object that does not, in any way,
+    /// restrict who can log in. It also will not induce any transition into
+    /// TLS. Use this at your own risk ... or, really, just don't use this,
+    /// ever, and certainly not in production.
+    pub fn unrestricted() -> SecurityParameters {
+        SecurityParameters {
+            allow_unauthenticated: true,
+            allow_connection: None,
+            check_password: None,
+            connect_tls: None,
+        }
+    }
+}
+
 impl<N: Networklike + Send + 'static> SOCKSv5Server<N> {
     pub fn new<S: Listenerlike<Error = N::Error> + 'static>(
         network: N,
@@ -70,108 +85,232 @@ impl<N: Networklike + Send + 'static> SOCKSv5Server<N> {
             let params = self.security_parameters.clone();
             let network_mutex_copy = locked_network.clone();
             task::spawn(async move {
-                if let Some(authed_stream) =
+                match run_authentication(params, stream, their_addr.clone(), their_port).await {
-                    run_authentication(params, stream, their_addr.clone(), their_port).await
+                    Ok(authed_stream) => {
-                {
+                        match run_main_loop(network_mutex_copy, authed_stream).await {
-                    if let Err(e) = run_main_loop(network_mutex_copy, authed_stream).await {
+                            Ok(_) => {}
-                        warn!("Failure in main loop: {}", e);
+                            Err(e) => warn!("Failure in main loop: {}", e),
+                        }
                     }
+                    Err(e) => warn!(
+                        "Failure running authentication from {}:{}: {}",
+                        their_addr, their_port, e
+                    ),
                 }
             });
         }
     }
 }
 
+enum ChosenMethod {
+    TLS(fn(GenericStream) -> Option<GenericStream>),
+    Password(fn(&str, &str) -> bool),
+    None,
+}
+
+impl From<ChosenMethod> for AuthenticationMethod {
+    fn from(x: ChosenMethod) -> Self {
+        match x {
+            ChosenMethod::TLS(_) => AuthenticationMethod::SSL,
+            ChosenMethod::Password(_) => AuthenticationMethod::UsernameAndPassword,
+            ChosenMethod::None => AuthenticationMethod::None,
+        }
+    }
+}
+
+// This is an opinionated function that tries to pick the most security-advantageous
+// authentication method that we can handle and our peer will be willing to accept.
+// If we find one we like, we return it. If we can't, we return `None`.
+fn choose_authentication_method(
+    params: &SecurityParameters,
+    client_suggestions: &[AuthenticationMethod],
+) -> Option<ChosenMethod> {
+    // First: everything is better with encryption. So if they offer it, and we can
+    // support it, we choose TLS.
+    if client_suggestions.contains(&AuthenticationMethod::SSL) {
+        if let Some(converter) = params.connect_tls {
+            return Some(ChosenMethod::TLS(converter));
+        }
+    }
+
+    // If they've got a username and password to give us, and we've got something
+    // that will check them, then let's use that.
+    if client_suggestions.contains(&AuthenticationMethod::UsernameAndPassword) {
+        if let Some(matcher) = params.check_password {
+            return Some(ChosenMethod::Password(matcher));
+        }
+    }
+
+    // Meh. OK, if we're both cool with an unauthenticated session, I guess we can
+    // do that.
+    if client_suggestions.contains(&AuthenticationMethod::None) && params.allow_unauthenticated {
+        return Some(ChosenMethod::None);
+    }
+
+    // if we get all the way here, there was nothing for us to settle on, so we
+    // give up.
+    None
+}
+
+#[test]
+fn reasonable_auth_method_choices() {
+    let mut params = SecurityParameters::unrestricted();
+    let mut client_suggestions = Vec::new();
+
+    // if the client's a jerk and send us nothing, we should get nothing, no matter what.
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        None
+    );
+    // but if they send us none, then we're cool with that with the unrestricted item.
+    client_suggestions.push(AuthenticationMethod::None);
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        Some(AuthenticationMethod::None)
+    );
+    // of course, if we set ourselves back to not allowing randos ... which we should do ...
+    // then we should get none again, even if the client's OK with it.
+    params.allow_unauthenticated = false;
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        None
+    );
+
+    // Even if we allow unauthenticated sessions, though, we'll take a username and password
+    // if someone will give them to us.
+    params.allow_unauthenticated = true;
+    params.check_password = Some(|_, _| true);
+    client_suggestions.push(AuthenticationMethod::UsernameAndPassword);
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        Some(AuthenticationMethod::UsernameAndPassword)
+    );
+    // which shouldn't matter if we turn off unauthenticated connections
+    params.allow_unauthenticated = false;
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        Some(AuthenticationMethod::UsernameAndPassword)
+    );
+    // ... or whether we don't offer None
+    client_suggestions.remove(0);
+    // That being said, if we don't have a way to check a password, we're hooped
+    params.check_password = None;
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        None
+    );
+    // Or, come to think of it, if we have a way to check a password, but they don't offer it up
+    params.check_password = Some(|_, _| true);
+    client_suggestions[0] = AuthenticationMethod::None;
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        None
+    );
+
+    // OK, cool. If we have a TLS handler, that shouldn't actually make a difference.
+    params.connect_tls = Some(|_| unimplemented!());
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        None
+    );
+    // or if they suggest it, but we don't have one: same deal
+    params.connect_tls = None;
+    client_suggestions[0] = AuthenticationMethod::SSL;
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        None
+    );
+    // but if we have a handler, and they go for it, we use it.
+    params.connect_tls = Some(|_| unimplemented!());
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        Some(AuthenticationMethod::SSL)
+    );
+    // even if the client's cool with passwords and we can handle it
+    params.check_password = Some(|_, _| true);
+    client_suggestions.push(AuthenticationMethod::UsernameAndPassword);
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        Some(AuthenticationMethod::SSL)
+    );
+    // even if they offer nothing at all and we're cool with it.
+    params.allow_unauthenticated = true;
+    client_suggestions.push(AuthenticationMethod::None);
+    assert_eq!(
+        choose_authentication_method(&params, &client_suggestions).map(AuthenticationMethod::from),
+        Some(AuthenticationMethod::SSL)
+    );
+}
+
 async fn run_authentication(
     params: SecurityParameters,
     mut stream: GenericStream,
     addr: SOCKSv5Address,
     port: u16,
-) -> Option<GenericStream> {
+) -> Result<GenericStream, AuthenticationError> {
-    match ClientGreeting::read(&mut stream).await {
+    // before we do anything at all, we check to see if we just want to blindly reject
-        Err(e) => {
+    // this connection, utterly and completely.
-            error!(
+    if let Some(firewall_allows) = params.allow_connection {
-                "Client hello deserialization error from {}:{}:  {}",
+        if !firewall_allows(&addr, port) {
-                addr, port, e
+            return Err(AuthenticationError::FirewallRejected(addr, port));
-            );
+        }
-            None
+    }
+
+    // OK, I guess we'll listen to you
+    let greeting = ClientGreeting::read(&mut stream).await?;
+
+    match choose_authentication_method(&params, &greeting.acceptable_methods) {
+        // it's not us, it's you
+        None => {
+            trace!("Failed to find acceptable authentication method.");
+            let rejection_letter = ServerChoice::rejection();
+
+            rejection_letter.write(&mut stream).await?;
+            stream.flush().await?;
+
+            Err(AuthenticationError::ItsNotUsItsYou)
         }
 
-        // So we get opinionated here, based on what we think should be our first choice if the
+        // the gold standard. great choice.
-        // server offers something up. So we'll first see if we can make this a TLS connection.
+        Some(ChosenMethod::TLS(converter)) => {
-        Ok(cg)
+            trace!("Choosing TLS for authentication.");
-            if cg.acceptable_methods.contains(&AuthenticationMethod::SSL)
+            let lets_do_this = ServerChoice::option(AuthenticationMethod::SSL);
-                && params.connect_tls.is_some() =>
+            lets_do_this.write(&mut stream).await?;
-        {
+            stream.flush().await?;
-            match params.connect_tls {
+
-                None => {
+            converter(stream).ok_or(AuthenticationError::FailedTLSHandshake)
-                    error!("Internal error: TLS handler was there, but is now gone");
+        }
-                    None
+
-                }
+        // well, I guess this is something?
-                Some(converter) => match converter(stream) {
+        Some(ChosenMethod::Password(checker)) => {
-                    None => {
+            trace!("Choosing Username/Password for authentication.");
-                        info!("Rejecting bad TLS handshake from {}:{}", addr, port);
+            let ok_lets_do_password =
-                        None
+                ServerChoice::option(AuthenticationMethod::UsernameAndPassword);
-                    }
+            ok_lets_do_password.write(&mut stream).await?;
-                    Some(new_stream) => Some(new_stream),
+            stream.flush().await?;
-                },
+
+            let their_info = ClientUsernamePassword::read(&mut stream).await?;
+            if checker(&their_info.username, &their_info.password) {
+                let its_all_good = ServerAuthResponse::success();
+                its_all_good.write(&mut stream).await?;
+                stream.flush().await?;
+                Ok(stream)
+            } else {
+                let yeah_no = ServerAuthResponse::failure();
+                yeah_no.write(&mut stream).await?;
+                stream.flush().await?;
+                Err(AuthenticationError::FailedUsernamePassword(
+                    their_info.username,
+                ))
             }
         }
 
-        // if we can't do that, we'll see if we can get a username and password
+        Some(ChosenMethod::None) => {
-        Ok(cg)
+            trace!("Just skipping the whole authentication thing.");
-            if cg
+            let nothin_i_guess = ServerChoice::option(AuthenticationMethod::None);
-                .acceptable_methods
+            nothin_i_guess.write(&mut stream).await?;
-                .contains(&AuthenticationMethod::UsernameAndPassword)
+            stream.flush().await?;
-                && params.check_password.is_some() =>
+            Ok(stream)
-        {
-            match ClientUsernamePassword::read(&mut stream).await {
-                Err(e) => {
-                    warn!(
-                        "Error reading username/password from {}:{}: {}",
-                        addr, port, e
-                    );
-                    None
-                }
-                Ok(userinfo) => {
-                    let checker = params.check_password.unwrap_or(|_, _| false);
-                    if checker(&userinfo.username, &userinfo.password) {
-                        Some(stream)
-                    } else {
-                        None
-                    }
-                }
-            }
-        }
-
-        // and, in the worst case, we'll see if our user is cool with unauthenticated connections
-        Ok(cg)
-            if cg.acceptable_methods.contains(&AuthenticationMethod::None)
-                && params.allow_unauthenticated =>
-        {
-            Some(stream)
-        }
-
-        Ok(_) => {
-            let rejection_letter = ServerChoice {
-                chosen_method: AuthenticationMethod::NoAcceptableMethods,
-            };
-
-            if let Err(e) = rejection_letter.write(&mut stream).await {
-                warn!(
-                    "Error sending rejection letter in authentication response: {}",
-                    e
-                );
-            }
-
-            if let Err(e) = stream.flush().await {
-                warn!(
-                    "Error flushing buffer after rejection latter in authentication response: {}",
-                    e
-                );
-            }
-
-            None
         }
     }
 }