Misc changes to cert_verify_tool for errors

net/tools/cert_verify_tool/cert_verify_tool.cc

Index: net/tools/cert_verify_tool/cert_verify_tool.cc
diff --git a/net/tools/cert_verify_tool/cert_verify_tool.cc b/net/tools/cert_verify_tool/cert_verify_tool.cc
index 210f310733ef172b86059d136f07429d8c4e143e..a2cf94412c919171d5cc1d88b4a429926392b3ee 100644
--- a/net/tools/cert_verify_tool/cert_verify_tool.cc
+++ b/net/tools/cert_verify_tool/cert_verify_tool.cc
@@ -15,26 +15,50 @@
 
 namespace {
 
+const char kUsage[] =
+    " [flags] <target/intermediates>\n"

[eroman, 2016/09/02 21:32:28]

If you don't like these changes I can revert them.

But I think it is fairly readable in terminal output.

[mattm, 2016/09/02 22:39:21]

"target/intermediates" seems a little misleading to me, since the "target" part
is required. It doesn't match the "target+chain" in the description below
either.

[eroman, 2016/09/02 22:53:30]

Did some rewording, what do you think now?

+    "\n"
+    " <target+chain> is a file containing certificates [1], with the target \n"
+    " certificate listed first, and possible intermediates next (in no\n"
+    " particular order).\n"
+    "\n"
+    "Flags:\n"
+    "\n"
+    " --hostname=<hostname>\n"
+    "      The hostname required to match the end-entity certificat. Required\n"
+    "      for the CertVerifyProc implementation.\n"
+    "\n"
+    " --roots=<certs path>\n"
+    "      <certs path> is a file containing certificates [1] to interpret as\n"
+    "      trust anchors (without any anchor constraints).\n"
+    "\n"
+    " --intermediates=<certs path>\n"
+    "      <certs path> is a file containing certificates [1] for use when\n"
+    "      path building is looking for intermediates. These are in addition\n"
+    "      to any intermediates provided by the main certificate file.\n"
+    "\n"
+    " --time=<time>\n"
+    "      Use <time> instead of the current system time. <time> is\n"
+    "      interpreted in local time if a timezone is not specified.\n"
+    "      Many common formats are supported, including:\n"
+    "        1994-11-15 12:45:26 GMT\n"
+    "        Tue, 15 Nov 1994 12:45:26 GMT\n"
+    "        Nov 15 12:45:26 1994 GMT\n"
+    "\n"
+    " --dump=<file prefix>\n"
+    "      Dumps the verified chain to PEM files starting with\n"
+    "      <file prefix>.\n"
+    "\n"
+    "\n"
+    "[1] A \"file containing certificates\" means a path to a file that can\n"
+    "    either be:\n"
+    "    * A binary file containing a single DER-encoded RFC 5280 Certificate\n"
+    "    * A PEM file containing one or more CERTIFICATE blocks (DER-encoded\n"
+    "      RFC 5280 Certificate)\n";
+
 void PrintUsage(const char* argv0) {
-  std::cerr << "Usage: " << argv0 << " [flags] <target/chain>\n";
-  std::cerr << " <target/chain> should be a file containing a single DER cert "
-               "or a PEM certificate chain (target first).\n";
-  std::cerr << "Flags:\n";
-  std::cerr << " --hostname=<hostname>\n";
-  std::cerr << " --roots=<certs path>\n";
-  std::cerr << " --intermediates=<certs path>\n";
-  std::cerr << " <certs path> should be a file containing a single DER cert or "
-               "one or more PEM CERTIFICATE blocks.\n";
-  std::cerr << " --time=<time>\n";
-  std::cerr << " Use <time> instead of the current system time. <time> is "
-               "interpreted in local time if a timezone is not specified.\n";
-  std::cerr << " Many common formats are supported, such as:\n";
-  std::cerr << "  1994-11-15 12:45:26 GMT\n";
-  std::cerr << "  Tue, 15 Nov 1994 12:45:26 GMT\n";
-  std::cerr << "  Nov 15 12:45:26 1994 GMT\n";
-  std::cerr << " --dump=<file prefix>\n";
-  std::cerr << " Dumps the verified chain to PEM files starting with <file "
-               "prefix>.\n";
+  std::cerr << "Usage: " << argv0 << kUsage;
+
   // TODO(mattm): allow <certs path> to be a directory containing DER/PEM files?
   // TODO(mattm): allow target to specify an HTTPS URL to check the cert of?
   // TODO(mattm): allow target to be a verify_certificate_chain_unittest PEM
@@ -62,10 +86,6 @@ int main(int argc, char** argv) {
   }
 
   std::string hostname = command_line.GetSwitchValueASCII("hostname");
-  if (hostname.empty()) {
-    std::cerr << "ERROR: --hostname is required\n";

[eroman, 2016/09/02 21:32:28]

Mostly just a convenience, since I find myself mostly running the
CertPathBuilder flavor.

-    return 1;
-  }
 
   base::Time verify_time;
   std::string time_flag = command_line.GetSwitchValueASCII("time");
@@ -93,7 +113,12 @@ int main(int argc, char** argv) {
     ReadCertificatesFromFile(roots_path, &root_der_certs);
   if (!intermediates_path.empty())
     ReadCertificatesFromFile(intermediates_path, &intermediate_der_certs);
-  ReadChainFromFile(target_path, &target_der_cert, &intermediate_der_certs);
+
+  if (!ReadChainFromFile(target_path, &target_der_cert,
+                         &intermediate_der_certs)) {
+    std::cerr << "ERROR: Couldn't read certifcate chain\n";

[mattm, 2016/09/02 22:39:21]

certifcate

[eroman, 2016/09/02 22:53:30]

Done.

+    return 1;
+  }
 
   if (target_der_cert.der_cert.empty()) {
     std::cerr << "ERROR: no target cert\n";
@@ -105,6 +130,8 @@ int main(int argc, char** argv) {
   if (!time_flag.empty()) {
     std::cerr << "ERROR: --time is not supported with CertVerifyProc, "
                  "skipping.\n";
+  } else if (hostname.empty()) {
+    std::cerr << "ERROR: --hostname is required for CertVerifyProc, skipping\n";
   } else {
     cert_verify_proc_ok = VerifyUsingCertVerifyProc(
         target_der_cert, hostname, intermediate_der_certs, root_der_certs,
@@ -112,6 +139,20 @@ int main(int argc, char** argv) {
   }
 
   std::cout << "\nCertPathBuilder:\n";
+
+  if (hostname.empty()) {

[mattm, 2016/09/02 22:39:21]

should be !empty  ?

[eroman, 2016/09/02 22:53:30]

Done.

+    std::cerr
+        << "WARNING: --hostname is not yet verified with CertPathBuilder\n";
+  }
+
+  if (root_der_certs.empty()) {
+    std::cerr << "ERROR: --roots is required for CertPathBuilder to succeed "
+                 "(as it doesn't use the OS trust store).\n";
+  } else {
+    std::cout << "NOTE: CertPathBuilder does not currently use OS trust "
+                 "settings (only --roots will be used)\n";
+  }

[mattm, 2016/09/02 22:39:21]

I think it's better to leave the trust store warnings inside
VerifyUsingPathBuilder. Especially since it gets more complicated with the NSS
trust store CL.

[eroman, 2016/09/02 22:53:30]

Done.

+
   bool path_builder_ok =
       VerifyUsingPathBuilder(target_der_cert, intermediate_der_certs,
                              root_der_certs, verify_time, dump_prefix_path);