Update x509 tool to write all output to common BIO which is a file or…

… stdout (#1800)

### Description of changes: 
Previously only some output followed the user's request using `-out` to
direct the output to a file. This change updates everything to use a
common `BIO` which is `stdout` by default or the user provided file.

### Testing:
Added new tests that test writing to a file directly with `-out` we have
existing tests that cover writing to `stdout` (and then getting
redirected to a file with `>`) works as expected.

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

@@ -16,7 +16,9 @@ jobs:
         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
+            cmake clang ninja-build golang make autoconf pkg-config openssl
+          echo "CC=clang" >> $GITHUB_ENV
+          echo "CXX=clang++" >> $GITHUB_ENV
       - name: Build AWS-LC & OpenSSL and Run Comparison Tests
         run: |
             ./tests/ci/run_openssl_comparison_tests.sh

tests/ci/run_openssl_comparison_tests.sh

@@ -10,25 +10,22 @@ scratch_folder=${SYS_ROOT}/"openssl-scratch"
 install_dir="${scratch_folder}/libcrypto_install_dir"
 openssl_url='https://github.com/openssl/openssl.git'
 openssl_1_1_1_branch='OpenSSL_1_1_1-stable'
-openssl_1_0_2_branch='OpenSSL_1_0_2-stable'
 openssl_3_1_branch='openssl-3.1'
 openssl_3_2_branch='openssl-3.2'
 openssl_master_branch='master'
 
 mkdir -p "${scratch_folder}"
 rm -rf "${scratch_folder:?}"/*
 
-build_openssl $openssl_1_0_2_branch
 build_openssl $openssl_1_1_1_branch
 build_openssl $openssl_3_1_branch
 build_openssl $openssl_3_2_branch
 build_openssl $openssl_master_branch
 
-run_build -DCMAKE_BUILD_TYPE=RelWithDebInfo -DCMAKE_C_STANDARD=11 -DENABLE_DILITHIUM=ON
+run_build -DASAN=1
 
 # OpenSSL 3.1.0 on switches from lib folder to lib64 folder
 declare -A openssl_branches=(
-  ["$openssl_1_0_2_branch"]="lib"
   ["$openssl_1_1_1_branch"]="lib"
   ["$openssl_3_1_branch"]="lib64"
   ["$openssl_3_2_branch"]="lib64"

tool-openssl/internal.h

@@ -23,8 +23,6 @@ bool IsNumeric(const std::string& str);
 
 X509* CreateAndSignX509Certificate();
 
-bool WriteSignedCertificate(X509 *x509, const std::string &out_path);
-
 bool LoadPrivateKeyAndSignCertificate(X509 *x509, const std::string &signkey_path);
 
 tool_func_t FindTool(const std::string &name);

tool-openssl/x509.cc

@@ -12,7 +12,7 @@ static const argument_t kArguments[] = {
   { "-in", kOptionalArgument, "Certificate input, or CSR input file with -req" },
   { "-req", kBooleanArgument, "Input is a CSR file (rather than a certificate)" },
   { "-signkey", kOptionalArgument, "Causes input file to be self signed using supplied private key" },
-  { "-out", kOptionalArgument, "Output file to write to" },
+  { "-out", kOptionalArgument, "Filepath to write all output to, if not set write to stdout" },
   { "-noout", kBooleanArgument, "Prevents output of the encoded version of the certificate" },
   { "-dates", kBooleanArgument, "Print the start and expiry dates of a certificate" },
   { "-modulus", kBooleanArgument, "Prints out value of the modulus of the public key contained in the certificate" },
@@ -22,13 +22,8 @@ static const argument_t kArguments[] = {
   { "", kOptionalArgument, "" }
 };
 
-bool WriteSignedCertificate(X509 *x509, const std::string &out_path) {
-  ScopedFILE out_file(fopen(out_path.c_str(), "wb"));
-  if (!out_file) {
-    fprintf(stderr, "Error: unable to open output file '%s'\n", out_path.c_str());
-    return false;
-  }
-  if (!PEM_write_X509(out_file.get(), x509)) {
+static bool WriteSignedCertificate(X509 *x509, bssl::UniquePtr<BIO> &output_bio, const std::string &out_path) {
+  if (!PEM_write_bio_X509(output_bio.get(), x509)) {
     fprintf(stderr, "Error: error writing certificate to '%s'\n", out_path.c_str());
     ERR_print_errors_fp(stderr);
     return false;
@@ -88,6 +83,13 @@ bool X509Tool(const args_list_t &args) {
     PrintUsage(kArguments);
     return false;
   }
+  bssl::UniquePtr<BIO> output_bio;
+  if (out_path.empty()) {
+    output_bio.reset(BIO_new_fp(stdout, BIO_NOCLOSE));
+  } else {
+    output_bio.reset(BIO_new(BIO_s_file()));
+    BIO_write_filename(output_bio.get(), out_path.c_str());
+  }
 
   // Check for required option -in, and -req must include -signkey
   if (in_path.empty()) {
@@ -188,11 +190,8 @@ bool X509Tool(const args_list_t &args) {
       }
     }
 
-    // Write the signed certificate to output file
-    if (!out_path.empty()) {
-      if (!WriteSignedCertificate(x509.get(), out_path)) {
-        return false;
-      }
+    if (!WriteSignedCertificate(x509.get(), output_bio, out_path)) {
+      return false;
     }
   } else {
     // Parse x509 certificate from input PEM file
@@ -203,24 +202,17 @@ bool X509Tool(const args_list_t &args) {
       return false;
     }
     if(text) {
-      bssl::UniquePtr<BIO> bio(BIO_new_fp(stdout, BIO_NOCLOSE));
-      X509_print(bio.get(), x509.get());
+      X509_print(output_bio.get(), x509.get());
     }
 
     if (dates) {
-      bssl::UniquePtr<BIO> bio(BIO_new(BIO_s_mem()));
+      BIO_printf(output_bio.get(), "notBefore=");
+      ASN1_TIME_print(output_bio.get(), X509_get_notBefore(x509.get()));
+      BIO_printf(output_bio.get(), "\n");
 
-      if (ASN1_TIME_print(bio.get(), X509_get_notBefore(x509.get()))) {
-        char not_before[30] = {};
-        BIO_read(bio.get(), not_before, sizeof(not_before) - 1);
-        fprintf(stdout, "notBefore=%s\n", not_before);
-      }
-
-      if (ASN1_TIME_print(bio.get(), X509_get_notAfter(x509.get()))) {
-        char not_after[30] = {};
-        BIO_read(bio.get(), not_after, sizeof(not_after) - 1);
-        fprintf(stdout, "notAfter=%s\n", not_after);
-      }
+      BIO_printf(output_bio.get(), "notAfter=");
+      ASN1_TIME_print(output_bio.get(), X509_get_notAfter(x509.get()));
+      BIO_printf(output_bio.get(), "\n");
     }
 
     if (modulus) {
@@ -249,7 +241,8 @@ bool X509Tool(const args_list_t &args) {
         for (char *p = hex_modulus; *p; ++p) {
           *p = toupper(*p);
         }
-        printf("Modulus=%s\n", hex_modulus);
+        BIO_printf(output_bio.get(), "Modulus=%s\n", hex_modulus);
+
         OPENSSL_free(hex_modulus);
       } else {
         fprintf(stderr, "Error: public key is not an RSA key\n");
@@ -267,9 +260,9 @@ bool X509Tool(const args_list_t &args) {
       }
 
       if ((days_left * 86400 + seconds_left) < static_cast<int>(*checkend)) {
-        printf("Certificate will expire\n");
+        BIO_printf(output_bio.get(), "Certificate will expire\n");
       } else {
-        printf("Certificate will not expire\n");
+        BIO_printf(output_bio.get(), "Certificate will not expire\n");
       }
     }
 
@@ -279,15 +272,8 @@ bool X509Tool(const args_list_t &args) {
       }
     }
 
-    if (!out_path.empty()) {
-      if (!WriteSignedCertificate(x509.get(), out_path)) {
-        return false;
-      }
-    }
-
-    if (!noout && !in_path.empty() && !checkend && parsed_args.count("-out")==0) {
-      bssl::UniquePtr<BIO> bio_out(BIO_new_fp(stdout, BIO_NOCLOSE));
-      if (!PEM_write_bio_X509(bio_out.get(), x509.get())) {
+    if (!noout && !checkend) {
+      if (!WriteSignedCertificate(x509.get(), output_bio, out_path)) {
         return false;
       }
     }

tool-openssl/x509_test.cc

@@ -316,6 +316,13 @@ TEST_F(X509ComparisonTest, X509ToolCompareModulusOpenSSL) {
   RunCommandsAndCompareOutput(tool_command, openssl_command, out_path_tool, out_path_openssl, tool_output_str, openssl_output_str);
 
   ASSERT_EQ(tool_output_str, openssl_output_str);
+
+  tool_command = std::string(tool_executable_path) + " x509 -in " + in_path + " -modulus -out " + out_path_tool;
+  openssl_command = std::string(openssl_executable_path) + " x509 -in " + in_path + " -modulus -out " + out_path_openssl;
+
+  RunCommandsAndCompareOutput(tool_command, openssl_command, out_path_tool, out_path_openssl, tool_output_str, openssl_output_str);
+
+  ASSERT_EQ(tool_output_str, openssl_output_str);
 }
 
 // Test against OpenSSL output "openssl x509 -in in_file -checkend 0"
@@ -352,4 +359,11 @@ TEST_F(X509ComparisonTest, X509ToolCompareDatesNooutOpenSSL) {
   RunCommandsAndCompareOutput(tool_command, openssl_command, out_path_tool, out_path_openssl, tool_output_str, openssl_output_str);
 
   ASSERT_EQ(tool_output_str, openssl_output_str);
+
+  tool_command = std::string(tool_executable_path) + " x509 -in " + in_path + " -dates -noout -out " + out_path_tool;
+  openssl_command = std::string(openssl_executable_path) + " x509 -in " + in_path + " -dates -noout -out " + out_path_openssl;
+
+  RunCommandsAndCompareOutput(tool_command, openssl_command, out_path_tool, out_path_openssl, tool_output_str, openssl_output_str);
+
+  ASSERT_EQ(tool_output_str, openssl_output_str);
 }