From f1aa42fae494c64deabd4ca0064a3408641e5389 Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Fri, 21 Oct 2022 18:58:52 +0200 Subject: [PATCH] Disallow Ulid generation without explicit timestamp and rng --- clippy.toml | 2 ++ crates/cli/src/commands/manage.rs | 8 ++++++-- crates/handlers/src/oauth2/registration.rs | 5 ++++- crates/storage/src/oauth2/client.rs | 24 +++++++++++++++++++--- crates/storage/src/user.rs | 2 +- 5 files changed, 34 insertions(+), 7 deletions(-) diff --git a/clippy.toml b/clippy.toml index 93fce8d5..f110f13f 100644 --- a/clippy.toml +++ b/clippy.toml @@ -4,6 +4,8 @@ doc-valid-idents = ["OpenID", "OAuth", ".."] disallowed-methods = [ { path = "rand::thread_rng", reason = "do not create rngs on the fly, pass them as parameters" }, { path = "chrono::Utc::now", reason = "source the current time from the clock instead" }, + { path = "ulid::Ulid::from_datetime", reason = "use Ulid::from_datetime_with_source instead" }, + { path = "ulid::Ulid::new", reason = "use Ulid::from_datetime_with_source instead" }, ] disallowed-types = [ diff --git a/crates/cli/src/commands/manage.rs b/crates/cli/src/commands/manage.rs index 8afd3b5a..015bd977 100644 --- a/crates/cli/src/commands/manage.rs +++ b/crates/cli/src/commands/manage.rs @@ -54,6 +54,8 @@ impl Options { pub async fn run(&self, root: &super::Options) -> anyhow::Result<()> { use Subcommand as SC; let clock = Clock::default(); + // XXX: we should disallow SeedableRng::from_entropy + let mut rng = rand_chacha::ChaChaRng::from_entropy(); match &self.subcommand { SC::Register { username, password } => { @@ -61,9 +63,9 @@ impl Options { let pool = config.connect().await?; let mut txn = pool.begin().await?; let hasher = Argon2::default(); - let rng = rand_chacha::ChaChaRng::from_entropy(); - let user = register_user(&mut txn, rng, &clock, hasher, username, password).await?; + let user = + register_user(&mut txn, &mut rng, &clock, hasher, username, password).await?; txn.commit().await?; info!(?user, "User registered"); @@ -126,6 +128,8 @@ impl Options { insert_client_from_config( &mut txn, + &mut rng, + &clock, client_id, client_auth_method, encrypted_client_secret.as_deref(), diff --git a/crates/handlers/src/oauth2/registration.rs b/crates/handlers/src/oauth2/registration.rs index c6226782..9c7a2ad9 100644 --- a/crates/handlers/src/oauth2/registration.rs +++ b/crates/handlers/src/oauth2/registration.rs @@ -109,6 +109,7 @@ pub(crate) async fn post( State(policy_factory): State>, Json(body): Json, ) -> Result { + let (clock, mut rng) = crate::rng_and_clock()?; info!(?body, "Client registration"); // Validate the body @@ -127,10 +128,12 @@ pub(crate) async fn post( let mut txn = pool.begin().await?; // Let's generate a random client ID - let client_id = Ulid::new(); + let client_id = Ulid::from_datetime_with_source(clock.now().into(), &mut rng); insert_client( &mut txn, + &mut rng, + &clock, client_id, metadata.redirect_uris(), None, diff --git a/crates/storage/src/oauth2/client.rs b/crates/storage/src/oauth2/client.rs index d9c77958..b22db555 100644 --- a/crates/storage/src/oauth2/client.rs +++ b/crates/storage/src/oauth2/client.rs @@ -21,13 +21,14 @@ use mas_iana::{ }; use mas_jose::jwk::PublicJsonWebKeySet; use oauth2_types::requests::GrantType; +use rand::Rng; use sqlx::{PgConnection, PgExecutor}; use thiserror::Error; use ulid::Ulid; use url::Url; use uuid::Uuid; -use crate::PostgresqlBackend; +use crate::{Clock, PostgresqlBackend}; // XXX: response_types & contacts #[derive(Debug)] @@ -317,6 +318,8 @@ pub async fn lookup_client_by_client_id( #[allow(clippy::too_many_arguments)] pub async fn insert_client( conn: &mut PgConnection, + mut rng: impl Rng + Send, + clock: &Clock, client_id: Ulid, redirect_uris: &[Url], encrypted_client_secret: Option<&str>, @@ -391,9 +394,15 @@ pub async fn insert_client( .execute(&mut *conn) .await?; + let now = clock.now(); let (ids, redirect_uris): (Vec, Vec) = redirect_uris .iter() - .map(|uri| (Uuid::from(Ulid::new()), uri.as_str().to_owned())) + .map(|uri| { + ( + Uuid::from(Ulid::from_datetime_with_source(now.into(), &mut rng)), + uri.as_str().to_owned(), + ) + }) .unzip(); sqlx::query!( @@ -413,8 +422,11 @@ pub async fn insert_client( Ok(()) } +#[allow(clippy::too_many_arguments)] pub async fn insert_client_from_config( conn: &mut PgConnection, + mut rng: impl Rng + Send, + clock: &Clock, client_id: Ulid, client_auth_method: OAuthClientAuthenticationMethod, encrypted_client_secret: Option<&str>, @@ -451,9 +463,15 @@ pub async fn insert_client_from_config( .execute(&mut *conn) .await?; + let now = clock.now(); let (ids, redirect_uris): (Vec, Vec) = redirect_uris .iter() - .map(|uri| (Uuid::from(Ulid::new()), uri.as_str().to_owned())) + .map(|uri| { + ( + Uuid::from(Ulid::from_datetime_with_source(now.into(), &mut rng)), + uri.as_str().to_owned(), + ) + }) .unzip(); sqlx::query!( diff --git a/crates/storage/src/user.rs b/crates/storage/src/user.rs index b9070ec6..63282ca5 100644 --- a/crates/storage/src/user.rs +++ b/crates/storage/src/user.rs @@ -442,7 +442,7 @@ pub async fn set_password( password: &str, ) -> Result<(), anyhow::Error> { let created_at = clock.now(); - let id = Ulid::from_datetime(created_at.into()); + let id = Ulid::from_datetime_with_source(created_at.into(), &mut rng); tracing::Span::current().record("user_password.id", tracing::field::display(id)); let salt = SaltString::generate(&mut rng);