diff options
| -rw-r--r-- | mod/hmac.cxx | 15 | ||||
| -rw-r--r-- | mod/hmac.hxx | 4 | ||||
| -rw-r--r-- | mod/mod-ci-github.cxx | 73 |
3 files changed, 42 insertions, 50 deletions
diff --git a/mod/hmac.cxx b/mod/hmac.cxx index dc9eca0..83a78bd 100644 --- a/mod/hmac.cxx +++ b/mod/hmac.cxx @@ -14,9 +14,12 @@ compute_hmac (const options::openssl_options& o, { fdpipe errp (fdopen_pipe ()); // stderr pipe. - // To compute an HMAC over stdin with the key "secret": + // To compute an HMAC over stdin with the key <secret>: // - // openssl mac -digest SHA256 -macopt "key:secret" HMAC + // openssl mac -digest SHA256 -macopt "key:<secret>" HMAC + // + // Note that here we assume both output and diagnostics will fit into pipe + // buffers and don't poll both with fdselect(). // openssl os (path ("-"), // Read message from openssl::out. path ("-"), // Write output to openssl::in. @@ -29,13 +32,13 @@ compute_hmac (const options::openssl_options& o, ifdstream err (move (errp.in)); - string h; // The HMAC. + string h; // The HMAC value. try { - // In case of exception, skip and close input after output. + // In case of an exception, skip and close input after output. // // Note: re-open in/out so that they get automatically closed on - // exception. + // an exception. // ifdstream in (os.in.release (), fdstream_mode::skip); ofdstream out (os.out.release ()); @@ -45,7 +48,7 @@ compute_hmac (const options::openssl_options& o, out.write (m.data (), m.size ()); out.close (); - // Read the HMAC from openssl's output. + // Read the HMAC value from openssl's output. // h = in.read_text (); in.close (); diff --git a/mod/hmac.hxx b/mod/hmac.hxx index 104b629..0f70b7e 100644 --- a/mod/hmac.hxx +++ b/mod/hmac.hxx @@ -9,9 +9,9 @@ namespace brep { // Compute the HMAC-SHA256 message authentication code over a message using - // the given key. + // the given key (alpha-numeric string, not encoded). // - // Return the HMAC or throw std::system_error in case of an error. + // Return the HMAC value or throw std::system_error in case of an error. // // Example output: // diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx index 7cdc01d..2473ba0 100644 --- a/mod/mod-ci-github.cxx +++ b/mod/mod-ci-github.cxx @@ -66,11 +66,7 @@ namespace brep HANDLER_DIAG; if (!options_->ci_github_app_webhook_secret_specified ()) - { - error << "webhook secret not configured"; - - throw invalid_request (404, "CI request submission disabled"); - } + throw invalid_request (404, "GitHub CI request submission disabled"); // Process headers. // @@ -83,8 +79,8 @@ namespace brep // we already do for the most part), and "webhook event" only when // more context would be useful? // - string event; // Webhook event. - string r_hmac; // Received HMAC. + string event; // Webhook event. + string hmac; // Received HMAC. { bool content_type (false); @@ -98,7 +94,17 @@ namespace brep if (!h.value) throw invalid_request (400, "missing x-hub-signature-256 value"); - r_hmac = *h.value; + // Parse the x-hub-signature-256 header value. For example: + // + // sha256=5e82258... + // + // Check for the presence of the "sha256=" prefix and then strip it + // to leave only the HMAC value. + // + if (h.value->find ("sha256=", 0, 7) == string::npos) + throw invalid_request (400, "invalid x-hub-signature-256 value"); + + hmac = h.value->substr (7); } // This event's UUID. // @@ -132,44 +138,29 @@ namespace brep } } - // Parse the x-hub-signature-256 header value. For example: - // - // sha256=5e822587094c68e646db8b916da1db2056d92f1dea4252136a533b4147a30cb7 - // - // Check for the presence of the "sha256=" prefix and then strip it to - // leave only the HMAC. - // - if (r_hmac.find ("sha256=", 0, 7) == string::npos) - { - throw invalid_request (400, - "invalid x-hub-signature-256 value: '" + - r_hmac + '\''); - } - r_hmac = r_hmac.substr (7); - if (!content_type) throw invalid_request (400, "missing content-type header"); if (event.empty ()) throw invalid_request (400, "missing x-github-event header"); + + if (hmac.empty ()) + throw invalid_request (400, "missing x-hub-signature-256 header"); } // Read the entire request body into a buffer because we need to compute // an HMAC over it and then parse it as JSON. The alternative of reading - // from the stream twice works out to be more complicated and expensive. - // - // @@ TMP Although it is possible (with a bit of work) to rewind the - // request's input stream, in that case we'd have to read from it - // twice (once for the HMAC and once for the JSON parsing) instead - // of just this once, temp buffer(s) would be necessary, and we'd - // still be storing the entire request body in memory anyway (just - // not here). + // from the stream twice works out to be more complicated (see also @@ + // TODO item in web/server/module.hxx). // - vector<char> body; + vector<char> body; // Change to string, use getline('\0') { - // Note: buffer=0 disables caching. + // Note that even though we may not need caching right now, we may later + // (e.g., to support cancel) so let's just enable it right away. // - istream& is (rq.content (64 * 1024, 0 /* buffer */)); + size_t limit (128 * 1024); + + istream& is (rq.content (limit, limit)); // Note that istream::read() sets failbit if unable to read the // requested number of bytes. @@ -197,18 +188,16 @@ namespace brep // Verify the received HMAC. // - // Compute the HMAC over the request body using the configured webhook - // secret as key and compare it to the received HMAC. + // Compute the HMAC value over the request body using the configured + // webhook secret as key and compare it to the received HMAC. // try { - string hmac ( - compute_hmac (*options_, - body, - options_->ci_github_app_webhook_secret ().c_str ())); + string h ( + compute_hmac (*options_, + body, + options_->ci_github_app_webhook_secret ().c_str ())); - // @@ TODO GitHub recommends constant-time comparison (timing attack). - // if (!icasecmp (hmac, r_hmac)) { string m ("computed HMAC does not match received HMAC"); |