more TLS cleanup.

- using NULL-terminated strings as opposed to const unsigned char *
with length since the credentials are in PEM format.
- aligning the structures with gRPC so that we have less convertions to
do back and forth.

4 files changed, 68 insertions, 118 deletions

diff --git a/src/core/lib/http/httpcli_security_connector.c b/src/core/lib/http/httpcli_security_connector.c
index 9eab1360a4..76946434f0 100644
--- a/src/core/lib/http/httpcli_security_connector.c
+++ b/src/core/lib/http/httpcli_security_connector.c
@@ -106,9 +106,8 @@ static grpc_security_connector_vtable httpcli_ssl_vtable = {
     httpcli_ssl_destroy, httpcli_ssl_check_peer};
 
 static grpc_security_status httpcli_ssl_channel_security_connector_create(
-    grpc_exec_ctx *exec_ctx, const unsigned char *pem_root_certs,
-    size_t pem_root_certs_size, const char *secure_peer_name,
-    grpc_channel_security_connector **sc) {
+    grpc_exec_ctx *exec_ctx, const char *pem_root_certs,
+    const char *secure_peer_name, grpc_channel_security_connector **sc) {
   tsi_result result = TSI_OK;
   grpc_httpcli_ssl_channel_security_connector *c;
 
@@ -126,8 +125,7 @@ static grpc_security_status httpcli_ssl_channel_security_connector_create(
     c->secure_peer_name = gpr_strdup(secure_peer_name);
   }
   result = tsi_create_ssl_client_handshaker_factory(
-      NULL, 0, NULL, 0, pem_root_certs, pem_root_certs_size, NULL, NULL, NULL,
-      0, &c->handshaker_factory);
+      NULL, pem_root_certs, NULL, NULL, 0, &c->handshaker_factory);
   if (result != TSI_OK) {
     gpr_log(GPR_ERROR, "Handshaker factory creation failed with %s.",
             tsi_result_to_string(result));
@@ -173,10 +171,9 @@ static void ssl_handshake(grpc_exec_ctx *exec_ctx, void *arg,
                           void (*on_done)(grpc_exec_ctx *exec_ctx, void *arg,
                                           grpc_endpoint *endpoint)) {
   grpc_channel_security_connector *sc = NULL;
-  const unsigned char *pem_root_certs = NULL;
   on_done_closure *c = gpr_malloc(sizeof(*c));
-  size_t pem_root_certs_size = grpc_get_default_ssl_roots(&pem_root_certs);
-  if (pem_root_certs == NULL || pem_root_certs_size == 0) {
+  const char *pem_root_certs = grpc_get_default_ssl_roots();
+  if (pem_root_certs == NULL) {
     gpr_log(GPR_ERROR, "Could not get default pem root certs.");
     on_done(exec_ctx, arg, NULL);
     gpr_free(c);
@@ -186,8 +183,7 @@ static void ssl_handshake(grpc_exec_ctx *exec_ctx, void *arg,
   c->arg = arg;
   c->handshake_mgr = grpc_handshake_manager_create();
   GPR_ASSERT(httpcli_ssl_channel_security_connector_create(
-                 exec_ctx, pem_root_certs, pem_root_certs_size, host, &sc) ==
-             GRPC_SECURITY_OK);
+                 exec_ctx, pem_root_certs, host, &sc) == GRPC_SECURITY_OK);
   grpc_channel_security_connector_add_handshakers(exec_ctx, sc,
                                                   c->handshake_mgr);
   grpc_handshake_manager_do_handshake(
diff --git a/src/core/lib/security/credentials/ssl/ssl_credentials.c b/src/core/lib/security/credentials/ssl/ssl_credentials.c
index b63bb6b6e9..7c35ebe684 100644
--- a/src/core/lib/security/credentials/ssl/ssl_credentials.c
+++ b/src/core/lib/security/credentials/ssl/ssl_credentials.c
@@ -40,28 +40,24 @@
 
 #include <grpc/support/alloc.h>
 #include <grpc/support/log.h>
+#include <grpc/support/string_util.h>
 
 //
-// Utils
+// SSL Channel Credentials.
 //
 
-static void ssl_copy_key_material(const char *input, unsigned char **output,
-                                  size_t *output_size) {
-  *output_size = strlen(input);
-  *output = gpr_malloc(*output_size);
-  memcpy(*output, input, *output_size);
+static void ssl_config_pem_key_cert_pair_destroy(
+    tsi_ssl_pem_key_cert_pair *kp) {
+  if (kp == NULL) return;
+  gpr_free((void *)kp->private_key);
+  gpr_free((void *)kp->cert_chain);
 }
 
-//
-// SSL Channel Credentials.
-//
-
 static void ssl_destruct(grpc_exec_ctx *exec_ctx,
                          grpc_channel_credentials *creds) {
   grpc_ssl_credentials *c = (grpc_ssl_credentials *)creds;
-  if (c->config.pem_root_certs != NULL) gpr_free(c->config.pem_root_certs);
-  if (c->config.pem_private_key != NULL) gpr_free(c->config.pem_private_key);
-  if (c->config.pem_cert_chain != NULL) gpr_free(c->config.pem_cert_chain);
+  gpr_free(c->config.pem_root_certs);
+  ssl_config_pem_key_cert_pair_destroy(&c->config.pem_key_cert_pair);
 }
 
 static grpc_security_status ssl_create_security_connector(
@@ -102,18 +98,15 @@ static void ssl_build_config(const char *pem_root_certs,
                              grpc_ssl_pem_key_cert_pair *pem_key_cert_pair,
                              grpc_ssl_config *config) {
   if (pem_root_certs != NULL) {
-    ssl_copy_key_material(pem_root_certs, &config->pem_root_certs,
-                          &config->pem_root_certs_size);
+    config->pem_root_certs = gpr_strdup(pem_root_certs);
   }
   if (pem_key_cert_pair != NULL) {
     GPR_ASSERT(pem_key_cert_pair->private_key != NULL);
     GPR_ASSERT(pem_key_cert_pair->cert_chain != NULL);
-    ssl_copy_key_material(pem_key_cert_pair->private_key,
-                          &config->pem_private_key,
-                          &config->pem_private_key_size);
-    ssl_copy_key_material(pem_key_cert_pair->cert_chain,
-                          &config->pem_cert_chain,
-                          &config->pem_cert_chain_size);
+    config->pem_key_cert_pair.cert_chain =
+        gpr_strdup(pem_key_cert_pair->cert_chain);
+    config->pem_key_cert_pair.private_key =
+        gpr_strdup(pem_key_cert_pair->private_key);
   }
 }
 
@@ -143,22 +136,10 @@ static void ssl_server_destruct(grpc_exec_ctx *exec_ctx,
   grpc_ssl_server_credentials *c = (grpc_ssl_server_credentials *)creds;
   size_t i;
   for (i = 0; i < c->config.num_key_cert_pairs; i++) {
-    if (c->config.pem_private_keys[i] != NULL) {
-      gpr_free(c->config.pem_private_keys[i]);
-    }
-    if (c->config.pem_cert_chains[i] != NULL) {
-      gpr_free(c->config.pem_cert_chains[i]);
-    }
-  }
-  if (c->config.pem_private_keys != NULL) gpr_free(c->config.pem_private_keys);
-  if (c->config.pem_private_keys_sizes != NULL) {
-    gpr_free(c->config.pem_private_keys_sizes);
-  }
-  if (c->config.pem_cert_chains != NULL) gpr_free(c->config.pem_cert_chains);
-  if (c->config.pem_cert_chains_sizes != NULL) {
-    gpr_free(c->config.pem_cert_chains_sizes);
+    ssl_config_pem_key_cert_pair_destroy(&c->config.pem_key_cert_pairs[i]);
   }
-  if (c->config.pem_root_certs != NULL) gpr_free(c->config.pem_root_certs);
+  gpr_free(c->config.pem_key_cert_pairs);
+  gpr_free(c->config.pem_root_certs);
 }
 
 static grpc_security_status ssl_server_create_security_connector(
@@ -179,30 +160,21 @@ static void ssl_build_server_config(
   size_t i;
   config->client_certificate_request = client_certificate_request;
   if (pem_root_certs != NULL) {
-    ssl_copy_key_material(pem_root_certs, &config->pem_root_certs,
-                          &config->pem_root_certs_size);
+    config->pem_root_certs = gpr_strdup(pem_root_certs);
   }
   if (num_key_cert_pairs > 0) {
     GPR_ASSERT(pem_key_cert_pairs != NULL);
-    config->pem_private_keys =
-        gpr_malloc(num_key_cert_pairs * sizeof(unsigned char *));
-    config->pem_cert_chains =
-        gpr_malloc(num_key_cert_pairs * sizeof(unsigned char *));
-    config->pem_private_keys_sizes =
-        gpr_malloc(num_key_cert_pairs * sizeof(size_t));
-    config->pem_cert_chains_sizes =
-        gpr_malloc(num_key_cert_pairs * sizeof(size_t));
+    config->pem_key_cert_pairs =
+        gpr_zalloc(num_key_cert_pairs * sizeof(tsi_ssl_pem_key_cert_pair));
   }
   config->num_key_cert_pairs = num_key_cert_pairs;
   for (i = 0; i < num_key_cert_pairs; i++) {
     GPR_ASSERT(pem_key_cert_pairs[i].private_key != NULL);
     GPR_ASSERT(pem_key_cert_pairs[i].cert_chain != NULL);
-    ssl_copy_key_material(pem_key_cert_pairs[i].private_key,
-                          &config->pem_private_keys[i],
-                          &config->pem_private_keys_sizes[i]);
-    ssl_copy_key_material(pem_key_cert_pairs[i].cert_chain,
-                          &config->pem_cert_chains[i],
-                          &config->pem_cert_chains_sizes[i]);
+    config->pem_key_cert_pairs[i].cert_chain =
+        gpr_strdup(pem_key_cert_pairs[i].cert_chain);
+    config->pem_key_cert_pairs[i].private_key =
+        gpr_strdup(pem_key_cert_pairs[i].private_key);
   }
 }
 
diff --git a/src/core/lib/security/transport/security_connector.c b/src/core/lib/security/transport/security_connector.c
index b15196e677..a7a5d65caf 100644
--- a/src/core/lib/security/transport/security_connector.c
+++ b/src/core/lib/security/transport/security_connector.c
@@ -695,6 +695,7 @@ static grpc_security_connector_vtable ssl_channel_vtable = {
 static grpc_security_connector_vtable ssl_server_vtable = {
     ssl_server_destroy, ssl_server_check_peer};
 
+/* returns a NULL terminated slice. */
 static grpc_slice compute_default_pem_root_certs_once(void) {
   grpc_slice result = grpc_empty_slice();
 
@@ -703,7 +704,7 @@ static grpc_slice compute_default_pem_root_certs_once(void) {
       gpr_getenv(GRPC_DEFAULT_SSL_ROOTS_FILE_PATH_ENV_VAR);
   if (default_root_certs_path != NULL) {
     GRPC_LOG_IF_ERROR("load_file",
-                      grpc_load_file(default_root_certs_path, 0, &result));
+                      grpc_load_file(default_root_certs_path, 1, &result));
     gpr_free(default_root_certs_path);
   }
 
@@ -714,15 +715,18 @@ static grpc_slice compute_default_pem_root_certs_once(void) {
     ovrd_res = ssl_roots_override_cb(&pem_root_certs);
     if (ovrd_res == GRPC_SSL_ROOTS_OVERRIDE_OK) {
       GPR_ASSERT(pem_root_certs != NULL);
-      result = grpc_slice_new(pem_root_certs, strlen(pem_root_certs), gpr_free);
+      result = grpc_slice_from_copied_buffer(
+          pem_root_certs,
+          strlen(pem_root_certs) + 1);  // NULL terminator.
     }
+    gpr_free(pem_root_certs);
   }
 
   /* Fall back to installed certs if needed. */
   if (GRPC_SLICE_IS_EMPTY(result) &&
       ovrd_res != GRPC_SSL_ROOTS_OVERRIDE_FAIL_PERMANENTLY) {
     GRPC_LOG_IF_ERROR("load_file",
-                      grpc_load_file(installed_roots_path, 0, &result));
+                      grpc_load_file(installed_roots_path, 1, &result));
   }
   return result;
 }
@@ -762,13 +766,14 @@ get_tsi_client_certificate_request_type(
   }
 }
 
-size_t grpc_get_default_ssl_roots(const unsigned char **pem_root_certs) {
+const char *grpc_get_default_ssl_roots(void) {
   /* TODO(jboeuf@google.com): Maybe revisit the approach which consists in
      loading all the roots once for the lifetime of the process. */
   static gpr_once once = GPR_ONCE_INIT;
   gpr_once_init(&once, init_default_pem_root_certs);
-  *pem_root_certs = GRPC_SLICE_START_PTR(default_pem_root_certs);
-  return GRPC_SLICE_LENGTH(default_pem_root_certs);
+  return GRPC_SLICE_IS_EMPTY(default_pem_root_certs)
+             ? NULL
+             : (const char *)GRPC_SLICE_START_PTR(default_pem_root_certs);
 }
 
 grpc_security_status grpc_ssl_channel_security_connector_create(
@@ -776,22 +781,16 @@ grpc_security_status grpc_ssl_channel_security_connector_create(
     const grpc_ssl_config *config, const char *target_name,
     const char *overridden_target_name, grpc_channel_security_connector **sc) {
   size_t num_alpn_protocols = grpc_chttp2_num_alpn_versions();
-  const unsigned char **alpn_protocol_strings =
+  const char **alpn_protocol_strings =
       gpr_malloc(sizeof(const char *) * num_alpn_protocols);
-  unsigned char *alpn_protocol_string_lengths =
-      gpr_malloc(sizeof(unsigned char) * num_alpn_protocols);
   tsi_result result = TSI_OK;
   grpc_ssl_channel_security_connector *c;
   size_t i;
-  const unsigned char *pem_root_certs;
-  size_t pem_root_certs_size;
+  const char *pem_root_certs;
   char *port;
 
   for (i = 0; i < num_alpn_protocols; i++) {
-    alpn_protocol_strings[i] =
-        (const unsigned char *)grpc_chttp2_get_alpn_version_index(i);
-    alpn_protocol_string_lengths[i] =
-        (unsigned char)strlen(grpc_chttp2_get_alpn_version_index(i));
+    alpn_protocol_strings[i] = grpc_chttp2_get_alpn_version_index(i);
   }
 
   if (config == NULL || target_name == NULL) {
@@ -799,14 +798,13 @@ grpc_security_status grpc_ssl_channel_security_connector_create(
     goto error;
   }
   if (config->pem_root_certs == NULL) {
-    pem_root_certs_size = grpc_get_default_ssl_roots(&pem_root_certs);
-    if (pem_root_certs == NULL || pem_root_certs_size == 0) {
+    pem_root_certs = grpc_get_default_ssl_roots();
+    if (pem_root_certs == NULL) {
       gpr_log(GPR_ERROR, "Could not get default pem root certs.");
       goto error;
     }
   } else {
     pem_root_certs = config->pem_root_certs;
-    pem_root_certs_size = config->pem_root_certs_size;
   }
 
   c = gpr_zalloc(sizeof(grpc_ssl_channel_security_connector));
@@ -823,11 +821,12 @@ grpc_security_status grpc_ssl_channel_security_connector_create(
   if (overridden_target_name != NULL) {
     c->overridden_target_name = gpr_strdup(overridden_target_name);
   }
+
+  bool has_key_cert_pair = config->pem_key_cert_pair.private_key != NULL &&
+                           config->pem_key_cert_pair.cert_chain != NULL;
   result = tsi_create_ssl_client_handshaker_factory(
-      config->pem_private_key, config->pem_private_key_size,
-      config->pem_cert_chain, config->pem_cert_chain_size, pem_root_certs,
-      pem_root_certs_size, ssl_cipher_suites(), alpn_protocol_strings,
-      alpn_protocol_string_lengths, (uint16_t)num_alpn_protocols,
+      has_key_cert_pair ? &config->pem_key_cert_pair : NULL, pem_root_certs,
+      ssl_cipher_suites(), alpn_protocol_strings, (uint16_t)num_alpn_protocols,
       &c->handshaker_factory);
   if (result != TSI_OK) {
     gpr_log(GPR_ERROR, "Handshaker factory creation failed with %s.",
@@ -838,12 +837,10 @@ grpc_security_status grpc_ssl_channel_security_connector_create(
   }
   *sc = &c->base;
   gpr_free((void *)alpn_protocol_strings);
-  gpr_free(alpn_protocol_string_lengths);
   return GRPC_SECURITY_OK;
 
 error:
   gpr_free((void *)alpn_protocol_strings);
-  gpr_free(alpn_protocol_string_lengths);
   return GRPC_SECURITY_ERROR;
 }
 
@@ -851,19 +848,14 @@ grpc_security_status grpc_ssl_server_security_connector_create(
     grpc_exec_ctx *exec_ctx, const grpc_ssl_server_config *config,
     grpc_server_security_connector **sc) {
   size_t num_alpn_protocols = grpc_chttp2_num_alpn_versions();
-  const unsigned char **alpn_protocol_strings =
+  const char **alpn_protocol_strings =
       gpr_malloc(sizeof(const char *) * num_alpn_protocols);
-  unsigned char *alpn_protocol_string_lengths =
-      gpr_malloc(sizeof(unsigned char) * num_alpn_protocols);
   tsi_result result = TSI_OK;
   grpc_ssl_server_security_connector *c;
   size_t i;
 
   for (i = 0; i < num_alpn_protocols; i++) {
-    alpn_protocol_strings[i] =
-        (const unsigned char *)grpc_chttp2_get_alpn_version_index(i);
-    alpn_protocol_string_lengths[i] =
-        (unsigned char)strlen(grpc_chttp2_get_alpn_version_index(i));
+    alpn_protocol_strings[i] = grpc_chttp2_get_alpn_version_index(i);
   }
 
   if (config == NULL || config->num_key_cert_pairs == 0) {
@@ -876,15 +868,11 @@ grpc_security_status grpc_ssl_server_security_connector_create(
   c->base.base.url_scheme = GRPC_SSL_URL_SCHEME;
   c->base.base.vtable = &ssl_server_vtable;
   result = tsi_create_ssl_server_handshaker_factory_ex(
-      (const unsigned char **)config->pem_private_keys,
-      config->pem_private_keys_sizes,
-      (const unsigned char **)config->pem_cert_chains,
-      config->pem_cert_chains_sizes, config->num_key_cert_pairs,
-      config->pem_root_certs, config->pem_root_certs_size,
-      get_tsi_client_certificate_request_type(
-          config->client_certificate_request),
-      ssl_cipher_suites(), alpn_protocol_strings, alpn_protocol_string_lengths,
-      (uint16_t)num_alpn_protocols, &c->handshaker_factory);
+      config->pem_key_cert_pairs, config->num_key_cert_pairs,
+      config->pem_root_certs, get_tsi_client_certificate_request_type(
+                                  config->client_certificate_request),
+      ssl_cipher_suites(), alpn_protocol_strings, (uint16_t)num_alpn_protocols,
+      &c->handshaker_factory);
   if (result != TSI_OK) {
     gpr_log(GPR_ERROR, "Handshaker factory creation failed with %s.",
             tsi_result_to_string(result));
@@ -895,11 +883,9 @@ grpc_security_status grpc_ssl_server_security_connector_create(
   c->base.add_handshakers = ssl_server_add_handshakers;
   *sc = &c->base;
   gpr_free((void *)alpn_protocol_strings);
-  gpr_free(alpn_protocol_string_lengths);
   return GRPC_SECURITY_OK;
 
 error:
   gpr_free((void *)alpn_protocol_strings);
-  gpr_free(alpn_protocol_string_lengths);
   return GRPC_SECURITY_ERROR;
 }
diff --git a/src/core/lib/security/transport/security_connector.h b/src/core/lib/security/transport/security_connector.h
index cf56cb3183..d74f6739c0 100644
--- a/src/core/lib/security/transport/security_connector.h
+++ b/src/core/lib/security/transport/security_connector.h
@@ -34,11 +34,14 @@
 #ifndef GRPC_CORE_LIB_SECURITY_TRANSPORT_SECURITY_CONNECTOR_H
 #define GRPC_CORE_LIB_SECURITY_TRANSPORT_SECURITY_CONNECTOR_H
 
+#include <stdbool.h>
+
 #include <grpc/grpc_security.h>
 
 #include "src/core/lib/channel/handshaker.h"
 #include "src/core/lib/iomgr/endpoint.h"
 #include "src/core/lib/iomgr/tcp_server.h"
+#include "src/core/tsi/ssl_transport_security.h"
 #include "src/core/tsi/transport_security_interface.h"
 
 /* --- status enum. --- */
@@ -184,13 +187,10 @@ grpc_server_security_connector *grpc_fake_server_security_connector_create(
     void);
 
 /* Config for ssl clients. */
+
 typedef struct {
-  unsigned char *pem_private_key;
-  size_t pem_private_key_size;
-  unsigned char *pem_cert_chain;
-  size_t pem_cert_chain_size;
-  unsigned char *pem_root_certs;
-  size_t pem_root_certs_size;
+  tsi_ssl_pem_key_cert_pair pem_key_cert_pair;
+  char *pem_root_certs;
 } grpc_ssl_config;
 
 /* Creates an SSL channel_security_connector.
@@ -211,21 +211,17 @@ grpc_security_status grpc_ssl_channel_security_connector_create(
     const grpc_ssl_config *config, const char *target_name,
     const char *overridden_target_name, grpc_channel_security_connector **sc);
 
-/* Gets the default ssl roots. */
-size_t grpc_get_default_ssl_roots(const unsigned char **pem_root_certs);
+/* Gets the default ssl roots. Returns NULL if not found. */
+const char *grpc_get_default_ssl_roots(void);
 
 /* Exposed for TESTING ONLY!. */
 grpc_slice grpc_get_default_ssl_roots_for_testing(void);
 
 /* Config for ssl servers. */
 typedef struct {
-  unsigned char **pem_private_keys;
-  size_t *pem_private_keys_sizes;
-  unsigned char **pem_cert_chains;
-  size_t *pem_cert_chains_sizes;
+  tsi_ssl_pem_key_cert_pair *pem_key_cert_pairs;
   size_t num_key_cert_pairs;
-  unsigned char *pem_root_certs;
-  size_t pem_root_certs_size;
+  char *pem_root_certs;
   grpc_ssl_client_certificate_request_type client_certificate_request;
 } grpc_ssl_server_config;