From 3c916a253028795381428582ace1f78576035908 Mon Sep 17 00:00:00 2001 From: Valentin Tolmer Date: Mon, 14 Jun 2021 16:02:36 +0200 Subject: [PATCH] Implement password checking using opaque --- .gitignore | 3 + Cargo.lock | 75 ++++++++++++++++- Cargo.toml | 3 +- model/src/opaque.rs | 65 +++++++++++---- src/domain/error.rs | 2 + src/domain/sql_backend_handler.rs | 134 ++++++++++++++++-------------- src/domain/sql_tables.rs | 6 +- src/infra/configuration.rs | 92 ++++++++++++++++---- src/infra/tcp_server.rs | 4 +- 9 files changed, 282 insertions(+), 102 deletions(-) diff --git a/.gitignore b/.gitignore index 34e1727..5b8cc47 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,6 @@ *.db *.db-shm *.db-wal + +# Server private key +server_key diff --git a/Cargo.lock b/Cargo.lock index 0a284e5..80c35b3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -698,6 +698,72 @@ dependencies = [ "zeroize", ] +[[package]] +name = "darling" +version = "0.12.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5f2c43f534ea4b0b049015d00269734195e6d3f0f6635cb692251aca6f9f8b3c" +dependencies = [ + "darling_core", + "darling_macro", +] + +[[package]] +name = "darling_core" +version = "0.12.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e91455b86830a1c21799d94524df0845183fa55bafd9aa137b01c7d1065fa36" +dependencies = [ + "fnv", + "ident_case", + "proc-macro2", + "quote", + "strsim", + "syn", +] + +[[package]] +name = "darling_macro" +version = "0.12.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "29b5acf0dea37a7f66f7b25d2c5e93fd46f8f6968b1a5d7a3e02e97768afc95a" +dependencies = [ + "darling_core", + "quote", + "syn", +] + +[[package]] +name = "derive_builder" +version = "0.10.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d13202debe11181040ae9063d739fa32cfcaaebe2275fe387703460ae2365b30" +dependencies = [ + "derive_builder_macro", +] + +[[package]] +name = "derive_builder_core" +version = "0.10.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "66e616858f6187ed828df7c64a6d71720d83767a7f19740b2d1b6fe6327b36e5" +dependencies = [ + "darling", + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "derive_builder_macro" +version = "0.10.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "58a94ace95092c5acb1e97a7e846b310cfbd499652f72297da7493f618a98d73" +dependencies = [ + "derive_builder_core", + "syn", +] + [[package]] name = "derive_more" version = "0.99.14" @@ -1088,6 +1154,12 @@ version = "1.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f3a87b616e37e93c22fb19bcd386f02f3af5ea98a25670ad0fce773de23c5e68" +[[package]] +name = "ident_case" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9e0384b61958566e926dc50660321d12159025e767c18e043daf26b70104c39" + [[package]] name = "idna" version = "0.2.3" @@ -1253,6 +1325,7 @@ dependencies = [ "chrono", "clap", "cron", + "derive_builder", "figment", "futures", "futures-util", @@ -1263,8 +1336,8 @@ dependencies = [ "lldap_model", "log", "mockall", + "opaque-ke", "rand 0.8.3", - "rust-argon2", "sea-query", "serde", "serde_json", diff --git a/Cargo.toml b/Cargo.toml index bbcbbdd..d9735fc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -12,11 +12,11 @@ actix-server = "2.0.0-beta.3" actix-service = "2.0.0" actix-web = "4.0.0-beta.3" anyhow = "*" -rust-argon2 = "0.8" async-trait = "0.1" chrono = { version = "*", features = [ "serde" ]} clap = "3.0.0-beta.2" cron = "*" +derive_builder = "0.10.2" futures = "*" futures-util = "*" hmac = "0.10" @@ -25,6 +25,7 @@ jwt = "0.13" ldap3_server = "*" lldap_model = { path = "model" } log = "*" +opaque-ke = "0.5" serde = "*" serde_json = "1" sha2 = "0.9" diff --git a/model/src/opaque.rs b/model/src/opaque.rs index c4c8858..cc11ee2 100644 --- a/model/src/opaque.rs +++ b/model/src/opaque.rs @@ -9,6 +9,41 @@ pub enum AuthenticationError { pub type AuthenticationResult = std::result::Result; +/// Wrapper around an opaque KeyPair to have type-checked public and private keys. +#[derive(Debug, Clone)] +pub struct KeyPair(pub opaque_ke::keypair::KeyPair<::Group>); + +pub struct PublicKey<'a>(&'a opaque_ke::keypair::Key); +pub struct PrivateKey<'a>(&'a opaque_ke::keypair::Key); + +impl <'a> std::ops::Deref for PublicKey<'a> { + type Target = &'a opaque_ke::keypair::Key; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl <'a> std::ops::Deref for PrivateKey<'a> { + type Target = &'a opaque_ke::keypair::Key; + fn deref(&self) -> &Self::Target { + &self.0 + } +} + +impl KeyPair { + pub fn private(&self) -> PrivateKey<'_> { + PrivateKey(self.0.private()) + } + + pub fn public(&self) -> PublicKey<'_> { + PublicKey(self.0.public()) + } + + pub fn from_private_key_slice(input: &[u8]) -> std::result::Result { + opaque_ke::keypair::KeyPair::<::Group>::from_private_key_slice(input).map(Self) + } +} + /// A wrapper around argon2 to provide the [`opaque_ke::slow_hash::SlowHash`] trait. pub struct ArgonHasher; @@ -56,11 +91,11 @@ impl CipherSuite for DefaultSuite { /// deserialized using the type's `deserialize` method. #[cfg(feature = "opaque_client")] pub mod client { - use super::*; + pub use super::*; /// Methods to register a new user, from the client side. pub mod registration { - use super::*; - use opaque_ke::{ + pub use super::*; + pub use opaque_ke::{ ClientRegistration, ClientRegistrationFinishParameters, ClientRegistrationFinishResult, ClientRegistrationStartResult, RegistrationResponse, }; @@ -91,8 +126,8 @@ pub mod client { /// Methods to login, from the client side. pub mod login { - use super::*; - use opaque_ke::{ + pub use super::*; + pub use opaque_ke::{ ClientLogin, ClientLoginFinishParameters, ClientLoginFinishResult, ClientLoginStartParameters, ClientLoginStartResult, CredentialResponse, }; @@ -123,24 +158,24 @@ pub mod client { /// intermediate results must be sent to the client using the serialized `.message`. #[cfg(feature = "opaque_server")] pub mod server { - use super::*; - use opaque_ke::{keypair::Key, ServerRegistration}; + pub use super::*; + pub use opaque_ke::ServerRegistration; /// Methods to register a new user, from the server side. pub mod registration { - use super::*; - use opaque_ke::{RegistrationRequest, RegistrationUpload, ServerRegistrationStartResult}; + pub use super::*; + pub use opaque_ke::{RegistrationRequest, RegistrationUpload, ServerRegistrationStartResult}; /// Start a registration process, from a request sent by the client. /// /// The result must be kept for the next step. pub fn start_registration( rng: &mut R, registration_request: RegistrationRequest, - server_public_key: &Key, + server_public_key: PublicKey<'_>, ) -> AuthenticationResult> { Ok(ServerRegistration::::start( rng, registration_request, - server_public_key, + *server_public_key, )?) } @@ -155,8 +190,8 @@ pub mod server { /// Methods to handle user login, from the server-side. pub mod login { - use super::*; - use opaque_ke::{ + pub use super::*; + pub use opaque_ke::{ CredentialFinalization, CredentialRequest, ServerLogin, ServerLoginFinishResult, ServerLoginStartParameters, ServerLoginStartResult, }; @@ -167,13 +202,13 @@ pub mod server { pub fn start_login( rng: &mut R, password_file: ServerRegistration, - server_private_key: &Key, + server_private_key: PrivateKey<'_>, credential_request: CredentialRequest, ) -> AuthenticationResult> { Ok(ServerLogin::start( rng, password_file, - server_private_key, + *server_private_key, credential_request, ServerLoginStartParameters::default(), )?) diff --git a/src/domain/error.rs b/src/domain/error.rs index a1c882c..955fac3 100644 --- a/src/domain/error.rs +++ b/src/domain/error.rs @@ -6,6 +6,8 @@ pub enum Error { AuthenticationError(String), #[error("Database error: `{0}`")] DatabaseError(#[from] sqlx::Error), + #[error("Authentication protocol error for `{0}`")] + AuthenticationProtocolError(#[from] lldap_model::opaque::AuthenticationError), } pub type Result = std::result::Result; diff --git a/src/domain/sql_backend_handler.rs b/src/domain/sql_backend_handler.rs index 30022fd..0a26e5d 100644 --- a/src/domain/sql_backend_handler.rs +++ b/src/domain/sql_backend_handler.rs @@ -3,6 +3,7 @@ use crate::infra::configuration::Configuration; use async_trait::async_trait; use futures_util::StreamExt; use futures_util::TryStreamExt; +use lldap_model::opaque; use log::*; use sea_query::{Expr, Iden, Order, Query, SimpleExpr, Value}; use sqlx::Row; @@ -20,31 +21,55 @@ impl SqlBackendHandler { } } -fn get_password_config(pepper: &str) -> argon2::Config { - argon2::Config { - secret: pepper.as_bytes(), - ..Default::default() - } +fn get_password_file( + clear_password: &str, + server_public_key: opaque::PublicKey<'_>, +) -> Result> { + use opaque::{client, server}; + let mut rng = rand::rngs::OsRng; + let client_register_start_result = + client::registration::start_registration(clear_password, &mut rng)?; + + let server_register_start_result = server::registration::start_registration( + &mut rng, + client_register_start_result.message, + server_public_key, + )?; + + let client_registration_result = client::registration::finish_registration( + client_register_start_result.state, + server_register_start_result.message, + &mut rng, + )?; + + Ok(server::registration::get_password_file( + server_register_start_result.state, + client_registration_result.message, + )?) } -fn hash_password(clear_password: &str, salt: &str, pepper: &str) -> String { - let config = get_password_config(pepper); - argon2::hash_encoded(clear_password.as_bytes(), salt.as_bytes(), &config) - .map_err(|e| anyhow::anyhow!("Error encoding password: {}", e)) - .unwrap() -} +fn passwords_match( + password_file_bytes: &[u8], + clear_password: &str, + server_private_key: opaque::PrivateKey<'_>, +) -> Result<()> { + use opaque::{client, client::login::*, server, server::login::*, DefaultSuite}; + let mut rng = rand::rngs::OsRng; + let client_login_start_result = client::login::start_login(clear_password, &mut rng)?; -fn passwords_match(encrypted_password: &str, clear_password: &str, pepper: &str) -> bool { - argon2::verify_encoded_ext( - encrypted_password, - clear_password.as_bytes(), - pepper.as_bytes(), - /*additional_data=*/ b"", - ) - .unwrap_or_else(|e| { - log::error!("Error checking password: {}", e); - false - }) + let password_file = ServerRegistration::::deserialize(password_file_bytes) + .map_err(opaque::AuthenticationError::ProtocolError)?; + let server_login_start_result = server::login::start_login( + &mut rng, + password_file, + server_private_key, + client_login_start_result.message, + )?; + finish_login( + client_login_start_result.state, + server_login_start_result.message, + )?; + Ok(()) } fn get_filter_expr(filter: RequestFilter) -> SimpleExpr { @@ -85,14 +110,14 @@ impl BackendHandler for SqlBackendHandler { .and_where(Expr::col(Users::UserId).eq(request.name.as_str())) .to_string(DbQueryBuilder {}); if let Ok(row) = sqlx::query(&query).fetch_one(&self.sql_pool).await { - if passwords_match( - &row.get::(&*Users::PasswordHash.to_string()), + if let Err(e) = passwords_match( + &row.get::, _>(&*Users::PasswordHash.to_string()), &request.password, - &self.config.secret_pepper, + self.config.get_server_keys().private(), ) { - return Ok(()); + debug!(r#"Invalid password for "{}": {}"#, request.name, e); } else { - debug!(r#"Invalid password for "{}""#, request.name); + return Ok(()); } } else { debug!(r#"No user found for "{}""#, request.name); @@ -208,16 +233,9 @@ impl BackendHandler for SqlBackendHandler { } async fn create_user(&self, request: CreateUserRequest) -> Result<()> { - use rand::{distributions::Alphanumeric, rngs::SmallRng, Rng, SeedableRng}; - // TODO: Initialize the rng only once. Maybe Arc? - let mut rng = SmallRng::from_entropy(); - let salt: String = std::iter::repeat(()) - .map(|()| rng.sample(Alphanumeric)) - .map(char::from) - .take(32) - .collect(); - // The salt is included in the password hash. - let password_hash = hash_password(&request.password, &salt, &self.config.secret_pepper); + let password_hash = + get_password_file(&request.password, self.config.get_server_keys().public())? + .serialize(); let query = Query::insert() .into_table(Users::Table) .columns(vec![ @@ -283,6 +301,14 @@ impl BackendHandler for SqlBackendHandler { mod tests { use super::*; use crate::domain::sql_tables::init_table; + use crate::infra::configuration::ConfigurationBuilder; + + fn get_default_config() -> Configuration { + ConfigurationBuilder::default() + .verbose(true) + .build() + .unwrap() + } async fn get_in_memory_db() -> Pool { PoolOptions::new().connect("sqlite::memory:").await.unwrap() @@ -328,11 +354,11 @@ mod tests { #[tokio::test] async fn test_bind_admin() { let sql_pool = get_in_memory_db().await; - let config = Configuration { - ldap_user_dn: "admin".to_string(), - ldap_user_pass: "test".to_string(), - ..Default::default() - }; + let config = ConfigurationBuilder::default() + .ldap_user_dn("admin".to_string()) + .ldap_user_pass("test".to_string()) + .build() + .unwrap(); let handler = SqlBackendHandler::new(config, sql_pool); handler .bind(BindRequest { @@ -343,24 +369,10 @@ mod tests { .unwrap(); } - #[test] - fn test_argon() { - let password = b"password"; - let salt = b"randomsalt"; - let pepper = b"pepper"; - let config = argon2::Config { - secret: pepper, - ..Default::default() - }; - let hash = argon2::hash_encoded(password, salt, &config).unwrap(); - let matches = argon2::verify_encoded_ext(&hash, password, pepper, b"").unwrap(); - assert!(matches); - } - #[tokio::test] async fn test_bind_user() { let sql_pool = get_initialized_db().await; - let config = Configuration::default(); + let config = get_default_config(); let handler = SqlBackendHandler::new(config, sql_pool.clone()); insert_user(&handler, "bob", "bob00").await; @@ -390,7 +402,7 @@ mod tests { #[tokio::test] async fn test_list_users() { let sql_pool = get_initialized_db().await; - let config = Configuration::default(); + let config = get_default_config(); let handler = SqlBackendHandler::new(config, sql_pool); insert_user(&handler, "bob", "bob00").await; insert_user(&handler, "patrick", "pass").await; @@ -455,7 +467,7 @@ mod tests { #[tokio::test] async fn test_list_groups() { let sql_pool = get_initialized_db().await; - let config = Configuration::default(); + let config = get_default_config(); let handler = SqlBackendHandler::new(config, sql_pool.clone()); insert_user(&handler, "bob", "bob00").await; insert_user(&handler, "patrick", "pass").await; @@ -484,7 +496,7 @@ mod tests { #[tokio::test] async fn test_get_user_groups() { let sql_pool = get_initialized_db().await; - let config = Configuration::default(); + let config = get_default_config(); let handler = SqlBackendHandler::new(config, sql_pool.clone()); insert_user(&handler, "bob", "bob00").await; insert_user(&handler, "patrick", "pass").await; @@ -519,7 +531,7 @@ mod tests { #[tokio::test] async fn test_delete_user() { let sql_pool = get_initialized_db().await; - let config = Configuration::default(); + let config = get_default_config(); let handler = SqlBackendHandler::new(config, sql_pool.clone()); insert_user(&handler, "val", "s3np4i").await; diff --git a/src/domain/sql_tables.rs b/src/domain/sql_tables.rs index 0c69b98..f0eeef3 100644 --- a/src/domain/sql_tables.rs +++ b/src/domain/sql_tables.rs @@ -54,11 +54,7 @@ pub async fn init_table(pool: &Pool) -> sqlx::Result<()> { .col(ColumnDef::new(Users::LastName).string_len(255)) .col(ColumnDef::new(Users::Avatar).binary()) .col(ColumnDef::new(Users::CreationDate).date_time().not_null()) - .col( - ColumnDef::new(Users::PasswordHash) - .string_len(255) - .not_null(), - ) + .col(ColumnDef::new(Users::PasswordHash).binary().not_null()) .col(ColumnDef::new(Users::TotpSecret).string_len(64)) .col(ColumnDef::new(Users::MfaType).string_len(64)) .to_string(DbQueryBuilder {}), diff --git a/src/infra/configuration.rs b/src/infra/configuration.rs index 04b60cf..fcc9011 100644 --- a/src/infra/configuration.rs +++ b/src/infra/configuration.rs @@ -1,45 +1,61 @@ -use anyhow::Result; +use anyhow::{anyhow, Result}; use figment::{ providers::{Env, Format, Serialized, Toml}, Figment, }; +use lldap_model::{opaque, opaque::KeyPair}; use serde::{Deserialize, Serialize}; use crate::infra::cli::CLIOpts; -#[derive(Clone, Debug, Deserialize, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize, derive_builder::Builder)] +#[builder( + pattern = "owned", + default = "Configuration::default()", + build_fn(name = "private_build", validate = "Self::validate") +)] pub struct Configuration { pub ldap_port: u16, pub ldaps_port: u16, pub http_port: u16, - pub secret_pepper: String, pub jwt_secret: String, pub ldap_base_dn: String, pub ldap_user_dn: String, pub ldap_user_pass: String, pub database_url: String, pub verbose: bool, + pub key_file: String, + #[serde(skip)] + #[builder(field(private), setter(strip_option))] + server_keys: Option, } -impl Default for Configuration { - fn default() -> Self { - Configuration { - ldap_port: 3890, - ldaps_port: 6360, - http_port: 17170, - secret_pepper: String::from("secretsecretpepper"), - jwt_secret: String::from("secretjwtsecret"), - ldap_base_dn: String::from("dc=example,dc=com"), - // cn=admin,dc=example,dc=com - ldap_user_dn: String::from("admin"), - ldap_user_pass: String::from("password"), - database_url: String::from("sqlite://users.db?mode=rwc"), - verbose: false, +impl ConfigurationBuilder { + #[cfg(test)] + pub fn build(self) -> Result { + let server_keys = get_server_keys( + &self + .key_file + .as_deref() + .unwrap_or("server_key"), + )?; + Ok(self.server_keys(server_keys).private_build()?) + } + + fn validate(&self) -> Result<(), String> { + if self.server_keys.is_none() { + Err("Don't use `private_build`, use `build` instead".to_string()) + } else { + Ok(()) } } } impl Configuration { + pub fn get_server_keys(&self) -> &KeyPair { + self.server_keys.as_ref().unwrap() + } + fn merge_with_cli(mut self: Configuration, cli_opts: CLIOpts) -> Configuration { if cli_opts.verbose { self.verbose = true; @@ -55,6 +71,45 @@ impl Configuration { self } + + pub(super) fn default() -> Self { + Configuration { + ldap_port: 3890, + ldaps_port: 6360, + http_port: 17170, + jwt_secret: String::from("secretjwtsecret"), + ldap_base_dn: String::from("dc=example,dc=com"), + // cn=admin,dc=example,dc=com + ldap_user_dn: String::from("admin"), + ldap_user_pass: String::from("password"), + database_url: String::from("sqlite://users.db?mode=rwc"), + verbose: false, + key_file: String::from("server_key"), + server_keys: None, + } + } +} + +fn get_server_keys(file_path: &str) -> Result { + use opaque_ke::ciphersuite::CipherSuite; + use std::path::Path; + let path = Path::new(file_path); + if path.exists() { + let bytes = std::fs::read(file_path) + .map_err(|e| anyhow!("Could not read key file `{}`: {}", file_path, e))?; + Ok(KeyPair::from_private_key_slice(&bytes)?) + } else { + let mut rng = rand::rngs::OsRng; + let keypair = opaque::DefaultSuite::generate_random_keypair(&mut rng); + std::fs::write(path, keypair.private().as_slice()).map_err(|e| { + anyhow!( + "Could not write the generated server keys to file `{}`: {}", + file_path, + e + ) + })?; + Ok(KeyPair(keypair)) + } } pub fn init(cli_opts: CLIOpts) -> Result { @@ -65,6 +120,7 @@ pub fn init(cli_opts: CLIOpts) -> Result { .merge(Env::prefixed("LLDAP_")) .extract()?; - let config = config.merge_with_cli(cli_opts); + let mut config = config.merge_with_cli(cli_opts); + config.server_keys = Some(get_server_keys(&config.key_file)?); Ok(config) } diff --git a/src/infra/tcp_server.rs b/src/infra/tcp_server.rs index 1bee320..581dd9f 100644 --- a/src/infra/tcp_server.rs +++ b/src/infra/tcp_server.rs @@ -25,7 +25,9 @@ async fn index(req: HttpRequest) -> actix_web::Result { pub(crate) fn error_to_http_response(error: DomainError) -> HttpResponse { match error { - DomainError::AuthenticationError(_) => HttpResponse::Unauthorized(), + DomainError::AuthenticationError(_) | DomainError::AuthenticationProtocolError(_) => { + HttpResponse::Unauthorized() + } DomainError::DatabaseError(_) => HttpResponse::InternalServerError(), } .body(error.to_string())