Verify webhook request HMACs

5 files changed, 236 insertions, 6 deletions

diff --git a/mod/hmac.cxx b/mod/hmac.cxx
new file mode 100644
index 0000000..dc9eca0
--- /dev/null
+++ b/mod/hmac.cxx
@@ -0,0 +1,92 @@
+#include <mod/hmac.hxx>
+
+#include <libbutl/openssl.hxx>
+
+using namespace std;
+using namespace butl;
+
+string brep::
+compute_hmac (const options::openssl_options& o,
+              const vector<char>& m,
+              const char* k)
+{
+  try
+  {
+    fdpipe errp (fdopen_pipe ()); // stderr pipe.
+
+    // To compute an HMAC over stdin with the key "secret":
+    //
+    //   openssl mac -digest SHA256 -macopt "key:secret" HMAC
+    //
+    openssl os (path ("-"), // Read message from openssl::out.
+                path ("-"), // Write output to openssl::in.
+                process::pipe (errp.in.get (), move (errp.out)),
+                process_env (o.openssl (), o.openssl_envvar ()),
+                "mac", o.openssl_option (),
+                "-digest", "SHA256",
+                "-macopt", string ("key:") + k,
+                "HMAC");
+
+    ifdstream err (move (errp.in));
+
+    string h; // The HMAC.
+    try
+    {
+      // In case of exception, skip and close input after output.
+      //
+      // Note: re-open in/out so that they get automatically closed on
+      // exception.
+      //
+      ifdstream in (os.in.release (), fdstream_mode::skip);
+      ofdstream out (os.out.release ());
+
+      // Write the message to openssl's input.
+      //
+      out.write (m.data (), m.size ());
+      out.close ();
+
+      // Read the HMAC from openssl's output.
+      //
+      h = in.read_text ();
+      in.close ();
+    }
+    catch (const io_error& e)
+    {
+      // If the process exits with non-zero status, assume the IO error is due
+      // to that and fall through.
+      //
+      if (os.wait ())
+      {
+        throw_generic_error (
+          e.code ().value (),
+          (string ("unable to read/write openssl stdout/stdin: ") +
+           e.what ()).c_str ());
+      }
+    }
+
+    if (!os.wait ())
+    {
+      string et (err.read_text ());
+      throw_generic_error (EINVAL,
+                           ("non-zero openssl exit status: " + et).c_str ());
+    }
+
+    err.close ();
+
+    return h;
+  }
+  catch (const process_error& e)
+  {
+    throw_generic_error (
+      e.code ().value (),
+      (string ("unable to execute openssl: ") + e.what ()).c_str ());
+  }
+  catch (const io_error& e)
+  {
+    // Unable to read diagnostics from stderr.
+    //
+    throw_generic_error (
+      e.code ().value (),
+      (string ("unable to read openssl stderr : ") + e.what ()).c_str ());
+  }
+}
diff --git a/mod/hmac.hxx b/mod/hmac.hxx
new file mode 100644
index 0000000..104b629
--- /dev/null
+++ b/mod/hmac.hxx
@@ -0,0 +1,29 @@
+#ifndef MOD_HMAC_HXX
+#define MOD_HMAC_HXX
+
+#include <libbrep/types.hxx>
+#include <libbrep/utility.hxx>
+
+#include <mod/module-options.hxx>
+
+namespace brep
+{
+  // Compute the HMAC-SHA256 message authentication code over a message using
+  // the given key.
+  //
+  // Return the HMAC or throw std::system_error in case of an error.
+  //
+  // Example output:
+  //
+  //   5e822587094c68e646db8b916da1db2056d92f1dea4252136a533b4147a30cb7
+  //
+  // Note that although any cryptographic hash function can be used to compute
+  // an HMAC, this implementation supports only SHA-256.
+  //
+  string
+  compute_hmac (const options::openssl_options&,
+                const vector<char>& message,
+                const char* key);
+}
+
+#endif
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index e19a41b..7cdc01d 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -7,6 +7,7 @@
 #include <libbutl/json/parser.hxx>
 
 #include <mod/jwt.hxx>
+#include <mod/hmac.hxx>
 #include <mod/module-options.hxx>
 
 #include <stdexcept>
@@ -64,10 +65,12 @@ namespace brep
 
     HANDLER_DIAG;
 
-    // @@ TODO: disable service if HMAC is not specified in config.
-    //
-    if (false)
+    if (!options_->ci_github_app_webhook_secret_specified ())
+    {
+      error << "webhook secret not configured";
+
       throw invalid_request (404, "CI request submission disabled");
+    }
 
     // Process headers.
     //
@@ -80,15 +83,26 @@ namespace brep
     //        we already do for the most part), and "webhook event" only when
     //        more context would be useful?
     //
-    string event; // Webhook event.
+    string event;  // Webhook event.
+    string r_hmac; // Received HMAC.
     {
       bool content_type (false);
 
       for (const name_value& h: rq.headers ())
       {
+        // HMAC authenticating this request. Note that it won't be present
+        // unless a webhook secret has been set in the GitHub app's settings.
+        //
+        if (icasecmp (h.name, "x-hub-signature-256") == 0)
+        {
+          if (!h.value)
+            throw invalid_request (400, "missing x-hub-signature-256 value");
+
+          r_hmac = *h.value;
+        }
         // This event's UUID.
         //
-        if (icasecmp (h.name, "x-github-delivery") == 0)
+        else if (icasecmp (h.name, "x-github-delivery") == 0)
         {
           // @@ TODO Check that delivery UUID has not been received before
           //         (replay attack).
@@ -118,6 +132,21 @@ 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");
 
@@ -125,6 +154,75 @@ namespace brep
         throw invalid_request (400, "missing x-github-event 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).
+    //
+    vector<char> body;
+    {
+      // Note: buffer=0 disables caching.
+      //
+      istream& is (rq.content (64 * 1024, 0 /* buffer */));
+
+      // Note that istream::read() sets failbit if unable to read the
+      // requested number of bytes.
+      //
+      is.exceptions (istream::badbit);
+
+      try
+      {
+        size_t n (0); // Total bytes read.
+
+        while (!eof (is))
+        {
+          body.resize (n + 8192);
+          is.read (body.data () + n, 8192);
+          n += is.gcount ();
+        }
+
+        body.resize (n);
+      }
+      catch (const io_error& e)
+      {
+        fail << "unable to read request body: " << e;
+      }
+    }
+
+    // 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.
+    //
+    try
+    {
+      string hmac (
+          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");
+
+        error << m;
+
+        throw invalid_request (400, move (m));
+      }
+    }
+    catch (const system_error& e)
+    {
+      fail << "unable to compute request HMAC: " << e;
+    }
+
     // There is a webhook event (specified in the x-github-event header) and
     // each event contains a bunch of actions (specified in the JSON request
     // body).
@@ -140,7 +238,7 @@ namespace brep
       check_suite_event cs;
       try
       {
-        json::parser p (rq.content (64 * 1024), "check_suite event");
+        json::parser p (body.data (), body.size (), "check_suite event");
 
         cs = check_suite_event (p);
       }
diff --git a/mod/module.cli b/mod/module.cli
index c635689..bec4c2d 100644
--- a/mod/module.cli
+++ b/mod/module.cli
@@ -832,6 +832,12 @@ namespace brep
         "The GitHub App ID. Found in the app's settings on GitHub."
       }
 
+      string ci-github-app-webhook-secret
+      {
+        "<secret>",
+        "The GitHub app's configured webhook secret."
+      }
+
       path ci-github-app-private-key
       {
         "<path>",
diff --git a/web/server/module.hxx b/web/server/module.hxx
index 20f6217..1b067f5 100644
--- a/web/server/module.hxx
+++ b/web/server/module.hxx
@@ -82,6 +82,11 @@ namespace web
   using name_values = std::vector<name_value>;
   using butl::path;
 
+  // @@ TODO: Expose the request input stream rewinding support provided by
+  //          web::apache::request::rewind(). Might come in useful in cases
+  //          where more than one thing needs to be done with the request
+  //          body, e.g., compute its MAC and then parse its contents.
+  //
   class request
   {
   public: