From eaac4924ef5c18eb40eabfbf246f5ae9c894889a Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Thu, 29 Feb 2024 10:57:07 +0100 Subject: [PATCH 1/2] [fix-auth-ct-eq] use argon2 hashing and verification for admin/metrics token checking --- Cargo.lock | 24 +++++++++++++++ Cargo.nix | 39 ++++++++++++++++++++++++- Cargo.toml | 1 + src/api/Cargo.toml | 1 + src/api/admin/api_server.rs | 58 ++++++++++++++++++++++++++----------- 5 files changed, 105 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 38cc5e1f..baa2f8a3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -120,6 +120,18 @@ version = "1.6.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bddcadddf5e9015d310179a59bb28c4d4b9920ad0f11e8e14dbadf654890c9a6" +[[package]] +name = "argon2" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3c3610892ee6e0cbce8ae2700349fcf8f98adb0dbfbee85aec3c9179d29cc072" +dependencies = [ + "base64ct", + "blake2", + "cpufeatures", + "password-hash", +] + [[package]] name = "arrayvec" version = "0.5.2" @@ -1321,6 +1333,7 @@ dependencies = [ name = "garage_api" version = "0.9.1" dependencies = [ + "argon2", "async-trait", "base64 0.21.7", "bytes", @@ -2799,6 +2812,17 @@ dependencies = [ "regex", ] +[[package]] +name = "password-hash" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "346f04948ba92c43e8469c1ee6736c7563d71012b17d40745260fe106aac2166" +dependencies = [ + "base64ct", + "rand_core", + "subtle", +] + [[package]] name = "paste" version = "1.0.14" diff --git a/Cargo.nix b/Cargo.nix index dfe01bfe..059cc744 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -34,7 +34,7 @@ args@{ ignoreLockHash, }: let - nixifiedLockHash = "9377d18da3b48658f9d8b2070db135db2d9ac6d9c692d6656948b765348498cc"; + nixifiedLockHash = "69c86fff0acd6c7a9a19dc6966b4cbd48e8a50c5a9fb40b3090ad71aaa5b55d0"; workspaceSrc = if args.workspaceSrc == null then ./. else args.workspaceSrc; currentLockHash = builtins.hashFile "sha256" (workspaceSrc + /Cargo.lock); lockHashIgnored = if ignoreLockHash @@ -235,6 +235,25 @@ in src = fetchCratesIo { inherit name version; sha256 = "bddcadddf5e9015d310179a59bb28c4d4b9920ad0f11e8e14dbadf654890c9a6"; }; }); + "registry+https://github.com/rust-lang/crates.io-index".argon2."0.5.3" = overridableMkRustCrate (profileName: rec { + name = "argon2"; + version = "0.5.3"; + registry = "registry+https://github.com/rust-lang/crates.io-index"; + src = fetchCratesIo { inherit name version; sha256 = "3c3610892ee6e0cbce8ae2700349fcf8f98adb0dbfbee85aec3c9179d29cc072"; }; + features = builtins.concatLists [ + [ "alloc" ] + [ "default" ] + [ "password-hash" ] + [ "rand" ] + ]; + dependencies = { + base64ct = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".base64ct."1.6.0" { inherit profileName; }).out; + blake2 = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".blake2."0.10.6" { inherit profileName; }).out; + ${ if hostPlatform.parsed.cpu.name == "i686" || hostPlatform.parsed.cpu.name == "x86_64" then "cpufeatures" else null } = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".cpufeatures."0.2.12" { inherit profileName; }).out; + password_hash = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".password-hash."0.5.0" { inherit profileName; }).out; + }; + }); + "registry+https://github.com/rust-lang/crates.io-index".arrayvec."0.5.2" = overridableMkRustCrate (profileName: rec { name = "arrayvec"; version = "0.5.2"; @@ -1939,6 +1958,7 @@ in (lib.optional (rootFeatures' ? "garage/default" || rootFeatures' ? "garage/metrics" || rootFeatures' ? "garage_api/metrics" || rootFeatures' ? "garage_api/prometheus") "prometheus") ]; dependencies = { + argon2 = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".argon2."0.5.3" { inherit profileName; }).out; async_trait = (buildRustPackages."registry+https://github.com/rust-lang/crates.io-index".async-trait."0.1.77" { profileName = "__noProfile"; }).out; base64 = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".base64."0.21.7" { inherit profileName; }).out; bytes = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".bytes."1.5.0" { inherit profileName; }).out; @@ -3989,6 +4009,23 @@ in }; }); + "registry+https://github.com/rust-lang/crates.io-index".password-hash."0.5.0" = overridableMkRustCrate (profileName: rec { + name = "password-hash"; + version = "0.5.0"; + registry = "registry+https://github.com/rust-lang/crates.io-index"; + src = fetchCratesIo { inherit name version; sha256 = "346f04948ba92c43e8469c1ee6736c7563d71012b17d40745260fe106aac2166"; }; + features = builtins.concatLists [ + [ "alloc" ] + [ "default" ] + [ "rand_core" ] + ]; + dependencies = { + base64ct = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".base64ct."1.6.0" { inherit profileName; }).out; + rand_core = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".rand_core."0.6.4" { inherit profileName; }).out; + subtle = (rustPackages."registry+https://github.com/rust-lang/crates.io-index".subtle."2.5.0" { inherit profileName; }).out; + }; + }); + "registry+https://github.com/rust-lang/crates.io-index".paste."1.0.14" = overridableMkRustCrate (profileName: rec { name = "paste"; version = "1.0.14"; diff --git a/Cargo.toml b/Cargo.toml index b7ffcfc5..07e06440 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,7 @@ k2v-client = { version = "0.0.4", path = "src/k2v-client" } # External crates from crates.io arc-swap = "1.0" +argon2 = "0.5" async-trait = "0.1.7" backtrace = "0.3" base64 = "0.21" diff --git a/src/api/Cargo.toml b/src/api/Cargo.toml index bc6b6aa7..cb87d9e1 100644 --- a/src/api/Cargo.toml +++ b/src/api/Cargo.toml @@ -21,6 +21,7 @@ garage_net.workspace = true garage_util.workspace = true garage_rpc.workspace = true +argon2.workspace = true async-trait.workspace = true base64.workspace = true bytes.workspace = true diff --git a/src/api/admin/api_server.rs b/src/api/admin/api_server.rs index 50813d11..265639c4 100644 --- a/src/api/admin/api_server.rs +++ b/src/api/admin/api_server.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::sync::Arc; +use argon2::password_hash::PasswordHash; use async_trait::async_trait; use http::header::{ACCESS_CONTROL_ALLOW_METHODS, ACCESS_CONTROL_ALLOW_ORIGIN, ALLOW}; @@ -45,14 +46,8 @@ impl AdminApiServer { #[cfg(feature = "metrics")] exporter: PrometheusExporter, ) -> Self { let cfg = &garage.config.admin; - let metrics_token = cfg - .metrics_token - .as_ref() - .map(|tok| format!("Bearer {}", tok)); - let admin_token = cfg - .admin_token - .as_ref() - .map(|tok| format!("Bearer {}", tok)); + let metrics_token = cfg.metrics_token.as_deref().map(hash_bearer_token); + let admin_token = cfg.admin_token.as_deref().map(hash_bearer_token); Self { garage, #[cfg(feature = "metrics")] @@ -248,11 +243,11 @@ impl ApiHandler for AdminApiServer { req: Request, endpoint: Endpoint, ) -> Result, Error> { - let expected_auth_header = + let required_auth_hash = match endpoint.authorization_type() { Authorization::None => None, - Authorization::MetricsToken => self.metrics_token.as_ref(), - Authorization::AdminToken => match &self.admin_token { + Authorization::MetricsToken => self.metrics_token.as_deref(), + Authorization::AdminToken => match self.admin_token.as_deref() { None => return Err(Error::forbidden( "Admin token isn't configured, admin API access is disabled for security.", )), @@ -260,14 +255,11 @@ impl ApiHandler for AdminApiServer { }, }; - if let Some(h) = expected_auth_header { + if let Some(password_hash) = required_auth_hash { match req.headers().get("Authorization") { None => return Err(Error::forbidden("Authorization token must be provided")), - Some(v) => { - let authorized = v.to_str().map(|hv| hv.trim() == h).unwrap_or(false); - if !authorized { - return Err(Error::forbidden("Invalid authorization token provided")); - } + Some(authorization) => { + verify_bearer_token(&authorization, password_hash)?; } } } @@ -342,3 +334,35 @@ impl ApiEndpoint for Endpoint { fn add_span_attributes(&self, _span: SpanRef<'_>) {} } + +fn hash_bearer_token(token: &str) -> String { + use argon2::{ + password_hash::{rand_core::OsRng, PasswordHasher, SaltString}, + Argon2, + }; + + let salt = SaltString::generate(&mut OsRng); + let argon2 = Argon2::default(); + argon2 + .hash_password(token.trim().as_bytes(), &salt) + .expect("could not hash API token") + .to_string() +} + +fn verify_bearer_token(token: &hyper::http::HeaderValue, password_hash: &str) -> Result<(), Error> { + use argon2::{password_hash::PasswordVerifier, Argon2}; + + let parsed_hash = PasswordHash::new(&password_hash).unwrap(); + + token + .to_str()? + .strip_prefix("Bearer ") + .and_then(|token| { + Argon2::default() + .verify_password(token.trim().as_bytes(), &parsed_hash) + .ok() + }) + .ok_or_else(|| Error::forbidden("Invalid authorization token"))?; + + Ok(()) +} From 6d33e721c41bdb0fe7da6404e6d6d32509eed6be Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Thu, 29 Feb 2024 12:43:25 +0100 Subject: [PATCH 2/2] [fix-auth-ct-eq] use consant time comparison for awsv4 signature verification --- src/api/signature/payload.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/api/signature/payload.rs b/src/api/signature/payload.rs index 949da601..a9e7d34d 100644 --- a/src/api/signature/payload.rs +++ b/src/api/signature/payload.rs @@ -375,9 +375,10 @@ pub async fn verify_v4( ) .ok_or_internal_error("Unable to build signing HMAC")?; hmac.update(payload); - let our_signature = hex::encode(hmac.finalize().into_bytes()); - if auth.signature != our_signature { - return Err(Error::forbidden("Invalid signature".to_string())); + let signature = + hex::decode(&auth.signature).map_err(|_| Error::forbidden("Invalid signature"))?; + if hmac.verify_slice(&signature).is_err() { + return Err(Error::forbidden("Invalid signature")); } Ok(key)