From ef63b7444f150de0031b9dea6f103bce54df1494 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Wed, 18 Aug 2021 17:46:30 +0200 Subject: [PATCH] Add check if configured directories are writable --- CHANGELOG.adoc | 7 +- src/bin/stackable-agent.rs | 135 +++++++++++++++++++++++++++++++++---- src/config/mod.rs | 115 +++++++++++++++++++++---------- src/fsext.rs | 79 +++++++++++++++++++++- 4 files changed, 285 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 4b6d4e91..c01b24b3 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -5,6 +5,12 @@ :262: https://github.com/stackabletech/agent/pull/262[#262] :267: https://github.com/stackabletech/agent/pull/267[#267] :270: https://github.com/stackabletech/agent/pull/270[#270] +:273: https://github.com/stackabletech/agent/pull/273[#273] + +=== Added +* Prints self-diagnostic information on startup ({270}) +* Check added on startup if the configured directories exist and are + writable by the Stackable agent ({273}). === Changed * Lazy validation of repository URLs changed to eager validation @@ -12,7 +18,6 @@ * `certificates.k8s.io/v1` used instead of `certificates.k8s.io/v1beta1` so that the Stackable Agent is now compatible with Kubernetes v1.22 but not any longer with versions prior to v1.19 ({267}). -* Prints self-diagnostic information on startup ({270}) == 0.5.0 - 2021-07-26 diff --git a/src/bin/stackable-agent.rs b/src/bin/stackable-agent.rs index 560aed0e..d0d4d0b9 100644 --- a/src/bin/stackable-agent.rs +++ b/src/bin/stackable-agent.rs @@ -1,13 +1,18 @@ +use std::collections::HashMap; use std::env; use std::ffi::OsString; +use std::io::ErrorKind; +use std::path::PathBuf; use kubelet::config::{Config, ServerConfig}; use kubelet::Kubelet; -use log::{info, warn}; -use stackable_config::ConfigBuilder; +use log::{error, info}; +use tokio::fs::File; use stackable_agent::config::AgentConfig; +use stackable_agent::fsext::check_dir_is_writable; use stackable_agent::provider::StackableProvider; +use stackable_config::{ConfigBuilder, ConfigOption}; mod built_info { // The file has been placed there by the build script. @@ -50,6 +55,9 @@ async fn main() -> anyhow::Result<()> { built_info::RUSTC_VERSION, ); + check_optional_files(&agent_config).await; + check_configured_directories(&agent_config).await; + // Currently the only way to _properly_ configure the Krustlet is via these environment exports, // as their config object only offers methods that parse from command line flags (or combinations // of those flags with other things). @@ -73,24 +81,23 @@ async fn main() -> anyhow::Result<()> { export_env("NODE_LABELS", &node_labels); - if let Some(cert_file_path) = &agent_config.server_cert_file { - export_env("KRUSTLET_CERT_FILE", cert_file_path.to_str().unwrap()); - } else { - warn!("Not exporting server cert file path, as non was specified that could be converted to a String."); - } + export_env( + "KRUSTLET_CERT_FILE", + agent_config.server_cert_file.to_str().unwrap(), + ); + + export_env( + "KRUSTLET_PRIVATE_KEY_FILE", + agent_config.server_key_file.to_str().unwrap(), + ); - if let Some(key_file_path) = &agent_config.server_key_file { - export_env("KRUSTLET_PRIVATE_KEY_FILE", key_file_path.to_str().unwrap()); - } else { - warn!("Not exporting server key file path, as non was specified that could be converted to a String."); - } info!("args: {:?}", env::args()); let server_config = ServerConfig { addr: agent_config.server_ip_address, port: agent_config.server_port, - cert_file: agent_config.server_cert_file.to_owned().unwrap_or_default(), - private_key_file: agent_config.server_key_file.to_owned().unwrap_or_default(), + cert_file: agent_config.server_cert_file.clone(), + private_key_file: agent_config.server_key_file.clone(), }; let plugins_directory = agent_config.data_directory.join("plugins"); @@ -139,3 +146,103 @@ fn export_env(var_name: &str, var_value: &str) { fn notify_bootstrap(message: String) { info!("Successfully bootstrapped TLS certificate: {}", message); } + +/// Checks if the optional files can be opened if they exist. An error +/// is logged if they cannot be opened. +async fn check_optional_files(config: &AgentConfig) { + for (config_option, file) in [ + (AgentConfig::SERVER_CERT_FILE, &config.server_cert_file), + (AgentConfig::SERVER_KEY_FILE, &config.server_key_file), + ] { + if file.is_file() { + if let Err(error) = File::open(file).await { + error!( + "Could not open file [{}] which is specified in \ + the configuration option [{}]. {}", + file.to_string_lossy(), + config_option.name, + error + ); + } + } + } +} + +/// Checks the configured directories if they are writable by the +/// current process. If this is not the case then errors are logged. +/// +/// This check is performed for informational purposes only. The process +/// is intentionally not terminated on failure because there can be +/// false positives, e.g. if the underlying file system does not support +/// temporary files which are used for the check. +/// +/// A successful check also does not guarantee that the process can +/// write to the directory at a later time, e.g. if permissions are +/// changed or a quota is hit. +async fn check_configured_directories(config: &AgentConfig) { + for (config_option, directory) in directories_where_write_access_is_required(config).await { + let directory = if directory.components().count() == 0 { + PathBuf::from(".") + } else { + directory + }; + + if let Err(error) = check_dir_is_writable(&directory).await { + match error.kind() { + ErrorKind::NotFound => error!( + "The directory [{}] specified in the configuration \ + option [{}] does not exist.", + directory.to_string_lossy(), + config_option.name + ), + ErrorKind::PermissionDenied => error!( + "The directory [{}] specified in the configuration \ + option [{}] is not writable by the process.", + directory.to_string_lossy(), + config_option.name + ), + _ => error!( + "An IO error occurred while checking the directory \ + [{}] specified in the configuration option [{}]. \ + {}", + directory.to_string_lossy(), + config_option.name, + error + ), + }; + } + } +} + +/// Returns all directories configured in the given `AgentConfig` where +/// write access is required. +/// +/// The directories of the certificate and key files are only returned +/// if they do not already exist. +async fn directories_where_write_access_is_required( + config: &AgentConfig, +) -> HashMap<&ConfigOption, PathBuf> { + let mut dirs = HashMap::new(); + dirs.insert( + &AgentConfig::PACKAGE_DIR, + config.parcel_directory.to_owned(), + ); + dirs.insert(&AgentConfig::CONFIG_DIR, config.config_directory.to_owned()); + dirs.insert(&AgentConfig::LOG_DIR, config.log_directory.to_owned()); + dirs.insert(&AgentConfig::DATA_DIR, config.data_directory.to_owned()); + + if !config.server_cert_file.is_file() { + dirs.insert( + &AgentConfig::SERVER_CERT_FILE, + config.server_cert_file_dir().into(), + ); + } + if !config.server_key_file.is_file() { + dirs.insert( + &AgentConfig::SERVER_KEY_FILE, + config.server_key_file_dir().into(), + ); + } + + dirs +} diff --git a/src/config/mod.rs b/src/config/mod.rs index d923e49f..50149375 100644 --- a/src/config/mod.rs +++ b/src/config/mod.rs @@ -2,7 +2,7 @@ use anyhow::anyhow; use std::collections::hash_map::RandomState; use std::collections::{HashMap, HashSet}; use std::net::IpAddr; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::str::FromStr; use log::{debug, error, info, trace}; @@ -13,6 +13,7 @@ use stackable_config::{ConfigOption, Configurable, Configuration}; use thiserror::Error; use crate::config::AgentConfigError::{ArgumentParseError, WrongArgumentCount}; +use crate::fsext::{is_valid_file_path, normalize_path}; #[derive(Error, Debug)] pub enum AgentConfigError { @@ -32,8 +33,8 @@ pub struct AgentConfig { pub data_directory: PathBuf, pub server_ip_address: IpAddr, pub server_port: u16, - pub server_cert_file: Option, - pub server_key_file: Option, + pub server_cert_file: PathBuf, + pub server_key_file: PathBuf, pub tags: HashMap, pub session: bool, pub pod_cidr: String, @@ -182,6 +183,41 @@ impl AgentConfig { list: false }; + /// Returns the directory in which the `server_cert_file` is + /// located. + /// + /// If `server_cert_file` contains only a file name then + /// `Path::new("")` is returned. + /// + /// # Panics + /// + /// Panics if `server_cert_file` does not contain a file name. An + /// [`AgentConfig`] which was created by + /// [`Configurable::parse_values`] always contains a valid file + /// name. + pub fn server_cert_file_dir(&self) -> &Path { + self.server_cert_file + .parent() + .expect("server_cert_file should contain a file") + } + + /// Returns the directory in which the `server_key_file` is located. + /// + /// If `server_key_file` contains only a file name then + /// `Path::new("")` is returned. + /// + /// # Panics + /// + /// Panics if `server_key_file` does not contain a file name. An + /// [`AgentConfig`] which was created by + /// [`Configurable::parse_values`] always contains a valid file + /// name. + pub fn server_key_file_dir(&self) -> &Path { + self.server_key_file + .parent() + .expect("server_key_file should contain a file") + } + fn get_options() -> HashSet { [ AgentConfig::HOSTNAME, @@ -380,35 +416,40 @@ impl Configurable for AgentConfig { &parsed_values, &AgentConfig::DATA_DIR, error_list.as_mut(), - ); + ) + .map(|path: PathBuf| normalize_path(&path)); // Parse bootstrap file from values let final_bootstrap_file = AgentConfig::get_with_default( &parsed_values, &AgentConfig::BOOTSTRAP_FILE, error_list.as_mut(), - ); + ) + .map(|path: PathBuf| normalize_path(&path)); // Parse log directory let final_log_dir = AgentConfig::get_with_default( &parsed_values, &AgentConfig::LOG_DIR, error_list.as_mut(), - ); + ) + .map(|path: PathBuf| normalize_path(&path)); // Parse config directory let final_config_dir = AgentConfig::get_with_default( &parsed_values, &AgentConfig::CONFIG_DIR, error_list.as_mut(), - ); + ) + .map(|path: PathBuf| normalize_path(&path)); // Parse parcel directory let final_package_dir = AgentConfig::get_with_default( &parsed_values, &AgentConfig::PACKAGE_DIR, error_list.as_mut(), - ); + ) + .map(|path: PathBuf| normalize_path(&path)); // Parse pod cidr let final_pod_cidr: Result = AgentConfig::get_with_default( @@ -418,32 +459,38 @@ impl Configurable for AgentConfig { ); // Parse cert file - let final_server_cert_file = if let Ok(server_cert_file) = - AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::SERVER_CERT_FILE) - { - Some(PathBuf::from_str(&server_cert_file).unwrap_or_else(|_| { - panic!( - "Error parsing valid server cert file directory from string: {}", - server_cert_file - ) - })) - } else { - None - }; + let final_server_cert_file = AgentConfig::get_with_default( + &parsed_values, + &AgentConfig::SERVER_CERT_FILE, + error_list.as_mut(), + ) + .map(|path: PathBuf| normalize_path(&path)); + + if let Ok(file) = &final_server_cert_file { + if !is_valid_file_path(file) { + let error = ArgumentParseError { + name: AgentConfig::SERVER_CERT_FILE.name.to_string(), + }; + error_list.push(error); + } + } // Parse key file - let final_server_key_file = if let Ok(server_key_file) = - AgentConfig::get_exactly_one_string(&parsed_values, &AgentConfig::SERVER_KEY_FILE) - { - Some(PathBuf::from_str(&server_key_file).unwrap_or_else(|_| { - panic!( - "Error parsing valid server key file directory from string: {}", - server_key_file - ) - })) - } else { - None - }; + let final_server_key_file = AgentConfig::get_with_default( + &parsed_values, + &AgentConfig::SERVER_KEY_FILE, + error_list.as_mut(), + ) + .map(|path: PathBuf| normalize_path(&path)); + + if let Ok(file) = &final_server_key_file { + if !is_valid_file_path(file) { + let error = ArgumentParseError { + name: AgentConfig::SERVER_KEY_FILE.name.to_string(), + }; + error_list.push(error); + } + } let final_port = AgentConfig::get_with_default( &parsed_values, @@ -501,8 +548,8 @@ impl Configurable for AgentConfig { bootstrap_file: final_bootstrap_file.unwrap(), server_ip_address: final_ip, server_port: final_port.unwrap(), - server_cert_file: final_server_cert_file, - server_key_file: final_server_key_file, + server_cert_file: final_server_cert_file.unwrap(), + server_key_file: final_server_key_file.unwrap(), tags: final_tags, session: final_session, pod_cidr: final_pod_cidr.unwrap(), diff --git a/src/fsext.rs b/src/fsext.rs index 8e301eeb..c9b6526d 100644 --- a/src/fsext.rs +++ b/src/fsext.rs @@ -3,9 +3,12 @@ //! This module contains additional operations which are not present in //! `std::fs` and `std::os::$platform`. +use std::io; +use std::path::{Component, Path, PathBuf}; + use anyhow::{anyhow, Result}; -use nix::unistd; -use std::path::Path; +use nix::{libc::O_TMPFILE, unistd}; +use tokio::fs::OpenOptions; /// User identifier pub struct Uid(unistd::Uid); @@ -62,3 +65,75 @@ where } Ok(()) } + +/// Checks if the given directory exists and is writable by the current +/// process. +/// +/// The check is performed by creating an unnamed temporary file in the +/// given directory. The file will be automatically removed by the +/// operating system when the last handle is closed. +pub async fn check_dir_is_writable(directory: &Path) -> io::Result<()> { + OpenOptions::new() + .read(true) + .write(true) + .custom_flags(O_TMPFILE) + .open(directory) + .await + .map(|_| ()) +} + +/// Normalizes a path. +/// +/// In contrast to [`std::fs::canonicalize`] the path does not need to +/// exist. +/// +/// # Examples +/// +/// ```rust +/// # use stackable_agent::fsext::*; +/// use std::path::Path; +/// +/// assert_eq!(Path::new("foo/bar"), normalize_path(Path::new("foo//bar"))); +/// assert_eq!(Path::new("foo/bar"), normalize_path(Path::new("foo/./bar"))); +/// assert_eq!(Path::new("foo/bar"), normalize_path(Path::new("foo/bar/."))); +/// assert_eq!(Path::new("foo/../bar"), normalize_path(Path::new("foo/../bar"))); +/// assert_eq!(Path::new("foo/bar/.."), normalize_path(Path::new("foo/bar/.."))); +/// assert_eq!(Path::new("/foo"), normalize_path(Path::new("/foo"))); +/// assert_eq!(Path::new("./foo"), normalize_path(Path::new("./foo"))); +/// assert_eq!(Path::new("foo"), normalize_path(Path::new("foo/"))); +/// assert_eq!(Path::new("foo"), normalize_path(Path::new("foo"))); +/// assert_eq!(Path::new("/"), normalize_path(Path::new("/"))); +/// assert_eq!(Path::new("."), normalize_path(Path::new("."))); +/// assert_eq!(Path::new(".."), normalize_path(Path::new(".."))); +/// assert_eq!(Path::new(""), normalize_path(Path::new(""))); +/// ``` +pub fn normalize_path(path: &Path) -> PathBuf { + path.components().collect() +} + +/// Returns true if the given path could reference a file. +/// +/// In contrast to [`std::path::Path::is_file`] the file does not need +/// to exist. +/// +/// Use normalized paths to avoid confusing results. +/// +/// # Examples +/// +/// ```rust +/// # use stackable_agent::fsext::*; +/// use std::path::Path; +/// +/// assert!(is_valid_file_path(Path::new("foo/bar"))); +/// assert!(is_valid_file_path(Path::new("foo/bar/"))); +/// assert!(is_valid_file_path(Path::new("foo/bar/."))); +/// +/// assert!(!is_valid_file_path(Path::new("foo/bar/.."))); +/// assert!(!is_valid_file_path(Path::new("/"))); +/// assert!(!is_valid_file_path(Path::new("."))); +/// assert!(!is_valid_file_path(Path::new(".."))); +/// assert!(!is_valid_file_path(Path::new(""))); +/// ``` +pub fn is_valid_file_path(path: &Path) -> bool { + matches!(path.components().last(), Some(Component::Normal(_))) +}