Skip to content

Use constant-time comparison for admin basic auth credentials#528

Open
ChristianPavilonis wants to merge 4 commits intomainfrom
fix/constant-time-auth
Open

Use constant-time comparison for admin basic auth credentials#528
ChristianPavilonis wants to merge 4 commits intomainfrom
fix/constant-time-auth

Conversation

@ChristianPavilonis
Copy link
Collaborator

@ChristianPavilonis ChristianPavilonis commented Mar 19, 2026

Summary

  • Replace standard == string equality with subtle::ConstantTimeEq for username and password checks in enforce_basic_auth, preventing timing side-channel attacks that could leak credential prefix information
  • Hash credentials with SHA-256 before comparing to normalise lengths — ct_eq on raw byte slices short-circuits when lengths differ, which would leak credential length
  • Both username and password are always fully compared (bitwise AND on Choice) regardless of where the first mismatch occurs

Changes

File Change
Cargo.toml Add subtle = "2.6" to workspace dependencies (already a transitive dep via sha2/digest, ed25519-dalek, chacha20poly1305, etc. — no new crate downloaded, no binary size change)
crates/common/Cargo.toml Add subtle = { workspace = true }
crates/common/src/auth.rs Replace == comparison with SHA-256 + ConstantTimeEq; use bitwise & on Choice to prevent early exit; add partial-match tests; switch to shared create_test_settings() helper
crates/common/src/redacted.rs No changes (stale TODO removed in follow-up commit)

Closes

Closes #447

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
  • Other:

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 ChristianPavilonis marked this pull request as ready for review March 19, 2026 21:36
Copy link
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChristianPavilonis Please resolve conflicts

Replace standard string equality with subtle::ConstantTimeEq for
username and password checks in enforce_basic_auth. This prevents
timing side-channel attacks that could leak credential prefix
information through response latency differences.

Closes #447
Copy link
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Solid security hardening — SHA-256 + ct_eq + bitwise AND is the right approach to close timing side-channels on credential length, prefix, and username existence. One non-blocking concern about the Redacted::PartialEq impls.

Non-blocking

🤔 thinking

  • Redacted::PartialEq still uses standard == — accidental misuse risk: The commit history shows a TODO warning was added in commit 2 then removed in commit 3. While auth.rs now correctly bypasses PartialEq via .expose() + SHA-256 + ct_eq, a future contributor could write handler.username == input and reintroduce the timing vulnerability. Consider keeping the TODO comment or adding a doc comment on Redacted::PartialEq noting it is NOT constant-time and should not be used for security-sensitive comparisons. (crates/trusted-server-core/src/redacted.rs:65)

🌱 seedling

  • Pre-compute expected credential hashes at settings load time: Currently SHA-256 digests of expected credentials are computed on every request. For admin endpoints this is negligible, but if constant-time auth is ever added to higher-traffic paths, pre-computing hashes in Handler would avoid redundant work. Not for this PR.

⛏ nitpick

  • PR checklist references tracing but project uses log: The checklist item says "Uses tracing macros (not println!)" but CLAUDE.md specifies log crate macros. The code correctly uses log::warn! — just a template inaccuracy.

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests: PASS
  • js tests: PASS
  • integration tests: PASS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use constant-time comparison for admin basic auth password checks

2 participants