From 1f3ace9434dccdff96e64c0412f9248558b6efbb Mon Sep 17 00:00:00 2001 From: Titouan Rigoudy Date: Fri, 22 Jan 2021 19:40:30 +0100 Subject: [PATCH] Move password check out of LoginRequest. --- src/client.rs | 20 ++++++--- src/proto/core/value.rs | 3 ++ src/proto/packet.rs | 2 +- src/proto/server/client.rs | 33 ++++++-------- src/proto/server/credentials.rs | 72 +++++++++++++++++++++++++++++++ src/proto/server/mod.rs | 1 + src/proto/server/request.rs | 76 ++++++--------------------------- 7 files changed, 116 insertions(+), 91 deletions(-) diff --git a/src/client.rs b/src/client.rs index 8ecf14a..d531a62 100644 --- a/src/client.rs +++ b/src/client.rs @@ -94,14 +94,20 @@ impl Client { /// Runs the client, potentially forever. pub fn run(&mut self) { info!("Logging in..."); + + let credentials = server::Credentials::new( + config::USERNAME.to_string(), + config::PASSWORD.to_string(), + ) + .unwrap(); + + let version = server::Version { + major: config::VER_MAJOR, + minor: config::VER_MINOR, + }; + self.send_to_server(server::ServerRequest::LoginRequest( - server::LoginRequest::new( - config::USERNAME, - config::PASSWORD, - config::VER_MAJOR, - config::VER_MINOR, - ) - .unwrap(), + credentials.into_login_request(version), )); self.login_status = LoginStatus::AwaitingResponse; diff --git a/src/proto/core/value.rs b/src/proto/core/value.rs index 49fb67a..772ddc1 100644 --- a/src/proto/core/value.rs +++ b/src/proto/core/value.rs @@ -15,6 +15,7 @@ //! //! See the `frame` module for the codec applied to byte streams. +use std::error; use std::io; use std::net; @@ -36,6 +37,8 @@ pub trait Encode { fn encode(&mut self, value: T) -> io::Result<()>; } +pub trait ComparableError: error::Error + PartialEq {} + // TODO: Add backtrace fields to each enum variant once std::backtrace is // stabilized. #[derive(PartialEq, Error, Debug)] diff --git a/src/proto/packet.rs b/src/proto/packet.rs index fdd194f..d4510e4 100644 --- a/src/proto/packet.rs +++ b/src/proto/packet.rs @@ -82,7 +82,7 @@ impl MutPacket { /// Provides the main way to write data into a binary packet. pub fn write_value(&mut self, val: &T) -> io::Result<()> where - T: WriteToPacket, + T: WriteToPacket + ?Sized, { val.write_to_packet(self) } diff --git a/src/proto/server/client.rs b/src/proto/server/client.rs index 272fad8..2bde218 100644 --- a/src/proto/server/client.rs +++ b/src/proto/server/client.rs @@ -9,7 +9,7 @@ use tokio::net; use crate::proto::core::frame::FrameStream; use crate::proto::server::{ - LoginRequest, LoginResponse, ServerRequest, ServerResponse, + Credentials, LoginResponse, ServerRequest, ServerResponse, }; /// Specifies a protocol version. @@ -32,8 +32,7 @@ impl Default for Version { /// Specifies options for a new `Client`. pub struct ClientOptions { - pub user_name: String, - pub password: String, + pub credentials: Credentials, pub version: Version, } @@ -45,9 +44,6 @@ pub struct Client { /// An error that arose while logging in to a remote server. #[derive(Debug, Error)] pub enum ClientLoginError { - #[error("invalid credentials: {0}")] - InvalidCredentials(String), - #[error("login failed: {0}")] LoginFailed(String), @@ -83,7 +79,7 @@ impl Client { frame_stream: FrameStream::new(tcp_stream), }; - client.handshake(&options).await?; + client.handshake(options).await?; Ok(client) } @@ -92,17 +88,11 @@ impl Client { // Called this way because `login` is already taken. async fn handshake( &mut self, - options: &ClientOptions, + options: ClientOptions, ) -> Result<(), ClientLoginError> { - let login_request = LoginRequest::new( - &options.user_name, - &options.password, - options.version.major, - options.version.minor, - ) - .map_err(|err| ClientLoginError::InvalidCredentials(err.to_string()))?; - - let request = ServerRequest::LoginRequest(login_request); + let request = ServerRequest::LoginRequest( + options.credentials.into_login_request(options.version), + ); self.frame_stream.write(&request).await?; let response = self.frame_stream.read().await?; @@ -188,7 +178,7 @@ mod tests { use tokio::net; use crate::proto::server::testing::fake_server; - use crate::proto::server::*; + use crate::proto::server::{Credentials, ServerRequest, UserStatusRequest}; use super::{Client, ClientOptions, Version}; @@ -206,9 +196,12 @@ mod tests { let stream = net::TcpStream::connect(handle.address()).await.unwrap(); + let user_name = "alice".to_string(); + let password = "sekrit".to_string(); + let credentials = Credentials::new(user_name, password).unwrap(); + let options = ClientOptions { - user_name: "alice".to_string(), - password: "sekrit".to_string(), + credentials, version: Version::default(), }; let client = Client::login(stream, options).await.unwrap(); diff --git a/src/proto/server/credentials.rs b/src/proto/server/credentials.rs index b6e5dbb..1f73c13 100644 --- a/src/proto/server/credentials.rs +++ b/src/proto/server/credentials.rs @@ -1,10 +1,16 @@ //! Defines the `Credentials` struct, for use when logging in to a server. +use crypto::digest::Digest; +use crypto::md5::Md5; + +use crate::proto::server::{LoginRequest, Version}; + /// Credentials for logging in a client to a server. #[derive(Debug, Eq, PartialEq)] pub struct Credentials { user_name: String, password: String, + digest: String, } impl Credentials { @@ -16,9 +22,15 @@ impl Credentials { return None; } + let mut hasher = Md5::new(); + hasher.input_str(&user_name); + hasher.input_str(&password); + let digest = hasher.result_str(); + Some(Credentials { user_name, password, + digest, }) } @@ -31,10 +43,27 @@ impl Credentials { pub fn password(&self) -> &str { &self.password } + + /// Returns md5(user_name + password). + pub fn digest(&self) -> &str { + &self.digest + } + + pub fn into_login_request(self, version: Version) -> LoginRequest { + LoginRequest { + username: self.user_name, + password: self.password, + digest: self.digest, + major: version.major, + minor: version.minor, + } + } } #[cfg(test)] mod tests { + use crate::proto::server::{LoginRequest, Version}; + use super::Credentials; #[test] @@ -52,4 +81,47 @@ mod tests { assert_eq!(credentials.user_name(), "alice"); assert_eq!(credentials.password(), "sekrit"); } + + #[test] + fn digest() { + let user_name = "alice".to_string(); + let password = "sekrit".to_string(); + + let credentials = Credentials::new(user_name, password).unwrap(); + + // To generate the expected value on the command line; + // + // ```sh + // $ user_name="alice" + // $ password="sekrit" + // $ echo -e "${user_name}${password}" | md5sum + // ``` + assert_eq!(credentials.digest(), "286da88eb442032bdd3913979af76e8a"); + } + + #[test] + fn into_login_request() { + let user_name = "alice".to_string(); + let password = "sekrit".to_string(); + let digest = "286da88eb442032bdd3913979af76e8a".to_string(); + + let credentials = + Credentials::new(user_name.clone(), password.clone()).unwrap(); + + let version = Version { + major: 13, + minor: 37, + }; + + assert_eq!( + credentials.into_login_request(version), + LoginRequest { + username: user_name, + password, + digest, + major: 13, + minor: 37, + } + ); + } } diff --git a/src/proto/server/mod.rs b/src/proto/server/mod.rs index 3706b30..04b3944 100644 --- a/src/proto/server/mod.rs +++ b/src/proto/server/mod.rs @@ -6,6 +6,7 @@ mod response; #[cfg(test)] mod testing; +pub use self::client::Version; pub use self::credentials::Credentials; pub use self::request::*; pub use self::response::*; diff --git a/src/proto/server/request.rs b/src/proto/server/request.rs index 44fa477..8862b64 100644 --- a/src/proto/server/request.rs +++ b/src/proto/server/request.rs @@ -1,8 +1,5 @@ use std::io; -use crypto::digest::Digest; -use crypto::md5::Md5; - use crate::proto::core::value::{ ValueDecode, ValueDecodeError, ValueDecoder, ValueEncode, ValueEncodeError, ValueEncoder, @@ -10,16 +7,6 @@ use crate::proto::core::value::{ use crate::proto::packet::{MutPacket, WriteToPacket}; use crate::proto::server::constants::*; -/* ------- * - * Helpers * - * ------- */ - -fn md5_str(string: &str) -> String { - let mut hasher = Md5::new(); - hasher.input_str(string); - hasher.result_str() -} - /*================* * SERVER REQUEST * *================*/ @@ -322,41 +309,12 @@ impl ValueDecode for FileSearchRequest { #[derive(Debug, Eq, PartialEq)] pub struct LoginRequest { - username: String, - password: String, - digest: String, - major: u32, - minor: u32, -} - -fn userpass_md5(username: &str, password: &str) -> String { - let userpass = String::new() + username + password; - md5_str(&userpass) -} - -impl LoginRequest { - pub fn new( - username: &str, - password: &str, - major: u32, - minor: u32, - ) -> Result { - if password.len() > 0 { - Ok(LoginRequest { - username: username.to_string(), - password: password.to_string(), - digest: userpass_md5(username, password), - major, - minor, - }) - } else { - Err("Empty password") - } - } - - fn has_correct_digest(&self) -> bool { - self.digest == userpass_md5(&self.username, &self.password) - } + // TODO: Rename to `user_name`. + pub username: String, + pub password: String, + pub digest: String, + pub major: u32, + pub minor: u32, } impl WriteToPacket for LoginRequest { @@ -636,23 +594,15 @@ mod tests { })) } - #[test] - #[should_panic] - fn new_login_request_with_empty_password() { - LoginRequest::new("alice", "", 1337, 42).unwrap(); - } - - #[test] - fn new_login_request_has_correct_digest() { - let request = LoginRequest::new("alice", "password1234", 1337, 42).unwrap(); - assert!(request.has_correct_digest()); - } - #[test] fn roundtrip_login_request() { - roundtrip(ServerRequest::LoginRequest( - LoginRequest::new("alice", "password1234", 1337, 42).unwrap(), - )) + roundtrip(ServerRequest::LoginRequest(LoginRequest { + username: "alice".to_string(), + password: "sekrit".to_string(), + digest: "abcdef".to_string(), + major: 13, + minor: 37, + })) } #[test]