From c52005c161281a4dd429a19aed380af2308dc885 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Sun, 11 Jul 2021 22:10:04 -0700 Subject: [PATCH] NACK xDS updates when certificate provider instace names are unrecognized (#26614) * NACK xDS updates when certificate provider instace names are unrecognized * Reviewer comments * Reviewer comments * Clang format * Fix compilation error --- src/core/ext/xds/certificate_provider_store.h | 2 +- src/core/ext/xds/xds_api.cc | 90 ++++++++++++------- src/core/ext/xds/xds_api.h | 5 +- src/core/ext/xds/xds_client.cc | 3 +- test/cpp/end2end/xds_end2end_test.cc | 44 +++++++-- 5 files changed, 104 insertions(+), 40 deletions(-) diff --git a/src/core/ext/xds/certificate_provider_store.h b/src/core/ext/xds/certificate_provider_store.h index fb6ca72d6d0..b6095e2822b 100644 --- a/src/core/ext/xds/certificate_provider_store.h +++ b/src/core/ext/xds/certificate_provider_store.h @@ -101,7 +101,7 @@ class CertificateProviderStore Mutex mu_; // Map of plugin configurations - PluginDefinitionMap plugin_config_map_ ABSL_GUARDED_BY(mu_); + const PluginDefinitionMap plugin_config_map_; // Underlying map for the providers. std::map certificate_providers_map_ ABSL_GUARDED_BY(mu_); diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index dbcbbd357c0..a221b675969 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -868,10 +868,13 @@ bool IsEds(absl::string_view type_url) { #endif XdsApi::XdsApi(XdsClient* client, TraceFlag* tracer, - const XdsBootstrap::Node* node) + const XdsBootstrap::Node* node, + const CertificateProviderStore::PluginDefinitionMap* + certificate_provider_definition_map) : client_(client), tracer_(tracer), node_(node), + certificate_provider_definition_map_(certificate_provider_definition_map), build_version_(absl::StrCat("gRPC C-core ", GPR_PLATFORM_STRING, " ", grpc_version_string(), GRPC_XDS_USER_AGENT_NAME_SUFFIX_STRING, @@ -903,11 +906,13 @@ XdsApi::XdsApi(XdsClient* client, TraceFlag* tracer, namespace { struct EncodingContext { - XdsClient* client; + XdsClient* client; // Used only for logging. Unsafe for dereferencing. TraceFlag* tracer; upb_symtab* symtab; upb_arena* arena; bool use_v3; + const CertificateProviderStore::PluginDefinitionMap* + certificate_provider_definition_map; }; // Works for both std::string and absl::string_view. @@ -1116,8 +1121,12 @@ grpc_slice XdsApi::CreateAdsRequest( const std::string& version, const std::string& nonce, grpc_error_handle error, bool populate_node) { upb::Arena arena; - const EncodingContext context = {client_, tracer_, symtab_.ptr(), arena.ptr(), - server.ShouldUseV3()}; + const EncodingContext context = {client_, + tracer_, + symtab_.ptr(), + arena.ptr(), + server.ShouldUseV3(), + certificate_provider_definition_map_}; // Create a request. envoy_service_discovery_v3_DiscoveryRequest* request = envoy_service_discovery_v3_DiscoveryRequest_new(arena.ptr()); @@ -1803,24 +1812,32 @@ grpc_error_handle RouteConfigParse( return GRPC_ERROR_NONE; } -XdsApi::CommonTlsContext::CertificateProviderInstance -CertificateProviderInstanceParse( +grpc_error_handle CertificateProviderInstanceParse( + const EncodingContext& context, const envoy_extensions_transport_sockets_tls_v3_CommonTlsContext_CertificateProviderInstance* - certificate_provider_instance_proto) { - return { + certificate_provider_instance_proto, + XdsApi::CommonTlsContext::CertificateProviderInstance* + certificate_provider_instance) { + *certificate_provider_instance = { UpbStringToStdString( envoy_extensions_transport_sockets_tls_v3_CommonTlsContext_CertificateProviderInstance_instance_name( certificate_provider_instance_proto)), UpbStringToStdString( envoy_extensions_transport_sockets_tls_v3_CommonTlsContext_CertificateProviderInstance_certificate_name( certificate_provider_instance_proto))}; + if (context.certificate_provider_definition_map->find( + certificate_provider_instance->instance_name) == + context.certificate_provider_definition_map->end()) { + return GRPC_ERROR_CREATE_FROM_COPIED_STRING( + absl::StrCat("Unrecognized certificate provider instance name: ", + certificate_provider_instance->instance_name) + .c_str()); + } + return GRPC_ERROR_NONE; } grpc_error_handle CommonTlsContextParse( - const envoy_extensions_transport_sockets_tls_v3_CommonTlsContext* - common_tls_context_proto, - XdsApi::CommonTlsContext* common_tls_context) GRPC_MUST_USE_RESULT; -grpc_error_handle CommonTlsContextParse( + const EncodingContext& context, const envoy_extensions_transport_sockets_tls_v3_CommonTlsContext* common_tls_context_proto, XdsApi::CommonTlsContext* common_tls_context) { @@ -1898,19 +1915,21 @@ grpc_error_handle CommonTlsContextParse( envoy_extensions_transport_sockets_tls_v3_CommonTlsContext_CombinedCertificateValidationContext_validation_context_certificate_provider_instance( combined_validation_context); if (validation_context_certificate_provider_instance != nullptr) { - common_tls_context->combined_validation_context - .validation_context_certificate_provider_instance = - CertificateProviderInstanceParse( - validation_context_certificate_provider_instance); + grpc_error_handle error = CertificateProviderInstanceParse( + context, validation_context_certificate_provider_instance, + &common_tls_context->combined_validation_context + .validation_context_certificate_provider_instance); + if (error != GRPC_ERROR_NONE) return error; } } auto* tls_certificate_certificate_provider_instance = envoy_extensions_transport_sockets_tls_v3_CommonTlsContext_tls_certificate_certificate_provider_instance( common_tls_context_proto); if (tls_certificate_certificate_provider_instance != nullptr) { - common_tls_context->tls_certificate_certificate_provider_instance = - CertificateProviderInstanceParse( - tls_certificate_certificate_provider_instance); + grpc_error_handle error = CertificateProviderInstanceParse( + context, tls_certificate_certificate_provider_instance, + &common_tls_context->tls_certificate_certificate_provider_instance); + if (error != GRPC_ERROR_NONE) return error; } return GRPC_ERROR_NONE; } @@ -2100,8 +2119,9 @@ grpc_error_handle DownstreamTlsContextParse( envoy_extensions_transport_sockets_tls_v3_DownstreamTlsContext_common_tls_context( downstream_tls_context_proto); if (common_tls_context != nullptr) { - grpc_error_handle error = CommonTlsContextParse( - common_tls_context, &downstream_tls_context->common_tls_context); + grpc_error_handle error = + CommonTlsContextParse(context, common_tls_context, + &downstream_tls_context->common_tls_context); if (error != GRPC_ERROR_NONE) return error; } auto* require_client_certificate = @@ -3058,7 +3078,7 @@ grpc_error_handle CdsResponseParse( upstream_tls_context); if (common_tls_context != nullptr) { grpc_error_handle error = CommonTlsContextParse( - common_tls_context, &cds_update.common_tls_context); + context, common_tls_context, &cds_update.common_tls_context); if (error != GRPC_ERROR_NONE) { errors.push_back(grpc_error_add_child( GRPC_ERROR_CREATE_FROM_COPIED_STRING( @@ -3416,8 +3436,12 @@ XdsApi::AdsParseResult XdsApi::ParseAdsResponse( const std::set& expected_eds_service_names) { AdsParseResult result; upb::Arena arena; - const EncodingContext context = {client_, tracer_, symtab_.ptr(), arena.ptr(), - server.ShouldUseV3()}; + const EncodingContext context = {client_, + tracer_, + symtab_.ptr(), + arena.ptr(), + server.ShouldUseV3(), + certificate_provider_definition_map_}; // Decode the response. const envoy_service_discovery_v3_DiscoveryResponse* response = envoy_service_discovery_v3_DiscoveryResponse_parse( @@ -3504,8 +3528,12 @@ grpc_slice SerializeLrsRequest( grpc_slice XdsApi::CreateLrsInitialRequest( const XdsBootstrap::XdsServer& server) { upb::Arena arena; - const EncodingContext context = {client_, tracer_, symtab_.ptr(), arena.ptr(), - server.ShouldUseV3()}; + const EncodingContext context = {client_, + tracer_, + symtab_.ptr(), + arena.ptr(), + server.ShouldUseV3(), + certificate_provider_definition_map_}; // Create a request. envoy_service_load_stats_v3_LoadStatsRequest* request = envoy_service_load_stats_v3_LoadStatsRequest_new(arena.ptr()); @@ -3575,8 +3603,9 @@ void LocalityStatsPopulate( grpc_slice XdsApi::CreateLrsRequest( ClusterLoadReportMap cluster_load_report_map) { upb::Arena arena; - const EncodingContext context = {client_, tracer_, symtab_.ptr(), arena.ptr(), - false}; + const EncodingContext context = { + client_, tracer_, symtab_.ptr(), + arena.ptr(), false, certificate_provider_definition_map_}; // Create a request. envoy_service_load_stats_v3_LoadStatsRequest* request = envoy_service_load_stats_v3_LoadStatsRequest_new(arena.ptr()); @@ -3909,8 +3938,9 @@ std::string XdsApi::AssembleClientConfig( // Fill-in the node information auto* node = envoy_service_status_v3_ClientConfig_mutable_node(client_config, arena.ptr()); - const EncodingContext context = {client_, tracer_, symtab_.ptr(), arena.ptr(), - true}; + const EncodingContext context = { + client_, tracer_, symtab_.ptr(), + arena.ptr(), true, certificate_provider_definition_map_}; PopulateNode(context, node_, build_version_, user_agent_name_, user_agent_version_, node); // Dump each xDS-type config into PerXdsConfig diff --git a/src/core/ext/xds/xds_api.h b/src/core/ext/xds/xds_api.h index 49132161cbc..861208d5595 100644 --- a/src/core/ext/xds/xds_api.h +++ b/src/core/ext/xds/xds_api.h @@ -628,7 +628,8 @@ class XdsApi { std::set resource_names_failed; }; - XdsApi(XdsClient* client, TraceFlag* tracer, const XdsBootstrap::Node* node); + XdsApi(XdsClient* client, TraceFlag* tracer, const XdsBootstrap::Node* node, + const CertificateProviderStore::PluginDefinitionMap* map); // Creates an ADS request. // Takes ownership of \a error. @@ -669,6 +670,8 @@ class XdsApi { XdsClient* client_; TraceFlag* tracer_; const XdsBootstrap::Node* node_; // Do not own. + const CertificateProviderStore::PluginDefinitionMap* + certificate_provider_definition_map_; // Do not own. upb::SymbolTable symtab_; const std::string build_version_; const std::string user_agent_name_; diff --git a/src/core/ext/xds/xds_client.cc b/src/core/ext/xds/xds_client.cc index d3549997e9d..f5ae4a61974 100644 --- a/src/core/ext/xds/xds_client.cc +++ b/src/core/ext/xds/xds_client.cc @@ -1803,7 +1803,8 @@ XdsClient::XdsClient(std::unique_ptr bootstrap, interested_parties_(grpc_pollset_set_create()), certificate_provider_store_(MakeOrphanable( bootstrap_->certificate_providers())), - api_(this, &grpc_xds_client_trace, bootstrap_->node()) { + api_(this, &grpc_xds_client_trace, bootstrap_->node(), + &bootstrap_->certificate_providers()) { if (GRPC_TRACE_FLAG_ENABLED(grpc_xds_client_trace)) { gpr_log(GPR_INFO, "[xds_client %p] creating xds client", this); } diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index e9f4c6815db..a4a03b54c7d 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -8029,7 +8029,7 @@ TEST_P( UpstreamTlsContext upstream_tls_context; upstream_tls_context.mutable_common_tls_context() ->mutable_tls_certificate_certificate_provider_instance() - ->set_instance_name(std::string("instance_name")); + ->set_instance_name(std::string("fake_plugin1")); transport_socket->mutable_typed_config()->PackFrom(upstream_tls_context); balancers_[0]->ads_service()->SetCdsResource(cluster); CheckRpcSendFailure(); @@ -8082,7 +8082,13 @@ TEST_P(XdsSecurityTest, UnknownRootCertificateProvider) { ->set_instance_name("unknown"); transport_socket->mutable_typed_config()->PackFrom(upstream_tls_context); balancers_[0]->ads_service()->SetCdsResource(cluster); - CheckRpcSendFailure(1, RpcOptions(), StatusCode::UNAVAILABLE); + CheckRpcSendFailure(); + const auto response_state = + balancers_[0]->ads_service()->cds_response_state(); + EXPECT_EQ(response_state.state, AdsServiceImpl::ResponseState::NACKED); + EXPECT_THAT(response_state.error_message, + ::testing::HasSubstr( + "Unrecognized certificate provider instance name: unknown")); } TEST_P(XdsSecurityTest, UnknownIdentityCertificateProvider) { @@ -8102,7 +8108,13 @@ TEST_P(XdsSecurityTest, UnknownIdentityCertificateProvider) { ->set_instance_name("fake_plugin1"); transport_socket->mutable_typed_config()->PackFrom(upstream_tls_context); balancers_[0]->ads_service()->SetCdsResource(cluster); - CheckRpcSendFailure(1, RpcOptions(), StatusCode::UNAVAILABLE); + CheckRpcSendFailure(); + const auto response_state = + balancers_[0]->ads_service()->cds_response_state(); + EXPECT_EQ(response_state.state, AdsServiceImpl::ResponseState::NACKED); + EXPECT_THAT(response_state.error_message, + ::testing::HasSubstr( + "Unrecognized certificate provider instance name: unknown")); g_fake1_cert_data_map = nullptr; } @@ -9028,14 +9040,32 @@ TEST_P(XdsServerSecurityTest, UnknownIdentityCertificateProvider) { SetLdsUpdate("", "", "unknown", "", false); SendRpc([this]() { return CreateTlsChannel(); }, {}, {}, true /* test_expects_failure */); + do { + CheckRpcSendFailure(); + } while (balancers_[0]->ads_service()->lds_response_state().state == + AdsServiceImpl::ResponseState::SENT); + const auto response_state = + balancers_[0]->ads_service()->lds_response_state(); + EXPECT_EQ(response_state.state, AdsServiceImpl::ResponseState::NACKED); + EXPECT_THAT(response_state.error_message, + ::testing::HasSubstr( + "Unrecognized certificate provider instance name: unknown")); } TEST_P(XdsServerSecurityTest, UnknownRootCertificateProvider) { FakeCertificateProvider::CertDataMap fake1_cert_map = { {"", {root_cert_, identity_pair_}}}; SetLdsUpdate("unknown", "", "fake_plugin1", "", false); - SendRpc([this]() { return CreateTlsChannel(); }, {}, {}, - true /* test_expects_failure */); + do { + CheckRpcSendFailure(); + } while (balancers_[0]->ads_service()->lds_response_state().state == + AdsServiceImpl::ResponseState::SENT); + const auto response_state = + balancers_[0]->ads_service()->lds_response_state(); + EXPECT_EQ(response_state.state, AdsServiceImpl::ResponseState::NACKED); + EXPECT_THAT(response_state.error_message, + ::testing::HasSubstr( + "Unrecognized certificate provider instance name: unknown")); } TEST_P(XdsServerSecurityTest, CertificatesNotAvailable) { @@ -9767,7 +9797,7 @@ TEST_P(XdsServerFilterChainMatchTest, for (int i = 1; i < 65536; i++) { filter_chain->mutable_filter_chain_match()->add_source_ports(i); } - // Add another filter chain with no source prefix range mentioned with a bad + // Add another filter chain with no source port mentioned with a bad // DownstreamTlsContext configuration. filter_chain = listener.add_filter_chains(); filter_chain->add_filters()->mutable_typed_config()->PackFrom( @@ -9777,7 +9807,7 @@ TEST_P(XdsServerFilterChainMatchTest, DownstreamTlsContext downstream_tls_context; downstream_tls_context.mutable_common_tls_context() ->mutable_tls_certificate_certificate_provider_instance() - ->set_instance_name("unknown"); + ->set_instance_name("fake_plugin1"); transport_socket->mutable_typed_config()->PackFrom(downstream_tls_context); balancers_[0]->ads_service()->SetLdsResource(listener); // A successful RPC proves that the filter chain with matching source port