refactor md5 tool with dgst and fix stdin behavior (aws#1766)

Our original md5 stdin behavior was acting weird. The issue seems to be
that we weren't actually parsing all of the input from `stdin`.
```
❯ echo "test" | openssl md5 
(stdin)= d8e8fca2dc0f896fd7cb4cb0031ba249

❯ echo "test" | ./test_build_dir/tool-openssl/openssl md5 
(stdin)= 098f6bcd4621d373cade4e832627b4f6
```
The original implementation used `getline` which only reads one line.
This worked with the original test since the file contents were sent
into `stdin` with `<`, but this doesn't work correctly with other
`stdin` behaviors such as `echo test | openssl md5`.
The correct and more useful way to handle `stdin` is to read it from
[the `0` file
descriptor.](https://en.wikipedia.org/wiki/File_descriptor) This is how
OpenSSL/BoringSSL reads from files.

1. This change fixes the behavior by rebuilding `md5` onto our `dgst`
implementation. This is identical to how OpenSSL factors the specific
hashing tool names. `md5` now uses `dgst` under the hood and inherits
the file hashing behavior we implemented for `dgst hmac`.
2. `stdin` was missing from `dgst`, so that was implemented as well.
`stdin` now works for both `openssl dgst -hmac ...` and `openssl md5`.
Corresponding tests have also been added to verify.
3. Code was structured so that we can easily extend new digest support
for the openssl tool with just a few lines. I considered adding it along
with this PR, but didn't want to muddle it with the new logic.

FIxed version:
```
❯ echo "test_fix" | openssl md5
(stdin)= 1312e69e9dc6ec31b51af24fc42ebdea

❯ echo "test_fix" | ./test_build_dir/tool-openssl/openssl md5
(stdin)= 1312e69e9dc6ec31b51af24fc42ebdea
```

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

.github/workflows/opensslcomparison.yml

@@ -12,16 +12,11 @@ jobs:
     steps:
       - name: Checkout repository
         uses: actions/checkout@v3
-
       - name: Install OS Dependencies
         run: |
           sudo apt-get update -o Acquire::Languages=none -o Acquire::Translation=none
           sudo apt-get -y --no-install-recommends install \
             cmake gcc ninja-build golang make autoconf pkg-config openssl
-
-      - name: Make the script executable
-        run: chmod +x ./tests/ci/run_openssl_comparison_tests.sh
-
       - name: Build AWS-LC & OpenSSL and Run Comparison Tests
         run: |
             ./tests/ci/run_openssl_comparison_tests.sh

tool-openssl/CMakeLists.txt

@@ -6,7 +6,6 @@ add_executable(
     ../tool/fd.cc
 
     dgst.cc
-    md5.cc
     rsa.cc
     tool.cc
     x509.cc
@@ -55,8 +54,6 @@ if(BUILD_TESTING)
 
         dgst.cc
         dgst_test.cc
-        md5.cc
-        md5_test.cc
         rsa.cc
         rsa_test.cc
         x509.cc

tool-openssl/dgst.cc

@@ -16,71 +16,120 @@ static const argument_t kArguments[] = {
      "Create a hashed MAC with the corresponding key"},
     {"", kOptionalArgument, ""}};
 
-static bool dgst_file_op(const std::string &filename, const EVP_MD *digest,
-                         const char *hmac_key, const size_t hmac_key_len) {
-  ScopedFD scoped_fd = OpenFD(filename.c_str(), O_RDONLY | O_BINARY);
-  int fd = scoped_fd.get();
-
+static bool dgst_file_op(const std::string &filename, const int fd,
+                         const EVP_MD *digest) {
   static const size_t kBufSize = 8192;
   std::unique_ptr<uint8_t[]> buf(new uint8_t[kBufSize]);
 
-  if (hmac_key) {
-    bssl::ScopedHMAC_CTX ctx;
-    if (!HMAC_Init_ex(ctx.get(), hmac_key, hmac_key_len, digest, nullptr)) {
-      fprintf(stderr, "Failed to initialize HMAC_Init_ex.\n");
+  bssl::ScopedEVP_MD_CTX ctx;
+  if (!EVP_DigestInit_ex(ctx.get(), digest, nullptr)) {
+    fprintf(stderr, "Failed to initialize EVP_MD_CTX.\n");
+    return false;
+  }
+
+  for (;;) {
+    size_t n;
+    if (!ReadFromFD(fd, &n, buf.get(), kBufSize)) {
+      fprintf(stderr, "Failed to read from %s: %s\n", filename.c_str(),
+              strerror(errno));
       return false;
     }
 
-    // Update |buf| from file continuously.
-    for (;;) {
-      size_t n;
-      if (!ReadFromFD(fd, &n, buf.get(), kBufSize)) {
-        fprintf(stderr, "Failed to read from %s: %s\n", filename.c_str(),
-                strerror(errno));
-        return false;
-      }
-
-      if (n == 0) {
-        break;
-      }
+    if (n == 0) {
+      break;
+    }
 
-      if (!HMAC_Update(ctx.get(), buf.get(), n)) {
-        fprintf(stderr, "Failed to update HMAC.\n");
-        return false;
-      }
+    if (!EVP_DigestUpdate(ctx.get(), buf.get(), n)) {
+      fprintf(stderr, "Failed to update hash.\n");
+      return false;
     }
+  }
+
+  uint8_t hash[EVP_MAX_MD_SIZE];
+  unsigned hash_len;
+  if (!EVP_DigestFinal_ex(ctx.get(), hash, &hash_len)) {
+    fprintf(stderr, "Failed to finish hash.\n");
+    return false;
+  }
+
+  // Print digest output. OpenSSL outputs the digest name with files, but not
+  // with stdin.
+  if (fd != 0) {
+    fprintf(stdout, "%s(%s)= ", EVP_MD_get0_name(digest), filename.c_str());
+  } else {
+    fprintf(stdout, "(%s)= ", filename.c_str());
+  };
+  for (size_t i = 0; i < hash_len; i++) {
+    fprintf(stdout, "%02x", hash[i]);
+  }
+  fprintf(stdout, "\n");
+  return true;
+}
+
+static bool hmac_file_op(const std::string &filename, const int fd,
+                         const EVP_MD *digest, const char *hmac_key,
+                         const size_t hmac_key_len) {
+  static const size_t kBufSize = 8192;
+  std::unique_ptr<uint8_t[]> buf(new uint8_t[kBufSize]);
+
+  bssl::ScopedHMAC_CTX ctx;
+  if (!HMAC_Init_ex(ctx.get(), hmac_key, hmac_key_len, digest, nullptr)) {
+    fprintf(stderr, "Failed to initialize HMAC_Init_ex.\n");
+    return false;
+  }
 
-    const unsigned expected_mac_len = EVP_MD_size(digest);
-    std::unique_ptr<uint8_t[]> mac(new uint8_t[expected_mac_len]);
-    unsigned mac_len;
-    if (!HMAC_Final(ctx.get(), mac.get(), &mac_len)) {
-      fprintf(stderr, "Failed to finalize HMAC.\n");
+  // Update |buf| from file continuously.
+  for (;;) {
+    size_t n;
+    if (!ReadFromFD(fd, &n, buf.get(), kBufSize)) {
+      fprintf(stderr, "Failed to read from %s: %s\n", filename.c_str(),
+              strerror(errno));
       return false;
     }
 
-    // Print HMAC output.
-    fprintf(stdout, "HMAC-%s(%s)= ", EVP_MD_get0_name(digest),
-            filename.c_str());
-    for (size_t i = 0; i < expected_mac_len; i++) {
-      fprintf(stdout, "%02x", mac[i]);
+    if (n == 0) {
+      break;
+    }
+
+    if (!HMAC_Update(ctx.get(), buf.get(), n)) {
+      fprintf(stderr, "Failed to update HMAC.\n");
+      return false;
     }
-    fprintf(stdout, "\n");
-    return true;
   }
 
+  const unsigned expected_mac_len = EVP_MD_size(digest);
+  std::unique_ptr<uint8_t[]> mac(new uint8_t[expected_mac_len]);
+  unsigned mac_len;
+  if (!HMAC_Final(ctx.get(), mac.get(), &mac_len)) {
+    fprintf(stderr, "Failed to finalize HMAC.\n");
+    return false;
+  }
 
-  return false;
+  // Print HMAC output. OpenSSL outputs the digest name with files, but not
+  // with stdin.
+  if (fd != 0) {
+    fprintf(stdout, "HMAC-%s(%s)= ", EVP_MD_get0_name(digest),
+            filename.c_str());
+  } else {
+    fprintf(stdout, "(%s)= ", filename.c_str());
+  };
+  for (size_t i = 0; i < expected_mac_len; i++) {
+    fprintf(stdout, "%02x", mac[i]);
+  }
+  fprintf(stdout, "\n");
+  return true;
 }
 
-// Map arguments using tool/args.cc
-bool dgstTool(const args_list_t &args) {
+static bool dgst_tool_op(const args_list_t &args, const EVP_MD *digest) {
   std::vector<std::string> file_inputs;
 
   // Default is SHA-256.
   // TODO: Make this customizable when "-digest" is introduced.
-  const EVP_MD *digest = EVP_sha256();
+  if (digest == nullptr) {
+    digest = EVP_sha256();
+  }
 
-  // HMAC keys can be empty, but C++ strings have no way to differentiate
+  // HMAC keys can be empty, but C++ std::string has no way to differentiate
   // between null and empty.
   const char *hmac_key = nullptr;
   size_t hmac_key_len = 0;
@@ -114,8 +163,8 @@ bool dgstTool(const args_list_t &args) {
           hmac_key_len = (*it).length();
         } else {
           fprintf(stderr,
-                  "dgst: Option -hmac needs a value\ndgst: Use -help for "
-                  "summary.\n");
+                  "dgst: Option -hmac needs a value\n"
+                  "dgst: Use -help for summary.\n");
           return false;
         }
       } else {
@@ -132,22 +181,40 @@ bool dgstTool(const args_list_t &args) {
     it++;
   }
 
-  if (hmac_key == nullptr) {
-    fprintf(stderr, "Only HMAC is supported as of now\n");
-    return false;
-  }
-
+  // Use stdin if no files are provided.
   if (file_inputs.empty()) {
-    fprintf(stderr, "stdin is not supported as of now\n");
-    return false;
-  };
+    // 0 denotes stdin.
+    std::string file_name = "stdin";
+    int fd = 0;
+    if (hmac_key) {
+      if (!hmac_file_op(file_name, fd, digest, hmac_key, hmac_key_len)) {
+        return false;
+      }
+    } else {
+      if (!dgst_file_op(file_name, fd, digest)) {
+        return false;
+      }
+    }
+    return true;
+  }
 
-  // Do the dgst/hmac operation on all file inputs.
-  for (const auto &file_input : file_inputs) {
-    if (!dgst_file_op(file_input, digest, hmac_key, hmac_key_len)) {
-      return false;
+  // Do the dgst operation on all file inputs.
+  for (const auto &file_name : file_inputs) {
+    ScopedFD scoped_fd = OpenFD(file_name.c_str(), O_RDONLY | O_BINARY);
+    int fd = scoped_fd.get();
+    if (hmac_key) {
+      if (!hmac_file_op(file_name, fd, digest, hmac_key, hmac_key_len)) {
+        return false;
+      }
+    } else {
+      if (!dgst_file_op(file_name, fd, digest)) {
+        return false;
+      }
     }
   }
 
   return true;
 }
+
+bool dgstTool(const args_list_t &args) { return dgst_tool_op(args, nullptr); }
+bool md5Tool(const args_list_t &args) { return dgst_tool_op(args, EVP_md5()); }

tool-openssl/dgst_test.cc

@@ -43,8 +43,17 @@ class DgstComparisonTest : public ::testing::Test {
   std::string openssl_output_str;
 };
 
+// OpenSSL versions 3.1.0 and later change from "(stdin)= " to "MD5(stdin) ="
+std::string GetHash(const std::string &str) {
+  size_t pos = str.find('=');
+  if (pos == std::string::npos) {
+    return "";
+  }
+  return str.substr(pos + 1);
+}
+
 // Test against OpenSSL output for "-hmac"
-TEST_F(DgstComparisonTest, HmacToolCompareOpenSSL) {
+TEST_F(DgstComparisonTest, HMAC_default_files) {
   std::string input_file = std::string(in_path);
   std::ofstream ofs(input_file);
   ofs << "AWS_LC_TEST_STRING_INPUT";
@@ -65,7 +74,7 @@ TEST_F(DgstComparisonTest, HmacToolCompareOpenSSL) {
   std::string awslc_hash = GetHash(awslc_output_str);
   std::string openssl_hash = GetHash(openssl_output_str);
 
-  ASSERT_EQ(awslc_hash, openssl_hash);
+  EXPECT_EQ(awslc_hash, openssl_hash);
 
   // Run -hmac again against multiple files.
   char in_path2[PATH_MAX];
@@ -89,7 +98,7 @@ TEST_F(DgstComparisonTest, HmacToolCompareOpenSSL) {
   awslc_hash = GetHash(awslc_output_str);
   openssl_hash = GetHash(openssl_output_str);
 
-  ASSERT_EQ(awslc_hash, openssl_hash);
+  EXPECT_EQ(awslc_hash, openssl_hash);
 
   // Run -hmac with empty key
   awslc_command = std::string(awslc_executable_path) +
@@ -106,9 +115,86 @@ TEST_F(DgstComparisonTest, HmacToolCompareOpenSSL) {
   awslc_hash = GetHash(awslc_output_str);
   openssl_hash = GetHash(openssl_output_str);
 
-  ASSERT_EQ(awslc_hash, openssl_hash);
-
+  EXPECT_EQ(awslc_hash, openssl_hash);
 
   RemoveFile(input_file.c_str());
   RemoveFile(input_file2.c_str());
 }
+
+
+TEST_F(DgstComparisonTest, HMAC_default_stdin) {
+  std::string tool_command = "echo hmac_this_string | " +
+                             std::string(awslc_executable_path) +
+                             " dgst -hmac key > " + out_path_awslc;
+  std::string openssl_command = "echo hmac_this_string | " +
+                                std::string(openssl_executable_path) +
+                                " dgst -hmac key > " + out_path_openssl;
+
+  RunCommandsAndCompareOutput(tool_command, openssl_command, out_path_awslc,
+                              out_path_openssl, awslc_output_str,
+                              openssl_output_str);
+
+  std::string tool_hash = GetHash(awslc_output_str);
+  std::string openssl_hash = GetHash(openssl_output_str);
+
+  EXPECT_EQ(tool_hash, openssl_hash);
+}
+
+TEST_F(DgstComparisonTest, MD5_files) {
+  std::string input_file = std::string(in_path);
+  std::ofstream ofs(input_file);
+  ofs << "AWS_LC_TEST_STRING_INPUT";
+  ofs.close();
+
+  // Input file as pipe (stdin)
+  std::string tool_command = std::string(awslc_executable_path) + " md5 < " +
+                             input_file + " > " + out_path_awslc;
+  std::string openssl_command = std::string(openssl_executable_path) +
+                                " md5 < " + input_file + " > " +
+                                out_path_openssl;
+
+  RunCommandsAndCompareOutput(tool_command, openssl_command, out_path_awslc,
+                              out_path_openssl, awslc_output_str,
+                              openssl_output_str);
+
+  std::string tool_hash = GetHash(awslc_output_str);
+  std::string openssl_hash = GetHash(openssl_output_str);
+
+  EXPECT_EQ(tool_hash, openssl_hash);
+
+  // Input file as regular command line option.
+  tool_command = std::string(awslc_executable_path) + " md5 " + input_file +
+                 " > " + out_path_awslc;
+  openssl_command = std::string(openssl_executable_path) + " md5 " +
+                    input_file + " > " + out_path_openssl;
+
+  RunCommandsAndCompareOutput(tool_command, openssl_command, out_path_awslc,
+                              out_path_openssl, awslc_output_str,
+                              openssl_output_str);
+
+  tool_hash = GetHash(awslc_output_str);
+  openssl_hash = GetHash(openssl_output_str);
+
+  EXPECT_EQ(tool_hash, openssl_hash);
+
+  RemoveFile(input_file.c_str());
+}
+
+// Test against OpenSSL output with stdin.
+TEST_F(DgstComparisonTest, MD5_stdin) {
+  std::string tool_command = "echo hash_this_string | " +
+                             std::string(awslc_executable_path) + " md5 > " +
+                             out_path_awslc;
+  std::string openssl_command = "echo hash_this_string | " +
+                                std::string(openssl_executable_path) +
+                                " md5 > " + out_path_openssl;
+
+  RunCommandsAndCompareOutput(tool_command, openssl_command, out_path_awslc,
+                              out_path_openssl, awslc_output_str,
+                              openssl_output_str);
+
+  std::string tool_hash = GetHash(awslc_output_str);
+  std::string openssl_hash = GetHash(openssl_output_str);
+
+  EXPECT_EQ(tool_hash, openssl_hash);
+}

tool-openssl/internal.h

@@ -14,6 +14,11 @@
 
 typedef bool (*tool_func_t)(const std::vector<std::string> &args);
 
+struct Tool {
+  const char *name;
+  tool_func_t func;
+};
+
 bool IsNumeric(const std::string& str);
 
 X509* CreateAndSignX509Certificate();