From eb15b5254cc933ba2627fc2b57cfb5823c80fc56 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 3 Mar 2026 17:43:14 -0500 Subject: [PATCH] Simplify mTLS implementation after code review - Replace boolean params (mtls_enabled, mtls_optional) with ClientAuth enum - Move serve_tls from serve.rs into tls module (encapsulate TLS internals) - Derive client auth mode from AuthMode (eliminate duplicate strategy checks) - Build JWT Validation once at startup, store in AuthStrategy (not per-request) - Add tilde expansion for TLS cert paths (~/.arc/certs/...) - Remove wasted String allocations from try_jwt/try_mtls return values - Remove duplicate rustls dev-dependency from Cargo.toml - Integration tests reuse tls::serve_tls instead of duplicating accept loop Co-Authored-By: Claude Opus 4.6 --- crates/arc-api/Cargo.toml | 1 - crates/arc-api/src/jwt_auth.rs | 52 +++++----- crates/arc-api/src/serve.rs | 88 ++++++----------- crates/arc-api/src/tls.rs | 142 ++++++++++++++++++++-------- crates/arc-api/tests/integration.rs | 62 ++---------- 5 files changed, 172 insertions(+), 173 deletions(-) diff --git a/crates/arc-api/Cargo.toml b/crates/arc-api/Cargo.toml index 17f918b68..6a795f806 100644 --- a/crates/arc-api/Cargo.toml +++ b/crates/arc-api/Cargo.toml @@ -48,4 +48,3 @@ tempfile = "3" openapiv3 = "2" serde_yaml = "0.9" reqwest = { workspace = true } -rustls = { version = "0.23", default-features = false, features = ["std", "ring"] } diff --git a/crates/arc-api/src/jwt_auth.rs b/crates/arc-api/src/jwt_auth.rs index d3236d272..5cd608624 100644 --- a/crates/arc-api/src/jwt_auth.rs +++ b/crates/arc-api/src/jwt_auth.rs @@ -25,11 +25,19 @@ struct Claims { pub enum AuthStrategy { Jwt { key: Arc, + validation: Arc, allowed_usernames: Vec, }, Mtls, } +pub fn jwt_validation() -> Validation { + let mut validation = Validation::new(Algorithm::EdDSA); + validation.set_required_spec_claims(&["iss", "iat", "exp"]); + validation.set_issuer(&["arc-web"]); + validation +} + /// Authentication mode resolved at startup. #[derive(Clone)] pub enum AuthMode { @@ -83,6 +91,7 @@ pub fn resolve_auth_mode( .expect("ARC_JWT_PUBLIC_KEY contains an invalid Ed25519 PEM public key"); AuthStrategy::Jwt { key: Arc::new(key), + validation: Arc::new(jwt_validation()), allowed_usernames: allowed_usernames.clone(), } } @@ -99,12 +108,13 @@ pub fn resolve_auth_mode( AuthMode::Strategies(strategies) } -/// Try to authenticate via JWT. Returns the subject (username) on success. +/// Try to authenticate via JWT. fn try_jwt( parts: &Parts, key: &DecodingKey, + validation: &Validation, allowed_usernames: &[String], -) -> Result { +) -> Result<(), StatusCode> { let header = parts .headers .get("authorization") @@ -115,11 +125,7 @@ fn try_jwt( .strip_prefix("Bearer ") .ok_or(StatusCode::UNAUTHORIZED)?; - let mut validation = Validation::new(Algorithm::EdDSA); - validation.set_required_spec_claims(&["iss", "iat", "exp"]); - validation.set_issuer(&["arc-web"]); - - let token_data = jsonwebtoken::decode::(token, key, &validation) + let token_data = jsonwebtoken::decode::(token, key, validation) .map_err(|_| StatusCode::UNAUTHORIZED)?; // Fail closed: if no usernames are allowed, reject all requests @@ -139,11 +145,11 @@ fn try_jwt( return Err(StatusCode::FORBIDDEN); } - Ok(username.to_string()) + Ok(()) } -/// Try to authenticate via mTLS peer certificates. Returns the CN on success. -fn try_mtls(parts: &Parts) -> Result { +/// Try to authenticate via mTLS peer certificates. +fn try_mtls(parts: &Parts) -> Result<(), StatusCode> { let peer_certs = parts .extensions .get::() @@ -154,19 +160,19 @@ fn try_mtls(parts: &Parts) -> Result { return Err(StatusCode::UNAUTHORIZED); } - // Extract CN from the first (leaf) certificate + // Verify we can parse the leaf certificate and extract a CN let cert = &peer_certs[0]; let (_, parsed) = x509_parser::parse_x509_certificate(cert) .map_err(|_| StatusCode::UNAUTHORIZED)?; - let cn = parsed + parsed .subject() .iter_common_name() .next() .and_then(|cn| cn.as_str().ok()) .ok_or(StatusCode::UNAUTHORIZED)?; - Ok(cn.to_string()) + Ok(()) } /// Axum extractor that enforces authentication on a route. @@ -197,18 +203,17 @@ impl FromRequestParts for AuthenticatedService { let mut last_err = StatusCode::UNAUTHORIZED; for strategy in strategies { - match strategy { - AuthStrategy::Mtls => match try_mtls(parts) { - Ok(_subject) => return Ok(AuthenticatedService), - Err(e) => last_err = e, - }, + let result = match strategy { + AuthStrategy::Mtls => try_mtls(parts), AuthStrategy::Jwt { key, + validation, allowed_usernames, - } => match try_jwt(parts, key, allowed_usernames) { - Ok(_subject) => return Ok(AuthenticatedService), - Err(e) => last_err = e, - }, + } => try_jwt(parts, key, validation, allowed_usernames), + }; + match result { + Ok(()) => return Ok(AuthenticatedService), + Err(e) => last_err = e, } } @@ -287,6 +292,7 @@ mod tests { fn jwt_mode(decoding: DecodingKey, allowed_usernames: Vec<&str>) -> AuthMode { AuthMode::Strategies(vec![AuthStrategy::Jwt { key: Arc::new(decoding), + validation: Arc::new(jwt_validation()), allowed_usernames: allowed_usernames.into_iter().map(String::from).collect(), }]) } @@ -619,6 +625,7 @@ mod tests { let mode = AuthMode::Strategies(vec![ AuthStrategy::Jwt { key: Arc::new(decoding), + validation: Arc::new(jwt_validation()), allowed_usernames: vec!["brynary".to_string()], }, AuthStrategy::Mtls, @@ -639,6 +646,7 @@ mod tests { AuthStrategy::Mtls, AuthStrategy::Jwt { key: Arc::new(decoding), + validation: Arc::new(jwt_validation()), allowed_usernames: vec!["brynary".to_string()], }, ]); diff --git a/crates/arc-api/src/serve.rs b/crates/arc-api/src/serve.rs index 52a00fd06..12c078871 100644 --- a/crates/arc-api/src/serve.rs +++ b/crates/arc-api/src/serve.rs @@ -3,13 +3,13 @@ use std::sync::Arc; use arc_llm::provider::Provider; use arc_util::terminal::Styles; use tokio::net::TcpListener; -use tracing::{error, info}; +use tracing::info; use clap::Args; -use crate::jwt_auth::PeerCertificates; +use crate::jwt_auth::{AuthMode, AuthStrategy}; use crate::server::{build_router, create_app_state_with_options}; -use crate::server_config::ApiAuthStrategy; +use crate::tls::ClientAuth; use arc_workflows::cli::backend::AgentApiBackend; use arc_workflows::cli::SandboxProvider; use arc_workflows::handler::default_registry; @@ -130,6 +130,13 @@ pub async fn serve_command(args: ServeArgs, styles: &'static Styles) -> anyhow:: ) }; + // Derive client auth mode before auth_mode is moved into the router + let client_auth = server_config + .api + .tls + .as_ref() + .map(|_| client_auth_from_mode(&auth_mode)); + let state = create_app_state_with_options(db, factory, dry_run_mode, args.demo); let router = build_router(state, auth_mode); @@ -151,19 +158,14 @@ pub async fn serve_command(args: ServeArgs, styles: &'static Styles) -> anyhow:: // Branch: TLS or plain HTTP if let Some(ref tls_config) = server_config.api.tls { - let mtls_enabled = server_config - .api - .authentication_strategies - .contains(&ApiAuthStrategy::Mtls); - let mtls_optional = mtls_enabled && server_config.api.authentication_strategies.len() > 1; + let client_auth = client_auth.unwrap(); - let rustls_config = - crate::tls::build_rustls_config(tls_config, mtls_enabled, mtls_optional); + let rustls_config = crate::tls::build_rustls_config(tls_config, client_auth); let tls_acceptor = tokio_rustls::TlsAcceptor::from(rustls_config); - info!("TLS enabled (mTLS {})", if mtls_enabled { "on" } else { "off" }); + info!("TLS enabled"); - serve_tls(listener, tls_acceptor, router).await?; + crate::tls::serve_tls(listener, tls_acceptor, router).await?; } else { axum::serve(listener, router).await?; } @@ -171,55 +173,21 @@ pub async fn serve_command(args: ServeArgs, styles: &'static Styles) -> anyhow:: Ok(()) } -/// Serve requests over TLS, extracting peer certificates into request extensions. -async fn serve_tls( - listener: TcpListener, - tls_acceptor: tokio_rustls::TlsAcceptor, - router: axum::Router, -) -> anyhow::Result<()> { - use hyper_util::rt::{TokioExecutor, TokioIo}; - use hyper_util::server::conn::auto::Builder; - use tower_service::Service; +/// Derive client certificate verification mode from the resolved auth strategies. +fn client_auth_from_mode(auth_mode: &AuthMode) -> ClientAuth { + let strategies = match auth_mode { + AuthMode::Strategies(s) => s, + AuthMode::Disabled => return ClientAuth::None, + }; - let builder = Builder::new(TokioExecutor::new()); + let has_mtls = strategies.iter().any(|s| matches!(s, AuthStrategy::Mtls)); + if !has_mtls { + return ClientAuth::None; + } - loop { - let (tcp_stream, remote_addr) = listener.accept().await?; - - let tls_acceptor = tls_acceptor.clone(); - let router = router.clone(); - let builder = builder.clone(); - - tokio::spawn(async move { - let tls_stream = match tls_acceptor.accept(tcp_stream).await { - Ok(s) => s, - Err(e) => { - error!(%remote_addr, "TLS handshake failed: {e}"); - return; - } - }; - - // Extract peer certificates from the TLS connection - let peer_certs = tls_stream - .get_ref() - .1 - .peer_certificates() - .map(|certs| certs.to_vec()); - - let io = TokioIo::new(tls_stream); - - let service = hyper::service::service_fn(move |mut req: hyper::Request| { - // Insert peer certificates into request extensions - req.extensions_mut() - .insert(PeerCertificates(peer_certs.clone())); - - let mut router = router.clone(); - async move { router.call(req).await } - }); - - if let Err(e) = builder.serve_connection(io, service).await { - error!(%remote_addr, "connection error: {e}"); - } - }); + if strategies.len() > 1 { + ClientAuth::Optional + } else { + ClientAuth::Required } } diff --git a/crates/arc-api/src/tls.rs b/crates/arc-api/src/tls.rs index 18f67ca16..b4cd1ccf2 100644 --- a/crates/arc-api/src/tls.rs +++ b/crates/arc-api/src/tls.rs @@ -1,60 +1,125 @@ -use std::path::Path; +use std::path::{Path, PathBuf}; use std::sync::Arc; use rustls::server::WebPkiClientVerifier; use rustls::ServerConfig; use rustls_pki_types::{CertificateDer, PrivateKeyDer}; +use tokio::net::TcpListener; +use tracing::error; +use crate::jwt_auth::PeerCertificates; use crate::server_config::TlsConfig; +/// How client certificates should be verified. +pub enum ClientAuth { + /// No client certificates requested (TLS encryption only). + None, + /// Client certificates required; reject connections without one. + Required, + /// Client certificates requested but not required (multi-strategy fallback). + Optional, +} + /// Build a rustls `ServerConfig` from the `[api.tls]` configuration. -/// -/// - `mtls_enabled`: whether mTLS is listed as an authentication strategy. -/// - `mtls_optional`: whether other strategies (e.g. JWT) are also present, -/// meaning client certs should be requested but not required. -pub fn build_rustls_config( - tls_config: &TlsConfig, - mtls_enabled: bool, - mtls_optional: bool, -) -> Arc { +pub fn build_rustls_config(tls_config: &TlsConfig, client_auth: ClientAuth) -> Arc { let certs = load_certs(&tls_config.cert); let key = load_private_key(&tls_config.key); - let config = if mtls_enabled { - let ca_certs = load_certs(&tls_config.ca); - let mut root_store = rustls::RootCertStore::empty(); - for cert in ca_certs { - root_store.add(cert).expect("failed to add CA certificate to root store"); - } - - let verifier = if mtls_optional { - WebPkiClientVerifier::builder(Arc::new(root_store)) - .allow_unauthenticated() - .build() - .expect("failed to build optional client verifier") - } else { - WebPkiClientVerifier::builder(Arc::new(root_store)) - .build() - .expect("failed to build required client verifier") - }; - - ServerConfig::builder() - .with_client_cert_verifier(verifier) - .with_single_cert(certs, key) - .expect("invalid server certificate or key") - } else { - // TLS for encryption only, no client cert verification - ServerConfig::builder() + let config = match client_auth { + ClientAuth::None => ServerConfig::builder() .with_no_client_auth() .with_single_cert(certs, key) - .expect("invalid server certificate or key") + .expect("invalid server certificate or key"), + ClientAuth::Required | ClientAuth::Optional => { + let ca_certs = load_certs(&tls_config.ca); + let mut root_store = rustls::RootCertStore::empty(); + for cert in ca_certs { + root_store.add(cert).expect("failed to add CA certificate to root store"); + } + + let builder = WebPkiClientVerifier::builder(Arc::new(root_store)); + let verifier = if matches!(client_auth, ClientAuth::Optional) { + builder.allow_unauthenticated() + } else { + builder + } + .build() + .expect("failed to build client verifier"); + + ServerConfig::builder() + .with_client_cert_verifier(verifier) + .with_single_cert(certs, key) + .expect("invalid server certificate or key") + } }; Arc::new(config) } +/// Serve requests over TLS, extracting peer certificates into request extensions. +pub async fn serve_tls( + listener: TcpListener, + tls_acceptor: tokio_rustls::TlsAcceptor, + router: axum::Router, +) -> anyhow::Result<()> { + use hyper_util::rt::{TokioExecutor, TokioIo}; + use hyper_util::server::conn::auto::Builder; + use tower_service::Service; + + let builder = Builder::new(TokioExecutor::new()); + + loop { + let (tcp_stream, remote_addr) = listener.accept().await?; + + let tls_acceptor = tls_acceptor.clone(); + let router = router.clone(); + let builder = builder.clone(); + + tokio::spawn(async move { + let tls_stream = match tls_acceptor.accept(tcp_stream).await { + Ok(s) => s, + Err(e) => { + error!(%remote_addr, "TLS handshake failed: {e}"); + return; + } + }; + + // Extract peer certificates once per connection (not per request) + let (_, server_conn) = tls_stream.get_ref(); + let peer_certs = PeerCertificates( + server_conn.peer_certificates().map(|certs| certs.to_vec()), + ); + + let io = TokioIo::new(tls_stream); + + let service = hyper::service::service_fn( + move |mut req: hyper::Request| { + req.extensions_mut().insert(peer_certs.clone()); + let mut router = router.clone(); + async move { router.call(req).await } + }, + ); + + if let Err(e) = builder.serve_connection(io, service).await { + error!(%remote_addr, "connection error: {e}"); + } + }); + } +} + +/// Expand `~/` prefix to the user's home directory. +fn expand_tilde(path: &Path) -> PathBuf { + if let Ok(rest) = path.strip_prefix("~") { + if let Some(home) = dirs::home_dir() { + return home.join(rest); + } + } + path.to_path_buf() +} + fn load_certs(path: &Path) -> Vec> { - let file = std::fs::File::open(path) + let path = expand_tilde(path); + let file = std::fs::File::open(&path) .unwrap_or_else(|e| panic!("failed to open certificate file {}: {e}", path.display())); let mut reader = std::io::BufReader::new(file); rustls_pemfile::certs(&mut reader) @@ -63,7 +128,8 @@ fn load_certs(path: &Path) -> Vec> { } fn load_private_key(path: &Path) -> PrivateKeyDer<'static> { - let file = std::fs::File::open(path) + let path = expand_tilde(path); + let file = std::fs::File::open(&path) .unwrap_or_else(|e| panic!("failed to open private key file {}: {e}", path.display())); let mut reader = std::io::BufReader::new(file); rustls_pemfile::private_key(&mut reader) diff --git a/crates/arc-api/tests/integration.rs b/crates/arc-api/tests/integration.rs index 1c1b86fed..7d120990a 100644 --- a/crates/arc-api/tests/integration.rs +++ b/crates/arc-api/tests/integration.rs @@ -7,10 +7,10 @@ mod mtls_e2e { use std::process::{Command, Stdio}; use std::sync::Arc; - use arc_api::jwt_auth::{AuthMode, AuthStrategy, PeerCertificates}; + use arc_api::jwt_auth::{AuthMode, AuthStrategy}; use arc_api::server::{build_router, create_app_state}; use arc_api::server_config::TlsConfig; - use arc_api::tls::build_rustls_config; + use arc_api::tls::{build_rustls_config, ClientAuth}; use arc_workflows::handler::codergen::CodergenHandler; use arc_workflows::handler::exit::ExitHandler; use arc_workflows::handler::start::StartHandler; @@ -128,65 +128,22 @@ mod mtls_e2e { } /// Start a TLS server on a random port, returning the bound address. - /// `mtls_optional`: if true, client certs are requested but not required (for multi-strategy). - /// `auth_mode`: the authentication mode to use for the router. async fn start_tls_server( tls_config: &TlsConfig, - mtls_optional: bool, + client_auth: ClientAuth, auth_mode: AuthMode, ) -> std::net::SocketAddr { let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let addr = listener.local_addr().unwrap(); - let rustls_config = build_rustls_config(tls_config, true, mtls_optional); + let rustls_config = build_rustls_config(tls_config, client_auth); let tls_acceptor = tokio_rustls::TlsAcceptor::from(rustls_config); let state = create_app_state(test_db().await, simple_registry); let router = build_router(state, auth_mode); tokio::spawn(async move { - use hyper_util::rt::{TokioExecutor, TokioIo}; - use hyper_util::server::conn::auto::Builder; - use tower_service::Service; - - let builder = Builder::new(TokioExecutor::new()); - - loop { - let (tcp_stream, _remote_addr) = match listener.accept().await { - Ok(s) => s, - Err(_) => return, - }; - - let tls_acceptor = tls_acceptor.clone(); - let router = router.clone(); - let builder = builder.clone(); - - tokio::spawn(async move { - let tls_stream = match tls_acceptor.accept(tcp_stream).await { - Ok(s) => s, - Err(_) => return, - }; - - let peer_certs = tls_stream - .get_ref() - .1 - .peer_certificates() - .map(|certs| certs.to_vec()); - - let io = TokioIo::new(tls_stream); - - let service = hyper::service::service_fn( - move |mut req: hyper::Request| { - req.extensions_mut() - .insert(PeerCertificates(peer_certs.clone())); - let mut router = router.clone(); - async move { router.call(req).await } - }, - ); - - let _ = builder.serve_connection(io, service).await; - }); - } + let _ = arc_api::tls::serve_tls(listener, tls_acceptor, router).await; }); addr @@ -234,7 +191,7 @@ mod mtls_e2e { }; let auth_mode = AuthMode::Strategies(vec![AuthStrategy::Mtls]); - let addr = start_tls_server(&tls_config, false, auth_mode).await; + let addr = start_tls_server(&tls_config, ClientAuth::Required, auth_mode).await; let client = build_client( &pki.ca_cert, @@ -264,7 +221,7 @@ mod mtls_e2e { }; let auth_mode = AuthMode::Strategies(vec![AuthStrategy::Mtls]); - let addr = start_tls_server(&tls_config, false, auth_mode).await; + let addr = start_tls_server(&tls_config, ClientAuth::Required, auth_mode).await; // Generate a DIFFERENT CA and client cert signed by it let wrong_dir = dir.path().join("wrong_ca"); @@ -306,7 +263,7 @@ mod mtls_e2e { // mTLS is the ONLY strategy → client cert is required at TLS level let auth_mode = AuthMode::Strategies(vec![AuthStrategy::Mtls]); - let addr = start_tls_server(&tls_config, false, auth_mode).await; + let addr = start_tls_server(&tls_config, ClientAuth::Required, auth_mode).await; // Client trusts the server CA but presents NO client cert let client = build_client(&pki.ca_cert, None, None); @@ -386,10 +343,11 @@ mod mtls_e2e { AuthStrategy::Mtls, AuthStrategy::Jwt { key: Arc::new(decoding_key), + validation: Arc::new(arc_api::jwt_auth::jwt_validation()), allowed_usernames: vec!["brynary".to_string()], }, ]); - let addr = start_tls_server(&tls_config, true, auth_mode).await; + let addr = start_tls_server(&tls_config, ClientAuth::Optional, auth_mode).await; // Client trusts the server CA but presents NO client cert let client = build_client(&pki.ca_cert, None, None);