Cleanup of tsi ssl handshaker factories.

There is no reason why the client and server factories should be
implementations from the same base.
Doing the cleanup now so that implementation of the #10528 feature will
be less noisy.
Also, re-added tsi to clang-format which was dropped when moved from
src/core/lib to src/core.

src/core/lib/http/httpcli_security_connector.c

@@ -47,7 +47,7 @@
 
 typedef struct {
   grpc_channel_security_connector base;
-  tsi_ssl_handshaker_factory *handshaker_factory;
+  tsi_ssl_client_handshaker_factory *handshaker_factory;
   char *secure_peer_name;
 } grpc_httpcli_ssl_channel_security_connector;
 
@@ -56,7 +56,7 @@ static void httpcli_ssl_destroy(grpc_exec_ctx *exec_ctx,
   grpc_httpcli_ssl_channel_security_connector *c =
       (grpc_httpcli_ssl_channel_security_connector *)sc;
   if (c->handshaker_factory != NULL) {
-    tsi_ssl_handshaker_factory_destroy(c->handshaker_factory);
+    tsi_ssl_client_handshaker_factory_destroy(c->handshaker_factory);
   }
   if (c->secure_peer_name != NULL) gpr_free(c->secure_peer_name);
   gpr_free(sc);
@@ -69,7 +69,7 @@ static void httpcli_ssl_add_handshakers(grpc_exec_ctx *exec_ctx,
       (grpc_httpcli_ssl_channel_security_connector *)sc;
   tsi_handshaker *handshaker = NULL;
   if (c->handshaker_factory != NULL) {
-    tsi_result result = tsi_ssl_handshaker_factory_create_handshaker(
+    tsi_result result = tsi_ssl_client_handshaker_factory_create_handshaker(
         c->handshaker_factory, c->secure_peer_name, &handshaker);
     if (result != TSI_OK) {
       gpr_log(GPR_ERROR, "Handshaker creation failed with error %s.",

src/core/lib/security/transport/security_connector.c

@@ -448,14 +448,14 @@ grpc_server_security_connector *grpc_fake_server_security_connector_create(
 
 typedef struct {
   grpc_channel_security_connector base;
-  tsi_ssl_handshaker_factory *handshaker_factory;
+  tsi_ssl_client_handshaker_factory *handshaker_factory;
   char *target_name;
   char *overridden_target_name;
 } grpc_ssl_channel_security_connector;
 
 typedef struct {
   grpc_server_security_connector base;
-  tsi_ssl_handshaker_factory *handshaker_factory;
+  tsi_ssl_server_handshaker_factory *handshaker_factory;
 } grpc_ssl_server_security_connector;
 
 static void ssl_channel_destroy(grpc_exec_ctx *exec_ctx,
@@ -464,7 +464,7 @@ static void ssl_channel_destroy(grpc_exec_ctx *exec_ctx,
       (grpc_ssl_channel_security_connector *)sc;
   grpc_call_credentials_unref(exec_ctx, c->base.request_metadata_creds);
   if (c->handshaker_factory != NULL) {
-    tsi_ssl_handshaker_factory_destroy(c->handshaker_factory);
+    tsi_ssl_client_handshaker_factory_destroy(c->handshaker_factory);
   }
   if (c->target_name != NULL) gpr_free(c->target_name);
   if (c->overridden_target_name != NULL) gpr_free(c->overridden_target_name);
@@ -476,26 +476,11 @@ static void ssl_server_destroy(grpc_exec_ctx *exec_ctx,
   grpc_ssl_server_security_connector *c =
       (grpc_ssl_server_security_connector *)sc;
   if (c->handshaker_factory != NULL) {
-    tsi_ssl_handshaker_factory_destroy(c->handshaker_factory);
+    tsi_ssl_server_handshaker_factory_destroy(c->handshaker_factory);
   }
   gpr_free(sc);
 }
 
-static grpc_security_status ssl_create_handshaker(
-    tsi_ssl_handshaker_factory *handshaker_factory, bool is_client,
-    const char *peer_name, tsi_handshaker **handshaker) {
-  tsi_result result = TSI_OK;
-  if (handshaker_factory == NULL) return GRPC_SECURITY_ERROR;
-  result = tsi_ssl_handshaker_factory_create_handshaker(
-      handshaker_factory, is_client ? peer_name : NULL, handshaker);
-  if (result != TSI_OK) {
-    gpr_log(GPR_ERROR, "Handshaker creation failed with error %s.",
-            tsi_result_to_string(result));
-    return GRPC_SECURITY_ERROR;
-  }
-  return GRPC_SECURITY_OK;
-}
-
 static void ssl_channel_add_handshakers(grpc_exec_ctx *exec_ctx,
                                         grpc_channel_security_connector *sc,
                                         grpc_handshake_manager *handshake_mgr) {
@@ -503,11 +488,17 @@ static void ssl_channel_add_handshakers(grpc_exec_ctx *exec_ctx,
       (grpc_ssl_channel_security_connector *)sc;
   // Instantiate TSI handshaker.
   tsi_handshaker *tsi_hs = NULL;
-  ssl_create_handshaker(c->handshaker_factory, true /* is_client */,
-                        c->overridden_target_name != NULL
-                            ? c->overridden_target_name
-                            : c->target_name,
-                        &tsi_hs);
+  tsi_result result = tsi_ssl_client_handshaker_factory_create_handshaker(
+      c->handshaker_factory,
+      c->overridden_target_name != NULL ? c->overridden_target_name
+                                        : c->target_name,
+      &tsi_hs);
+  if (result != TSI_OK) {
+    gpr_log(GPR_ERROR, "Handshaker creation failed with error %s.",
+            tsi_result_to_string(result));
+    return;
+  }
+
   // Create handshakers.
   grpc_handshake_manager_add(handshake_mgr, grpc_security_handshaker_create(
                                                 exec_ctx, tsi_hs, &sc->base));
@@ -520,8 +511,14 @@ static void ssl_server_add_handshakers(grpc_exec_ctx *exec_ctx,
       (grpc_ssl_server_security_connector *)sc;
   // Instantiate TSI handshaker.
   tsi_handshaker *tsi_hs = NULL;
-  ssl_create_handshaker(c->handshaker_factory, false /* is_client */,
-                        NULL /* peer_name */, &tsi_hs);
+  tsi_result result = tsi_ssl_server_handshaker_factory_create_handshaker(
+      c->handshaker_factory, &tsi_hs);
+  if (result != TSI_OK) {
+    gpr_log(GPR_ERROR, "Handshaker creation failed with error %s.",
+            tsi_result_to_string(result));
+    return;
+  }
+
   // Create handshakers.
   grpc_handshake_manager_add(handshake_mgr, grpc_security_handshaker_create(
                                                 exec_ctx, tsi_hs, &sc->base));

src/core/tsi/ssl_transport_security.c

@@ -81,23 +81,13 @@
 
 /* --- Structure definitions. ---*/
 
-struct tsi_ssl_handshaker_factory {
-  tsi_result (*create_handshaker)(tsi_ssl_handshaker_factory *self,
-                                  const char *server_name_indication,
-                                  tsi_handshaker **handshaker);
-  void (*destroy)(tsi_ssl_handshaker_factory *self);
-};
-
-typedef struct {
-  tsi_ssl_handshaker_factory base;
+struct tsi_ssl_client_handshaker_factory {
   SSL_CTX *ssl_context;
   unsigned char *alpn_protocol_list;
   size_t alpn_protocol_list_length;
-} tsi_ssl_client_handshaker_factory;
-
-typedef struct {
-  tsi_ssl_handshaker_factory base;
+};
 
+struct tsi_ssl_server_handshaker_factory {
   /* Several contexts to support SNI.
      The tsi_peer array contains the subject names of the server certificates
      associated with the contexts at the same index.  */
@@ -106,7 +96,7 @@ typedef struct {
   size_t ssl_context_count;
   unsigned char *alpn_protocol_list;
   size_t alpn_protocol_list_length;
-} tsi_ssl_server_handshaker_factory;
+};
 
 typedef struct {
   tsi_handshaker base;
@@ -1053,18 +1043,6 @@ static const tsi_handshaker_vtable handshaker_vtable = {
 
 /* --- tsi_ssl_handshaker_factory common methods. --- */
 
-tsi_result tsi_ssl_handshaker_factory_create_handshaker(
-    tsi_ssl_handshaker_factory *self, const char *server_name_indication,
-    tsi_handshaker **handshaker) {
-  if (self == NULL || handshaker == NULL) return TSI_INVALID_ARGUMENT;
-  return self->create_handshaker(self, server_name_indication, handshaker);
-}
-
-void tsi_ssl_handshaker_factory_destroy(tsi_ssl_handshaker_factory *self) {
-  if (self == NULL) return;
-  self->destroy(self);
-}
-
 static tsi_result create_tsi_ssl_handshaker(SSL_CTX *ctx, int is_client,
                                             const char *server_name_indication,
                                             tsi_handshaker **handshaker) {
@@ -1152,24 +1130,20 @@ static int select_protocol_list(const unsigned char **out,
   return SSL_TLSEXT_ERR_NOACK;
 }
 
-/* --- tsi_ssl__client_handshaker_factory methods implementation. --- */
+/* --- tsi_ssl_client_handshaker_factory methods implementation. --- */
 
-static tsi_result ssl_client_handshaker_factory_create_handshaker(
-    tsi_ssl_handshaker_factory *self, const char *server_name_indication,
+tsi_result tsi_ssl_client_handshaker_factory_create_handshaker(
+    tsi_ssl_client_handshaker_factory *self, const char *server_name_indication,
     tsi_handshaker **handshaker) {
-  tsi_ssl_client_handshaker_factory *impl =
-      (tsi_ssl_client_handshaker_factory *)self;
-  return create_tsi_ssl_handshaker(impl->ssl_context, 1, server_name_indication,
+  return create_tsi_ssl_handshaker(self->ssl_context, 1, server_name_indication,
                                    handshaker);
 }
 
-static void ssl_client_handshaker_factory_destroy(
-    tsi_ssl_handshaker_factory *self) {
-  tsi_ssl_client_handshaker_factory *impl =
-      (tsi_ssl_client_handshaker_factory *)self;
-  if (impl->ssl_context != NULL) SSL_CTX_free(impl->ssl_context);
-  if (impl->alpn_protocol_list != NULL) gpr_free(impl->alpn_protocol_list);
-  gpr_free(impl);
+void tsi_ssl_client_handshaker_factory_destroy(
+    tsi_ssl_client_handshaker_factory *self) {
+  if (self->ssl_context != NULL) SSL_CTX_free(self->ssl_context);
+  if (self->alpn_protocol_list != NULL) gpr_free(self->alpn_protocol_list);
+  gpr_free(self);
 }
 
 static int client_handshaker_factory_npn_callback(SSL *ssl, unsigned char **out,
@@ -1186,36 +1160,29 @@ static int client_handshaker_factory_npn_callback(SSL *ssl, unsigned char **out,
 
 /* --- tsi_ssl_server_handshaker_factory methods implementation. --- */
 
-static tsi_result ssl_server_handshaker_factory_create_handshaker(
-    tsi_ssl_handshaker_factory *self, const char *server_name_indication,
-    tsi_handshaker **handshaker) {
-  tsi_ssl_server_handshaker_factory *impl =
-      (tsi_ssl_server_handshaker_factory *)self;
-  if (impl->ssl_context_count == 0 || server_name_indication != NULL) {
-    return TSI_INVALID_ARGUMENT;
-  }
+tsi_result tsi_ssl_server_handshaker_factory_create_handshaker(
+    tsi_ssl_server_handshaker_factory *self, tsi_handshaker **handshaker) {
+  if (self->ssl_context_count == 0) return TSI_INVALID_ARGUMENT;
   /* Create the handshaker with the first context. We will switch if needed
      because of SNI in ssl_server_handshaker_factory_servername_callback.  */
-  return create_tsi_ssl_handshaker(impl->ssl_contexts[0], 0, NULL, handshaker);
+  return create_tsi_ssl_handshaker(self->ssl_contexts[0], 0, NULL, handshaker);
 }
 
-static void ssl_server_handshaker_factory_destroy(
-    tsi_ssl_handshaker_factory *self) {
-  tsi_ssl_server_handshaker_factory *impl =
-      (tsi_ssl_server_handshaker_factory *)self;
+void tsi_ssl_server_handshaker_factory_destroy(
+    tsi_ssl_server_handshaker_factory *self) {
   size_t i;
-  for (i = 0; i < impl->ssl_context_count; i++) {
-    if (impl->ssl_contexts[i] != NULL) {
-      SSL_CTX_free(impl->ssl_contexts[i]);
-      tsi_peer_destruct(&impl->ssl_context_x509_subject_names[i]);
+  for (i = 0; i < self->ssl_context_count; i++) {
+    if (self->ssl_contexts[i] != NULL) {
+      SSL_CTX_free(self->ssl_contexts[i]);
+      tsi_peer_destruct(&self->ssl_context_x509_subject_names[i]);
     }
   }
-  if (impl->ssl_contexts != NULL) gpr_free(impl->ssl_contexts);
-  if (impl->ssl_context_x509_subject_names != NULL) {
-    gpr_free(impl->ssl_context_x509_subject_names);
+  if (self->ssl_contexts != NULL) gpr_free(self->ssl_contexts);
+  if (self->ssl_context_x509_subject_names != NULL) {
+    gpr_free(self->ssl_context_x509_subject_names);
   }
-  if (impl->alpn_protocol_list != NULL) gpr_free(impl->alpn_protocol_list);
-  gpr_free(impl);
+  if (self->alpn_protocol_list != NULL) gpr_free(self->alpn_protocol_list);
+  gpr_free(self);
 }
 
 static int does_entry_match_name(const char *entry, size_t entry_length,
@@ -1317,7 +1284,7 @@ tsi_result tsi_create_ssl_client_handshaker_factory(
     const unsigned char *pem_root_certs, size_t pem_root_certs_size,
     const char *cipher_list, const unsigned char **alpn_protocols,
     const unsigned char *alpn_protocols_lengths, uint16_t num_alpn_protocols,
-    tsi_ssl_handshaker_factory **factory) {
+    tsi_ssl_client_handshaker_factory **factory) {
   SSL_CTX *ssl_context = NULL;
   tsi_ssl_client_handshaker_factory *impl = NULL;
   tsi_result result = TSI_OK;
@@ -1373,16 +1340,13 @@ tsi_result tsi_create_ssl_client_handshaker_factory(
     }
   } while (0);
   if (result != TSI_OK) {
-    ssl_client_handshaker_factory_destroy(&impl->base);
+    tsi_ssl_client_handshaker_factory_destroy(impl);
     return result;
   }
   SSL_CTX_set_verify(ssl_context, SSL_VERIFY_PEER, NULL);
   /* TODO(jboeuf): Add revocation verification. */
 
-  impl->base.create_handshaker =
-      ssl_client_handshaker_factory_create_handshaker;
-  impl->base.destroy = ssl_client_handshaker_factory_destroy;
-  *factory = &impl->base;
+  *factory = impl;
   return TSI_OK;
 }
 
@@ -1394,7 +1358,7 @@ tsi_result tsi_create_ssl_server_handshaker_factory(
     size_t pem_client_root_certs_size, int force_client_auth,
     const char *cipher_list, const unsigned char **alpn_protocols,
     const unsigned char *alpn_protocols_lengths, uint16_t num_alpn_protocols,
-    tsi_ssl_handshaker_factory **factory) {
+    tsi_ssl_server_handshaker_factory **factory) {
   return tsi_create_ssl_server_handshaker_factory_ex(
       pem_private_keys, pem_private_keys_sizes, pem_cert_chains,
       pem_cert_chains_sizes, key_cert_pair_count, pem_client_root_certs,
@@ -1414,7 +1378,7 @@ tsi_result tsi_create_ssl_server_handshaker_factory_ex(
     tsi_client_certificate_request_type client_certificate_request,
     const char *cipher_list, const unsigned char **alpn_protocols,
     const unsigned char *alpn_protocols_lengths, uint16_t num_alpn_protocols,
-    tsi_ssl_handshaker_factory **factory) {
+    tsi_ssl_server_handshaker_factory **factory) {
   tsi_ssl_server_handshaker_factory *impl = NULL;
   tsi_result result = TSI_OK;
   size_t i = 0;
@@ -1429,15 +1393,12 @@ tsi_result tsi_create_ssl_server_handshaker_factory_ex(
   }
 
   impl = gpr_zalloc(sizeof(*impl));
-  impl->base.create_handshaker =
-      ssl_server_handshaker_factory_create_handshaker;
-  impl->base.destroy = ssl_server_handshaker_factory_destroy;
   impl->ssl_contexts = gpr_zalloc(key_cert_pair_count * sizeof(SSL_CTX *));
   impl->ssl_context_x509_subject_names =
       gpr_zalloc(key_cert_pair_count * sizeof(tsi_peer));
   if (impl->ssl_contexts == NULL ||
       impl->ssl_context_x509_subject_names == NULL) {
-    tsi_ssl_handshaker_factory_destroy(&impl->base);
+    tsi_ssl_server_handshaker_factory_destroy(impl);
     return TSI_OUT_OF_RESOURCES;
   }
   impl->ssl_context_count = key_cert_pair_count;
@@ -1447,7 +1408,7 @@ tsi_result tsi_create_ssl_server_handshaker_factory_ex(
         alpn_protocols, alpn_protocols_lengths, num_alpn_protocols,
         &impl->alpn_protocol_list, &impl->alpn_protocol_list_length);
     if (result != TSI_OK) {
-      tsi_ssl_handshaker_factory_destroy(&impl->base);
+      tsi_ssl_server_handshaker_factory_destroy(impl);
       return result;
     }
   }
@@ -1520,11 +1481,11 @@ tsi_result tsi_create_ssl_server_handshaker_factory_ex(
     } while (0);
 
     if (result != TSI_OK) {
-      tsi_ssl_handshaker_factory_destroy(&impl->base);
+      tsi_ssl_server_handshaker_factory_destroy(impl);
       return result;
     }
   }
-  *factory = &impl->base;
+  *factory = impl;
   return TSI_OK;
 }
 

src/core/tsi/ssl_transport_security.h

@@ -52,12 +52,13 @@ extern "C" {
 
 #define TSI_SSL_ALPN_SELECTED_PROTOCOL "ssl_alpn_selected_protocol"
 
-/* --- tsi_ssl_handshaker_factory object ---
+/* --- tsi_ssl_client_handshaker_factory object ---
 
-   This object creates tsi_handshaker objects implemented in terms of the
-   TLS 1.2 specificiation.  */
+   This object creates a client tsi_handshaker objects implemented in terms of
+   the TLS 1.2 specificiation.  */
 
-typedef struct tsi_ssl_handshaker_factory tsi_ssl_handshaker_factory;
+typedef struct tsi_ssl_client_handshaker_factory
+    tsi_ssl_client_handshaker_factory;
 
 /* Creates a client handshaker factory.
    - pem_private_key is the buffer containing the PEM encoding of the client's
@@ -92,7 +93,33 @@ tsi_result tsi_create_ssl_client_handshaker_factory(
     const unsigned char *pem_root_certs, size_t pem_root_certs_size,
     const char *cipher_suites, const unsigned char **alpn_protocols,
     const unsigned char *alpn_protocols_lengths, uint16_t num_alpn_protocols,
-    tsi_ssl_handshaker_factory **factory);
+    tsi_ssl_client_handshaker_factory **factory);
+
+/* Creates a client handshaker.
+  - self is the factory from which the handshaker will be created.
+  - server_name_indication indicates the name of the server the client is
+    trying to connect to which will be relayed to the server using the SNI
+    extension.
+  - handshaker is the address of the handshaker pointer to be created.
+
+  - This method returns TSI_OK on success or TSI_INVALID_PARAMETER in the case
+    where a parameter is invalid.  */
+tsi_result tsi_ssl_client_handshaker_factory_create_handshaker(
+    tsi_ssl_client_handshaker_factory *self, const char *server_name_indication,
+    tsi_handshaker **handshaker);
+
+/* Destroys the handshaker factory. WARNING: it is unsafe to destroy a factory
+   while handshakers created with this factory are still in use.  */
+void tsi_ssl_client_handshaker_factory_destroy(
+    tsi_ssl_client_handshaker_factory *self);
+
+/* --- tsi_ssl_server_handshaker_factory object ---
+
+   This object creates a client tsi_handshaker objects implemented in terms of
+   the TLS 1.2 specificiation.  */
+
+typedef struct tsi_ssl_server_handshaker_factory
+    tsi_ssl_server_handshaker_factory;
 
 /* Creates a server handshaker factory.
    - version indicates which version of the specification to use.
@@ -140,7 +167,7 @@ tsi_result tsi_create_ssl_server_handshaker_factory(
     size_t pem_client_root_certs_size, int force_client_auth,
     const char *cipher_suites, const unsigned char **alpn_protocols,
     const unsigned char *alpn_protocols_lengths, uint16_t num_alpn_protocols,
-    tsi_ssl_handshaker_factory **factory);
+    tsi_ssl_server_handshaker_factory **factory);
 
 /* Same as tsi_create_ssl_server_handshaker_factory method except uses
    tsi_client_certificate_request_type to support more ways to handle client
@@ -157,25 +184,21 @@ tsi_result tsi_create_ssl_server_handshaker_factory_ex(
     tsi_client_certificate_request_type client_certificate_request,
     const char *cipher_suites, const unsigned char **alpn_protocols,
     const unsigned char *alpn_protocols_lengths, uint16_t num_alpn_protocols,
-    tsi_ssl_handshaker_factory **factory);
+    tsi_ssl_server_handshaker_factory **factory);
 
-/* Creates a handshaker.
+/* Creates a server handshaker.
   - self is the factory from which the handshaker will be created.
-  - server_name_indication indicates the name of the server the client is
-    trying to connect to which will be relayed to the server using the SNI
-    extension.
-    This parameter must be NULL for a server handshaker factory.
-  - handhshaker is the address of the handshaker pointer to be created.
+  - handshaker is the address of the handshaker pointer to be created.
 
   - This method returns TSI_OK on success or TSI_INVALID_PARAMETER in the case
     where a parameter is invalid.  */
-tsi_result tsi_ssl_handshaker_factory_create_handshaker(
-    tsi_ssl_handshaker_factory *self, const char *server_name_indication,
-    tsi_handshaker **handshaker);
+tsi_result tsi_ssl_server_handshaker_factory_create_handshaker(
+    tsi_ssl_server_handshaker_factory *self, tsi_handshaker **handshaker);
 
 /* Destroys the handshaker factory. WARNING: it is unsafe to destroy a factory
    while handshakers created with this factory are still in use.  */
-void tsi_ssl_handshaker_factory_destroy(tsi_ssl_handshaker_factory *self);
+void tsi_ssl_server_handshaker_factory_destroy(
+    tsi_ssl_server_handshaker_factory *self);
 
 /* Util that checks that an ssl peer matches a specific name.
    Still TODO(jboeuf):

tools/dockerfile/grpc_clang_format/clang_format_all_the_things.sh

@@ -31,7 +31,7 @@
 set -e
 
 # directories to run against
-DIRS="src/core/lib src/core/ext src/cpp test/core test/cpp include src/compiler"
+DIRS="src/core/lib src/core/tsi src/core/ext src/cpp test/core test/cpp include src/compiler"
 
 # file matching patterns to check
 GLOB="*.h *.c *.cc"