From 2c0e9c6a16f9df58799a96755bcaa07287b6c29d Mon Sep 17 00:00:00 2001 From: Hannes de Jager Date: Fri, 2 Sep 2022 14:47:42 +0200 Subject: [PATCH] Support EC private keys We didn't support elliptic curve private keys. See https://github.com/bolcom/unFTP/issues/126 . I took the opportunity to also improve the error handling in the TLS code. --- Cargo.toml | 28 +++++----- src/server/ftpserver/error.rs | 8 ++- src/server/tls.rs | 98 +++++++++++++++++++---------------- 3 files changed, 74 insertions(+), 60 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index aba9e527..8cffdfc6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,32 +30,32 @@ members = [ ] [dependencies] -async-trait = "0.1.56" +async-trait = "0.1.57" bitflags = "1.3.2" -bytes = "1.2.0" -chrono = { version = "0.4.19", default-features = false, features = ["std"] } +bytes = "1.2.1" +chrono = { version = "0.4.22", default-features = false, features = ["std"] } derive_more = { version = "0.99.17", features = ["display"] } -futures-util = { version = "0.3.21", default-features = false, features = ["alloc", "sink"] } +futures-util = { version = "0.3.24", default-features = false, features = ["alloc", "sink"] } getrandom = "0.2.7" lazy_static = "1.4.0" -md-5 = "0.10.1" -moka = "0.9.2" -prometheus = { version = "0.13.1", default-features = false } +md-5 = "0.10.4" +moka = "0.9.4" +prometheus = { version = "0.13.2", default-features = false } proxy-protocol = "0.5.0" rustls = "0.20.6" -rustls-pemfile = "1.0.0" +rustls-pemfile = "1.0.1" slog = { version = "2.7.0", features = ["max_level_trace", "release_max_level_info"] } slog-stdlog = "4.1.1" -thiserror = "1.0.31" -tokio = { version = "1.20.0", features = ["macros", "rt", "net", "sync", "io-util", "time"] } +thiserror = "1.0.35" +tokio = { version = "1.21.1", features = ["macros", "rt", "net", "sync", "io-util", "time"] } tokio-rustls = "0.23.4" -tokio-util = { version = "0.7.3", features = ["codec"] } -tracing = { version = "0.1.35", default-features = false } +tokio-util = { version = "0.7.4", features = ["codec"] } +tracing = { version = "0.1.36", default-features = false } tracing-attributes = "0.1.22" uuid = { version = "1.1.2", features = ["v4"] } x509-parser = "0.14.0" [dev-dependencies] -pretty_assertions = "1.2.1" -tokio = { version = "1.20.0", features = ["macros", "rt-multi-thread"] } +pretty_assertions = "1.3.0" +tokio = { version = "1.21.1", features = ["macros", "rt-multi-thread"] } unftp-sbe-fs = { path = "../libunftp/crates/unftp-sbe-fs"} diff --git a/src/server/ftpserver/error.rs b/src/server/ftpserver/error.rs index f0562476..653230e8 100644 --- a/src/server/ftpserver/error.rs +++ b/src/server/ftpserver/error.rs @@ -23,7 +23,7 @@ impl ServerError { } } -impl From for ServerError { +impl From for ServerError { fn from(e: AddrParseError) -> Self { ServerError::new("could not parse address", e) } @@ -35,6 +35,12 @@ impl From for ServerError { } } +impl From for ServerError { + fn from(e: super::tls::ConfigError) -> Self { + ServerError::new(format!("error with TLS configuration: {}", e), e) + } +} + #[derive(Error, Debug)] #[error("shutdown error: {msg}")] pub struct ShutdownError { diff --git a/src/server/tls.rs b/src/server/tls.rs index 29649a02..be930e5c 100644 --- a/src/server/tls.rs +++ b/src/server/tls.rs @@ -1,20 +1,19 @@ use crate::options::{FtpsClientAuth, TlsFlags}; -use rustls::server::StoresServerSessions; use rustls::{ - server::{AllowAnyAnonymousOrAuthenticatedClient, AllowAnyAuthenticatedClient, NoClientAuth, NoServerSessionStorage}, + server::{AllowAnyAnonymousOrAuthenticatedClient, AllowAnyAuthenticatedClient, NoClientAuth, NoServerSessionStorage, StoresServerSessions}, version::{TLS12, TLS13}, Certificate, NoKeyLog, PrivateKey, RootCertStore, ServerConfig, SupportedProtocolVersion, Ticketer, }; use std::{ - error::Error, - fmt, - fmt::Formatter, + fmt::{self, Display, Formatter}, fs::File, - io::BufReader, + io::{self, BufReader}, path::{Path, PathBuf}, sync::Arc, time::Duration, }; +use thiserror::Error; +use tokio_rustls::webpki; // FTPSConfig shows how TLS security is configured for the server or a particular channel. #[derive(Clone)] @@ -37,13 +36,30 @@ impl fmt::Debug for FtpsConfig { #[derive(Debug, Copy, Clone)] pub struct FtpsNotAvailable; -impl fmt::Display for FtpsNotAvailable { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { +impl Display for FtpsNotAvailable { + fn fmt(&self, f: &mut Formatter) -> fmt::Result { write!(f, "FTPS not configured/available") } } -impl Error for FtpsNotAvailable {} +impl std::error::Error for FtpsNotAvailable {} + +// The error returned by new_config +#[derive(Error, Debug)] +#[error("TLS configuration error")] +pub enum ConfigError { + #[error("found no private key")] + NoPrivateKey, + + #[error("error reading key/cert input")] + Load(#[from] io::Error), + + #[error("error building root certs")] + RootCerts(#[from] webpki::Error), + + #[error("error initialising Rustls")] + RustlsInit(#[from] rustls::Error), +} pub fn new_config>( certs_file: P, @@ -51,7 +67,7 @@ pub fn new_config>( flags: TlsFlags, client_auth: FtpsClientAuth, trust_store: P, -) -> std::io::Result> { +) -> Result, ConfigError> { let certs: Vec = load_certs(certs_file)?; let privkey: PrivateKey = load_private_key(key_file)?; @@ -78,10 +94,10 @@ pub fn new_config>( let mut config = ServerConfig::builder() .with_safe_default_cipher_suites() .with_safe_default_kx_groups() - .with_protocol_versions(&versions).map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))? + .with_protocol_versions(&versions).map_err(ConfigError::RustlsInit)? .with_client_cert_verifier(client_auther) // No SNI, single certificate - .with_single_cert(certs, privkey).map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))?; + .with_single_cert(certs, privkey).map_err(ConfigError::RustlsInit)?; // Support session resumption with server side state (Session IDs) config.session_storage = if flags.contains(TlsFlags::RESUMPTION_SESS_ID) { @@ -91,7 +107,7 @@ pub fn new_config>( }; // Support session resumption with tickets. See https://tools.ietf.org/html/rfc5077 if flags.contains(TlsFlags::RESUMPTION_TICKETS) { - config.ticketer = Ticketer::new().map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))?; + config.ticketer = Ticketer::new().map_err(ConfigError::RustlsInit)?; }; // Don't allow dumping session keys config.key_log = Arc::new(NoKeyLog {}); @@ -99,53 +115,45 @@ pub fn new_config>( Ok(Arc::new(config)) } -fn root_cert_store>(trust_pem: P) -> std::io::Result { +fn root_cert_store>(trust_pem: P) -> Result { let mut store = RootCertStore::empty(); let certs = load_certs(trust_pem)?; for cert in certs.iter() { - store.add(cert).map_err(|_| std::io::Error::from(std::io::ErrorKind::Other))? + store.add(cert).map_err(ConfigError::RootCerts)? } Ok(store) } -fn load_certs>(filename: P) -> std::io::Result> { +fn load_certs>(filename: P) -> Result, ConfigError> { let certfile: File = File::open(filename)?; let mut reader: BufReader = BufReader::new(certfile); - rustls_pemfile::certs(&mut reader) - .map_err(|_| std::io::Error::from(std::io::ErrorKind::Other)) - .map(|v| { - let mut res = Vec::with_capacity(v.len()); - for e in v { - res.push(Certificate(e)); - } - res - }) + rustls_pemfile::certs(&mut reader).map_err(ConfigError::Load).map(|v| { + let mut res = Vec::with_capacity(v.len()); + for e in v { + res.push(Certificate(e)); + } + res + }) } -fn load_private_key>(filename: P) -> std::io::Result { - let rsa_keys = { - let keyfile = File::open(&filename)?; - let mut reader = BufReader::new(keyfile); - rustls_pemfile::rsa_private_keys(&mut reader).map_err(|_| std::io::Error::from(std::io::ErrorKind::Other))? - }; +fn load_private_key>(filename: P) -> Result { + use rustls_pemfile::{read_one, Item}; + use std::iter; - let pkcs8_keys = { - let keyfile = File::open(&filename)?; - let mut reader = BufReader::new(keyfile); - rustls_pemfile::pkcs8_private_keys(&mut reader).map_err(|_| std::io::Error::from(std::io::ErrorKind::Other))? - }; + let keyfile = File::open(&filename)?; + let mut reader = BufReader::new(keyfile); - // prefer to load pkcs8 keys - let key = if !pkcs8_keys.is_empty() { - pkcs8_keys[0].clone() - } else { - if rsa_keys.is_empty() { - return Err(std::io::Error::from(std::io::ErrorKind::Other)); + for item in iter::from_fn(|| read_one(&mut reader).transpose()) { + match item { + Ok(Item::RSAKey(key)) => return Ok(PrivateKey(key)), + Ok(Item::PKCS8Key(key)) => return Ok(PrivateKey(key)), + Ok(Item::ECKey(key)) => return Ok(PrivateKey(key)), + Err(e) => return Err(ConfigError::Load(e)), + _ => {} } - rsa_keys[0].clone() - }; + } - Ok(PrivateKey(key)) + Err(ConfigError::NoPrivateKey) } /// Stores the session IDs server side.