From beecb54d38e7c3d5eeada0ca99e22a8920ac5862 Mon Sep 17 00:00:00 2001 From: Nadja Reitzenstein Date: Sun, 25 Dec 2022 11:54:36 +0100 Subject: [PATCH] Move miette towards edges of BFFH for more structured error reporting --- bffhd/actors/mod.rs | 35 +++++++++++++++---- bffhd/audit.rs | 9 ++++- bffhd/authentication/mod.rs | 6 ++-- bffhd/capnp/mod.rs | 10 ++++-- bffhd/db/mod.rs | 4 +-- bffhd/lib.rs | 65 +++++++++++++++++++++++++++++----- bffhd/resources/state/db.rs | 2 +- bffhd/tls.rs | 69 +++++++++++++++++++++++++++---------- bffhd/users/db.rs | 2 ++ bffhd/users/mod.rs | 18 ++++++---- 10 files changed, 172 insertions(+), 48 deletions(-) diff --git a/bffhd/actors/mod.rs b/bffhd/actors/mod.rs index d91da4a..e56c0e9 100644 --- a/bffhd/actors/mod.rs +++ b/bffhd/actors/mod.rs @@ -12,9 +12,10 @@ use std::future::Future; use std::pin::Pin; -use miette::IntoDiagnostic; +use miette::{Diagnostic, IntoDiagnostic}; use std::task::{Context, Poll}; use std::time::Duration; +use thiserror::Error; use once_cell::sync::Lazy; use rumqttc::ConnectReturnCode::Success; @@ -111,11 +112,33 @@ static ROOT_CERTS: Lazy = Lazy::new(|| { store }); -pub fn load(executor: Executor, config: &Config, resources: ResourcesHandle) -> miette::Result<()> { +#[derive(Debug, Error, Diagnostic)] +pub enum ActorError { + #[error("failed to parse MQTT url")] + UrlParseError( + #[from] + #[source] + url::ParseError, + ), + #[error("MQTT config is invalid")] + InvalidConfig, + #[error("MQTT connection failed")] + ConnectionError( + #[from] + #[source] + rumqttc::ConnectionError, + ), +} + +pub fn load( + executor: Executor, + config: &Config, + resources: ResourcesHandle, +) -> Result<(), ActorError> { let span = tracing::info_span!("loading actors"); let _guard = span; - let mqtt_url = Url::parse(config.mqtt_url.as_str()).into_diagnostic()?; + let mqtt_url = Url::parse(config.mqtt_url.as_str())?; let (transport, default_port) = match mqtt_url.scheme() { "mqtts" | "ssl" => ( rumqttc::Transport::tls_with_config( @@ -132,12 +155,12 @@ pub fn load(executor: Executor, config: &Config, resources: ResourcesHandle) -> scheme => { tracing::error!(%scheme, "MQTT url uses invalid scheme"); - miette::bail!("invalid config"); + return Err(ActorError::InvalidConfig); } }; let host = mqtt_url.host_str().ok_or_else(|| { tracing::error!("MQTT url must contain a hostname"); - miette::miette!("invalid config") + ActorError::InvalidConfig })?; let port = mqtt_url.port().unwrap_or(default_port); @@ -168,7 +191,7 @@ pub fn load(executor: Executor, config: &Config, resources: ResourcesHandle) -> } Err(error) => { tracing::error!(?error, "MQTT connection failed"); - miette::bail!("mqtt connection failed") + return Err(ActorError::ConnectionError(error)); } } diff --git a/bffhd/audit.rs b/bffhd/audit.rs index 0bf135c..d736156 100644 --- a/bffhd/audit.rs +++ b/bffhd/audit.rs @@ -1,8 +1,10 @@ +use miette::Diagnostic; use once_cell::sync::OnceCell; use std::fs::{File, OpenOptions}; use std::io; use std::io::{LineWriter, Write}; use std::sync::Mutex; +use thiserror::Error; use crate::Config; use serde::{Deserialize, Serialize}; @@ -23,8 +25,13 @@ pub struct AuditLogLine<'a> { state: &'a str, } +#[derive(Debug, Error, Diagnostic)] +#[error(transparent)] +#[repr(transparent)] +pub struct Error(#[from] pub io::Error); + impl AuditLog { - pub fn new(config: &Config) -> io::Result<&'static Self> { + pub fn new(config: &Config) -> Result<&'static Self, Error> { AUDIT.get_or_try_init(|| { tracing::debug!(path = %config.auditlog_path.display(), "Initializing audit log"); let fd = OpenOptions::new() diff --git a/bffhd/authentication/mod.rs b/bffhd/authentication/mod.rs index 6ec5c8d..6fa514c 100644 --- a/bffhd/authentication/mod.rs +++ b/bffhd/authentication/mod.rs @@ -66,9 +66,9 @@ impl SessionCallback for Callback { .get_ref::() .ok_or(ValidationError::MissingRequiredProperty)?; - // if authzid.is_some() { - // return Ok(()) - // } + if authzid.is_some() { + return Ok(()); + } if let Some(user) = self.users.get_user(authcid) { match user.check_password(password) { diff --git a/bffhd/capnp/mod.rs b/bffhd/capnp/mod.rs index 70345f7..523e921 100644 --- a/bffhd/capnp/mod.rs +++ b/bffhd/capnp/mod.rs @@ -1,5 +1,7 @@ -use async_net::TcpListener; +use miette::Diagnostic; +use thiserror::Error; +use async_net::TcpListener; use capnp_rpc::rpc_twoparty_capnp::Side; use capnp_rpc::twoparty::VatNetwork; use capnp_rpc::RpcSystem; @@ -37,6 +39,10 @@ pub struct APIServer { authentication: AuthenticationHandle, } +#[derive(Debug, Error, Diagnostic)] +#[error("Reached Void error, this should not be possible")] +pub enum Error {} + impl APIServer { pub fn new( executor: Executor<'static>, @@ -60,7 +66,7 @@ impl APIServer { acceptor: TlsAcceptor, sessionmanager: SessionManager, authentication: AuthenticationHandle, - ) -> miette::Result { + ) -> Result { let span = tracing::info_span!("binding API listen sockets"); let _guard = span.enter(); diff --git a/bffhd/db/mod.rs b/bffhd/db/mod.rs index 36a5690..0ceb684 100644 --- a/bffhd/db/mod.rs +++ b/bffhd/db/mod.rs @@ -13,9 +13,9 @@ pub type ErrorO = lmdb::Error; pub type Result = std::result::Result; -#[repr(transparent)] -#[derive(Debug, Error)] +#[derive(Clone, Debug, PartialEq, Eq, Error)] #[error(transparent)] +#[repr(transparent)] pub struct Error(#[from] lmdb::Error); impl Diagnostic for Error { diff --git a/bffhd/lib.rs b/bffhd/lib.rs index 81dd9ea..77451c1 100644 --- a/bffhd/lib.rs +++ b/bffhd/lib.rs @@ -8,6 +8,10 @@ //! This is the capnp component of the FabAccess project. //! The entry point of bffhd can be found in [bin/bffhd/main.rs](../bin/bffhd/main.rs) +use miette::Diagnostic; +use std::io; +use thiserror::Error; + pub mod config; /// Internal Databases build on top of LMDB, a mmap()'ed B-tree DB optimized for reads @@ -82,10 +86,58 @@ impl error::Description for SignalHandlerErr { const CODE: &'static str = "signals::new"; } +#[derive(Debug, Error, Diagnostic)] +pub enum BFFHError { + #[error("DB operation failed")] + DBError( + #[from] + #[source] + db::Error, + ), + #[error("failed to initialize global user store")] + UsersError( + #[from] + #[source] + users::Error, + ), + #[error("failed to initialize state database")] + StateDBError( + #[from] + #[source] + resources::state::db::StateDBError, + ), + #[error("audit log failed")] + AuditLogError( + #[from] + #[source] + audit::Error, + ), + #[error("Failed to initialize signal handler")] + SignalsError(#[source] std::io::Error), + #[error("error in actor subsystem")] + ActorError( + #[from] + #[source] + actors::ActorError, + ), + #[error("failed to initialize TLS config")] + TlsSetup( + #[from] + #[source] + tls::Error, + ), + #[error("API handler failed")] + ApiError( + #[from] + #[source] + capnp::Error, + ), +} + impl Diflouroborane { pub fn setup() {} - pub fn new(config: Config) -> miette::Result { + pub fn new(config: Config) -> Result { let mut server = logging::init(&config.logging); let span = tracing::info_span!( target: "bffh", @@ -121,9 +173,7 @@ impl Diflouroborane { let users = Users::new(env.clone())?; let roles = Roles::new(config.roles.clone()); - let _audit_log = AuditLog::new(&config) - .into_diagnostic() - .wrap_err("Failed to initialize audit log")?; + let _audit_log = AuditLog::new(&config)?; let resources = ResourcesHandle::new(config.machines.iter().map(|(id, desc)| { Resource::new(Arc::new(resources::Inner::new( @@ -145,10 +195,10 @@ impl Diflouroborane { }) } - pub fn run(&mut self) -> miette::Result<()> { + pub fn run(&mut self) -> Result<(), BFFHError> { let _guard = self.span.enter(); let mut signals = signal_hook_async_std::Signals::new(&[SIGINT, SIGQUIT, SIGTERM]) - .map_err(|ioerr| error::wrap::(ioerr.into()))?; + .map_err(BFFHError::SignalsError)?; let sessionmanager = SessionManager::new(self.users.clone(), self.roles.clone()); let authentication = AuthenticationHandle::new(self.users.clone()); @@ -162,8 +212,7 @@ impl Diflouroborane { ); actors::load(self.executor.clone(), &self.config, self.resources.clone())?; - let tlsconfig = TlsConfig::new(self.config.tlskeylog.as_ref(), !self.config.is_quiet()) - .into_diagnostic()?; + let tlsconfig = TlsConfig::new(self.config.tlskeylog.as_ref(), !self.config.is_quiet())?; let acceptor = tlsconfig.make_tls_acceptor(&self.config.tlsconfig)?; let apiserver = self.executor.run(APIServer::bind( diff --git a/bffhd/resources/state/db.rs b/bffhd/resources/state/db.rs index 3abc0df..8c2854f 100644 --- a/bffhd/resources/state/db.rs +++ b/bffhd/resources/state/db.rs @@ -17,7 +17,7 @@ pub struct StateDB { db: DB>, } -#[derive(Debug, Error, Diagnostic)] +#[derive(Clone, Debug, PartialEq, Eq, Error, Diagnostic)] pub enum StateDBError { #[error("opening the state db environment failed")] #[diagnostic( diff --git a/bffhd/tls.rs b/bffhd/tls.rs index bc327cf..1589406 100644 --- a/bffhd/tls.rs +++ b/bffhd/tls.rs @@ -1,17 +1,19 @@ use std::fs::File; use std::io; use std::io::BufReader; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::sync::Arc; use crate::capnp::TlsListen; use futures_rustls::TlsAcceptor; -use miette::IntoDiagnostic; +use miette::Diagnostic; use rustls::version::{TLS12, TLS13}; use rustls::{Certificate, PrivateKey, ServerConfig, SupportedCipherSuite}; +use thiserror::Error; use tracing::Level; use crate::keylog::KeyLogFile; +use crate::tls::Error::KeyLogOpen; fn lookup_cipher_suite(name: &str) -> Option { match name { @@ -47,8 +49,32 @@ pub struct TlsConfig { keylog: Option>, } +#[derive(Debug, Error, Diagnostic)] +pub enum Error { + #[error("failed to open certificate file at path {0}")] + OpenCertFile(PathBuf, #[source] io::Error), + #[error("failed to open private key file at path {0}")] + OpenKeyFile(PathBuf, #[source] io::Error), + #[error("failed to read system certs")] + SystemCertsFile(#[source] io::Error), + #[error("failed to read from key file")] + ReadKeyFile(#[source] io::Error), + #[error("private key file must contain a single PEM-encoded private key")] + KeyFileFormat, + #[error("invalid TLS version {0}")] + TlsVersion(String), + #[error("Initializing TLS context failed")] + Builder( + #[from] + #[source] + rustls::Error, + ), + #[error("failed to initialize key log")] + KeyLogOpen(#[source] io::Error), +} + impl TlsConfig { - pub fn new(keylogfile: Option>, warn: bool) -> io::Result { + pub fn new(keylogfile: Option>, warn: bool) -> Result { let span = tracing::span!(Level::INFO, "tls"); let _guard = span.enter(); @@ -57,7 +83,11 @@ impl TlsConfig { } if let Some(path) = keylogfile { - let keylog = Some(KeyLogFile::new(path).map(|ok| Arc::new(ok))?); + let keylog = Some( + KeyLogFile::new(path) + .map(|ok| Arc::new(ok)) + .map_err(KeyLogOpen)?, + ); Ok(Self { keylog }) } else { Ok(Self { keylog: None }) @@ -75,27 +105,31 @@ impl TlsConfig { } } - pub fn make_tls_acceptor(&self, config: &TlsListen) -> miette::Result { + pub fn make_tls_acceptor(&self, config: &TlsListen) -> Result { let span = tracing::debug_span!("tls"); let _guard = span.enter(); - tracing::debug!(path = %config.certfile.as_path().display(), "reading certificates"); - let mut certfp = BufReader::new(File::open(config.certfile.as_path()).into_diagnostic()?); + let path = config.certfile.as_path(); + tracing::debug!(path = %path.display(), "reading certificates"); + let mut certfp = + BufReader::new(File::open(path).map_err(|e| Error::OpenCertFile(path.into(), e))?); let certs = rustls_pemfile::certs(&mut certfp) - .into_diagnostic()? + .map_err(Error::SystemCertsFile)? .into_iter() .map(Certificate) .collect(); - tracing::debug!(path = %config.keyfile.as_path().display(), "reading private key"); - let mut keyfp = BufReader::new(File::open(config.keyfile.as_path()).into_diagnostic()?); - let key = match rustls_pemfile::read_one(&mut keyfp).into_diagnostic()? { + let path = config.keyfile.as_path(); + tracing::debug!(path = %path.display(), "reading private key"); + let mut keyfp = + BufReader::new(File::open(path).map_err(|err| Error::OpenKeyFile(path.into(), err))?); + let key = match rustls_pemfile::read_one(&mut keyfp).map_err(Error::ReadKeyFile)? { Some(rustls_pemfile::Item::PKCS8Key(key) | rustls_pemfile::Item::RSAKey(key)) => { PrivateKey(key) } _ => { tracing::error!("private key file invalid"); - miette::bail!("private key file must contain a PEM-encoded private key") + return Err(Error::KeyFileFormat); } }; @@ -104,20 +138,19 @@ impl TlsConfig { .with_safe_default_kx_groups(); let tls_builder = if let Some(ref min) = config.tls_min_version { - match min.as_str() { + let v = min.to_lowercase(); + match v.as_str() { "tls12" => tls_builder.with_protocol_versions(&[&TLS12]), "tls13" => tls_builder.with_protocol_versions(&[&TLS13]), - x => miette::bail!("TLS version {} is invalid", x), + _ => return Err(Error::TlsVersion(v)), } } else { tls_builder.with_safe_default_protocol_versions() - } - .into_diagnostic()?; + }?; let mut tls_config = tls_builder .with_no_client_auth() - .with_single_cert(certs, key) - .into_diagnostic()?; + .with_single_cert(certs, key)?; if let Some(keylog) = &self.keylog { tls_config.key_log = keylog.clone(); diff --git a/bffhd/users/db.rs b/bffhd/users/db.rs index 761fbc5..c87449b 100644 --- a/bffhd/users/db.rs +++ b/bffhd/users/db.rs @@ -11,6 +11,8 @@ use rkyv::ser::serializers::AllocSerializer; use rkyv::ser::Serializer; use rkyv::Deserialize; +pub use crate::db::Error; + #[derive( Clone, PartialEq, diff --git a/bffhd/users/mod.rs b/bffhd/users/mod.rs index 01360e2..bd7a7f9 100644 --- a/bffhd/users/mod.rs +++ b/bffhd/users/mod.rs @@ -11,6 +11,7 @@ use clap::ArgMatches; use miette::{Context, Diagnostic, IntoDiagnostic, SourceOffset, SourceSpan}; use std::path::Path; use std::sync::Arc; + use thiserror::Error; pub mod db; @@ -69,17 +70,20 @@ pub struct Users { userdb: &'static UserDB, } +#[derive(Clone, Debug, PartialEq, Eq, Error, Diagnostic)] +#[error(transparent)] +#[repr(transparent)] +pub struct Error(#[from] pub db::Error); + impl Users { - pub fn new(env: Arc) -> miette::Result { + pub fn new(env: Arc) -> Result { let span = tracing::debug_span!("users", ?env, "Creating Users handle"); let _guard = span.enter(); - let userdb = USERDB - .get_or_try_init(|| { - tracing::debug!("Global resource not yet initialized, initializing…"); - unsafe { UserDB::create(env) } - }) - .wrap_err("Failed to open userdb")?; + let userdb = USERDB.get_or_try_init(|| { + tracing::debug!("Global resource not yet initialized, initializing…"); + unsafe { UserDB::create(env) } + })?; Ok(Self { userdb }) }