aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorBoris Kolpackov <boris@codesynthesis.com>2024-03-01 12:30:12 +0200
committerFrancois Kritzinger <francois@codesynthesis.com>2024-04-24 15:14:54 +0200
commit08b10c0f688f45df9bb6e7e7f42b2e13d3041610 (patch)
tree0c7440e0e8a0b2f968f97d6a06f0b05bca45b009
parent3bda6ce90b877aabbda0ac4c4238901f3b8b1674 (diff)
Review
-rw-r--r--mod/hmac.cxx15
-rw-r--r--mod/hmac.hxx4
-rw-r--r--mod/mod-ci-github.cxx73
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");