-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Fetch certificate validation context using SDS service. #4355
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
9cb2e20
cd35a2c
61bdf6c
cf3cbf3
a905c0a
d9414bd
a12b042
058d3d0
820f6a2
99b9957
91996d6
0925dd4
d7532e8
d89a836
259322a
a02fef6
4e2594c
1557732
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,13 +24,12 @@ namespace Secret { | |
| * SDS API implementation that fetches secrets from SDS server via Subscription. | ||
| */ | ||
| class SdsApi : public Init::Target, | ||
| public TlsCertificateConfigProvider, | ||
| public Config::SubscriptionCallbacks<envoy::api::v2::auth::Secret> { | ||
| 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, | ||
| const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name, | ||
| std::function<void()> destructor_cb); | ||
|
|
||
| // Init::Target | ||
|
|
@@ -43,14 +42,11 @@ class SdsApi : public Init::Target, | |
| return MessageUtil::anyConvert<envoy::api::v2::auth::Secret>(resource).name(); | ||
| } | ||
|
|
||
| // SecretProvider | ||
| const Ssl::TlsCertificateConfig* secret() const override { | ||
| return tls_certificate_secrets_.get(); | ||
| } | ||
|
|
||
| Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override { | ||
| return update_callback_manager_.add(callback); | ||
| } | ||
| protected: | ||
| // Updates local storage of dynamic secrets and invokes callbacks. | ||
| virtual void updateConfigHelper(const envoy::api::v2::auth::Secret&) PURE; | ||
| uint64_t secret_hash_; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be private, thanks for catching this. |
||
| Common::CallbackManager<> update_callback_manager_; | ||
|
|
||
| private: | ||
| void runInitializeCallbackIfAny(); | ||
|
|
@@ -66,13 +62,71 @@ class SdsApi : public Init::Target, | |
| std::function<void()> initialize_callback_; | ||
| const std::string sds_config_name_; | ||
|
|
||
| uint64_t secret_hash_; | ||
| Cleanup clean_up_; | ||
| }; | ||
|
|
||
| typedef std::shared_ptr<SdsApi> SdsApiSharedPtr; | ||
|
|
||
| /** | ||
| * TlsCertificateSdsApi implementation maintains and updates dynamic TLS certificate secrets. | ||
| */ | ||
| class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider { | ||
| public: | ||
| TlsCertificateSdsApi(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<void()> destructor_cb) | ||
| : SdsApi(local_info, dispatcher, random, stats, cluster_manager, init_manager, sds_config, | ||
| sds_config_name, destructor_cb) {} | ||
|
|
||
| // SecretProvider | ||
| const Ssl::TlsCertificateConfig* secret() const override { | ||
| return tls_certificate_secrets_.get(); | ||
| } | ||
| Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override { | ||
| return update_callback_manager_.add(callback); | ||
| } | ||
|
|
||
| private: | ||
| // SdsApi | ||
| void updateConfigHelper(const envoy::api::v2::auth::Secret& secret) override; | ||
|
|
||
| Ssl::TlsCertificateConfigPtr tls_certificate_secrets_; | ||
| Common::CallbackManager<> update_callback_manager_; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<SdsApi> SdsApiPtr; | ||
| /** | ||
| * CertificateValidationContextSdsApi implementation maintains and updates dynamic certificate | ||
| * validation context secrets. | ||
| */ | ||
| class CertificateValidationContextSdsApi : public SdsApi, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first I thought templates might be a nice thing here to avoid the boiler plate, but then we have another problem, namely how to marry templates and virtual inheritance. Worth thinking about if you have any way to reduce this repetition.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We tried using templates at first (cd35a2c), and then we found that causes many duplicated methods. We have to add more methods into secret manager to create each type of provider, and the provider creation methods have duplicated code. We also need two maps for each type of providers in secret manager. Besides, each type of provider also overrides some methods of provider interface, and those methods have duplicated code, too. Then we decide to switch to this way and simplify the code a lot. We don't find a better way for now. |
||
| public CertificateValidationContextConfigProvider { | ||
| public: | ||
| CertificateValidationContextSdsApi(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<void()> destructor_cb) | ||
| : SdsApi(local_info, dispatcher, random, stats, cluster_manager, init_manager, sds_config, | ||
| sds_config_name, destructor_cb) {} | ||
|
|
||
| // SecretProvider | ||
| const Ssl::CertificateValidationContextConfig* secret() const override { | ||
| return certificate_validation_context_secrets_.get(); | ||
| } | ||
| Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override { | ||
| return update_callback_manager_.add(callback); | ||
| } | ||
|
|
||
| private: | ||
| // SdsApi | ||
| void updateConfigHelper(const envoy::api::v2::auth::Secret& secret) override; | ||
|
|
||
| Ssl::CertificateValidationContextConfigPtr certificate_validation_context_secrets_; | ||
| }; | ||
|
|
||
| } // namespace Secret | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,7 +65,7 @@ SecretManagerImpl::createInlineCertificateValidationContextProvider( | |
| } | ||
|
|
||
| void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key) { | ||
| ENVOY_LOG(debug, "Unregister secret provider. hash key: {}", map_key); | ||
| ENVOY_LOG(debug, "Unregister tls certificate provider. hash key: {}", map_key); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this definitely TLS? It seems the general
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this message is not correct. Changed to secret provider. |
||
|
|
||
| auto num_deleted = dynamic_secret_providers_.erase(map_key); | ||
| ASSERT(num_deleted == 1, ""); | ||
|
|
@@ -76,7 +76,7 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertific | |
| Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { | ||
| const std::string map_key = sds_config_source.SerializeAsString() + config_name; | ||
|
|
||
| TlsCertificateConfigProviderSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock(); | ||
| SdsApiSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock(); | ||
| if (!secret_provider) { | ||
| ASSERT(secret_provider_context.initManager() != nullptr); | ||
|
|
||
|
|
@@ -86,15 +86,42 @@ TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertific | |
| removeDynamicSecretProvider(map_key); | ||
| }; | ||
|
|
||
| secret_provider = std::make_shared<SdsApi>( | ||
| secret_provider = std::make_shared<TlsCertificateSdsApi>( | ||
| secret_provider_context.localInfo(), secret_provider_context.dispatcher(), | ||
| secret_provider_context.random(), secret_provider_context.stats(), | ||
| secret_provider_context.clusterManager(), *secret_provider_context.initManager(), | ||
| sds_config_source, config_name, unregister_secret_provider); | ||
| dynamic_secret_providers_[map_key] = secret_provider; | ||
| } | ||
|
|
||
| return secret_provider; | ||
| return std::dynamic_pointer_cast<TlsCertificateConfigProvider>(secret_provider); | ||
| } | ||
|
|
||
| CertificateValidationContextConfigProviderSharedPtr | ||
| SecretManagerImpl::findOrCreateCertificateValidationContextProvider( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code is similar for this function and the other. try to share them by create a SdsApiSharedPtr innerFindOrCreate(..., create_fn); Then, each function will provide its creation_function to create proper object,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name, | ||
| Server::Configuration::TransportSocketFactoryContext& secret_provider_context) { | ||
| const std::string map_key = sds_config_source.SerializeAsString() + config_name; | ||
|
|
||
| SdsApiSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock(); | ||
| if (!secret_provider) { | ||
| ASSERT(secret_provider_context.initManager() != nullptr); | ||
|
|
||
| // SdsApi is owned by ListenerImpl and ClusterInfo which are destroyed before | ||
| // SecretManagerImpl. It is safe to invoke this callback at the destructor of SdsApi. | ||
| std::function<void()> unregister_secret_provider = [map_key, this]() { | ||
| removeDynamicSecretProvider(map_key); | ||
| }; | ||
|
|
||
| secret_provider = std::make_shared<CertificateValidationContextSdsApi>( | ||
| secret_provider_context.localInfo(), secret_provider_context.dispatcher(), | ||
| secret_provider_context.random(), secret_provider_context.stats(), | ||
| secret_provider_context.clusterManager(), *secret_provider_context.initManager(), | ||
| sds_config_source, config_name, unregister_secret_provider); | ||
| dynamic_secret_providers_[map_key] = secret_provider; | ||
| } | ||
|
|
||
| return std::dynamic_pointer_cast<CertificateValidationContextConfigProvider>(secret_provider); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we templatize
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are building sidecar with SDS feature from istio:collab-gcp-identity branch, and @quanjielin is working on merging istio:collab-gcp-identity branch into istio:master. This PR blocks the merge. |
||
| } | ||
|
|
||
| } // namespace Secret | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,9 @@ getCertificateValidationContextConfigProvider( | |
| sds_secret_config.name())); | ||
| } | ||
| return secret_provider; | ||
| } else { | ||
| return factory_context.secretManager().findOrCreateCertificateValidationContextProvider( | ||
| sds_secret_config.sds_config(), sds_secret_config.name(), factory_context); | ||
| } | ||
| } | ||
| return nullptr; | ||
|
|
@@ -98,17 +101,21 @@ ContextConfigImpl::ContextConfigImpl( | |
| ecdh_curves_(StringUtil::nonEmptyStringOrDefault( | ||
| RepeatedPtrUtil::join(config.tls_params().ecdh_curves(), ":"), DEFAULT_ECDH_CURVES)), | ||
| tls_certficate_provider_(getTlsCertificateConfigProvider(config, factory_context)), | ||
| secret_update_callback_handle_(nullptr), | ||
| tls_certificate_update_callback_handle_(nullptr), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use {} to initiliaze raw pointer
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| certficate_validation_context_provider_( | ||
| getCertificateValidationContextConfigProvider(config, factory_context)), | ||
| certificate_validation_context_update_callback_handle_(nullptr), | ||
| min_protocol_version_( | ||
| tlsVersionFromProto(config.tls_params().tls_minimum_protocol_version(), TLS1_VERSION)), | ||
| max_protocol_version_(tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(), | ||
| TLS1_2_VERSION)) {} | ||
|
|
||
| ContextConfigImpl::~ContextConfigImpl() { | ||
| if (secret_update_callback_handle_) { | ||
| secret_update_callback_handle_->remove(); | ||
| if (tls_certificate_update_callback_handle_) { | ||
| tls_certificate_update_callback_handle_->remove(); | ||
| } | ||
| if (certificate_validation_context_update_callback_handle_) { | ||
| certificate_validation_context_update_callback_handle_->remove(); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,16 +39,28 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { | |
|
|
||
| bool isReady() const override { | ||
| // Either tls_certficate_provider_ is nullptr or | ||
| // tls_certficate_provider_->secret() is NOT nullptr. | ||
| return !tls_certficate_provider_ || tls_certficate_provider_->secret() != nullptr; | ||
| // tls_certficate_provider_->secret() is NOT nullptr and | ||
| // either certficate_validation_context_provider_ is nullptr or | ||
| // certficate_validation_context_provider_->secret() is NOT nullptr. | ||
| return (!tls_certficate_provider_ || tls_certficate_provider_->secret() != nullptr) && | ||
| (!certficate_validation_context_provider_ || | ||
| certficate_validation_context_provider_->secret() != nullptr); | ||
| } | ||
|
|
||
| void setSecretUpdateCallback(std::function<void()> callback) override { | ||
| if (tls_certficate_provider_) { | ||
| if (secret_update_callback_handle_) { | ||
| secret_update_callback_handle_->remove(); | ||
| if (tls_certificate_update_callback_handle_) { | ||
| tls_certificate_update_callback_handle_->remove(); | ||
| } | ||
| secret_update_callback_handle_ = tls_certficate_provider_->addUpdateCallback(callback); | ||
| tls_certificate_update_callback_handle_ = | ||
| tls_certficate_provider_->addUpdateCallback(callback); | ||
| } | ||
| if (certficate_validation_context_provider_) { | ||
| if (certificate_validation_context_update_callback_handle_) { | ||
| certificate_validation_context_update_callback_handle_->remove(); | ||
| } | ||
| certificate_validation_context_update_callback_handle_ = | ||
| certficate_validation_context_provider_->addUpdateCallback(callback); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -69,9 +81,10 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { | |
| const std::string cipher_suites_; | ||
| const std::string ecdh_curves_; | ||
| Secret::TlsCertificateConfigProviderSharedPtr tls_certficate_provider_; | ||
| Common::CallbackHandle* secret_update_callback_handle_; | ||
| Common::CallbackHandle* tls_certificate_update_callback_handle_; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just add {} next to the raw pointer to initailize it.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| Secret::CertificateValidationContextConfigProviderSharedPtr | ||
| certficate_validation_context_provider_; | ||
| Common::CallbackHandle* certificate_validation_context_update_callback_handle_; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. variable name is too long |
||
| const unsigned min_protocol_version_; | ||
| const unsigned max_protocol_version_; | ||
| }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two updateConfigHelper are almost the same. can we move them to the sds_api base class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.