Merge pull request #23295 from jiangtaoli2016/spiffe

Update SPIFFE ID check

src/core/lib/security/security_connector/ssl_utils.cc

@@ -257,7 +257,8 @@ grpc_core::RefCountedPtr<grpc_auth_context> grpc_ssl_peer_to_auth_context(
       transport_security_type);
   const char* spiffe_data = nullptr;
   size_t spiffe_length = 0;
-  int spiffe_id_count = 0;
+  int uri_count = 0;
+  bool has_spiffe_id = false;
   for (i = 0; i < peer->property_count; i++) {
     const tsi_peer_property* prop = &peer->properties[i];
     if (prop->name == nullptr) continue;
@@ -290,11 +291,12 @@ grpc_core::RefCountedPtr<grpc_auth_context> grpc_ssl_peer_to_auth_context(
           ctx.get(), GRPC_TRANSPORT_SECURITY_LEVEL_PROPERTY_NAME,
           prop->value.data, prop->value.length);
     } else if (strcmp(prop->name, TSI_X509_URI_PEER_PROPERTY) == 0) {
+      uri_count++;
       absl::string_view spiffe_id(prop->value.data, prop->value.length);
       if (IsSpiffeId(spiffe_id)) {
         spiffe_data = prop->value.data;
         spiffe_length = prop->value.length;
-        spiffe_id_count += 1;
+        has_spiffe_id = true;
       }
     }
   }
@@ -302,16 +304,17 @@ grpc_core::RefCountedPtr<grpc_auth_context> grpc_ssl_peer_to_auth_context(
     GPR_ASSERT(grpc_auth_context_set_peer_identity_property_name(
                    ctx.get(), peer_identity_property_name) == 1);
   }
-  // SPIFFE ID should be unique. If we find more than one SPIFFE IDs, we log
-  // the error without returning the error.
-  if (spiffe_id_count > 1) {
-    gpr_log(GPR_INFO, "Invalid SPIFFE ID: SPIFFE ID should be unique.");
-  }
-  if (spiffe_id_count == 1) {
-    GPR_ASSERT(spiffe_length > 0);
-    GPR_ASSERT(spiffe_data != nullptr);
-    grpc_auth_context_add_property(ctx.get(), GRPC_PEER_SPIFFE_ID_PROPERTY_NAME,
-                                   spiffe_data, spiffe_length);
+  // A valid SPIFFE certificate can only have exact one URI SAN field.
+  if (has_spiffe_id) {
+    if (uri_count == 1) {
+      GPR_ASSERT(spiffe_length > 0);
+      GPR_ASSERT(spiffe_data != nullptr);
+      grpc_auth_context_add_property(ctx.get(),
+                                     GRPC_PEER_SPIFFE_ID_PROPERTY_NAME,
+                                     spiffe_data, spiffe_length);
+    } else {
+      gpr_log(GPR_INFO, "Invalid SPIFFE ID: multiple URI SANs.");
+    }
   }
   return ctx;
 }

test/core/security/security_connector_test.cc

@@ -472,16 +472,13 @@ static void test_spiffe_id_peer_to_auth_context(void) {
   GPR_ASSERT(check_spiffe_id(invalid_ctx.get(), nullptr, false));
   tsi_peer_destruct(&invalid_peer);
   invalid_ctx.reset(DEBUG_LOCATION, "test");
-  // A valid SPIFFE ID with other URI fields should be plumbed.
+  // A valid SPIFFE ID should be plumbed.
   tsi_peer valid_peer;
-  std::vector<std::string> valid_spiffe_id = {"spiffe://foo.bar.com/wl",
-                                              "https://xyz"};
-  GPR_ASSERT(tsi_construct_peer(valid_spiffe_id.size(), &valid_peer) == TSI_OK);
-  for (i = 0; i < valid_spiffe_id.size(); i++) {
-    GPR_ASSERT(tsi_construct_string_peer_property_from_cstring(
-                   TSI_X509_URI_PEER_PROPERTY, valid_spiffe_id[i].c_str(),
-                   &valid_peer.properties[i]) == TSI_OK);
-  }
+  std::string valid_spiffe_id = "spiffe://foo.bar.com/wl";
+  GPR_ASSERT(tsi_construct_peer(1, &valid_peer) == TSI_OK);
+  GPR_ASSERT(tsi_construct_string_peer_property_from_cstring(
+                 TSI_X509_URI_PEER_PROPERTY, valid_spiffe_id.c_str(),
+                 &valid_peer.properties[0]) == TSI_OK);
   grpc_core::RefCountedPtr<grpc_auth_context> valid_ctx =
       grpc_ssl_peer_to_auth_context(&valid_peer,
                                     GRPC_SSL_TRANSPORT_SECURITY_TYPE);
@@ -507,6 +504,25 @@ static void test_spiffe_id_peer_to_auth_context(void) {
   GPR_ASSERT(check_spiffe_id(multiple_ctx.get(), nullptr, false));
   tsi_peer_destruct(&multiple_peer);
   multiple_ctx.reset(DEBUG_LOCATION, "test");
+  // A valid SPIFFE certificate should only has one URI SAN field.
+  // SPIFFE ID should not be plumbed if there are multiple URIs.
+  tsi_peer multiple_uri_peer;
+  std::vector<std::string> multiple_uri = {"spiffe://foo.bar.com/wl",
+                                           "https://xyz", "ssh://foo.bar.com/"};
+  GPR_ASSERT(tsi_construct_peer(multiple_uri.size(), &multiple_uri_peer) ==
+             TSI_OK);
+  for (i = 0; i < multiple_spiffe_id.size(); i++) {
+    GPR_ASSERT(tsi_construct_string_peer_property_from_cstring(
+                   TSI_X509_URI_PEER_PROPERTY, multiple_uri[i].c_str(),
+                   &multiple_uri_peer.properties[i]) == TSI_OK);
+  }
+  grpc_core::RefCountedPtr<grpc_auth_context> multiple_uri_ctx =
+      grpc_ssl_peer_to_auth_context(&multiple_uri_peer,
+                                    GRPC_SSL_TRANSPORT_SECURITY_TYPE);
+  GPR_ASSERT(multiple_uri_ctx != nullptr);
+  GPR_ASSERT(check_spiffe_id(multiple_uri_ctx.get(), nullptr, false));
+  tsi_peer_destruct(&multiple_uri_peer);
+  multiple_uri_ctx.reset(DEBUG_LOCATION, "test");
 }
 
 static const char* roots_for_override_api = "roots for override api";

test/core/tsi/ssl_transport_security_test.cc

@@ -895,6 +895,7 @@ void ssl_tsi_test_extract_x509_subject_names() {
   GPR_ASSERT(check_subject_alt_name(&peer, "foo.test.domain.com") == 1);
   GPR_ASSERT(check_subject_alt_name(&peer, "bar.test.domain.com") == 1);
   // Check URI
+  // Note that a valid SPIFFE certificate should only have one URI.
   GPR_ASSERT(check_subject_alt_name(&peer, "spiffe://foo.com/bar/baz") == 1);
   GPR_ASSERT(
       check_subject_alt_name(&peer, "https://foo.test.domain.com/test") == 1);