From 00daa7b13399ea1fbe05472136cacc7642f585d8 Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Mon, 13 Apr 2020 01:39:52 -0400 Subject: [PATCH 1/6] Migrate to rust-argon2 --- oxide-auth/Cargo.toml | 2 + oxide-auth/src/lib.rs | 2 + oxide-auth/src/primitives/registrar.rs | 123 +++++-------------------- 3 files changed, 25 insertions(+), 102 deletions(-) diff --git a/oxide-auth/Cargo.toml b/oxide-auth/Cargo.toml index 649a33ef..85b5aec0 100644 --- a/oxide-auth/Cargo.toml +++ b/oxide-auth/Cargo.toml @@ -17,6 +17,7 @@ autoexamples = false base64 = "0.11" chrono = "0.4.2" hmac = "0.7.1" +once_cell = "1.3.1" serde = "1.0" serde_derive = "1.0" serde_json = "1.0" @@ -24,6 +25,7 @@ sha2 = "0.8.1" subtle = "2.2.2" rand = "0.7.3" ring = ">=0.13,<0.15" +rust-argon2 = "0.8.2" rmp-serde = "0.14" url = "1.7" diff --git a/oxide-auth/src/lib.rs b/oxide-auth/src/lib.rs index 968f6e86..42426669 100644 --- a/oxide-auth/src/lib.rs +++ b/oxide-auth/src/lib.rs @@ -65,10 +65,12 @@ //! [`Scopes`]: endpoint/trait.Scopes.html #![warn(missing_docs)] +extern crate argon2; extern crate base64; extern crate chrono; extern crate hmac; extern crate rand; +extern crate once_cell; extern crate ring; extern crate rmp_serde; extern crate serde; diff --git a/oxide-auth/src/primitives/registrar.rs b/oxide-auth/src/primitives/registrar.rs index f29a3594..a741a995 100644 --- a/oxide-auth/src/primitives/registrar.rs +++ b/oxide-auth/src/primitives/registrar.rs @@ -10,14 +10,12 @@ use std::cmp; use std::collections::HashMap; use std::fmt; use std::iter::{Extend, FromIterator}; -use std::num::NonZeroU32; use std::sync::{Arc, MutexGuard, RwLockWriteGuard}; use std::rc::Rc; +use argon2::{self, Config}; +use once_cell::sync::Lazy; use url::Url; -use ring::{digest, pbkdf2}; -use ring::error::Unspecified; -use ring::rand::{SystemRandom, SecureRandom}; /// Registrars provie a way to interact with clients. /// @@ -188,12 +186,6 @@ impl fmt::Debug for ClientType { } } -impl RegistrarError { - fn from(err: Unspecified) -> Self { - match err { Unspecified => RegistrarError::Unspecified } - } -} - impl Client { /// Create a public client. pub fn public(client_id: &str, redirect_uri: Url, default_scope: Scope) -> Client { @@ -281,8 +273,7 @@ impl cmp::PartialOrd for PreGrant { /// Determines how passphrases are stored and checked. /// -/// The provided library implementation is based on `Pbkdf2`. Other users may prefer to write their -/// own adaption with `Argon2`. If you do so, you could send a pull request my way. +/// The provided library implementation is based on `Argon2`. pub trait PasswordPolicy: Send + Sync { /// Transform the passphrase so it can be stored in the confidential client. fn store(&self, client_id: &str, passphrase: &[u8]) -> Vec; @@ -291,100 +282,26 @@ pub trait PasswordPolicy: Send + Sync { fn check(&self, client_id: &str, passphrase: &[u8], stored: &[u8]) -> Result<(), RegistrarError>; } -/// Store passwords using `Pbkdf2` to derive the stored value. -/// -/// Each instantiation generates a 16 byte random salt and prepends this additionally with the -/// username. This combined string is then used as the salt using the passphrase as the secret to -/// derive the output. The iteration count defaults to `65536` but can be customized. -pub struct Pbkdf2 { - /// A prebuilt random, or constructing one as needed. - random: Option, - iterations: NonZeroU32, -} +/// Store passwords using `Argon2` to derive the stored value. +#[derive(Clone, Debug, Default)] +pub struct Argon2 {} -impl Default for Pbkdf2 { - fn default() -> Self { - Pbkdf2 { - random: Some(SystemRandom::new()), - .. *PBKDF2_DEFAULTS - } - } -} - -impl Clone for Pbkdf2 { - fn clone(&self) -> Self { - Pbkdf2 { - random: Some(SystemRandom::new()), - .. *self - } - } -} - -impl fmt::Debug for Pbkdf2 { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - f.debug_struct("Pbkdf2") - .field("iterations", &self.iterations) - .field("random", &()) - .finish() - } -} - -impl Pbkdf2 { - /// Set the iteration count to `(1 << strength)`. - /// - /// This function will panic when the `strength` is larger or equal to `32`. - pub fn set_relative_strength(&mut self, strength: u8) { - assert!(strength < 32, "Strength value out of range (0-31): {}", strength); - self.iterations = NonZeroU32::new(1u32 << strength).unwrap(); - } - - fn salt(&self, user_identifier: &[u8]) -> Vec { - let mut vec = Vec::with_capacity(user_identifier.len() + 64); - let mut rnd_salt = [0; 16]; - - match self.random.as_ref() { - Some(random) => random.fill(&mut rnd_salt), - None => SystemRandom::new().fill(&mut rnd_salt), - }.expect("Failed to property initialize password storage salt"); - - vec.extend_from_slice(user_identifier); - vec.extend_from_slice(&rnd_salt[..]); - vec - } -} - -// A default instance for pbkdf2, randomness is sampled from the system each time. -// -// TODO: in the future there might be a way to get static memory initialized with an rng at load -// time by the loader. Then, a constant instance of the random generator may be available and we -// could get rid of the `Option`. -static PBKDF2_DEFAULTS: &Pbkdf2 = &Pbkdf2 { - random: None, - iterations: unsafe { NonZeroU32::new_unchecked(1 << 16) }, -}; - -impl PasswordPolicy for Pbkdf2 { +impl PasswordPolicy for Argon2 { fn store(&self, client_id: &str, passphrase: &[u8]) -> Vec { - let mut output = vec![0; 64]; - output.append(&mut self.salt(client_id.as_bytes())); - { - let (output, salt) = output.split_at_mut(64); - pbkdf2::derive(&digest::SHA256, self.iterations.into(), salt, passphrase, - output); - } - output + let config = Config::default(); + let encoded = argon2::hash_encoded(passphrase, client_id.as_bytes(), &config); + encoded.unwrap().as_bytes().to_vec() } fn check(&self, _client_id: &str /* Was interned */, passphrase: &[u8], stored: &[u8]) -> Result<(), RegistrarError> { - if stored.len() < 64 { - return Err(RegistrarError::PrimitiveError) - } - - let (verifier, salt) = stored.split_at(64); - pbkdf2::verify(&digest::SHA256, self.iterations.into(), salt, passphrase, verifier) - .map_err(RegistrarError::from) + String::from_utf8(stored.to_vec()) + .map_err(|_| RegistrarError::PrimitiveError) + .and_then(|hash| + argon2::verify_encoded(&hash, passphrase) + .map_err(|_| RegistrarError::Unspecified )) + .and_then(|valid| if valid { Ok(()) } else { Err(RegistrarError::Unspecified) } ) } } @@ -392,6 +309,8 @@ impl PasswordPolicy for Pbkdf2 { // Standard Implementations of Registrars // /////////////////////////////////////////////////////////////////////////////////////////////////// +static DEFAULT_PASSWORD_POLICY: Lazy = Lazy::new(|| { Argon2::default() }); + impl ClientMap { /// Create an empty map without any clients in it. pub fn new() -> ClientMap { @@ -413,7 +332,7 @@ impl ClientMap { fn current_policy<'a>(policy: &'a Option>) -> &'a dyn PasswordPolicy { policy .as_ref().map(|boxed| &**boxed) - .unwrap_or(PBKDF2_DEFAULTS) + .unwrap_or(&*DEFAULT_PASSWORD_POLICY) } } @@ -616,7 +535,7 @@ mod tests { #[test] fn public_client() { - let policy = Pbkdf2::default(); + let policy = Argon2::default(); let client = Client::public( "ClientId", "https://example.com".parse().unwrap(), @@ -632,7 +551,7 @@ mod tests { #[test] fn confidential_client() { - let policy = Pbkdf2::default(); + let policy = Argon2::default(); let pass = b"AB3fAj6GJpdxmEVeNCyPoA=="; let client = Client::confidential( "ClientId", From 1d25c34fe89ca4e846706345a1235601b88e28e1 Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Mon, 13 Apr 2020 10:16:31 -0400 Subject: [PATCH 2/6] Rework store and check --- oxide-auth/src/primitives/registrar.rs | 39 +++++++++++++++++++------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/oxide-auth/src/primitives/registrar.rs b/oxide-auth/src/primitives/registrar.rs index a741a995..c2ca0963 100644 --- a/oxide-auth/src/primitives/registrar.rs +++ b/oxide-auth/src/primitives/registrar.rs @@ -15,6 +15,7 @@ use std::rc::Rc; use argon2::{self, Config}; use once_cell::sync::Lazy; +use rand::{RngCore, thread_rng}; use url::Url; /// Registrars provie a way to interact with clients. @@ -189,7 +190,13 @@ impl fmt::Debug for ClientType { impl Client { /// Create a public client. pub fn public(client_id: &str, redirect_uri: Url, default_scope: Scope) -> Client { - Client { client_id: client_id.to_string(), redirect_uri, additional_redirect_uris: vec![], default_scope, client_type: ClientType::Public } + Client { + client_id: client_id.to_string(), + redirect_uri, + additional_redirect_uris: vec![], + default_scope, + client_type: ClientType::Public + } } /// Create a confidential client. @@ -288,20 +295,32 @@ pub struct Argon2 {} impl PasswordPolicy for Argon2 { fn store(&self, client_id: &str, passphrase: &[u8]) -> Vec { - let config = Config::default(); - let encoded = argon2::hash_encoded(passphrase, client_id.as_bytes(), &config); + let mut config = Config::default(); + config.ad = client_id.as_bytes(); + config.secret = &[]; + + let mut salt = vec![0; 32]; + thread_rng().try_fill_bytes(salt.as_mut_slice()) + .expect("Failed to generate password salt"); + + let encoded = argon2::hash_encoded(passphrase, &salt, &config); encoded.unwrap().as_bytes().to_vec() } - fn check(&self, _client_id: &str /* Was interned */, passphrase: &[u8], stored: &[u8]) + fn check(&self, client_id: &str /* Was interned */, passphrase: &[u8], stored: &[u8]) -> Result<(), RegistrarError> { - String::from_utf8(stored.to_vec()) - .map_err(|_| RegistrarError::PrimitiveError) - .and_then(|hash| - argon2::verify_encoded(&hash, passphrase) - .map_err(|_| RegistrarError::Unspecified )) - .and_then(|valid| if valid { Ok(()) } else { Err(RegistrarError::Unspecified) } ) + let hash = String::from_utf8(stored.to_vec()); + let valid = match hash { + Ok(hash) => argon2::verify_encoded_ext(&hash, passphrase, &[], client_id.as_bytes()) + .map_err(|_| RegistrarError::Unspecified), + _ => Err(RegistrarError::Unspecified), + }; + + match valid { + Ok(true) => Ok(()), + _ => Err(RegistrarError::Unspecified), + } } } From 678fd09c5a90cdc2fead92a4ab1d67aed28d05b8 Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Mon, 13 Apr 2020 10:18:54 -0400 Subject: [PATCH 3/6] Wrong error type --- oxide-auth/src/primitives/registrar.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/oxide-auth/src/primitives/registrar.rs b/oxide-auth/src/primitives/registrar.rs index c2ca0963..df4d0eaf 100644 --- a/oxide-auth/src/primitives/registrar.rs +++ b/oxide-auth/src/primitives/registrar.rs @@ -291,7 +291,9 @@ pub trait PasswordPolicy: Send + Sync { /// Store passwords using `Argon2` to derive the stored value. #[derive(Clone, Debug, Default)] -pub struct Argon2 {} +pub struct Argon2 { + _private: () +} impl PasswordPolicy for Argon2 { fn store(&self, client_id: &str, passphrase: &[u8]) -> Vec { @@ -313,8 +315,8 @@ impl PasswordPolicy for Argon2 { let hash = String::from_utf8(stored.to_vec()); let valid = match hash { Ok(hash) => argon2::verify_encoded_ext(&hash, passphrase, &[], client_id.as_bytes()) - .map_err(|_| RegistrarError::Unspecified), - _ => Err(RegistrarError::Unspecified), + .map_err(|_| RegistrarError::PrimitiveError), + _ => Err(RegistrarError::PrimitiveError), }; match valid { From b23454b2acfb3860728ca458dbce574a4cd58b3b Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Mon, 13 Apr 2020 10:25:37 -0400 Subject: [PATCH 4/6] Removed obsolete comment --- oxide-auth/src/primitives/registrar.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/oxide-auth/src/primitives/registrar.rs b/oxide-auth/src/primitives/registrar.rs index df4d0eaf..9734f7d7 100644 --- a/oxide-auth/src/primitives/registrar.rs +++ b/oxide-auth/src/primitives/registrar.rs @@ -309,7 +309,7 @@ impl PasswordPolicy for Argon2 { encoded.unwrap().as_bytes().to_vec() } - fn check(&self, client_id: &str /* Was interned */, passphrase: &[u8], stored: &[u8]) + fn check(&self, client_id: &str, passphrase: &[u8], stored: &[u8]) -> Result<(), RegistrarError> { let hash = String::from_utf8(stored.to_vec()); From bc2440a0c31add19361c335b916851eadc384811 Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Mon, 13 Apr 2020 11:20:56 -0400 Subject: [PATCH 5/6] Fixed error matching in check --- oxide-auth/src/primitives/registrar.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/oxide-auth/src/primitives/registrar.rs b/oxide-auth/src/primitives/registrar.rs index 9734f7d7..7649e077 100644 --- a/oxide-auth/src/primitives/registrar.rs +++ b/oxide-auth/src/primitives/registrar.rs @@ -321,7 +321,8 @@ impl PasswordPolicy for Argon2 { match valid { Ok(true) => Ok(()), - _ => Err(RegistrarError::Unspecified), + Ok(false) => Err(RegistrarError::Unspecified), + Err(err) => Err(err), } } } From e092eeffc4cc5425fb70792ff4629ae88295abc3 Mon Sep 17 00:00:00 2001 From: TheSpooler Date: Mon, 13 Apr 2020 11:35:07 -0400 Subject: [PATCH 6/6] Implement verification with try and main path --- oxide-auth/src/primitives/registrar.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/oxide-auth/src/primitives/registrar.rs b/oxide-auth/src/primitives/registrar.rs index 7649e077..bee25870 100644 --- a/oxide-auth/src/primitives/registrar.rs +++ b/oxide-auth/src/primitives/registrar.rs @@ -10,8 +10,8 @@ use std::cmp; use std::collections::HashMap; use std::fmt; use std::iter::{Extend, FromIterator}; -use std::sync::{Arc, MutexGuard, RwLockWriteGuard}; use std::rc::Rc; +use std::sync::{Arc, MutexGuard, RwLockWriteGuard}; use argon2::{self, Config}; use once_cell::sync::Lazy; @@ -312,17 +312,13 @@ impl PasswordPolicy for Argon2 { fn check(&self, client_id: &str, passphrase: &[u8], stored: &[u8]) -> Result<(), RegistrarError> { - let hash = String::from_utf8(stored.to_vec()); - let valid = match hash { - Ok(hash) => argon2::verify_encoded_ext(&hash, passphrase, &[], client_id.as_bytes()) - .map_err(|_| RegistrarError::PrimitiveError), - _ => Err(RegistrarError::PrimitiveError), - }; - + let hash = String::from_utf8(stored.to_vec()) + .map_err(|_| RegistrarError::PrimitiveError)?; + let valid = argon2::verify_encoded_ext(&hash, passphrase, &[], client_id.as_bytes()) + .map_err(|_| RegistrarError::PrimitiveError)?; match valid { - Ok(true) => Ok(()), - Ok(false) => Err(RegistrarError::Unspecified), - Err(err) => Err(err), + true => Ok(()), + false => Err(RegistrarError::Unspecified), } } }