Merge pull request #1580 from jboeuf/tsi_properties_cleanup

Clean up tsi properties.

6 files changed, 79 insertions, 212 deletions

diff --git a/src/core/security/security_connector.c b/src/core/security/security_connector.c
index dbd79c5b22..61cb20f6b9 100644
--- a/src/core/security/security_connector.c
+++ b/src/core/security/security_connector.c
@@ -81,6 +81,24 @@ static const char *ssl_cipher_suites(void) {
 
 /* -- Common methods. -- */
 
+/* Returns the first property with that name. */
+static const tsi_peer_property *tsi_peer_get_property_by_name(
+    const tsi_peer *peer, const char *name) {
+  size_t i;
+  if (peer == NULL) return NULL;
+  for (i = 0; i < peer->property_count; i++) {
+    const tsi_peer_property* property = &peer->properties[i];
+    if (name == NULL && property->name == NULL) {
+      return property;
+    }
+    if (name != NULL && property->name != NULL &&
+        strcmp(property->name, name) == 0) {
+      return property;
+    }
+  }
+  return NULL;
+}
+
 grpc_security_status grpc_security_connector_create_handshaker(
     grpc_security_connector *sc, tsi_handshaker **handshaker) {
   if (sc == NULL || handshaker == NULL) return GRPC_SECURITY_ERROR;
@@ -212,13 +230,8 @@ static grpc_security_status fake_check_peer(grpc_security_connector *sc,
     status = GRPC_SECURITY_ERROR;
     goto end;
   }
-  if (peer.properties[0].type != TSI_PEER_PROPERTY_TYPE_STRING) {
-    gpr_log(GPR_ERROR, "Invalid type of cert type property.");
-    status = GRPC_SECURITY_ERROR;
-    goto end;
-  }
-  if (strncmp(peer.properties[0].value.string.data, TSI_FAKE_CERTIFICATE_TYPE,
-              peer.properties[0].value.string.length)) {
+  if (strncmp(peer.properties[0].value.data, TSI_FAKE_CERTIFICATE_TYPE,
+              peer.properties[0].value.length)) {
     gpr_log(GPR_ERROR, "Invalid value for cert type property.");
     status = GRPC_SECURITY_ERROR;
     goto end;
@@ -365,12 +378,7 @@ static grpc_security_status ssl_check_peer(const char *peer_name,
     gpr_log(GPR_ERROR, "Missing selected ALPN property.");
     return GRPC_SECURITY_ERROR;
   }
-  if (p->type != TSI_PEER_PROPERTY_TYPE_STRING) {
-    gpr_log(GPR_ERROR, "Invalid selected ALPN property.");
-    return GRPC_SECURITY_ERROR;
-  }
-  if (!grpc_chttp2_is_alpn_version_supported(p->value.string.data,
-                                             p->value.string.length)) {
+  if (!grpc_chttp2_is_alpn_version_supported(p->value.data, p->value.length)) {
     gpr_log(GPR_ERROR, "Invalid ALPN value.");
     return GRPC_SECURITY_ERROR;
   }
diff --git a/src/core/tsi/ssl_transport_security.c b/src/core/tsi/ssl_transport_security.c
index 22cc7d4f4c..9718a0b048 100644
--- a/src/core/tsi/ssl_transport_security.c
+++ b/src/core/tsi/ssl_transport_security.c
@@ -269,31 +269,16 @@ static tsi_result peer_property_from_x509_common_name(
 }
 
 /* Gets the subject SANs from an X509 cert as a tsi_peer_property. */
-static tsi_result peer_property_from_x509_subject_alt_names(
-    X509* cert, tsi_peer_property* property) {
-  int i = 0;
-  int subject_alt_name_count = 0;
+static tsi_result add_subject_alt_names_properties_to_peer(
+    tsi_peer* peer, GENERAL_NAMES* subject_alt_names,
+    int subject_alt_name_count) {
+  int i;
   tsi_result result = TSI_OK;
-  GENERAL_NAMES* subject_alt_names =
-      X509_get_ext_d2i(cert, NID_subject_alt_name, 0, 0);
-  if (subject_alt_names == NULL) {
-    /* Empty list. */
-    return tsi_construct_list_peer_property(
-        TSI_X509_SUBJECT_ALTERNATIVE_NAMES_PEER_PROPERTY, 0, property);
-  }
-
-  subject_alt_name_count = sk_GENERAL_NAME_num(subject_alt_names);
-  result = tsi_construct_list_peer_property(
-      TSI_X509_SUBJECT_ALTERNATIVE_NAMES_PEER_PROPERTY, subject_alt_name_count,
-      property);
-  if (result != TSI_OK) return result;
 
   /* Reset for DNS entries filtering. */
-  subject_alt_name_count = property->value.list.child_count;
-  property->value.list.child_count = 0;
+  peer->property_count -= subject_alt_name_count;
 
   for (i = 0; i < subject_alt_name_count; i++) {
-    tsi_peer_property* child_property = NULL;
     GENERAL_NAME* subject_alt_name =
         sk_GENERAL_NAME_value(subject_alt_names, i);
     /* Filter out the non-dns entries names. */
@@ -306,40 +291,50 @@ static tsi_result peer_property_from_x509_subject_alt_names(
         result = TSI_INTERNAL_ERROR;
         break;
       }
-      child_property =
-          &property->value.list.children[property->value.list.child_count++];
       result = tsi_construct_string_peer_property(
-          NULL, (const char*)dns_name, dns_name_size, child_property);
+          TSI_X509_SUBJECT_ALTERNATIVE_NAME_PEER_PROPERTY,
+          (const char*)dns_name, dns_name_size,
+          &peer->properties[peer->property_count++]);
       OPENSSL_free(dns_name);
       if (result != TSI_OK) break;
     }
   }
-  if (result != TSI_OK) tsi_peer_property_destruct(property);
-  sk_GENERAL_NAME_pop_free(subject_alt_names, GENERAL_NAME_free);
-  return TSI_OK;
+  return result;
 }
 
 /* Gets information about the peer's X509 cert as a tsi_peer object. */
 static tsi_result peer_from_x509(X509* cert, int include_certificate_type,
                                  tsi_peer* peer) {
   /* TODO(jboeuf): Maybe add more properties. */
-  size_t property_count = include_certificate_type ? 3 : 2;
+  GENERAL_NAMES* subject_alt_names =
+      X509_get_ext_d2i(cert, NID_subject_alt_name, 0, 0);
+  int subject_alt_name_count =
+      (subject_alt_names != NULL) ? sk_GENERAL_NAME_num(subject_alt_names) : 0;
+  size_t property_count = (include_certificate_type ? 1 : 0) +
+                          1 /* common name */ + subject_alt_name_count;
   tsi_result result = tsi_construct_peer(property_count, peer);
   if (result != TSI_OK) return result;
   do {
-    result = peer_property_from_x509_common_name(cert, &peer->properties[0]);
-    if (result != TSI_OK) break;
-    result =
-        peer_property_from_x509_subject_alt_names(cert, &peer->properties[1]);
-    if (result != TSI_OK) break;
     if (include_certificate_type) {
       result = tsi_construct_string_peer_property_from_cstring(
           TSI_CERTIFICATE_TYPE_PEER_PROPERTY, TSI_X509_CERTIFICATE_TYPE,
-          &peer->properties[2]);
+          &peer->properties[0]);
+      if (result != TSI_OK) break;
+    }
+    result = peer_property_from_x509_common_name(
+        cert, &peer->properties[include_certificate_type ? 1 : 0]);
+    if (result != TSI_OK) break;
+
+    if (subject_alt_name_count != 0) {
+      result = add_subject_alt_names_properties_to_peer(peer, subject_alt_names,
+                                                        subject_alt_name_count);
       if (result != TSI_OK) break;
     }
   } while (0);
 
+  if (subject_alt_names != NULL) {
+    sk_GENERAL_NAME_pop_free(subject_alt_names, GENERAL_NAME_free);
+  }
   if (result != TSI_OK) tsi_peer_destruct(peer);
   return result;
 }
@@ -1345,43 +1340,32 @@ tsi_result tsi_create_ssl_server_handshaker_factory(
 int tsi_ssl_peer_matches_name(const tsi_peer* peer, const char* name) {
   size_t i = 0;
   size_t san_count = 0;
-  const tsi_peer_property* property = NULL;
+  const tsi_peer_property* cn_property = NULL;
 
   /* For now reject what looks like an IP address. */
   if (looks_like_ip_address(name)) return 0;
 
   /* Check the SAN first. */
-  property = tsi_peer_get_property_by_name(
-      peer, TSI_X509_SUBJECT_ALTERNATIVE_NAMES_PEER_PROPERTY);
-  if (property == NULL || property->type != TSI_PEER_PROPERTY_TYPE_LIST) {
-    gpr_log(GPR_ERROR, "Invalid x509 subject alternative names property.");
-    return 0;
-  }
-
-  san_count = property->value.list.child_count;
-  for (i = 0; i < san_count; i++) {
-    const tsi_peer_property* alt_name_property =
-        &property->value.list.children[i];
-    if (alt_name_property->type != TSI_PEER_PROPERTY_TYPE_STRING) {
-      gpr_log(GPR_ERROR, "Invalid x509 subject alternative name property.");
-      return 0;
-    }
-    if (does_entry_match_name(alt_name_property->value.string.data,
-                              alt_name_property->value.string.length, name)) {
-      return 1;
+  for (i = 0; i < peer->property_count; i++) {
+    const tsi_peer_property* property = &peer->properties[i];
+    if (property->name == NULL) continue;
+    if (strcmp(property->name,
+               TSI_X509_SUBJECT_ALTERNATIVE_NAME_PEER_PROPERTY) == 0) {
+      san_count++;
+      if (does_entry_match_name(property->value.data, property->value.length,
+                                name)) {
+        return 1;
+      }
+    } else if (strcmp(property->name,
+                      TSI_X509_SUBJECT_COMMON_NAME_PEER_PROPERTY) == 0) {
+      cn_property = property;
     }
   }
 
   /* If there's no SAN, try the CN. */
-  if (san_count == 0) {
-    property = tsi_peer_get_property_by_name(
-        peer, TSI_X509_SUBJECT_COMMON_NAME_PEER_PROPERTY);
-    if (property == NULL || property->type != TSI_PEER_PROPERTY_TYPE_STRING) {
-      gpr_log(GPR_ERROR, "Invalid x509 subject common name property.");
-      return 0;
-    }
-    if (does_entry_match_name(property->value.string.data,
-                              property->value.string.length, name)) {
+  if (san_count == 0 && cn_property != NULL) {
+    if (does_entry_match_name(cn_property->value.data,
+                              cn_property->value.length, name)) {
       return 1;
     }
   }
diff --git a/src/core/tsi/ssl_transport_security.h b/src/core/tsi/ssl_transport_security.h
index 192f7acf0e..b2aa2f393e 100644
--- a/src/core/tsi/ssl_transport_security.h
+++ b/src/core/tsi/ssl_transport_security.h
@@ -45,13 +45,9 @@ extern "C" {
 
 /* This property is of type TSI_PEER_PROPERTY_STRING.  */
 #define TSI_X509_SUBJECT_COMMON_NAME_PEER_PROPERTY "x509_subject_common_name"
+#define TSI_X509_SUBJECT_ALTERNATIVE_NAME_PEER_PROPERTY \
+  "x509_subject_alternative_name"
 
-/* This property is of type TSI_PEER_PROPERTY_LIST and the children contain
-   unnamed (name == NULL) properties of type TSI_PEER_PROPERTY_STRING.  */
-#define TSI_X509_SUBJECT_ALTERNATIVE_NAMES_PEER_PROPERTY \
-  "x509_subject_alternative_names"
-
-/* This property is of type TSI_PEER_PROPERTY_STRING. */
 #define TSI_SSL_ALPN_SELECTED_PROTOCOL "ssl_alpn_selected_protocol"
 
 /* --- tsi_ssl_handshaker_factory object ---
diff --git a/src/core/tsi/transport_security.c b/src/core/tsi/transport_security.c
index f4ab9d2bc6..ec02a478ba 100644
--- a/src/core/tsi/transport_security.c
+++ b/src/core/tsi/transport_security.c
@@ -198,23 +198,6 @@ void tsi_handshaker_destroy(tsi_handshaker* self) {
 
 /* --- tsi_peer implementation. --- */
 
-const tsi_peer_property* tsi_peer_get_property_by_name(const tsi_peer* self,
-                                                       const char* name) {
-  size_t i;
-  if (self == NULL) return NULL;
-  for (i = 0; i < self->property_count; i++) {
-    const tsi_peer_property* property = &self->properties[i];
-    if (name == NULL && property->name == NULL) {
-      return property;
-    }
-    if (name != NULL && property->name != NULL &&
-        strcmp(property->name, name) == 0) {
-      return property;
-    }
-  }
-  return NULL;
-}
-
 tsi_peer_property tsi_init_peer_property(void) {
   tsi_peer_property property;
   memset(&property, 0, sizeof(tsi_peer_property));
@@ -234,18 +217,8 @@ void tsi_peer_property_destruct(tsi_peer_property* property) {
   if (property->name != NULL) {
     free(property->name);
   }
-  switch (property->type) {
-    case TSI_PEER_PROPERTY_TYPE_STRING:
-      if (property->value.string.data != NULL) {
-        free(property->value.string.data);
-      }
-      break;
-    case TSI_PEER_PROPERTY_TYPE_LIST:
-      tsi_peer_destroy_list_property(property->value.list.children,
-                                     property->value.list.child_count);
-    default:
-      /* Nothing to free. */
-      break;
+  if (property->value.data != NULL) {
+    free(property->value.data);
   }
   *property = tsi_init_peer_property(); /* Reset everything to 0. */
 }
@@ -259,57 +232,20 @@ void tsi_peer_destruct(tsi_peer* self) {
   self->property_count = 0;
 }
 
-tsi_result tsi_construct_signed_integer_peer_property(
-    const char* name, int64_t value, tsi_peer_property* property) {
-  *property = tsi_init_peer_property();
-  property->type = TSI_PEER_PROPERTY_TYPE_SIGNED_INTEGER;
-  if (name != NULL) {
-    property->name = tsi_strdup(name);
-    if (property->name == NULL) return TSI_OUT_OF_RESOURCES;
-  }
-  property->value.signed_int = value;
-  return TSI_OK;
-}
-
-tsi_result tsi_construct_unsigned_integer_peer_property(
-    const char* name, uint64_t value, tsi_peer_property* property) {
-  *property = tsi_init_peer_property();
-  property->type = TSI_PEER_PROPERTY_TYPE_UNSIGNED_INTEGER;
-  if (name != NULL) {
-    property->name = tsi_strdup(name);
-    if (property->name == NULL) return TSI_OUT_OF_RESOURCES;
-  }
-  property->value.unsigned_int = value;
-  return TSI_OK;
-}
-
-tsi_result tsi_construct_real_peer_property(const char* name, double value,
-                                            tsi_peer_property* property) {
-  *property = tsi_init_peer_property();
-  property->type = TSI_PEER_PROPERTY_TYPE_REAL;
-  if (name != NULL) {
-    property->name = tsi_strdup(name);
-    if (property->name == NULL) return TSI_OUT_OF_RESOURCES;
-  }
-  property->value.real = value;
-  return TSI_OK;
-}
-
 tsi_result tsi_construct_allocated_string_peer_property(
     const char* name, size_t value_length, tsi_peer_property* property) {
   *property = tsi_init_peer_property();
-  property->type = TSI_PEER_PROPERTY_TYPE_STRING;
   if (name != NULL) {
     property->name = tsi_strdup(name);
     if (property->name == NULL) return TSI_OUT_OF_RESOURCES;
   }
   if (value_length > 0) {
-    property->value.string.data = calloc(1, value_length);
-    if (property->value.string.data == NULL) {
+    property->value.data = calloc(1, value_length);
+    if (property->value.data == NULL) {
       tsi_peer_property_destruct(property);
       return TSI_OUT_OF_RESOURCES;
     }
-    property->value.string.length = value_length;
+    property->value.length = value_length;
   }
   return TSI_OK;
 }
@@ -328,28 +264,7 @@ tsi_result tsi_construct_string_peer_property(const char* name,
       name, value_length, property);
   if (result != TSI_OK) return result;
   if (value_length > 0) {
-    memcpy(property->value.string.data, value, value_length);
-  }
-  return TSI_OK;
-}
-
-tsi_result tsi_construct_list_peer_property(const char* name,
-                                            size_t child_count,
-                                            tsi_peer_property* property) {
-  *property = tsi_init_peer_property();
-  property->type = TSI_PEER_PROPERTY_TYPE_LIST;
-  if (name != NULL) {
-    property->name = tsi_strdup(name);
-    if (property->name == NULL) return TSI_OUT_OF_RESOURCES;
-  }
-  if (child_count > 0) {
-    property->value.list.children =
-        calloc(child_count, sizeof(tsi_peer_property));
-    if (property->value.list.children == NULL) {
-      tsi_peer_property_destruct(property);
-      return TSI_OUT_OF_RESOURCES;
-    }
-    property->value.list.child_count = child_count;
+    memcpy(property->value.data, value, value_length);
   }
   return TSI_OK;
 }
diff --git a/src/core/tsi/transport_security.h b/src/core/tsi/transport_security.h
index 59e5dcff9a..4cd0ec2cfb 100644
--- a/src/core/tsi/transport_security.h
+++ b/src/core/tsi/transport_security.h
@@ -92,12 +92,6 @@ struct tsi_handshaker {
 tsi_result tsi_construct_peer(size_t property_count, tsi_peer* peer);
 tsi_peer_property tsi_init_peer_property(void);
 void tsi_peer_property_destruct(tsi_peer_property* property);
-tsi_result tsi_construct_signed_integer_peer_property(
-    const char* name, int64_t value, tsi_peer_property* property);
-tsi_result tsi_construct_unsigned_integer_peer_property(
-    const char* name, uint64_t value, tsi_peer_property* property);
-tsi_result tsi_construct_real_peer_property(const char* name, double value,
-                                            tsi_peer_property* property);
 tsi_result tsi_construct_string_peer_property(const char* name,
                                               const char* value,
                                               size_t value_length,
@@ -106,9 +100,6 @@ tsi_result tsi_construct_allocated_string_peer_property(
     const char* name, size_t value_length, tsi_peer_property* property);
 tsi_result tsi_construct_string_peer_property_from_cstring(
     const char* name, const char* value, tsi_peer_property* property);
-tsi_result tsi_construct_list_peer_property(const char* name,
-                                            size_t child_count,
-                                            tsi_peer_property* property);
 
 /* Utils. */
 char* tsi_strdup(const char* src); /* Sadly, no strdup in C89. */
diff --git a/src/core/tsi/transport_security_interface.h b/src/core/tsi/transport_security_interface.h
index 0edff54235..936b0c25b0 100644
--- a/src/core/tsi/transport_security_interface.h
+++ b/src/core/tsi/transport_security_interface.h
@@ -179,33 +179,13 @@ void tsi_frame_protector_destroy(tsi_frame_protector* self);
 /* This property is of type TSI_PEER_PROPERTY_STRING.  */
 #define TSI_CERTIFICATE_TYPE_PEER_PROPERTY "certificate_type"
 
-/* Properties of type TSI_PEER_PROPERTY_TYPE_STRING may contain NULL characters
-   just like C++ strings. The length field gives the length of the string.  */
-typedef enum {
-  TSI_PEER_PROPERTY_TYPE_SIGNED_INTEGER,
-  TSI_PEER_PROPERTY_TYPE_UNSIGNED_INTEGER,
-  TSI_PEER_PROPERTY_TYPE_REAL,
-  TSI_PEER_PROPERTY_TYPE_STRING,
-  TSI_PEER_PROPERTY_TYPE_LIST
-} tsi_peer_property_type;
-
-/* The relevant field in the union value is dictated by the type field.
-   name may be NULL in case of an unnamed property. */
+/* Property values may contain NULL characters just like C++ strings.
+   The length field gives the length of the string. */
 typedef struct tsi_peer_property {
   char* name;
-  tsi_peer_property_type type;
-  union {
-    int64_t signed_int;
-    uint64_t unsigned_int;
-    double real;
-    struct {
-      char* data;
-      size_t length;
-    } string;
-    struct {
-      struct tsi_peer_property* children;
-      size_t child_count;
-    } list;
+  struct {
+    char* data;
+    size_t length;
   } value;
 } tsi_peer_property;
 
@@ -214,13 +194,6 @@ typedef struct {
   size_t property_count;
 } tsi_peer;
 
-/* Gets the first property with the specified name. Iteration over the
-   properties of the peer should be used if the client of the API is expecting
-   several properties with the same name.
-   Returns NULL if there is no corresponding property.  */
-const tsi_peer_property* tsi_peer_get_property_by_name(const tsi_peer* self,
-                                                       const char* name);
-
 /* Destructs the tsi_peer object. */
 void tsi_peer_destruct(tsi_peer* self);