From 1d257cc0e617ecf91cea812123628dc46ab8f21e Mon Sep 17 00:00:00 2001 From: drone Date: Wed, 13 Jul 2022 00:12:05 +0000 Subject: [PATCH 1/4] Update Rust crate actix-identity to 0.5.1 --- Cargo.lock | 39 ++++++++++++++++++++++++++++++++++----- Cargo.toml | 2 +- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index df53623..49db778 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -82,17 +82,18 @@ dependencies = [ [[package]] name = "actix-identity" -version = "0.4.0" +version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "171fe3ed055b2dd50c61967911d253d47e76e1d4308acfbf99fc7affe5ec42aa" +checksum = "568d86155c0c6637a132d1fd351adf1c7164f526639fcdaf07d0dae1b7ee0464" dependencies = [ "actix-service", + "actix-session", "actix-utils", "actix-web", - "futures-util", + "anyhow", + "futures-core", "serde", - "serde_json", - "time", + "tracing", ] [[package]] @@ -158,6 +159,23 @@ dependencies = [ "pin-project-lite", ] +[[package]] +name = "actix-session" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "861c2463ccba4af8f272936fcf4999af6305492fc939bf0dfe71db86142ae843" +dependencies = [ + "actix-service", + "actix-utils", + "actix-web", + "anyhow", + "async-trait", + "derive_more", + "serde", + "serde_json", + "tracing", +] + [[package]] name = "actix-utils" version = "3.0.0" @@ -367,6 +385,17 @@ dependencies = [ "toml", ] +[[package]] +name = "async-trait" +version = "0.1.56" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "96cf8829f67d2eab0b2dfa42c5d0ef737e0724e4a82b01b3e292456202b19716" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "atty" version = "0.2.14" diff --git a/Cargo.toml b/Cargo.toml index eeb183e..452a7e9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,7 +7,7 @@ edition = "2021" [dependencies] actix = "0.13.0" -actix-identity = "0.4.0" +actix-identity = "0.5.1" actix-web = "4" clap = { version = "3.2.6", features = ["derive", "env"] } include_dir = "0.7.2" From 07542abf8b655581ace35f5638173a853028b2d7 Mon Sep 17 00:00:00 2001 From: Pierre Hubert Date: Fri, 22 Jul 2022 12:21:38 +0200 Subject: [PATCH 2/4] Update actix_identity --- Cargo.lock | 5 ++- Cargo.toml | 3 +- src/constants.rs | 4 +- src/controllers/login_api.rs | 11 +++--- src/controllers/login_controller.rs | 58 +++++++++++++++------------- src/controllers/openid_controller.rs | 2 +- src/data/current_user.rs | 2 +- src/data/session_identity.rs | 26 +++++++++---- src/main.rs | 33 ++++++++++------ src/middlewares/auth_middleware.rs | 9 ++++- 10 files changed, 94 insertions(+), 59 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bf878d4..06632c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -82,9 +82,9 @@ dependencies = [ [[package]] name = "actix-identity" -version = "0.5.1" +version = "0.5.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "568d86155c0c6637a132d1fd351adf1c7164f526639fcdaf07d0dae1b7ee0464" +checksum = "1224c9f9593dc27c9077b233ce04adedc1d7febcfc35ee9f53ea3c24df180bec" dependencies = [ "actix-service", "actix-session", @@ -452,6 +452,7 @@ version = "0.1.0" dependencies = [ "actix", "actix-identity", + "actix-session", "actix-web", "aes-gcm", "askama", diff --git a/Cargo.toml b/Cargo.toml index 32a7819..4e52a8d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,8 +7,9 @@ edition = "2021" [dependencies] actix = "0.13.0" -actix-identity = "0.5.1" +actix-identity = "0.5.2" actix-web = "4" +actix-session = { version = "0.7.0", features = ["cookie-session"] } clap = { version = "3.2.12", features = ["derive", "env"] } include_dir = "0.7.2" log = "0.4.17" diff --git a/src/constants.rs b/src/constants.rs index f60cbba..7c5c047 100644 --- a/src/constants.rs +++ b/src/constants.rs @@ -14,10 +14,10 @@ pub const DEFAULT_ADMIN_PASSWORD: &str = "admin"; pub const APP_NAME: &str = "Basic OIDC"; /// Maximum session duration after inactivity, in seconds -pub const MAX_INACTIVITY_DURATION: i64 = 60 * 30; +pub const MAX_INACTIVITY_DURATION: u64 = 60 * 30; /// Maximum session duration (6 hours) -pub const MAX_SESSION_DURATION: i64 = 3600 * 6; +pub const MAX_SESSION_DURATION: u64 = 3600 * 6; /// Minimum password length pub const MIN_PASS_LEN: usize = 4; diff --git a/src/controllers/login_api.rs b/src/controllers/login_api.rs index afa4355..c7ebdab 100644 --- a/src/controllers/login_api.rs +++ b/src/controllers/login_api.rs @@ -1,5 +1,5 @@ use actix_identity::Identity; -use actix_web::{HttpResponse, Responder, web}; +use actix_web::{HttpRequest, HttpResponse, Responder, web}; use webauthn_rs::proto::PublicKeyCredential; use crate::data::session_identity::{SessionIdentity, SessionStatus}; @@ -13,16 +13,17 @@ pub struct AuthWebauthnRequest { pub async fn auth_webauthn(id: Identity, req: web::Json, - manager: WebAuthManagerReq) -> impl Responder { - if !SessionIdentity(&id).need_2fa_auth() { + manager: WebAuthManagerReq, + http_req: HttpRequest) -> impl Responder { + if !SessionIdentity(Some(&id)).need_2fa_auth() { return HttpResponse::Unauthorized().json("No 2FA required!"); } - let user_id = SessionIdentity(&id).user_id(); + let user_id = SessionIdentity(Some(&id)).user_id(); match manager.finish_authentication(&user_id, &req.opaque_state, &req.credential) { Ok(_) => { - SessionIdentity(&id).set_status(SessionStatus::SignedIn); + SessionIdentity(Some(&id)).set_status(&http_req, SessionStatus::SignedIn); HttpResponse::Ok().body("You are authenticated!") } Err(e) => { diff --git a/src/controllers/login_controller.rs b/src/controllers/login_controller.rs index 613edc5..4c109f1 100644 --- a/src/controllers/login_controller.rs +++ b/src/controllers/login_controller.rs @@ -1,6 +1,6 @@ use actix::Addr; use actix_identity::Identity; -use actix_web::{HttpResponse, Responder, web}; +use actix_web::{HttpRequest, HttpResponse, Responder, web}; use askama::Template; use crate::actors::{bruteforce_actor, users_actor}; @@ -80,7 +80,8 @@ pub async fn login_route( bruteforce: web::Data>, query: web::Query, req: Option>, - id: Identity, + id: Option, + http_req: HttpRequest, ) -> impl Responder { let mut danger = None; let mut success = None; @@ -97,27 +98,29 @@ pub async fn login_route( // Check if user session must be closed if let Some(true) = query.logout { - id.forget(); + if let Some(id) = id { + id.logout(); + } success = Some("Goodbye!".to_string()); } // Check if user is already authenticated - if SessionIdentity(&id).is_authenticated() { + else if SessionIdentity(id.as_ref()).is_authenticated() { return redirect_user(query.redirect.get()); } // Check if the password of the user has to be changed - if SessionIdentity(&id).need_new_password() { + else if SessionIdentity(id.as_ref()).need_new_password() { return redirect_user(&format!("/reset_password?redirect={}", query.redirect.get_encoded())); } // Check if the user has to valide a second factor - if SessionIdentity(&id).need_2fa_auth() { + else if SessionIdentity(id.as_ref()).need_2fa_auth() { return redirect_user(&format!("/2fa_auth?redirect={}", query.redirect.get_encoded())); } // Try to authenticate user - if let Some(req) = &req { + else if let Some(req) = &req { login = req.login.clone(); let response: LoginResult = users .send(users_actor::LoginRequest { @@ -129,13 +132,13 @@ pub async fn login_route( match response { LoginResult::Success(user) => { - SessionIdentity(&id).set_user(&user); + SessionIdentity(id.as_ref()).set_user(&http_req, &user); return if user.need_reset_password { - SessionIdentity(&id).set_status(SessionStatus::NeedNewPassword); + SessionIdentity(id.as_ref()).set_status(&http_req, SessionStatus::NeedNewPassword); redirect_user(&format!("/reset_password?redirect={}", query.redirect.get_encoded())) } else if user.has_two_factor() { - SessionIdentity(&id).set_status(SessionStatus::Need2FA); + SessionIdentity(id.as_ref()).set_status(&http_req, SessionStatus::Need2FA); redirect_user(&format!("/2fa_auth?redirect={}", query.redirect.get_encoded())) } else { redirect_user(query.redirect.get()) @@ -189,12 +192,13 @@ pub struct PasswordResetQuery { } /// Reset user password route -pub async fn reset_password_route(id: Identity, query: web::Query, +pub async fn reset_password_route(id: Option, query: web::Query, req: Option>, - users: web::Data>) -> impl Responder { + users: web::Data>, + http_req: HttpRequest) -> impl Responder { let mut danger = None; - if !SessionIdentity(&id).need_new_password() { + if !SessionIdentity(id.as_ref()).need_new_password() { return redirect_user_for_login(query.redirect.get()); } @@ -205,7 +209,7 @@ pub async fn reset_password_route(id: Identity, query: web::Query, +pub async fn choose_2fa_method(id: Option, query: web::Query, users: web::Data>) -> impl Responder { - if !SessionIdentity(&id).need_2fa_auth() { + if !SessionIdentity(id.as_ref()).need_2fa_auth() { + log::trace!("User does not require 2fa auth, redirecting"); return redirect_user_for_login(query.redirect.get()); } - let user: User = users.send(users_actor::GetUserRequest(SessionIdentity(&id).user_id())) + let user: User = users.send(users_actor::GetUserRequest(SessionIdentity(id.as_ref()).user_id())) .await.unwrap().0.expect("Could not find user!"); // Automatically choose factor if there is only one factor @@ -290,16 +295,17 @@ pub struct LoginWithOTPForm { /// Login with OTP -pub async fn login_with_otp(id: Identity, query: web::Query, +pub async fn login_with_otp(id: Option, query: web::Query, form: Option>, - users: web::Data>) -> impl Responder { + users: web::Data>, + http_req: HttpRequest) -> impl Responder { let mut danger = None; - if !SessionIdentity(&id).need_2fa_auth() { + if !SessionIdentity(id.as_ref()).need_2fa_auth() { return redirect_user_for_login(query.redirect.get()); } - let user: User = users.send(users_actor::GetUserRequest(SessionIdentity(&id).user_id())) + let user: User = users.send(users_actor::GetUserRequest(SessionIdentity(id.as_ref()).user_id())) .await.unwrap().0.expect("Could not find user!"); let factor = match user.find_factor(&query.id) { @@ -318,7 +324,7 @@ pub async fn login_with_otp(id: Identity, query: web::Query, if !key.check_code(&form.code).unwrap_or(false) { danger = Some("Specified code is invalid!".to_string()); } else { - SessionIdentity(&id).set_status(SessionStatus::SignedIn); + SessionIdentity(id.as_ref()).set_status(&http_req, SessionStatus::SignedIn); return redirect_user(query.redirect.get()); } } @@ -344,14 +350,14 @@ pub struct LoginWithWebauthnQuery { /// Login with Webauthn -pub async fn login_with_webauthn(id: Identity, query: web::Query, +pub async fn login_with_webauthn(id: Option, query: web::Query, manager: WebAuthManagerReq, users: web::Data>) -> impl Responder { - if !SessionIdentity(&id).need_2fa_auth() { + if !SessionIdentity(id.as_ref()).need_2fa_auth() { return redirect_user_for_login(query.redirect.get()); } - let user: User = users.send(users_actor::GetUserRequest(SessionIdentity(&id).user_id())) + let user: User = users.send(users_actor::GetUserRequest(SessionIdentity(id.as_ref()).user_id())) .await.unwrap().0.expect("Could not find user!"); let factor = match user.find_factor(&query.id) { diff --git a/src/controllers/openid_controller.rs b/src/controllers/openid_controller.rs index f25d9e8..05b9a94 100644 --- a/src/controllers/openid_controller.rs +++ b/src/controllers/openid_controller.rs @@ -146,7 +146,7 @@ pub async fn authorize(user: CurrentUser, id: Identity, query: web::Query = user_actor.as_ref().clone(); let identity: Identity = Identity::from_request(req, payload).into_inner() .expect("Failed to get identity!"); - let user_id = SessionIdentity(&identity).user_id(); + let user_id = SessionIdentity(Some(&identity)).user_id(); Box::pin(async move { diff --git a/src/data/session_identity.rs b/src/data/session_identity.rs index 547824e..c0b6b4e 100644 --- a/src/data/session_identity.rs +++ b/src/data/session_identity.rs @@ -1,4 +1,5 @@ use actix_identity::Identity; +use actix_web::{HttpMessage, HttpRequest}; use serde::{Deserialize, Serialize}; use crate::data::user::{User, UserID}; @@ -26,11 +27,16 @@ pub struct SessionIdentityData { pub status: SessionStatus, } -pub struct SessionIdentity<'a>(pub &'a Identity); +pub struct SessionIdentity<'a>(pub Option<&'a Identity>); impl<'a> SessionIdentity<'a> { fn get_session_data(&self) -> Option { - Self::deserialize_session_data(self.0.identity()) + if let Some(id) = self.0 { + Self::deserialize_session_data(id.id().ok()) + } + else { + None + } } pub fn deserialize_session_data(data: Option) -> Option { @@ -54,14 +60,18 @@ impl<'a> SessionIdentity<'a> { res } - fn set_session_data(&self, data: &SessionIdentityData) { + fn set_session_data(&self, req: &HttpRequest, data: &SessionIdentityData) { let s = serde_json::to_string(data).expect("Failed to serialize session data!"); - self.0.remember(s); + log::debug!("Will set user session data."); + if let Err(e) = Identity::login(&req.extensions(), s) { + log::error!("Failed to set session data! {}", e); + } + log::debug!("Did set user session data."); } - pub fn set_user(&self, user: &User) { - self.set_session_data(&SessionIdentityData { + pub fn set_user(&self, req: &HttpRequest, user: &User) { + self.set_session_data(req, &SessionIdentityData { id: Some(user.uid.clone()), is_admin: user.admin, auth_time: time(), @@ -69,10 +79,10 @@ impl<'a> SessionIdentity<'a> { }); } - pub fn set_status(&self, status: SessionStatus) { + pub fn set_status(&self, req: &HttpRequest, status: SessionStatus) { let mut sess = self.get_session_data().unwrap_or_default(); sess.status = status; - self.set_session_data(&sess); + self.set_session_data(req, &sess); } pub fn is_authenticated(&self) -> bool { diff --git a/src/main.rs b/src/main.rs index 9672996..270286f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,10 +1,13 @@ +use core::time::Duration; use std::sync::Arc; use actix::Actor; -use actix_identity::{CookieIdentityPolicy, IdentityService}; +use actix_identity::config::LogoutBehaviour; +use actix_identity::IdentityMiddleware; +use actix_session::SessionMiddleware; +use actix_session::storage::CookieSessionStore; use actix_web::{App, get, HttpResponse, HttpServer, middleware, web}; -use actix_web::cookie::SameSite; -use actix_web::cookie::time::Duration; +use actix_web::cookie::{Key, SameSite}; use actix_web::middleware::Logger; use clap::Parser; @@ -35,7 +38,7 @@ async fn main() -> std::io::Result<()> { // In debug mode only, use dummy token if cfg!(debug_assertions) && config.token_key.is_empty() { - config.token_key = String::from_utf8_lossy(&[32; 32]).to_string(); + config.token_key = String::from_utf8_lossy(&[32; 64]).to_string(); } if !config.storage_path().exists() { @@ -81,12 +84,19 @@ async fn main() -> std::io::Result<()> { .expect("Failed to load clients list!"); clients.apply_environment_variables(); - let policy = CookieIdentityPolicy::new(config.token_key.as_bytes()) - .name(SESSION_COOKIE_NAME) - .secure(config.secure_cookie()) - .visit_deadline(Duration::seconds(MAX_INACTIVITY_DURATION)) - .login_deadline(Duration::seconds(MAX_SESSION_DURATION)) - .same_site(SameSite::Lax); + let session_mw = + SessionMiddleware::builder(CookieSessionStore::default(), + Key::from(config.token_key.as_bytes())) + .cookie_name(SESSION_COOKIE_NAME.to_string()) + .cookie_secure(config.secure_cookie()) + .cookie_same_site(SameSite::Lax) + .build(); + + let identity_middleware = IdentityMiddleware::builder() + .logout_behaviour(LogoutBehaviour::PurgeSession) + .visit_deadline(Some(Duration::from_secs(MAX_INACTIVITY_DURATION))) + .login_deadline(Some(Duration::from_secs(MAX_SESSION_DURATION))) + .build(); App::new() .app_data(web::Data::new(users_actor.clone())) @@ -101,7 +111,8 @@ async fn main() -> std::io::Result<()> { .add(("Permissions-Policy", "interest-cohort=()"))) .wrap(Logger::default()) .wrap(AuthMiddleware {}) - .wrap(IdentityService::new(policy)) + .wrap(identity_middleware) + .wrap(session_mw) // main route .route("/", web::get() diff --git a/src/middlewares/auth_middleware.rs b/src/middlewares/auth_middleware.rs index 97d1b8e..0298dae 100644 --- a/src/middlewares/auth_middleware.rs +++ b/src/middlewares/auth_middleware.rs @@ -4,7 +4,7 @@ use std::future::{Future, ready, Ready}; use std::pin::Pin; use std::rc::Rc; -use actix_identity::RequestIdentity; +use actix_identity::IdentityExt; use actix_web::{ dev::{forward_ready, Service, ServiceRequest, ServiceResponse, Transform}, Error, HttpResponse, web, @@ -114,7 +114,9 @@ impl Service for AuthInnerMiddleware )); } - let session = match SessionIdentity::deserialize_session_data(req.get_identity()) { + let id = req.get_identity().ok().map(|r| r.id().unwrap_or_default()); + let session_data = SessionIdentity::deserialize_session_data(id); + let session = match session_data { Some(SessionIdentityData { status: SessionStatus::SignedIn, is_admin: true, @@ -127,6 +129,9 @@ impl Service for AuthInnerMiddleware _ => ConnStatus::SignedOut, }; + log::trace!("Connection data: {:#?}", session_data); + log::debug!("Connection status: {:?}", session); + // Redirect user to login page if !session.is_auth() && (req.path().starts_with(ADMIN_ROUTES) From 24240ca4fd3ed31ed1ca435829a2c5b55be5d599 Mon Sep 17 00:00:00 2001 From: Pierre Hubert Date: Fri, 22 Jul 2022 14:28:44 +0200 Subject: [PATCH 3/4] Refactor code --- src/controllers/login_controller.rs | 16 ++++++++-------- src/data/session_identity.rs | 4 ++-- src/middlewares/auth_middleware.rs | 3 +++ 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/controllers/login_controller.rs b/src/controllers/login_controller.rs index 4c109f1..4948408 100644 --- a/src/controllers/login_controller.rs +++ b/src/controllers/login_controller.rs @@ -132,17 +132,16 @@ pub async fn login_route( match response { LoginResult::Success(user) => { - SessionIdentity(id.as_ref()).set_user(&http_req, &user); - - return if user.need_reset_password { - SessionIdentity(id.as_ref()).set_status(&http_req, SessionStatus::NeedNewPassword); - redirect_user(&format!("/reset_password?redirect={}", query.redirect.get_encoded())) + let status = if user.need_reset_password { + SessionStatus::NeedNewPassword } else if user.has_two_factor() { - SessionIdentity(id.as_ref()).set_status(&http_req, SessionStatus::Need2FA); - redirect_user(&format!("/2fa_auth?redirect={}", query.redirect.get_encoded())) + SessionStatus::Need2FA } else { - redirect_user(query.redirect.get()) + SessionStatus::SignedIn }; + + SessionIdentity(id.as_ref()).set_user(&http_req, &user, status); + redirect_user(query.redirect.get()); } LoginResult::AccountDisabled => { @@ -262,6 +261,7 @@ pub async fn choose_2fa_method(id: Option, query: web::Query SessionIdentity<'a> { log::debug!("Did set user session data."); } - pub fn set_user(&self, req: &HttpRequest, user: &User) { + pub fn set_user(&self, req: &HttpRequest, user: &User, status: SessionStatus) { self.set_session_data(req, &SessionIdentityData { id: Some(user.uid.clone()), is_admin: user.admin, auth_time: time(), - status: SessionStatus::SignedIn, + status, }); } diff --git a/src/middlewares/auth_middleware.rs b/src/middlewares/auth_middleware.rs index 0298dae..ea4e735 100644 --- a/src/middlewares/auth_middleware.rs +++ b/src/middlewares/auth_middleware.rs @@ -137,6 +137,9 @@ impl Service for AuthInnerMiddleware && (req.path().starts_with(ADMIN_ROUTES) || req.path().starts_with(AUTHENTICATED_ROUTES) || req.path().eq(AUTHORIZE_URI)) { + log::debug!("Redirect unauthenticated user from {} to authorization route.", + req.path()); + let path = req.uri().to_string(); return Ok(req .into_response(redirect_user_for_login(path)) From 8679b1c367d98de60dd78b723fd7f3480fff3a49 Mon Sep 17 00:00:00 2001 From: Pierre Hubert Date: Fri, 22 Jul 2022 14:36:39 +0200 Subject: [PATCH 4/4] Fix redirection issue after login --- src/controllers/login_controller.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controllers/login_controller.rs b/src/controllers/login_controller.rs index 4948408..6386515 100644 --- a/src/controllers/login_controller.rs +++ b/src/controllers/login_controller.rs @@ -141,7 +141,7 @@ pub async fn login_route( }; SessionIdentity(id.as_ref()).set_user(&http_req, &user, status); - redirect_user(query.redirect.get()); + return redirect_user(query.redirect.get()); } LoginResult::AccountDisabled => {