From 2b6c2c5bc014c368dca0241700f7c9871e9b4699 Mon Sep 17 00:00:00 2001 From: Luca P Date: Sat, 12 Mar 2022 00:45:26 +0000 Subject: [PATCH] Add login form with errors via HMAC-protected query parameters --- Cargo.lock | 43 +++++++++- Cargo.toml | 5 ++ configuration/base.yaml | 1 + src/authentication.rs | 90 +++++++++++++++++++++ src/configuration.rs | 1 + src/lib.rs | 1 + src/routes/home/home.html | 10 +++ src/routes/home/mod.rs | 7 ++ src/routes/login/get.rs | 77 ++++++++++++++++++ src/routes/login/login.html | 28 +++++++ src/routes/login/mod.rs | 5 ++ src/routes/login/post.rs | 74 +++++++++++++++++ src/routes/mod.rs | 4 + src/routes/newsletters.rs | 157 ++++++++++-------------------------- src/startup.rs | 14 +++- 15 files changed, 398 insertions(+), 119 deletions(-) create mode 100644 src/authentication.rs create mode 100644 src/routes/home/home.html create mode 100644 src/routes/home/mod.rs create mode 100644 src/routes/login/get.rs create mode 100644 src/routes/login/login.html create mode 100644 src/routes/login/mod.rs create mode 100644 src/routes/login/post.rs diff --git a/Cargo.lock b/Cargo.lock index f1bd953..1d9e524 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -945,6 +945,21 @@ dependencies = [ "digest 0.9.0", ] +[[package]] +name = "hmac" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6c49c37c09c17a53d937dfbb742eb3a961d65a994e6bcdcf37e7399d0cc8ab5e" +dependencies = [ + "digest 0.10.3", +] + +[[package]] +name = "htmlescape" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e9025058dae765dee5070ec375f591e2ba14638c63feff74f13805a72e523163" + [[package]] name = "http" version = "0.2.6" @@ -1869,6 +1884,17 @@ dependencies = [ "opaque-debug", ] +[[package]] +name = "sha2" +version = "0.10.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "55deaec60f81eefe3cce0dc50bda92d6d8e88f2a27df7c5033b42afeb1ed2676" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest 0.10.3", +] + [[package]] name = "sharded-slab" version = "0.1.4" @@ -1959,7 +1985,7 @@ dependencies = [ "futures-util", "hashlink", "hex", - "hmac", + "hmac 0.11.0", "indexmap", "itoa", "libc", @@ -1974,7 +2000,7 @@ dependencies = [ "serde", "serde_json", "sha-1 0.9.8", - "sha2", + "sha2 0.9.9", "smallvec", "sqlformat", "sqlx-rt", @@ -2003,7 +2029,7 @@ dependencies = [ "quote", "serde", "serde_json", - "sha2", + "sha2 0.9.9", "sqlx-core", "sqlx-rt", "syn", @@ -2393,6 +2419,12 @@ dependencies = [ "serde", ] +[[package]] +name = "urlencoding" +version = "2.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "68b90931029ab9b034b300b797048cf23723400aa757e8a2bfb9d748102f9821" + [[package]] name = "uuid" version = "0.8.2" @@ -2708,6 +2740,9 @@ dependencies = [ "claim", "config", "fake", + "hex", + "hmac 0.12.1", + "htmlescape", "linkify", "log", "once_cell", @@ -2719,6 +2754,7 @@ dependencies = [ "serde", "serde-aux", "serde_json", + "sha2 0.10.2", "sqlx", "thiserror", "tokio", @@ -2728,6 +2764,7 @@ dependencies = [ "tracing-log", "tracing-subscriber", "unicode-segmentation", + "urlencoding", "uuid", "validator", "wiremock", diff --git a/Cargo.toml b/Cargo.toml index e6814f9..b6cd6e0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,6 +36,11 @@ base64 = "0.13.0" argon2 = { version = "0.3", features = ["std"] } tracing-actix-web = "0.5" secrecy = { version = "0.8", features = ["serde"] } +urlencoding = "2" +htmlescape = "0.3" +hmac = { version = "0.12", features = ["std"] } +sha2 = "0.10" +hex = "0.4" [dev-dependencies] once_cell = "1.7.2" diff --git a/configuration/base.yaml b/configuration/base.yaml index f890bdf..9608b08 100644 --- a/configuration/base.yaml +++ b/configuration/base.yaml @@ -1,6 +1,7 @@ application: port: 8000 host: 0.0.0.0 + hmac_secret: "super-long-and-secret-random-key-needed-to-verify-message-integrity" database: host: "127.0.0.1" port: 5432 diff --git a/src/authentication.rs b/src/authentication.rs new file mode 100644 index 0000000..f08498f --- /dev/null +++ b/src/authentication.rs @@ -0,0 +1,90 @@ +use anyhow::Context; +use argon2::{Argon2, PasswordHash, PasswordVerifier}; +use secrecy::{ExposeSecret, Secret}; +use sqlx::PgPool; + +use crate::telemetry::spawn_blocking_with_tracing; + +#[derive(thiserror::Error, Debug)] +pub enum AuthError { + #[error("Invalid credentials.")] + InvalidCredentials(#[source] anyhow::Error), + #[error(transparent)] + UnexpectedError(#[from] anyhow::Error), +} + +pub struct Credentials { + pub username: String, + pub password: Secret, +} + +#[tracing::instrument(name = "Get stored credentials", skip(username, pool))] +async fn get_stored_credentials( + username: &str, + pool: &PgPool, +) -> Result)>, anyhow::Error> { + let row = sqlx::query!( + r#" + SELECT user_id, password_hash + FROM users + WHERE username = $1 + "#, + username, + ) + .fetch_optional(pool) + .await + .context("Failed to performed a query to retrieve stored credentials.")? + .map(|row| (row.user_id, Secret::new(row.password_hash))); + Ok(row) +} + +#[tracing::instrument(name = "Validate credentials", skip(credentials, pool))] +pub async fn validate_credentials( + credentials: Credentials, + pool: &PgPool, +) -> Result { + let mut user_id = None; + let mut expected_password_hash = Secret::new( + "$argon2id$v=19$m=15000,t=2,p=1$\ + gZiV/M1gPc22ElAH/Jh1Hw$\ + CWOrkoo7oJBQ/iyh7uJ0LO2aLEfrHwTWllSAxT0zRno" + .to_string(), + ); + + if let Some((stored_user_id, stored_password_hash)) = + get_stored_credentials(&credentials.username, pool).await? + { + user_id = Some(stored_user_id); + expected_password_hash = stored_password_hash; + } + + spawn_blocking_with_tracing(move || { + verify_password_hash(expected_password_hash, credentials.password) + }) + .await + .context("Failed to spawn blocking task.")??; + + user_id + .ok_or_else(|| anyhow::anyhow!("Unknown username.")) + .map_err(AuthError::InvalidCredentials) +} + +#[tracing::instrument( + name = "Validate credentials", + skip(expected_password_hash, password_candidate) +)] +fn verify_password_hash( + expected_password_hash: Secret, + password_candidate: Secret, +) -> Result<(), AuthError> { + let expected_password_hash = PasswordHash::new(expected_password_hash.expose_secret()) + .context("Failed to parse hash in PHC string format.")?; + + Argon2::default() + .verify_password( + password_candidate.expose_secret().as_bytes(), + &expected_password_hash, + ) + .context("Invalid password.") + .map_err(AuthError::InvalidCredentials) +} diff --git a/src/configuration.rs b/src/configuration.rs index 0576dbc..ab41bbe 100644 --- a/src/configuration.rs +++ b/src/configuration.rs @@ -18,6 +18,7 @@ pub struct ApplicationSettings { pub port: u16, pub host: String, pub base_url: String, + pub hmac_secret: Secret, } #[derive(serde::Deserialize, Clone)] diff --git a/src/lib.rs b/src/lib.rs index 66e386a..1bb9b5c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,3 +1,4 @@ +pub mod authentication; pub mod configuration; pub mod domain; pub mod email_client; diff --git a/src/routes/home/home.html b/src/routes/home/home.html new file mode 100644 index 0000000..985e625 --- /dev/null +++ b/src/routes/home/home.html @@ -0,0 +1,10 @@ + + + + + Home + + +

Welcome to our newsletter!

+ + \ No newline at end of file diff --git a/src/routes/home/mod.rs b/src/routes/home/mod.rs new file mode 100644 index 0000000..92f809a --- /dev/null +++ b/src/routes/home/mod.rs @@ -0,0 +1,7 @@ +use actix_web::{http::header::ContentType, HttpResponse}; + +pub async fn home() -> HttpResponse { + HttpResponse::Ok() + .content_type(ContentType::html()) + .body(include_str!("home.html")) +} diff --git a/src/routes/login/get.rs b/src/routes/login/get.rs new file mode 100644 index 0000000..b8f0c10 --- /dev/null +++ b/src/routes/login/get.rs @@ -0,0 +1,77 @@ +use crate::startup::HmacSecret; +use actix_web::{http::header::ContentType, web, HttpResponse}; +use hmac::{Hmac, Mac}; +use secrecy::ExposeSecret; + +#[derive(serde::Deserialize)] +pub struct QueryParams { + error: String, + tag: String, +} + +impl QueryParams { + fn verify(self, secret: &HmacSecret) -> Result { + let tag = hex::decode(self.tag)?; + let query_string = format!("error={}", urlencoding::Encoded::new(&self.error)); + + let mut mac = + Hmac::::new_from_slice(secret.0.expose_secret().as_bytes()).unwrap(); + mac.update(query_string.as_bytes()); + mac.verify_slice(&tag)?; + + Ok(self.error) + } +} + +pub async fn login_form( + query: Option>, + secret: web::Data, +) -> HttpResponse { + let error_html = match query { + None => "".into(), + Some(query) => match query.0.verify(&secret) { + Ok(error) => { + format!("

{}

", htmlescape::encode_minimal(&error)) + } + Err(e) => { + tracing::warn!( + error.message = %e, + error.cause_chain = ?e, + "Failed to verify query parameters using the HMAC tag" + ); + "".into() + } + }, + }; + HttpResponse::Ok() + .content_type(ContentType::html()) + .body(format!( + r#" + + + + Login + + + {error_html} +
+ + + +
+ +"#, + )) +} diff --git a/src/routes/login/login.html b/src/routes/login/login.html new file mode 100644 index 0000000..67a0e9e --- /dev/null +++ b/src/routes/login/login.html @@ -0,0 +1,28 @@ + + + + + Login + + +
+ + + + + +
+ + \ No newline at end of file diff --git a/src/routes/login/mod.rs b/src/routes/login/mod.rs new file mode 100644 index 0000000..11fbf41 --- /dev/null +++ b/src/routes/login/mod.rs @@ -0,0 +1,5 @@ +mod get; +mod post; + +pub use get::login_form; +pub use post::login; diff --git a/src/routes/login/post.rs b/src/routes/login/post.rs new file mode 100644 index 0000000..3904bb9 --- /dev/null +++ b/src/routes/login/post.rs @@ -0,0 +1,74 @@ +use crate::authentication::AuthError; +use crate::authentication::{validate_credentials, Credentials}; +use crate::routes::error_chain_fmt; +use crate::startup::HmacSecret; +use actix_web::error::InternalError; +use actix_web::http::header::LOCATION; +use actix_web::web; +use actix_web::HttpResponse; +use hmac::{Hmac, Mac}; +use secrecy::{ExposeSecret, Secret}; +use sqlx::PgPool; + +#[derive(serde::Deserialize)] +pub struct FormData { + username: String, + password: Secret, +} + +#[tracing::instrument( + skip(form, pool, secret), + fields(username=tracing::field::Empty, user_id=tracing::field::Empty) +)] +// We are now injecting `PgPool` to retrieve stored credentials from the database +pub async fn login( + form: web::Form, + pool: web::Data, + secret: web::Data, +) -> Result> { + let credentials = Credentials { + username: form.0.username, + password: form.0.password, + }; + tracing::Span::current().record("username", &tracing::field::display(&credentials.username)); + match validate_credentials(credentials, &pool).await { + Ok(user_id) => { + tracing::Span::current().record("user_id", &tracing::field::display(&user_id)); + Ok(HttpResponse::SeeOther() + .insert_header((LOCATION, "/")) + .finish()) + } + Err(e) => { + let e = match e { + AuthError::InvalidCredentials(_) => LoginError::AuthError(e.into()), + AuthError::UnexpectedError(_) => LoginError::UnexpectedError(e.into()), + }; + let query_string = format!("error={}", urlencoding::Encoded::new(e.to_string())); + let hmac_tag = { + let mut mac = + Hmac::::new_from_slice(secret.0.expose_secret().as_bytes()) + .unwrap(); + mac.update(query_string.as_bytes()); + mac.finalize().into_bytes() + }; + let response = HttpResponse::SeeOther() + .insert_header((LOCATION, format!("/login?{query_string}&tag={hmac_tag:x}"))) + .finish(); + Err(InternalError::from_response(e, response)) + } + } +} + +#[derive(thiserror::Error)] +pub enum LoginError { + #[error("Authentication failed")] + AuthError(#[source] anyhow::Error), + #[error("Something went wrong")] + UnexpectedError(#[from] anyhow::Error), +} + +impl std::fmt::Debug for LoginError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + error_chain_fmt(self, f) + } +} diff --git a/src/routes/mod.rs b/src/routes/mod.rs index 1055760..b0d8892 100644 --- a/src/routes/mod.rs +++ b/src/routes/mod.rs @@ -1,9 +1,13 @@ mod health_check; +mod home; +mod login; mod newsletters; mod subscriptions; mod subscriptions_confirm; pub use health_check::*; +pub use home::*; +pub use login::*; pub use newsletters::*; pub use subscriptions::*; pub use subscriptions_confirm::*; diff --git a/src/routes/newsletters.rs b/src/routes/newsletters.rs index 346363a..8964bb7 100644 --- a/src/routes/newsletters.rs +++ b/src/routes/newsletters.rs @@ -1,15 +1,14 @@ -use crate::domain::SubscriberEmail; +use crate::authentication::{validate_credentials, Credentials}; use crate::email_client::EmailClient; use crate::routes::error_chain_fmt; -use crate::telemetry::spawn_blocking_with_tracing; +use crate::{authentication::AuthError, domain::SubscriberEmail}; use actix_web::http::{ header::{HeaderMap, HeaderValue}, StatusCode, }; -use actix_web::{web, HttpResponse, HttpRequest, ResponseError}; +use actix_web::{web, HttpRequest, HttpResponse, ResponseError}; use anyhow::Context; -use argon2::{Argon2, PasswordHash, PasswordVerifier}; -use secrecy::{ExposeSecret, Secret}; +use secrecy::Secret; use sqlx::PgPool; #[derive(serde::Deserialize)] @@ -24,6 +23,37 @@ pub struct Content { text: String, } +pub fn basic_authentication(headers: &HeaderMap) -> Result { + // The header value, if present, must be a valid UTF8 string + let header_value = headers + .get("Authorization") + .context("The 'Authorization' header was missing")? + .to_str() + .context("The 'Authorization' header was not a valid UTF8 string.")?; + let base64encoded_credentials = header_value + .strip_prefix("Basic ") + .context("The authorization scheme was not 'Basic'.")?; + let decoded_credentials = base64::decode_config(base64encoded_credentials, base64::STANDARD) + .context("Failed to base64-decode 'Basic' credentials.")?; + let decoded_credentials = String::from_utf8(decoded_credentials) + .context("The decoded credential string is valid UTF8.")?; + + let mut credentials = decoded_credentials.splitn(2, ':'); + let username = credentials + .next() + .ok_or_else(|| anyhow::anyhow!("A username must be provided in 'Basic' auth."))? + .to_string(); + let password = credentials + .next() + .ok_or_else(|| anyhow::anyhow!("A password must be provided in 'Basic' auth."))? + .to_string(); + + Ok(Credentials { + username, + password: Secret::new(password), + }) +} + #[derive(thiserror::Error)] pub enum PublishError { #[error("Authentication failed.")] @@ -56,115 +86,6 @@ impl ResponseError for PublishError { } } -struct Credentials { - username: String, - password: Secret, -} - -fn basic_authentication(headers: &HeaderMap) -> Result { - // The header value, if present, must be a valid UTF8 string - let header_value = headers - .get("Authorization") - .context("The 'Authorization' header was missing")? - .to_str() - .context("The 'Authorization' header was not a valid UTF8 string.")?; - let base64encoded_credentials = header_value - .strip_prefix("Basic ") - .context("The authorization scheme was not 'Basic'.")?; - let decoded_credentials = base64::decode_config(base64encoded_credentials, base64::STANDARD) - .context("Failed to base64-decode 'Basic' credentials.")?; - let decoded_credentials = String::from_utf8(decoded_credentials) - .context("The decoded credential string is valid UTF8.")?; - - let mut credentials = decoded_credentials.splitn(2, ':'); - let username = credentials - .next() - .ok_or_else(|| anyhow::anyhow!("A username must be provided in 'Basic' auth."))? - .to_string(); - let password = credentials - .next() - .ok_or_else(|| anyhow::anyhow!("A password must be provided in 'Basic' auth."))? - .to_string(); - - Ok(Credentials { - username, - password: Secret::new(password), - }) -} - -#[tracing::instrument(name = "Get stored credentials", skip(username, pool))] -async fn get_stored_credentials( - username: &str, - pool: &PgPool, -) -> Result)>, anyhow::Error> { - let row = sqlx::query!( - r#" - SELECT user_id, password_hash - FROM users - WHERE username = $1 - "#, - username, - ) - .fetch_optional(pool) - .await - .context("Failed to performed a query to retrieve stored credentials.")? - .map(|row| (row.user_id, Secret::new(row.password_hash))); - Ok(row) -} - -#[tracing::instrument(name = "Validate credentials", skip(credentials, pool))] -async fn validate_credentials( - credentials: Credentials, - pool: &PgPool, -) -> Result { - let mut user_id = None; - let mut expected_password_hash = Secret::new( - "$argon2id$v=19$m=15000,t=2,p=1$\ - gZiV/M1gPc22ElAH/Jh1Hw$\ - CWOrkoo7oJBQ/iyh7uJ0LO2aLEfrHwTWllSAxT0zRno" - .to_string(), - ); - - if let Some((stored_user_id, stored_password_hash)) = - get_stored_credentials(&credentials.username, pool) - .await - .map_err(PublishError::UnexpectedError)? - { - user_id = Some(stored_user_id); - expected_password_hash = stored_password_hash; - } - - spawn_blocking_with_tracing(move || { - verify_password_hash(expected_password_hash, credentials.password) - }) - .await - .context("Failed to spawn blocking task.") - .map_err(PublishError::UnexpectedError)??; - - user_id.ok_or_else(|| PublishError::AuthError(anyhow::anyhow!("Unknown username."))) -} - -#[tracing::instrument( - name = "Validate credentials", - skip(expected_password_hash, password_candidate) -)] -fn verify_password_hash( - expected_password_hash: Secret, - password_candidate: Secret, -) -> Result<(), PublishError> { - let expected_password_hash = PasswordHash::new(expected_password_hash.expose_secret()) - .context("Failed to parse hash in PHC string format.") - .map_err(PublishError::UnexpectedError)?; - - Argon2::default() - .verify_password( - password_candidate.expose_secret().as_bytes(), - &expected_password_hash, - ) - .context("Invalid password.") - .map_err(PublishError::AuthError) -} - #[tracing::instrument( name = "Publish a newsletter issue", skip(body, pool, email_client, request), @@ -178,7 +99,13 @@ pub async fn publish_newsletter( ) -> Result { let credentials = basic_authentication(request.headers()).map_err(PublishError::AuthError)?; tracing::Span::current().record("username", &tracing::field::display(&credentials.username)); - let user_id = validate_credentials(credentials, &pool).await?; + let user_id = validate_credentials(credentials, &pool) + .await + .map_err(|e| match e { + AuthError::InvalidCredentials(_) => PublishError::AuthError(e.into()), + AuthError::UnexpectedError(_) => PublishError::UnexpectedError(e.into()), + })?; + tracing::Span::current().record("user_id", &tracing::field::display(&user_id)); let subscribers = get_confirmed_subscribers(&pool).await?; diff --git a/src/startup.rs b/src/startup.rs index b3f95a2..d29ec2e 100644 --- a/src/startup.rs +++ b/src/startup.rs @@ -1,9 +1,12 @@ use crate::configuration::{DatabaseSettings, Settings}; use crate::email_client::EmailClient; -use crate::routes::{confirm, health_check, publish_newsletter, subscribe}; +use crate::routes::{ + confirm, health_check, home, login, login_form, publish_newsletter, subscribe, +}; use actix_web::dev::Server; use actix_web::web::Data; use actix_web::{web, App, HttpServer}; +use secrecy::Secret; use sqlx::postgres::PgPoolOptions; use sqlx::PgPool; use std::net::TcpListener; @@ -43,6 +46,7 @@ impl Application { connection_pool, email_client, configuration.application.base_url, + configuration.application.hmac_secret, )?; Ok(Self { port, server }) @@ -71,6 +75,7 @@ fn run( db_pool: PgPool, email_client: EmailClient, base_url: String, + hmac_secret: Secret, ) -> Result { let db_pool = Data::new(db_pool); let email_client = Data::new(email_client); @@ -78,6 +83,9 @@ fn run( let server = HttpServer::new(move || { App::new() .wrap(TracingLogger::default()) + .route("/", web::get().to(home)) + .route("/login", web::get().to(login_form)) + .route("/login", web::post().to(login)) .route("/health_check", web::get().to(health_check)) .route("/subscriptions", web::post().to(subscribe)) .route("/subscriptions/confirm", web::get().to(confirm)) @@ -85,8 +93,12 @@ fn run( .app_data(db_pool.clone()) .app_data(email_client.clone()) .app_data(base_url.clone()) + .app_data(Data::new(HmacSecret(hmac_secret.clone()))) }) .listen(listener)? .run(); Ok(server) } + +#[derive(Clone)] +pub struct HmacSecret(pub Secret);