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

🔧 wrench — Unauthorized variant name contradicts its 403 status code

In HTTP, 401 is "Unauthorized" and 403 is "Forbidden". This variant maps to StatusCode::FORBIDDEN (403) but is named Unauthorized, which will mislead maintainers into expecting a 401.

The doc comment ("Authorization failure — request understood but not permitted") describes 403 perfectly — the name just doesn't match.

Fix: Rename to Forbidden:

/// Authorization failure — request understood but not permitted.
#[display("Forbidden: {message}")]
Forbidden { message: String },

And update the usage in proxy.rs accordingly.

[aram356]

❓ question — Status code change from 502 → 403 for invalid tstoken: intentional?

Previously, an invalid tstoken returned TrustedServerError::Proxy → 502 Bad Gateway. Now it returns Unauthorized → 403 Forbidden. This is a client-visible behavior change.

403 is semantically much better than 502 for a signature validation failure — 502 implied an upstream failure, not a client error. But are there any clients or monitoring that key off the 502 for invalid tokens?

[aram356]

🤔 thinking — ct_str_eq length check leaks length equality (by design)

The && short-circuits on length mismatch, revealing whether lengths match via timing. The doc comment correctly documents this as safe because token lengths are public (always 43 bytes for base64url-encoded SHA-256).

However, the function is pub — a future caller with variable-length secrets could misuse it. Consider either:

• pub(crate) to limit exposure, or
• Adding a # Security doc section noting the length-leak invariant

Not blocking since all current call-sites use fixed-length inputs, and auth.rs correctly uses SHA-256 hashing to normalize lengths before calling ct_eq directly.