Support Envoy to fetch secrets using SDS service. by JimmyCYJ · Pull Request #4256 · envoyproxy/envoy

JimmyCYJ · 2018-08-26T05:20:45Z

Description: Use SDS api that fetches secrets from remote SDS server. Secrets are stored in Secret Provider. Listeners and Clusters are updated when secrets are received.
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-08-26T05:21:04Z

cc @htuch @lizan @PiotrSikora

dnoe · 2018-08-28T15:50:21Z

Load balancing senior maintainer load a bit.

JimmyCYJ · 2018-08-28T19:53:13Z

Hi @ggreenway, this PR is split from #4176. Please let me know if you have any questions. Thanks!

ggreenway · 2018-08-28T22:18:08Z

@lizan Can you take a first pass on this one please?

lizan · 2018-08-28T22:42:29Z

include/envoy/secret/secret_manager.h

+   * secret provider.
+   * @return TlsCertificateConfigProviderSharedPtr the dynamic TLS secret provider.
+   */
+  virtual TlsCertificateConfigProviderSharedPtr findOrCreateDynamicSecretProvider(


will this work for other Secret types? name it findOrCreateTlsCertificateProvider

This method name is updated to findOrCreateTlsCertificateProvider(). Will add findOrCreateCertificateValidationContextProvider() once #4264 is in. Thanks.

lizan · 2018-08-28T22:50:48Z

source/common/ssl/ssl_socket.cc

+}

 Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const {
  if (ssl_ctx_) {


while locking is not neccessary, you have to capture ssl_ctx_ into a local variable here, onAddOrUpdateSecret may overwrite it since it's on main thread.

Good point. Thanks! Fixed this issue and added comment above.

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

lizan · 2018-08-29T00:16:21Z

@ggreenway Done, I was already did passes in #4176 and before @JimmyCYJ sending out it.

ggreenway

Overall looks good.

Is there any unit-test coverage for the SslSocketFactory behavior when the secrets aren't ready? If not, please add some.

ggreenway · 2018-08-29T16:56:21Z

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);
+
+  RELEASE_ASSERT(dynamic_secret_providers_.erase(map_key) == 1, "");


style: I prefer not putting actions inside ASSERT, even if it's RELEASE_ASSERT.

auto num_deleted = dynamic_secret_providers_.erase(map_key); RELEASE_ASSERT(num_deleted == 1, "");

Also, does this need to be RELEASE_ASSERT? ASSERT is preferable in most cases.

Thanks for pointing this out. Moved erase() call outside of RELEASE_ASSERT.

RELEASE_ASSERT was recommended here comment

This should be RELEASE_ASSERT; otherwise in opt builds, this entire line disappears.

With it split into two lines, the rational for making it RELEASE_ASSERT instead of ASSERT is no longer true, so this should now be ASSERT

ggreenway · 2018-08-29T17:02:18Z

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) {
+  const std::string map_key = std::to_string(MessageUtil::hash(sds_config_source)) + config_name;


Why use a hash as part of the key? Doesn't this leave the (rare) possibility of a hash collision? Can we flatten sds_config_source to string representation instead of taking a hash?

Make sense. Thanks! rds_impl.cc also use string representation as key. Changed map_key to make it consistent with rds.

ggreenway · 2018-08-29T17:02:57Z

source/common/secret/secret_manager_impl.cc

+    Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
+  const std::string map_key = std::to_string(MessageUtil::hash(sds_config_source)) + config_name;
+
+  auto secret_provider = dynamic_secret_providers_[map_key].lock();


This would be easier to read with a typename instead of auto

Replaced with typename. Thanks.

ggreenway · 2018-08-29T17:15:42Z

source/common/ssl/context_config_impl.h

  unsigned maxProtocolVersion() const override { return max_protocol_version_; };

+  bool isReady() const override {
+    // either tls_certficate_provider_ is nullptr or


Nit: capitalize Either

Done. Thanks.

ggreenway · 2018-08-29T17:17:07Z

source/common/ssl/context_config_impl.h

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


|| tls_certificate_provider_->secret() != nullptr

ggreenway · 2018-08-29T17:25:15Z

source/common/ssl/ssl_socket.cc

+    return std::make_unique<Ssl::SslSocket>(std::move(ssl_ctx), Ssl::InitialState::Client);
  } else {
+    ENVOY_LOG(debug, "Create NotReadySslSocket");
+    stats_.upstream_connection_reset_by_sds_.inc();


This stat name is confusing to me. Maybe rename to "upstream_context_secrets_not_ready" or something.

Changed to upstream_context_secrets_not_ready

ggreenway · 2018-08-29T17:27:45Z

source/server/server.cc

      handler_(new ConnectionHandlerImpl(ENVOY_LOGGER(), *dispatcher_)),
-      random_generator_(std::move(random_generator)), listener_component_factory_(*this),
+      random_generator_(std::move(random_generator)),
+      secret_manager_(new Secret::SecretManagerImpl()), listener_component_factory_(*this),


std::make_unique

ggreenway · 2018-08-29T17:30:08Z

test/common/secret/secret_manager_impl_test.cc

+    Protobuf::RepeatedPtrField<envoy::api::v2::auth::Secret> secret_resources;
+    auto secret_config = secret_resources.Add();
+    MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), *secret_config);
+    std::dynamic_pointer_cast<SdsApi>(secret_provider)->onConfigUpdate(secret_resources, "");


Is there any way to refactor this so we don't have to do this cast?

I don't find a way to refactor this. The onConfigUpdate() is inherited from SubscriptionCallbacks, not SecretProvider.

Yeah I think there is no way to refactor this without dynamic_cast, but this could be dynamic_cast<SdsApi&>(*secret_provider).onConfigUpdate

ggreenway · 2018-08-29T17:37:31Z

test/integration/sds_dynamic_integration_test.cc

+  // Wait for ssl_context_updated_by_sds counter.
+  if (version_ == Network::Address::IpVersion::v4) {
+    test_server_->waitForCounterGe(
+        "listener.127.0.0.1_0.server_ssl_socket_factory.ssl_context_update_by_sds", 1);


It would be nice to be able to set the stat_prefix on listeners, so that you don't have to care which ip version the test is for. But that's a fix for another day.

Good point. Thanks!

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

JimmyCYJ · 2018-08-29T19:17:34Z

@ggreenway Thanks for the review! I have addressed some comments. Will work on the rest soon.

Add certificate validation context config provider and refactor ContextConfigImpl. Make static and inline CertificateValidationContext utilize it. This is independent of PR #4256. Once #4256 is in, next step is to support fetching CertificateValidationContext via validation_context_sds_secret_config. Risk Level: Low Testing: unit test, integration test Docs Changes: N/A Release Notes: N/A Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

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

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

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

JimmyCYJ · 2018-08-31T00:44:43Z

@ggreenway Thanks for the review! I have added two unit tests into ssl_socket_test.cc for NotReadySslSocket. Please take a look.

lizan · 2018-08-31T00:48:59Z

test/common/secret/secret_manager_impl_test.cc

+  NiceMock<Server::Configuration::MockTransportSocketFactoryContext> secret_context;
+
+  envoy::api::v2::core::ConfigSource config_source;
+  {


nit: why do you need this block?

I think I added that block to cover some some function call, but many changes have been made and this block is not needed.

lizan · 2018-08-31T00:50:29Z

test/common/secret/secret_manager_impl_test.cc


+TEST_F(SecretManagerImplTest, SdsDynamicSecretUpdateSuccess) {
+  Server::MockInstance server;
+  std::unique_ptr<SecretManager> secret_manager(new SecretManagerImpl());


nit: std::make_unique

lizan · 2018-08-31T00:52:16Z

test/common/secret/secret_manager_impl_test.cc

+    Protobuf::RepeatedPtrField<envoy::api::v2::auth::Secret> secret_resources;
+    auto secret_config = secret_resources.Add();
+    MessageUtil::loadFromYaml(TestEnvironment::substitute(yaml), *secret_config);
+    std::dynamic_pointer_cast<SdsApi>(secret_provider)->onConfigUpdate(secret_resources, "");


Yeah I think there is no way to refactor this without dynamic_cast, but this could be dynamic_cast<SdsApi&>(*secret_provider).onConfigUpdate

lizan · 2018-08-31T00:52:42Z

test/common/secret/secret_manager_impl_test.cc

+
+    auto secret_provider = secret_manager->findOrCreateTlsCertificateProvider(
+        config_source, "abc.com", secret_context);
+    std::string yaml =


changed to const std::string and replaced dynamic_pointer_cast with dynamic_cast. Thanks.

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

ggreenway · 2018-08-31T17:15:23Z

Waiting for review of most recent commit by @lizan

Pulling the following changes from github.com/envoyproxy/envoy: f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345) e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323) ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326) aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232) 5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250) c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257) 752483e Fixing the fix (envoyproxy#4333) 83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338) 7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309) 69474b3 admin: order stats in clusters json admin (envoyproxy#4306) 2d155f9 ppc64le build (envoyproxy#4183) 07efc6d fix static initialization fiasco problem (envoyproxy#4314) 0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297) 1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328) 0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319) cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335) f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322) e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329) 0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296) 00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321) af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318) 3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311) 42f6048 Proto string issue fix (envoyproxy#4320) 9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256) a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303) 1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307) 1212423 alts: add gRPC TSI socket (envoyproxy#4153) f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300) 01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304) 1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308) aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244) 89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269) 97eba59 build: bump googletest version. (envoyproxy#4293) 0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262) 9d094e5 Revert ac0bd74 (envoyproxy#4295) ddb28a4 Add validation context provider (envoyproxy#4264) 3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986) cf87d50 docs: update SNI FAQ. (envoyproxy#4285) f952033 config: fix update empty stat for eds (envoyproxy#4276) 329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219) 68d20b4 thrift: refactor build files and imports (envoyproxy#4271) 5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144) fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282) 53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927) c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247) cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188) b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220) 0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252) da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265) 9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253) 52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238) f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239) c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260) 35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255) ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258) b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248) 8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254) 28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233) f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245) Fixes istio/istio#8310 (once pulled into istio/istio). Signed-off-by: Piotr Sikora <piotrsikora@google.com>

JimmyCYJ added 16 commits August 24, 2018 15:31

Rebase to master.

2ada52e

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

add files.

98d9f26

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

Fix format.

98b7ffc

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

Revise per comments.

f33fe64

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

Revise per comments.

448dff3

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

Revise per comments.

41432b3

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

Revise per comments.

006c41c

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

Revise per comments.

95acda8

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

Revise per comments.

045dcdf

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

Revise per comments.

00d4d23

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

Remove unnecessary lock and fix compile issue.

6f75d4b

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

Revise per comments.

74f0d1e

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

Remove lock.

166a30f

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

Remove unused include.

98b29db

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

Update caller to use SdsApi.

b182b23

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

Check provider is nullptr.

508f4ab

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

lizan assigned lizan and htuch Aug 26, 2018

lizan requested a review from PiotrSikora August 26, 2018 08:49

This was referenced Aug 27, 2018

Add validation context provider #4264

Merged

Implementation of SDS #4176

Closed

dnoe assigned ggreenway and unassigned htuch Aug 28, 2018

lizan reviewed Aug 28, 2018

View reviewed changes

Revise per comments.

1b8e91e

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

lizan requested a review from ggreenway August 29, 2018 00:15

ggreenway requested changes Aug 29, 2018

View reviewed changes

Revise per comments.

336a950

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

JimmyCYJ added 10 commits August 29, 2018 13:34

Fix stats name.

f36a870

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

Fix compile error.

f02ae4b

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

Fix conflicts.

2844cb9

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

Fix format.

54a8dd1

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

Fix compile error.

599a459

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

Fix test.

11a358d

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

Replace RELEASE_ASSERT with ASSERT.

92a4fca

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

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

93778bf

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

Fix integration test caused by merging master.

cf3400a

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

Add unit tests to cover NotReadySslSocket.

a7b9d9f

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

lizan reviewed Aug 31, 2018

View reviewed changes

Revise per comments.

0dcd867

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

ggreenway approved these changes Aug 31, 2018

View reviewed changes

lizan approved these changes Aug 31, 2018

View reviewed changes

lizan merged commit 9c492a0 into envoyproxy:master Aug 31, 2018

Conversation

JimmyCYJ commented Aug 26, 2018

Uh oh!

JimmyCYJ commented Aug 26, 2018

Uh oh!

dnoe commented Aug 28, 2018

Uh oh!

JimmyCYJ commented Aug 28, 2018

Uh oh!

ggreenway commented Aug 28, 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!

lizan commented Aug 29, 2018

Uh oh!

ggreenway 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

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!

JimmyCYJ commented Aug 29, 2018

Uh oh!

JimmyCYJ commented Aug 31, 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!