From 7de01a3d556a8745b7c6a19fde8b2c52fee42800 Mon Sep 17 00:00:00 2001 From: Devdutt Shenoi Date: Wed, 22 Nov 2023 00:07:20 +0530 Subject: [PATCH 1/8] refactor: feature flag `verify-client-cert` --- rumqttd/Cargo.toml | 3 ++- rumqttd/src/lib.rs | 13 ++++++++++--- rumqttd/src/server/tls.rs | 32 ++++++++++++++++++++------------ 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/rumqttd/Cargo.toml b/rumqttd/Cargo.toml index 54907d29..18c446e3 100644 --- a/rumqttd/Cargo.toml +++ b/rumqttd/Cargo.toml @@ -43,7 +43,8 @@ default = ["use-rustls", "websocket"] use-rustls = ["dep:tokio-rustls", "dep:rustls-webpki", "dep:rustls-pemfile", "dep:x509-parser"] use-native-tls = ["dep:tokio-native-tls", "dep:x509-parser"] websocket = ["dep:async-tungstenite", "dep:tokio-util", "dep:futures-util", "dep:ws_stream_tungstenite"] -validate-tenant-prefix = [] +verify-client-cert = [] +validate-tenant-prefix = ["verify-client-cert"] allow-duplicate-clientid = [] [dev-dependencies] diff --git a/rumqttd/src/lib.rs b/rumqttd/src/lib.rs index b2026047..5fc8b51b 100644 --- a/rumqttd/src/lib.rs +++ b/rumqttd/src/lib.rs @@ -67,6 +67,7 @@ pub struct PrometheusSetting { #[serde(untagged)] pub enum TlsConfig { Rustls { + #[cfg(feature = "verify-client-cert")] capath: String, certpath: String, keypath: String, @@ -83,12 +84,18 @@ impl TlsConfig { pub fn validate_paths(&self) -> bool { match self { TlsConfig::Rustls { + #[cfg(feature = "verify-client-cert")] capath, certpath, keypath, - } => [capath, certpath, keypath] - .iter() - .all(|v| Path::new(v).exists()), + } => [ + #[cfg(feature = "verify-client-cert")] + capath, + certpath, + keypath, + ] + .iter() + .all(|v| Path::new(v).exists()), TlsConfig::NativeTls { pkcs12path, .. } => Path::new(pkcs12path).exists(), } } diff --git a/rumqttd/src/server/tls.rs b/rumqttd/src/server/tls.rs index dc557042..da41e4de 100644 --- a/rumqttd/src/server/tls.rs +++ b/rumqttd/src/server/tls.rs @@ -7,19 +7,18 @@ use { tokio_native_tls::native_tls::Error as NativeTlsError, }; +use crate::TlsConfig; +#[cfg(feature = "verify-client-cert")] +use tokio_rustls::rustls::{server::AllowAnyAuthenticatedClient, RootCertStore}; #[cfg(feature = "use-rustls")] use { rustls_pemfile::Item, std::{io::BufReader, sync::Arc}, - tokio_rustls::rustls::{ - server::AllowAnyAuthenticatedClient, Certificate, Error as RustlsError, PrivateKey, - RootCertStore, ServerConfig, - }, + tokio_rustls::rustls::{Certificate, Error as RustlsError, PrivateKey, ServerConfig}, tracing::error, }; use crate::link::network::N; -use crate::TlsConfig; #[derive(Debug, thiserror::Error)] #[error("Acceptor error")] @@ -100,10 +99,16 @@ impl TLSAcceptor { match config { #[cfg(feature = "use-rustls")] TlsConfig::Rustls { + #[cfg(feature = "verify-client-cert")] capath, certpath, keypath, - } => Self::rustls(capath, certpath, keypath), + } => Self::rustls( + #[cfg(feature = "verify-client-cert")] + capath, + certpath, + keypath, + ), #[cfg(feature = "use-native-tls")] TlsConfig::NativeTls { pkcs12path, @@ -172,7 +177,7 @@ impl TLSAcceptor { #[cfg(feature = "use-rustls")] fn rustls( - ca_path: &String, + #[cfg(feature = "verify-client-cert")] ca_path: &String, cert_path: &String, key_path: &String, ) -> Result { @@ -193,8 +198,10 @@ impl TLSAcceptor { (certs, key) }; + let builder = ServerConfig::builder().with_safe_defaults(); // client authentication with a CA. CA isn't required otherwise - let server_config = { + #[cfg(feature = "verify-client-cert")] + let builder = { let ca_file = File::open(ca_path); let ca_file = ca_file.map_err(|_| Error::CaFileNotFound(ca_path.clone()))?; let ca_file = &mut BufReader::new(ca_file); @@ -209,11 +216,12 @@ impl TLSAcceptor { .add(&ca_cert) .map_err(|_| Error::InvalidCACert(ca_path.to_string()))?; - ServerConfig::builder() - .with_safe_defaults() - .with_client_cert_verifier(Arc::new(AllowAnyAuthenticatedClient::new(store))) - .with_single_cert(certs, key)? + builder.with_client_cert_verifier(Arc::new(AllowAnyAuthenticatedClient::new(store))) }; + #[cfg(not(feature = "verify-client-cert"))] + let builder = builder.with_no_client_auth(); + + let server_config = builder.with_single_cert(certs, key)?; let acceptor = tokio_rustls::TlsAcceptor::from(Arc::new(server_config)); Ok(TLSAcceptor::Rustls { acceptor }) From 7e5dac8883c1fc87fbe17dddb55e1619753680f6 Mon Sep 17 00:00:00 2001 From: Devdutt Shenoi Date: Wed, 22 Nov 2023 00:19:09 +0530 Subject: [PATCH 2/8] doc: update CHANGELOG --- rumqttd/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/rumqttd/CHANGELOG.md b/rumqttd/CHANGELOG.md index 6e984369..f1b481aa 100644 --- a/rumqttd/CHANGELOG.md +++ b/rumqttd/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Removed ### Fixed +- Feature flag rustls client auth ### Security - Update tungstenite and dependencies to fix [CVE](https://rustsec.org/advisories/RUSTSEC-2023-0065). From 450ccf97cd1cab01f9513ab38ec221b06dd5bc0d Mon Sep 17 00:00:00 2001 From: Devdutt Shenoi Date: Wed, 22 Nov 2023 14:44:25 +0530 Subject: [PATCH 3/8] fix: warn user of unconfigured tls.capath field --- rumqttd/src/main.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/rumqttd/src/main.rs b/rumqttd/src/main.rs index 8b22805f..d3ce1fa0 100644 --- a/rumqttd/src/main.rs +++ b/rumqttd/src/main.rs @@ -76,7 +76,19 @@ fn main() { )), }; - let mut configs: rumqttd::Config = config_builder.build().unwrap().try_deserialize().unwrap(); + let config = config_builder.build().unwrap(); + #[cfg(feature = "verify-client-cert")] + for protocol in ["v4", "v5", "ws"] { + if let Ok(servers) = config.get_table(protocol) { + for (name, config) in servers { + let server = config.into_table().unwrap(); + if server.contains_key("tls") && !server.contains_key("tls.capth") { + panic!("Missing capth in: \"{protocol}.{name}\"") + } + } + } + } + let mut configs: rumqttd::Config = config.try_deserialize().unwrap(); if let Some(console_config) = configs.console.as_mut() { console_config.set_filter_reload_handle(reload_handle) From 42e532de0ea3b499b40ae9d0401cebadc7b05ae5 Mon Sep 17 00:00:00 2001 From: swanandx <73115739+swanandx@users.noreply.github.com> Date: Wed, 22 Nov 2023 16:29:52 +0530 Subject: [PATCH 4/8] feat: make capath optional --- rumqttd/src/lib.rs | 17 ++++++----------- rumqttd/src/server/tls.rs | 18 ++++++++++++++---- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/rumqttd/src/lib.rs b/rumqttd/src/lib.rs index 5fc8b51b..418579b3 100644 --- a/rumqttd/src/lib.rs +++ b/rumqttd/src/lib.rs @@ -67,8 +67,7 @@ pub struct PrometheusSetting { #[serde(untagged)] pub enum TlsConfig { Rustls { - #[cfg(feature = "verify-client-cert")] - capath: String, + capath: Option, certpath: String, keypath: String, }, @@ -84,18 +83,14 @@ impl TlsConfig { pub fn validate_paths(&self) -> bool { match self { TlsConfig::Rustls { - #[cfg(feature = "verify-client-cert")] capath, certpath, keypath, - } => [ - #[cfg(feature = "verify-client-cert")] - capath, - certpath, - keypath, - ] - .iter() - .all(|v| Path::new(v).exists()), + } => { + let ca = capath.is_none() || capath.as_ref().is_some_and(|v| Path::new(v).exists()); + + ca && [certpath, keypath].iter().all(|v| Path::new(v).exists()) + } TlsConfig::NativeTls { pkcs12path, .. } => Path::new(pkcs12path).exists(), } } diff --git a/rumqttd/src/server/tls.rs b/rumqttd/src/server/tls.rs index da41e4de..465a7960 100644 --- a/rumqttd/src/server/tls.rs +++ b/rumqttd/src/server/tls.rs @@ -99,13 +99,12 @@ impl TLSAcceptor { match config { #[cfg(feature = "use-rustls")] TlsConfig::Rustls { - #[cfg(feature = "verify-client-cert")] - capath, + capath: _capath, certpath, keypath, } => Self::rustls( #[cfg(feature = "verify-client-cert")] - capath, + _capath, certpath, keypath, ), @@ -177,10 +176,19 @@ impl TLSAcceptor { #[cfg(feature = "use-rustls")] fn rustls( - #[cfg(feature = "verify-client-cert")] ca_path: &String, + #[cfg(feature = "verify-client-cert")] ca_path: &Option, cert_path: &String, key_path: &String, ) -> Result { + #[cfg(feature = "verify-client-cert")] + let Some(ca_path) = ca_path + else { + return Err(Error::CaFileNotFound( + "capath must be specified in config when verify-client-cert is enabled." + .to_string(), + )); + }; + let (certs, key) = { // Get certificates let cert_file = File::open(cert_path); @@ -199,6 +207,7 @@ impl TLSAcceptor { }; let builder = ServerConfig::builder().with_safe_defaults(); + // client authentication with a CA. CA isn't required otherwise #[cfg(feature = "verify-client-cert")] let builder = { @@ -218,6 +227,7 @@ impl TLSAcceptor { builder.with_client_cert_verifier(Arc::new(AllowAnyAuthenticatedClient::new(store))) }; + #[cfg(not(feature = "verify-client-cert"))] let builder = builder.with_no_client_auth(); From e7b877617916f437301d8a256760a84c5dbf449f Mon Sep 17 00:00:00 2001 From: swanandx <73115739+swanandx@users.noreply.github.com> Date: Wed, 22 Nov 2023 16:36:38 +0530 Subject: [PATCH 5/8] fix: extract tenant_id only if we have peer certs --- rumqttd/src/server/tls.rs | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/rumqttd/src/server/tls.rs b/rumqttd/src/server/tls.rs index 465a7960..f9d9dea4 100644 --- a/rumqttd/src/server/tls.rs +++ b/rumqttd/src/server/tls.rs @@ -59,7 +59,7 @@ pub enum Error { CertificateParse, } -#[cfg(feature = "use-rustls")] +#[cfg(feature = "verify-client-cert")] /// Extract uid from certificate's subject organization field fn extract_tenant_id(der: &[u8]) -> Result, Error> { let (_, cert) = @@ -125,11 +125,18 @@ impl TLSAcceptor { #[cfg(feature = "use-rustls")] TLSAcceptor::Rustls { acceptor } => { let stream = acceptor.accept(stream).await?; - let (_, session) = stream.get_ref(); - let peer_certificates = session - .peer_certificates() - .ok_or(Error::NoPeerCertificate)?; - let tenant_id = extract_tenant_id(&peer_certificates[0].0)?; + + #[cfg(feature = "verify-client-cert")] + let tenant_id = { + let (_, session) = stream.get_ref(); + let peer_certificates = session + .peer_certificates() + .ok_or(Error::NoPeerCertificate)?; + extract_tenant_id(&peer_certificates[0].0)? + }; + #[cfg(not(feature = "verify-client-cert"))] + let tenant_id: Option = None; + let network = Box::new(stream); Ok((tenant_id, network)) } From 6e4f994ddaec2906c1740940bdb9eb2ec596e4fc Mon Sep 17 00:00:00 2001 From: swanandx <73115739+swanandx@users.noreply.github.com> Date: Wed, 22 Nov 2023 16:42:19 +0530 Subject: [PATCH 6/8] Revert "fix: warn user of unconfigured tls.capath field" This reverts commit 450ccf97cd1cab01f9513ab38ec221b06dd5bc0d. --- rumqttd/src/main.rs | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/rumqttd/src/main.rs b/rumqttd/src/main.rs index d3ce1fa0..8b22805f 100644 --- a/rumqttd/src/main.rs +++ b/rumqttd/src/main.rs @@ -76,19 +76,7 @@ fn main() { )), }; - let config = config_builder.build().unwrap(); - #[cfg(feature = "verify-client-cert")] - for protocol in ["v4", "v5", "ws"] { - if let Ok(servers) = config.get_table(protocol) { - for (name, config) in servers { - let server = config.into_table().unwrap(); - if server.contains_key("tls") && !server.contains_key("tls.capth") { - panic!("Missing capth in: \"{protocol}.{name}\"") - } - } - } - } - let mut configs: rumqttd::Config = config.try_deserialize().unwrap(); + let mut configs: rumqttd::Config = config_builder.build().unwrap().try_deserialize().unwrap(); if let Some(console_config) = configs.console.as_mut() { console_config.set_filter_reload_handle(reload_handle) From 45fb1e26866ed00a58b27d1cee26c69ec36a7ef0 Mon Sep 17 00:00:00 2001 From: swanandx <73115739+swanandx@users.noreply.github.com> Date: Thu, 23 Nov 2023 11:30:12 +0530 Subject: [PATCH 7/8] feat: log warning incase CA cert is specified and getting ignored --- rumqttd/src/server/tls.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/rumqttd/src/server/tls.rs b/rumqttd/src/server/tls.rs index f9d9dea4..d402c844 100644 --- a/rumqttd/src/server/tls.rs +++ b/rumqttd/src/server/tls.rs @@ -99,15 +99,10 @@ impl TLSAcceptor { match config { #[cfg(feature = "use-rustls")] TlsConfig::Rustls { - capath: _capath, + capath, certpath, keypath, - } => Self::rustls( - #[cfg(feature = "verify-client-cert")] - _capath, - certpath, - keypath, - ), + } => Self::rustls(capath, certpath, keypath), #[cfg(feature = "use-native-tls")] TlsConfig::NativeTls { pkcs12path, @@ -183,7 +178,7 @@ impl TLSAcceptor { #[cfg(feature = "use-rustls")] fn rustls( - #[cfg(feature = "verify-client-cert")] ca_path: &Option, + ca_path: &Option, cert_path: &String, key_path: &String, ) -> Result { @@ -196,6 +191,11 @@ impl TLSAcceptor { )); }; + #[cfg(not(feature = "verify-client-cert"))] + if ca_path.is_some() { + tracing::warn!("verify-client-cert feature is disabled, CA cert will be ignored and no client authentication is done."); + } + let (certs, key) = { // Get certificates let cert_file = File::open(cert_path); From 726f961dbd8f8003d64961d0729e6d95bb8f89f3 Mon Sep 17 00:00:00 2001 From: swanandx <73115739+swanandx@users.noreply.github.com> Date: Thu, 23 Nov 2023 11:33:28 +0530 Subject: [PATCH 8/8] chore: update changelog entry --- rumqttd/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rumqttd/CHANGELOG.md b/rumqttd/CHANGELOG.md index f1b481aa..628daeb0 100644 --- a/rumqttd/CHANGELOG.md +++ b/rumqttd/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - v4 config is optional, user can specify v4 and/or v5 config - websocket feature is enabled by default - console configuration is optional +- rustls client auth is featured gated behind "verify-client-cert" ( disabled by default ). ### Deprecated - "websockets" feature is removed in favour of "websocket" @@ -23,7 +24,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Removed ### Fixed -- Feature flag rustls client auth ### Security - Update tungstenite and dependencies to fix [CVE](https://rustsec.org/advisories/RUSTSEC-2023-0065).