From 967bc7c9df8841a2e124d5739f99abc4e63a6338 Mon Sep 17 00:00:00 2001 From: Firstyear Date: Tue, 23 Jan 2024 16:13:14 +1000 Subject: [PATCH] Improve TLS configuration errors (#2447) This improves the errors during TLS configuration to localise them to the error site, as well as calling our file path diagnostics tool to assist with permission errors. --- server/core/src/config.rs | 14 +++++------ server/core/src/crypto.rs | 47 +++++++++++++++++++++++++----------- server/core/src/https/mod.rs | 5 +++- server/core/src/lib.rs | 14 ++++------- 4 files changed, 49 insertions(+), 31 deletions(-) diff --git a/server/core/src/config.rs b/server/core/src/config.rs index 122371077..87247c1e9 100644 --- a/server/core/src/config.rs +++ b/server/core/src/config.rs @@ -7,7 +7,7 @@ use std::fmt; use std::fs::File; use std::io::Read; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::str::FromStr; use kanidm_proto::constants::DEFAULT_SERVER_ADDRESS; @@ -75,9 +75,9 @@ fn default_online_backup_versions() -> usize { #[derive(Deserialize, Debug, Clone)] pub struct TlsConfiguration { - pub chain: String, - pub key: String, - pub client_ca: Option, + pub chain: PathBuf, + pub key: PathBuf, + pub client_ca: Option, } /// This is the Server Configuration as read from `server.toml`. @@ -648,9 +648,9 @@ impl Configuration { match (chain, key) { (None, None) => {} (Some(chainp), Some(keyp)) => { - let chain = chainp.to_string(); - let key = keyp.to_string(); - let client_ca = client_ca.clone(); + let chain = PathBuf::from(keyp.clone()); + let key = PathBuf::from(chainp.clone()); + let client_ca = client_ca.clone().map(PathBuf::from); self.tls_config = Some(TlsConfiguration { chain, key, diff --git a/server/core/src/crypto.rs b/server/core/src/crypto.rs index 2953e618e..588b63ca6 100644 --- a/server/core/src/crypto.rs +++ b/server/core/src/crypto.rs @@ -93,30 +93,49 @@ pub fn check_privkey_minimums(privkey: &PKeyRef) -> Result<(), String> /// From the server configuration, generate an OpenSSL acceptor that we can use /// to build our sockets for HTTPS/LDAPS. -pub fn setup_tls(config: &Configuration) -> Result, ErrorStack> { +pub fn setup_tls(config: &Configuration) -> Result, ()> { match &config.tls_config { Some(tls_config) => { // Signing algorithm minimums are enforced by the SSLAcceptor - it won't start up with a sha1-signed cert. - let mut ssl_builder = SslAcceptor::mozilla_modern(SslMethod::tls())?; - ssl_builder.set_certificate_chain_file(&tls_config.chain)?; + // https://wiki.mozilla.org/Security/Server_Side_TLS + let mut ssl_builder = + SslAcceptor::mozilla_intermediate_v5(SslMethod::tls()).map_err(|openssl_err| { + error!("Failed to start TLS builder"); + error!(?openssl_err); + })?; - ssl_builder.set_private_key_file(&tls_config.key, SslFiletype::PEM)?; - ssl_builder.check_private_key()?; + ssl_builder + .set_certificate_chain_file(&tls_config.chain) + .map_err(|openssl_err| { + error!("Failed to access certificate chain file"); + error!(?openssl_err); + let diag = kanidm_lib_file_permissions::diagnose_path(&tls_config.chain); + info!(%diag); + })?; + + ssl_builder + .set_private_key_file(&tls_config.key, SslFiletype::PEM) + .map_err(|openssl_err| { + error!("Failed to access private key file"); + error!(?openssl_err); + let diag = kanidm_lib_file_permissions::diagnose_path(&tls_config.chain); + info!(%diag); + })?; + + ssl_builder.check_private_key().map_err(|openssl_err| { + error!("Failed to validate private key"); + error!(?openssl_err); + })?; let acceptor = ssl_builder.build(); // let's enforce some TLS minimums! - #[allow(clippy::expect_used)] - let privkey = acceptor - .context() - .private_key() - .expect("Couldn't pull TLS key after configuring one!"); + let privkey = acceptor.context().private_key().ok_or_else(|| { + error!("Failed to access acceptor private key"); + })?; check_privkey_minimums(privkey).map_err(|err| { - #[cfg(any(test, debug_assertions))] - println!("{}", err); - admin_error!("{}", err); - ErrorStack::get() // this probably should be a real errorstack but... how? + error!("{}", err); })?; Ok(Some(acceptor)) diff --git a/server/core/src/https/mod.rs b/server/core/src/https/mod.rs index 728cc26a7..9e3d685ee 100644 --- a/server/core/src/https/mod.rs +++ b/server/core/src/https/mod.rs @@ -397,7 +397,10 @@ async fn server_loop( // If configured, setup TLS client authentication. if let Some(client_ca) = tls_param.client_ca.as_ref() { - debug!("Configuring client certificates from {}", client_ca); + debug!( + "Configuring client certificates from {}", + client_ca.display() + ); let verify = SslVerifyMode::PEER; // In future we may add a "require mTLS option" which would necesitate this. diff --git a/server/core/src/lib.rs b/server/core/src/lib.rs index 63b5a938f..5ac741db9 100644 --- a/server/core/src/lib.rs +++ b/server/core/src/lib.rs @@ -36,7 +36,6 @@ mod repl; mod utils; use std::fmt::{Display, Formatter}; -use std::path::Path; use std::sync::Arc; use crate::utils::touch_file_or_quit; @@ -628,10 +627,7 @@ pub fn cert_generate_core(config: &Configuration) { // Get the cert root let (tls_key_path, tls_chain_path) = match &config.tls_config { - Some(tls_config) => ( - Path::new(tls_config.key.as_str()), - Path::new(tls_config.chain.as_str()), - ), + Some(tls_config) => (tls_config.key.as_path(), tls_config.chain.as_path()), None => { error!("Unable to find tls configuration"); std::process::exit(1); @@ -820,8 +816,8 @@ pub async fn create_server_core( // Setup TLS (if any) let _opt_tls_params = match crypto::setup_tls(&config) { Ok(opt_tls_params) => opt_tls_params, - Err(e) => { - error!("Failed to configure TLS parameters -> {:?}", e); + Err(()) => { + error!("Failed to configure TLS parameters"); return Err(()); } }; @@ -1017,8 +1013,8 @@ pub async fn create_server_core( Some(la) => { let opt_ldap_ssl_acceptor = match crypto::setup_tls(&config) { Ok(t) => t, - Err(e) => { - error!("Failed to configure LDAP TLS parameters -> {:?}", e); + Err(()) => { + error!("Failed to configure LDAP TLS parameters"); return Err(()); } };