From c338e801801ba06de84a8d339d335b884a876b5d Mon Sep 17 00:00:00 2001 From: andrew Date: Fri, 17 Apr 2026 21:40:51 +0300 Subject: [PATCH] Harden bearer auth: constant-time compare, hashed at rest, authoritative actor_id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes two live authz bugs in omnigraph-server: - Bearer-token lookup previously used HashMap::get, which compares keys with Eq and short-circuits on the first differing byte — a network-observable timing oracle for brute-forcing tokens. Tokens are now stored as SHA-256 digests and compared with subtle::ConstantTimeEq, iterating every entry unconditionally so total work is independent of which slot matches. Raw token bytes no longer live in server memory after startup. - authorize_request now overwrites PolicyRequest.actor_id from the authenticated session instead of trusting the handler-supplied field, which previously defaulted to "" via unwrap_or_default(). The empty string can no longer reach Cedar as a policy subject even if a future refactor drops the None check. External API of AppState constructors is unchanged — tokens still enter as Vec<(String, String)> and are hashed on the way in. Co-Authored-By: Claude Opus 4.7 (1M context) --- Cargo.lock | 2 + Cargo.toml | 1 + crates/omnigraph-server/Cargo.toml | 2 + crates/omnigraph-server/src/lib.rs | 69 ++++++++++++++++++-- crates/omnigraph-server/tests/server.rs | 85 +++++++++++++++++++++++++ 5 files changed, 152 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8dd53e8..db7d525 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4597,6 +4597,8 @@ dependencies = [ "serde_json", "serde_yaml", "serial_test", + "sha2", + "subtle", "tempfile", "tokio", "tower", diff --git a/Cargo.toml b/Cargo.toml index 34e2062..1e7129d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -66,6 +66,7 @@ utoipa = { version = "5", features = ["axum_extras"] } url = "2" cedar-policy = "4.9" sha2 = "0.10" +subtle = "2" [profile.dev] debug = 0 diff --git a/crates/omnigraph-server/Cargo.toml b/crates/omnigraph-server/Cargo.toml index 90c33d3..d4724dc 100644 --- a/crates/omnigraph-server/Cargo.toml +++ b/crates/omnigraph-server/Cargo.toml @@ -28,6 +28,8 @@ tower-http = { workspace = true } utoipa = { workspace = true } cedar-policy = { workspace = true } futures = { workspace = true } +sha2 = { workspace = true } +subtle = { workspace = true } [dev-dependencies] tempfile = { workspace = true } diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index 52d2718..3fa8787 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -42,6 +42,8 @@ pub use policy::{ PolicyRequest, PolicyTestConfig, }; use serde_json::Value; +use sha2::{Digest, Sha256}; +use subtle::ConstantTimeEq; use tokio::net::TcpListener; use tokio::sync::{RwLock, mpsc}; use tower_http::trace::TraceLayer; @@ -50,6 +52,15 @@ use tracing_subscriber::EnvFilter; use utoipa::OpenApi; use utoipa::openapi::security::{Http, HttpAuthScheme, SecurityScheme}; +type BearerTokenHash = [u8; 32]; + +fn hash_bearer_token(token: &str) -> BearerTokenHash { + let digest = Sha256::digest(token.as_bytes()); + let mut out = [0u8; 32]; + out.copy_from_slice(&digest); + out +} + #[derive(OpenApi)] #[openapi( info( @@ -110,7 +121,7 @@ pub struct ServerConfig { pub struct AppState { uri: String, db: Arc>, - bearer_tokens: Arc, Arc>>, + bearer_tokens: Arc<[(BearerTokenHash, Arc)]>, policy_engine: Option>, } @@ -175,14 +186,14 @@ impl AppState { bearer_tokens: Vec<(String, String)>, policy_engine: Option, ) -> Self { - let bearer_tokens = bearer_tokens + let bearer_tokens: Vec<(BearerTokenHash, Arc)> = bearer_tokens .into_iter() - .map(|(actor, token)| (Arc::::from(token), Arc::::from(actor))) + .map(|(actor, token)| (hash_bearer_token(&token), Arc::::from(actor))) .collect(); Self { uri, db: Arc::new(RwLock::new(db)), - bearer_tokens: Arc::new(bearer_tokens), + bearer_tokens: Arc::from(bearer_tokens), policy_engine: policy_engine.map(Arc::new), } } @@ -242,7 +253,17 @@ impl AppState { } fn authenticate_bearer_token(&self, provided_token: &str) -> Option> { - self.bearer_tokens.get(provided_token).cloned() + // Hash the incoming token and compare against every stored digest in + // constant time. Iterate all entries unconditionally so total work — + // and therefore response timing — doesn't depend on which slot matches. + let provided_hash = hash_bearer_token(provided_token); + let mut matched: Option> = None; + for (hash, actor) in self.bearer_tokens.iter() { + if bool::from(hash.ct_eq(&provided_hash)) && matched.is_none() { + matched = Some(Arc::clone(actor)); + } + } + matched } fn policy_engine(&self) -> Option<&PolicyEngine> { @@ -554,7 +575,7 @@ fn log_policy_decision(actor_id: &str, request: &PolicyRequest, decision: &Polic fn authorize_request( state: &AppState, actor: Option<&AuthenticatedActor>, - request: PolicyRequest, + mut request: PolicyRequest, ) -> std::result::Result<(), ApiError> { let Some(engine) = state.policy_engine() else { return Ok(()); @@ -562,6 +583,10 @@ fn authorize_request( let Some(actor) = actor else { return Err(ApiError::unauthorized("missing bearer token")); }; + // Authoritative actor_id is the authenticated session, not whatever the + // handler put in the request. Prevents an empty-string default at any + // call site from ever reaching the engine as a policy subject. + request.actor_id = actor.as_str().to_string(); let decision = engine .authorize(&request) .map_err(|err| ApiError::internal(format!("policy: {err}")))?; @@ -1481,13 +1506,43 @@ fn server_bearer_tokens_from_env() -> Result> { #[cfg(test)] mod tests { use super::{ - load_server_settings, normalize_bearer_token, parse_bearer_tokens_json, + hash_bearer_token, load_server_settings, normalize_bearer_token, parse_bearer_tokens_json, server_bearer_tokens_from_env, }; use std::env; use std::fs; use tempfile::tempdir; + #[test] + fn hash_bearer_token_produces_32_byte_output() { + let hash = hash_bearer_token("any-token"); + assert_eq!(hash.len(), 32); + } + + #[test] + fn hash_bearer_token_is_deterministic() { + assert_eq!( + hash_bearer_token("stable-input"), + hash_bearer_token("stable-input"), + ); + } + + #[test] + fn hash_bearer_token_differs_for_different_inputs() { + assert_ne!(hash_bearer_token("token-a"), hash_bearer_token("token-b")); + } + + #[test] + fn hash_bearer_token_matches_known_sha256_vector() { + // SHA-256("abc"). If this ever fails, the hash function was swapped. + let hash = hash_bearer_token("abc"); + let hex: String = hash.iter().map(|b| format!("{:02x}", b)).collect(); + assert_eq!( + hex, + "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad" + ); + } + #[test] fn server_settings_load_from_yaml_config() { let temp = tempdir().unwrap(); diff --git a/crates/omnigraph-server/tests/server.rs b/crates/omnigraph-server/tests/server.rs index 7a00c51..77b9118 100644 --- a/crates/omnigraph-server/tests/server.rs +++ b/crates/omnigraph-server/tests/server.rs @@ -894,6 +894,91 @@ async fn protected_routes_accept_any_configured_team_bearer_token() { assert!(body["runs"].is_array()); } +/// Verifies the hashed-token lookup correctly resolves each bearer to its +/// associated actor, and that the resolved actor — not the handler-supplied +/// default — is what the policy engine sees. Two tokens for two distinct +/// actors; policy grants read to actor-A only. Swapping tokens must swap +/// the policy outcome. +#[tokio::test(flavor = "multi_thread")] +async fn bearer_token_resolves_to_correct_actor_for_policy_decisions() { + let temp = init_loaded_repo().await; + let repo = repo_path(temp.path()); + let policy_path = temp.path().join("policy.yaml"); + fs::write( + &policy_path, + r#" +version: 1 +groups: + readers: [act-a] + writers: [act-b] +protected_branches: [main] +rules: + - id: readers-only + allow: + actors: { group: readers } + actions: [read] + branch_scope: any +"#, + ) + .unwrap(); + let state = AppState::open_with_bearer_tokens_and_policy( + repo.to_string_lossy().to_string(), + vec![ + ("act-a".to_string(), "token-a".to_string()), + ("act-b".to_string(), "token-b".to_string()), + ], + Some(&policy_path), + ) + .await + .unwrap(); + let app = build_app(state); + + // act-a is authenticated AND authorized. + let (ok_status, _) = json_response( + &app, + Request::builder() + .uri("/snapshot?branch=main") + .method(Method::GET) + .header("authorization", "Bearer token-a") + .body(Body::empty()) + .unwrap(), + ) + .await; + assert_eq!(ok_status, StatusCode::OK); + + // act-b is authenticated but policy rejects — proves the resolved actor + // (not some default) was the policy subject. + let (denied_status, denied_body) = json_response( + &app, + Request::builder() + .uri("/snapshot?branch=main") + .method(Method::GET) + .header("authorization", "Bearer token-b") + .body(Body::empty()) + .unwrap(), + ) + .await; + let denied_error: ErrorOutput = serde_json::from_value(denied_body).unwrap(); + assert_eq!(denied_status, StatusCode::FORBIDDEN); + assert_eq!( + denied_error.code, + Some(omnigraph_server::api::ErrorCode::Forbidden) + ); + + // Unknown token: 401, never reaches the policy engine. + let (bad_status, _) = json_response( + &app, + Request::builder() + .uri("/snapshot?branch=main") + .method(Method::GET) + .header("authorization", "Bearer wrong-token") + .body(Body::empty()) + .unwrap(), + ) + .await; + assert_eq!(bad_status, StatusCode::UNAUTHORIZED); +} + #[tokio::test(flavor = "multi_thread")] async fn policy_allows_read_but_distinguishes_401_from_403() { let (_temp, app) = app_for_loaded_repo_with_auth_tokens_and_policy(