Generate challenge out of database transaction in build task handlerHEADmaster

1 files changed, 71 insertions, 68 deletions

diff --git a/mod/mod-build-task.cxx b/mod/mod-build-task.cxx
index c4968a7..07aff8d 100644
--- a/mod/mod-build-task.cxx
+++ b/mod/mod-build-task.cxx
@@ -44,6 +44,28 @@ using namespace odb::core;
 
 static thread_local mt19937 rand_gen (random_device {} ());
 
+// The challenge (nonce) is randomly generated for every build task if brep is
+// configured to authenticate bbot agents.
+//
+// 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.
+//
+// Note that since generating a challenge is not exactly cheap/fast, we will
+// generate it in advance for every task request, out of the database
+// transaction, and will cache it if it turns out that it wasn't used (no
+// package configuration to (re-)build, etc).
+//
+static thread_local optional<string> challenge;
+
 // Generate a random number in the specified range (max value is included).
 //
 static inline size_t
@@ -617,63 +639,6 @@ handle (request& rq, response& rs)
          : optional<size_t> ()),
         options_->build_hard_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<string> 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,
-                      process_env (options_->openssl (),
-                                   options_->openssl_envvar ()),
-                      "rand",
-                      options_->openssl_option (), 64);
-
-          vector<char> 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<chrono::nanoseconds> (
-                        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 ());
@@ -1373,6 +1338,39 @@ handle (request& rq, response& rs)
           move (tms), move (bms), move (tests)};
       };
 
+      if (agent_fp && !challenge)
+      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,
+                    process_env (options_->openssl (),
+                                 options_->openssl_envvar ()),
+                    "rand",
+                    options_->openssl_option (), 64);
+
+        vector<char> 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<chrono::nanoseconds> (
+                      now.time_since_epoch ()).count ());
+
+        sha256 cs (nonce.data (), nonce.size ());
+        cs.append (&t, sizeof (t));
+        challenge = cs.string ();
+      }
+      catch (const system_error& e)
+      {
+        fail << "unable to generate nonce: " << e;
+      }
+
       // While at it, collect the aborted for various reasons builds
       // (interactive builds in multiple configurations, builds with too many
       // auxiliary machines, etc) to send the notification emails at the end
@@ -1758,7 +1756,6 @@ handle (request& rq, response& rs)
                               toolchain_version);
 
                 shared_ptr<build> b (build_db_->find<build> (bid));
-                optional<string> cl (challenge ());
 
                 // Move the interactive build login information into the build
                 // object, if the package to be built interactively.
@@ -1783,13 +1780,15 @@ handle (request& rq, response& rs)
                                           move (toolchain_version),
                                           move (login),
                                           move (agent_fp),
-                                          move (cl),
+                                          move (challenge),
                                           build_machine {
                                             mh.name, move (mh.summary)},
                                           move (aux->build_auxiliary_machines),
                                           controller_checksum (*cm->config),
                                           machine_checksum (*cm->machine));
 
+                  challenge = nullopt;
+
                   build_db_->persist (b);
                 }
                 else
@@ -1825,7 +1824,10 @@ handle (request& rq, response& rs)
                   }
 
                   b->agent_fingerprint = move (agent_fp);
-                  b->agent_challenge = move (cl);
+
+                  b->agent_challenge = move (challenge);
+                  challenge = nullopt;
+
                   b->machine = build_machine {mh.name, move (mh.summary)};
 
                   // Mark the section as loaded, so auxiliary_machines are
@@ -1994,8 +1996,6 @@ handle (request& rq, response& rs)
 
         sort (rebuilds.begin (), rebuilds.end (), cmp);
 
-        optional<string> cl (challenge ());
-
         // Pick the first build configuration from the ordered list.
         //
         // Note that the configurations and packages may not match the
@@ -2086,10 +2086,10 @@ handle (request& rq, response& rs)
 
                   unforced = (b->force == force_state::unforced);
 
-                  // Can't move from, as may need them on the next iteration.
-                  //
-                  b->agent_fingerprint = agent_fp;
-                  b->agent_challenge = cl;
+                  b->agent_fingerprint = move (agent_fp);
+
+                  b->agent_challenge = move (challenge);
+                  challenge = nullopt;
 
                   const machine_header_manifest& mh (*cm.machine);
                   b->machine = build_machine {mh.name,  mh.summary};
@@ -2182,9 +2182,12 @@ handle (request& rq, response& rs)
           }
           catch (const odb::deadlock&)
           {
-            // Just try with the next rebuild. But first, reset the task
-            // manifest and the session that we may have prepared.
+            // Just try with the next rebuild. But first, restore the agent's
+            // fingerprint and challenge and reset the task manifest and the
+            // session that we may have prepared.
             //
+            agent_fp      = move (b->agent_fingerprint);
+            challenge     = move (b->agent_challenge);
             task_response = task_response_manifest ();
           }