Merge pull request 'Fix potential timing side-channels in authentication mechanisms' (#737) from fix-auth-ct-eq into mainv0.9.2-rc1

Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/737

6 files changed, 109 insertions, 21 deletions

diff --git a/Cargo.lock b/Cargo.lock
index 38cc5e1f..baa2f8a3 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -121,6 +121,18 @@ 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"
 source = "registry+https://github.com/rust-lang/crates.io-index"
@@ -1321,6 +1333,7 @@ dependencies = [
 name = "garage_api"
 version = "0.9.1"
 dependencies = [
+ "argon2",
  "async-trait",
  "base64 0.21.7",
  "bytes",
@@ -2800,6 +2813,17 @@ dependencies = [
 ]
 
 [[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"
 source = "registry+https://github.com/rust-lang/crates.io-index"
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<IncomingBody>,
 		endpoint: Endpoint,
 	) -> Result<Response<ResBody>, 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(())
+}
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)