From e8f17c5f9610451400cdbf68155aa586665ff740 Mon Sep 17 00:00:00 2001 From: jiangtaoli2016 Date: Tue, 23 Jun 2020 22:43:41 -0700 Subject: [PATCH] Fix SPIFFE ID check --- .../security/security_connector/ssl_utils.cc | 27 ++++++++------- test/core/security/security_connector_test.cc | 34 ++++++++++++++----- test/core/tsi/ssl_transport_security_test.cc | 1 + 3 files changed, 41 insertions(+), 21 deletions(-) diff --git a/src/core/lib/security/security_connector/ssl_utils.cc b/src/core/lib/security/security_connector/ssl_utils.cc index 664f3a18e61..3aa12a8f1d8 100644 --- a/src/core/lib/security/security_connector/ssl_utils.cc +++ b/src/core/lib/security/security_connector/ssl_utils.cc @@ -257,7 +257,8 @@ grpc_core::RefCountedPtr 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_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_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; } diff --git a/test/core/security/security_connector_test.cc b/test/core/security/security_connector_test.cc index 8f249bec97b..c6b760be18a 100644 --- a/test/core/security/security_connector_test.cc +++ b/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 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 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 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 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"; diff --git a/test/core/tsi/ssl_transport_security_test.cc b/test/core/tsi/ssl_transport_security_test.cc index 8c4e736fc8b..26d4d477050 100644 --- a/test/core/tsi/ssl_transport_security_test.cc +++ b/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);