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 <noreply@anthropic.com>

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"] }

crates/arc-api/src/jwt_auth.rs

@@ -25,11 +25,19 @@ struct Claims {
 pub enum AuthStrategy {
     Jwt {
         key: Arc<DecodingKey>,
+        validation: Arc<Validation>,
         allowed_usernames: Vec<String>,
     },
     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<String, StatusCode> {
+) -> 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::<Claims>(token, key, &validation)
+    let token_data = jsonwebtoken::decode::<Claims>(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<String, StatusCode> {
+/// Try to authenticate via mTLS peer certificates.
+fn try_mtls(parts: &Parts) -> Result<(), StatusCode> {
     let peer_certs = parts
         .extensions
         .get::<PeerCertificates>()
@@ -154,19 +160,19 @@ fn try_mtls(parts: &Parts) -> Result<String, StatusCode> {
         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<S: Send + Sync> FromRequestParts<S> 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()],
             },
         ]);

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<hyper::body::Incoming>| {
-                // 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
     }
 }

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<ServerConfig> {
+pub fn build_rustls_config(tls_config: &TlsConfig, client_auth: ClientAuth) -> Arc<ServerConfig> {
     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<hyper::body::Incoming>| {
+                    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<CertificateDer<'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 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<CertificateDer<'static>> {
 }
 
 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)

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<hyper::body::Incoming>| {
-                            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);