Revise per comments.

Signed-off-by: JimmyCYJ <[email protected]>
envoyproxy · Aug 20, 2018 · d6eb302 · d6eb302
1 parent 1aaaa12
commit d6eb302
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 4 deletions.
diff --git a/source/common/ssl/ssl_socket.cc b/source/common/ssl/ssl_socket.cc
@@ -427,12 +427,17 @@ ClientSslSocketFactory::ClientSslSocketFactory(ClientContextConfigPtr config,
                                                Ssl::ContextManager& manager,
                                                Stats::Scope& stats_scope)
     : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("client", stats_scope)),
-      config_(std::move(config)),
-      ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) {
+      config_(std::move(config)) {
   config_->setSecretUpdateCallback(*this);
+
+  std::unique_lock<std::shared_timed_mutex> lock(ssl_ctx_mutex_);
+  ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_);
 }
 
 Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() const {
+  // SDS would update ssl_ctx_ when Envoy is running.
+  // Need a read lock to let multiple threads gain read access to ssl_ctx_.
+  std::shared_lock<std::shared_timed_mutex> lock(ssl_ctx_mutex_);
   if (ssl_ctx_) {
     return std::make_unique<Ssl::SslSocket>(ssl_ctx_, Ssl::InitialState::Client);
   } else {
@@ -445,6 +450,11 @@ Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket() cons
 bool ClientSslSocketFactory::implementsSecureTransport() const { return true; }
 
 void ClientSslSocketFactory::onAddOrUpdateSecret() {
+  // SSL context update happens when Envoy is running and SDS is updating SSL
+  // context, need a write lock to make sure only main thread could have write access
+  // to ssl_ctx_.
+  std::unique_lock<std::shared_timed_mutex> lock(ssl_ctx_mutex_);
+
   ENVOY_LOG(debug, "Secret is updated.");
   ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_);
   stats_.ssl_context_update_by_sds_.inc();
@@ -455,12 +465,17 @@ ServerSslSocketFactory::ServerSslSocketFactory(ServerContextConfigPtr config,
                                                Stats::Scope& stats_scope,
                                                const std::vector<std::string>& server_names)
     : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("server", stats_scope)),
-      config_(std::move(config)), server_names_(server_names),
-      ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_)) {
+      config_(std::move(config)), server_names_(server_names) {
   config_->setSecretUpdateCallback(*this);
+
+  std::unique_lock<std::shared_timed_mutex> lock(ssl_ctx_mutex_);
+  ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_);
 }
 
 Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() const {
+  // SDS would update ssl_ctx_ when Envoy is running.
+  // Need a read lock to let multiple threads gain read access to ssl_ctx_.
+  std::shared_lock<std::shared_timed_mutex> lock(ssl_ctx_mutex_);
   if (ssl_ctx_) {
     return std::make_unique<Ssl::SslSocket>(ssl_ctx_, Ssl::InitialState::Server);
   } else {
@@ -473,6 +488,11 @@ Network::TransportSocketPtr ServerSslSocketFactory::createTransportSocket() cons
 bool ServerSslSocketFactory::implementsSecureTransport() const { return true; }
 
 void ServerSslSocketFactory::onAddOrUpdateSecret() {
+  // SSL context update happens when Envoy is running and SDS is updating SSL
+  // context, need a write lock to make sure only main thread could have write access
+  // to ssl_ctx_.
+  std::unique_lock<std::shared_timed_mutex> lock(ssl_ctx_mutex_);
+
   ENVOY_LOG(debug, "Secret is updated.");
   ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_);
   stats_.ssl_context_update_by_sds_.inc();

diff --git a/source/common/ssl/ssl_socket.h b/source/common/ssl/ssl_socket.h
@@ -1,6 +1,7 @@
 #pragma once
 
 #include <cstdint>
+#include <shared_mutex>
 #include <string>
 
 #include "envoy/network/connection.h"
@@ -102,6 +103,9 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory,
   SslSocketFactoryStats stats_;
   ClientContextConfigPtr config_;
   ClientContextSharedPtr ssl_ctx_;
+  // Protects ssl_ctx_ from read access by multiple threads, and guarantees only
+  // one thread can write to ssl_ctx_.
+  mutable std::shared_timed_mutex ssl_ctx_mutex_;
 };
 
 class ServerSslSocketFactory : public Network::TransportSocketFactory,
@@ -124,6 +128,9 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory,
   ServerContextConfigPtr config_;
   const std::vector<std::string> server_names_;
   ServerContextSharedPtr ssl_ctx_;
+  // Protects ssl_ctx_ from read access by multiple threads, and guarantees only
+  // one thread can write to ssl_ctx_.
+  mutable std::shared_timed_mutex ssl_ctx_mutex_;
 };
 
 } // namespace Ssl