sds: clusters and listeners read static secrets from Bootstrap.static_resources #3465

mangchiandjjoe · 2018-05-23T01:10:40Z

Description:
Clusters and listeners read secrets from the static resources in the bootstrap configuration.
Reading secrets from the Secret Discovery Service will follow.

Risk Level: Low

Fixes #1194

Signed-off-by: Jae Kim [email protected]

mattklein123 · 2018-05-23T16:28:50Z

@mangchiandjjoe please check CI.

lizan · 2018-05-23T18:38:31Z

CI seems to be OK now.

mattklein123

Some quick comments to get started. Mostly nits. A few other things:

Will need release notes
Likely there are doc fields that need to be unhidden. Can you check that?

mattklein123 · 2018-05-23T19:27:31Z

include/envoy/secret/BUILD

+    hdrs = ["secret_manager.h"],
+    deps = [
+        ":secret_interface",
+        "//source/common/json:json_loader_lib",


Shouldn't be needed in public interface

mattklein123 · 2018-05-23T19:28:07Z

include/envoy/secret/secret.h

+namespace Secret {
+
+/**
+ * Secret contains certificate chain and private key


nit: Please go through and end all comments with full stop ('.').

mattklein123 · 2018-05-23T19:28:55Z

include/envoy/secret/secret.h

+  virtual const std::string& name() const PURE;
+
+  /**
+   * @return a string of certificate chain


Should we use strings here? Or vectors of bytes? @PiotrSikora any preference?

mattklein123 · 2018-05-23T19:29:19Z

include/envoy/secret/secret_manager.h

@@ -0,0 +1,38 @@
+#pragma once
+
+#include <google/protobuf/util/json_util.h>


mattklein123 · 2018-05-23T19:29:24Z

include/envoy/secret/secret_manager.h

+
+#include "envoy/secret/secret.h"
+
+#include "common/json/json_loader.h"


mattklein123 · 2018-05-23T19:31:09Z

include/envoy/server/transport_socket_config.h

@@ -3,6 +3,7 @@
 #include <string>

 #include "envoy/network/transport_socket.h"
+#include "envoy/secret/secret_manager.h"


mattklein123 · 2018-05-23T19:31:59Z

source/common/config/bootstrap_json.cc

@@ -130,5 +131,20 @@ void BootstrapJson::translateBootstrap(const Json::Object& json_config,
  }
 }

+void BootstrapJson::translateStaticSecretsBootstrap(


We won't support SDS in v1 config. Please remove all v1 config references in this PR.

Removed the method and related test cases

mattklein123 · 2018-05-23T19:33:48Z

source/common/secret/secret_impl.h

+  const std::string& privateKey() const override { return private_key_; }
+
+private:
+  std::string name_;


const for all of these

mattklein123 · 2018-05-23T19:34:10Z

source/common/secret/secret_manager_impl.cc

+
+bool SecretManagerImpl::addOrUpdateStaticSecret(const SecretSharedPtr secret) {
+  static_secrets_[secret->name()] = secret;
+  return true;


looks like it can't fail. Please make this a void function for now.

Changed to void

mattklein123 · 2018-05-23T19:34:26Z

source/common/secret/secret_manager_impl.h

+public:
+  SecretManagerImpl(){};
+
+  virtual ~SecretManagerImpl() {}


mattklein123

Thank you. Flushing comments.

mattklein123 · 2018-05-24T17:32:25Z

include/envoy/secret/secret_manager.h

+namespace Secret {
+
+/**
+ * A manager for all static secrets.


Aren't we eventually going to use this for dynamic secrets also? What is the long term plan for this code?

We are going to support dynamic secrets. SecretManager will also manage SdsApi clients.
Added TODO comment

mattklein123 · 2018-05-24T17:32:32Z

include/envoy/secret/secret_manager.h

+  /**
+   * @param secret Updated Secret.
+   */
+


nit: del newline

mattklein123 · 2018-05-24T17:33:08Z

include/envoy/secret/secret_manager.h

+   * @param secret Updated Secret.
+   */
+
+  virtual void addOrUpdateStaticSecret(const SecretSharedPtr secret) PURE;


nit: const doesn't do anything here, just remove. Better would probably be to make this a SecretConstSharedPtr and have const inside the pointer definition.

mattklein123 · 2018-05-24T17:33:41Z

include/envoy/secret/secret_manager.h

+  /**
+   * @return the static secret for the given name.
+   */
+  virtual const SecretSharedPtr staticSecret(const std::string& name) const PURE;


Same question here. Can we remove static from all of these method definitions? Aren't we going to do dynamic updates later?

Renamed to findSecret

mattklein123 · 2018-05-24T17:35:23Z

source/common/secret/secret_manager_impl.h

+namespace Envoy {
+namespace Secret {
+
+typedef std::unordered_map<std::string, SecretSharedPtr> SecretSharedPtrMap;


This typedef is unnecessary and/or can be moved private within the impl class.

Moved in to the private

mattklein123 · 2018-05-24T17:36:56Z

source/common/ssl/context_config_impl.cc

-              ? ""
-              : Config::DataSource::read(config.tls_certificates()[0].private_key(), true)),
+      private_key_([&config, &secret_manager] {
+        if (!config.tls_certificates().empty()) {


Can this logic be lifted out into a utility and used both here and above? it looks the same.

Those are same structure but calling method are different.
function might requires either an argument to choose certificate chain/private key or function pointer.
I am not sure that is efficient.

Efficiency doesn't really matter in this path. I would rather have common handling for some of the error handling logic. Please see what you can do here to consolidate.

mattklein123 · 2018-05-24T17:39:26Z

source/common/ssl/context_config_impl.cc

@@ -139,17 +169,21 @@ ServerContextConfigImpl::ServerContextConfigImpl(
        return ret;
      }()) {
  // TODO(PiotrSikora): Support multiple TLS certificates.
-  if (config.common_tls_context().tls_certificates().size() != 1) {
+  if (config.common_tls_context().tls_certificates().size() != 1 &&


I don't think this is right. You need there to be at least 1 cert across both sources, right? Assuming so, please add a test that should fail with this code and then fix it.

There need to be exactly 1 cert.

Modified to check the sum is 1

mattklein123 · 2018-05-24T17:40:26Z

source/server/configuration_impl.cc

+  for (ssize_t i = 0; i < secrets.size(); i++) {
+    ENVOY_LOG(debug, "static secret #{}: {}", i, secrets[i].name());
+    server.secretManager().addOrUpdateStaticSecret(
+        Secret::SecretSharedPtr(new Secret::SecretImpl(secrets[i])));


nit: std::make_shared

mattklein123 · 2018-05-24T17:40:50Z

source/server/server.cc

@@ -248,6 +248,9 @@ void InstanceImpl::initialize(Options& options,
  listener_manager_.reset(
      new ListenerManagerImpl(*this, listener_component_factory_, worker_factory_));

+  // Shared storage of secrets from SDS
+  secret_manager_.reset(new Secret::SecretManagerImpl());


You should be able to initialize this in the initializer list.

Moved to the initializer list

PiotrSikora · 2018-05-24T22:20:52Z

include/envoy/secret/secret.h

+namespace Secret {
+
+/**
+ * Secret contains certificate chain and private key.


That diverges quite a lot from the API, in which Secret is opaque container for various secrets (certificates, session ticket keys, etc.).

Could we perhaps make this opaque interface that returns Protobuf::Message& which can then be used for config generation?

Added a new virtual method and removed TLS certificate related methods.

Please update the class comment now that it is more general.

PiotrSikora · 2018-05-24T22:28:04Z

include/envoy/secret/secret_manager.h

+  /**
+   * @return the static secret for the given name.
+   */
+  virtual const SecretSharedPtr staticSecret(const std::string& name) const PURE;


Do we care whether the secret was configured as static or not? I think the only difference between "static" and "dynamic" secrets is going to be the missing config source, so perhaps something like that:

const SecretSharedPtr findSecret(const std::string& name, const std::string &source);

could be used for both, with source being EMPTY_STRING for the static secrets?

renamed the method.
Since source paramter is not going to be used for now, I will add source parameter when it supports dynamic secret.

PiotrSikora · 2018-05-24T22:31:23Z

source/common/ssl/context_config_impl.cc

@@ -139,17 +169,21 @@ ServerContextConfigImpl::ServerContextConfigImpl(
        return ret;
      }()) {
  // TODO(PiotrSikora): Support multiple TLS certificates.
-  if (config.common_tls_context().tls_certificates().size() != 1) {
+  if (config.common_tls_context().tls_certificates().size() != 1 &&
+      config.common_tls_context().tls_certificate_sds_secret_configs().size() != 1) {


We can have only 1 in total, so perhaps test if (sum != 1).

PiotrSikora · 2018-05-24T22:40:33Z

source/common/secret/secret_impl.h

+namespace Envoy {
+namespace Secret {
+
+class SecretImpl : public Secret {


This should really be class TlsCertificateConfigImpl : public Secret or something like that, otherwise it won't work scale for other secret types.

Perhaps move it to source/common/ssl as well?

mattklein123

Making progress. Thank you! Some more comments.

mattklein123 · 2018-05-29T16:41:02Z

include/envoy/secret/secret.h

+namespace Secret {
+
+/**
+ * Secret contains certificate chain and private key.


Please update the class comment now that it is more general.

mattklein123 · 2018-05-29T16:41:54Z

include/envoy/secret/secret_manager.h

+  /**
+   * @param secret Updated Secret.
+   */
+  virtual void addOrUpdateStaticSecret(SecretSharedPtr secret) PURE;


Asking again. Why does this need to be called addOrUpdateStaticSecret? Why can't it just be addOrUpdateSecret?

mattklein123 · 2018-05-29T16:42:23Z

include/envoy/secret/secret_manager.h

+  virtual void addOrUpdateStaticSecret(SecretSharedPtr secret) PURE;
+
+  /**
+   * @return the static secret for the given name.


update method comment

mattklein123 · 2018-05-29T17:02:30Z

source/common/ssl/context_config_impl.cc

-              ? ""
-              : Config::DataSource::read(config.tls_certificates()[0].private_key(), true)),
+      private_key_([&config, &secret_manager] {
+        if (!config.tls_certificates().empty()) {


Efficiency doesn't really matter in this path. I would rather have common handling for some of the error handling logic. Please see what you can do here to consolidate.

mattklein123 · 2018-05-29T17:04:27Z

source/common/ssl/context_config_impl.cc

+                            config.tls_certificate_sds_secret_configs()[0].name()));
+          }
+
+          return std::dynamic_pointer_cast<Ssl::TlsCertificateConfigImpl>(static_secret)


This dynamic cast here is not great. There is nothing to prevent the user from having the wrong type of object mapped to name. Feel free to follow up with @PiotrSikora, but we either need to do extra error checking logic prior to dynamic cast, or potentially store the type of object in the secret manager and then have a map for each type of object with name -> object mapping. Same comment applies below. Please make sure you have a test case for this config issue.

I added the error checking for the date type of secret and the test case.

mattklein123 · 2018-05-29T17:09:07Z

source/common/ssl/tls_certificate_config_impl.h

+public:
+  TlsCertificateConfigImpl(const envoy::api::v2::auth::Secret& config);
+
+  const Protobuf::Message& message() const override { return message_; }


Not sure I fully understand this class now. Why do you need to return the abstract message here if you end up doing the dynamic cast to this object and getting cert chain and private key? Per my other comments I think we need to rework all of this to avoid dynamic_casts entirely.

Fixed to return the same config data type.

qiwzhang · 2018-05-30T17:08:52Z

source/common/secret/secret_manager_impl.h

+  const SecretSharedPtr findSecret(const std::string& name) const override;
+
+private:
+  typedef std::unordered_map<std::string, SecretSharedPtr> SecretSharedPtrMap;


can Secret be stored as unique_ptr?

Secret needs to be shared pointer to typecast with std::dynamic_pointer_cast.

qiwzhang · 2018-05-30T17:30:48Z

source/common/ssl/context_config_impl.cc

+        } else {
+          return std::string("");
+        }
+      }()),


IMHO, it is easier to read if making it as a separate function.

+1, please figure out how to break up / simplify the logic in this function.

Created separated functions

qiwzhang · 2018-05-30T17:39:44Z

source/common/ssl/tls_certificate_config_impl.cc

+        envoy::api::v2::auth::Secret message;
+        message.CopyFrom(config);
+        return message;
+      }()),


Line 9 to 13 is overkill. I think it can be simplified as: message_(config)

Per my other comment you can remove all of this entirely, I don't think it's needed.

qiwzhang · 2018-05-30T18:07:13Z

test/common/ssl/context_impl_test.cc

+          envoy::api::v2::auth::Secret cfg;
+          cfg.CopyFrom(message);
+          return cfg;
+        }()),


can it be simplified as message_(message)

qiwzhang · 2018-05-30T18:09:48Z

test/common/ssl/context_impl_test.cc

+
+TEST(ClientContextConfigImplTest, StaticTlsCertificates) {
+  std::string kExpectedCertificateChain =
+      R"EOF(-----BEGIN CERTIFICATE-----


are these two strings the same as ones in secret_manager_impl_test.cc? If so, can they be defined once?

Created and used separated test data library.

mattklein123 · 2018-05-30T18:37:45Z

include/envoy/secret/secret.h

+  /**
+   * @return protobuf message that initialized the secret.
+   */
+  virtual const Protobuf::Message& message() const PURE;


I don't think this function is actually used anywhere. It can be removed from the interface.

mattklein123 · 2018-05-30T18:38:05Z

include/envoy/secret/secret_manager.h

+  /**
+   * @param secret Updated Secret.
+   */
+  virtual void addOrUpdateSecret(SecretSharedPtr secret) PURE;


nit: const SecretSharedPtr&

mattklein123 · 2018-05-30T18:39:14Z

source/common/ssl/context_config_impl.cc

+namespace {
+
+template <typename T>
+const std::shared_ptr<T> findSecretAndCheckType(const Secret::SecretManager& secret_manager,


This function will end up getting used for other types of secrets. I would add this to the primary interface.

Moved to SecretManager interface

mattklein123 · 2018-05-30T18:40:01Z

source/common/ssl/context_config_impl.cc

+        } else {
+          return std::string("");
+        }
+      }()),


+1, please figure out how to break up / simplify the logic in this function.

mattklein123 · 2018-05-30T18:40:23Z

source/common/ssl/tls_certificate_config_impl.cc

+        envoy::api::v2::auth::Secret message;
+        message.CopyFrom(config);
+        return message;
+      }()),


Per my other comment you can remove all of this entirely, I don't think it's needed.

mattklein123 · 2018-05-30T18:42:42Z

source/common/ssl/tls_certificate_config_impl.cc

+        return message;
+      }()),
+      name_(config.name()), certificate_chain_(Config::DataSource::read(
+                                config.tls_certificate().certificate_chain(), true)),


The way you have structured the code, there is no guarantee that tls_certificate() is actually defined. You need more error checking here.

I don't feel that strongly about it, but I would in general prefer we do away w/ the dynamic casting and just have a base secret interface, and then sub-class interfaces for each secret type, and then potentially a way to lookup a secret in the manager by type. I think that would consolidate a lot of the error checking logic.

Created multi layered map and added template functions that add and lookup secret based on the type.
map<key, map<name, secret>>

qiwzhang · 2018-05-31T04:59:14Z

include/envoy/secret/secret_manager.h

@@ -17,14 +21,37 @@ class SecretManager {
  virtual ~SecretManager() {}

  /**
+   * @param secret a raw pointer of the derived class of Secret.
+   */
+  template <typename T> void addOrUpdateSecret(T* secret) {


It is so clear about the owership transfer when using raw pointer. Can we pass in shared_ptr ?

Changed variable type

qiwzhang · 2018-05-31T05:00:46Z

source/common/secret/secret_manager_impl.h


-  SecretSharedPtrMap static_secrets_;
+  std::unordered_map<std::string, std::unordered_map<std::string, SecretSharedPtr>> secrets_;


please add comment about two dimension maps, what is the first level key, what is the second level key and its value

Added comment

qiwzhang · 2018-05-31T05:11:30Z

source/common/ssl/context_config_impl.cc

+    }
+    return secret->privateKey();
+  } else {
+    return std::string("");


These two functions are similar. like to combine them. How about this:

string readConfig(config, mgr, READ_TCL_CERT_FN, READ_CONFIG_IMPL) {
if (!config.tls_certificates().empty()) {
return Config::DataSource::read(READ_TCL_CERT_FN(config.tls_certificates()[0])), true); } else if (!config.tls_certificate_sds_secret_configs().empty()) {
+
return READ_CONFIG_IMPL(second_impl);
}

typedef std::function<const DataStore& (const tcl_certificate&)> read_tcl_cert_fn;

lizan · 2018-05-31T17:30:27Z

include/envoy/secret/secret_manager.h

+   * add or update secret grouped by type.
+   * @param secret a shared_ptr of an implementation of Secret.
+   */
+  template <typename T> void addOrUpdateSecret(const SecretSharedPtr& secret) {


-1 to this heavy use of RTTI:
https://google.github.io/styleguide/cppguide.html#Run-Time_Type_Information__RTTI_

You should be able to add a type() method returning an string (or better enum) to Secret interface and just use it from here.

Created enum SecretType and removed RTTI

qiwzhang · 2018-05-31T22:21:40Z

source/common/secret/secret_manager_impl.cc

-    secrets_[type] = {};
-    type_secrets = secrets_.find(type);
+    secrets_[secret->type()] = {};
+    type_secrets = secrets_.find(secret->type());
  }

  type_secrets->second[secret->name()] = secret;


I think you can combine 7 to 13 to:

map[type][name] = secret;

mattklein123

Thanks, getting close.

mattklein123 · 2018-06-01T20:54:17Z

api/envoy/config/grpc_credentials/v2alpha/file_based_metadata.proto

@@ -22,4 +22,3 @@ message FileBasedMetadataConfig {
  // if no prefix is set, the default is to use no prefix
  string header_prefix = 3;
 }
-


Please merge master

It is weird. I haven't change the file. After I copied the file from the master branch I still see this change.

Could you fix that?

mattklein123 · 2018-06-01T21:00:14Z

source/server/configuration_impl.cc

+  ENVOY_LOG(info, "loading {} static secret(s)", secrets.size());
+  for (ssize_t i = 0; i < secrets.size(); i++) {
+    ENVOY_LOG(debug, "static secret #{}: {}", i, secrets[i].name());
+    server.secretManager().addOrUpdateSecret(


This is where the code should be to check for type. It doesn't make sense to assume a secret here is of this type given that we have a oneof(). Please add some type of factory function that switches on the type of secret and then create the appropriate type.

Currently we only support TLS certificate, the constructor of TlsCertificateConfigImpl is checking error. But I agree with your idea. I will create a SecretReadFactory function.

mattklein123 · 2018-06-01T21:34:22Z

source/common/ssl/context_config_impl.cc

+    if (!secret) {
+      throw EnvoyException(fmt::format("Static secret is not defined: {}", name));
+    }
+    return read_secret(std::dynamic_pointer_cast<Ssl::TlsCertificateConfigImpl>(secret));


I'm still really not a fan of this dynamic cast stuff. Is there any real benefit to doing it this way? Can we just have the secret manager manage different types of secrets and have different add/get functions for each of them? They can all derive from a base secret type so that common logic can be done across all secrets at a later time?

With that said, I don't feel super strongly about this. @PiotrSikora any opinions here given what we need to do in the future in this area?

@PiotrSikora Could you please update your opinions?

slightly +1 to @mattklein123. We can move back to this dynamic cast stuff where the type of secret increases.

qiwzhang · 2018-06-04T17:06:24Z

source/common/secret/secret_manager_impl.cc

+}
+
+const SecretSharedPtr
+SecretManagerImpl::loadSecret(const envoy::api::v2::auth::Secret& secret) const {


It seems that this should be a static function since it is NOT using any object's data.

Combined loadSecret to addOrUpdateSecret

qiwzhang · 2018-06-04T17:10:43Z

source/server/configuration_impl.cc

+  ENVOY_LOG(info, "loading {} static secret(s)", secrets.size());
+  for (ssize_t i = 0; i < secrets.size(); i++) {
+    ENVOY_LOG(debug, "static secret #{}: {}", i, secrets[i].name());
+    server.secretManager().addOrUpdateSecret(server.secretManager().loadSecret(secrets[i]));


these two calls can be combined into one call?

Combined loadSecret to addOrUpdateSecret

qiwzhang · 2018-06-04T17:12:29Z

test/common/secret/secret_manager_impl_test.cc

+
+class SecretManagerImplTest : public testing::Test {};
+
+TEST_F(SecretManagerImplTest, WeightedClusterFallthroughConfig) {


does the test case name match the test?

Changed the test name

qiwzhang · 2018-06-04T17:13:36Z

test/common/secret/secret_manager_impl_test.cc

+  tls_certificate->mutable_certificate_chain()->set_filename(
+      "test/common/ssl/test_data/selfsigned_cert.pem");
+  tls_certificate->mutable_private_key()->set_filename(
+      "test/common/ssl/test_data/selfsigned_key.pem");


use loadFromYaml()?

lizan · 2018-06-05T18:09:20Z

source/common/ssl/context_config_impl.cc

+    }
+    return read_secret(std::dynamic_pointer_cast<Ssl::TlsCertificateConfigImpl>(secret));
+  } else {
+    return std::string("");


nit: EMPTY_STRING

lizan · 2018-06-05T18:13:10Z

source/common/ssl/context_config_impl.cc

+    const envoy::api::v2::auth::CommonTlsContext& config, Secret::SecretManager& secret_manager,
+    const std::function<std::string(const envoy::api::v2::auth::TlsCertificate& tls_certificate)>&
+        read_inline_config,
+    const std::function<std::string(const std::shared_ptr<Ssl::TlsCertificateConfigImpl>& secret)>&


the std::function doesn't have to take shared_ptr, just use const Ssl::TlsCertificateConfigImpl&

lizan · 2018-06-05T18:13:57Z

source/common/ssl/context_config_impl.cc

+    if (!secret) {
+      throw EnvoyException(fmt::format("Static secret is not defined: {}", name));
+    }
+    return read_secret(std::dynamic_pointer_cast<Ssl::TlsCertificateConfigImpl>(secret));


slightly +1 to @mattklein123. We can move back to this dynamic cast stuff where the type of secret increases.

lizan · 2018-06-07T00:01:34Z

test/test_common/certs_test_expected.cc

@@ -0,0 +1,47 @@
+#include <string>


Seems these certificate data are from test/common/ssl/test_data/, which might be regenerated/modified in future.

You can just use TestEnvironment::readFileToStringForTest to read those data and use as expected strings.

lizan

Almost good to go, one comment in test.

lizan · 2018-06-07T04:13:11Z

source/common/ssl/context_config_impl.cc

+    if (!secret) {
+      throw EnvoyException(fmt::format("Static secret is not defined: {}", name));
+    }
+    return read_secret(*(std::dynamic_pointer_cast<Ssl::TlsCertificateConfigImpl>(secret)).get());


nit: dynamic_cast<const Ssl::TlsCertificateConfigImpl&>(*secret) should work.

I would rather just get rid of the dynamic casts. I don't think it's necessary. Since @PiotrSikora hasn't responded back, can we please rework this so that the secret manager supports different types of secrets with a normal class structure? I think that will be the cleanest moving forward. Thank you!

I will update. Thanks.

Removed dynamic casts

Sorry, those comments get hidden with every push and I've missed this one... I believe the find<TYPE>() is what I suggested to @mangchiandjjoe when we were discussing this together with @qiwzhang last week, so the current code works fine for me.

lizan · 2018-06-07T04:14:11Z

test/test_common/certs_test_expected.cc

@@ -0,0 +1,47 @@
+#include <string>


delete these two files.

lizan · 2018-06-07T04:47:15Z

test/mocks/secret/mocks.cc

@@ -0,0 +1,11 @@
+#include "mocks.h"


nit: full path test/mocks/secret/mocks.h

lizan · 2018-06-07T04:47:43Z

source/common/ssl/tls_certificate_config_impl.h

+#pragma once
+
+#include <string>
+#include <unordered_map>


nit: unused

lizan · 2018-06-07T04:48:43Z

test/mocks/secret/mocks.h

@@ -0,0 +1,28 @@
+#pragma once
+
+#include <chrono>


nit: clean unused includes

PiotrSikora · 2018-06-08T09:57:42Z

include/envoy/secret/secret_manager.h

+   * @param name a name of the TlsCertificateSecret.
+   * @return the TlsCertificate secret. Returns nullptr if the secret is not found.
+   */
+  virtual const TlsCertificateSecretSharedPtr


I think we can return const TlsCertificateSecret* here... we immediately extract raw pointer from the result of the call to this function anyway.

PiotrSikora · 2018-06-08T10:05:23Z

source/common/ssl/tls_certificate_secret_impl.h

+public:
+  TlsCertificateSecretImpl(const std::string& name,
+                           const envoy::api::v2::auth::TlsCertificate& config);
+  virtual ~TlsCertificateSecretImpl() {}


I don't think this destructor is needed?

PiotrSikora · 2018-06-08T10:12:13Z

source/common/ssl/tls_certificate_secret_impl.h

+namespace Envoy {
+namespace Ssl {
+
+class TlsCertificateSecretImpl : public Secret::TlsCertificateSecret {


I believe I mentioned this before, but this should be called TlsCertificateConfigImpl, since there is nothing Secret-specific about this, and both old-style certificates and SDS certificates could be represented by this object.

Basically, this should be materialized version of the TlsCertificate from the proto.

Renamed back to TlsCertificateConfigImpl

PiotrSikora · 2018-06-08T10:12:53Z

source/common/ssl/tls_certificate_secret_impl.h

+                           const envoy::api::v2::auth::TlsCertificate& config);
+  virtual ~TlsCertificateSecretImpl() {}
+
+  const std::string& name() const override { return name_; }


I don't believe this is ever used? It's just a lookup key for SecretManager, so it doesn't have to be part of the value itself.

PiotrSikora · 2018-06-08T10:18:11Z

include/envoy/secret/secret_manager.h

+   * @return the TlsCertificate secret. Returns nullptr if the secret is not found.
+   */
+  virtual const TlsCertificateSecretSharedPtr
+  findTlsCertificateSecret(const std::string& name) const PURE;


Nit: Secret seems to be redundant here, perhaps secretManager->findTlsCertificate() is meaningful enough?

mattklein123 · 2018-06-08T14:30:21Z

@mangchiandjjoe please ping me once @lizan and @PiotrSikora sign off. I'm generally OK with what we have now so will take a final pass once that is complete.

PiotrSikora · 2018-06-08T21:21:13Z

include/envoy/secret/secret.h

+namespace Secret {
+
+/**
+ * An instance of the TlsCertificateSecret


Following the other change, this should be part of Envoy::Ssl and not Envoy::Secret (nothing Secret-specific here).

Also, s/TlsCertificateSecret/TlsCertificateConfig/g.

Renamed Secret::Secret to Ssl::TlsCertificateConfig

PiotrSikora

Other than moving stuff out from Envoy::Secret and renaming the interface, code LGTM.

PiotrSikora · 2018-06-08T22:07:17Z

include/envoy/server/BUILD

@@ -172,6 +173,7 @@ envoy_cc_library(
    hdrs = ["transport_socket_config.h"],
    deps = [
        "//include/envoy/network:transport_socket_interface",
+        "//include/envoy/secret:secret_manager_interface",


Nit: This isn't needed anymore.

secret_manager_interface is required here again.

PiotrSikora · 2018-06-08T22:27:49Z

include/envoy/ssl/context_manager.h

+  /**
+   * Return the instance of secret manager.
+   */
+  virtual Secret::SecretManager& secretManager() PURE;


Actually, looking at this again, what's the reasoning for Ssl::ContextManager having SecretManager()?
Should you use the global one from Server::InstanceImpl->secretManager()?

Consumer of the secret manager is the createTransportSocketFactory of UpstreamSslSocketFactory and DownstreamSslSocketFactory.

sslContextManager() of Server::Configuration::TransportSocketFactoryContext& context is
available in that function.

Is there a direct access to the global one?

Looking at the code, I'd rather move it one level up and put it in Configuration::TransportSocketFactoryContext next to the sslContextManager(). That way, it's still going to be accessible in createTransportSocketFactory().

Note: perhaps once you add dynamic SDS there will be a justification for tying Ssl::ContextManager and Secret::SecretManager together (although, it would be probably the other way around), but I don't think there is a reason to do that just yet.

ListenerImpl and ClusterInfoImpl are implementations of Configuration::TransportSocketFactoryContext interface.
If I need to add secretManager() to those class, I need to update constructors of derived classes and instance creation codes .
Adding secretManager() to Ssl::ContextManagerImpl seems to be easy to use.
Please let me know your opinion.

ListenerImpl and ClusterInfoImpl are implementations of Configuration::TransportSocketFactoryContext interface.
If I need to add secretManager() to those class, I need to update constructors of derived classes and instance creation codes .

Adding secretManager() to listeners should be as easy as adding this line:

Secret::SecretManager& secretManager() override { return parent_.server_.secretManager(); }

For clusters, you'd indeed need to change the constructor, and I'd even say that you might change it to pass ClusterManagerImpl, to match listener, and then you should be able to return parent_.server_.secretManager() as well (perhaps in separate PR).

cc @mattklein123 for thoughts on this.

I'm fine either way. If @PiotrSikora has thought about it and thinks it's better I would just take that approach.

ok I am moving the method to the Configuration::TransportSocketFactoryContext interface.
But It requires 25 files changes and related test cases. It would take some time.

mattklein123

Thanks, approach LGTM. Just some last nits. Please also merge master. Not sure what you have the credentials build file change in there. @PiotrSikora @lizan PTAL for one last pass.

mattklein123 · 2018-06-12T22:57:02Z

include/envoy/secret/secret_manager.h

+
+  /**
+   * @param secret a protobuf message of envoy::api::v2::auth::Secret.
+   * @throw an EnovyEception when the secret type is not implemented yet.


typo "EnovyEception". "@throw an EnvoyException if the secret is invalid or not supported."

mattklein123 · 2018-06-12T22:58:20Z

include/envoy/ssl/tls_certificate_config.h

+namespace Ssl {
+
+/**
+ * An instance of the TlsCertificateConfig


This comment isn't very useful. Either remove or say more about what this is for. Also, periods end of sentences in this file.

mattklein123 · 2018-06-12T23:01:16Z

include/envoy/ssl/tls_certificate_config.h

+  virtual const std::string& privateKey() const PURE;
+};
+
+typedef std::shared_ptr<TlsCertificateConfig> TlsCertificateConfigSharedPtr;


Do we need shared pointers if we are only going to return const raw pointers? Can we make this unique_ptr?

To support dynamic secret, secrets managed by secrets managed referenced by multiple components.
It can be changed to unique_ptr but it needs to be change back to shared_ptr in the next PR.
I will update it to unique_ptr.

mattklein123 · 2018-06-12T23:03:05Z

source/common/secret/secret_manager_impl.h

+
+class SecretManagerImpl : public SecretManager, Logger::Loggable<Logger::Id::upstream> {
+public:
+  SecretManagerImpl() {}


mattklein123 · 2018-06-12T23:03:19Z

source/common/secret/secret_manager_impl.h

+  const Ssl::TlsCertificateConfig* findTlsCertificate(const std::string& name) const override;
+
+private:
+  // manages pairs of name and Ssl::TlsCertificateConfigSharedPtr


comment obvious, delete

mattklein123 · 2018-06-12T23:07:22Z

source/server/listener_manager_impl.h

@@ -299,6 +299,8 @@ class ListenerImpl : public Network::ListenerConfig,
  Ssl::ContextManager& sslContextManager() override { return parent_.server_.sslContextManager(); }
  Stats::Scope& statsScope() const override { return *listener_scope_; }

+  Secret::SecretManager& secretManager() override { return parent_.server_.secretManager(); }


nit: del newline before this line

mangchiandjjoe · 2018-06-13T02:37:11Z

@mattklein123 I am not sure how I can resolve the conflict.
I ran the "tools/check_format.py fix" command before I check in and it updated the file.
I copied the original content, but it makes format job failed.
It looks it was added recently by @lizan
Please let me know how I can resolve the issue?

lizan · 2018-06-13T02:53:52Z

since you're not changing any files in api try:

git checkout upstream/master -- api/config/ then commit

lizan · 2018-06-13T16:31:43Z

@mangchiandjjoe can you fix the build?

lizan

@mangchiandjjoe the one of last commits messed DCO again, can you squash once? Otherwise LGTM.

…urces Signed-off-by: Jae Kim <[email protected]>

PiotrSikora

You can avoid a lot of those changes if you pass SecretManager to Upstream::ProdClusterManagerFactory() instead of passing it as the argument in all the calls.

Also, instead of passing and storing each of TransportSocketFactoryContext's members (Ssl::ContextManager, Secret::SecretManager and Stats::Store) separately, we should probably be passing TransportSocketFactoryContext itself.

Signed-off-by: Jae Kim <[email protected]>

JimmyCYJ · 2018-06-15T19:54:52Z

@lizan and @PiotrSikora, this PR is ready for review now. could you please take a look?

mattklein123

This change is still missing doc updates (there are likely fields that need to be unhidden in the docs). In the interest of moving this along, I'm going to merge this, but please fix this and other nits in a follow up (there are still comments missing periods at end of sentences, etc.). Thank you!

mattklein123 · 2018-06-18T23:54:05Z

source/server/configuration_impl.cc

@@ -46,6 +46,13 @@ bool FilterChainUtility::buildFilterChain(
 void MainImpl::initialize(const envoy::config::bootstrap::v2::Bootstrap& bootstrap,
                          Instance& server,
                          Upstream::ClusterManagerFactory& cluster_manager_factory) {
+  const auto& secrets = bootstrap.static_resources().secrets();
+  ENVOY_LOG(info, "loading {} static secret(s)", secrets.size());
+  for (ssize_t i = 0; i < secrets.size(); i++) {


nit: this should be size_t I think

…p.static_resources (envoyproxy#3465)" revert to get clean master This reverts commit 08c1399.

mangchiandjjoe changed the title ~~sds: clusters and listeners read static secrets from Bootstrap.static_resources~~ [WIP] sds: clusters and listeners read static secrets from Bootstrap.static_resources May 23, 2018

mattklein123 mentioned this pull request May 23, 2018

sds: clusters and listeners read static secrets from Bootstrap.static_resources #3378

Closed

mangchiandjjoe changed the title ~~[WIP] sds: clusters and listeners read static secrets from Bootstrap.static_resources~~ sds: clusters and listeners read static secrets from Bootstrap.static_resources May 23, 2018

mattklein123 reviewed May 23, 2018

View reviewed changes

mattklein123 self-assigned this May 24, 2018

mattklein123 reviewed May 24, 2018

View reviewed changes

PiotrSikora suggested changes May 24, 2018

View reviewed changes

mattklein123 reviewed May 29, 2018

View reviewed changes

qiwzhang reviewed May 30, 2018

View reviewed changes

mattklein123 reviewed May 30, 2018

View reviewed changes

qiwzhang reviewed May 31, 2018

View reviewed changes

lizan reviewed May 31, 2018

View reviewed changes

qiwzhang reviewed May 31, 2018

View reviewed changes

mattklein123 reviewed Jun 1, 2018

View reviewed changes

qiwzhang reviewed Jun 4, 2018

View reviewed changes

lizan reviewed Jun 5, 2018

View reviewed changes

lizan reviewed Jun 7, 2018

View reviewed changes

mattklein123 assigned lizan Jun 7, 2018

lizan previously approved these changes Jun 8, 2018

View reviewed changes

PiotrSikora suggested changes Jun 8, 2018

View reviewed changes

mangchiandjjoe dismissed lizan’s stale review via d5b4744 June 8, 2018 17:01

PiotrSikora reviewed Jun 8, 2018

View reviewed changes

mattklein123 reviewed Jun 12, 2018

View reviewed changes

lizan reviewed Jun 14, 2018

View reviewed changes

clusters and listeners read static secrets from Bootstrap.static_reso…

c8fe612

…urces Signed-off-by: Jae Kim <[email protected]>

mangchiandjjoe force-pushed the sds_static_secret_integration branch from 9be0bc6 to c8fe612 Compare June 14, 2018 05:11

lizan previously approved these changes Jun 14, 2018

View reviewed changes

PiotrSikora suggested changes Jun 14, 2018

View reviewed changes

Added secretManager() to ClusterManagerFactory interface

8194b5b

Signed-off-by: Jae Kim <[email protected]>

mangchiandjjoe dismissed lizan’s stale review via 8194b5b June 14, 2018 18:56

mangchiandjjoe added 3 commits June 14, 2018 12:08

Removed unnecessary changes

3ebeff4

Signed-off-by: Jae Kim <[email protected]>

Changed the location of secret_manager argument

4753fe0

Signed-off-by: Jae Kim <[email protected]>

Changed the location of secret_manager argument

0b730b1

Signed-off-by: Jae Kim <[email protected]>

lizan approved these changes Jun 18, 2018

View reviewed changes

mattklein123 approved these changes Jun 18, 2018

View reviewed changes

mattklein123 merged commit 08c1399 into envoyproxy:master Jun 18, 2018

stevenzzzz added a commit to stevenzzzz/envoy that referenced this pull request Jun 19, 2018

Revert "sds: clusters and listeners read static secrets from Bootstra…

8e5bfa7

…p.static_resources (envoyproxy#3465)" revert to get clean master This reverts commit 08c1399.

qiwzhang mentioned this pull request Jul 20, 2018

Add integration tests for static secrets #3910

Merged

		@@ -0,0 +1,38 @@
		#pragma once

		#include <google/protobuf/util/json_util.h>


		#include "envoy/secret/secret.h"

		#include "common/json/json_loader.h"


		SecretSharedPtrMap static_secrets_;
		std::unordered_map<std::string, std::unordered_map<std::string, SecretSharedPtr>> secrets_;


		class SecretManagerImplTest : public testing::Test {};

		TEST_F(SecretManagerImplTest, WeightedClusterFallthroughConfig) {

sds: clusters and listeners read static secrets from Bootstrap.static_resources #3465

sds: clusters and listeners read static secrets from Bootstrap.static_resources #3465

Conversation

mangchiandjjoe commented May 23, 2018

mattklein123 commented May 23, 2018

lizan commented May 23, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment