Extend asn1_san to support multiple entries (#1552)

CMakeLists.txt

@@ -343,6 +343,11 @@ if(BUILD_TESTS)
       tls_test PRIVATE ${CMAKE_THREAD_LIBS_INIT} secp256k1.host
     )
 
+    add_test_bin(cert_test ${CMAKE_CURRENT_SOURCE_DIR}/src/tls/test/cert.cpp)
+    target_link_libraries(
+      cert_test PRIVATE ${CMAKE_THREAD_LIBS_INIT} secp256k1.host
+    )
+
     add_unit_test(
       key_exchange_test
       ${CMAKE_CURRENT_SOURCE_DIR}/src/tls/test/key_exchange.cpp
@@ -449,6 +454,15 @@ if(BUILD_TESTS)
         ${CMAKE_SOURCE_DIR}
     )
     set_property(TEST raft_scenario_test PROPERTY LABELS raft_scenario)
+
+    add_test(NAME csr_test COMMAND ${PYTHON} ${CMAKE_SOURCE_DIR}/tests/certs.py
+                                   ./cert_test
+    )
+    set_property(
+      TEST csr_test
+      APPEND
+      PROPERTY LABELS unit_test
+    )
   endif()
 
   # Picobench benchmarks

cmake/common.cmake

@@ -191,6 +191,17 @@ function(add_unit_test name)
   )
 endfunction()
 
+# Test binary wrapper
+function(add_test_bin name)
+  add_executable(${name} ${CCF_DIR}/src/enclave/thread_local.cpp ${ARGN})
+  target_compile_options(${name} PRIVATE -stdlib=libc++)
+  target_include_directories(${name} PRIVATE src ${CCFCRYPTO_INC})
+  enable_coverage(${name})
+  target_link_libraries(${name} PRIVATE ${LINK_LIBCXX} ccfcrypto.host)
+  use_client_mbedtls(${name})
+  add_san(${name})
+endfunction()
+
 if("sgx" IN_LIST COMPILE_TARGETS)
   # Host Executable
   add_executable(

src/tls/asn1_san.h

@@ -3,6 +3,7 @@
 #pragma once
 
 #include "ds/net.h"
+#include "tls/san.h"
 
 #define FMT_HEADER_ONLY
 #include <fmt/format.h>
@@ -17,9 +18,10 @@ namespace tls
   // extensions for all supported Subject Alternative Name (SAN). Until they do,
   // we have to write raw ASN1 ourselves.
 
-  // rfc5280 does not specify a maximum length for SAN. Common Name is limited
-  // to 64 so use that here.
-  static constexpr auto max_san_length = 64;
+  // rfc5280 does not specify a maximum length for SAN,
+  // but rfc1035 specified that 255 bytes is enough for a DNS name
+  static constexpr auto max_san_length = 256;
+  static constexpr auto max_san_entries = 8;
 
   // As per https://tools.ietf.org/html/rfc5280#section-4.2.1.6
   enum san_type
@@ -121,4 +123,100 @@ namespace tls
       san_buf + max_san_length - len,
       len);
   }
+
+  inline int x509write_crt_set_subject_alt_names(
+    mbedtls_x509write_cert* ctx, const std::vector<SubjectAltName>& sans)
+  {
+    if (sans.size() == 0)
+      return 0;
+
+    if (sans.size() > max_san_entries)
+    {
+      throw std::logic_error(fmt::format(
+        "Cannot set more than {} subject alternative names", max_san_entries));
+    }
+    // The factor of two is an extremely conservative provision for ASN.1
+    // metadata
+    size_t buf_len = sans.size() * max_san_length * 2;
+
+    std::vector<uint8_t> buf(buf_len);
+    uint8_t* san_buf = buf.data();
+
+    int ret = 0;
+    size_t len = 0;
+
+    // mbedtls asn1 write API writes backward in san_buf
+    uint8_t* pc = san_buf + buf_len;
+
+    for (auto& san : sans)
+    {
+      if (san.san.size() > max_san_length)
+      {
+        throw std::logic_error(fmt::format(
+          "Subject Alternative Name {} is too long ({}>{})",
+          san.san,
+          san.san.size(),
+          max_san_length));
+      }
+
+      if (san.is_ip)
+      {
+        // mbedtls (2.16.2) only supports parsing of subject alternative name
+        // that is DNS= (so no IPAddress=). When connecting to a node that has
+        // IPAddress set, mbedtls_ssl_set_hostname() should not be called.
+        // However, it should work fine with a majority of other clients (e.g.
+        // curl).
+
+        auto addr = ds::ip_to_binary(san.san.c_str());
+        if (!addr.has_value())
+        {
+          throw std ::logic_error(fmt::format(
+            "Subject Alternative Name {} is not a valid IPv4 or "
+            "IPv6 address",
+            san.san));
+        }
+
+        MBEDTLS_ASN1_CHK_ADD(
+          len,
+          mbedtls_asn1_write_raw_buffer(
+            &pc, san_buf, (const unsigned char*)&addr->buf, addr->size));
+        MBEDTLS_ASN1_CHK_ADD(
+          len, mbedtls_asn1_write_len(&pc, san_buf, addr->size));
+      }
+      else
+      {
+        MBEDTLS_ASN1_CHK_ADD(
+          len,
+          mbedtls_asn1_write_raw_buffer(
+            &pc,
+            san_buf,
+            (const unsigned char*)san.san.data(),
+            san.san.size()));
+        MBEDTLS_ASN1_CHK_ADD(
+          len, mbedtls_asn1_write_len(&pc, san_buf, san.san.size()));
+      }
+
+      MBEDTLS_ASN1_CHK_ADD(
+        len,
+        mbedtls_asn1_write_tag(
+          &pc,
+          san_buf,
+          MBEDTLS_ASN1_CONTEXT_SPECIFIC |
+            (san.is_ip ? san_type::ip_address : san_type::dns_name)));
+    }
+
+    MBEDTLS_ASN1_CHK_ADD(len, mbedtls_asn1_write_len(&pc, san_buf, len));
+    MBEDTLS_ASN1_CHK_ADD(
+      len,
+      mbedtls_asn1_write_tag(
+        &pc, san_buf, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE));
+
+    return mbedtls_x509write_crt_set_extension(
+      ctx,
+      MBEDTLS_OID_SUBJECT_ALT_NAME,
+      MBEDTLS_OID_SIZE(MBEDTLS_OID_SUBJECT_ALT_NAME),
+      0, // Mark SAN as non-critical
+      san_buf + buf_len - len,
+      len);
+  }
 }

src/tls/key_pair.h

@@ -670,15 +670,20 @@ namespace tls
       // Because mbedtls does not support parsing x509v3 extensions from a
       // CSR (https://github.com/ARMmbed/mbedtls/issues/2912), the CA sets the
       // SAN directly instead of reading it from the CSR
-      for (auto& subject_alt_name : subject_alt_names)
+      try
       {
-        if (
-          x509write_crt_set_subject_alt_name(
-            &sign.crt,
-            subject_alt_name.san.c_str(),
-            (subject_alt_name.is_ip ? san_type::ip_address :
-                                      san_type::dns_name)) != 0)
+        auto rc =
+          x509write_crt_set_subject_alt_names(&sign.crt, subject_alt_names);
+        if (rc != 0)
+        {
+          LOG_FAIL_FMT("Failed to set subject alternative names ({})", rc);
           return {};
+        }
+      }
+      catch (const std::logic_error& err)
+      {
+        LOG_FAIL_FMT(err.what());
+        return {};
       }
 
       uint8_t buf[4096];

src/tls/test/cert.cpp

@@ -0,0 +1,32 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the Apache 2.0 License.
+#include "../../ds/cli_helper.h"
+#include "../key_pair.h"
+
+#include <CLI11/CLI11.hpp>
+
+int main(int argc, char** argv)
+{
+  CLI::App app{"Cert creation"};
+  std::string subject_name;
+  app
+    .add_option(
+      "--sn", subject_name, "Subject Name in node certificate, eg. CN=CCF Node")
+    ->capture_default_str();
+
+  std::vector<tls::SubjectAltName> subject_alternative_names;
+  cli::add_subject_alternative_name_option(
+    app,
+    subject_alternative_names,
+    "--san",
+    "Subject Alternative Name in node certificate. Can be either "
+    "iPAddress:xxx.xxx.xxx.xxx, or dNSName:sub.domain.tld");
+  CLI11_PARSE(app, argc, argv);
+
+  auto kp = tls::make_key_pair();
+  auto cert = kp->sign_csr(
+    kp->create_csr(subject_name), subject_name, subject_alternative_names);
+
+  std::cout << cert.str() << std::endl;
+  return 0;
+}

tests/certs.py

@@ -0,0 +1,71 @@
+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the Apache 2.0 License.
+import subprocess
+import sys
+import tempfile
+import os
+
+
+def run(cert_test):
+    def test(args, *substrs):
+        with tempfile.NamedTemporaryFile() as ntf:
+            subprocess.run([cert_test] + args, stdout=ntf, check=True)
+            ntf.flush()
+            rv = subprocess.run(
+                [
+                    "openssl",
+                    "x509",
+                    "-in",
+                    os.path.join(tempfile.gettempdir(), ntf.name),
+                    "-text",
+                ],
+                capture_output=True,
+                check=True,
+            )
+            try:
+                for substr in substrs:
+                    assert substr in rv.stdout.decode()
+            except AssertionError:
+                print(rv.stdout.decode(), file=sys.stderr)
+                raise
+
+    test(
+        ["--sn=CN=subject", "--san=iPAddress:1.2.3.4"],
+        "Subject: CN = subject\n",
+        "X509v3 Subject Alternative Name: \n" + 16 * " " + "IP Address:1.2.3.4",
+    )
+
+    test(
+        [
+            "--sn=CN=subject",
+            "--san=iPAddress:1.2.3.4",
+            "--san=iPAddress:192.168.200.123",
+        ],
+        "Subject: CN = subject\n",
+        "X509v3 Subject Alternative Name: \n"
+        + 16 * " "
+        + "IP Address:192.168.200.123, IP Address:1.2.3.4",
+    )
+
+    test(
+        ["--sn=CN=subject", "--san=dNSName:sub.domain.tld"],
+        "Subject: CN = subject\n",
+        "X509v3 Subject Alternative Name: \n" + 16 * " " + "DNS:sub.domain.tld",
+    )
+
+    test(
+        [
+            "--sn=CN=subject",
+            "--san=iPAddress:1.2.3.4",
+            "--san=dNSName:sub.domain.tld",
+            "--san=iPAddress:192.168.200.123",
+        ],
+        "Subject: CN = subject\n",
+        "X509v3 Subject Alternative Name: \n"
+        + 16 * " "
+        + "IP Address:192.168.200.123, DNS:sub.domain.tld, IP Address:1.2.3.4",
+    )
+
+
+if __name__ == "__main__":
+    run(sys.argv[1])