Merge pull request #1087 from fabriziosestito/fix/tls-config

jvanz · web-flow · commit aa5bb785a391 · 2025-02-12T11:53:38.000-03:00
fix: TlsConfig and mTLS logic
diff --git a/cli-docs.md b/cli-docs.md
@@ -24,11 +24,7 @@ This document contains the help content for the `policy-server` command-line pro
   Default value: `0.0.0.0`
 * `--always-accept-admission-reviews-on-namespace <NAMESPACE>` — Always accept AdmissionReviews that target the given namespace
 * `--cert-file <CERT_FILE>` — Path to an X.509 certificate file for HTTPS
-
-  Default value: ``
 * `--client-ca-file <CLIENT_CA_FILE>` — Path to an CA certificate file that issued the client certificate. Required to enable mTLS
-
-  Default value: ``
 * `--daemon` — If set, runs policy-server in detached mode as a daemon
 * `--daemon-pid-file <DAEMON-PID-FILE>` — Path to the PID file, used only when running in daemon mode
 
@@ -41,8 +37,6 @@ This document contains the help content for the `policy-server` command-line pro
 * `--enable-pprof` — Enable pprof profiling
 * `--ignore-kubernetes-connection-failure` — Do not exit with an error if the Kubernetes connection fails. This will cause context-aware policies to break when there's no connection with Kubernetes.
 * `--key-file <KEY_FILE>` — Path to an X.509 private key file for HTTPS
-
-  Default value: ``
 * `--log-fmt <LOG_FMT>` — Log output format
 
   Default value: `text`
diff --git a/src/cli.rs b/src/cli.rs
@@ -84,21 +84,18 @@ pub(crate) fn build_cli() -> Command {
         Arg::new("cert-file")
             .long("cert-file")
             .value_name("CERT_FILE")
-            .default_value("")
             .env("KUBEWARDEN_CERT_FILE")
             .help("Path to an X.509 certificate file for HTTPS"),
 
         Arg::new("key-file")
             .long("key-file")
             .value_name("KEY_FILE")
-            .default_value("")
             .env("KUBEWARDEN_KEY_FILE")
             .help("Path to an X.509 private key file for HTTPS"),
 
         Arg::new("client-ca-file")
             .long("client-ca-file")
             .value_name("CLIENT_CA_FILE")
-            .default_value("")
             .env("KUBEWARDEN_CLIENT_CA_FILE")
             .help("Path to an CA certificate file that issued the client certificate. Required to enable mTLS"),
 
diff --git a/src/config.rs b/src/config.rs
@@ -54,7 +54,7 @@ pub struct Config {
 pub struct TlsConfig {
     pub cert_file: String,
     pub key_file: String,
-    pub client_ca_cert_file: Option<String>,
+    pub client_ca_file: Option<String>,
 }
 
 impl Config {
@@ -130,7 +130,7 @@ impl Config {
             .expect("clap should have assigned a default value")
             .to_owned();
 
-        let tls_config = Some(build_tls_config(matches)?);
+        let tls_config = build_tls_config(matches)?;
 
         let enable_pprof = matches
             .get_one::<bool>("enable-pprof")
@@ -189,18 +189,28 @@ fn readiness_probe_bind_address(matches: &clap::ArgMatches) -> Result<SocketAddr
     .map_err(|e| anyhow!("error parsing arguments: {}", e))
 }
 
-fn build_tls_config(matches: &clap::ArgMatches) -> Result<TlsConfig> {
-    let cert_file = matches.get_one::<String>("cert-file").unwrap().to_owned();
-    let key_file = matches.get_one::<String>("key-file").unwrap().to_owned();
-    let client_ca_cert_file = matches.get_one::<String>("client-ca-file").cloned();
-    if cert_file.is_empty() != key_file.is_empty() {
-        return Err(anyhow!("error parsing arguments: either both --cert-file and --key-file must be provided, or neither"));
+fn build_tls_config(matches: &clap::ArgMatches) -> Result<Option<TlsConfig>> {
+    let cert_file = matches.get_one::<String>("cert-file").cloned();
+    let key_file = matches.get_one::<String>("key-file").cloned();
+    let client_ca_file = matches.get_one::<String>("client-ca-file").cloned();
+
+    match (cert_file, key_file, &client_ca_file) {
+        (Some(cert_file), Some(key_file), _) => Ok(Some(TlsConfig {
+            cert_file,
+            key_file,
+            client_ca_file,
+        })),
+        // No TLS configuration provided
+        (None, None, None) => Ok(None),
+        // Client CA certificate provided without server certificate and key
+        (None, None, Some(_)) => Err(anyhow!(
+            "client CA certificate requires server certificate and key to be specified"
+        )),
+        // Server certificate or key provided without the other
+        (Some(_), None, _) | (None, Some(_), _) => Err(anyhow!(
+            "both certificate and key must be provided together"
+        )),
     }
-    Ok(TlsConfig {
-        cert_file,
-        key_file,
-        client_ca_cert_file,
-    })
 }
 
 fn policies(matches: &clap::ArgMatches) -> Result<HashMap<String, PolicyOrPolicyGroup>> {
diff --git a/src/lib.rs b/src/lib.rs
@@ -33,12 +33,17 @@ use policy_evaluator::{
 use profiling::activate_memory_profiling;
 use rayon::prelude::*;
 use std::{fs, net::SocketAddr, sync::Arc};
+use std::{fs::File, io::BufReader};
 use tokio::{
     sync::{oneshot, Notify, Semaphore},
     time,
 };
 use tower_http::trace::{self, TraceLayer};
 
+use rustls::{server::WebPkiClientVerifier, RootCertStore, ServerConfig};
+use rustls_pemfile::Item;
+use rustls_pki_types::{CertificateDer, PrivateKeyDer};
+
 // This is required by certificate hot reload when using inotify, which is available only on linux
 #[cfg(target_os = "linux")]
 use tokio_stream::StreamExt;
@@ -286,21 +291,14 @@ impl PolicyServer {
     }
 }
 
-// Load the ServerConfig to be used by the Policy Server configuring the server
-// certificate and mTLS when necessary
-//
-// RustlsConfig does not offer a function to load the client CA certificate together with the
-// service certificates. Therefore, we need to load everything and build the ServerConfig
+/// Load the ServerConfig to be used by the Policy Server configuring the server
+/// certificate and mTLS when necessary
+///
+/// RustlsConfig does not offer a function to load the client CA certificate together with the
+/// service certificates. Therefore, we need to load everything and build the ServerConfig
 async fn build_tls_server_config(tls_config: &TlsConfig) -> Result<rustls::ServerConfig> {
-    use std::{fs::File, io::BufReader};
-
-    use rustls::{server::WebPkiClientVerifier, RootCertStore, ServerConfig};
-    use rustls_pemfile::Item;
-    use rustls_pki_types::{CertificateDer, PrivateKeyDer};
-
-    let cert_file = &mut BufReader::new(File::open(tls_config.cert_file.clone())?);
-    let key_file = &mut BufReader::new(File::open(tls_config.key_file.clone())?);
-    let cert: Vec<CertificateDer> = rustls_pemfile::certs(cert_file)
+    let cert_reader = &mut BufReader::new(File::open(tls_config.cert_file.clone())?);
+    let cert: Vec<CertificateDer> = rustls_pemfile::certs(cert_reader)
         .filter_map(|it| {
             if let Err(ref e) = it {
                 warn!("Cannot parse certificate: {e}");
@@ -312,7 +310,9 @@ async fn build_tls_server_config(tls_config: &TlsConfig) -> Result<rustls::Serve
     if cert.len() > 1 {
         return Err(anyhow!("Multiple certificates provided in cert file"));
     }
-    let mut key_vec: Vec<Vec<u8>> = rustls_pemfile::read_all(key_file)
+
+    let key_file_reader = &mut BufReader::new(File::open(tls_config.key_file.clone())?);
+    let mut key_vec: Vec<Vec<u8>> = rustls_pemfile::read_all(key_file_reader)
         .filter_map(|i| match i.ok()? {
             Item::Sec1Key(key) => Some(key.secret_sec1_der().to_vec()),
             Item::Pkcs1Key(key) => Some(key.secret_pkcs1_der().to_vec()),
@@ -332,36 +332,35 @@ async fn build_tls_server_config(tls_config: &TlsConfig) -> Result<rustls::Serve
     let key = PrivateKeyDer::try_from(key_vec.pop().unwrap())
         .map_err(|e| anyhow!("Cannot parse server key: {e}"))?;
 
-    let config = if let Some(client_ca_cert_file_path) = tls_config.client_ca_cert_file.clone() {
+    if let Some(client_ca_file) = tls_config.client_ca_file.clone() {
         // we have the client CA. Therefore, we should enable mTLS.
-        let client_ca_cert_file = &mut BufReader::new(File::open(client_ca_cert_file_path)?);
+        let client_ca_reader = &mut BufReader::new(File::open(client_ca_file)?);
 
-        let mut ca_certs = RootCertStore::empty();
-        let client_ca_certs: Vec<_> = rustls_pemfile::certs(client_ca_cert_file)
+        let mut store = RootCertStore::empty();
+        let client_ca_certs: Vec<_> = rustls_pemfile::certs(client_ca_reader)
             .filter_map(|it| {
                 if let Err(ref e) = it {
                     warn!("Cannot parse client CA certificate: {e}");
                 }
                 it.ok()
             })
             .collect();
-        let (cert_added, cert_ignored) = ca_certs.add_parsable_certificates(client_ca_certs);
+        let (cert_added, cert_ignored) = store.add_parsable_certificates(client_ca_certs);
         info!(
             client_ca_certs_added = cert_added,
             client_ca_certs_ignored = cert_ignored,
             "Loaded client CA certificates"
         );
-        let client_verifier = WebPkiClientVerifier::builder(Arc::new(ca_certs)).build()?;
+        let client_verifier = WebPkiClientVerifier::builder(Arc::new(store)).build()?;
 
-        ServerConfig::builder()
+        return Ok(ServerConfig::builder()
             .with_client_cert_verifier(client_verifier)
-            .with_single_cert(cert, key)?
-    } else {
-        ServerConfig::builder()
-            .with_no_client_auth()
-            .with_single_cert(cert, key)?
-    };
-    Ok(config)
+            .with_single_cert(cert, key)?);
+    }
+
+    Ok(ServerConfig::builder()
+        .with_no_client_auth()
+        .with_single_cert(cert, key)?)
 }
 
 /// There's no watching of the certificate files on non-linux platforms
@@ -386,10 +385,6 @@ async fn create_tls_config_and_watch_certificate_changes(
 ) -> Result<RustlsConfig> {
     use ::tracing::error;
 
-    let cert_file_path = tls_config.cert_file.clone();
-    let key_file_path = tls_config.key_file.clone();
-    let client_ca_cert_path = tls_config.client_ca_cert_file.clone();
-
     let config = build_tls_server_config(&tls_config).await?;
 
     let rust_config = RustlsConfig::from_config(Arc::new(config));
@@ -399,19 +394,22 @@ async fn create_tls_config_and_watch_certificate_changes(
         inotify::Inotify::init().map_err(|e| anyhow!("Cannot initialize inotify: {e}"))?;
     let cert_watch = inotify
         .watches()
-        .add(cert_file_path.clone(), inotify::WatchMask::CLOSE_WRITE)
+        .add(
+            tls_config.cert_file.clone(),
+            inotify::WatchMask::CLOSE_WRITE,
+        )
         .map_err(|e| anyhow!("Cannot watch certificate file: {e}"))?;
     let key_watch = inotify
         .watches()
-        .add(key_file_path.clone(), inotify::WatchMask::CLOSE_WRITE)
+        .add(tls_config.key_file.clone(), inotify::WatchMask::CLOSE_WRITE)
         .map_err(|e| anyhow!("Cannot watch key file: {e}"))?;
 
     let mut client_cert_watch = None;
-    if let Some(ref client_cert_file) = client_ca_cert_path {
+    if let Some(ref client_ca_file) = tls_config.client_ca_file {
         client_cert_watch = Some(
             inotify
                 .watches()
-                .add(client_cert_file.clone(), inotify::WatchMask::CLOSE_WRITE)
+                .add(client_ca_file, inotify::WatchMask::CLOSE_WRITE)
                 .map_err(|e| anyhow!("Cannot watch client certificate file: {e}"))?,
         );
     }
diff --git a/tests/integration_test.rs b/tests/integration_test.rs
@@ -688,7 +688,7 @@ async fn test_detect_certificate_rotation() {
     config.tls_config = Some(policy_server::config::TlsConfig {
         cert_file: cert_file.to_str().unwrap().to_owned(),
         key_file: key_file.to_str().unwrap().to_owned(),
-        client_ca_cert_file: Some(client_ca.to_str().unwrap().to_owned()),
+        client_ca_file: Some(client_ca.to_str().unwrap().to_owned()),
     });
     config.policies = HashMap::new();
 
