Merge branch 'main' into feature/edgezero-pr2-platform-traits

Author: prk-Jr

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);
-    }
 }

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,

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");

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::<String>();
+        // 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<TrustedServerError> =
+            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();

crates/trusted-server-core/src/redacted.rs

@@ -62,18 +62,6 @@ impl<T> fmt::Display for Redacted<T> {
     }
 }
 
-impl<T: PartialEq> PartialEq for Redacted<T> {
-    fn eq(&self, other: &Self) -> bool {
-        self.0 == other.0
-    }
-}
-
-impl<T: PartialEq> PartialEq<T> for Redacted<T> {
-    fn eq(&self, other: &T) -> bool {
-        self.0 == *other
-    }
-}
-
 impl From<String> for Redacted<String> {
     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)]