diff --git a/crates/trusted-server-core/src/auth.rs b/crates/trusted-server-core/src/auth.rs index c322bf5f..547784df 100644 --- a/crates/trusted-server-core/src/auth.rs +++ b/crates/trusted-server-core/src/auth.rs @@ -10,12 +10,17 @@ use crate::settings::Settings; const BASIC_AUTH_REALM: &str = r#"Basic realm="Trusted Server""#; -/// Enforce HTTP basic auth for the matched handler, if any. +/// Enforces HTTP Basic authentication for configured handler paths. /// /// Returns `Ok(None)` when the request does not target a protected handler or /// when the supplied credentials are valid. Returns `Ok(Some(Response))` with /// the auth challenge when credentials are missing or invalid. /// +/// Admin endpoints are protected by requiring a handler at build time; see +/// [`Settings::from_toml_and_env`]. Credential checks use constant-time +/// comparison for both username and password, and evaluate both regardless of +/// individual match results to avoid timing oracles. +/// /// # Errors /// /// Returns an error when handler configuration is invalid, such as an @@ -48,6 +53,7 @@ pub fn enforce_basic_auth( if bool::from(username_match & password_match) { Ok(None) } else { + log::warn!("Basic auth failed for path: {}", req.get_path()); Ok(Some(unauthorized_response())) } } @@ -143,6 +149,41 @@ 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 = create_test_settings(); + 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 evaluate auth") + .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 = create_test_settings(); + 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 evaluate auth") + .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 = create_test_settings(); @@ -204,30 +245,4 @@ mod tests { .expect("should challenge admin path with missing credentials"); assert_eq!(response.get_status(), StatusCode::UNAUTHORIZED); } - - #[test] - fn challenge_when_username_wrong_password_correct() { - let settings = create_test_settings(); - let mut req = Request::new(Method::GET, "https://example.com/secure/data"); - let token = STANDARD.encode("wrong:pass"); - req.set_header(header::AUTHORIZATION, format!("Basic {token}")); - - let response = enforce_basic_auth(&settings, &req) - .expect("should evaluate auth") - .expect("should challenge when only username is wrong"); - assert_eq!(response.get_status(), StatusCode::UNAUTHORIZED); - } - - #[test] - fn challenge_when_username_correct_password_wrong() { - let settings = create_test_settings(); - let mut req = Request::new(Method::GET, "https://example.com/secure/data"); - let token = STANDARD.encode("user:wrong"); - req.set_header(header::AUTHORIZATION, format!("Basic {token}")); - - let response = enforce_basic_auth(&settings, &req) - .expect("should evaluate auth") - .expect("should challenge when only password is wrong"); - assert_eq!(response.get_status(), StatusCode::UNAUTHORIZED); - } } diff --git a/crates/trusted-server-core/src/error.rs b/crates/trusted-server-core/src/error.rs index 21ecd35b..ca9e1e81 100644 --- a/crates/trusted-server-core/src/error.rs +++ b/crates/trusted-server-core/src/error.rs @@ -60,6 +60,10 @@ pub enum TrustedServerError { #[display("Proxy error: {message}")] Proxy { message: String }, + /// Request understood but not permitted — results in a 403 Forbidden response. + #[display("Forbidden: {message}")] + Forbidden { message: String }, + /// A redirect destination was blocked by the proxy allowlist. #[display("Redirect to `{host}` blocked: host not in proxy allowed_domains")] AllowlistViolation { host: String }, @@ -103,6 +107,7 @@ impl IntoHttpResponse for TrustedServerError { Self::Prebid { .. } => StatusCode::BAD_GATEWAY, Self::Integration { .. } => StatusCode::BAD_GATEWAY, Self::Proxy { .. } => StatusCode::BAD_GATEWAY, + Self::Forbidden { .. } => StatusCode::FORBIDDEN, Self::AllowlistViolation { .. } => 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 a53a2e4a..710fa56d 100644 --- a/crates/trusted-server-core/src/http_util.rs +++ b/crates/trusted-server-core/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 as _; use crate::constants::INTERNAL_HEADERS; use crate::settings::Settings; @@ -318,10 +319,34 @@ 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. +/// +/// # 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. +#[must_use] +pub(crate) 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. +/// 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 { - sign_clear_url(settings, clear_url) == token + let expected = sign_clear_url(settings, clear_url); + ct_str_eq(&expected, token) } /// Compute tstoken for the new proxy scheme: SHA-256 of the encrypted full URL (including query). @@ -388,6 +413,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] @@ -561,6 +613,24 @@ mod tests { ); } + #[test] + fn test_ct_str_eq() { + assert!(ct_str_eq("hello", "hello"), "should match equal strings"); + assert!( + !ct_str_eq("hello", "world"), + "should not match different strings" + ); + assert!( + !ct_str_eq("hello", "hell"), + "should not match different lengths" + ); + assert!( + !ct_str_eq("hell", "hello"), + "should not match when first is shorter" + ); + assert!(ct_str_eq("", ""), "should match empty strings"); + } + #[test] fn test_copy_custom_headers_filters_internal() { let mut req = Request::new(fastly::http::Method::GET, "https://example.com"); diff --git a/crates/trusted-server-core/src/proxy.rs b/crates/trusted-server-core/src/proxy.rs index 01257c08..d05bf61c 100644 --- a/crates/trusted-server-core/src/proxy.rs +++ b/crates/trusted-server-core/src/proxy.rs @@ -1,4 +1,4 @@ -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}; @@ -1148,8 +1148,11 @@ fn reconstruct_and_validate_signed_target( }; let expected = compute_encrypted_sha256_token(settings, &full_for_token); - if expected != sig { - return Err(Report::new(TrustedServerError::Proxy { + // 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. + if !ct_str_eq(&expected, &sig) { + return Err(Report::new(TrustedServerError::Forbidden { message: "invalid tstoken".to_string(), })); } @@ -1350,6 +1353,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::FORBIDDEN, + "should return 403 for invalid tstoken" + ); + } + #[tokio::test] async fn click_missing_params_returns_400() { let settings = create_test_settings(); 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)]