Use constant-time comparison for token and credential verification

[prk-Jr]

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 log macros (not println!)
• New code has tests
• No secrets or credentials committed

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

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.

[aram356]

Summary

This PR replaces == comparisons with subtle::ConstantTimeEq in all three secret-verification sites (tstoken, clear-URL signature, Basic Auth), fixing timing side-channel vulnerabilities. The bitwise & in auth to eliminate the username-existence oracle is textbook. One structural issue to address before merge.

Blocking

🔧 wrench

• Redacted::PartialEq re-introduces the timing oracle this PR fixes: PartialEq for Redacted<T> (in redacted.rs:69-72, not in this PR's diff but directly relevant) delegates to T::PartialEq, which for String is a short-circuiting byte comparison. This is the type wrapping proxy_secret, username, and password. While the call sites in this PR correctly use expose() + ct_eq(), the PartialEq impl creates an attractive nuisance — a future developer comparing Redacted values directly would unknowingly bypass constant-time. Consider removing the PartialEq impls or implementing via ct_eq in a follow-up.

❓ question

• reconstruct_and_validate_signed_target returns 502 for invalid tstoken (proxy.rs:1084): A tampered or forged tstoken is a client error, not a gateway error. Should this be 400 Bad Request or 403 Forbidden instead of 502 Bad Gateway?

Non-blocking

♻️ refactor

• Duplicated constant-time comparison pattern: The len == len && bool::from(ct_eq) pattern appears in auth.rs:39, http_util.rs:331, and proxy.rs:1082. Extract a ct_str_eq(a, b) -> bool helper to prevent future copies from forgetting the length check or bool::from conversion. (auth.rs:39)

🌱 seedling

• sign_clear_url is SHA-256(prefix || secret || message), not HMAC (http_util.rs:320): The doc comment acknowledges this. The construction is safe here because URLs are validated against the signature (not extended), but SHA-256(prefix || secret || msg) is vulnerable to length-extension if reused elsewhere. Consider migrating to hmac::Hmac<Sha256> in a future PR — sha2 is already a dependency.

⛏ nitpick

• PR body checklist says "Uses tracing macros (not println!)" but the codebase uses log, not tracing — minor template inconsistency.

CI Status

• cargo fmt: PASS
• cargo clippy: PASS
• cargo test: PASS
• vitest: PASS
• integration tests: PASS
• browser integration tests: PASS
• CodeQL: PASS

[aram356]

Summary

Solid hardening PR that replaces == comparisons with subtle::ConstantTimeEq across all three secret-verification sites (tstoken, clear-URL signature, Basic Auth), closing timing side-channel attack vectors. The Redacted<T> PartialEq removal is a nice touch to prevent future regressions.

Blocking

🔧 wrench

• Unauthorized variant name contradicts its 403 status code: The new error variant is named Unauthorized but maps to StatusCode::FORBIDDEN (403). In HTTP, 401 is "Unauthorized" and 403 is "Forbidden". Suggest renaming to Forbidden to match the status code semantics. (error.rs:69)

❓ question

• Status code change from 502 → 403 for invalid tstoken: Previously invalid tstoken returned Proxy → 502 Bad Gateway. Now returns Unauthorized → 403 Forbidden. 403 is semantically correct, but this is a client-visible behavior change — any clients or monitoring keying off 502 for this? (proxy.rs:1080)

Non-blocking

🤔 thinking

• ct_str_eq length check leaks length equality (by design): The && short-circuits on length mismatch. Documented as safe for current call-sites (fixed-length base64url SHA-256 digests), but the function is pub — a future caller with variable-length secrets could misuse it. Consider pub(crate) or a # Security doc section. (http_util.rs:339)

🌱 seedling

• Consider HMAC-SHA-256 for URL signing: sign_clear_url uses SHA-256(prefix || secret || url) rather than HMAC-SHA-256. Not practically exploitable here, but HMAC is the standard construct for keyed message authentication. Out of scope — worth a follow-up issue.

CI Status

• cargo fmt: PASS
• cargo clippy: PASS
• cargo test: PASS
• vitest: PASS
• integration tests: PASS
• browser integration tests: PASS
• CodeQL: PASS

[aram356]

Summary

Deep re-review after the Unauthorized → Forbidden rename was addressed. This PR is well-executed: constant-time comparisons via subtle::ConstantTimeEq at all three secret-verification sites (tstoken, clear-URL signature, Basic Auth), bitwise & on subtle::Choice to eliminate the short-circuit oracle in Basic Auth, Redacted<T> PartialEq removal to prevent regressions, and good test coverage for each fix.

No blocking findings remain.

Non-blocking

🤔 thinking

• ct_str_eq is pub but has a length-leak invariant: The && short-circuits on length mismatch, which is safe for current call-sites (fixed-length base64url SHA-256 digests, always 43 bytes). But the function is pub and a future caller with variable-length secrets could misuse it. Consider pub(crate) to limit exposure or keep the thorough # Security doc section (which is already excellent). (http_util.rs:347)

• Status code change from 502 → 403 for invalid tstoken: Previously invalid tstoken returned TrustedServerError::Proxy → 502 Bad Gateway. Now returns Forbidden → 403 Forbidden. The new code is semantically correct (a failed signature check is a client auth error, not an upstream failure). Just flagging for awareness in case any monitoring or client retry logic depends on the 502. (proxy.rs:1080)

🌱 seedling

• Consider HMAC-SHA-256 for URL signing: sign_clear_url uses SHA-256(prefix || secret || url) — a keyed hash with domain separation. While not practically exploitable here (the prefix prevents length-extension on the secret, and the output is compared not forwarded), HMAC-SHA-256 is the standard construct for keyed message authentication and would be a stronger primitive. Worth a follow-up issue, not this PR.

CI Status

• cargo fmt: PASS
• cargo clippy: PASS
• cargo test: PASS
• vitest: PASS
• integration tests: PASS
• browser integration tests: PASS
• CodeQL: PASS

[aram356]

Summary

Solid security hardening — replaces timing-vulnerable == comparisons with subtle::ConstantTimeEq across all three secret-verification sites (tstoken, clear-URL signature, Basic Auth). The bitwise & fix for the username/password oracle and SHA-256 length normalization are both well-implemented.

Non-blocking

👍 praise

• Excellent ct_str_eq documentation (http_util.rs) — clearly explains when the length leak is safe and when this function should NOT be used. Exactly the kind of security doc that prevents future misuse.
• SHA-256 length normalization in auth — hashing credentials before comparison to normalize to fixed-size digests is the right approach, and the comment explaining why is valuable.
• Bitwise & instead of && for credential matching — eliminates the username-oracle timing channel cleanly.
• Removing PartialEq from Redacted<T> — defense-in-depth: makes it impossible to accidentally ==-compare Redacted values at compile time.
• WASM-caveat comment — honestly acknowledges the inherent limitation of constant-time guarantees on WASM/JIT targets.

🤔 thinking

• Forbidden vs Proxy for invalid tstoken — semantically correct (403 vs 502), but this is a behavioral change for clients. Worth confirming no monitoring/alerting keys off 502 rates for token validation failures.
• log::warn! includes request path — good that credentials aren't logged. If protected paths contain sensitive info, logging the full path reveals which endpoints are under attack. Probably fine for operational visibility.

⛏ nitpick

• PR body checklist says "Uses tracing macros" but the project uses log macros per CLAUDE.md. The actual code correctly uses log::warn! — just a copy-paste inaccuracy in the description.

CI Status

• fmt: PASS
• clippy: PASS
• cargo test: PASS
• vitest: PASS
• CodeQL: PASS
• integration tests: PASS
• browser integration tests: PASS