mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-09 01:35:18 +02:00
Harden bearer auth: constant-time compare, hashed at rest, authoritative actor_id
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) <noreply@anthropic.com>
This commit is contained in:
parent
e926c925d6
commit
c338e80180
5 changed files with 152 additions and 7 deletions
2
Cargo.lock
generated
2
Cargo.lock
generated
|
|
@ -4597,6 +4597,8 @@ dependencies = [
|
|||
"serde_json",
|
||||
"serde_yaml",
|
||||
"serial_test",
|
||||
"sha2",
|
||||
"subtle",
|
||||
"tempfile",
|
||||
"tokio",
|
||||
"tower",
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 }
|
||||
|
|
|
|||
|
|
@ -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<RwLock<Omnigraph>>,
|
||||
bearer_tokens: Arc<HashMap<Arc<str>, Arc<str>>>,
|
||||
bearer_tokens: Arc<[(BearerTokenHash, Arc<str>)]>,
|
||||
policy_engine: Option<Arc<PolicyEngine>>,
|
||||
}
|
||||
|
||||
|
|
@ -175,14 +186,14 @@ impl AppState {
|
|||
bearer_tokens: Vec<(String, String)>,
|
||||
policy_engine: Option<PolicyEngine>,
|
||||
) -> Self {
|
||||
let bearer_tokens = bearer_tokens
|
||||
let bearer_tokens: Vec<(BearerTokenHash, Arc<str>)> = bearer_tokens
|
||||
.into_iter()
|
||||
.map(|(actor, token)| (Arc::<str>::from(token), Arc::<str>::from(actor)))
|
||||
.map(|(actor, token)| (hash_bearer_token(&token), Arc::<str>::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<Arc<str>> {
|
||||
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<Arc<str>> = 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<Vec<(String, String)>> {
|
|||
#[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();
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue