From d6a34b68d4667d4b99c1e76d63604a7bc1c9c3dd Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Thu, 25 May 2017 21:12:03 +0300 Subject: Add support for bbot agent authentication --- etc/brep-module.conf | 28 +++++++++++ libbrep/build.cxx | 2 + libbrep/build.hxx | 10 +++- libbrep/build.xml | 2 + libbrep/utility.hxx | 3 +- mod/build-config.cxx | 77 ++++++++++++++++++++++++++++- mod/build-config.hxx | 15 ++++++ mod/database-module.cxx | 6 ++- mod/database-module.hxx | 3 ++ mod/diagnostics.hxx | 2 +- mod/mod-build-result.cxx | 123 +++++++++++++++++++++++++++++++++++++++-------- mod/mod-build-task.cxx | 89 ++++++++++++++++++++++++++++++++-- mod/options.cli | 42 +++++++++++++++- 13 files changed, 371 insertions(+), 31 deletions(-) diff --git a/etc/brep-module.conf b/etc/brep-module.conf index 53678bb..6499d4b 100644 --- a/etc/brep-module.conf +++ b/etc/brep-module.conf @@ -76,6 +76,20 @@ menu About=?about # build-config +# Directory containing build bot agent public keys. If specified, then brep +# will perform agent authentication and will reject build results from +# unauthenticated ones. If not specified, then build results are accepted from +# all agents (which will be a security risk if the brep instance is publicly +# accessible). +# +# The directory is expected to contain one PEM-encoded public key per file with +# the .pem extension. All other files and subdirectories are ignored. The brep +# instance needs to be restarted after adding new key files for the changes to +# take effect. +# +# build-bot-agent-keys + + # Number of packages build configurations per page. # # build-configurations 10 @@ -144,6 +158,20 @@ menu About=?about # build-db-retry 10 +# The openssl program to be used for crypto operations. You can also specify +# additional options that should be passed to the openssl program with +# openssl-option. If the openssl program is not explicitly specified, then brep +# will use openssl by default. +# +# openssl openssl + + +# Additional option to be passed to the openssl program (see openssl for +# details). Repeat this option to specify multiple openssl options. +# +# openssl-option + + # Trace verbosity. Disabled by default. # # verbosity 0 diff --git a/libbrep/build.cxx b/libbrep/build.cxx index 2391165..0941c5f 100644 --- a/libbrep/build.cxx +++ b/libbrep/build.cxx @@ -60,6 +60,7 @@ namespace brep build (string pnm, version pvr, string cfg, string tnm, version tvr, + optional afp, optional ach, string mnm, string msm, optional trg) : id (package_id (move (pnm), pvr), move (cfg), tvr), @@ -71,6 +72,7 @@ namespace brep state (build_state::building), timestamp (timestamp_type::clock::now ()), force (force_state::unforced), + agent_fingerprint (move (afp)), agent_challenge (move (ach)), machine (move (mnm)), machine_summary (move (msm)), target (move (trg)) diff --git a/libbrep/build.hxx b/libbrep/build.hxx index afa96ed..6b58402 100644 --- a/libbrep/build.hxx +++ b/libbrep/build.hxx @@ -145,6 +145,8 @@ namespace brep build (string package_name, version package_version, string configuration, string toolchain_name, version toolchain_version, + optional agent_fingerprint, + optional agent_challenge, string machine, string machine_summary, optional target); @@ -164,10 +166,16 @@ namespace brep force_state force; - // Must present for the built state, may present for the building state. + // Must be present for the built state, may be present for the building + // state. // optional status; + // May be present only for the building state. + // + optional agent_fingerprint; + optional agent_challenge; + // Present only for building and built states. // optional machine; diff --git a/libbrep/build.xml b/libbrep/build.xml index 7466c97..cee65ac 100644 --- a/libbrep/build.xml +++ b/libbrep/build.xml @@ -20,6 +20,8 @@ + + diff --git a/libbrep/utility.hxx b/libbrep/utility.hxx index c81413c..1900bc4 100644 --- a/libbrep/utility.hxx +++ b/libbrep/utility.hxx @@ -11,7 +11,8 @@ #include // assert() #include // make_move_iterator() -#include // reverse_iterate(), operator<<(ostream, exception) +#include // reverse_iterate(), + // operator<<(ostream, exception) namespace brep { diff --git a/mod/build-config.cxx b/mod/build-config.cxx index 9eb40ce..9e30b64 100644 --- a/mod/build-config.cxx +++ b/mod/build-config.cxx @@ -5,18 +5,26 @@ #include #include +#include + +#include +#include // throw_generic_error() +#include +#include #include namespace brep { + using namespace std; using namespace web; + using namespace butl; using namespace bbot; shared_ptr shared_build_config (const path& p) { - static std::map> configs; + static map> configs; auto i (configs.find (p)); if (i != configs.end ()) @@ -32,6 +40,73 @@ namespace brep return c; } + shared_ptr + shared_bot_agent_keys (const options::openssl_options& o, const dir_path& d) + { + static map> keys; + + auto i (keys.find (d)); + if (i != keys.end ()) + { + if (shared_ptr k = i->second.lock ()) + return k; + } + + shared_ptr ak (make_shared ()); + + // Intercept exception handling to make error descriptions more + // informative. + // + // Path of the key being converted. Used for diagnostics. + // + path p; + + try + { + for (const dir_entry& de: dir_iterator (d)) + { + if (de.path ().extension () == "pem" && + de.type () == entry_type::regular) + { + p = d / de.path (); + + openssl os (p, path ("-"), 2, + o.openssl (), "pkey", + o.openssl_option (), "-pubin", "-outform", "DER"); + + vector k (os.in.read_binary ()); + os.in.close (); + + if (!os.wait ()) + throw io_error (""); + + ak->emplace (sha256 (k.data (), k.size ()).string (), move (p)); + } + } + } + catch (const io_error&) + { + ostringstream os; + os << "unable to convert bbot agent pubkey " << p; + throw_generic_error (EIO, os.str ().c_str ()); + } + catch (const process_error& e) + { + ostringstream os; + os << "unable to convert bbot agent pubkey " << p; + throw_generic_error (e.code ().value (), os.str ().c_str ()); + } + catch (const system_error& e) + { + ostringstream os; + os<< "unable to iterate over agents keys directory '" << d << "'"; + throw_generic_error (e.code ().value (), os.str ().c_str ()); + } + + keys[d] = ak; + return ak; + } + string build_log_url (const string& host, const dir_path& root, const build& b, diff --git a/mod/build-config.hxx b/mod/build-config.hxx index 6058774..b49819d 100644 --- a/mod/build-config.hxx +++ b/mod/build-config.hxx @@ -5,6 +5,8 @@ #ifndef MOD_BUILD_CONFIG_HXX #define MOD_BUILD_CONFIG_HXX +#include + #include #include @@ -12,6 +14,8 @@ #include +#include + namespace brep { // Return pointer to the shared build configurations instance, creating one @@ -21,6 +25,17 @@ namespace brep shared_ptr shared_build_config (const path&); + // Map of build bot agent public keys fingerprints to the key file paths. + // + using bot_agent_keys = std::map; + + // Return pointer to the shared build bot agent public keys map, creating + // one on the first call. Throw system_error on the underlying openssl or OS + // error. Not thread-safe. + // + shared_ptr + shared_bot_agent_keys (const options::openssl_options&, const dir_path&); + // Return the package configuration build log url. By default the url is to // the operations combined log. // diff --git a/mod/database-module.cxx b/mod/database-module.cxx index 9daff53..67e4c9d 100644 --- a/mod/database-module.cxx +++ b/mod/database-module.cxx @@ -34,7 +34,8 @@ namespace brep build_db_ (r.initialized_ ? r.build_db_ : nullptr), build_conf_ (r.initialized_ ? r.build_conf_ : nullptr), build_conf_names_ (r.initialized_ ? r.build_conf_names_ : nullptr), - build_conf_map_ (r.initialized_ ? r.build_conf_map_ : nullptr) + build_conf_map_ (r.initialized_ ? r.build_conf_map_ : nullptr), + bot_agent_keys_ (r.initialized_ ? r.bot_agent_keys_ : nullptr) { } @@ -67,6 +68,9 @@ namespace brep throw_generic_error (EIO, os.str ().c_str ()); } + if (bo.build_bot_agent_keys_specified ()) + bot_agent_keys_ = shared_bot_agent_keys (bo, bo.build_bot_agent_keys ()); + cstrings conf_names; using conf_map_type = map #include +#include namespace brep { @@ -74,6 +75,8 @@ namespace brep butl::compare_c_string>> build_conf_map_; + shared_ptr bot_agent_keys_; + private: virtual bool handle (request&, response&, log&); diff --git a/mod/diagnostics.hxx b/mod/diagnostics.hxx index e05d56a..46b9e17 100644 --- a/mod/diagnostics.hxx +++ b/mod/diagnostics.hxx @@ -261,7 +261,7 @@ namespace brep const char* name_; const void* data_; }; - typedef diag_mark basic_mark; + using basic_mark = diag_mark; template struct fail_mark_base diff --git a/mod/mod-build-result.cxx b/mod/mod-build-result.cxx index ae6c5b1..41bfb2b 100644 --- a/mod/mod-build-result.cxx +++ b/mod/mod-build-result.cxx @@ -7,7 +7,9 @@ #include #include +#include #include +#include #include #include #include @@ -226,6 +228,11 @@ handle (request& rq, response&) return true; } + auto print_args = [&trace, this] (const char* args[], size_t n) + { + l2 ([&]{trace << process_args {args, n};}); + }; + // Load and update the package build configuration (if present). // shared_ptr b; @@ -245,28 +252,109 @@ handle (request& rq, response&) warn_expired ("non-matching timestamp"); else { - unforced = b->force == force_state::unforced; + // Check the challenge. + // + // If the challenge doesn't match expectations (probably due to the + // authentication settings change), then we log this case with the + // warning severity and respond with the 200 HTTP code as if the + // challenge is valid. The thinking is that we shouldn't alarm a + // law-abaiding agent and shouldn't provide any information to a + // malicious one. + // + auto warn_auth = [&rqm, &warn] (const string& d) + { + warn << "session '" << rqm.session << "' authentication failed: " << d; + }; + + bool auth (false); - // Don's send email for the success-to-success status change, unless the - // build was forced. + // Must both be present or absent. // - notify = !(rqm.result.status == result_status::success && - b->status && *b->status == rqm.result.status && unforced); + if (!b->agent_challenge != !rqm.challenge) + warn_auth (rqm.challenge + ? "unexpected challenge" + : "challenge is expected"); + else if (bot_agent_keys_ == nullptr) // Authentication is disabled. + auth = true; + else if (!b->agent_challenge) // Authentication is recently enabled. + warn_auth ("challenge is required now"); + else + { + assert (b->agent_fingerprint && rqm.challenge); + auto i (bot_agent_keys_->find (*b->agent_fingerprint)); + + // The agent's key is recently replaced. + // + if (i == bot_agent_keys_->end ()) + warn_auth ("agent's public key not found"); + else + { + try + { + openssl os (print_args, + path ("-"), fdstream_mode::text, 2, + options_->openssl (), "rsautl", + options_->openssl_option (), + "-verify", "-pubin", "-inkey", i->second); + + for (const auto& c: *rqm.challenge) + os.out.put (c); // Sets badbit on failure. + + os.out.close (); + + string s; + getline (os.in, s); + + bool v (os.in.eof ()); + os.in.close (); + + if (os.wait () && v) + { + auth = s == *b->agent_challenge; + + if (!auth) + warn_auth ("challenge mismatched"); + } + else // The signature is presumably meaningless. + warn_auth ("unable to verify challenge"); + } + catch (const system_error& e) + { + fail << "unable to verify challenge: " << e; + } + } + } - prev_status = move (b->status); + if (auth) + { + unforced = b->force == force_state::unforced; - b->state = build_state::built; - b->status = rqm.result.status; - b->force = force_state::unforced; + // Don's send email for the success-to-success status change, unless + // the build was forced. + // + notify = !(rqm.result.status == result_status::success && + b->status && *b->status == rqm.result.status && unforced); - // Mark the section as loaded, so results are updated. - // - b->results_section.load (); - b->results = move (rqm.result.results); + prev_status = move (b->status); + + b->state = build_state::built; + b->status = rqm.result.status; + b->force = force_state::unforced; + + // Cleanup the authentication data. + // + b->agent_fingerprint = nullopt; + b->agent_challenge = nullopt; + + // Mark the section as loaded, so results are updated. + // + b->results_section.load (); + b->results = move (rqm.result.results); - b->timestamp = timestamp::clock::now (); + b->timestamp = timestamp::clock::now (); - build_db_->update (b); + build_db_->update (b); + } } t.commit (); @@ -299,11 +387,6 @@ handle (request& rq, response&) ? *p->package_email : p->email); - auto print_args = [&trace, this] (const char* args[], size_t n) - { - l2 ([&]{trace << process_args {args, n};}); - }; - // Redirect the diagnostics to webserver error log. // // Note: if using this somewhere else, then need to factor out all this diff --git a/mod/mod-build-task.cxx b/mod/mod-build-task.cxx index e47ae60..c018b65 100644 --- a/mod/mod-build-task.cxx +++ b/mod/mod-build-task.cxx @@ -11,8 +11,12 @@ #include #include +#include #include // compare_c_string +#include +#include // nullfd #include // path_match() +#include #include #include @@ -110,6 +114,21 @@ handle (request& rq, response& rs) throw invalid_request (400, e.what ()); } + // Obtain the agent's public key fingerprint if requested. If the fingerprint + // is requested but is not present in the request or is unknown, then respond + // with 401 HTTP code (unauthorized). + // + optional agent_fp; + + if (bot_agent_keys_ != nullptr) + { + if (!tqm.fingerprint || + bot_agent_keys_->find (*tqm.fingerprint) == bot_agent_keys_->end ()) + throw invalid_request (401, "unauthorized"); + + agent_fp = move (tqm.fingerprint); + } + task_response_manifest tsm; // Map build configurations to machines that are capable of building them. @@ -188,10 +207,8 @@ handle (request& rq, response& rs) cm.config->target, cm.config->vars); - // @@ We don't support challenge at the moment. - // return task_response_manifest (move (session), - nullopt, + move (b->agent_challenge), move (result_url), move (task)); }; @@ -224,6 +241,61 @@ handle (request& rq, response& rs) timestamp forced_rebuild_expiration ( expiration (options_->build_forced_rebuild_timeout ())); + // Return the challenge (nonce) if brep is configured to authenticate bbot + // agents. Return nullopt otherwise. + // + // Nonce generator must guarantee a probabilistically insignificant chance + // of repeating a previously generated value. The common approach is to use + // counters or random number generators (alone or in combination), that + // produce values of the sufficient length. 64-bit non-repeating and + // 512-bit random numbers are considered to be more than sufficient for + // most practical purposes. + // + // We will produce the challenge as the sha256sum of the 512-bit random + // number and the 64-bit current timestamp combination. The latter is + // not really a non-repeating counter and can't be used alone. However + // adding it is a good and cheap uniqueness improvement. + // + auto challenge = [&agent_fp, &now, &fail, &trace, this] () + { + optional r; + + if (agent_fp) + { + try + { + auto print_args = [&trace, this] (const char* args[], size_t n) + { + l2 ([&]{trace << process_args {args, n};}); + }; + + openssl os (print_args, + nullfd, path ("-"), 2, + options_->openssl (), "rand", + options_->openssl_option (), 64); + + vector nonce (os.in.read_binary ()); + os.in.close (); + + if (!os.wait () || nonce.size () != 64) + fail << "unable to generate nonce"; + + uint64_t t (chrono::duration_cast ( + now.time_since_epoch ()).count ()); + + sha256 cs (nonce.data (), nonce.size ()); + cs.append (&t, sizeof (t)); + r = cs.string (); + } + catch (const system_error& e) + { + fail << "unable to generate nonce: " << e; + } + } + + return r; + }; + // Convert butl::standard_version type to brep::version. // brep::version toolchain_version (tqm.toolchain_version.string ()); @@ -393,6 +465,7 @@ handle (request& rq, response& rs) machine_header_manifest& mh (*cm.machine); build_id bid (move (id), cm.config->name, toolchain_version); shared_ptr b (build_db_->find (bid)); + optional cl (challenge ()); // If build configuration doesn't exist then create the new one // and persist. Otherwise put it into the building state, refresh @@ -405,6 +478,8 @@ handle (request& rq, response& rs) move (bid.configuration), move (tqm.toolchain_name), move (toolchain_version), + move (agent_fp), + move (cl), mh.name, move (mh.summary), cm.config->target); @@ -438,6 +513,8 @@ handle (request& rq, response& rs) b->force = force_state::forced; b->toolchain_name = move (tqm.toolchain_name); + b->agent_fingerprint = move (agent_fp); + b->agent_challenge = move (cl); b->machine = mh.name; b->machine_summary = move (mh.summary); b->target = cm.config->target; @@ -504,6 +581,8 @@ handle (request& rq, response& rs) sort (rebuilds.begin (), rebuilds.end (), cmp); + optional cl (challenge ()); + // Pick the first package configuration from the ordered list. // // Note that the configurations may not match the required criteria @@ -557,8 +636,10 @@ handle (request& rq, response& rs) b->state = build_state::building; b->machine = mh.name; - // Can't move from, as may need it on the next iteration. + // Can't move from, as may need them on the next iteration. // + b->agent_fingerprint = agent_fp; + b->agent_challenge = cl; b->toolchain_name = tqm.toolchain_name; b->machine_summary = mh.summary; diff --git a/mod/options.cli b/mod/options.cli index 65f5549..e6beb6e 100644 --- a/mod/options.cli +++ b/mod/options.cli @@ -52,6 +52,27 @@ namespace brep } }; + class openssl_options + { + path openssl = "openssl" + { + "", + "The openssl program to be used for crypto operations. You can also + specify additional options that should be passed to the openssl + program with \cb{openssl-option}. If the openssl program is not + explicitly specified, then \cb{brep} will use \cb{openssl} by + default." + } + + strings openssl-option + { + "", + "Additional option to be passed to the openssl program (see + \cb{openssl} for details). Repeat this option to specify multiple + openssl options." + } + }; + class package_db { string package-db-user @@ -107,14 +128,31 @@ namespace brep } }; - class build + class build: openssl_options { path build-config { "", "Build configuration file. If not specified, then the package building functionality will be disabled. If specified, then the build database - must be configured (see \cb{build-db-*})." + must be configured (see \cb{build-db-*}). The \cb{brep} instance + needs to be restarted after modifying for the changes to + take effect." + } + + dir_path build-bot-agent-keys + { + "", + "Directory containing build bot agent public keys. If specified, then + \cb{brep} will perform agent authentication and will reject build + results from unauthenticated ones. If not specified, then build + results are accepted from all agents (which will be a security + risk if the \cb{brep} instance is publicly accessible). + + The directory is expected to contain one PEM-encoded public key + per file with the \cb{.pem} extension. All other files and + subdirectories are ignored. The \cb{brep} instance needs to be + restarted after adding new key files for the changes to take effect." } size_t build-forced-rebuild-timeout = 600 -- cgit v1.1