Use constant-time comparison for token and credential verification by prk-Jr · Pull Request #506 · IABTechLab/trusted-server

prk-Jr · 2026-03-16T07:07:02Z

Summary

Replace == comparisons with subtle::ConstantTimeEq in all three secret-verification sites (tstoken, clear-URL signature, Basic Auth), eliminating timing side-channel attacks
Fix a secondary short-circuit oracle in Basic Auth: && allowed an attacker to distinguish wrong-username from wrong-password via timing — replaced with bitwise & on subtle::Choice so both fields are always evaluated
Add log::warn! on auth failure (path only, no credentials) and five new targeted tests covering each fix

Changes

File	Change
`Cargo.toml`	Add `subtle = "2"` to workspace dependencies
`crates/common/Cargo.toml`	Add `subtle` as direct dependency
`crates/common/src/auth.rs`	CT comparison with `&` instead of `&&`; `log::warn!` on failure; 2 new tests
`crates/common/src/http_util.rs`	CT comparison in `verify_clear_url_signature`; 2 new tests
`crates/common/src/proxy.rs`	CT comparison for tstoken in `reconstruct_and_validate_signed_target`; 1 new test

Closes

Closes #410

Test plan

cargo test --workspace
cargo clippy --all-targets --all-features -- -D warnings
cargo fmt --all -- --check
JS tests: cd crates/js/lib && npx vitest run
JS format: cd crates/js/lib && npm run format
Docs format: cd docs && npm run format
WASM build: cargo build --bin trusted-server-fastly --release --target wasm32-wasip1
Manual testing via fastly compute serve

JS tests are failing on main due to a pre-existing ESM/CJS incompatibility in html-encoding-sniffer node_modules — unrelated to this PR.

Checklist

Changes follow CLAUDE.md conventions
No unwrap() in production code — use expect("should ...")
Uses tracing macros (not println!)
New code has tests
No secrets or credentials committed

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

ChristianPavilonis

See below

ChristianPavilonis

Verdict: Clean, well-focused security hardening PR. Ready to merge with minor improvements.

Excellent anti-oracle design in auth.rs — bitwise & on subtle::Choice instead of && prevents username-existence oracle. Minimal, focused changes — only touches the 3 comparison sites + targeted tests. Good test coverage (5 new tests). subtle v2 is the right dependency choice.

ChristianPavilonis · 2026-03-16T19:45:32Z

crates/common/src/http_util.rs

 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()


P1 — ct_eq on different-length inputs leaks length via timing

subtle::ConstantTimeEq for [u8] returns 0 immediately when lengths differ — the comparison short-circuits on length mismatch. Practically low risk since token length is public knowledge (SHA-256 → base64url = always 43 chars), but it technically doesn't achieve fully constant-time behavior.

Suggestion — either add a length guard or document the invariant:

pub fn verify_clear_url_signature(settings: &Settings, clear_url: &str, token: &str) -> bool { let expected = sign_clear_url(settings, clear_url); // Length is not secret (always 43 bytes for base64url-encoded SHA-256), // but we check explicitly to document the invariant. expected.len() == token.len() && bool::from(expected.as_bytes().ct_eq(token.as_bytes())) }

Same applies to the proxy.rs site.

ChristianPavilonis · 2026-03-16T19:45:32Z

crates/common/src/http_util.rs

@@ -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]


P2 — Doc comment should mention constant-time behavior

The existing doc comment doesn't mention CT comparison. Future maintainers need to know not to regress this:

/// Verify a `tstoken` for the given clear-text URL. /// /// Uses constant-time comparison to prevent timing side-channel attacks.

ChristianPavilonis · 2026-03-16T19:45:32Z

crates/common/src/auth.rs

@@ -14,9 +15,14 @@ pub fn enforce_basic_auth(settings: &Settings, req: &Request) -> Option<Response
        None => return Some(unauthorized_response()),


P2 — Missing doc comment on enforce_basic_auth

Public function with no doc comment (CLAUDE.md convention). The timing-attack mitigation and bitwise-AND design should be documented:

/// 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.

ChristianPavilonis · 2026-03-16T19:45:32Z

crates/common/src/proxy.rs


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


P2 — Add inline comment explaining why ct_eq is used here

Unlike auth.rs which has a nice comment about the bitwise &, this site has no comment explaining why ct_eq is used. A one-liner would help future maintainers:

// Constant-time comparison to prevent timing side-channel attacks on the token. let expected = compute_encrypted_sha256_token(settings, &full_for_token);

ChristianPavilonis · 2026-03-16T19:45:32Z

crates/common/src/auth.rs

 use base64::{engine::general_purpose::STANDARD, Engine as _};
 use fastly::http::{header, StatusCode};
 use fastly::{Request, Response};
+use subtle::ConstantTimeEq;


P3 — Convention: use Trait as _

Per CLAUDE.md: "Use use Trait as _ when you only need trait methods, not the trait name." ConstantTimeEq is only used via .ct_eq() — the trait name is never referenced directly.

use subtle::ConstantTimeEq as _;

Same applies to the imports in http_util.rs and proxy.rs.

ChristianPavilonis · 2026-03-16T19:45:32Z

Cargo.toml

 toml = "0.9.8"
 url = "2.5.8"
 urlencoding = "2.1"
+subtle = "2"


P3 — Alphabetical ordering

subtle should go between sha2 and temp-env to maintain the alphabetical ordering of workspace dependencies.

ChristianPavilonis

Approved — ready to merge

This PR correctly hardens the three token/credential comparison sites with constant-time operations from the subtle crate. The changes are minimal, focused, and well-documented.

Highlights

Excellent anti-oracle design in auth.rs: using bitwise & on subtle::Choice instead of && prevents username-existence oracles. The inline comment explaining why is exactly what future maintainers need.
Well-chosen tests covering tampered tokens (same length, wrong bytes), wrong-username/right-password, correct-username/wrong-password, and empty tokens.
Good doc comments explaining security invariants on enforce_basic_auth, verify_clear_url_signature, and inline in proxy.rs.
log::warn! on auth failure logs path only (no credentials) — correct observability without leaking secrets.
Minimal, focused changes — only the 3 comparison sites plus targeted tests. No unnecessary refactoring.
All CI checks pass (fmt, test, clippy, vitest, CodeQL).

Minor note (not actionable for this PR)

extract_credentials in auth.rs (lines 49-73) has pre-existing early returns that leak timing about credential format (missing header, wrong scheme, malformed base64). This is not introduced by this PR and is fine — the CT comparison correctly protects credential values, not header format parsing.

ChristianPavilonis · 2026-03-17T14:10:10Z

crates/common/src/http_util.rs

 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.len() == token.len() && bool::from(expected.as_bytes().ct_eq(token.as_bytes()))


🤔 Length check short-circuits before ct_eq (informational — no change needed)

The && short-circuits on length mismatch, which technically creates a timing signal distinguishing "wrong length" from "wrong bytes." However, as the comment correctly notes, the length is not secret — it's always 43 bytes for base64url-encoded SHA-256. Additionally, subtle::ct_eq itself returns 0 immediately on length mismatch anyway, so there's no benefit to removing this check. Noting for completeness only.

ChristianPavilonis · 2026-03-17T14:10:10Z

crates/common/src/proxy.rs

+    // 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()));


🤔 Same length-check pattern as http_util.rs (informational — no change needed)

Same observation as above: the && short-circuits on length mismatch, but since token length is not secret (always 43 bytes for base64url-encoded SHA-256) and subtle::ct_eq itself returns 0 on mismatched lengths, this is the pragmatic correct approach.

prk-Jr self-assigned this Mar 16, 2026

prk-Jr requested review from ChristianPavilonis and aram356 March 16, 2026 07:16

ChristianPavilonis reviewed Mar 16, 2026

View reviewed changes

prk-Jr added 2 commits March 17, 2026 13:30

Address PR review feedback on constant-time comparison hardening

0d5376e

Merge branch 'main' into hardening/constant-time-comparisons

2523a92

prk-Jr requested a review from ChristianPavilonis March 17, 2026 12:39

ChristianPavilonis approved these changes Mar 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use constant-time comparison for token and credential verification#506

Use constant-time comparison for token and credential verification#506
prk-Jr wants to merge 3 commits intomainfrom
hardening/constant-time-comparisons

prk-Jr commented Mar 16, 2026

Uh oh!

ChristianPavilonis left a comment •

edited

Loading

Uh oh!

ChristianPavilonis left a comment

Uh oh!

ChristianPavilonis Mar 16, 2026

Uh oh!

ChristianPavilonis Mar 16, 2026

Uh oh!

ChristianPavilonis Mar 16, 2026

Uh oh!

ChristianPavilonis Mar 16, 2026

Uh oh!

ChristianPavilonis Mar 16, 2026

Uh oh!

ChristianPavilonis Mar 16, 2026

Uh oh!

ChristianPavilonis left a comment

Uh oh!

ChristianPavilonis Mar 17, 2026

Uh oh!

ChristianPavilonis Mar 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -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]

		@@ -14,9 +15,14 @@ pub fn enforce_basic_auth(settings: &Settings, req: &Request) -> Option<Response
		None => return Some(unauthorized_response()),

Conversation

prk-Jr commented Mar 16, 2026

Summary

Changes

Closes

Test plan

Checklist

Uh oh!

ChristianPavilonis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristianPavilonis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristianPavilonis left a comment

Choose a reason for hiding this comment

Approved — ready to merge

Highlights

Minor note (not actionable for this PR)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ChristianPavilonis left a comment •

edited

Loading