Fetch certificate validation context using SDS service. by JimmyCYJ · Pull Request #4355 · envoyproxy/envoy

JimmyCYJ · 2018-09-06T01:12:02Z

Description: Refactor SdsApi to support dynamic certificate validation context, and support Envoy to fetch certificate validation context from remote server via SDS API.
Risk Level: Low
Testing: Unit tests and integration tests.
Fixes #1194

Signed-off-by: Jimmy Chen jimmychen.0102@gmail.com

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ · 2018-09-06T01:16:00Z

I am adding unit tests and integration tests to cover this change.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

qiwzhang · 2018-09-06T01:48:00Z

source/common/ssl/context_config_impl.h

+  Common::CallbackHandle* tls_certificate_update_callback_handle_;
  Secret::CertificateValidationContextConfigProviderSharedPtr
      certficate_validation_context_provider_;
+  Common::CallbackHandle* certificate_validation_context_update_callback_handle_;


variable name is too long

…_api Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

qiwzhang · 2018-09-07T01:51:57Z

source/common/secret/secret_manager_impl.cc

+}
+
+CertificateValidationContextConfigProviderSharedPtr
+SecretManagerImpl::findOrCreateCertificateValidationContextProvider(


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,

qiwzhang · 2018-09-07T01:53:05Z

source/common/ssl/context_config_impl.cc

          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),


use {} to initiliaze raw pointer

qiwzhang · 2018-09-07T01:53:32Z

source/common/ssl/context_config_impl.h

  const std::string ecdh_curves_;
  Secret::TlsCertificateConfigProviderSharedPtr tls_certficate_provider_;
-  Common::CallbackHandle* secret_update_callback_handle_;
+  Common::CallbackHandle* tls_certificate_update_callback_handle_;


just add {} next to the raw pointer to initailize it.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

qiwzhang · 2018-09-07T19:07:04Z

source/common/secret/secret_manager_impl.cc

+TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider(
+    const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name,
+    Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
+  std::function<SdsApiSharedPtr(std::function<void()> unregister_secret_provider)> create_fn =


just use auto as
auto create_fn = [&]

in the capture, not need to list all used variable, just one & to tell compiler that all used variables are using reference.

qiwzhang · 2018-09-07T19:09:52Z

source/common/secret/secret_manager_impl.cc

+  std::function<SdsApiSharedPtr(std::function<void()> unregister_secret_provider)> create_fn =
+      [&sds_config_source, &config_name, &secret_provider_context](
+          std::function<void()> unregister_secret_provider) -> SdsApiSharedPtr {
+    return std::make_shared<TlsCertificateSdsApi>(


How about move this make_shared code to a static create() function in the

TlsCertificateSdsApi class

qiwzhang · 2018-09-07T19:12:09Z

source/common/secret/sds_api.cc

      secret.type_case() == envoy::api::v2::auth::Secret::TypeCase::kValidationContext) {
    secret_hash_ = new_hash;
-    secrets_ =
+    certificate_validation_context_secrets_ =


these two updateConfigHelper are almost the same. can we move them to the sds_api base class

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

qiwzhang · 2018-09-07T21:50:47Z

source/common/secret/sds_api.h

+                                Stats::Store& stats, Upstream::ClusterManager& cluster_manager,
+                                Init::Manager& init_manager,
+                                const envoy::api::v2::core::ConfigSource& sds_config,
+                                const std::string& sds_config_name,


can we use provider_context in create() function so less number of argument to pass?

qiwzhang · 2018-09-07T21:55:02Z

test/common/secret/sds_api_test.cc

+  auto sds_api = std::static_pointer_cast<CertificateValidationContextSdsApi>(
+      CertificateValidationContextSdsApi::create(
+          server.localInfo(), server.dispatcher(), server.random(), server.stats(),
+          server.clusterManager(), init_manager, config_source, "abc.com", []() {}));


you don't need to change this. It can still use constructor

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

qiwzhang · 2018-09-07T22:31:29Z

source/common/secret/sds_api.h

+      : SdsApi(local_info, dispatcher, random, stats, cluster_manager, init_manager, sds_config,
+               sds_config_name, destructor_cb,
+               [this](const uint64_t new_hash, const envoy::api::v2::auth::Secret& secret) {
+                 if (new_hash != secret_hash_ &&


This create_secret_fn is doing more than creating secret. It is confusing.

My idea: derived class overload two virtual functions:
secret_type() and set_secret(). That is it. Most of updateConfig() logic are in the sds_api class

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

qiwzhang · 2018-09-07T23:20:07Z

LGTM

JimmyCYJ · 2018-09-07T23:21:56Z

cc @lizan @htuch

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

…_api Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

htuch

Looks good, should be shippable after this round of feedback.

htuch · 2018-09-14T17:41:11Z

source/common/secret/sds_api.h

-  }
+protected:
+  // Creates new secrets.
+  virtual void set_secret(const envoy::api::v2::auth::Secret&) PURE;


Nit: setSecret

Done. Thanks!

htuch · 2018-09-14T17:41:49Z

source/common/secret/sds_api.h

+  // Creates new secrets.
+  virtual void set_secret(const envoy::api::v2::auth::Secret&) PURE;
+  Common::CallbackManager<> update_callback_manager_;
+  uint64_t secret_hash_;


Why is secret_hash_ no longer private?

It should be private, thanks for catching this.
It was changed to be protected because it was assigned to a new hash in derived class. But then we simplified that part of code and moved hash calculation back to base class.

htuch · 2018-09-14T17:45:39Z

source/common/secret/sds_api.h

+ * CertificateValidationContextSdsApi implementation maintains and updates dynamic certificate
+ * validation context secrets.
+ */
+class CertificateValidationContextSdsApi : public SdsApi,


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.

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.

htuch · 2018-09-14T17:47:35Z

source/common/secret/secret_manager_impl.h

+  // Removes dynamic secret provider which has been deleted.
  void removeDynamicSecretProvider(const std::string& map_key);
+  // Finds or creates SdsApi object.
+  SdsApiSharedPtr innerFindOrCreate(


Can we rename innerFindOrCreate to something else? Even findOrCreate would be fine, I just find "inner" leads to more questions (e.g. "is this an inner class?", "inner of what?"), so this isn't as clear as it could be.

Good point. Changed to findOrCreate. Thanks.

htuch · 2018-09-14T17:49:17Z

source/common/secret/secret_manager_impl.cc


 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);


Is this definitely TLS? It seems the general SdsApi type of dynamic_secret_providers_ might suggest otherwise?

Yes, this message is not correct. Changed to secret provider.

htuch · 2018-09-14T17:50:42Z

source/common/secret/secret_manager_impl.cc

+TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider(
+    const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name,
+    Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
+  auto create_fn = [&](std::function<void()> unregister_secret_provider) -> SdsApiSharedPtr {


Prefer to use explicit capture rather than & for lambdas (I think this is a style guide recommendation).

Thanks, done.

htuch · 2018-09-14T17:55:47Z

source/common/ssl/context_config_impl.h

-      if (secret_update_callback_handle_) {
-        secret_update_callback_handle_->remove();
+      if (tc_update_callback_handle_) {
+        tc_update_callback_handle_->remove();


Missing coverage, https://95081-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.bazel-out_k8-dbg_bin_source_common_ssl__virtual_includes_context_config_lib_common_ssl_context_config_impl.h.html

Fixed in context_impl_test.cc. Thanks.

htuch · 2018-09-14T17:55:51Z

source/common/ssl/context_config_impl.h

+    }
+    if (certficate_validation_context_provider_) {
+      if (cvc_update_callback_handle_) {
+        cvc_update_callback_handle_->remove();


Fixed in context_impl_test.cc. Thanks.

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

…_api Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

htuch

Rad, thanks!

lizan · 2018-09-17T20:23:29Z

source/common/secret/secret_manager_impl.cc

+  };
+  SdsApiSharedPtr secret_provider = findOrCreate(sds_config_source, config_name, create_fn);
+
+  return std::dynamic_pointer_cast<CertificateValidationContextConfigProvider>(secret_provider);


Can we templatize findOrCreate and remove avoid std::dynamic_pointer_cast here?

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.
Maybe let's make this change in a follow up PR. I would like to get this in soon.

According to comment in PR #4355, we want to avoid std::dynamic_pointer_cast in SecretManagerImpl. This PR creates a template class and moves findOrCreate method into the template class. Risk Level: Low Testing: Existing unit tests and integration tests. Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ added 4 commits September 4, 2018 21:55

Refactor SdsApi to support dynamic certificate validation context.

9cb2e20

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Revise per comments.

cd35a2c

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

pass sds config name as const std::string&.

61bdf6c

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Add explicit instantiation.

cf3cbf3

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Update tests.

a905c0a

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

qiwzhang reviewed Sep 6, 2018

View reviewed changes

JimmyCYJ added 2 commits September 6, 2018 11:41

Merge remote-tracking branch 'envoyupstream/master' into refactor_sds…

d9414bd

…_api Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Add test and refactor SdsApi.

a12b042

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ mentioned this pull request Sep 7, 2018

Refactor SdsApi to support dynamic certificate validation context. istio/envoy#11

Closed

qiwzhang reviewed Sep 7, 2018

View reviewed changes

Revise per comments.

058d3d0

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

qiwzhang reviewed Sep 7, 2018

View reviewed changes

JimmyCYJ added 2 commits September 7, 2018 13:53

Add tests and revise per comment.

820f6a2

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Remove updateConfigHelper

99b9957

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

qiwzhang reviewed Sep 7, 2018

View reviewed changes

Revise per comments.

91996d6

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

qiwzhang reviewed Sep 7, 2018

View reviewed changes

Revise per comments.

0925dd4

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ changed the title ~~[WIP] Support Envoy to fetch certificate validation context using SDS service.~~ Support Envoy to fetch certificate validation context using SDS service. Sep 7, 2018

zuercher assigned lizan Sep 10, 2018

Merge master into branch.

d7532e8

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

htuch self-assigned this Sep 12, 2018

JimmyCYJ added 2 commits September 13, 2018 16:11

Update comments in cert.proto.

d89a836

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Merge remote-tracking branch 'envoyupstream/master' into refactor_sds…

259322a

…_api Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

JimmyCYJ changed the title ~~Support Envoy to fetch certificate validation context using SDS service.~~ Fetch certificate validation context using SDS service. Sep 13, 2018

htuch suggested changes Sep 14, 2018

View reviewed changes

JimmyCYJ added 3 commits September 14, 2018 12:40

Revise per comments.

a02fef6

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Add more tests.

4e2594c

Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

Merge remote-tracking branch 'envoyupstream/master' into refactor_sds…

1557732

…_api Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

htuch approved these changes Sep 17, 2018

View reviewed changes

lizan reviewed Sep 17, 2018

View reviewed changes

htuch merged commit 15cfc5a into envoyproxy:master Sep 17, 2018

JimmyCYJ deleted the refactor_sds_api branch September 17, 2018 23:19

JimmyCYJ mentioned this pull request Sep 18, 2018

Refactor SecretManagerImpl to templatize findOrCreate method. #4448

Merged

Conversation

JimmyCYJ commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JimmyCYJ commented Sep 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang commented Sep 7, 2018

Uh oh!

JimmyCYJ commented Sep 7, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JimmyCYJ commented Sep 6, 2018 •

edited

Loading