From 9d21b26946b26037ded0a1d1187ff808eb1cad03 Mon Sep 17 00:00:00 2001 From: parisiale Date: Mon, 11 Apr 2016 19:47:19 +0100 Subject: [PATCH 1/5] (maint) Bump leatherman dependency on README --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 0f38bb7c..0a1cd942 100644 --- a/README.md +++ b/README.md @@ -19,7 +19,7 @@ Dependencies - Boost - OpenSSL - ruby (2.0 and newer) - - [leatherman][leatherman] (0.3.5 or newer) + - [leatherman][leatherman] (0.4.2 or newer) - [cpp-pcp-client][cpp-pcp-client] (master) Initial Setup From eb366abb83a9536f527734443edde93ae48c1e33 Mon Sep 17 00:00:00 2001 From: parisiale Date: Mon, 11 Apr 2016 19:50:33 +0100 Subject: [PATCH 2/5] (maint) Trivial style changes --- README.md | 8 +++-- lib/inc/pxp-agent/configuration.hpp | 24 ++++++++++----- lib/src/configuration.cc | 36 ++++++++++++++-------- lib/tests/unit/configuration_test.cc | 2 +- lib/tests/unit/util/posix/pid_file_test.cc | 2 +- 5 files changed, 48 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 0a1cd942..9ddd0dd0 100644 --- a/README.md +++ b/README.md @@ -47,7 +47,9 @@ The following will install most required tools and libraries: #### Setup on Windows -[MinGW-w64][MinGW-w64] is used for full C++11 support, and [Chocolatey][Chocolatey] can be used to install. You should have at least 2GB of memory for compilation. +[MinGW-w64][MinGW-w64] is used for full C++11 support, and +[Chocolatey][Chocolatey] can be used to install. You should have at least 2GB of +memory for compilation. * install [CMake][CMake-choco] & [7zip][7zip-choco] @@ -57,7 +59,9 @@ The following will install most required tools and libraries: choco install mingw --params "/threads:win32" -For the remaining tasks, build commands can be executed in the shell from Start > MinGW-w64 project > Run Terminal +For the remaining tasks, build commands can be executed in the shell from: + + Start > MinGW-w64 project > Run Terminal * select an install location for dependencies, such as C:\\tools or cmake\\release\\ext; we'll refer to it as $install diff --git a/lib/inc/pxp-agent/configuration.hpp b/lib/inc/pxp-agent/configuration.hpp index cd10a81e..befb81bf 100644 --- a/lib/inc/pxp-agent/configuration.hpp +++ b/lib/inc/pxp-agent/configuration.hpp @@ -29,7 +29,8 @@ extern const std::string DEFAULT_SPOOL_DIR; // used by unit tests enum Types { Integer, String, Bool, Double }; -struct EntryBase { +struct EntryBase +{ // Config option name // HERE: must match one of the flag names and config file option std::string name; @@ -51,7 +52,8 @@ struct EntryBase { }; template -struct Entry : EntryBase { +struct Entry : EntryBase +{ // Default value T value; @@ -107,18 +109,22 @@ void configure_platform_file_logging(); // Configuration (singleton) // -class Configuration { +class Configuration +{ public: - struct Error : public std::runtime_error { + struct Error : public std::runtime_error + { explicit Error(std::string const& msg) : std::runtime_error(msg) {} }; - static Configuration& Instance() { + static Configuration& Instance() + { static Configuration instance {}; return instance; } - struct Agent { + struct Agent + { std::string modules_dir; std::string broker_ws_uri; std::string ca; @@ -167,7 +173,8 @@ class Configuration { /// value in case nd the requested flag is known or throw a /// Configuration::Error otherwise. template - T get(std::string flagname) { + T get(std::string flagname) + { if (valid_) { try { return HW::GetFlag(flagname); @@ -193,7 +200,8 @@ class Configuration { /// Throw a Configuration::Error in case the specified flag is /// unknown or the value is invalid. template - void set(std::string flagname, T value) { + void set(std::string flagname, T value) + { if (!valid_) { throw Configuration::Error { "cannot set configuration value until " "configuration is validated" }; diff --git a/lib/src/configuration.cc b/lib/src/configuration.cc index 9973d605..35988187 100644 --- a/lib/src/configuration.cc +++ b/lib/src/configuration.cc @@ -102,7 +102,8 @@ static const std::string AGENT_CLIENT_TYPE { "agent" }; // void Configuration::initialize( - std::function)> start_function) { + std::function)> start_function) +{ // Ensure the state is reset (useful for testing) HW::Reset(); HW::SetAppName("pxp-agent"); @@ -127,7 +128,8 @@ void Configuration::initialize( start_function); // callback } -HW::ParseResult parseArguments(const int argc, char* const argv[]) { +HW::ParseResult parseArguments(const int argc, char* const argv[]) +{ // manipulate argc and v to make start the default action. // TODO(ploubser): Add ability to specify default action to HorseWhisperer int modified_argc = argc + 1; @@ -142,7 +144,8 @@ HW::ParseResult parseArguments(const int argc, char* const argv[]) { return HW::Parse(modified_argc, modified_argv); } -HW::ParseResult Configuration::parseOptions(int argc, char *argv[]) { +HW::ParseResult Configuration::parseOptions(int argc, char *argv[]) +{ auto parse_result = parseArguments(argc, argv); if (parse_result == HW::ParseResult::FAILURE @@ -162,7 +165,8 @@ HW::ParseResult Configuration::parseOptions(int argc, char *argv[]) { return parse_result; } -static void validateLogDirPath(const std::string& logfile) { +static void validateLogDirPath(const std::string& logfile) +{ auto logdir_path = fs::path(logfile).parent_path(); if (fs::exists(logdir_path)) { @@ -178,7 +182,8 @@ static void validateLogDirPath(const std::string& logfile) { } } -void Configuration::setupLogging() { +void Configuration::setupLogging() +{ logfile_ = HW::GetFlag("logfile"); auto log_on_stdout = (logfile_ == "-"); auto loglevel = HW::GetFlag("loglevel"); @@ -253,13 +258,15 @@ void Configuration::setupLogging() { } } -void Configuration::validate() { +void Configuration::validate() +{ validateAndNormalizeWebsocketSettings(); validateAndNormalizeOtherSettings(); valid_ = true; } -const Configuration::Agent& Configuration::getAgentConfiguration() const { +const Configuration::Agent& Configuration::getAgentConfiguration() const +{ assert(valid_); agent_configuration_ = Configuration::Agent { HW::GetFlag("modules-dir"), @@ -275,7 +282,8 @@ const Configuration::Agent& Configuration::getAgentConfiguration() const { return agent_configuration_; } -void Configuration::reopenLogfile() const { +void Configuration::reopenLogfile() const +{ if (!logfile_.empty() && logfile_ != "-") { try { logfile_fstream_.close(); @@ -299,11 +307,13 @@ Configuration::Configuration() : valid_ { false }, config_file_ { "" }, agent_configuration_ {}, logfile_ { "" }, - logfile_fstream_ {} { + logfile_fstream_ {} +{ defineDefaultValues(); } -void Configuration::defineDefaultValues() { +void Configuration::defineDefaultValues() +{ defaults_.insert( Option { "config-file", Base_ptr { new Entry( @@ -445,7 +455,8 @@ void Configuration::defineDefaultValues() { #endif } -void Configuration::setDefaultValues() { +void Configuration::setDefaultValues() +{ for (auto opt_idx = defaults_.get().begin(); opt_idx != defaults_.get().end(); ++opt_idx) { @@ -499,7 +510,8 @@ void Configuration::setDefaultValues() { } } -void Configuration::parseConfigFile() { +void Configuration::parseConfigFile() +{ lth_jc::JsonContainer config_json; if (!lth_file::file_readable(config_file_)) { diff --git a/lib/tests/unit/configuration_test.cc b/lib/tests/unit/configuration_test.cc index 06a41bc1..03960076 100644 --- a/lib/tests/unit/configuration_test.cc +++ b/lib/tests/unit/configuration_test.cc @@ -187,7 +187,7 @@ TEST_CASE("Configuration::get", "[configuration]") { #endif Configuration::Instance().validate(); - REQUIRE(Configuration::Instance().get("foreground") == false); + REQUIRE_FALSE(Configuration::Instance().get("foreground")); } SECTION("return the correct value after the flag has been set") { diff --git a/lib/tests/unit/util/posix/pid_file_test.cc b/lib/tests/unit/util/posix/pid_file_test.cc index 1eba91ed..c6e39dc1 100644 --- a/lib/tests/unit/util/posix/pid_file_test.cc +++ b/lib/tests/unit/util/posix/pid_file_test.cc @@ -290,7 +290,7 @@ TEST_CASE("PIDFile::unlockFile", "[util]") { FAIL("failed to create spool directory"); } - SECTION("it unlock a locked file") { + SECTION("it unlocks a locked file") { auto first_fd = open(LOCK_PATH.data(), O_RDWR | O_CREAT, 0640); if (first_fd == -1) FAIL(std::string { "failed to open " } + LOCK_PATH); PIDFile::lockFile(first_fd, F_WRLCK); From e3e4b5335f254750b4ac680ae17a768282d961f8 Mon Sep 17 00:00:00 2001 From: parisiale Date: Mon, 11 Apr 2016 19:54:38 +0100 Subject: [PATCH 3/5] (maint) Don't define namespace alias in header --- lib/inc/pxp-agent/configuration.hpp | 14 ++++++-------- lib/src/configuration.cc | 2 ++ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/inc/pxp-agent/configuration.hpp b/lib/inc/pxp-agent/configuration.hpp index befb81bf..189248e9 100644 --- a/lib/inc/pxp-agent/configuration.hpp +++ b/lib/inc/pxp-agent/configuration.hpp @@ -15,8 +15,6 @@ namespace PXPAgent { -namespace HW = HorseWhisperer; - // // Tokens // @@ -153,7 +151,7 @@ class Configuration /// Throw a Configuration::Error: if it fails to parse the CLI /// arguments; if the specified config file cannot be parsed or /// has any invalid JSON entry. - HW::ParseResult parseOptions(int argc, char *argv[]); + HorseWhisperer::ParseResult parseOptions(int argc, char *argv[]); /// Validate logging configuration options and enable logging. /// Throw a Configuration::Error: in case of invalid the specified @@ -177,8 +175,8 @@ class Configuration { if (valid_) { try { - return HW::GetFlag(flagname); - } catch (HW::undefined_flag_error& e) { + return HorseWhisperer::GetFlag(flagname); + } catch (HorseWhisperer::undefined_flag_error& e) { throw Configuration::Error { std::string { e.what() } }; } } @@ -208,10 +206,10 @@ class Configuration } try { - HW::SetFlag(flagname, value); - } catch (HW::flag_validation_error) { + HorseWhisperer::SetFlag(flagname, value); + } catch (HorseWhisperer::flag_validation_error) { throw Configuration::Error { "invalid value for " + flagname }; - } catch (HW::undefined_flag_error) { + } catch (HorseWhisperer::undefined_flag_error) { throw Configuration::Error { "unknown flag name: " + flagname }; } } diff --git a/lib/src/configuration.cc b/lib/src/configuration.cc index 35988187..edfe0ef0 100644 --- a/lib/src/configuration.cc +++ b/lib/src/configuration.cc @@ -34,6 +34,8 @@ namespace PXPAgent { +namespace HW = HorseWhisperer; + namespace fs = boost::filesystem; namespace lth_file = leatherman::file_util; namespace lth_jc = leatherman::json_container; From c6b4236c543b468b04925b240db37fe6b4e912e5 Mon Sep 17 00:00:00 2001 From: parisiale Date: Mon, 11 Apr 2016 19:55:55 +0100 Subject: [PATCH 4/5] (maint) Use boost::format for error messages --- lib/src/configuration.cc | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/lib/src/configuration.cc b/lib/src/configuration.cc index edfe0ef0..85636ede 100644 --- a/lib/src/configuration.cc +++ b/lib/src/configuration.cc @@ -693,21 +693,24 @@ void Configuration::validateAndNormalizeOtherSettings() { fs::remove_all(tmp_path); } } catch (const std::exception& e) { - LOG_DEBUG("Failed to create the test dir in spool path during" - "configuration validation: %1%", e.what()); + LOG_DEBUG("Failed to create the test dir in spool path '%1%' during" + "configuration validation: %2%", + spool_dir_path.string(), e.what()); } if (!is_writable) { - throw Configuration::Error { "--spool-dir '" - + spool_dir_path.string() + "' is not writable" }; + throw Configuration::Error { + (boost::format("the spool-dir '%1%' is not writable") + % spool_dir_path.string()).str() }; } #ifndef _WIN32 if (!HW::GetFlag("foreground")) { auto pid_file = lth_file::tilde_expand(HW::GetFlag("pidfile")); if (fs::exists(pid_file) && !(fs::is_regular_file(pid_file))) { - throw Configuration::Error { "the PID file '" + pid_file - + "' is not a regular file" }; + throw Configuration::Error { + (boost::format("the PID file '%1%' is not a regular file") + % pid_file).str() }; } auto pid_dir = fs::path(pid_file).parent_path().string(); @@ -722,18 +725,22 @@ void Configuration::validateAndNormalizeOtherSettings() { fs::create_directories(pid_dir); } catch (const std::exception& e) { LOG_DEBUG("Failed to create '%1%' directory: %2%", pid_dir, e.what()); - throw Configuration::Error { "failed to create the PID file directory" }; + throw Configuration::Error { + "failed to create the PID file directory" }; } } if (fs::exists(pid_dir)) { if (!fs::is_directory(pid_dir)) { - throw Configuration::Error { "the PID directory '" + pid_dir - + "' is not a directory" }; + throw Configuration::Error { + (boost::format("'%1%' is not a directory") + % pid_dir).str() }; } } else { - throw Configuration::Error { "the PID directory '" + pid_dir + "' " - "does not exist; cannot create PID file" }; + throw Configuration::Error { + (boost::format("the PID directory '%1%' does not exist; " + "cannot create the PID file") + % pid_dir).str() }; } HW::SetFlag("pidfile", pid_file); From 9512695dd0add3105c633167dd47cf8e7b51bde1 Mon Sep 17 00:00:00 2001 From: parisiale Date: Mon, 11 Apr 2016 19:56:49 +0100 Subject: [PATCH 5/5] (maint) Refactor Configuration functions Removing duplicated code. --- lib/src/configuration.cc | 165 ++++++++++++++++----------------------- 1 file changed, 68 insertions(+), 97 deletions(-) diff --git a/lib/src/configuration.cc b/lib/src/configuration.cc index 85636ede..2531e96f 100644 --- a/lib/src/configuration.cc +++ b/lib/src/configuration.cc @@ -514,11 +514,10 @@ void Configuration::setDefaultValues() void Configuration::parseConfigFile() { - lth_jc::JsonContainer config_json; + if (!lth_file::file_readable(config_file_)) + return; - if (!lth_file::file_readable(config_file_)) { - return; - } + lth_jc::JsonContainer config_json; try { config_json = lth_jc::JsonContainer(lth_file::read(config_file_)); @@ -526,115 +525,86 @@ void Configuration::parseConfigFile() throw Configuration::Error { "cannot parse config file; invalid JSON" }; } - if (config_json.type() != lth_jc::DataType::Object) { + if (config_json.type() != lth_jc::DataType::Object) throw Configuration::Error { "invalid config file content; not a " "JSON object" }; - } + + boost::format err_format { "field '%1%' must be of type %2%" }; + auto check_key_type = + [&config_json, &err_format] (const std::string& key, + const std::string& type_str, + const lth_jc::DataType& type) -> void + { + if (config_json.type(key) != type) + throw Configuration::Error { (err_format % key % type_str).str() }; + }; for (const auto& key : config_json.keys()) { const auto& opt_idx = defaults_.get().find(key); - if (opt_idx != defaults_.get().end()) { - if (opt_idx->ptr->configured) { - continue; - } + if (opt_idx == defaults_.get().end()) + throw Configuration::Error { + (boost::format("field '%1%' is not a valid configuration variable") + % key).str() }; - switch (opt_idx->ptr->type) { - case Integer: - if (config_json.type(key) == lth_jc::DataType::Int) { - HW::SetFlag(key, config_json.get(key)); - } else { - std::string err { "field '" + key + "' must be of " - "type Integer" }; - throw Configuration::Error { err }; - } - break; - case Bool: - if (config_json.type(key) == lth_jc::DataType::Bool) { - HW::SetFlag(key, config_json.get(key)); - } else { - std::string err { "field '" + key + "' must be of " - "type Bool" }; - throw Configuration::Error { err }; - } - break; - case Double: - if (config_json.type(key) == lth_jc::DataType::Double) { - HW::SetFlag(key, config_json.get(key)); - } else { - std::string err { "field '" + key + "' must be of " - "type Double" }; - throw Configuration::Error { err }; - } - break; - default: - if (config_json.type(key) == lth_jc::DataType::String) { - auto val = config_json.get(key); - HW::SetFlag(key, val); - } else { - std::string err { "field '" + key + "' must be of " - "type String" }; - throw Configuration::Error { err }; - } - } - } else { - std::string err { "field '" + key + "' is not a valid " - "configuration variable" }; - throw Configuration::Error { err }; + if (opt_idx->ptr->configured) + continue; + + switch (opt_idx->ptr->type) { + case Integer: + check_key_type(key, "Integer", lth_jc::DataType::Int); + HW::SetFlag(key, config_json.get(key)); + break; + case Bool: + check_key_type(key, "Bool", lth_jc::DataType::Bool); + HW::SetFlag(key, config_json.get(key)); + break; + case Double: + check_key_type(key, "Double", lth_jc::DataType::Double); + HW::SetFlag(key, config_json.get(key)); + break; + default: + check_key_type(key, "String", lth_jc::DataType::String); + HW::SetFlag(key, config_json.get(key)); } } } -void Configuration::validateAndNormalizeWebsocketSettings() { - if (HW::GetFlag("broker-ws-uri").empty()) { - throw Configuration::Error { "broker-ws-uri value must be defined" }; - } else if (HW::GetFlag("broker-ws-uri").find("wss://") != 0) { - throw Configuration::Error { "broker-ws-uri value must start with wss://" }; - } +// Helper function +std::string check_and_expand_ssl_cert(const std::string& cert_name) +{ + auto c = HW::GetFlag(cert_name); + if (c.empty()) + throw Configuration::Error { + (boost::format("%1% value must be defined") % cert_name).str() }; - auto ca = HW::GetFlag("ssl-ca-cert"); - auto cert = HW::GetFlag("ssl-cert"); - auto key = HW::GetFlag("ssl-key"); + c = lth_file::tilde_expand(c); + if (!fs::exists(c)) + throw Configuration::Error { + (boost::format("%1% file '%2%' not found") % cert_name % c).str() }; - if (ca.empty()) { - throw Configuration::Error { "ssl-ca-cert value must be defined" }; - } else { - ca = lth_file::tilde_expand(ca); - if (!fs::exists(ca)) { - throw Configuration::Error { - (boost::format("ssl-ca-cert file '%1%' not found") % ca).str() }; - } else if (!lth_file::file_readable(ca)) { - throw Configuration::Error { - (boost::format("ssl-ca-cert file '%1%' not readable") % ca).str() }; - } - } + if (!lth_file::file_readable(c)) + throw Configuration::Error { + (boost::format("%1% file '%2%' not readable") % cert_name % c).str() }; - if (cert.empty()) { - throw Configuration::Error { "ssl-cert value must be defined" }; - } else { - cert = lth_file::tilde_expand(cert); - if (!fs::exists(cert)) { - throw Configuration::Error { - (boost::format("ssl-cert file '%1%' not found") % cert).str() }; - } else if (!lth_file::file_readable(cert)) { - throw Configuration::Error { - (boost::format("ssl-cert file '%1%' not readable") % cert).str() }; - } - } + return c; +} - if (key.empty()) { - throw Configuration::Error { "ssl-key value must be defined" }; - } else { - key = lth_file::tilde_expand(key); - if (!fs::exists(key)) { - throw Configuration::Error { - (boost::format("ssl-key file '%1%' not found") % key).str() }; - } else if (!lth_file::file_readable(key)) { - throw Configuration::Error { - (boost::format("ssl-key file '%1%' not readable") % key).str() }; - } - } +void Configuration::validateAndNormalizeWebsocketSettings() +{ + // Check the broker's WebSocket URI + auto broker_ws_uri = HW::GetFlag("broker-ws-uri"); + if (broker_ws_uri.empty()) + throw Configuration::Error { "broker-ws-uri value must be defined" }; + if (broker_ws_uri.find("wss://") != 0) + throw Configuration::Error { "broker-ws-uri value must start with wss://" }; + + // Check the SSL options and expand the paths + auto ca = check_and_expand_ssl_cert("ssl-ca-cert"); + auto cert = check_and_expand_ssl_cert("ssl-cert"); + auto key = check_and_expand_ssl_cert("ssl-key"); + // Ensure client certs are good try { PCPClient::getCommonNameFromCert(cert); PCPClient::validatePrivateKeyCertPair(key, cert); @@ -642,6 +612,7 @@ void Configuration::validateAndNormalizeWebsocketSettings() { throw Configuration::Error { e.what() }; } + // Set the expanded cert paths HW::SetFlag("ssl-ca-cert", ca); HW::SetFlag("ssl-cert", cert); HW::SetFlag("ssl-key", key);