From cf68bc6da44df08cde2c713c4ac9bbe374c11fdb Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 22 Aug 2018 11:56:48 -0700 Subject: [PATCH 1/5] Add Sds api and dummy socket. Signed-off-by: JimmyCYJ --- include/envoy/secret/BUILD | 6 +++ include/envoy/secret/secret_provider.h | 14 ++++++- source/common/secret/BUILD | 19 ++++++++++ source/common/secret/secret_provider_impl.h | 4 ++ source/common/ssl/ssl_socket.cc | 41 ++++++++++++++++++++- test/common/secret/BUILD | 19 ++++++++++ test/mocks/secret/BUILD | 1 + test/mocks/secret/mocks.cc | 4 ++ test/mocks/secret/mocks.h | 7 ++++ 9 files changed, 112 insertions(+), 3 deletions(-) diff --git a/include/envoy/secret/BUILD b/include/envoy/secret/BUILD index 6124d51a3210..2353a9df31d4 100644 --- a/include/envoy/secret/BUILD +++ b/include/envoy/secret/BUILD @@ -8,10 +8,16 @@ load( envoy_package() +envoy_cc_library( + name = "secret_callbacks_interface", + hdrs = ["secret_callbacks.h"], +) + envoy_cc_library( name = "secret_provider_interface", hdrs = ["secret_provider.h"], deps = [ + ":secret_callbacks_interface", "//include/envoy/ssl:tls_certificate_config_interface", ], ) diff --git a/include/envoy/secret/secret_provider.h b/include/envoy/secret/secret_provider.h index b6f9f26ca76c..282d54c2f976 100644 --- a/include/envoy/secret/secret_provider.h +++ b/include/envoy/secret/secret_provider.h @@ -1,6 +1,7 @@ #pragma once #include "envoy/common/pure.h" +#include "envoy/secret/secret_callbacks.h" #include "envoy/ssl/tls_certificate_config.h" namespace Envoy { @@ -18,7 +19,18 @@ template class SecretProvider { */ virtual const SecretType* secret() const PURE; - // TODO(lizan): Add more methods for dynamic secret provider. + /** + * Add secret callback into secret provider. + * It is safe to call this method by main thread and is safe to be invoked + * on main thread. + * @param callback callback that is executed by secret provider. + */ + virtual void addUpdateCallback(SecretCallbacks& callback) PURE; + /** + * Remove secret callback from secret provider. + * @param callback callback that is executed by secret provider. + */ + virtual void removeUpdateCallback(SecretCallbacks& callback) PURE; }; typedef SecretProvider TlsCertificateConfigProvider; diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 598d8249fa5e..c514d1e06ca5 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -30,3 +30,22 @@ envoy_cc_library( "@envoy_api//envoy/api/v2/auth:cert_cc", ], ) + +envoy_cc_library( + name = "sds_api_lib", + srcs = ["sds_api.cc"], + hdrs = ["sds_api.h"], + deps = [ + "//include/envoy/config:subscription_interface", + "//include/envoy/event:dispatcher_interface", + "//include/envoy/init:init_interface", + "//include/envoy/local_info:local_info_interface", + "//include/envoy/runtime:runtime_interface", + "//include/envoy/secret:secret_provider_interface", + "//include/envoy/stats:stats_interface", + "//source/common/config:resources_lib", + "//source/common/config:subscription_factory_lib", + "//source/common/protobuf:utility_lib", + "//source/common/ssl:tls_certificate_config_impl_lib", + ], +) diff --git a/source/common/secret/secret_provider_impl.h b/source/common/secret/secret_provider_impl.h index 9ac79c66009c..e6731e0b0af8 100644 --- a/source/common/secret/secret_provider_impl.h +++ b/source/common/secret/secret_provider_impl.h @@ -13,6 +13,10 @@ class TlsCertificateConfigProviderImpl : public TlsCertificateConfigProvider { const Ssl::TlsCertificateConfig* secret() const override { return tls_certificate_.get(); } + void addUpdateCallback(SecretCallbacks&) override {} + + void removeUpdateCallback(SecretCallbacks&) override {} + private: Ssl::TlsCertificateConfigPtr tls_certificate_; }; diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 3f634c7d9fa2..f6cf356c212d 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -17,6 +17,35 @@ using Envoy::Network::PostIoAction; namespace Envoy { namespace Ssl { +namespace { +// This SslSocket will be used when SSL secret is not fetched from SDS server. +class NotReadySslSocket : public Network::TransportSocket, public Connection { +public: + // Ssl::Connection + bool peerCertificatePresented() const override { return false; } + std::string uriSanLocalCertificate() const override { return EMPTY_STRING; } + const std::string& sha256PeerCertificateDigest() const override { return EMPTY_STRING; } + std::string serialNumberPeerCertificate() const override { return EMPTY_STRING; } + std::string subjectPeerCertificate() const override { return EMPTY_STRING; } + std::string subjectLocalCertificate() const override { return EMPTY_STRING; } + std::string uriSanPeerCertificate() const override { return EMPTY_STRING; } + const std::string& urlEncodedPemEncodedPeerCertificate() const override { return EMPTY_STRING; } + std::vector dnsSansPeerCertificate() const override { return {}; } + std::vector dnsSansLocalCertificate() const override { return {}; } + // Network::TransportSocket + void setTransportSocketCallbacks(Network::TransportSocketCallbacks&) override {} + std::string protocol() const override { return EMPTY_STRING; } + bool canFlushClose() override { return true; } + void closeSocket(Network::ConnectionEvent) override {} + Network::IoResult doRead(Buffer::Instance&) override { return {PostIoAction::Close, 0, false}; } + Network::IoResult doWrite(Buffer::Instance&, bool) override { + return {PostIoAction::Close, 0, false}; + } + void onConnected() override {} + const Ssl::Connection* ssl() const override { return this; } +}; +} // namespace + SslSocket::SslSocket(ContextSharedPtr ctx, InitialState state) : ctx_(std::dynamic_pointer_cast(ctx)), ssl_(ctx_->newSsl()) { if (state == InitialState::Client) { @@ -391,7 +420,11 @@ ClientSslSocketFactory::ClientSslSocketFactory(ClientContextConfigPtr config, ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) {} Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const { - return std::make_unique(ssl_ctx_, Ssl::InitialState::Client); + if (ssl_ctx_) { + return std::make_unique(ssl_ctx_, Ssl::InitialState::Client); + } else { + return std::make_unique(); + } } bool ClientSslSocketFactory::implementsSecureTransport() const { return true; } @@ -405,7 +438,11 @@ ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config, ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_)) {} Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const { - return std::make_unique(ssl_ctx_, Ssl::InitialState::Server); + if (ssl_ctx_) { + return std::make_unique(ssl_ctx_, Ssl::InitialState::Server); + } else { + return std::make_unique(); + } } bool ServerSslSocketFactory::implementsSecureTransport() const { return true; } diff --git a/test/common/secret/BUILD b/test/common/secret/BUILD index c65489952a84..7f6013a3db8d 100644 --- a/test/common/secret/BUILD +++ b/test/common/secret/BUILD @@ -21,3 +21,22 @@ envoy_cc_test( "//test/test_common:utility_lib", ], ) + +envoy_cc_test( + name = "sds_api_test", + srcs = ["sds_api_test.cc"], + data = [ + "//test/common/ssl/test_data:certs", + ], + deps = [ + "//source/common/secret:sds_api_lib", + "//test/mocks/grpc:grpc_mocks", + "//test/mocks/init:init_mocks", + "//test/mocks/secret:secret_mocks", + "//test/mocks/server:server_mocks", + "//test/test_common:environment_lib", + "//test/test_common:registry_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/service/discovery/v2:sds_cc", + ], +) diff --git a/test/mocks/secret/BUILD b/test/mocks/secret/BUILD index e01f07aaae59..d3283e265d0d 100644 --- a/test/mocks/secret/BUILD +++ b/test/mocks/secret/BUILD @@ -13,6 +13,7 @@ envoy_cc_mock( srcs = ["mocks.cc"], hdrs = ["mocks.h"], deps = [ + "//include/envoy/secret:secret_callbacks_interface", "//include/envoy/secret:secret_manager_interface", "//include/envoy/ssl:tls_certificate_config_interface", "//source/common/secret:secret_provider_impl_lib", diff --git a/test/mocks/secret/mocks.cc b/test/mocks/secret/mocks.cc index 3de2c5d039bb..7461830a5693 100644 --- a/test/mocks/secret/mocks.cc +++ b/test/mocks/secret/mocks.cc @@ -17,5 +17,9 @@ MockSecretManager::MockSecretManager() { MockSecretManager::~MockSecretManager() {} +MockSecretCallbacks::MockSecretCallbacks() {} + +MockSecretCallbacks::~MockSecretCallbacks() {} + } // namespace Secret } // namespace Envoy diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index 212cc8985cbb..9f41891d4950 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -22,5 +22,12 @@ class MockSecretManager : public SecretManager { const envoy::api::v2::auth::TlsCertificate& tls_certificate)); }; +class MockSecretCallbacks : public SecretCallbacks { +public: + MockSecretCallbacks(); + ~MockSecretCallbacks(); + MOCK_METHOD0(onAddOrUpdateSecret, void()); +}; + } // namespace Secret } // namespace Envoy From b3d06808f20ce00d251f619ca936618ac09d0c7e Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Wed, 22 Aug 2018 11:57:19 -0700 Subject: [PATCH 2/5] Add more files. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_callbacks.h | 18 +++ source/common/secret/sds_api.cc | 89 +++++++++++++ source/common/secret/sds_api.h | 80 ++++++++++++ test/common/secret/sds_api_test.cc | 163 ++++++++++++++++++++++++ 4 files changed, 350 insertions(+) create mode 100644 include/envoy/secret/secret_callbacks.h create mode 100644 source/common/secret/sds_api.cc create mode 100644 source/common/secret/sds_api.h create mode 100644 test/common/secret/sds_api_test.cc diff --git a/include/envoy/secret/secret_callbacks.h b/include/envoy/secret/secret_callbacks.h new file mode 100644 index 000000000000..f27cd7583b37 --- /dev/null +++ b/include/envoy/secret/secret_callbacks.h @@ -0,0 +1,18 @@ +#pragma once + +#include "envoy/common/pure.h" + +namespace Envoy { +namespace Secret { + +/** + * Callbacks invoked by a dynamic secret provider. + */ +class SecretCallbacks { +public: + virtual ~SecretCallbacks() {} + virtual void onAddOrUpdateSecret() PURE; +}; + +} // namespace Secret +} // namespace Envoy diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc new file mode 100644 index 000000000000..3181795f7f3f --- /dev/null +++ b/source/common/secret/sds_api.cc @@ -0,0 +1,89 @@ +#include "common/secret/sds_api.h" + +#include + +#include "envoy/api/v2/auth/cert.pb.validate.h" + +#include "common/config/resources.h" +#include "common/config/subscription_factory.h" +#include "common/protobuf/utility.h" +#include "common/ssl/tls_certificate_config_impl.h" + +namespace Envoy { +namespace Secret { + +SdsApi::SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, + Runtime::RandomGenerator& random, Stats::Store& stats, + Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, + const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name, + std::function unregister_secret_provider) + : local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats), + cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name), + secret_hash_(0), unregister_secret_provider_cb_(unregister_secret_provider) { + // TODO(JimmyCYJ): Implement chained_init_manager, so that multiple init_manager + // can be chained together to behave as one init_manager. In that way, we let + // two listeners which share same SdsApi to register at separate init managers, and + // each init manager has a chance to initialize its targets. + init_manager.registerTarget(*this); +} + +SdsApi::~SdsApi() { unregister_secret_provider_cb_(); } + +void SdsApi::initialize(std::function callback) { + initialize_callback_ = callback; + + subscription_ = Envoy::Config::SubscriptionFactory::subscriptionFromConfigSource< + envoy::api::v2::auth::Secret>( + sds_config_, local_info_, dispatcher_, cluster_manager_, random_, stats_, + /* rest_legacy_constructor */ nullptr, + "envoy.service.discovery.v2.SecretDiscoveryService.FetchSecrets", + "envoy.service.discovery.v2.SecretDiscoveryService.StreamSecrets"); + Config::Utility::checkLocalInfo("sds", local_info_); + + subscription_->start({sds_config_name_}, *this); +} + +void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) { + if (resources.empty()) { + throw EnvoyException( + fmt::format("Missing SDS resources for {} in onConfigUpdate()", sds_config_name_)); + } + if (resources.size() != 1) { + throw EnvoyException(fmt::format("Unexpected SDS secrets length: {}", resources.size())); + } + const auto& secret = resources[0]; + MessageUtil::validate(secret); + if (secret.name() != sds_config_name_) { + throw EnvoyException( + fmt::format("Unexpected SDS secret (expecting {}): {}", sds_config_name_, secret.name())); + } + + const uint64_t new_hash = MessageUtil::hash(secret); + if (new_hash != secret_hash_ && + secret.type_case() == envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate) { + secret_hash_ = new_hash; + tls_certificate_secrets_ = + std::make_unique(secret.tls_certificate()); + + for (auto cb : update_callbacks_) { + cb->onAddOrUpdateSecret(); + } + } + + runInitializeCallbackIfAny(); +} + +void SdsApi::onConfigUpdateFailed(const EnvoyException*) { + // We need to allow server startup to continue, even if we have a bad config. + runInitializeCallbackIfAny(); +} + +void SdsApi::runInitializeCallbackIfAny() { + if (initialize_callback_) { + initialize_callback_(); + initialize_callback_ = nullptr; + } +} + +} // namespace Secret +} // namespace Envoy diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h new file mode 100644 index 000000000000..a26a1f47f101 --- /dev/null +++ b/source/common/secret/sds_api.h @@ -0,0 +1,80 @@ +#pragma once + +#include + +#include "envoy/api/v2/auth/cert.pb.h" +#include "envoy/api/v2/core/config_source.pb.h" +#include "envoy/config/subscription.h" +#include "envoy/event/dispatcher.h" +#include "envoy/init/init.h" +#include "envoy/local_info/local_info.h" +#include "envoy/runtime/runtime.h" +#include "envoy/secret/secret_callbacks.h" +#include "envoy/secret/secret_provider.h" +#include "envoy/stats/stats.h" +#include "envoy/upstream/cluster_manager.h" + +namespace Envoy { +namespace Secret { + +/** + * SDS API implementation that fetches secrets from SDS server via Subscription. + */ +class SdsApi : public Init::Target, + public TlsCertificateConfigProvider, + public Config::SubscriptionCallbacks { +public: + SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, + Runtime::RandomGenerator& random, Stats::Store& stats, + Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, + const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name, + std::function unregister_secret_provider); + + ~SdsApi() override; + + // Init::Target + void initialize(std::function callback) override; + + // Config::SubscriptionCallbacks + void onConfigUpdate(const ResourceVector& resources, const std::string& version_info) override; + void onConfigUpdateFailed(const EnvoyException* e) override; + std::string resourceName(const ProtobufWkt::Any& resource) override { + return MessageUtil::anyConvert(resource).name(); + } + + // SecretProvider + const Ssl::TlsCertificateConfig* secret() const override { + return tls_certificate_secrets_.get(); + } + + void addUpdateCallback(SecretCallbacks& callback) override { + update_callbacks_.push_back(&callback); + } + void removeUpdateCallback(SecretCallbacks& callback) override { + update_callbacks_.remove(&callback); + } + +private: + void runInitializeCallbackIfAny(); + + const LocalInfo::LocalInfo& local_info_; + Event::Dispatcher& dispatcher_; + Runtime::RandomGenerator& random_; + Stats::Store& stats_; + Upstream::ClusterManager& cluster_manager_; + + const envoy::api::v2::core::ConfigSource sds_config_; + std::unique_ptr> subscription_; + std::function initialize_callback_; + const std::string sds_config_name_; + + uint64_t secret_hash_; + std::function unregister_secret_provider_cb_; + Ssl::TlsCertificateConfigPtr tls_certificate_secrets_; + std::list update_callbacks_; +}; + +typedef std::unique_ptr SdsApiPtr; + +} // namespace Secret +} // namespace Envoy diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc new file mode 100644 index 000000000000..9539e8c609fa --- /dev/null +++ b/test/common/secret/sds_api_test.cc @@ -0,0 +1,163 @@ +#include + +#include "envoy/api/v2/auth/cert.pb.h" +#include "envoy/common/exception.h" +#include "envoy/service/discovery/v2/sds.pb.h" + +#include "common/secret/sds_api.h" + +#include "test/mocks/grpc/mocks.h" +#include "test/mocks/init/mocks.h" +#include "test/mocks/secret/mocks.h" +#include "test/mocks/server/mocks.h" +#include "test/test_common/environment.h" +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +using ::testing::_; +using ::testing::Invoke; +using ::testing::Return; + +namespace Envoy { +namespace Secret { +namespace { + +class SdsApiTest : public testing::Test {}; + +TEST_F(SdsApiTest, BasicTest) { + ::testing::InSequence s; + const envoy::service::discovery::v2::SdsDummy dummy; + NiceMock server; + NiceMock init_manager; + EXPECT_CALL(init_manager, registerTarget(_)); + + envoy::api::v2::core::ConfigSource config_source; + config_source.mutable_api_config_source()->set_api_type( + envoy::api::v2::core::ApiConfigSource::GRPC); + auto grpc_service = config_source.mutable_api_config_source()->add_grpc_services(); + auto google_grpc = grpc_service->mutable_google_grpc(); + google_grpc->set_target_uri("fake_address"); + google_grpc->set_stat_prefix("test"); + Secret::SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), + server.clusterManager(), init_manager, config_source, "abc.com", []() {}); + + NiceMock* grpc_client{new NiceMock()}; + NiceMock* factory{new NiceMock()}; + EXPECT_CALL(server.cluster_manager_.async_client_manager_, factoryForGrpcService(_, _, _)) + .WillOnce(Invoke([factory](const envoy::api::v2::core::GrpcService&, Stats::Scope&, bool) { + return Grpc::AsyncClientFactoryPtr{factory}; + })); + EXPECT_CALL(*factory, create()).WillOnce(Invoke([grpc_client] { + return Grpc::AsyncClientPtr{grpc_client}; + })); + EXPECT_CALL(init_manager.initialized_, ready()); + init_manager.initialize(); +} + +TEST_F(SdsApiTest, SecretUpdateSuccess) { + NiceMock server; + NiceMock init_manager; + envoy::api::v2::core::ConfigSource config_source; + SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), + server.clusterManager(), init_manager, config_source, "abc.com", []() {}); + + NiceMock secret_callback; + sds_api.addUpdateCallback(secret_callback); + + std::string yaml = + R"EOF( + name: "abc.com" + tls_certificate: + certificate_chain: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" + )EOF"; + + Protobuf::RepeatedPtrField secret_resources; + auto secret_config = secret_resources.Add(); + MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), *secret_config); + EXPECT_CALL(secret_callback, onAddOrUpdateSecret()); + sds_api.onConfigUpdate(secret_resources, ""); + + const std::string cert_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(cert_pem)), + sds_api.secret()->certificateChain()); + + const std::string key_pem = "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem"; + EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)), + sds_api.secret()->privateKey()); + + sds_api.removeUpdateCallback(secret_callback); +} + +TEST_F(SdsApiTest, EmptyResource) { + NiceMock server; + NiceMock init_manager; + envoy::api::v2::core::ConfigSource config_source; + SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), + server.clusterManager(), init_manager, config_source, "abc.com", []() {}); + + Protobuf::RepeatedPtrField secret_resources; + + EXPECT_THROW_WITH_MESSAGE(sds_api.onConfigUpdate(secret_resources, ""), EnvoyException, + "Missing SDS resources for abc.com in onConfigUpdate()"); +} + +TEST_F(SdsApiTest, SecretUpdateWrongSize) { + NiceMock server; + NiceMock init_manager; + envoy::api::v2::core::ConfigSource config_source; + SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), + server.clusterManager(), init_manager, config_source, "abc.com", []() {}); + + std::string yaml = + R"EOF( + name: "abc.com" + tls_certificate: + certificate_chain: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" + )EOF"; + + Protobuf::RepeatedPtrField secret_resources; + auto secret_config_1 = secret_resources.Add(); + MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), *secret_config_1); + auto secret_config_2 = secret_resources.Add(); + MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), *secret_config_2); + + EXPECT_THROW_WITH_MESSAGE(sds_api.onConfigUpdate(secret_resources, ""), EnvoyException, + "Unexpected SDS secrets length: 2"); +} + +TEST_F(SdsApiTest, SecretUpdateWrongSecretName) { + NiceMock server; + NiceMock init_manager; + envoy::api::v2::core::ConfigSource config_source; + SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), + server.clusterManager(), init_manager, config_source, "abc.com", []() {}); + + std::string yaml = + R"EOF( + name: "wrong.name.com" + tls_certificate: + certificate_chain: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" + )EOF"; + + Protobuf::RepeatedPtrField secret_resources; + auto secret_config = secret_resources.Add(); + MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), *secret_config); + + EXPECT_THROW_WITH_MESSAGE(sds_api.onConfigUpdate(secret_resources, ""), EnvoyException, + "Unexpected SDS secret (expecting abc.com): wrong.name.com"); +} + +} // namespace +} // namespace Secret +} // namespace Envoy From 0766ff5e4ab365ccef1493a1526434cb45db3d8f Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 23 Aug 2018 11:54:09 -0700 Subject: [PATCH 3/5] Revise per comments. Signed-off-by: JimmyCYJ --- include/envoy/secret/secret_provider.h | 1 + source/common/secret/sds_api.cc | 7 ++----- source/common/secret/sds_api.h | 6 +----- source/common/ssl/ssl_socket.cc | 15 ++------------- test/common/secret/sds_api_test.cc | 18 ++++++++++++------ 5 files changed, 18 insertions(+), 29 deletions(-) diff --git a/include/envoy/secret/secret_provider.h b/include/envoy/secret/secret_provider.h index 282d54c2f976..f5be69a4343e 100644 --- a/include/envoy/secret/secret_provider.h +++ b/include/envoy/secret/secret_provider.h @@ -26,6 +26,7 @@ template class SecretProvider { * @param callback callback that is executed by secret provider. */ virtual void addUpdateCallback(SecretCallbacks& callback) PURE; + /** * Remove secret callback from secret provider. * @param callback callback that is executed by secret provider. diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 3181795f7f3f..beb0ad9db4ca 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -15,11 +15,10 @@ namespace Secret { SdsApi::SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, Stats::Store& stats, Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, - const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name, - std::function unregister_secret_provider) + const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name) : local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats), cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name), - secret_hash_(0), unregister_secret_provider_cb_(unregister_secret_provider) { + secret_hash_(0) { // TODO(JimmyCYJ): Implement chained_init_manager, so that multiple init_manager // can be chained together to behave as one init_manager. In that way, we let // two listeners which share same SdsApi to register at separate init managers, and @@ -27,8 +26,6 @@ SdsApi::SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispat init_manager.registerTarget(*this); } -SdsApi::~SdsApi() { unregister_secret_provider_cb_(); } - void SdsApi::initialize(std::function callback) { initialize_callback_ = callback; diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index a26a1f47f101..dcf09959e28a 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -27,10 +27,7 @@ class SdsApi : public Init::Target, SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, Stats::Store& stats, Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, - const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name, - std::function unregister_secret_provider); - - ~SdsApi() override; + const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name); // Init::Target void initialize(std::function callback) override; @@ -69,7 +66,6 @@ class SdsApi : public Init::Target, const std::string sds_config_name_; uint64_t secret_hash_; - std::function unregister_secret_provider_cb_; Ssl::TlsCertificateConfigPtr tls_certificate_secrets_; std::list update_callbacks_; }; diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index f6cf356c212d..27e149dd0c63 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -19,19 +19,8 @@ namespace Ssl { namespace { // This SslSocket will be used when SSL secret is not fetched from SDS server. -class NotReadySslSocket : public Network::TransportSocket, public Connection { +class NotReadySslSocket : public Network::TransportSocket { public: - // Ssl::Connection - bool peerCertificatePresented() const override { return false; } - std::string uriSanLocalCertificate() const override { return EMPTY_STRING; } - const std::string& sha256PeerCertificateDigest() const override { return EMPTY_STRING; } - std::string serialNumberPeerCertificate() const override { return EMPTY_STRING; } - std::string subjectPeerCertificate() const override { return EMPTY_STRING; } - std::string subjectLocalCertificate() const override { return EMPTY_STRING; } - std::string uriSanPeerCertificate() const override { return EMPTY_STRING; } - const std::string& urlEncodedPemEncodedPeerCertificate() const override { return EMPTY_STRING; } - std::vector dnsSansPeerCertificate() const override { return {}; } - std::vector dnsSansLocalCertificate() const override { return {}; } // Network::TransportSocket void setTransportSocketCallbacks(Network::TransportSocketCallbacks&) override {} std::string protocol() const override { return EMPTY_STRING; } @@ -42,7 +31,7 @@ class NotReadySslSocket : public Network::TransportSocket, public Connection { return {PostIoAction::Close, 0, false}; } void onConnected() override {} - const Ssl::Connection* ssl() const override { return this; } + const Ssl::Connection* ssl() const override { return nullptr; } }; } // namespace diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 9539e8c609fa..e721b88ebcc2 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -26,6 +26,7 @@ namespace { class SdsApiTest : public testing::Test {}; +// Validate that SdsApi object is created and initialized successfully. TEST_F(SdsApiTest, BasicTest) { ::testing::InSequence s; const envoy::service::discovery::v2::SdsDummy dummy; @@ -40,8 +41,8 @@ TEST_F(SdsApiTest, BasicTest) { auto google_grpc = grpc_service->mutable_google_grpc(); google_grpc->set_target_uri("fake_address"); google_grpc->set_stat_prefix("test"); - Secret::SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}); + SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), + server.clusterManager(), init_manager, config_source, "abc.com"); NiceMock* grpc_client{new NiceMock()}; NiceMock* factory{new NiceMock()}; @@ -56,12 +57,13 @@ TEST_F(SdsApiTest, BasicTest) { init_manager.initialize(); } +// Validate that SdsApi updates secrets successfully if a good secret is passed to onConfigUpdate(). TEST_F(SdsApiTest, SecretUpdateSuccess) { NiceMock server; NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}); + server.clusterManager(), init_manager, config_source, "abc.com"); NiceMock secret_callback; sds_api.addUpdateCallback(secret_callback); @@ -93,12 +95,13 @@ TEST_F(SdsApiTest, SecretUpdateSuccess) { sds_api.removeUpdateCallback(secret_callback); } +// Validate that SdsApi throws exception if an empty secret is passed to onConfigUpdate(). TEST_F(SdsApiTest, EmptyResource) { NiceMock server; NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}); + server.clusterManager(), init_manager, config_source, "abc.com"); Protobuf::RepeatedPtrField secret_resources; @@ -106,12 +109,13 @@ TEST_F(SdsApiTest, EmptyResource) { "Missing SDS resources for abc.com in onConfigUpdate()"); } +// Validate that SdsApi throws exception if multiple secrets are passed to onConfigUpdate(). TEST_F(SdsApiTest, SecretUpdateWrongSize) { NiceMock server; NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}); + server.clusterManager(), init_manager, config_source, "abc.com"); std::string yaml = R"EOF( @@ -133,12 +137,14 @@ TEST_F(SdsApiTest, SecretUpdateWrongSize) { "Unexpected SDS secrets length: 2"); } +// Validate that SdsApi throws exception if secret name passed to onConfigUpdate() +// does not match configured name. TEST_F(SdsApiTest, SecretUpdateWrongSecretName) { NiceMock server; NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com", []() {}); + server.clusterManager(), init_manager, config_source, "abc.com"); std::string yaml = R"EOF( From 550287f8fdc23c17435d7372ac6a20ae6d509ddd Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 23 Aug 2018 12:15:36 -0700 Subject: [PATCH 4/5] Add Cleanup into SdsApi. Signed-off-by: JimmyCYJ --- source/common/secret/BUILD | 1 + source/common/secret/sds_api.cc | 5 +++-- source/common/secret/sds_api.h | 6 +++++- test/common/secret/sds_api_test.cc | 10 +++++----- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index c514d1e06ca5..0e9fb357b147 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -43,6 +43,7 @@ envoy_cc_library( "//include/envoy/runtime:runtime_interface", "//include/envoy/secret:secret_provider_interface", "//include/envoy/stats:stats_interface", + "//source/common/common:cleanup_lib", "//source/common/config:resources_lib", "//source/common/config:subscription_factory_lib", "//source/common/protobuf:utility_lib", diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index beb0ad9db4ca..ae991587fc46 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -15,10 +15,11 @@ namespace Secret { SdsApi::SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, Stats::Store& stats, Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, - const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name) + const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name, + std::function destructor_cb) : local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats), cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name), - secret_hash_(0) { + secret_hash_(0), clean_up_(destructor_cb) { // TODO(JimmyCYJ): Implement chained_init_manager, so that multiple init_manager // can be chained together to behave as one init_manager. In that way, we let // two listeners which share same SdsApi to register at separate init managers, and diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index dcf09959e28a..5ec93e80f3e1 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -14,6 +14,8 @@ #include "envoy/stats/stats.h" #include "envoy/upstream/cluster_manager.h" +#include "common/common/cleanup.h" + namespace Envoy { namespace Secret { @@ -27,7 +29,8 @@ class SdsApi : public Init::Target, SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, Stats::Store& stats, Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager, - const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name); + const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name, + std::function destructor_cb); // Init::Target void initialize(std::function callback) override; @@ -66,6 +69,7 @@ class SdsApi : public Init::Target, const std::string sds_config_name_; uint64_t secret_hash_; + Cleanup clean_up_; Ssl::TlsCertificateConfigPtr tls_certificate_secrets_; std::list update_callbacks_; }; diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index e721b88ebcc2..6e472f44dc15 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -42,7 +42,7 @@ TEST_F(SdsApiTest, BasicTest) { google_grpc->set_target_uri("fake_address"); google_grpc->set_stat_prefix("test"); SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com"); + server.clusterManager(), init_manager, config_source, "abc.com", []() {}); NiceMock* grpc_client{new NiceMock()}; NiceMock* factory{new NiceMock()}; @@ -63,7 +63,7 @@ TEST_F(SdsApiTest, SecretUpdateSuccess) { NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com"); + server.clusterManager(), init_manager, config_source, "abc.com", []() {}); NiceMock secret_callback; sds_api.addUpdateCallback(secret_callback); @@ -101,7 +101,7 @@ TEST_F(SdsApiTest, EmptyResource) { NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com"); + server.clusterManager(), init_manager, config_source, "abc.com", []() {}); Protobuf::RepeatedPtrField secret_resources; @@ -115,7 +115,7 @@ TEST_F(SdsApiTest, SecretUpdateWrongSize) { NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com"); + server.clusterManager(), init_manager, config_source, "abc.com", []() {}); std::string yaml = R"EOF( @@ -144,7 +144,7 @@ TEST_F(SdsApiTest, SecretUpdateWrongSecretName) { NiceMock init_manager; envoy::api::v2::core::ConfigSource config_source; SdsApi sds_api(server.localInfo(), server.dispatcher(), server.random(), server.stats(), - server.clusterManager(), init_manager, config_source, "abc.com"); + server.clusterManager(), init_manager, config_source, "abc.com", []() {}); std::string yaml = R"EOF( From 1ec170133e84a361266556dcc19d15f88dbf320a Mon Sep 17 00:00:00 2001 From: JimmyCYJ Date: Thu, 23 Aug 2018 16:11:08 -0700 Subject: [PATCH 5/5] Add CallbackManager into SdsApi. Signed-off-by: JimmyCYJ --- include/envoy/secret/BUILD | 1 + include/envoy/secret/secret_provider.h | 15 ++++++--------- source/common/secret/BUILD | 1 + source/common/secret/sds_api.cc | 4 +--- source/common/secret/sds_api.h | 10 ++++------ source/common/secret/secret_provider_impl.h | 6 +++--- test/common/secret/sds_api_test.cc | 5 +++-- test/mocks/secret/mocks.h | 1 + 8 files changed, 20 insertions(+), 23 deletions(-) diff --git a/include/envoy/secret/BUILD b/include/envoy/secret/BUILD index 2353a9df31d4..6304003c5546 100644 --- a/include/envoy/secret/BUILD +++ b/include/envoy/secret/BUILD @@ -18,6 +18,7 @@ envoy_cc_library( hdrs = ["secret_provider.h"], deps = [ ":secret_callbacks_interface", + "//include/envoy/common:callback", "//include/envoy/ssl:tls_certificate_config_interface", ], ) diff --git a/include/envoy/secret/secret_provider.h b/include/envoy/secret/secret_provider.h index f5be69a4343e..b43b06ed727a 100644 --- a/include/envoy/secret/secret_provider.h +++ b/include/envoy/secret/secret_provider.h @@ -1,7 +1,9 @@ #pragma once +#include + +#include "envoy/common/callback.h" #include "envoy/common/pure.h" -#include "envoy/secret/secret_callbacks.h" #include "envoy/ssl/tls_certificate_config.h" namespace Envoy { @@ -20,18 +22,13 @@ template class SecretProvider { virtual const SecretType* secret() const PURE; /** - * Add secret callback into secret provider. + * Add secret update callback into secret provider. * It is safe to call this method by main thread and is safe to be invoked * on main thread. * @param callback callback that is executed by secret provider. + * @return CallbackHandle the handle which can remove that update callback. */ - virtual void addUpdateCallback(SecretCallbacks& callback) PURE; - - /** - * Remove secret callback from secret provider. - * @param callback callback that is executed by secret provider. - */ - virtual void removeUpdateCallback(SecretCallbacks& callback) PURE; + virtual Common::CallbackHandle* addUpdateCallback(std::function callback) PURE; }; typedef SecretProvider TlsCertificateConfigProvider; diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 0e9fb357b147..a92edf5117bc 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -43,6 +43,7 @@ envoy_cc_library( "//include/envoy/runtime:runtime_interface", "//include/envoy/secret:secret_provider_interface", "//include/envoy/stats:stats_interface", + "//source/common/common:callback_impl_lib", "//source/common/common:cleanup_lib", "//source/common/config:resources_lib", "//source/common/config:subscription_factory_lib", diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index ae991587fc46..3129c3c05c85 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -63,9 +63,7 @@ void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&) tls_certificate_secrets_ = std::make_unique(secret.tls_certificate()); - for (auto cb : update_callbacks_) { - cb->onAddOrUpdateSecret(); - } + update_callback_manager_.runCallbacks(); } runInitializeCallbackIfAny(); diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 5ec93e80f3e1..f9211e3832b2 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -14,6 +14,7 @@ #include "envoy/stats/stats.h" #include "envoy/upstream/cluster_manager.h" +#include "common/common/callback_impl.h" #include "common/common/cleanup.h" namespace Envoy { @@ -47,11 +48,8 @@ class SdsApi : public Init::Target, return tls_certificate_secrets_.get(); } - void addUpdateCallback(SecretCallbacks& callback) override { - update_callbacks_.push_back(&callback); - } - void removeUpdateCallback(SecretCallbacks& callback) override { - update_callbacks_.remove(&callback); + Common::CallbackHandle* addUpdateCallback(std::function callback) override { + return update_callback_manager_.add(callback); } private: @@ -71,7 +69,7 @@ class SdsApi : public Init::Target, uint64_t secret_hash_; Cleanup clean_up_; Ssl::TlsCertificateConfigPtr tls_certificate_secrets_; - std::list update_callbacks_; + Common::CallbackManager<> update_callback_manager_; }; typedef std::unique_ptr SdsApiPtr; diff --git a/source/common/secret/secret_provider_impl.h b/source/common/secret/secret_provider_impl.h index e6731e0b0af8..959eecef5fa9 100644 --- a/source/common/secret/secret_provider_impl.h +++ b/source/common/secret/secret_provider_impl.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "envoy/api/v2/auth/cert.pb.h" #include "envoy/secret/secret_provider.h" #include "envoy/ssl/tls_certificate_config.h" @@ -13,9 +15,7 @@ class TlsCertificateConfigProviderImpl : public TlsCertificateConfigProvider { const Ssl::TlsCertificateConfig* secret() const override { return tls_certificate_.get(); } - void addUpdateCallback(SecretCallbacks&) override {} - - void removeUpdateCallback(SecretCallbacks&) override {} + Common::CallbackHandle* addUpdateCallback(std::function) override { return nullptr; } private: Ssl::TlsCertificateConfigPtr tls_certificate_; diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 6e472f44dc15..17435fe5c8d8 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -66,7 +66,8 @@ TEST_F(SdsApiTest, SecretUpdateSuccess) { server.clusterManager(), init_manager, config_source, "abc.com", []() {}); NiceMock secret_callback; - sds_api.addUpdateCallback(secret_callback); + auto handle = + sds_api.addUpdateCallback([&secret_callback]() { secret_callback.onAddOrUpdateSecret(); }); std::string yaml = R"EOF( @@ -92,7 +93,7 @@ TEST_F(SdsApiTest, SecretUpdateSuccess) { EXPECT_EQ(TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(key_pem)), sds_api.secret()->privateKey()); - sds_api.removeUpdateCallback(secret_callback); + handle->remove(); } // Validate that SdsApi throws exception if an empty secret is passed to onConfigUpdate(). diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h index 9f41891d4950..907d7b66e8e5 100644 --- a/test/mocks/secret/mocks.h +++ b/test/mocks/secret/mocks.h @@ -1,5 +1,6 @@ #pragma once +#include "envoy/secret/secret_callbacks.h" #include "envoy/secret/secret_manager.h" #include "envoy/ssl/tls_certificate_config.h"