From 8cdbc77ca8713f6cb416555b07d78c9218ac9065 Mon Sep 17 00:00:00 2001 From: Nitish Tiwari Date: Thu, 18 May 2023 19:32:48 +0530 Subject: [PATCH 1/5] Minor changes in function names and add relevant characters --- server/src/handlers/http.rs | 16 +++++++++++----- server/src/handlers/http/rbac.rs | 14 +++++++------- server/src/rbac.rs | 2 +- server/src/rbac/user.rs | 8 +++++--- server/src/validator.rs | 10 +++++++--- 5 files changed, 31 insertions(+), 19 deletions(-) diff --git a/server/src/handlers/http.rs b/server/src/handlers/http.rs index ae8238684..dcf37b497 100644 --- a/server/src/handlers/http.rs +++ b/server/src/handlers/http.rs @@ -31,7 +31,7 @@ use rustls::{Certificate, PrivateKey, ServerConfig}; use rustls_pemfile::{certs, pkcs8_private_keys}; use crate::option::CONFIG; -use crate::rbac::get_user_map; +use crate::rbac::user_map; mod health_check; mod ingest; @@ -62,14 +62,14 @@ macro_rules! create_app { }; } -async fn validator( +async fn authenticate( req: ServiceRequest, credentials: BasicAuth, ) -> Result { let username = credentials.user_id().trim(); let password = credentials.password().unwrap().trim(); - if let Some(user) = get_user_map().read().unwrap().get(username) { + if let Some(user) = user_map().read().unwrap().get(username) { if user.verify(password) { return Ok(req); } @@ -130,6 +130,7 @@ pub async fn run_http(prometheus: PrometheusMetrics) -> anyhow::Result<()> { pub fn configure_routes(cfg: &mut web::ServiceConfig) { let generated = generate(); + //log stream API let logstream_api = web::scope("/{logstream}") .service( web::resource("") @@ -163,6 +164,8 @@ pub fn configure_routes(cfg: &mut web::ServiceConfig) { // GET "/logstream/{logstream}/retention" ==> Get retention for given logstream .route(web::get().to(logstream::get_retention)), ); + + // User API let user_api = web::scope("/user").service( web::resource("/{username}") // POST /user/{username} => Create a new user @@ -170,7 +173,10 @@ pub fn configure_routes(cfg: &mut web::ServiceConfig) { // DELETE /user/{username} => Delete a user .route(web::delete().to(rbac::delete_user)) .wrap_fn(|req, srv| { - // deny request if username is same as username from config + // deny request if username is same as username from env variable (P_USERNAME) + // The root credentials set in the env vars (P_USERNAME & P_PASSWORD) are treated + // as root credentials. Any other user is not allowed to modify / delete + // the root user. let username = req.match_info().get("username").unwrap_or(""); let is_root = username == CONFIG.parseable.username; let call = srv.call(req); @@ -210,7 +216,7 @@ pub fn configure_routes(cfg: &mut web::ServiceConfig) { ), ) .service(user_api) - .wrap(HttpAuthentication::basic(validator)), + .wrap(HttpAuthentication::basic(authenticate)), ) // GET "/" ==> Serve the static frontend directory .service(ResourceFiles::new("/", generated)); diff --git a/server/src/handlers/http/rbac.rs b/server/src/handlers/http/rbac.rs index c24f86fa3..25531caaa 100644 --- a/server/src/handlers/http/rbac.rs +++ b/server/src/handlers/http/rbac.rs @@ -19,7 +19,7 @@ use crate::{ option::CONFIG, rbac::{ - get_user_map, + user_map, user::{PassCode, User}, }, storage::{self, ObjectStorageError, StorageMetadata}, @@ -38,9 +38,9 @@ static UPDATE_LOCK: Mutex<()> = Mutex::const_new(()); // returns password generated for this user pub async fn put_user(username: web::Path) -> Result { let username = username.into_inner(); - validator::verify_username(&username)?; + validator::user_name(&username)?; let _ = UPDATE_LOCK.lock().await; - let user_exists = get_user_map().read().unwrap().contains_key(&username); + let user_exists = user_map().read().unwrap().contains_key(&username); if user_exists { reset_password(username).await } else { @@ -54,7 +54,7 @@ pub async fn put_user(username: web::Path) -> Result) -> Result) -> Result Result { put_metadata(&metadata).await?; // update in mem table - get_user_map() + user_map() .write() .unwrap() .get_mut(&username) diff --git a/server/src/rbac.rs b/server/src/rbac.rs index 04b6d14a1..d53be66ec 100644 --- a/server/src/rbac.rs +++ b/server/src/rbac.rs @@ -26,7 +26,7 @@ pub mod user; pub static USERS: OnceCell> = OnceCell::new(); -pub fn get_user_map() -> &'static RwLock { +pub fn user_map() -> &'static RwLock { USERS.get().expect("user map is set") } diff --git a/server/src/rbac/user.rs b/server/src/rbac/user.rs index e98098ad0..26e70b4f3 100644 --- a/server/src/rbac/user.rs +++ b/server/src/rbac/user.rs @@ -49,8 +49,8 @@ impl User { ) } - // Verification works because the PasswordHash is in PHC format - // $[$v=][$=(,=)*][$[$]] + // Take the password and compare with the hash stored internally (PHC format ==> + // $[$v=][$=(,=)*][$[$]]) // ref https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md#specification pub fn verify(&self, password: &str) -> bool { let parsed_hash = PasswordHash::new(&self.password_hash).unwrap(); @@ -59,7 +59,7 @@ impl User { .is_ok() } - // gen new password + // generate a new password pub fn gen_new_password() -> PassCode { let password = Alphanumeric.sample_string(&mut rand::thread_rng(), 16); let hash = gen_hash(&password); @@ -67,6 +67,8 @@ impl User { } } +// generate a one way hash for password to be stored in metadata file +// ref https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md fn gen_hash(password: &str) -> String { let salt = SaltString::generate(&mut OsRng); let argon2 = Argon2::default(); diff --git a/server/src/validator.rs b/server/src/validator.rs index 956facfa5..3f10fe236 100644 --- a/server/src/validator.rs +++ b/server/src/validator.rs @@ -114,7 +114,11 @@ pub fn stream_name(stream_name: &str) -> Result<(), StreamNameValidationError> { Ok(()) } -pub fn verify_username(username: &str) -> Result<(), UsernameValidationError> { +// validate if username is valid +// username should be between 3 and 64 characters long +// username should contain only alphanumeric characters or underscores +// username should be lowercase +pub fn user_name(username: &str) -> Result<(), UsernameValidationError> { // Check if the username meets the required criteria if username.len() < 3 || username.len() > 64 { return Err(UsernameValidationError::InvalidLength); @@ -254,10 +258,10 @@ pub mod error { #[derive(Debug, thiserror::Error)] pub enum UsernameValidationError { - #[error("Username length should be between 3 and 64 chars")] + #[error("Username should be between 3 and 64 chars long")] InvalidLength, #[error( - "Username contains invalid characters. Only lowercase aplhanumeric and _ is allowed" + "Username contains invalid characters. Please use lowercase, alphanumeric or underscore" )] SpecialChar, } From 81595c2fee07bc8f63b59e10cb4e63a55e0b8654 Mon Sep 17 00:00:00 2001 From: Nitish Tiwari Date: Thu, 18 May 2023 19:37:18 +0530 Subject: [PATCH 2/5] clippy --- server/src/handlers/http/rbac.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/handlers/http/rbac.rs b/server/src/handlers/http/rbac.rs index 25531caaa..db78247f0 100644 --- a/server/src/handlers/http/rbac.rs +++ b/server/src/handlers/http/rbac.rs @@ -74,7 +74,7 @@ pub async fn delete_user(username: web::Path) -> Result Date: Thu, 18 May 2023 19:37:39 +0530 Subject: [PATCH 3/5] cargo fmt --- server/src/handlers/http/rbac.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/handlers/http/rbac.rs b/server/src/handlers/http/rbac.rs index db78247f0..3d1bd71a1 100644 --- a/server/src/handlers/http/rbac.rs +++ b/server/src/handlers/http/rbac.rs @@ -19,8 +19,8 @@ use crate::{ option::CONFIG, rbac::{ - user_map, user::{PassCode, User}, + user_map, }, storage::{self, ObjectStorageError, StorageMetadata}, validator::{self, error::UsernameValidationError}, From 666881de73cf263139256ec3a3b09af6288cb8f8 Mon Sep 17 00:00:00 2001 From: Nitish Tiwari Date: Thu, 18 May 2023 19:45:30 +0530 Subject: [PATCH 4/5] fix comment --- server/src/handlers/http.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/handlers/http.rs b/server/src/handlers/http.rs index dcf37b497..bf0fc158f 100644 --- a/server/src/handlers/http.rs +++ b/server/src/handlers/http.rs @@ -173,10 +173,10 @@ pub fn configure_routes(cfg: &mut web::ServiceConfig) { // DELETE /user/{username} => Delete a user .route(web::delete().to(rbac::delete_user)) .wrap_fn(|req, srv| { - // deny request if username is same as username from env variable (P_USERNAME) - // The root credentials set in the env vars (P_USERNAME & P_PASSWORD) are treated + // The credentials set in the env vars (P_USERNAME & P_PASSWORD) are treated // as root credentials. Any other user is not allowed to modify / delete - // the root user. + // the root user. Deny request if username is same as username + // from env variable P_USERNAME. let username = req.match_info().get("username").unwrap_or(""); let is_root = username == CONFIG.parseable.username; let call = srv.call(req); From 9aea72412e361a04baefa622b984019979dfbf1e Mon Sep 17 00:00:00 2001 From: Nitish Tiwari Date: Thu, 18 May 2023 19:48:54 +0530 Subject: [PATCH 5/5] cargo fmt --- server/src/handlers/http.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/src/handlers/http.rs b/server/src/handlers/http.rs index bf0fc158f..cd9ddfca1 100644 --- a/server/src/handlers/http.rs +++ b/server/src/handlers/http.rs @@ -174,8 +174,8 @@ pub fn configure_routes(cfg: &mut web::ServiceConfig) { .route(web::delete().to(rbac::delete_user)) .wrap_fn(|req, srv| { // The credentials set in the env vars (P_USERNAME & P_PASSWORD) are treated - // as root credentials. Any other user is not allowed to modify / delete - // the root user. Deny request if username is same as username + // as root credentials. Any other user is not allowed to modify or delete + // the root user. Deny request if username is same as username // from env variable P_USERNAME. let username = req.match_info().get("username").unwrap_or(""); let is_root = username == CONFIG.parseable.username;