Review

3 files changed, 54 insertions, 31 deletions

diff --git a/mod/jwt.cxx b/mod/jwt.cxx
index 11b76ec..43b8629 100644
--- a/mod/jwt.cxx
+++ b/mod/jwt.cxx
@@ -52,18 +52,18 @@ using namespace butl;
 //
 //   base64url($header) + '.' + base64url($payload) + '.' + base64url($signature)
 //
-string
-brep::gen_jwt (const options::openssl_options& o,
-               const path& pk,
-               const string& iss,
-               const std::chrono::minutes& vp)
+string brep::
+gen_jwt (const options::openssl_options& o,
+         const path& pk,
+         const string& iss,
+         const std::chrono::minutes& vp)
 {
   // Create the header.
   //
   string h; // Header (base64url-encoded).
   {
     vector<char> b;
-    json::buffer_serializer s (b);
+    json::buffer_serializer s (b, 0 /* indentation */);
 
     s.begin_object ();
     s.member ("typ", "JWT");
@@ -81,11 +81,6 @@ brep::gen_jwt (const options::openssl_options& o,
 
     // "Issued at" time.
     //
-    // @@ TODO GitHub recommends setting this time to 60 seconds in the past
-    //         to combat clock drift. Seems likely to be a general problem
-    //         with client/server authentication schemes so perhaps passing
-    //         the expected drift/skew as an argument might make sense?
-    //
     seconds iat (
         duration_cast<seconds> (system_clock::now ().time_since_epoch ()));
 
@@ -109,9 +104,10 @@ brep::gen_jwt (const options::openssl_options& o,
   //
 
   // The signature (base64url-encoded). Will be left empty if openssl exits
-  // with a non-zero status.
+  // with a non-zero status. @@
   //
   string s;
+  try
   {
     // Sign the concatenated header and payload using openssl.
     //
@@ -119,27 +115,49 @@ brep::gen_jwt (const options::openssl_options& o,
     //
     // Note that RSA is indicated by the contents of the private key.
     //
+    // Note that here we assume both output and diagnostics will fit into pipe
+    // buffers and don't both with fdselect().
+    //
     openssl os (path ("-"), // Read message from openssl::out.
                 path ("-"), // Write output to openssl::in.
                 2,          // Diagnostics to stderr.
                 process_env (o.openssl (), o.openssl_envvar ()),
                 "dgst", o.openssl_option (), "-sha256", "-sign", pk);
 
-    // Write the concatenated header and payload to openssl's input.
-    //
-    os.out << h << '.' << p;
-    os.out.close ();
+    // @@ TODO redirect stderr to pipe/ofdstream.
 
-    // Read the binary signature from openssl's output.
-    //
-    vector<char> bs (os.in.read_binary ());
-    os.in.close ();
+    try
+    {
+      // Write the concatenated header and payload to openssl's input.
+      //
+      os.out << h << '.' << p;
+      os.out.close ();
 
-    if (os.wait ())
-      s = base64url_encode (bs);
+      // Read the binary signature from openssl's output.
+      //
+      vector<char> bs (os.in.read_binary ());
+      os.in.close ();
+    }
+    catch (const io_error&)
+    {
+      if (!os.wait ())
+        throw system_error (); // @@
+
+      // Fall through.
+    }
+
+    if (!os.wait ())
+      throw system_error (); // @@
+
+    s = base64url_encode (bs);
+  }
+  catch ()
+  {
+    // @@ TODO: catch all possible errors and translate to suitable
+    //    system_error.
   }
 
-  // Return the token, or empty if openssl exited with a non-zero status.
+  // Return the token, or empty if openssl exited with a non-zero status. @@
   //
   return !s.empty ()
          ? h + '.' + p + '.' + s
diff --git a/mod/jwt.hxx b/mod/jwt.hxx
index 65ad5c5..25e9c21 100644
--- a/mod/jwt.hxx
+++ b/mod/jwt.hxx
@@ -10,7 +10,7 @@
 
 namespace brep
 {
-  // Generate a JSON Web Token (JWT), defined in RFC 7519.
+  // Generate a JSON Web Token (JWT), defined in RFC7519.
   //
   // A JWT is essentially the token issuer's name along with a number of
   // claims, signed with a private key.
@@ -20,16 +20,18 @@ namespace brep
   //
   // The token expires when the validity period has elapsed.
   //
-  // Return the token or empty if openssl exited with a non-zero status.
+  // The backdate argument specifies the number of seconds to subtract from
+  // the "issued at" time in order to combat potential clock drift (which can
+  // casue the token to be not valid yet).
   //
-  // Throw process_error or io_error (both derived from std::system_error) if
-  // openssl could not be executed or communication with its process failed.
+  // Return the token or std::system_error in case if an error.
   //
   string
   gen_jwt (const options::openssl_options&,
            const path& private_key,
            const string& issuer,
-           const std::chrono::minutes& validity_period);
+           const std::chrono::minutes& validity_period,
+           const std::chrono::seconds& backdate = 60);
 }
 
 #endif
diff --git a/mod/mod-ci-github.cxx b/mod/mod-ci-github.cxx
index a82ce43..86f52b9 100644
--- a/mod/mod-ci-github.cxx
+++ b/mod/mod-ci-github.cxx
@@ -114,7 +114,8 @@ struct repository
   string full_name;
   string default_branch;
 
-  explicit repository (json::parser&);
+  explicit
+  repository (json::parser&);
 
   repository () = default;
 };
@@ -125,7 +126,8 @@ struct check_suite_event
   ::check_suite check_suite;
   ::repository  repository;
 
-  explicit check_suite_event (json::parser&);
+  explicit
+  check_suite_event (json::parser&);
 
   check_suite_event () = default;
 };
@@ -234,6 +236,7 @@ handle (request& rq, response& rs)
       try
       {
         // Use the maximum validity period allowed by GitHub (10 minutes).
+        // @@ Let's make configurable.
         //
         string jwt (gen_jwt (*options_,
                              options_->ci_github_app_private_key (),
@@ -270,7 +273,7 @@ handle (request& rq, response& rs)
     // @@ TODO: should we write more detailed diagnostics to log? Maybe we
     //    should do this for all unsuccessful calls to respond().
     //
-    //    @@ TMP These exceptions end up in the apache error log.
+    // Note: these exceptions end up in the apache error log.
     //
     throw invalid_request (400, "malformed JSON in request body");
   }