From 3002b4de5352a4f55689c91a125e82b93976841a Mon Sep 17 00:00:00 2001 From: Aram Peres Date: Sat, 31 Jul 2021 11:12:45 -0400 Subject: [PATCH] Strict SSL verification (#9) Fixes #8 --- Cargo.lock | 10 +++++ README.md | 8 +++- nut-client/Cargo.toml | 3 +- nut-client/examples/blocking.rs | 18 ++++---- nut-client/src/blocking/mod.rs | 56 ++++++++++++++--------- nut-client/src/config.rs | 80 ++++++++++++++++++++++++++++----- nut-client/src/error.rs | 6 +++ nut-client/src/ssl/mod.rs | 24 +++------- rupsc/README.md | 15 ++++--- rupsc/src/main.rs | 12 ++++- rupsc/src/parser.rs | 14 +++--- 11 files changed, 171 insertions(+), 75 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 931519a..dd50715 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -119,6 +119,7 @@ dependencies = [ "rustls", "shell-words", "webpki", + "webpki-roots", ] [[package]] @@ -328,6 +329,15 @@ dependencies = [ "untrusted", ] +[[package]] +name = "webpki-roots" +version = "0.21.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "aabe153544e473b775453675851ecc86863d2a81d786d741f6b76778f2a48940" +dependencies = [ + "webpki", +] + [[package]] name = "winapi" version = "0.3.9" diff --git a/README.md b/README.md index 279da26..14d420c 100644 --- a/README.md +++ b/README.md @@ -85,5 +85,9 @@ fn main() -> nut_client::Result<()> { You can turn on SSL support by adding `.with_ssl(true)` in the `ConfigBuilder`. This requires the `ssl` feature, which uses `rustls` under the hood. -Note that this crate turns off all certificate validation at the moment, effectively giving a false sense of security. -If you'd like to contribute to this, see issue #8. +Note that, by default, `.with_ssl(true)` will enable **strict** verification. This means it will verify the server +certificate's DNS entries, check for revocation, and verify the chain using the local root trust. You must also ensure +that the connection hostname is a valid DNS name (e.g. `localhost`, not `127.0.0.1`). + +If the server is using a self-signed certificate, and you'd like to ignore the strict validation, you can add +`.with_insecure_ssl(true)` along with `.with_ssl(true)`. diff --git a/nut-client/Cargo.toml b/nut-client/Cargo.toml index 9fe7eed..c38420b 100644 --- a/nut-client/Cargo.toml +++ b/nut-client/Cargo.toml @@ -17,6 +17,7 @@ license = "MIT" shell-words = "1.0.0" rustls = { version = "0.19", optional = true, features = ["dangerous_configuration"] } webpki = { version = "0.21", optional = true } +webpki-roots = { version = "0.21", optional = true } [features] -ssl = ["rustls", "webpki"] +ssl = ["rustls", "webpki", "webpki-roots"] diff --git a/nut-client/examples/blocking.rs b/nut-client/examples/blocking.rs index d7d415d..4a48b58 100644 --- a/nut-client/examples/blocking.rs +++ b/nut-client/examples/blocking.rs @@ -1,23 +1,23 @@ use std::env; -use std::net::ToSocketAddrs; use nut_client::blocking::Connection; -use nut_client::{Auth, ConfigBuilder, Host}; +use nut_client::{Auth, ConfigBuilder}; +use std::convert::TryInto; fn main() -> nut_client::Result<()> { - let addr = env::var("NUT_ADDR") - .unwrap_or_else(|_| "localhost:3493".into()) - .to_socket_addrs() - .unwrap() - .next() - .unwrap(); + let host = env::var("NUT_HOST").unwrap_or_else(|_| "localhost".into()); + let port = env::var("NUT_PORT") + .ok() + .map(|s| s.parse::().ok()) + .flatten() + .unwrap_or(3493); let username = env::var("NUT_USER").ok(); let password = env::var("NUT_PASSWORD").ok(); let auth = username.map(|username| Auth::new(username, password)); let config = ConfigBuilder::new() - .with_host(Host::Tcp(addr)) + .with_host((host, port).try_into().unwrap_or_default()) .with_auth(auth) .with_debug(false) // Turn this on for debugging network chatter .build(); diff --git a/nut-client/src/blocking/mod.rs b/nut-client/src/blocking/mod.rs index a1189bf..4081177 100644 --- a/nut-client/src/blocking/mod.rs +++ b/nut-client/src/blocking/mod.rs @@ -17,9 +17,7 @@ impl Connection { /// Initializes a connection to a NUT server (upsd). pub fn new(config: &Config) -> crate::Result { match &config.host { - Host::Tcp(socket_addr) => { - Ok(Self::Tcp(TcpConnection::new(config.clone(), socket_addr)?)) - } + Host::Tcp(host) => Ok(Self::Tcp(TcpConnection::new(config.clone(), &host.addr)?)), } } @@ -62,7 +60,7 @@ impl Connection { /// A blocking TCP NUT client connection. pub struct TcpConnection { config: Config, - pipeline: ConnectionStream, + stream: ConnectionStream, } impl TcpConnection { @@ -71,7 +69,7 @@ impl TcpConnection { let tcp_stream = TcpStream::connect_timeout(socket_addr, config.timeout)?; let mut connection = Self { config, - pipeline: ConnectionStream::Plain(tcp_stream), + stream: ConnectionStream::Plain(tcp_stream), }; // Initialize SSL connection @@ -98,19 +96,37 @@ impl TcpConnection { })? .expect_ok()?; - let mut config = rustls::ClientConfig::new(); - config - .dangerous() - .set_certificate_verifier(std::sync::Arc::new( - crate::ssl::NutCertificateValidator::new(&self.config), - )); + let mut ssl_config = rustls::ClientConfig::new(); + let sess = if self.config.ssl_insecure { + ssl_config + .dangerous() + .set_certificate_verifier(std::sync::Arc::new( + crate::ssl::InsecureCertificateValidator::new(&self.config), + )); - // todo: this DNS name is temporary; should get from connection hostname? (#8) - let dns_name = webpki::DNSNameRef::try_from_ascii_str("www.google.com").unwrap(); - let sess = rustls::ClientSession::new(&std::sync::Arc::new(config), dns_name); + let dns_name = webpki::DNSNameRef::try_from_ascii_str("www.google.com").unwrap(); + + rustls::ClientSession::new(&std::sync::Arc::new(ssl_config), dns_name) + } else { + // Try to get hostname as given (e.g. localhost can be used for strict SSL, but not 127.0.0.1) + let hostname = self + .config + .host + .hostname() + .ok_or(ClientError::Nut(NutError::SslInvalidHostname))?; + + let dns_name = webpki::DNSNameRef::try_from_ascii_str(&hostname) + .map_err(|_| ClientError::Nut(NutError::SslInvalidHostname))?; + + ssl_config + .root_store + .add_server_trust_anchors(&webpki_roots::TLS_SERVER_ROOTS); + + rustls::ClientSession::new(&std::sync::Arc::new(ssl_config), dns_name) + }; // Wrap and override the TCP stream - self.pipeline = self.pipeline.upgrade_ssl(sess)?; + self.stream = self.stream.upgrade_ssl(sess)?; // Send a test command self.get_network_version()?; @@ -179,8 +195,8 @@ impl TcpConnection { if self.config.debug { eprint!("DEBUG -> {}", line); } - self.pipeline.write_all(line.as_bytes())?; - self.pipeline.flush()?; + self.stream.write_all(line.as_bytes())?; + self.stream.flush()?; Ok(()) } @@ -203,19 +219,19 @@ impl TcpConnection { } fn read_response(&mut self) -> crate::Result { - let mut reader = BufReader::new(&mut self.pipeline); + let mut reader = BufReader::new(&mut self.stream); let args = Self::parse_line(&mut reader, self.config.debug)?; Response::from_args(args) } fn read_plain_response(&mut self) -> crate::Result { - let mut reader = BufReader::new(&mut self.pipeline); + let mut reader = BufReader::new(&mut self.stream); let args = Self::parse_line(&mut reader, self.config.debug)?; Ok(args.join(" ")) } fn read_list(&mut self, query: &[&str]) -> crate::Result> { - let mut reader = BufReader::new(&mut self.pipeline); + let mut reader = BufReader::new(&mut self.stream); let args = Self::parse_line(&mut reader, self.config.debug)?; Response::from_args(args)?.expect_begin_list(query)?; diff --git a/nut-client/src/config.rs b/nut-client/src/config.rs index 5fe9fc8..689c9f1 100644 --- a/nut-client/src/config.rs +++ b/nut-client/src/config.rs @@ -1,29 +1,66 @@ use core::fmt; +use std::convert::{TryFrom, TryInto}; use std::net::{SocketAddr, ToSocketAddrs}; use std::time::Duration; +use crate::ClientError; + /// A host specification. #[derive(Clone, Debug)] pub enum Host { - /// A TCP hostname and port. - Tcp(SocketAddr), + /// A TCP hostname, and address (IP + port). + Tcp(TcpHost), // TODO: Support Unix socket streams. } +impl Host { + /// Returns the hostname as given, if any. + pub fn hostname(&self) -> Option { + match self { + Host::Tcp(host) => Some(host.hostname.to_owned()), + // _ => None, + } + } +} + impl Default for Host { fn default() -> Self { - let addr = (String::from("127.0.0.1"), 3493) - .to_socket_addrs() - .expect("Failed to create local UPS socket address. This is a bug.") - .next() - .expect("Failed to create local UPS socket address. This is a bug."); - Self::Tcp(addr) + (String::from("localhost"), 3493) + .try_into() + .expect("Failed to parse local hostname; this is a bug.") } } impl From for Host { fn from(addr: SocketAddr) -> Self { - Self::Tcp(addr) + let hostname = addr.ip().to_string(); + Self::Tcp(TcpHost { hostname, addr }) + } +} + +/// A TCP address, preserving the original DNS hostname if any. +#[derive(Clone, Debug)] +pub struct TcpHost { + pub(crate) hostname: String, + pub(crate) addr: SocketAddr, +} + +impl TryFrom<(String, u16)> for Host { + type Error = ClientError; + + fn try_from(hostname_port: (String, u16)) -> Result { + let (hostname, _) = hostname_port.clone(); + let addr = hostname_port + .to_socket_addrs() + .map_err(ClientError::Io)? + .next() + .ok_or_else(|| { + ClientError::Io(std::io::Error::new( + std::io::ErrorKind::AddrNotAvailable, + "no address given", + )) + })?; + Ok(Host::Tcp(TcpHost { hostname, addr })) } } @@ -59,17 +96,26 @@ pub struct Config { pub(crate) auth: Option, pub(crate) timeout: Duration, pub(crate) ssl: bool, + pub(crate) ssl_insecure: bool, pub(crate) debug: bool, } impl Config { /// Creates a connection configuration. - pub fn new(host: Host, auth: Option, timeout: Duration, ssl: bool, debug: bool) -> Self { + pub fn new( + host: Host, + auth: Option, + timeout: Duration, + ssl: bool, + ssl_insecure: bool, + debug: bool, + ) -> Self { Config { host, auth, timeout, ssl, + ssl_insecure, debug, } } @@ -82,6 +128,7 @@ pub struct ConfigBuilder { auth: Option, timeout: Option, ssl: Option, + ssl_insecure: Option, debug: Option, } @@ -111,12 +158,24 @@ impl ConfigBuilder { } /// Enables SSL on the connection. + /// + /// This will enable strict SSL verification (including hostname), + /// unless `.with_insecure_ssl` is also set to `true`. #[cfg(feature = "ssl")] pub fn with_ssl(mut self, ssl: bool) -> Self { self.ssl = Some(ssl); self } + /// Turns off SSL verification. + /// + /// Note: you must still use `.with_ssl(true)` to turn on SSL. + #[cfg(feature = "ssl")] + pub fn with_insecure_ssl(mut self, ssl_insecure: bool) -> Self { + self.ssl_insecure = Some(ssl_insecure); + self + } + /// Enables debugging network calls by printing to stderr. pub fn with_debug(mut self, debug: bool) -> Self { self.debug = Some(debug); @@ -130,6 +189,7 @@ impl ConfigBuilder { self.auth, self.timeout.unwrap_or_else(|| Duration::from_secs(5)), self.ssl.unwrap_or(false), + self.ssl_insecure.unwrap_or(false), self.debug.unwrap_or(false), ) } diff --git a/nut-client/src/error.rs b/nut-client/src/error.rs index 2939c71..91c5fa1 100644 --- a/nut-client/src/error.rs +++ b/nut-client/src/error.rs @@ -15,6 +15,8 @@ pub enum NutError { /// Occurs when attempting to use SSL in a transport that doesn't support it, or /// if the server is not configured for it. SslNotSupported, + /// Occurs when trying to initialize a strict SSL connection with an invalid hostname. + SslInvalidHostname, /// Occurs when the client used a feature that is disabled by the server. FeatureNotConfigured, /// Generic (usually internal) client error. @@ -29,6 +31,10 @@ impl fmt::Display for NutError { Self::UnexpectedResponse => write!(f, "Unexpected server response content"), Self::UnknownResponseType(ty) => write!(f, "Unknown response type: {}", ty), Self::SslNotSupported => write!(f, "SSL not supported by server or transport"), + Self::SslInvalidHostname => write!( + f, + "Given hostname cannot be used for a strict SSL connection" + ), Self::FeatureNotConfigured => write!(f, "Feature not configured by server"), Self::Generic(msg) => write!(f, "Internal client error: {}", msg), } diff --git a/nut-client/src/ssl/mod.rs b/nut-client/src/ssl/mod.rs index ed243dd..c33c956 100644 --- a/nut-client/src/ssl/mod.rs +++ b/nut-client/src/ssl/mod.rs @@ -1,40 +1,30 @@ use crate::Config; -/// The certificate validation mechanism for NUT. -pub struct NutCertificateValidator { +/// The certificate validation mechanism that allows any certificate. +pub struct InsecureCertificateValidator { debug: bool, } -impl NutCertificateValidator { +impl InsecureCertificateValidator { /// Initialize a new instance. pub fn new(config: &Config) -> Self { - NutCertificateValidator { + InsecureCertificateValidator { debug: config.debug, } } } -impl rustls::ServerCertVerifier for NutCertificateValidator { +impl rustls::ServerCertVerifier for InsecureCertificateValidator { fn verify_server_cert( &self, _roots: &rustls::RootCertStore, - presented_certs: &[rustls::Certificate], + _presented_certs: &[rustls::Certificate], _dns_name: webpki::DNSNameRef<'_>, _ocsp: &[u8], ) -> Result { - // todo: verify certificates, but not hostnames - if self.debug { - let parsed = webpki::EndEntityCert::from(presented_certs[0].0.as_slice()).ok(); - if let Some(_parsed) = parsed { - eprintln!("DEBUG <- Certificate received and parsed"); - // todo: reading values here... https://github.com/briansmith/webpki/pull/103 - } else { - eprintln!("DEBUG <- Certificate not-parseable"); - } + eprintln!("DEBUG <- (!) Certificate received, but not verified"); } - - // trust everything for now Ok(rustls::ServerCertVerified::assertion()) } } diff --git a/rupsc/README.md b/rupsc/README.md index 8c2d70c..cb6cd4d 100644 --- a/rupsc/README.md +++ b/rupsc/README.md @@ -38,11 +38,11 @@ This is a clone of [`upsc`](https://networkupstools.org/docs/man/upsc.html), so # Show usage rupsc -h -# List variables on UPS device "nutdev1" (assumes upsd running on 127.0.0.1:3493) +# List variables on UPS device "nutdev1" (assumes upsd running on localhost:3493) rupsc nutdev1 -# List variables on UPS device "nutdev1" (remove upsd) -rupsc nutdev1@192.168.1.2:3493 +# List variables on UPS device "nutdev1" (remote upsd) +rupsc nutdev1@upsd.remote:3493 # List available UPS devices rupsc -l @@ -54,14 +54,17 @@ rupsc -L rupsc -c nutdev1 ``` -However, there are also some additions: +However, there are also some additions to the original tool: ```bash -# Enable network debugging (global flag). +# Enable network debugging rupsc -D -# Enable SSL +# Enable SSL (strict verification) rupsc -S + +# Enable SSL (no verification) +rupsc --insecure-ssl ``` ## Pronunciation diff --git a/rupsc/src/main.rs b/rupsc/src/main.rs index 17f5598..58fb2af 100644 --- a/rupsc/src/main.rs +++ b/rupsc/src/main.rs @@ -41,15 +41,23 @@ fn main() -> anyhow::Result<()> { .arg( Arg::with_name("debug") .short("D") + .long("debug") .takes_value(false) .help("Enables debug mode (logs network commands to stderr)."), ) .arg( Arg::with_name("ssl") .short("S") + .long("ssl") .takes_value(false) .help("Enables SSL on the connection with upsd."), ) + .arg( + Arg::with_name("insecure-ssl") + .long("insecure-ssl") + .takes_value(false) + .help("Disables SSL verification on the connection with upsd."), + ) .arg( Arg::with_name("upsd-server") .required(false) @@ -70,13 +78,15 @@ fn main() -> anyhow::Result<()> { )?; let debug = args.is_present("debug"); - let ssl = args.is_present("ssl"); + let insecure_ssl = args.is_present("insecure-ssl"); + let ssl = insecure_ssl || args.is_present("ssl"); let host = server.try_into()?; let config = nut_client::ConfigBuilder::new() .with_host(host) .with_debug(debug) .with_ssl(ssl) + .with_insecure_ssl(insecure_ssl) .build(); if args.is_present("list") { diff --git a/rupsc/src/parser.rs b/rupsc/src/parser.rs index bf5779d..0afb2d4 100644 --- a/rupsc/src/parser.rs +++ b/rupsc/src/parser.rs @@ -1,9 +1,8 @@ use anyhow::Context; use std::convert::{TryFrom, TryInto}; use std::fmt; -use std::net::ToSocketAddrs; -pub const DEFAULT_HOSTNAME: &str = "127.0.0.1"; +pub const DEFAULT_HOSTNAME: &str = "localhost"; pub const DEFAULT_PORT: u16 = 3493; /// Connection information for a upsd server. @@ -69,12 +68,9 @@ impl<'a> TryInto for UpsdName<'a> { type Error = anyhow::Error; fn try_into(self) -> anyhow::Result { - Ok((String::from(self.hostname), self.port) - .to_socket_addrs() - .with_context(|| "Failed to convert to SocketAddr")? - .next() - .with_context(|| "Failed to convert to SocketAddr")? - .into()) + (self.hostname.to_owned(), self.port) + .try_into() + .with_context(|| "Invalid hostname/port") } } @@ -131,7 +127,7 @@ mod tests { port: DEFAULT_PORT } ); - assert_eq!(format!("{}", name), "ups0@127.0.0.1:3493"); + assert_eq!(format!("{}", name), "ups0@localhost:3493"); } #[test]