secret: add secret provider interface and use it for TlsCertificates by lizan · Pull Request #4086 · envoyproxy/envoy

lizan · 2018-08-08T10:12:01Z

Signed-off-by: Lizan Zhou zlizan@google.com

Description:
Add secret provider interface which is part of DynamicTlsCertificateProvider interface proposed in #3700. Make static and inline TlsCertificate utilize it.

Risk Level: Low
Testing: unit test, integration test
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan · 2018-08-08T10:12:20Z

cc @JimmyCYJ @qiwzhang

Signed-off-by: Lizan Zhou <zlizan@google.com>

qiwzhang · 2018-08-08T17:09:14Z

include/envoy/ssl/context_config.h

   */
  virtual const std::string& certificateRevocationListPath() const PURE;

+  // TODO(lizan): consider refactor 4 methods below to Ssl::TlsCertificateConfig


+1 for this change.

FWIW: lizan/envoy@static_secret_provider...tls_certificate_config , I prefer to have this change in a follow up PRs, but if reviewers prefer in same PR, I will add a commit.

qiwzhang

LGTM

JimmyCYJ

LGTM

lizan · 2018-08-08T21:11:05Z

@ggreenway and/or @mattklein123, PTAL. For a non-Google review.

mattklein123 · 2018-08-08T21:14:45Z

Yes I will look. Please assign me as a reviewer on all SDS PRs.

PiotrSikora

I'm probably missing something, but what's the reason for having TlsCertificateConfigProvider, which is effectively a wrapper around TlsCertificateConfig?

I do agree that we need to move all certificate-specific fields, regardless of whether they were read from file, inlined or fetched via SDS, into a common object, which is why I proposed TlsCertificateConfig a few SDS PRs ago, but I don't see the reason for another layer of misdirection?

PiotrSikora · 2018-08-08T23:56:56Z

include/envoy/secret/secret_manager.h

+   * @param tls_certificate the protobuf config of the TLS certificate.
+   * @return a TlsCertificateConfigProviderSharedPtr created from tls_certificate.
+   */
+  virtual TlsCertificateConfigProviderSharedPtr createInlineTlsCertificateProvider(


Why do we need this? And couldn't this be static? It doesn't seem to be modifying or requiring SecretManager at all.

It could be static with current implementation since I wanted to keep this PR as a refactor. Though secret_manager might do more in future, (e.g. returning same provider if config is exactly same), and/or track all secrets for dump config like rds does.

Sorry what is the different between an inline secret and a static secret? I'm confused.

inline secret is the secret directly embedded in the config. static secrets are specified in the static_resources, the config only has a name to point to it.

OK then I guess I have the same question as @PiotrSikora. Why isn't this static?

My understanding is: inline is the legacy way of configuring ssl cert. statis secrets are only added when SDS works started to share secrets used in many places. By removing inline, it may break existing users.

I'm not saying to remove inline, I think that will always be there. My question is why isn't inline just a static secret?

@mattklein123 IIUC @PiotrSikora asks why not making the C++ method static, your question is to merge inline secret and static secret.
Static secrets can be only registered via static_resources in bootstrap.proto, looked up by name, owned by secret manager. Inline secrets are in each tls_context, have no name, in future, secret manager may track more than current implementation. They have different lifecycles.

Ah I see. OK.

lizan · 2018-08-09T00:10:57Z

I'm probably missing something, but what's the reason for having TlsCertificateConfigProvider, which is effectively a wrapper around TlsCertificateConfig?

It is to extract the callbacks for dynamic secret, which will swap the holding TlsCertificateConfigPtr, like this: https://github.com/envoyproxy/envoy/pull/3700/files#diff-93a68bfee9eaf7ed295925ab50201db8R16

mattklein123 · 2018-08-10T17:53:00Z

include/envoy/secret/secret_provider.h

+namespace Secret {
+
+/**
+ * A secret provider for each kind of secrets.


nit: s/secrets/secret

mattklein123 · 2018-08-10T17:54:01Z

source/common/config/datasource.h

@@ -19,7 +21,7 @@ std::string read(const envoy::api::v2::core::DataSource& source, bool allow_empt
 * @param source data source.
 * @return std::string path to DataSource if a filename, otherwise an empty string.


mattklein123 · 2018-08-10T17:55:42Z

include/envoy/secret/secret_manager.h

+   * @param tls_certificate the protobuf config of the TLS certificate.
+   * @return a TlsCertificateConfigProviderSharedPtr created from tls_certificate.
+   */
+  virtual TlsCertificateConfigProviderSharedPtr createInlineTlsCertificateProvider(


Sorry what is the different between an inline secret and a static secret? I'm confused.

mattklein123

LGTM after typos.

Signed-off-by: Lizan Zhou <zlizan@google.com>

…vider

* origin/master: (38 commits) test: add tests for corner-cases around sending requests before run() starts or after run() ends. (envoyproxy#4114) perf: reduce the memory usage of LC Trie construction (envoyproxy#4117) test: moving redundant code in websocket_integration_test to utilities (envoyproxy#4127) test: make YamlLoadFromStringFail less picky about error msg. (envoyproxy#4141) rbac: add rbac network filter. (envoyproxy#4083) fuzz: route lookup and header finalization fuzzer. (envoyproxy#4116) Set content-type and content-length (envoyproxy#4113) fault: use FractionalPercent for percent (envoyproxy#3978) test: Fix inverted exact match logic in IntegrationTcpClient::waitForData() (envoyproxy#4134) Added cluster_name to load assignment config for static cluster (envoyproxy#4123) ssl: refactor ContextConfig to use TlsCertificateConfig (envoyproxy#4115) syscall: refactor OsSysCalls for deeper errno latching (envoyproxy#4111) thrift_proxy: fix oneway bugs (envoyproxy#4025) Do not crash when converting YAML to JSON fails (envoyproxy#4110) config: allow unknown fields flag (take 2) (envoyproxy#4096) Use a jittered backoff strategy for handling HdsDelegate stream/connection failures (envoyproxy#4108) bazel: use GCS remote cache (envoyproxy#4050) Add thread local cache of overload action states (envoyproxy#4090) Added TCP healthcheck capabilities to the HdsDelegate (envoyproxy#4079) secret: add secret provider interface and use it for TlsCertificates (envoyproxy#4086) ... Signed-off-by: Snow Pettersen <snowp@squareup.com>

secret: add secret provider interface and use it for TlsCertificates

c5615b3

Signed-off-by: Lizan Zhou <zlizan@google.com>

lizan requested a review from PiotrSikora August 8, 2018 10:12

remove unused include

232a46e

Signed-off-by: Lizan Zhou <zlizan@google.com>

alyssawilk assigned PiotrSikora Aug 8, 2018

qiwzhang reviewed Aug 8, 2018

View reviewed changes

qiwzhang previously approved these changes Aug 8, 2018

View reviewed changes

JimmyCYJ reviewed Aug 8, 2018

View reviewed changes

JimmyCYJ previously approved these changes Aug 8, 2018

View reviewed changes

lizan requested review from ggreenway and mattklein123 August 8, 2018 21:10

mattklein123 self-assigned this Aug 8, 2018

PiotrSikora reviewed Aug 9, 2018

View reviewed changes

mattklein123 reviewed Aug 10, 2018

View reviewed changes

lizan added 2 commits August 10, 2018 13:45

fix comments

a7386ba

Signed-off-by: Lizan Zhou <zlizan@google.com>

Merge remote-tracking branch 'upstream/master' into static_secret_pro…

85339f8

…vider

lizan dismissed stale reviews from JimmyCYJ and qiwzhang via 85339f8 August 10, 2018 20:46

mattklein123 approved these changes Aug 11, 2018

View reviewed changes

lizan merged commit 98037ed into envoyproxy:master Aug 11, 2018

lizan deleted the static_secret_provider branch August 11, 2018 20:53

lizan mentioned this pull request Aug 11, 2018

ssl: refactor ContextConfig to use TlsCertificateConfig #4115

Merged

PiotrSikora mentioned this pull request Aug 25, 2018

Envoy segfaults when validating TLS ADS cluster #4213

Closed

		@@ -19,7 +21,7 @@ std::string read(const envoy::api::v2::core::DataSource& source, bool allow_empt
		* @param source data source.
		* @return std::string path to DataSource if a filename, otherwise an empty string.

Conversation

lizan commented Aug 8, 2018

Uh oh!

lizan commented Aug 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang left a comment

Choose a reason for hiding this comment

Uh oh!

JimmyCYJ left a comment

Choose a reason for hiding this comment

Uh oh!

lizan commented Aug 8, 2018

Uh oh!

mattklein123 commented Aug 8, 2018

Uh oh!

PiotrSikora 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!

qiwzhang Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lizan commented Aug 9, 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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

qiwzhang Aug 10, 2018 •

edited

Loading