From 99df2e23423a8bb27eeca0c696de44a9dbb1f843 Mon Sep 17 00:00:00 2001 From: Jimmy Chen <28548492+JimmyCYJ@users.noreply.github.com> Date: Thu, 23 Aug 2018 20:27:13 -0700 Subject: [PATCH] Sds api split (#4231) Implement SDS API and dummy socket, and they are not in use. This is split from PR #4176. Risk Level: Low Testing: Unit tests Docs Changes: None Fixes #1194 Signed-off-by: JimmyCYJ --- include/envoy/secret/BUILD | 7 + include/envoy/secret/secret_callbacks.h | 18 +++ include/envoy/secret/secret_provider.h | 12 +- source/common/secret/BUILD | 21 +++ source/common/secret/sds_api.cc | 85 ++++++++++ source/common/secret/sds_api.h | 78 +++++++++ source/common/secret/secret_provider_impl.h | 4 + source/common/ssl/ssl_socket.cc | 30 +++- test/common/secret/BUILD | 19 +++ test/common/secret/sds_api_test.cc | 170 ++++++++++++++++++++ test/mocks/secret/BUILD | 1 + test/mocks/secret/mocks.cc | 4 + test/mocks/secret/mocks.h | 8 + 13 files changed, 454 insertions(+), 3 deletions(-) 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/BUILD b/include/envoy/secret/BUILD index 6124d51a3210..6304003c5546 100644 --- a/include/envoy/secret/BUILD +++ b/include/envoy/secret/BUILD @@ -8,10 +8,17 @@ 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/common:callback", "//include/envoy/ssl:tls_certificate_config_interface", ], ) 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/include/envoy/secret/secret_provider.h b/include/envoy/secret/secret_provider.h index b6f9f26ca76c..b43b06ed727a 100644 --- a/include/envoy/secret/secret_provider.h +++ b/include/envoy/secret/secret_provider.h @@ -1,5 +1,8 @@ #pragma once +#include + +#include "envoy/common/callback.h" #include "envoy/common/pure.h" #include "envoy/ssl/tls_certificate_config.h" @@ -18,7 +21,14 @@ template class SecretProvider { */ virtual const SecretType* secret() const PURE; - // TODO(lizan): Add more methods for dynamic 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 Common::CallbackHandle* addUpdateCallback(std::function callback) PURE; }; typedef SecretProvider TlsCertificateConfigProvider; diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 598d8249fa5e..a92edf5117bc 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -30,3 +30,24 @@ 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/common:callback_impl_lib", + "//source/common/common:cleanup_lib", + "//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/sds_api.cc b/source/common/secret/sds_api.cc new file mode 100644 index 000000000000..3129c3c05c85 --- /dev/null +++ b/source/common/secret/sds_api.cc @@ -0,0 +1,85 @@ +#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 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), 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 + // each init manager has a chance to initialize its targets. + init_manager.registerTarget(*this); +} + +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()); + + update_callback_manager_.runCallbacks(); + } + + 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..f9211e3832b2 --- /dev/null +++ b/source/common/secret/sds_api.h @@ -0,0 +1,78 @@ +#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" + +#include "common/common/callback_impl.h" +#include "common/common/cleanup.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 destructor_cb); + + // 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(); + } + + Common::CallbackHandle* addUpdateCallback(std::function callback) override { + return update_callback_manager_.add(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_; + Cleanup clean_up_; + Ssl::TlsCertificateConfigPtr tls_certificate_secrets_; + Common::CallbackManager<> update_callback_manager_; +}; + +typedef std::unique_ptr SdsApiPtr; + +} // namespace Secret +} // namespace Envoy diff --git a/source/common/secret/secret_provider_impl.h b/source/common/secret/secret_provider_impl.h index 9ac79c66009c..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,6 +15,8 @@ class TlsCertificateConfigProviderImpl : public TlsCertificateConfigProvider { const Ssl::TlsCertificateConfig* secret() const override { return tls_certificate_.get(); } + Common::CallbackHandle* addUpdateCallback(std::function) override { return nullptr; } + private: Ssl::TlsCertificateConfigPtr tls_certificate_; }; diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc index 3f634c7d9fa2..27e149dd0c63 100644 --- a/source/common/ssl/ssl_socket.cc +++ b/source/common/ssl/ssl_socket.cc @@ -17,6 +17,24 @@ 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: + // 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 nullptr; } +}; +} // namespace + SslSocket::SslSocket(ContextSharedPtr ctx, InitialState state) : ctx_(std::dynamic_pointer_cast(ctx)), ssl_(ctx_->newSsl()) { if (state == InitialState::Client) { @@ -391,7 +409,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 +427,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/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc new file mode 100644 index 000000000000..17435fe5c8d8 --- /dev/null +++ b/test/common/secret/sds_api_test.cc @@ -0,0 +1,170 @@ +#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 {}; + +// Validate that SdsApi object is created and initialized successfully. +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"); + 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(); +} + +// 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", []() {}); + + NiceMock secret_callback; + auto handle = + sds_api.addUpdateCallback([&secret_callback]() { secret_callback.onAddOrUpdateSecret(); }); + + 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()); + + handle->remove(); +} + +// 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", []() {}); + + Protobuf::RepeatedPtrField secret_resources; + + EXPECT_THROW_WITH_MESSAGE(sds_api.onConfigUpdate(secret_resources, ""), EnvoyException, + "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", []() {}); + + 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"); +} + +// 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", []() {}); + + 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 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..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" @@ -22,5 +23,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