Make use of butl::openssl, ifdstream::read_text() and ifdstream::read_binary()

8 files changed, 257 insertions, 327 deletions

diff --git a/bpkg/archive.cxx b/bpkg/archive.cxx
index ce62fab..f3c41f8 100644
--- a/bpkg/archive.cxx
+++ b/bpkg/archive.cxx
@@ -144,8 +144,7 @@ namespace bpkg
       //
       ifdstream is (move (pr.second.in_ofd), ifdstream::badbit);
 
-      string s;
-      getline (is, s, '\0');
+      string s (is.read_text ());
       is.close ();
 
       if (pr.second.wait () && pr.first.wait ())
diff --git a/bpkg/auth.cxx b/bpkg/auth.cxx
index ceacabe..4faf9ed 100644
--- a/bpkg/auth.cxx
+++ b/bpkg/auth.cxx
@@ -7,15 +7,15 @@
 #include <ratio>
 #include <limits>    // numeric_limits
 #include <cstring>   // strlen(), strcmp()
-#include <iterator>  // ostreambuf_iterator, istreambuf_iterator
+#include <iterator>  // ostreambuf_iterator
 
 #include <libbutl/sha256.hxx>
 #include <libbutl/base64.hxx>
 #include <libbutl/process.hxx>
+#include <libbutl/openssl.hxx>
 #include <libbutl/fdstream.hxx>
 #include <libbutl/filesystem.hxx>
 
-#include <bpkg/openssl.hxx>
 #include <bpkg/package.hxx>
 #include <bpkg/package-odb.hxx>
 #include <bpkg/database.hxx>
@@ -26,6 +26,15 @@ using namespace butl;
 
 namespace bpkg
 {
+  // Print process command line.
+  //
+  static void
+  print_command (const char* const args[], size_t n)
+  {
+    if (verb >= 2)
+      print_process (args, n);
+  }
+
   // Find the repository location prefix that ends with the version component.
   // We consider all repositories under this location to be related.
   //
@@ -112,60 +121,58 @@ namespace bpkg
   {
     tracer trace ("real_fingerprint");
 
-    try
+    auto calc_failed = [&rl] (const exception* e = nullptr)
     {
-      process pr (start_openssl (
-        co, "x509", {"-sha256", "-noout", "-fingerprint"}, true, true));
+      diag_record dr (error);
+      dr << "unable to calculate certificate fingerprint for "
+         << rl.canonical_name ();
 
-      try
-      {
-        ifdstream is (move (pr.in_ofd), fdstream_mode::skip);
-        ofdstream os (move (pr.out_fd));
-        os << pem;
-        os.close ();
+      if (e != nullptr)
+        dr << ": " << *e;
+    };
 
-        string s;
-        getline (is, s);
-        is.close ();
+    try
+    {
+      openssl os (print_command,
+                  fdstream_mode::text, fdstream_mode::text, 2,
+                  co.openssl (), "x509",
+                  co.openssl_option (), "-sha256", "-noout", "-fingerprint");
 
-        const size_t n (19);
-        if (!(s.size () > n && s.compare (0, n, "SHA256 Fingerprint=") == 0))
-          throw io_error ("");
+      os.out << pem;
+      os.out.close ();
 
-        string fp;
+      string s;
+      getline (os.in, s);
+      os.in.close ();
 
-        try
-        {
-          fp = fingerprint_to_sha256 (string (s, n));
-        }
-        catch (const invalid_argument&)
-        {
-          throw io_error ("");
-        }
-
-        if (pr.wait ())
-          return fp;
+      try
+      {
+        const size_t n (19);
 
-        // Fall through.
-        //
+        if (os.wait () &&
+            s.size () > n && s.compare (0, n, "SHA256 Fingerprint=") == 0)
+          return fingerprint_to_sha256 (string (s, n));
       }
-      catch (const io_error&)
+      catch (const invalid_argument&)
       {
-        // Child exit status doesn't matter. Just wait for the process
-        // completion and fall through.
-        //
-        pr.wait (); // Check throw.
       }
 
-      error << "unable to calculate certificate fingerprint for "
-            << rl.canonical_name ();
+      calc_failed ();
+
+      // Fall through.
+    }
+    // For old versions of g++ (as of 4.9) ios_base::failure is not derived
+    // from system_error.
+    //
+    catch (const io_error& e)
+    {
+      calc_failed (&e);
 
       // Fall through.
     }
-    catch (const process_error& e)
+    catch (const system_error& e)
     {
-      error << "unable to calculate certificate fingerprint for "
-            << rl.canonical_name () << ": " << e;
+      calc_failed (&e);
 
       // Fall through.
     }
@@ -196,6 +203,15 @@ namespace bpkg
   {
     tracer trace ("parse_cert");
 
+    auto parse_failed = [&repo] (const exception* e = nullptr)
+    {
+      diag_record dr (error);
+      dr << "unable to parse certificate for " << repo;
+
+      if (e != nullptr)
+        dr << ": " << *e;
+    };
+
     try
     {
       // The order of the options we pass to openssl determines the order in
@@ -217,76 +233,72 @@ namespace bpkg
       // The final line should be the email but will be silently missing if
       // the cert has no email.
       //
-      process pr (start_openssl (
-        co,
-        "x509",
-        {
-          "-noout",
-          "-subject",
-          "-dates",
-          "-email",
-
-          // Previously we have used "RFC2253,sep_multiline" format to display
-          // the requested fields, but that resulted in some undesirable
-          // behavior like escaping commas (\,) while dispaying only one field
-          // per line. The reason for that is RFC2253 specifier which get
-          // expanded into:
-          //
-          // esc_2253,esc_ctrl,esc_msb,utf8,dump_nostr,dump_unknown,dump_der,
-          // sep_comma_plus,dn_rev,sname.
-          //
-          // Now we filtered them and leave just those specifiers that we
-          // really need:
-          //
-          // utf8          - use UTF8 encoding for strings;
-          //
-          // esc_ctrl      - display control characters in \XX notation (we
-          //                 don't expect them in properly created
-          //                 certificates, but it's better to print this way if
-          //                 they appear);
-          //
-          // sname         - use short form for field names (like
-          //                 "O=Code Synthesis" vs
-          //                 "organizationName=Code Synthesis");
-          //
-          // dump_nostr    - do not print any binary data in the binary form;
-          // dump_der
-          //
-          // sep_multiline - display field per line.
-          //
-          "-nameopt", "utf8,esc_ctrl,dump_nostr,dump_der,sname,sep_multiline"
-        },
-        true,
-        true));
-
-      try
-      {
-        // We unset failbit to provide the detailed error description (which
-        // certificate field is missed) on failure.
+      openssl os (
+        print_command,
+        fdstream_mode::text, fdstream_mode::text, 2,
+        co.openssl (), "x509",
+        co.openssl_option (),
+        "-noout",
+        "-subject",
+        "-dates",
+        "-email",
+
+        // Previously we have used "RFC2253,sep_multiline" format to display
+        // the requested fields, but that resulted in some undesirable
+        // behavior like escaping commas (\,) while dispaying only one field
+        // per line. The reason for that is RFC2253 specifier which get
+        // expanded into:
         //
-        ifdstream is (
-          move (pr.in_ofd), fdstream_mode::skip, ifdstream::badbit);
-
-        ofdstream os (move (pr.out_fd));
-
-        // Reading from and writing to the child process standard streams from
-        // the same thread is generally a bad idea. Depending on the program
-        // implementation we can block on writing if the process input pipe
-        // buffer get filled. That can happen if the process do not read
-        // anymore, being blocked on writing to the filled output pipe, which
-        // get filled not being read on the other end.
+        // esc_2253,esc_ctrl,esc_msb,utf8,dump_nostr,dump_unknown,dump_der,
+        // sep_comma_plus,dn_rev,sname.
+        //
+        // Now we filtered them and leave just those specifiers that we
+        // really need:
+        //
+        // utf8          - use UTF8 encoding for strings;
+        //
+        // esc_ctrl      - display control characters in \XX notation (we
+        //                 don't expect them in properly created
+        //                 certificates, but it's better to print this way if
+        //                 they appear);
+        //
+        // sname         - use short form for field names (like
+        //                 "O=Code Synthesis" vs
+        //                 "organizationName=Code Synthesis");
         //
-        // Fortunatelly openssl reads the certificate before performing any
-        // output.
+        // dump_nostr    - do not print any binary data in the binary form;
+        // dump_der
         //
-        os << pem;
-        os.close ();
+        // sep_multiline - display field per line.
+        //
+        "-nameopt", "utf8,esc_ctrl,dump_nostr,dump_der,sname,sep_multiline"
+      );
 
+      // We unset failbit to provide the detailed error description (which
+      // certificate field is missed) on failure.
+      //
+      os.in.exceptions (ifdstream::badbit);
+
+      // Reading from and writing to the child process standard streams from
+      // the same thread is generally a bad idea. Depending on the program
+      // implementation we can block on writing if the process input pipe
+      // buffer get filled. That can happen if the process do not read
+      // anymore, being blocked on writing to the filled output pipe, which
+      // get filled not being read on the other end.
+      //
+      // Fortunatelly openssl reads the certificate before performing any
+      // output.
+      //
+      os.out << pem;
+      os.out.close ();
+
+      try
+      {
         auto bad_cert ([](const string& d) {throw invalid_argument (d);});
 
-        auto get = [&is, &trace] (string& s) -> bool
+        auto get = [&os, &trace] (string& s) -> bool
         {
-          bool r (getline (is, s));
+          bool r (getline (os.in, s));
           l6 ([&]{trace << s;});
           return r;
         };
@@ -307,41 +319,41 @@ namespace bpkg
         };
 
         auto parse_date = [&s](size_t o, const char* name) -> timestamp
+        {
+          // Certificate validity dates are internally represented as ASN.1
+          // GeneralizedTime and UTCTime
+          // (https://www.ietf.org/rfc/rfc4517.txt). While GeneralizedTime
+          // format allows fraction of a second to be specified, the x.509
+          // Certificate specification (https://www.ietf.org/rfc/rfc5280.txt)
+          // do not permit them to be included into the validity dates. These
+          // dates are printed by openssl in the 'MON DD HH:MM:SS[ GMT]'
+          // format. MON is a month abbreviated name (C locale), timezone is
+          // either GMT or absent (means local time). Examples:
+          //
+          // Apr 11 10:20:02 2016 GMT
+          // Apr 11 10:20:02 2016
+          //
+          // We will require the date to be in GMT, as generally can not
+          // interpret the certificate origin local time. Note:
+          // openssl-generated certificate dates are always in GMT.
+          //
+          try
           {
-            // Certificate validity dates are internally represented as ASN.1
-            // GeneralizedTime and UTCTime
-            // (https://www.ietf.org/rfc/rfc4517.txt). While GeneralizedTime
-            // format allows fraction of a second to be specified, the x.509
-            // Certificate specification (https://www.ietf.org/rfc/rfc5280.txt)
-            // do not permit them to be included into the validity dates. These
-            // dates are printed by openssl in the 'MON DD HH:MM:SS[ GMT]'
-            // format. MON is a month abbreviated name (C locale), timezone is
-            // either GMT or absent (means local time). Examples:
+            // Assume the global locale is not changed, and still "C".
             //
-            // Apr 11 10:20:02 2016 GMT
-            // Apr 11 10:20:02 2016
-            //
-            // We will require the date to be in GMT, as generally can not
-            // interpret the certificate origin local time. Note:
-            // openssl-generated certificate dates are always in GMT.
-            //
-            try
-            {
-              // Assume the global locale is not changed, and still "C".
-              //
-              const char* end;
-              timestamp t (from_string (
-                s.c_str () + o, "%b %d %H:%M:%S %Y", false, &end));
-
-              if (strcmp (end, " GMT") == 0)
-                return t;
-            }
-            catch (const system_error&)
-            {
-            }
-
-            throw invalid_argument ("invalid " + string (name) + " date");
-          };
+            const char* end;
+            timestamp t (from_string (
+                           s.c_str () + o, "%b %d %H:%M:%S %Y", false, &end));
+
+            if (strcmp (end, " GMT") == 0)
+              return t;
+          }
+          catch (const system_error&)
+          {
+          }
+
+          throw invalid_argument ("invalid " + string (name) + " date");
+        };
 
         string name;
         string org;
@@ -368,7 +380,7 @@ namespace bpkg
         if (org.empty ())
           bad_cert ("no organization name (O)");
 
-        if (!is || s.compare (0, 10, "notBefore=") != 0)
+        if (!os.in || s.compare (0, 10, "notBefore=") != 0)
           bad_cert ("no start date");
 
         timestamp not_before (parse_date (10, "start"));
@@ -387,10 +399,10 @@ namespace bpkg
 
         // Ensure no data left in the stream.
         //
-        if (is.peek () != ifdstream::traits_type::eof ())
+        if (os.in.peek () != ifdstream::traits_type::eof ())
           bad_cert ("unexpected data");
 
-        is.close ();
+        os.in.close ();
 
         shared_ptr<certificate> cert (
           make_shared<certificate> (
@@ -401,37 +413,39 @@ namespace bpkg
             move (not_before),
             move (not_after)));
 
-        if (pr.wait ())
+        if (os.wait ())
           return cert;
 
         // Fall through.
         //
       }
-      catch (const io_error&)
-      {
-        // Child exit status doesn't matter. Just wait for the process
-        // completion and fall through.
-        //
-        pr.wait (); // Check throw.
-      }
       catch (const invalid_argument& e)
       {
         // If the child exited with an error status, then omit any output
         // parsing diagnostics since we were probably parsing garbage.
         //
-        if (pr.wait ())
+        if (os.wait ())
           fail << "invalid certificate for " << repo << ": " << e << endf;
 
         // Fall through.
       }
 
-      error << "unable to parse certificate for " << repo;
+      parse_failed ();
 
       // Fall through.
     }
-    catch (const process_error& e)
+    // For old versions of g++ (as of 4.9) ios_base::failure is not derived
+    // from system_error.
+    //
+    catch (const io_error& e)
     {
-      error << "unable to parse certificate for " << repo << ": " << e;
+      parse_failed (&e);
+
+      // Fall through.
+    }
+    catch (const system_error& e)
+    {
+      parse_failed (&e);
 
       // Fall through.
     }
@@ -749,66 +763,58 @@ namespace bpkg
            << rl.canonical_name () <<
         info << "certificate name is " << cert.name;
 
-    try
+    auto auth_failed = [&rl] (const exception* e = nullptr)
     {
-      process pr (start_openssl (
-        co, "rsautl",
-        {
-          "-verify",
-          "-certin",
-          "-inkey",
-          f.string ().c_str ()
-        },
-        true,
-        true));
-
-      try
-      {
-        ifdstream is (move (pr.in_ofd), fdstream_mode::skip);
+      diag_record dr (error);
+      dr << "unable to authenticate repository " << rl.canonical_name ();
 
-        // Write the signature to the openssl process input in the binary mode.
-        //
-        ofdstream os (move (pr.out_fd), fdstream_mode::binary);
-
-        for (const auto& c: sm.signature)
-          os.put (c); // Sets badbit on failure.
+      if (e != nullptr)
+        dr << ": " << *e;
+    };
 
-        os.close ();
+    try
+    {
+      openssl os (print_command,
+                  path ("-"), fdstream_mode::text, 2,
+                  co.openssl (), "rsautl",
+                  co.openssl_option (), "-verify", "-certin", "-inkey", f);
 
-        string s;
-        getline (is, s);
+      for (const auto& c: sm.signature)
+        os.out.put (c); // Sets badbit on failure.
 
-        bool v (is.eof ());
-        is.close ();
+      os.out.close ();
 
-        if (pr.wait () && v)
-        {
-          if (s != sm.sha256sum)
-            fail << "packages manifest file signature mismatch for "
-                 << rl.canonical_name ();
+      string s;
+      getline (os.in, s);
 
-          return; // All good.
-        }
+      bool v (os.in.eof ());
+      os.in.close ();
 
-        // Fall through.
-        //
-      }
-      catch (const io_error&)
+      if (os.wait () && v)
       {
-        // Child exit status doesn't matter. Just wait for the process
-        // completion and fall through.
-        //
-        pr.wait (); // Check throw.
+        if (s != sm.sha256sum)
+          fail << "packages manifest file signature mismatch for "
+               << rl.canonical_name ();
+
+        return; // All good.
       }
 
-      error << "unable to authenticate repository " << rl.canonical_name ();
+      auth_failed ();
 
       // Fall through.
     }
-    catch (const process_error& e)
+    // For old versions of g++ (as of 4.9) ios_base::failure is not derived
+    // from system_error.
+    //
+    catch (const io_error& e)
     {
-      error << "unable to authenticate repository "
-            << rl.canonical_name () << ": " << e;
+      auth_failed (&e);
+
+      // Fall through.
+    }
+    catch (const system_error& e)
+    {
+      auth_failed (&e);
 
       // Fall through.
     }
@@ -844,52 +850,47 @@ namespace bpkg
       warn << "certificate for repository " << r
            << " expires in less than " << left.count () + 1 << " day(s)";
 
-    try
+    auto sign_failed = [&r] (const exception* e = nullptr)
     {
-      process pr (start_openssl (
-        co, "rsautl", {"-sign", "-inkey", key_name.c_str ()}, true, true));
+      diag_record dr (error);
+      dr << "unable to sign repository " << r;
 
-      try
-      {
-        // Read the signature from the openssl process output in the binary
-        // mode.
-        //
-        ifdstream is (
-          move (pr.in_ofd), fdstream_mode::binary | fdstream_mode::skip);
+      if (e != nullptr)
+        dr << ": " << *e;
+    };
 
-        ofdstream os (move (pr.out_fd));
-        os << sha256sum;
-        os.close ();
+    try
+    {
+      openssl os (print_command,
+                  fdstream_mode::text, path ("-"), 2,
+                  co.openssl (), "rsautl",
+                  co.openssl_option (), "-sign", "-inkey", key_name);
 
-        // Additional parentheses required to make compiler to distinguish
-        // the variable definition from a function declaration.
-        //
-        vector<char> signature
-          ((istreambuf_iterator<char> (is)), istreambuf_iterator<char> ());
+      os.out << sha256sum;
+      os.out.close ();
 
-        is.close ();
+      vector<char> signature (os.in.read_binary ());
+      os.in.close ();
 
-        if (pr.wait ())
-          return signature;
+      if (os.wait ())
+        return signature;
 
-        // Fall through.
-        //
-      }
-      catch (const io_error&)
-      {
-        // Child exit status doesn't matter. Just wait for the process
-        // completion and fall through.
-        //
-        pr.wait (); // Check throw.
-      }
+      sign_failed ();
 
-      error << "unable to sign repository " << r;
+      // Fall through.
+    }
+    // For old versions of g++ (as of 4.9) ios_base::failure is not derived
+    // from system_error.
+    //
+    catch (const io_error& e)
+    {
+      sign_failed (&e);
 
       // Fall through.
     }
-    catch (const process_error& e)
+    catch (const system_error& e)
     {
-      error << "unable to sign repository " << r << ": " << e;
+      sign_failed (&e);
 
       // Fall through.
     }
diff --git a/bpkg/buildfile b/bpkg/buildfile
index e555178..e214bc9 100644
--- a/bpkg/buildfile
+++ b/bpkg/buildfile
@@ -21,7 +21,6 @@ exe{bpkg}:                                                                    \
 {hxx            }{ forward           }                                        \
 {hxx         cxx}{ help              } {hxx ixx cxx}{ help-options          } \
 {hxx         cxx}{ manifest-utility  }                                        \
-{hxx         cxx}{ openssl           }                                        \
 {hxx            }{ options-types     }                                        \
 {hxx ixx     cxx}{ package           }                                        \
 {hxx ixx     cxx}{ package-odb       }          file{ package.xml           } \
diff --git a/bpkg/diagnostics.cxx b/bpkg/diagnostics.cxx
index 16dcc46..7fafbb9 100644
--- a/bpkg/diagnostics.cxx
+++ b/bpkg/diagnostics.cxx
@@ -96,6 +96,18 @@ namespace bpkg
       static_cast<trace_mark&> (*this) << "DEALLOCATE " << s.text ();
   }
 
+  // tracer
+  //
+  void tracer::
+  operator() (const char* const args[], size_t n) const
+  {
+    if (verb >= 3)
+    {
+      diag_record dr (*this);
+      process::print (dr.os, args, n);
+    }
+  }
+
   const basic_mark error ("error");
   const basic_mark warn  ("warning");
   const basic_mark info  ("info");
diff --git a/bpkg/diagnostics.hxx b/bpkg/diagnostics.hxx
index 8edf606..ecff515 100644
--- a/bpkg/diagnostics.hxx
+++ b/bpkg/diagnostics.hxx
@@ -198,7 +198,17 @@ namespace bpkg
     deallocate (odb::connection&, const odb::statement&);
   };
   using trace_mark = butl::diag_mark<trace_mark_base>;
-  using tracer = trace_mark;
+
+  class tracer: public trace_mark
+  {
+  public:
+    using trace_mark::trace_mark;
+
+    // process_run() command tracer interface.
+    //
+    void
+    operator() (const char* const [], std::size_t) const;
+  };
 
   // fail
   //
diff --git a/bpkg/openssl.cxx b/bpkg/openssl.cxx
deleted file mode 100644
index 5a1e29c..0000000
--- a/bpkg/openssl.cxx
+++ /dev/null
@@ -1,60 +0,0 @@
-// file      : bpkg/openssl.cxx -*- C++ -*-
-// copyright : Copyright (c) 2014-2017 Code Synthesis Ltd
-// license   : MIT; see accompanying LICENSE file
-
-#include <bpkg/openssl.hxx>
-
-#include <libbutl/process.hxx>
-
-#include <bpkg/diagnostics.hxx>
-
-using namespace std;
-using namespace butl;
-
-namespace bpkg
-{
-  process
-  start_openssl (const common_options& co,
-                 const char* command,
-                 const cstrings& options,
-                 bool in,
-                 bool out,
-                 bool err)
-  {
-    cstrings args {co.openssl ().string ().c_str (), command};
-
-    // Add extra options. Normally the order of options is not important
-    // (unless they override each other). However, openssl 1.0.1 seems to have
-    // bugs in that department (that were apparently fixed in 1.0.2). To work
-    // around these bugs we pass user-supplied options first.
-    //
-    for (const string& o: co.openssl_option ())
-      args.push_back (o.c_str ());
-
-    args.insert (args.end (), options.begin (), options.end ());
-    args.push_back (nullptr);
-
-    try
-    {
-      process_path pp (process::path_search (args[0]));
-
-      if (verb >= 2)
-        print_process (args);
-
-      // If the caller is interested in reading STDOUT and STDERR, then
-      // redirect STDERR to STDOUT, so both can be read from the same stream.
-      //
-      return process (
-        pp, args.data (), in ? -1 : 0, out ? -1 : 1, err ? (out ? 1 : -1): 2);
-    }
-    catch (const process_error& e)
-    {
-      error << "unable to execute " << args[0] << ": " << e;
-
-      if (e.child)
-        exit (1);
-
-      throw failed ();
-    }
-  }
-}
diff --git a/bpkg/openssl.hxx b/bpkg/openssl.hxx
deleted file mode 100644
index 8023db6..0000000
--- a/bpkg/openssl.hxx
+++ /dev/null
@@ -1,31 +0,0 @@
-// file      : bpkg/openssl.hxx -*- C++ -*-
-// copyright : Copyright (c) 2014-2017 Code Synthesis Ltd
-// license   : MIT; see accompanying LICENSE file
-
-#ifndef BPKG_OPENSSL_HXX
-#define BPKG_OPENSSL_HXX
-
-#include <libbutl/process.hxx>
-
-#include <bpkg/types.hxx>
-#include <bpkg/utility.hxx>
-
-#include <bpkg/common-options.hxx>
-
-namespace bpkg
-{
-  // Start the openssl process. Parameters in, out, err flags if the caller
-  // wish to write to, or read from the process STDIN, STDOUT, STDERR streams.
-  // If out and err are both true, then STDERR is redirected to STDOUT, and
-  // they both can be read from in_ofd descriptor.
-  //
-  butl::process
-  start_openssl (const common_options&,
-                 const char* command,
-                 const cstrings& options,
-                 bool in = false,
-                 bool out = false,
-                 bool err = false);
-}
-
-#endif // BPKG_OPENSSL_HXX
diff --git a/tests/rep-auth.test b/tests/rep-auth.test
index 12815f9..c67bd75 100644
--- a/tests/rep-auth.test
+++ b/tests/rep-auth.test
@@ -582,7 +582,7 @@ sc = " " # Space character to append to here-document line when required.
   :
   $rep_info $rep/signature-mismatch 2>>~%EOE% != 0
     %.*
-    %error: unable to authenticate repository build2.org/rep-auth/signature-mismatch%
+    %error: unable to authenticate repository build2.org/rep-auth/signature-mismatch: .*%
     EOE
 
   : create-rep