From 6a4c4c95a4bb189a37701fb1e3c2218c8b02617d Mon Sep 17 00:00:00 2001 From: Draco-Lunaris-Echo Date: Sat, 6 Jun 2026 13:58:01 -0500 Subject: [PATCH] fix: remove dead MtlsMiddleware, add security header middleware, document rustls as auth gate (closes #13) - Remove dead MtlsMiddleware struct, MtlsMiddlewareService, Transform/Service impls - Remove validate_client_certificate() stub (returned Ok(()) unconditionally) - Remove has_duplicate_critical_headers() from mtls.rs (moved to new module) - Convert build_rustls_config() from method on MtlsMiddleware to free function - Create SecurityHeadersMiddleware in src/auth/security_headers.rs for VULN-006 - Wire SecurityHeadersMiddleware into Actix-web pipeline in main.rs - Add ADR documenting rustls as authoritative client-auth gate - Preserve CrlAwareVerifier, MtlsConfig, MtlsError, ClientCertInfo, build_rustls_config - Add integration tests for duplicate header detection - Update HARDENING_REPORT.md and SECURITY_FINDINGS_REPORT.md with ADR Co-authored-by: git-echo --- Cargo.toml | 4 + HARDENING_REPORT.md | 43 +++- SECURITY_FINDINGS_REPORT.md | 34 +++ src/auth/mod.rs | 16 +- src/auth/mtls.rs | 450 +++++++-------------------------- src/auth/security_headers.rs | 166 ++++++++++++ src/main.rs | 96 +++---- tests/integration/auth_test.rs | 70 ++++- 8 files changed, 458 insertions(+), 421 deletions(-) create mode 100644 src/auth/security_headers.rs diff --git a/Cargo.toml b/Cargo.toml index 805920f..35be20e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -114,6 +114,10 @@ path = "tests/integration/enrollment_test.rs" name = "enrollment_e2e" path = "tests/e2e/test_enrollment_e2e.rs" +[[test]] +name = "auth_test" +path = "tests/integration/auth_test.rs" + [[bench]] name = "api_benchmarks" harness = false diff --git a/HARDENING_REPORT.md b/HARDENING_REPORT.md index 416309d..f084f25 100644 --- a/HARDENING_REPORT.md +++ b/HARDENING_REPORT.md @@ -20,7 +20,7 @@ This report documents the implementation of 6 security hardening fixes deferred | VULN-003 | LOW | Input Validation | ✅ RESOLVED | src/api/handlers/packages.rs | | VULN-004 | MEDIUM | Header Security | ✅ RESOLVED | src/main.rs | | VULN-005 | LOW | HTTP Protocol | ✅ RESOLVED | src/api/routes.rs | -| VULN-006 | LOW | Header Security | ✅ RESOLVED | src/auth/mtls.rs | +| VULN-006 | LOW | Header Security | ✅ RESOLVED | src/auth/security_headers.rs | --- @@ -176,20 +176,19 @@ web::scope("/api/v1") **Finding:** Duplicate Content-Type headers were accepted. **Implementation:** -- Added `has_duplicate_critical_headers()` function to check for duplicate headers +- `has_duplicate_critical_headers()` function checks for duplicate headers on every request - Monitors critical headers: `content-type`, `authorization`, `host` -- Integrated into mTLS middleware `call()` method -- Rejects requests with duplicate critical headers before further processing +- Implemented as `SecurityHeadersMiddleware` — a dedicated Actix-web middleware +- Wired into the middleware pipeline in `main.rs` between WhitelistMiddleware and Logger +- Rejects requests with duplicate critical headers with HTTP 400 Bad Request -**Code Location:** `src/auth/mtls.rs` (lines 26-49, 203-212) +**Code Location:** `src/auth/security_headers.rs` ```rust -fn has_duplicate_critical_headers(req: &ServiceRequest) -> bool { - let critical_headers = ["content-type", "authorization", "host"]; - - for header_name in critical_headers.iter() { +pub fn has_duplicate_critical_headers(headers: &HeaderMap) -> bool { + for header_name in CRITICAL_HEADERS.iter() { let mut count = 0; - for (name, _) in req.headers().iter() { + for (name, _value) in headers.iter() { if name.as_str().eq_ignore_ascii_case(header_name) { count += 1; if count > 1 { @@ -202,7 +201,29 @@ fn has_duplicate_critical_headers(req: &ServiceRequest) -> bool { } ``` -**Response:** HTTP 400 Bad Request with message "Duplicate critical headers not allowed" +**Response:** HTTP 400 Bad Request with error message "Duplicate critical headers not allowed" + +**Architecture Note:** The duplicate-header check was originally in `MtlsMiddleware`, which was dead code (never wired into the pipeline). It has been extracted into `SecurityHeadersMiddleware`, which IS wired into the pipeline and runs on every request. Client certificate authentication is handled at the TLS handshake level by rustls via `CrlAwareVerifier` — no application-layer certificate middleware is needed. See `src/auth/mtls.rs` for the ADR documenting this decision. + +--- + +## Architecture Decision Record: rustls as Authoritative Client-Auth Gate + +**Decision:** Client certificate authentication is enforced at the TLS handshake level by rustls via `CrlAwareVerifier`, NOT by application-layer middleware. + +**Context:** The original `MtlsMiddleware` was never wired into the Actix-web pipeline. It contained both a duplicate-header check (VULN-006) and a `validate_client_certificate()` stub that returned `Ok(())` unconditionally. Meanwhile, the actual client certificate verification was always performed by rustls at the TLS handshake level through `CrlAwareVerifier`, which wraps `WebPkiClientVerifier`. + +**Rationale:** +- rustls provides battle-tested X.509 verification at the TLS handshake level +- Enforcing auth at the TLS layer eliminates bypass vulnerabilities (middleware ordering bugs, route-specific skips) +- CRL revocation checking is integrated into the same handshake path via `CrlAwareVerifier` +- Application-layer certificate validation is redundant when the TLS layer already rejects untrusted connections + +**Consequences:** +- `MtlsMiddleware` (Transform/Service) and `validate_client_certificate()` have been removed as dead code +- `build_rustls_config()` is now a free function (no longer a method on `MtlsMiddleware`) +- `SecurityHeadersMiddleware` handles VULN-006 (duplicate critical header rejection) as a dedicated, wired middleware +- `ClientCertInfo` struct is preserved for potential future use in extracting certificate details from TLS sessions --- diff --git a/SECURITY_FINDINGS_REPORT.md b/SECURITY_FINDINGS_REPORT.md index a0992af..0740ef2 100644 --- a/SECURITY_FINDINGS_REPORT.md +++ b/SECURITY_FINDINGS_REPORT.md @@ -265,5 +265,39 @@ The Linux_Patch_API Phase 3 is now **SECURE FOR DEPLOYMENT** in an internal netw --- +## Architecture Decision Record: rustls as Authoritative Client-Auth Gate + +**Date:** 2026-06-06 +**Status:** Accepted +**Context:** Issue #13 + +### Decision + +Client certificate authentication is enforced at the TLS handshake level by rustls via `CrlAwareVerifier`, NOT by application-layer middleware. + +### Context + +The original `MtlsMiddleware` was never wired into the Actix-web pipeline (dead code). It contained: +1. A duplicate-header check (VULN-006) that never ran +2. A `validate_client_certificate()` stub that returned `Ok(())` unconditionally + +Meanwhile, actual client certificate verification was always performed by rustls at the TLS handshake level through `CrlAwareVerifier` (which wraps `WebPkiClientVerifier`), with CRL revocation checking integrated into the same path. + +### Changes Made + +1. **Removed dead code:** `MtlsMiddleware`, `MtlsMiddlewareService`, `validate_client_certificate()`, and the Transform/Service impls +2. **Extracted VULN-006:** `has_duplicate_critical_headers()` moved to new `SecurityHeadersMiddleware` (wired into pipeline) +3. **Converted `build_rustls_config()`** from method on `MtlsMiddleware` to free function +4. **Preserved:** `CrlAwareVerifier`, `MtlsConfig`, `MtlsError`, `ClientCertInfo`, `build_rustls_config()`, and all CRL infrastructure + +### Rationale + +- rustls provides battle-tested X.509 verification at the TLS handshake level +- Enforcing auth at the TLS layer eliminates bypass vulnerabilities (middleware ordering bugs, route-specific skips) +- CRL revocation checking is integrated into the same handshake path +- Application-layer certificate validation is redundant when TLS already rejects untrusted connections + +--- + **Report Generated:** 2026-04-09T22:57:00Z **Verified By:** Security Verification Agent (Agent Zero) diff --git a/src/auth/mod.rs b/src/auth/mod.rs index ce5be04..66531bd 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -1,17 +1,27 @@ -//! Auth Module - mTLS and IP Whitelist Enforcement +//! Auth Module - mTLS, IP Whitelist, and Security Headers //! //! This module provides security authentication and authorization: -//! - mTLS (Mutual TLS) certificate-based authentication +//! - mTLS (Mutual TLS) certificate-based authentication (enforced at TLS handshake by rustls) //! - IP whitelist enforcement with CIDR subnet support +//! - Security header validation (VULN-006: duplicate critical header rejection) //! - Silent drop for non-compliant connections //! - Comprehensive audit logging +//! +//! # Architecture Decision Record: rustls as Authoritative Client-Auth Gate +//! +//! Client certificate authentication is enforced at the TLS handshake level by +//! rustls via `CrlAwareVerifier`. No application-layer certificate validation +//! middleware is needed — rustls rejects connections that fail client-cert +//! verification before any HTTP request is processed. See `mtls.rs` for details. pub mod crl; pub mod mtls; +pub mod security_headers; pub mod whitelist; pub use crl::{new_shared_state, CrlState, CrlStatus, SharedCrlState}; -pub use mtls::{ClientCertInfo, MtlsConfig, MtlsError, MtlsMiddleware}; +pub use mtls::{ClientCertInfo, MtlsConfig, MtlsError}; +pub use security_headers::SecurityHeadersMiddleware; pub use whitelist::{ WhitelistConfig, WhitelistEntry, WhitelistManager, WhitelistMiddleware, WhitelistMiddlewareService, diff --git a/src/auth/mtls.rs b/src/auth/mtls.rs index 7bed771..38063b9 100644 --- a/src/auth/mtls.rs +++ b/src/auth/mtls.rs @@ -1,20 +1,33 @@ -//! mTLS Authentication Module +//! mTLS Configuration Module //! -//! Provides mutual TLS authentication middleware for Actix-web. -//! Non-mTLS connections are silently dropped (no response). -//! Supports CRL-aware client certificate verification when CRL is available. +//! Provides rustls-based mutual TLS configuration for the API server. +//! +//! # Architecture Decision Record: rustls as Authoritative Client-Auth Gate +//! +//! Client certificate authentication is enforced at the TLS handshake level by +//! rustls via `CrlAwareVerifier` (which wraps `WebPkiClientVerifier`). This means: +//! +//! - **rustls + CrlAwareVerifier IS the authoritative client-auth gate.** +//! - No application-layer certificate validation middleware is needed because +//! rustls rejects connections that fail client-cert verification before any +//! HTTP request is processed. +//! - `build_rustls_config()` configures the TLS listener to require client +//! certificates (`with_client_cert_verifier`), making mTLS enforcement +//! unavoidable at the transport layer. +//! - CRL revocation checking is integrated into the same handshake path via +//! `CrlAwareVerifier`, so revoked certificates are also rejected before any +//! HTTP handler runs. +//! +//! This design was chosen because rustls provides battle-tested X.509 +//! verification, and enforcing auth at the TLS layer eliminates an entire +//! class of bypass vulnerabilities that application-layer checks are +//! susceptible to (e.g., middleware ordering bugs, route-specific skips). -use actix_web::{ - dev::{forward_ready, Service, ServiceRequest, ServiceResponse, Transform}, - Error, HttpMessage, -}; -#[allow(unused_imports)] -use chrono::{DateTime, Duration, Utc}; -use futures_util::future::LocalBoxFuture; +use chrono::{DateTime, Utc}; use rustls::{ client::danger::HandshakeSignatureValid, crypto::aws_lc_rs, - pki_types::{CertificateDer, UnixTime}, + pki_types::CertificateDer, server::{ danger::{ClientCertVerified, ClientCertVerifier}, ServerConfig, WebPkiClientVerifier, @@ -24,35 +37,10 @@ use rustls::{ }; use rustls_pemfile::{certs, private_key}; use std::{fs::File, io::BufReader, sync::Arc}; -use tracing::{debug, error, info, warn}; +use tracing::{error, info, warn}; use super::crl::{cert_serial_hex, SharedCrlState}; -/// Check for duplicate critical headers (VULN-006) -/// Returns true if duplicate headers are detected -fn has_duplicate_critical_headers(req: &ServiceRequest) -> bool { - let critical_headers = ["content-type", "authorization", "host"]; - - for header_name in critical_headers.iter() { - // Count occurrences of this header - let mut count = 0; - for (name, _) in req.headers().iter() { - if name.as_str().eq_ignore_ascii_case(header_name) { - count += 1; - if count > 1 { - warn!( - peer_addr = ?req.peer_addr(), - header = header_name, - "Duplicate critical header detected - rejecting request" - ); - return true; - } - } - } - } - false -} - /// CRL-aware client certificate verifier. /// /// Wraps WebPkiClientVerifier for chain validation, then checks the @@ -87,7 +75,7 @@ impl ClientCertVerifier for CrlAwareVerifier { &self, end_entity: &CertificateDer<'_>, intermediates: &[CertificateDer<'_>], - now: UnixTime, + now: rustls::pki_types::UnixTime, ) -> Result { // 1. Delegate chain validation to WebPKI self.inner @@ -163,51 +151,40 @@ pub struct MtlsConfig { pub min_tls_version: String, } -/// mTLS Middleware for Actix-web -pub struct MtlsMiddleware { - config: Arc, - cert_store: Arc, -} +/// Build a rustls ServerConfig with client certificate verification. +/// +/// This is the authoritative mTLS gate — rustls enforces client certificate +/// validation at the TLS handshake level, before any HTTP request is processed. +/// +/// When `crl_state` is provided and the CRL is available, wraps the +/// WebPkiClientVerifier with CrlAwareVerifier for revocation checking. +/// When CRL is missing/degraded, falls back to WebPKI-only verification. +pub fn build_rustls_config( + config: &MtlsConfig, + crl_state: Option, +) -> Result, MtlsError> { + let cert_store = load_ca_certs(&config.ca_cert_path)?; -impl MtlsMiddleware { - /// Create a new mTLS middleware - pub fn new(config: MtlsConfig) -> Result { - let cert_store = load_ca_certs(&config.ca_cert_path)?; + let webpki_verifier = WebPkiClientVerifier::builder(cert_store.clone().into()) + .build() + .map_err(|e| MtlsError::ClientVerifierError(e.to_string()))?; - Ok(Self { - config: Arc::new(config), - cert_store: Arc::new(cert_store), - }) - } + let client_verifier: Arc = match crl_state { + Some(state) => { + info!("CRL-aware client verification enabled"); + Arc::new(CrlAwareVerifier::new(webpki_verifier, state)) + } + None => { + info!("No CRL state provided -- using WebPKI-only client verification"); + webpki_verifier + } + }; - /// Build rustls server configuration with client certificate verification. - /// - /// When `crl_state` is provided and the CRL is available, wraps the - /// WebPkiClientVerifier with CrlAwareVerifier for revocation checking. - /// When CRL is missing/degraded, falls back to WebPKI-only verification. - pub fn build_rustls_config( - &self, - crl_state: Option, - ) -> Result, MtlsError> { - let webpki_verifier = WebPkiClientVerifier::builder(self.cert_store.clone()) - .build() - .map_err(|e| MtlsError::ClientVerifierError(e.to_string()))?; + let server_cert = load_certs(&config.server_cert_path)?; + let server_key = load_private_key(&config.server_key_path)?; - let client_verifier: Arc = match crl_state { - Some(state) => { - info!("CRL-aware client verification enabled"); - Arc::new(CrlAwareVerifier::new(webpki_verifier, state)) - } - None => { - info!("No CRL state provided -- using WebPKI-only client verification"); - webpki_verifier - } - }; - - let server_cert = load_certs(&self.config.server_cert_path)?; - let server_key = load_private_key(&self.config.server_key_path)?; - - let config = ServerConfig::builder_with_provider(Arc::new(aws_lc_rs::default_provider())) + let server_config = + ServerConfig::builder_with_provider(Arc::new(aws_lc_rs::default_provider())) .with_protocol_versions(&[&TLS13]) .map_err(|e| { MtlsError::ServerConfigError(format!("Failed to set TLS 1.3 only: {}", e)) @@ -216,8 +193,7 @@ impl MtlsMiddleware { .with_single_cert(server_cert, server_key) .map_err(|e| MtlsError::ServerConfigError(e.to_string()))?; - Ok(Arc::new(config)) - } + Ok(Arc::new(server_config)) } /// Load CA certificates from PEM file @@ -268,6 +244,21 @@ fn load_private_key(path: &str) -> Result, + pub not_after: DateTime, +} + /// mTLS Error types #[derive(Debug, thiserror::Error)] pub enum MtlsError { @@ -285,229 +276,16 @@ pub enum MtlsError { ValidationError(String), } -impl Transform for MtlsMiddleware -where - S: Service, Error = Error>, - S::Future: 'static, - B: 'static, -{ - type Response = ServiceResponse; - type Error = Error; - type InitError = (); - type Transform = MtlsMiddlewareService; - type Future = futures_util::future::Ready>; - - fn new_transform(&self, service: S) -> Self::Future { - futures_util::future::ok(MtlsMiddlewareService { - service, - config: self.config.clone(), - cert_store: self.cert_store.clone(), - }) - } -} - -pub struct MtlsMiddlewareService { - service: S, - #[allow(dead_code)] - config: Arc, - cert_store: Arc, -} - -impl Service for MtlsMiddlewareService -where - S: Service, Error = Error>, - S::Future: 'static, - B: 'static, -{ - type Response = ServiceResponse; - type Error = Error; - type Future = LocalBoxFuture<'static, Result>; - - forward_ready!(service); - - fn call(&self, req: ServiceRequest) -> Self::Future { - let cert_store = self.cert_store.clone(); - let peer_addr = req.peer_addr(); - - // VULN-006: Check for duplicate critical headers before processing - if has_duplicate_critical_headers(&req) { - warn!( - peer_addr = ?peer_addr, - "Duplicate critical headers detected - rejecting request (VULN-006)" - ); - return Box::pin(async move { - Err(actix_web::error::ErrorBadRequest( - "Duplicate critical headers not allowed", - )) - }); - } - - // Check for client certificate in request extensions - // In a proper mTLS setup with Actix-web + rustls, the certificate - // would be extracted from the TLS connection before reaching this middleware - let has_client_cert = req.extensions().get::().is_some(); - - if !has_client_cert { - // No client certificate provided - silent drop - warn!( - peer_addr = ?peer_addr, - "No client certificate provided - dropping connection (mTLS required)" - ); - // Return error immediately without calling service - return Box::pin(async move { - Err(actix_web::error::ErrorBadRequest( - "Client certificate required", - )) - }); - } - - // Certificate present - validate it - let cert_info = req.extensions().get::().cloned(); - - if let Some(info) = cert_info { - // Validate certificate against CA store - match validate_client_certificate(&info, &cert_store) { - Ok(_) => { - info!( - subject = %info.subject, - issuer = %info.issuer, - peer_addr = ?peer_addr, - "mTLS client certificate validated successfully" - ); - } - Err(e) => { - warn!( - error = %e, - peer_addr = ?peer_addr, - "mTLS client certificate validation failed - dropping connection" - ); - return Box::pin(async move { - Err(actix_web::error::ErrorBadRequest( - "Certificate validation failed", - )) - }); - } - } - } else { - warn!( - peer_addr = ?peer_addr, - "No client certificate provided - dropping connection (mTLS required)" - ); - return Box::pin(async move { - Err(actix_web::error::ErrorBadRequest( - "Client certificate required", - )) - }); - } - - debug!("mTLS authentication passed for request"); - - // All checks passed - call the service - let fut = self.service.call(req); - Box::pin(fut) - } -} - -/// Certificate information extracted from client certificate -#[derive(Debug, Clone)] -pub struct ClientCertInfo { - pub subject: String, - pub issuer: String, - pub serial: String, - pub not_before: DateTime, - pub not_after: DateTime, -} - -/// Validate client certificate against CA store -fn validate_client_certificate( - cert_info: &ClientCertInfo, - _cert_store: &RootCertStore, -) -> Result<(), MtlsError> { - // Check certificate validity period - let now = Utc::now(); - - if now < cert_info.not_before { - return Err(MtlsError::ValidationError( - "Certificate is not yet valid".to_string(), - )); - } - - if now > cert_info.not_after { - return Err(MtlsError::ValidationError( - "Certificate has expired".to_string(), - )); - } - - // In production, would verify certificate chain against CA store - // For now, we trust certificates that were extracted from the TLS connection - - Ok(()) -} - #[cfg(test)] mod tests { use super::*; + use std::collections::HashSet; - #[test] - fn test_mtls_config_creation() { - let config = MtlsConfig { - ca_cert_path: "/etc/linux_patch_api/certs/ca.pem".to_string(), - server_cert_path: "/etc/linux_patch_api/certs/server.pem".to_string(), - server_key_path: "/etc/linux_patch_api/certs/server.key".to_string(), - min_tls_version: "1.3".to_string(), - }; - - assert_eq!(config.ca_cert_path, "/etc/linux_patch_api/certs/ca.pem"); - assert_eq!(config.min_tls_version, "1.3"); - } - - #[test] - fn test_client_cert_info() { - let info = ClientCertInfo { - subject: "CN=test-client".to_string(), - issuer: "CN=Test CA".to_string(), - serial: "12345".to_string(), - not_before: Utc::now() - Duration::days(1), - not_after: Utc::now() + Duration::days(365), - }; - - assert!(info.subject.contains("CN=")); - assert!(info.issuer.contains("CN=")); - - // Test validation with valid cert - let cert_store = RootCertStore::empty(); - assert!(validate_client_certificate(&info, &cert_store).is_ok()); - } - - #[test] - fn test_client_cert_expired() { - let info = ClientCertInfo { - subject: "CN=expired-client".to_string(), - issuer: "CN=Test CA".to_string(), - serial: "12345".to_string(), - not_before: Utc::now() - Duration::days(365), - not_after: Utc::now() - Duration::days(1), - }; - - let cert_store = RootCertStore::empty(); - let result = validate_client_certificate(&info, &cert_store); - assert!(result.is_err()); - assert!(result.unwrap_err().to_string().contains("expired")); - } - - // ----------------------------------------------------------------------- - // CrlAwareVerifier unit tests - // ----------------------------------------------------------------------- - - /// Test that CrlAwareVerifier can be constructed with a WebPKI verifier - /// and a SharedCrlState. This verifies the wiring is correct. - #[test] - fn crl_aware_verifier_construction() { + fn init_crypto_provider() { let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); - use super::super::crl::{new_shared_state, CrlState, CrlStatus}; - use std::collections::HashSet; + } - // Build a simple CA cert + key for the root store. + fn make_test_ca_and_root_store() -> (rcgen::KeyPair, RootCertStore) { let ca_key = rcgen::KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256).unwrap(); let mut ca_params = rcgen::CertificateParams::default(); ca_params.not_before = time::OffsetDateTime::now_utc(); @@ -519,18 +297,26 @@ mod tests { ca_params.distinguished_name = dn; let ca_cert = ca_params.self_signed(&ca_key).unwrap(); - // Build root cert store with the CA. let mut root_store = RootCertStore::empty(); root_store.add(ca_cert.der().to_owned()).unwrap(); - // Build WebPKI verifier — build() returns Arc - // which coerces to Arc. + (ca_key, root_store) + } + + /// Test that CrlAwareVerifier can be constructed with a WebPKI verifier + /// and a SharedCrlState. This verifies the wiring is correct. + #[test] + fn crl_aware_verifier_construction() { + init_crypto_provider(); + use super::super::crl::{new_shared_state, CrlState, CrlStatus}; + + let (_ca_key, root_store) = make_test_ca_and_root_store(); + let webpki_verifier: Arc = WebPkiClientVerifier::builder(root_store.into()) .build() .unwrap(); - // Build CRL state in Valid status. let crl_state = new_shared_state(); let valid_state = CrlState { status: CrlStatus::Valid, @@ -540,31 +326,16 @@ mod tests { }; crl_state.store(Arc::new(valid_state)); - // Construct CrlAwareVerifier — should succeed. let _verifier = CrlAwareVerifier::new(webpki_verifier, crl_state); - // If we reach here without panic, construction succeeded. } /// Test that CrlAwareVerifier with Missing CRL state can be constructed. - /// Missing CRL means the verifier falls back to WebPKI-only. #[test] fn crl_aware_verifier_with_missing_crl() { - let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); + init_crypto_provider(); use super::super::crl::new_shared_state; - let ca_key = rcgen::KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256).unwrap(); - let mut ca_params = rcgen::CertificateParams::default(); - ca_params.not_before = time::OffsetDateTime::now_utc(); - ca_params.not_after = time::OffsetDateTime::now_utc() + time::Duration::days(365); - ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); - ca_params.key_usages = vec![rcgen::KeyUsagePurpose::KeyCertSign]; - let mut dn = rcgen::DistinguishedName::new(); - dn.push(rcgen::DnType::CommonName, "Test CA for Verifier"); - ca_params.distinguished_name = dn; - let ca_cert = ca_params.self_signed(&ca_key).unwrap(); - - let mut root_store = RootCertStore::empty(); - root_store.add(ca_cert.der().to_owned()).unwrap(); + let (_ca_key, root_store) = make_test_ca_and_root_store(); let webpki_verifier: Arc = WebPkiClientVerifier::builder(root_store.into()) @@ -577,26 +348,12 @@ mod tests { } /// Test that CrlAwareVerifier with Invalid CRL state can be constructed. - /// Invalid CRL means the verifier should reject ALL client certificates. #[test] fn crl_aware_verifier_with_invalid_crl() { - let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); + init_crypto_provider(); use super::super::crl::{new_shared_state, CrlState, CrlStatus}; - use std::collections::HashSet; - let ca_key = rcgen::KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256).unwrap(); - let mut ca_params = rcgen::CertificateParams::default(); - ca_params.not_before = time::OffsetDateTime::now_utc(); - ca_params.not_after = time::OffsetDateTime::now_utc() + time::Duration::days(365); - ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); - ca_params.key_usages = vec![rcgen::KeyUsagePurpose::KeyCertSign]; - let mut dn = rcgen::DistinguishedName::new(); - dn.push(rcgen::DnType::CommonName, "Test CA for Verifier"); - ca_params.distinguished_name = dn; - let ca_cert = ca_params.self_signed(&ca_key).unwrap(); - - let mut root_store = RootCertStore::empty(); - root_store.add(ca_cert.der().to_owned()).unwrap(); + let (_ca_key, root_store) = make_test_ca_and_root_store(); let webpki_verifier: Arc = WebPkiClientVerifier::builder(root_store.into()) @@ -616,27 +373,13 @@ mod tests { } /// Test that CrlAwareVerifier with a revoked serial in Valid CRL state - /// can be constructed. The actual verification logic is tested through - /// integration tests since it requires a full TLS handshake. + /// can be constructed. #[test] fn crl_aware_verifier_with_revoked_serial() { - let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); + init_crypto_provider(); use super::super::crl::{new_shared_state, CrlState, CrlStatus}; - use std::collections::HashSet; - let ca_key = rcgen::KeyPair::generate_for(&rcgen::PKCS_ECDSA_P256_SHA256).unwrap(); - let mut ca_params = rcgen::CertificateParams::default(); - ca_params.not_before = time::OffsetDateTime::now_utc(); - ca_params.not_after = time::OffsetDateTime::now_utc() + time::Duration::days(365); - ca_params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); - ca_params.key_usages = vec![rcgen::KeyUsagePurpose::KeyCertSign]; - let mut dn = rcgen::DistinguishedName::new(); - dn.push(rcgen::DnType::CommonName, "Test CA for Verifier"); - ca_params.distinguished_name = dn; - let ca_cert = ca_params.self_signed(&ca_key).unwrap(); - - let mut root_store = RootCertStore::empty(); - root_store.add(ca_cert.der().to_owned()).unwrap(); + let (_ca_key, root_store) = make_test_ca_and_root_store(); let webpki_verifier: Arc = WebPkiClientVerifier::builder(root_store.into()) @@ -655,6 +398,5 @@ mod tests { crl_state.store(Arc::new(valid_with_revoked)); let _verifier = CrlAwareVerifier::new(webpki_verifier, crl_state); - // Construction succeeded — the verifier is ready to reject revoked certs. } } diff --git a/src/auth/security_headers.rs b/src/auth/security_headers.rs new file mode 100644 index 0000000..18bcf3e --- /dev/null +++ b/src/auth/security_headers.rs @@ -0,0 +1,166 @@ +//! Security Headers Middleware Module +//! +//! Provides request-level security header validation for Actix-web. +//! Enforces VULN-006: rejects requests with duplicate critical headers +//! (content-type, authorization, host) to prevent HTTP request smuggling +//! and response-splitting attacks. + +use actix_web::{ + dev::{forward_ready, Service, ServiceRequest, ServiceResponse, Transform}, + Error, +}; +use futures_util::future::LocalBoxFuture; +use tracing::warn; + +/// Critical headers that MUST NOT appear more than once in a request. +/// Duplicate values for these headers can enable request smuggling, +/// response splitting, and other HTTP parsing ambiguities. +const CRITICAL_HEADERS: &[&str] = &["content-type", "authorization", "host"]; + +/// Security headers middleware for Actix-web. +/// +/// Checks every incoming request for duplicate critical headers (VULN-006) +/// and rejects malformed requests with HTTP 400 Bad Request. +pub struct SecurityHeadersMiddleware; + +impl SecurityHeadersMiddleware { + /// Create a new security headers middleware instance. + pub fn new() -> Self { + Self + } +} + +impl Default for SecurityHeadersMiddleware { + fn default() -> Self { + Self::new() + } +} + +/// Actix-web Transform implementation — wraps SecurityHeadersMiddleware as middleware +impl Transform for SecurityHeadersMiddleware +where + S: Service, Error = Error>, + S::Future: 'static, + B: 'static, +{ + type Response = ServiceResponse; + type Error = Error; + type InitError = (); + type Transform = SecurityHeadersMiddlewareService; + type Future = futures_util::future::Ready>; + + fn new_transform(&self, service: S) -> Self::Future { + futures_util::future::ok(SecurityHeadersMiddlewareService { service }) + } +} + +/// Security headers middleware service — performs per-request duplicate header checks +pub struct SecurityHeadersMiddlewareService { + service: S, +} + +impl Service for SecurityHeadersMiddlewareService +where + S: Service, Error = Error>, + S::Future: 'static, + B: 'static, +{ + type Response = ServiceResponse; + type Error = Error; + type Future = LocalBoxFuture<'static, Result>; + + forward_ready!(service); + + fn call(&self, req: ServiceRequest) -> Self::Future { + // VULN-006: Check for duplicate critical headers before processing + if has_duplicate_critical_headers(req.headers()) { + let peer_addr = req.peer_addr(); + warn!( + peer_addr = ?peer_addr, + "Duplicate critical headers detected - rejecting request (VULN-006)" + ); + return Box::pin(async move { + Err(actix_web::error::ErrorBadRequest( + "Duplicate critical headers not allowed", + )) + }); + } + + // All checks passed — call the service + let fut = self.service.call(req); + Box::pin(fut) + } +} + +/// Check for duplicate critical headers (VULN-006). +/// Returns true if any critical header appears more than once. +/// +/// This function is public for testing purposes. +pub fn has_duplicate_critical_headers(headers: &actix_web::http::header::HeaderMap) -> bool { + for header_name in CRITICAL_HEADERS.iter() { + let mut count = 0; + for (name, _value) in headers.iter() { + if name.as_str().eq_ignore_ascii_case(header_name) { + count += 1; + if count > 1 { + return true; + } + } + } + } + false +} + +#[cfg(test)] +mod tests { + use super::*; + use actix_web::http::header; + + #[test] + fn test_no_duplicate_headers_passes() { + let mut headers = header::HeaderMap::new(); + headers.insert(header::CONTENT_TYPE, "application/json".parse().unwrap()); + headers.insert(header::AUTHORIZATION, "Bearer test".parse().unwrap()); + headers.insert(header::HOST, "localhost".parse().unwrap()); + assert!(!has_duplicate_critical_headers(&headers)); + } + + #[test] + fn test_duplicate_content_type_rejected() { + let mut headers = header::HeaderMap::new(); + headers.insert(header::CONTENT_TYPE, "application/json".parse().unwrap()); + headers.append(header::CONTENT_TYPE, "text/plain".parse().unwrap()); + assert!(has_duplicate_critical_headers(&headers)); + } + + #[test] + fn test_duplicate_authorization_rejected() { + let mut headers = header::HeaderMap::new(); + headers.insert(header::AUTHORIZATION, "Bearer test1".parse().unwrap()); + headers.append(header::AUTHORIZATION, "Bearer test2".parse().unwrap()); + assert!(has_duplicate_critical_headers(&headers)); + } + + #[test] + fn test_duplicate_host_rejected() { + let mut headers = header::HeaderMap::new(); + headers.insert(header::HOST, "localhost".parse().unwrap()); + headers.append(header::HOST, "evil.com".parse().unwrap()); + assert!(has_duplicate_critical_headers(&headers)); + } + + #[test] + fn test_non_critical_duplicate_headers_allowed() { + // Duplicate non-critical headers should be allowed + let mut headers = header::HeaderMap::new(); + headers.insert(header::ACCEPT, "text/html".parse().unwrap()); + headers.append(header::ACCEPT, "application/json".parse().unwrap()); + assert!(!has_duplicate_critical_headers(&headers)); + } + + #[test] + fn test_empty_headers_passes() { + let headers = header::HeaderMap::new(); + assert!(!has_duplicate_critical_headers(&headers)); + } +} diff --git a/src/main.rs b/src/main.rs index a3363dc..dd8066d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -28,7 +28,9 @@ use tracing::{error, info, warn}; use linux_patch_api::api::{configure_api_routes, configure_health_route}; use linux_patch_api::auth::crl::{self, CrlStatus}; -use linux_patch_api::auth::{mtls, MtlsMiddleware, WhitelistManager, WhitelistMiddleware}; +use linux_patch_api::auth::{ + mtls, SecurityHeadersMiddleware, WhitelistManager, WhitelistMiddleware, +}; use linux_patch_api::config::loader::{validate_certs, CertStatus}; use linux_patch_api::enroll; use linux_patch_api::packages::cache::PackageCacheState; @@ -310,9 +312,14 @@ async fn main() -> Result<()> { let wl = whitelist_manager.clone(); // Create server builder + // Security middleware stack (order matters): + // 1. WhitelistMiddleware — IP-based access control (deny-by-default) + // 2. SecurityHeadersMiddleware — VULN-006: reject duplicate critical headers + // 3. Logger — request logging (after auth decisions) let server_builder = HttpServer::new(move || { let mut app = App::new() .wrap(WhitelistMiddleware::new(wl.clone())) + .wrap(SecurityHeadersMiddleware::new()) .wrap(Logger::default()) .app_data(job_manager_data.clone()) .app_data(backend_data.clone()) @@ -406,63 +413,58 @@ async fn main() -> Result<()> { info!("No manager URL configured -- CRL auto-refresh disabled"); } - match MtlsMiddleware::new(mtls_config.clone()) { - Ok(middleware) => { - // Build rustls server configuration with CRL-aware verifier - let rustls_config = middleware - .build_rustls_config(Some(shared_crl_state.clone())) - .map_err(|e| anyhow::anyhow!("Failed to build rustls config: {}", e))?; + // ADR: rustls is the authoritative client-auth gate. + // Client certificate verification happens at the TLS handshake level + // via CrlAwareVerifier (which wraps WebPkiClientVerifier). No + // application-layer certificate validation middleware is needed. + // See src/auth/mtls.rs for the full ADR. + let rustls_config = mtls::build_rustls_config(&mtls_config, Some(shared_crl_state.clone())) + .map_err(|e| anyhow::anyhow!("Failed to build rustls config: {}", e))?; - info!("mTLS middleware and rustls config initialized successfully"); + info!( + "mTLS rustls config initialized successfully (client auth enforced at TLS handshake)" + ); - // Create TCP listener with SO_REUSEADDR using socket2 - // This prevents "Address already in use" errors when restarting after a crash - let socket = socket2::Socket::new( - socket2::Domain::IPV4, - socket2::Type::STREAM, - Some(socket2::Protocol::TCP), - ) - .map_err(|e| anyhow::anyhow!("Failed to create socket: {}", e))?; + // Create TCP listener with SO_REUSEADDR using socket2 + // This prevents "Address already in use" errors when restarting after a crash + let socket = socket2::Socket::new( + socket2::Domain::IPV4, + socket2::Type::STREAM, + Some(socket2::Protocol::TCP), + ) + .map_err(|e| anyhow::anyhow!("Failed to create socket: {}", e))?; - socket - .set_reuse_address(true) - .map_err(|e| anyhow::anyhow!("Failed to set SO_REUSEADDR: {}", e))?; + socket + .set_reuse_address(true) + .map_err(|e| anyhow::anyhow!("Failed to set SO_REUSEADDR: {}", e))?; - let bind_addr: std::net::SocketAddr = bind_address.parse().map_err(|e| { - anyhow::anyhow!("Invalid bind address '{}': {}", bind_address, e) - })?; + let bind_addr: std::net::SocketAddr = bind_address + .parse() + .map_err(|e| anyhow::anyhow!("Invalid bind address '{}': {}", bind_address, e))?; - socket - .bind(&socket2::SockAddr::from(bind_addr)) - .map_err(|e| { - anyhow::anyhow!("Failed to bind socket to {}: {}", bind_address, e) - })?; + socket + .bind(&socket2::SockAddr::from(bind_addr)) + .map_err(|e| anyhow::anyhow!("Failed to bind socket to {}: {}", bind_address, e))?; - socket - .listen(128) - .map_err(|e| anyhow::anyhow!("Failed to listen on socket: {}", e))?; + socket + .listen(128) + .map_err(|e| anyhow::anyhow!("Failed to listen on socket: {}", e))?; - let tcp_listener: std::net::TcpListener = socket.into(); + let tcp_listener: std::net::TcpListener = socket.into(); - // Log listening AFTER successful bind - info!("Listening on {} (mTLS enabled)", bind_address); + // Log listening AFTER successful bind + info!("Listening on {} (mTLS enabled)", bind_address); - // Clone the ServerConfig from Arc for listen_rustls_0_23 - let server_config = (*rustls_config).clone(); + // Clone the ServerConfig from Arc for listen_rustls_0_23 + let server_config = (*rustls_config).clone(); - info!("Binding server with TLS 1.3 - non-TLS connections will be rejected"); + info!("Binding server with TLS 1.3 - non-TLS connections will be rejected"); - // Bind with TLS using rustls 0.23 - non-TLS connections fail at handshake - server_builder - .listen_rustls_0_23(tcp_listener, server_config)? - .run() - .await?; - } - Err(e) => { - error!(error = %e, "Failed to initialize mTLS middleware"); - return Err(anyhow::anyhow!("mTLS initialization failed: {}", e)); - } - } + // Bind with TLS using rustls 0.23 - non-TLS connections fail at handshake + server_builder + .listen_rustls_0_23(tcp_listener, server_config)? + .run() + .await?; } else { // Create TCP listener with SO_REUSEADDR for non-TLS mode let socket = socket2::Socket::new( diff --git a/tests/integration/auth_test.rs b/tests/integration/auth_test.rs index ceb6ce6..c121111 100644 --- a/tests/integration/auth_test.rs +++ b/tests/integration/auth_test.rs @@ -19,8 +19,14 @@ mod mtls_tests { }; assert_eq!(config.ca_cert_path, "/etc/linux_patch_api/certs/ca.pem"); - assert_eq!(config.server_cert_path, "/etc/linux_patch_api/certs/server.pem"); - assert_eq!(config.server_key_path, "/etc/linux_patch_api/certs/server.key"); + assert_eq!( + config.server_cert_path, + "/etc/linux_patch_api/certs/server.pem" + ); + assert_eq!( + config.server_key_path, + "/etc/linux_patch_api/certs/server.key" + ); assert_eq!(config.min_tls_version, "1.3"); } @@ -232,9 +238,61 @@ mod auth_result_tests { assert!(result.is_authenticated()); assert!(result.cert_info.is_some()); - assert_eq!( - result.cert_info.unwrap().subject, - "CN=client001" - ); + assert_eq!(result.cert_info.unwrap().subject, "CN=client001"); + } +} + +/// Integration tests for SecurityHeadersMiddleware (VULN-006) +#[cfg(test)] +mod security_headers_tests { + use actix_web::http::header; + use linux_patch_api::auth::security_headers::has_duplicate_critical_headers; + + #[test] + fn test_no_duplicate_headers_passes() { + let mut headers = header::HeaderMap::new(); + headers.insert(header::CONTENT_TYPE, "application/json".parse().unwrap()); + headers.insert(header::AUTHORIZATION, "Bearer test".parse().unwrap()); + headers.insert(header::HOST, "localhost".parse().unwrap()); + assert!(!has_duplicate_critical_headers(&headers)); + } + + #[test] + fn test_duplicate_content_type_detected() { + let mut headers = header::HeaderMap::new(); + headers.insert(header::CONTENT_TYPE, "application/json".parse().unwrap()); + headers.append(header::CONTENT_TYPE, "text/plain".parse().unwrap()); + assert!(has_duplicate_critical_headers(&headers)); + } + + #[test] + fn test_duplicate_authorization_detected() { + let mut headers = header::HeaderMap::new(); + headers.insert(header::AUTHORIZATION, "Bearer test1".parse().unwrap()); + headers.append(header::AUTHORIZATION, "Bearer test2".parse().unwrap()); + assert!(has_duplicate_critical_headers(&headers)); + } + + #[test] + fn test_duplicate_host_detected() { + let mut headers = header::HeaderMap::new(); + headers.insert(header::HOST, "localhost".parse().unwrap()); + headers.append(header::HOST, "evil.com".parse().unwrap()); + assert!(has_duplicate_critical_headers(&headers)); + } + + #[test] + fn test_non_critical_duplicates_allowed() { + // Duplicate Accept headers should be fine + let mut headers = header::HeaderMap::new(); + headers.insert(header::ACCEPT, "text/html".parse().unwrap()); + headers.append(header::ACCEPT, "application/json".parse().unwrap()); + assert!(!has_duplicate_critical_headers(&headers)); + } + + #[test] + fn test_empty_headers_passes() { + let headers = header::HeaderMap::new(); + assert!(!has_duplicate_critical_headers(&headers)); } }