Removing the peer from the SSL security connector.

- Missing unit tests.

src/core/httpcli/httpcli_security_connector.c

@@ -84,7 +84,8 @@ static void httpcli_ssl_do_handshake(grpc_exec_ctx *exec_ctx,
 }
 
 static void httpcli_ssl_check_peer(grpc_exec_ctx *exec_ctx,
-                                   grpc_security_connector *sc, tsi_peer peer,
+                                   grpc_security_connector *sc,
+                                   tsi_peer peer,
                                    grpc_security_peer_check_cb cb,
                                    void *user_data) {
   grpc_httpcli_ssl_channel_security_connector *c =
@@ -98,8 +99,8 @@ static void httpcli_ssl_check_peer(grpc_exec_ctx *exec_ctx,
             c->secure_peer_name);
     status = GRPC_SECURITY_ERROR;
   }
-  tsi_peer_destruct(&peer);
   cb(exec_ctx, user_data, status, NULL);
+  tsi_peer_destruct(&peer);
 }
 
 static grpc_security_connector_vtable httpcli_ssl_vtable = {

src/core/security/client_auth_filter.c

@@ -250,28 +250,14 @@ static void auth_start_transport_op(grpc_exec_ctx *exec_ctx,
       }
     }
     if (calld->host != NULL) {
-      grpc_security_status status;
       const char *call_host = grpc_mdstr_as_c_string(calld->host);
       calld->op = *op; /* Copy op (originates from the caller's stack). */
-      status = grpc_channel_security_connector_check_call_host(
+      grpc_channel_security_connector_check_call_host(
-          exec_ctx, chand->security_connector, call_host, on_host_checked,
+          exec_ctx, chand->security_connector, call_host, chand->auth_context,
-          elem);
+          on_host_checked, elem);
-      if (status != GRPC_SECURITY_OK) {
-        if (status == GRPC_SECURITY_ERROR) {
-          char *error_msg;
-          gpr_asprintf(&error_msg,
-                       "Invalid host %s set in :authority metadata.",
-                       call_host);
-          bubble_up_error(exec_ctx, elem, GRPC_STATUS_INVALID_ARGUMENT,
-                          error_msg);
-          gpr_free(error_msg);
-        }
       return; /* early exit */
     }
   }
-    send_security_metadata(exec_ctx, elem, op);
-    return; /* early exit */
-  }
 
   /* pass control down the stack */
   grpc_call_next_op(exec_ctx, elem, op);

src/core/security/credentials.c

@@ -881,7 +881,7 @@ static grpc_security_status fake_transport_security_create_security_connector(
     grpc_channel_credentials *c, grpc_call_credentials *call_creds,
     const char *target, const grpc_channel_args *args,
     grpc_channel_security_connector **sc, grpc_channel_args **new_args) {
-  *sc = grpc_fake_channel_security_connector_create(call_creds, 1);
+  *sc = grpc_fake_channel_security_connector_create(call_creds);
   return GRPC_SECURITY_OK;
 }
 

src/core/security/security_connector.c

@@ -130,22 +130,28 @@ void grpc_security_connector_do_handshake(grpc_exec_ctx *exec_ctx,
   }
 }
 
-void grpc_security_connector_check_peer(
+void grpc_security_connector_check_peer(grpc_exec_ctx *exec_ctx,
-    grpc_exec_ctx *exec_ctx, grpc_security_connector *sc, tsi_peer peer,
+                                        grpc_security_connector *sc,
-    grpc_security_peer_check_cb cb, void *user_data) {
+                                        tsi_peer peer,
+                                        grpc_security_peer_check_cb cb,
+                                        void *user_data) {
   if (sc == NULL) {
-    tsi_peer_destruct(&peer);
     cb(exec_ctx, user_data, GRPC_SECURITY_ERROR, NULL);
+    tsi_peer_destruct(&peer);
   } else {
     sc->vtable->check_peer(exec_ctx, sc, peer, cb, user_data);
   }
 }
 
-grpc_security_status grpc_channel_security_connector_check_call_host(
+void grpc_channel_security_connector_check_call_host(
     grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *sc,
-    const char *host, grpc_security_call_host_check_cb cb, void *user_data) {
+    const char *host, grpc_auth_context *auth_context,
-  if (sc == NULL || sc->check_call_host == NULL) return GRPC_SECURITY_ERROR;
+    grpc_security_call_host_check_cb cb, void *user_data) {
-  return sc->check_call_host(exec_ctx, sc, host, cb, user_data);
+  if (sc == NULL || sc->check_call_host == NULL) {
+    cb(exec_ctx, user_data, GRPC_SECURITY_ERROR);
+  } else {
+    sc->check_call_host(exec_ctx, sc, host, auth_context, cb, user_data);
+  }
 }
 
 #ifdef GRPC_SECURITY_CONNECTOR_REFCOUNT_DEBUG
@@ -222,11 +228,6 @@ grpc_security_connector *grpc_find_security_connector_in_args(
 
 /* -- Fake implementation. -- */
 
-typedef struct {
-  grpc_channel_security_connector base;
-  int call_host_check_is_async;
-} grpc_fake_channel_security_connector;
-
 static void fake_channel_destroy(grpc_security_connector *sc) {
   grpc_channel_security_connector *c = (grpc_channel_security_connector *)sc;
   grpc_call_credentials_unref(c->request_metadata_creds);
@@ -270,21 +271,17 @@ static void fake_check_peer(grpc_exec_ctx *exec_ctx,
 
 end:
   cb(exec_ctx, user_data, status, auth_context);
-  tsi_peer_destruct(&peer);
   grpc_auth_context_unref(auth_context);
+  tsi_peer_destruct(&peer);
 }
 
-static grpc_security_status fake_channel_check_call_host(
+static void fake_channel_check_call_host(grpc_exec_ctx *exec_ctx,
-    grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *sc,
+                                         grpc_channel_security_connector *sc,
-    const char *host, grpc_security_call_host_check_cb cb, void *user_data) {
+                                         const char *host,
-  grpc_fake_channel_security_connector *c =
+                                         grpc_auth_context *auth_context,
-      (grpc_fake_channel_security_connector *)sc;
+                                         grpc_security_call_host_check_cb cb,
-  if (c->call_host_check_is_async) {
+                                         void *user_data) {
   cb(exec_ctx, user_data, GRPC_SECURITY_OK);
-    return GRPC_SECURITY_PENDING;
-  } else {
-    return GRPC_SECURITY_OK;
-  }
 }
 
 static void fake_channel_do_handshake(grpc_exec_ctx *exec_ctx,
@@ -312,20 +309,17 @@ static grpc_security_connector_vtable fake_server_vtable = {
     fake_server_destroy, fake_server_do_handshake, fake_check_peer};
 
 grpc_channel_security_connector *grpc_fake_channel_security_connector_create(
-    grpc_call_credentials *request_metadata_creds,
+    grpc_call_credentials *request_metadata_creds) {
-    int call_host_check_is_async) {
+  grpc_channel_security_connector *c = gpr_malloc(sizeof(*c));
-  grpc_fake_channel_security_connector *c =
+  memset(c, 0, sizeof(*c));
-      gpr_malloc(sizeof(grpc_fake_channel_security_connector));
+  gpr_ref_init(&c->base.refcount, 1);
-  memset(c, 0, sizeof(grpc_fake_channel_security_connector));
+  c->base.is_client_side = 1;
-  gpr_ref_init(&c->base.base.refcount, 1);
+  c->base.url_scheme = GRPC_FAKE_SECURITY_URL_SCHEME;
-  c->base.base.is_client_side = 1;
+  c->base.vtable = &fake_channel_vtable;
-  c->base.base.url_scheme = GRPC_FAKE_SECURITY_URL_SCHEME;
+  c->request_metadata_creds =
-  c->base.base.vtable = &fake_channel_vtable;
-  c->base.request_metadata_creds =
       grpc_call_credentials_ref(request_metadata_creds);
-  c->base.check_call_host = fake_channel_check_call_host;
+  c->check_call_host = fake_channel_check_call_host;
-  c->call_host_check_is_async = call_host_check_is_async;
+  return c;
-  return &c->base;
 }
 
 grpc_security_connector *grpc_fake_server_security_connector_create(void) {
@@ -346,9 +340,6 @@ typedef struct {
   tsi_ssl_handshaker_factory *handshaker_factory;
   char *target_name;
   char *overridden_target_name;
-  /* TODO(jboeuf): Remove this: the security connector is channel-wide construct
-     as opposed to the peer which is for one transport (or sub-channel). */
-  tsi_peer peer;
 } grpc_ssl_channel_security_connector;
 
 typedef struct {
@@ -365,7 +356,6 @@ static void ssl_channel_destroy(grpc_security_connector *sc) {
   }
   if (c->target_name != NULL) gpr_free(c->target_name);
   if (c->overridden_target_name != NULL) gpr_free(c->overridden_target_name);
-  tsi_peer_destruct(&c->peer);
   gpr_free(sc);
 }
 
@@ -517,14 +507,13 @@ static void ssl_channel_check_peer(
       (grpc_ssl_channel_security_connector *)sc;
   grpc_security_status status;
   grpc_auth_context *auth_context = NULL;
-  tsi_peer_destruct(&c->peer);
-  c->peer = peer;
   status = ssl_check_peer(sc, c->overridden_target_name != NULL
                                   ? c->overridden_target_name
                                   : c->target_name,
                           &peer, &auth_context);
   cb(exec_ctx, user_data, status, auth_context);
   grpc_auth_context_unref(auth_context);
+  tsi_peer_destruct(&peer);
 }
 
 static void ssl_server_check_peer(
@@ -537,22 +526,66 @@ static void ssl_server_check_peer(
   grpc_auth_context_unref(auth_context);
 }
 
-static grpc_security_status ssl_channel_check_call_host(
+static void add_shalow_auth_property_to_peer(tsi_peer *peer,
-    grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *sc,
+                                             const grpc_auth_property *prop,
-    const char *host, grpc_security_call_host_check_cb cb, void *user_data) {
+                                             const char *tsi_prop_name) {
+  tsi_peer_property *tsi_prop = &peer->properties[peer->property_count++];
+  tsi_prop->name = (char *)tsi_prop_name;
+  tsi_prop->value.data = prop->value;
+  tsi_prop->value.length = prop->value_length;
+}
+
+tsi_peer tsi_shallow_peer_from_ssl_auth_context(
+    const grpc_auth_context *auth_context) {
+  size_t max_num_props = 0;
+  grpc_auth_property_iterator it;
+  const grpc_auth_property *prop;
+  tsi_peer peer;
+  memset(&peer, 0, sizeof(peer));
+
+  it = grpc_auth_context_property_iterator(auth_context);
+  while (grpc_auth_property_iterator_next(&it) != NULL) max_num_props++;
+
+  if (max_num_props > 0) {
+    peer.properties = gpr_malloc(max_num_props * sizeof(tsi_peer_property));
+    it = grpc_auth_context_property_iterator(auth_context);
+    while ((prop = grpc_auth_property_iterator_next(&it)) != NULL) {
+      if (strcmp(prop->name, GRPC_X509_SAN_PROPERTY_NAME) == 0) {
+        add_shalow_auth_property_to_peer(
+            &peer, prop, TSI_X509_SUBJECT_ALTERNATIVE_NAME_PEER_PROPERTY);
+      } else if (strcmp(prop->name, GRPC_X509_CN_PROPERTY_NAME) == 0) {
+        add_shalow_auth_property_to_peer(
+            &peer, prop, TSI_X509_SUBJECT_COMMON_NAME_PEER_PROPERTY);
+      }
+    }
+  }
+  return peer;
+}
+
+void tsi_shallow_peer_destruct(tsi_peer *peer) {
+  if (peer->properties != NULL) gpr_free(peer->properties);
+}
+
+static void ssl_channel_check_call_host(grpc_exec_ctx *exec_ctx,
+                                        grpc_channel_security_connector *sc,
+                                        const char *host,
+                                        grpc_auth_context *auth_context,
+                                        grpc_security_call_host_check_cb cb,
+                                        void *user_data) {
   grpc_ssl_channel_security_connector *c =
       (grpc_ssl_channel_security_connector *)sc;
-
+  grpc_security_status status = GRPC_SECURITY_ERROR;
-  if (ssl_host_matches_name(&c->peer, host)) return GRPC_SECURITY_OK;
+  tsi_peer peer = tsi_shallow_peer_from_ssl_auth_context(auth_context);
+  if (ssl_host_matches_name(&peer, host)) status = GRPC_SECURITY_OK;
 
   /* If the target name was overridden, then the original target_name was
      'checked' transitively during the previous peer check at the end of the
      handshake. */
   if (c->overridden_target_name != NULL && strcmp(host, c->target_name) == 0) {
-    return GRPC_SECURITY_OK;
+    status = GRPC_SECURITY_OK;
-  } else {
-    return GRPC_SECURITY_ERROR;
   }
+  cb(exec_ctx, user_data, status);
+  tsi_shallow_peer_destruct(&peer);
 }
 
 static grpc_security_connector_vtable ssl_channel_vtable = {

src/core/security/security_connector.h

@@ -42,7 +42,6 @@
 
 typedef enum {
   GRPC_SECURITY_OK = 0,
-  GRPC_SECURITY_PENDING,
   GRPC_SECURITY_ERROR
 } grpc_security_status;
 
@@ -124,9 +123,8 @@ void grpc_security_connector_do_handshake(grpc_exec_ctx *exec_ctx,
                                           grpc_security_handshake_done_cb cb,
                                           void *user_data);
 
-/* Check the peer.
+/* Check the peer. Callee takes ownership of the peer object.
-   Ownership of the peer is transfered.
+   The callback will include the resulting auth_context. */
-   TODO(jboeuf): Pass the peer by const pointer and do not pass ownership. */
 void grpc_security_connector_check_peer(grpc_exec_ctx *exec_ctx,
                                         grpc_security_connector *sc,
                                         tsi_peer peer,
@@ -160,30 +158,24 @@ typedef void (*grpc_security_call_host_check_cb)(grpc_exec_ctx *exec_ctx,
 struct grpc_channel_security_connector {
   grpc_security_connector base; /* requires is_client_side to be non 0. */
   grpc_call_credentials *request_metadata_creds;
-  grpc_security_status (*check_call_host)(grpc_exec_ctx *exec_ctx,
+  void (*check_call_host)(grpc_exec_ctx *exec_ctx,
-                                          grpc_channel_security_connector *sc,
+                          grpc_channel_security_connector *sc, const char *host,
-                                          const char *host,
+                          grpc_auth_context *auth_context,
-                                          grpc_security_call_host_check_cb cb,
+                          grpc_security_call_host_check_cb cb, void *user_data);
-                                          void *user_data);
 };
 
-/* Checks that the host that will be set for a call is acceptable.
+/* Checks that the host that will be set for a call is acceptable. */
-   Implementations can choose do the check either synchronously or
+void grpc_channel_security_connector_check_call_host(
-   asynchronously. In the first case, a successful call will return
-   GRPC_SECURITY_OK. In the asynchronous case, the call will return
-   GRPC_SECURITY_PENDING unless an error is detected early on.
-   TODO(jboeuf): add a grpc_auth_context param to test against. */
-grpc_security_status grpc_channel_security_connector_check_call_host(
     grpc_exec_ctx *exec_ctx, grpc_channel_security_connector *sc,
-    const char *host, grpc_security_call_host_check_cb cb, void *user_data);
+    const char *host, grpc_auth_context *auth_context,
+    grpc_security_call_host_check_cb cb, void *user_data);
 
 /* --- Creation security connectors. --- */
 
 /* For TESTING ONLY!
    Creates a fake connector that emulates real channel security.  */
 grpc_channel_security_connector *grpc_fake_channel_security_connector_create(
-    grpc_call_credentials *request_metadata_creds,
+    grpc_call_credentials *request_metadata_creds);
-    int call_host_check_is_async);
 
 /* For TESTING ONLY!
    Creates a fake connector that emulates real server security.  */
@@ -247,5 +239,8 @@ const tsi_peer_property *tsi_peer_get_property_by_name(const tsi_peer *peer,
 
 /* Exposed for testing only. */
 grpc_auth_context *tsi_ssl_peer_to_auth_context(const tsi_peer *peer);
+tsi_peer tsi_shallow_peer_from_auth_context(
+    const grpc_auth_context *auth_context);
+void tsi_shallow_peer_destruct(tsi_peer *peer);
 
 #endif /* GRPC_INTERNAL_CORE_SECURITY_SECURITY_CONNECTOR_H */

test/core/security/security_connector_test.c

@@ -242,6 +242,8 @@ static void test_cn_and_multiple_sans_and_others_ssl_peer_to_auth_context(
   GRPC_AUTH_CONTEXT_UNREF(ctx, "test");
 }
 
+/* TODO(jboeuf): Unit-test tsi_shallow_peer_from_auth_context. */
+
 int main(int argc, char **argv) {
   grpc_test_init(argc, argv);
   grpc_init();