From 6177f5e5551c2e06a551852b3d17364fbd1197a6 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Mon, 16 Mar 2026 12:36:21 +0530 Subject: [PATCH 1/4] Use constant-time comparison for token and credential verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace standard == comparisons with subtle::ConstantTimeEq in the three places that verify secrets: tstoken signature in proxy.rs, clear-URL signature in http_util.rs, and Basic Auth credentials in auth.rs. The auth fix also removes the && short-circuit that created a username- existence oracle — both username and password are now always evaluated using bitwise & on subtle::Choice values. Adds log::warn on auth failure (path only, no credentials) and five targeted tests covering tampered tokens, wrong-username-right-password, and empty tokens. Closes #410 --- Cargo.lock | 1 + Cargo.toml | 1 + crates/common/Cargo.toml | 1 + crates/common/src/auth.rs | 39 +++++++++++++++++++++++++++++++++- crates/common/src/http_util.rs | 31 ++++++++++++++++++++++++++- crates/common/src/proxy.rs | 27 ++++++++++++++++++++++- 6 files changed, 97 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 459fbb9b..b9310e78 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2369,6 +2369,7 @@ dependencies = [ "serde", "serde_json", "sha2 0.10.9", + "subtle", "temp-env", "tokio", "tokio-test", diff --git a/Cargo.toml b/Cargo.toml index 3ce93495..fbf036dd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -76,6 +76,7 @@ tokio-test = "0.4" toml = "0.9.8" url = "2.5.8" urlencoding = "2.1" +subtle = "2" uuid = { version = "1.18", features = ["v4"] } validator = { version = "0.20", features = ["derive"] } which = "8" diff --git a/crates/common/Cargo.toml b/crates/common/Cargo.toml index c360474e..cb7dedc1 100644 --- a/crates/common/Cargo.toml +++ b/crates/common/Cargo.toml @@ -40,6 +40,7 @@ regex = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } sha2 = { workspace = true } +subtle = { workspace = true } tokio = { workspace = true } toml = { workspace = true } trusted-server-js = { path = "../js" } diff --git a/crates/common/src/auth.rs b/crates/common/src/auth.rs index ba669e6a..ac332e67 100644 --- a/crates/common/src/auth.rs +++ b/crates/common/src/auth.rs @@ -1,6 +1,7 @@ use base64::{engine::general_purpose::STANDARD, Engine as _}; use fastly::http::{header, StatusCode}; use fastly::{Request, Response}; +use subtle::ConstantTimeEq; use crate::settings::Settings; @@ -14,9 +15,14 @@ pub fn enforce_basic_auth(settings: &Settings, req: &Request) -> Option return Some(unauthorized_response()), }; - if username == handler.username && password == handler.password { + // Use bitwise & (not &&) so both sides always evaluate — eliminates the + // username-existence oracle that short-circuit evaluation would create. + let username_ok = username.as_bytes().ct_eq(handler.username.as_bytes()); + let password_ok = password.as_bytes().ct_eq(handler.password.as_bytes()); + if (username_ok & password_ok).into() { None } else { + log::warn!("Basic auth failed for path: {}", req.get_path()); Some(unauthorized_response()) } } @@ -109,6 +115,37 @@ mod tests { assert_eq!(response.get_status(), StatusCode::UNAUTHORIZED); } + #[test] + fn challenge_when_username_wrong_password_correct() { + // Validates that both fields are always evaluated — no short-circuit username oracle. + let settings = settings_with_handlers(); + let mut req = Request::new(Method::GET, "https://example.com/secure/data"); + let token = STANDARD.encode("wrong-user:pass"); + req.set_header(header::AUTHORIZATION, format!("Basic {token}")); + + let response = enforce_basic_auth(&settings, &req).expect("should challenge"); + assert_eq!( + response.get_status(), + StatusCode::UNAUTHORIZED, + "should reject wrong username even with correct password" + ); + } + + #[test] + fn challenge_when_username_correct_password_wrong() { + let settings = settings_with_handlers(); + let mut req = Request::new(Method::GET, "https://example.com/secure/data"); + let token = STANDARD.encode("user:wrong-pass"); + req.set_header(header::AUTHORIZATION, format!("Basic {token}")); + + let response = enforce_basic_auth(&settings, &req).expect("should challenge"); + assert_eq!( + response.get_status(), + StatusCode::UNAUTHORIZED, + "should reject correct username with wrong password" + ); + } + #[test] fn challenge_when_scheme_is_not_basic() { let settings = settings_with_handlers(); diff --git a/crates/common/src/http_util.rs b/crates/common/src/http_util.rs index 931c894f..a4c88a92 100644 --- a/crates/common/src/http_util.rs +++ b/crates/common/src/http_util.rs @@ -3,6 +3,7 @@ use chacha20poly1305::{aead::Aead, aead::KeyInit, XChaCha20Poly1305, XNonce}; use fastly::http::{header, StatusCode}; use fastly::{Request, Response}; use sha2::{Digest, Sha256}; +use subtle::ConstantTimeEq; use crate::constants::INTERNAL_HEADERS; use crate::settings::Settings; @@ -287,7 +288,8 @@ pub fn sign_clear_url(settings: &Settings, clear_url: &str) -> String { /// Verify a `tstoken` for the given clear-text URL. #[must_use] pub fn verify_clear_url_signature(settings: &Settings, clear_url: &str, token: &str) -> bool { - sign_clear_url(settings, clear_url) == token + let expected = sign_clear_url(settings, clear_url); + expected.as_bytes().ct_eq(token.as_bytes()).into() } /// Compute tstoken for the new proxy scheme: SHA-256 of the encrypted full URL (including query). @@ -354,6 +356,33 @@ mod tests { )); } + #[test] + fn verify_clear_url_rejects_tampered_token() { + let settings = crate::test_support::tests::create_test_settings(); + let url = "https://cdn.example/a.png?x=1"; + let valid_token = sign_clear_url(&settings, url); + + // Flip one bit in the first byte — same URL, same length, wrong bytes + let mut tampered = valid_token.into_bytes(); + tampered[0] ^= 0x01; + let tampered = + String::from_utf8(tampered).expect("should be valid utf8 after single-bit flip"); + + assert!( + !verify_clear_url_signature(&settings, url, &tampered), + "should reject token with tampered bytes" + ); + } + + #[test] + fn verify_clear_url_rejects_empty_token() { + let settings = crate::test_support::tests::create_test_settings(); + assert!( + !verify_clear_url_signature(&settings, "https://cdn.example/a.png", ""), + "should reject empty token" + ); + } + // RequestInfo tests #[test] diff --git a/crates/common/src/proxy.rs b/crates/common/src/proxy.rs index 99db6328..d46f161d 100644 --- a/crates/common/src/proxy.rs +++ b/crates/common/src/proxy.rs @@ -4,6 +4,7 @@ use fastly::http::{header, HeaderValue, Method, StatusCode}; use fastly::{Request, Response}; use serde::{Deserialize, Serialize}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use subtle::ConstantTimeEq; use crate::constants::{ HEADER_ACCEPT, HEADER_ACCEPT_ENCODING, HEADER_ACCEPT_LANGUAGE, HEADER_REFERER, @@ -1052,7 +1053,8 @@ fn reconstruct_and_validate_signed_target( }; let expected = compute_encrypted_sha256_token(settings, &full_for_token); - if expected != sig { + let valid: bool = expected.as_bytes().ct_eq(sig.as_bytes()).into(); + if !valid { return Err(Report::new(TrustedServerError::Proxy { message: "invalid tstoken".to_string(), })); @@ -1238,6 +1240,29 @@ mod tests { assert_eq!(err.current_context().status_code(), StatusCode::BAD_GATEWAY); } + #[tokio::test] + async fn reconstruct_rejects_tampered_tstoken() { + let settings = create_test_settings(); + let tsurl = "https://cdn.example/asset.js"; + let tsurl_encoded = + url::form_urlencoded::byte_serialize(tsurl.as_bytes()).collect::(); + // Syntactically valid base64url token of the right length, but not the correct signature + let bad_token = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; + let url = format!( + "https://edge.example/first-party/proxy?tsurl={}&tstoken={}", + tsurl_encoded, bad_token + ); + + let err: Report = + reconstruct_and_validate_signed_target(&settings, &url) + .expect_err("should reject tampered token"); + assert_eq!( + err.current_context().status_code(), + StatusCode::BAD_GATEWAY, + "should return 502 for invalid tstoken" + ); + } + #[tokio::test] async fn click_missing_params_returns_400() { let settings = create_test_settings(); From 0d5376ef69964bb2503b13830d9e2bbb1e677d8e Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Tue, 17 Mar 2026 13:30:41 +0530 Subject: [PATCH 2/4] Address PR review feedback on constant-time comparison hardening --- Cargo.toml | 2 +- crates/common/src/auth.rs | 9 ++++++++- crates/common/src/http_util.rs | 8 ++++++-- crates/common/src/proxy.rs | 8 ++++++-- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index fbf036dd..6a7c8570 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -70,13 +70,13 @@ regex = "1.12.3" serde = { version = "1.0", features = ["derive"] } serde_json = "1.0.149" sha2 = "0.10.9" +subtle = "2" temp-env = "0.3.6" tokio = { version = "1.49", features = ["sync", "macros", "io-util", "rt", "time"] } tokio-test = "0.4" toml = "0.9.8" url = "2.5.8" urlencoding = "2.1" -subtle = "2" uuid = { version = "1.18", features = ["v4"] } validator = { version = "0.20", features = ["derive"] } which = "8" diff --git a/crates/common/src/auth.rs b/crates/common/src/auth.rs index ac332e67..18876706 100644 --- a/crates/common/src/auth.rs +++ b/crates/common/src/auth.rs @@ -1,12 +1,19 @@ use base64::{engine::general_purpose::STANDARD, Engine as _}; use fastly::http::{header, StatusCode}; use fastly::{Request, Response}; -use subtle::ConstantTimeEq; +use subtle::ConstantTimeEq as _; use crate::settings::Settings; const BASIC_AUTH_REALM: &str = r#"Basic realm="Trusted Server""#; +/// Enforce HTTP Basic Authentication for paths that require credentials. +/// +/// Returns `None` if the request is authorized (or the path has no handler), +/// `Some(401 response)` otherwise. Uses constant-time comparison for both +/// username and password, and evaluates both regardless of individual match +/// results to prevent timing oracles. +#[must_use] pub fn enforce_basic_auth(settings: &Settings, req: &Request) -> Option { let handler = settings.handler_for_path(req.get_path())?; diff --git a/crates/common/src/http_util.rs b/crates/common/src/http_util.rs index a4c88a92..3f81aef3 100644 --- a/crates/common/src/http_util.rs +++ b/crates/common/src/http_util.rs @@ -3,7 +3,7 @@ use chacha20poly1305::{aead::Aead, aead::KeyInit, XChaCha20Poly1305, XNonce}; use fastly::http::{header, StatusCode}; use fastly::{Request, Response}; use sha2::{Digest, Sha256}; -use subtle::ConstantTimeEq; +use subtle::ConstantTimeEq as _; use crate::constants::INTERNAL_HEADERS; use crate::settings::Settings; @@ -286,10 +286,14 @@ pub fn sign_clear_url(settings: &Settings, clear_url: &str) -> String { } /// Verify a `tstoken` for the given clear-text URL. +/// +/// Uses constant-time comparison to prevent timing side-channel attacks. +/// Length is not secret (always 43 bytes for base64url-encoded SHA-256), +/// but we check explicitly to document the invariant. #[must_use] pub fn verify_clear_url_signature(settings: &Settings, clear_url: &str, token: &str) -> bool { let expected = sign_clear_url(settings, clear_url); - expected.as_bytes().ct_eq(token.as_bytes()).into() + expected.len() == token.len() && bool::from(expected.as_bytes().ct_eq(token.as_bytes())) } /// Compute tstoken for the new proxy scheme: SHA-256 of the encrypted full URL (including query). diff --git a/crates/common/src/proxy.rs b/crates/common/src/proxy.rs index d46f161d..b6500b02 100644 --- a/crates/common/src/proxy.rs +++ b/crates/common/src/proxy.rs @@ -4,7 +4,7 @@ use fastly::http::{header, HeaderValue, Method, StatusCode}; use fastly::{Request, Response}; use serde::{Deserialize, Serialize}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; -use subtle::ConstantTimeEq; +use subtle::ConstantTimeEq as _; use crate::constants::{ HEADER_ACCEPT, HEADER_ACCEPT_ENCODING, HEADER_ACCEPT_LANGUAGE, HEADER_REFERER, @@ -1053,7 +1053,11 @@ fn reconstruct_and_validate_signed_target( }; let expected = compute_encrypted_sha256_token(settings, &full_for_token); - let valid: bool = expected.as_bytes().ct_eq(sig.as_bytes()).into(); + // Constant-time comparison to prevent timing side-channel attacks on the token. + // Length is not secret (always 43 bytes for base64url-encoded SHA-256), + // but we check explicitly to document the invariant. + let valid = + expected.len() == sig.len() && bool::from(expected.as_bytes().ct_eq(sig.as_bytes())); if !valid { return Err(Report::new(TrustedServerError::Proxy { message: "invalid tstoken".to_string(), From 2a4d19c49b0fd317481313c9c350a10752f22025 Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Sat, 21 Mar 2026 09:06:19 +0530 Subject: [PATCH 3/4] Resolve pr review findings --- crates/trusted-server-core/src/auth.rs | 12 +++----- crates/trusted-server-core/src/error.rs | 5 +++ crates/trusted-server-core/src/http_util.rs | 22 ++++++++++++- crates/trusted-server-core/src/proxy.rs | 13 +++----- crates/trusted-server-core/src/redacted.rs | 34 --------------------- 5 files changed, 35 insertions(+), 51 deletions(-) diff --git a/crates/trusted-server-core/src/auth.rs b/crates/trusted-server-core/src/auth.rs index 03a9bb2a..9d8c1bf5 100644 --- a/crates/trusted-server-core/src/auth.rs +++ b/crates/trusted-server-core/src/auth.rs @@ -1,8 +1,8 @@ use base64::{engine::general_purpose::STANDARD, Engine as _}; use fastly::http::{header, StatusCode}; use fastly::{Request, Response}; -use subtle::ConstantTimeEq as _; +use crate::http_util::ct_str_eq; use crate::settings::Settings; const BASIC_AUTH_REALM: &str = r#"Basic realm="Trusted Server""#; @@ -36,13 +36,9 @@ pub fn enforce_basic_auth(settings: &Settings, req: &Request) -> Option StatusCode::BAD_GATEWAY, Self::Integration { .. } => StatusCode::BAD_GATEWAY, Self::Proxy { .. } => StatusCode::BAD_GATEWAY, + Self::Unauthorized { .. } => StatusCode::FORBIDDEN, Self::SyntheticId { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::Template { .. } => StatusCode::INTERNAL_SERVER_ERROR, } diff --git a/crates/trusted-server-core/src/http_util.rs b/crates/trusted-server-core/src/http_util.rs index 9e8a9c64..ec23093e 100644 --- a/crates/trusted-server-core/src/http_util.rs +++ b/crates/trusted-server-core/src/http_util.rs @@ -319,6 +319,26 @@ pub fn sign_clear_url(settings: &Settings, clear_url: &str) -> String { URL_SAFE_NO_PAD.encode(digest) } +/// Constant-time string comparison. +/// +/// The explicit length check documents the invariant that both values have known, +/// non-secret lengths. Both checks always run — the short-circuit `&&` is safe +/// here because token lengths are public information, not secrets. +/// +/// # Examples +/// +/// ``` +/// use trusted_server_core::http_util::ct_str_eq; +/// +/// assert!(ct_str_eq("hello", "hello")); +/// assert!(!ct_str_eq("hello", "world")); +/// assert!(!ct_str_eq("hello", "hell")); +/// ``` +#[must_use] +pub fn ct_str_eq(a: &str, b: &str) -> bool { + a.len() == b.len() && bool::from(a.as_bytes().ct_eq(b.as_bytes())) +} + /// Verify a `tstoken` for the given clear-text URL. /// /// Uses constant-time comparison to prevent timing side-channel attacks. @@ -327,7 +347,7 @@ pub fn sign_clear_url(settings: &Settings, clear_url: &str) -> String { #[must_use] pub fn verify_clear_url_signature(settings: &Settings, clear_url: &str, token: &str) -> bool { let expected = sign_clear_url(settings, clear_url); - expected.len() == token.len() && bool::from(expected.as_bytes().ct_eq(token.as_bytes())) + ct_str_eq(&expected, token) } /// Compute tstoken for the new proxy scheme: SHA-256 of the encrypted full URL (including query). diff --git a/crates/trusted-server-core/src/proxy.rs b/crates/trusted-server-core/src/proxy.rs index 89b889f0..1da7b545 100644 --- a/crates/trusted-server-core/src/proxy.rs +++ b/crates/trusted-server-core/src/proxy.rs @@ -1,10 +1,9 @@ -use crate::http_util::compute_encrypted_sha256_token; +use crate::http_util::{compute_encrypted_sha256_token, ct_str_eq}; use error_stack::{Report, ResultExt}; use fastly::http::{header, HeaderValue, Method, StatusCode}; use fastly::{Request, Response}; use serde::{Deserialize, Serialize}; use std::time::{Duration, SystemTime, UNIX_EPOCH}; -use subtle::ConstantTimeEq as _; use crate::constants::{ HEADER_ACCEPT, HEADER_ACCEPT_ENCODING, HEADER_ACCEPT_LANGUAGE, HEADER_REFERER, @@ -1078,10 +1077,8 @@ fn reconstruct_and_validate_signed_target( // Constant-time comparison to prevent timing side-channel attacks on the token. // Length is not secret (always 43 bytes for base64url-encoded SHA-256), // but we check explicitly to document the invariant. - let valid = - expected.len() == sig.len() && bool::from(expected.as_bytes().ct_eq(sig.as_bytes())); - if !valid { - return Err(Report::new(TrustedServerError::Proxy { + if !ct_str_eq(&expected, &sig) { + return Err(Report::new(TrustedServerError::Unauthorized { message: "invalid tstoken".to_string(), })); } @@ -1299,8 +1296,8 @@ mod tests { .expect_err("should reject tampered token"); assert_eq!( err.current_context().status_code(), - StatusCode::BAD_GATEWAY, - "should return 502 for invalid tstoken" + StatusCode::FORBIDDEN, + "should return 403 for invalid tstoken" ); } diff --git a/crates/trusted-server-core/src/redacted.rs b/crates/trusted-server-core/src/redacted.rs index 1cc91b1f..7193a00a 100644 --- a/crates/trusted-server-core/src/redacted.rs +++ b/crates/trusted-server-core/src/redacted.rs @@ -62,18 +62,6 @@ impl fmt::Display for Redacted { } } -impl PartialEq for Redacted { - fn eq(&self, other: &Self) -> bool { - self.0 == other.0 - } -} - -impl PartialEq for Redacted { - fn eq(&self, other: &T) -> bool { - self.0 == *other - } -} - impl From for Redacted { fn from(value: String) -> Self { Self(value) @@ -152,28 +140,6 @@ mod tests { ); } - #[test] - fn partial_eq_with_inner_type() { - let secret = Redacted::new("my-secret".to_string()); - assert!( - secret == "my-secret".to_string(), - "should equal the inner value" - ); - assert!( - secret != "other".to_string(), - "should not equal a different value" - ); - } - - #[test] - fn partial_eq_between_redacted() { - let a = Redacted::new("same".to_string()); - let b = Redacted::new("same".to_string()); - let c = Redacted::new("different".to_string()); - assert!(a == b, "should equal when inner values match"); - assert!(a != c, "should not equal when inner values differ"); - } - #[test] fn struct_with_redacted_field_debug() { #[derive(Debug)] From 53846f8571ba8d32447b2b83af0bc5ac7bca733c Mon Sep 17 00:00:00 2001 From: prk-Jr Date: Wed, 25 Mar 2026 11:33:02 +0530 Subject: [PATCH 4/4] Rename Unauthorized error variant to Forbidden to match 403 status code The variant mapped to StatusCode::FORBIDDEN (403) but was named Unauthorized, which implies 401. Rename to Forbidden throughout and add a # Security doc section to ct_str_eq warning callers about the length-leak invariant. --- crates/trusted-server-core/src/error.rs | 8 ++++---- crates/trusted-server-core/src/http_util.rs | 9 +++++++++ crates/trusted-server-core/src/proxy.rs | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/crates/trusted-server-core/src/error.rs b/crates/trusted-server-core/src/error.rs index 19040893..ef961bdb 100644 --- a/crates/trusted-server-core/src/error.rs +++ b/crates/trusted-server-core/src/error.rs @@ -66,9 +66,9 @@ pub enum TrustedServerError { #[display("Proxy error: {message}")] Proxy { message: String }, - /// Authorization failure — request understood but not permitted. - #[display("Unauthorized: {message}")] - Unauthorized { message: String }, + /// Request understood but not permitted — results in a 403 Forbidden response. + #[display("Forbidden: {message}")] + Forbidden { message: String }, /// Settings parsing or validation failed. #[display("Settings error: {message}")] @@ -110,7 +110,7 @@ impl IntoHttpResponse for TrustedServerError { Self::Prebid { .. } => StatusCode::BAD_GATEWAY, Self::Integration { .. } => StatusCode::BAD_GATEWAY, Self::Proxy { .. } => StatusCode::BAD_GATEWAY, - Self::Unauthorized { .. } => StatusCode::FORBIDDEN, + Self::Forbidden { .. } => StatusCode::FORBIDDEN, Self::SyntheticId { .. } => StatusCode::INTERNAL_SERVER_ERROR, Self::Template { .. } => StatusCode::INTERNAL_SERVER_ERROR, } diff --git a/crates/trusted-server-core/src/http_util.rs b/crates/trusted-server-core/src/http_util.rs index ec23093e..7976c858 100644 --- a/crates/trusted-server-core/src/http_util.rs +++ b/crates/trusted-server-core/src/http_util.rs @@ -325,6 +325,15 @@ pub fn sign_clear_url(settings: &Settings, clear_url: &str) -> String { /// non-secret lengths. Both checks always run — the short-circuit `&&` is safe /// here because token lengths are public information, not secrets. /// +/// # Security +/// +/// The length equality check short-circuits (via `&&`), which reveals whether the +/// two strings have equal length via timing. This is safe when both strings have +/// **publicly known, fixed lengths** (e.g. base64url-encoded SHA-256 digests are +/// always 43 bytes). Do **not** use this function to compare secrets of +/// variable or confidential length — use a constant-time comparison that +/// also hides length, such as comparing HMAC outputs. +/// /// # Examples /// /// ``` diff --git a/crates/trusted-server-core/src/proxy.rs b/crates/trusted-server-core/src/proxy.rs index 1da7b545..4cc33de7 100644 --- a/crates/trusted-server-core/src/proxy.rs +++ b/crates/trusted-server-core/src/proxy.rs @@ -1078,7 +1078,7 @@ fn reconstruct_and_validate_signed_target( // Length is not secret (always 43 bytes for base64url-encoded SHA-256), // but we check explicitly to document the invariant. if !ct_str_eq(&expected, &sig) { - return Err(Report::new(TrustedServerError::Unauthorized { + return Err(Report::new(TrustedServerError::Forbidden { message: "invalid tstoken".to_string(), })); }