Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions api/envoy/api/v2/auth/cert.proto
Original file line number Diff line number Diff line change
Expand Up @@ -232,14 +232,14 @@ message CommonTlsContext {
// relaxed in the future.
repeated TlsCertificate tls_certificates = 2 [(validate.rules).repeated .max_items = 1];

// [#not-implemented-hide:]
// Configs for fetching TLS certificates via SDS API.
repeated SdsSecretConfig tls_certificate_sds_secret_configs = 6;

oneof validation_context_type {
// How to validate peer certificates.
CertificateValidationContext validation_context = 3;

// [#not-implemented-hide:]
// Config for fetching validation context via SDS API.
SdsSecretConfig validation_context_sds_secret_config = 7;
}

Expand Down Expand Up @@ -302,7 +302,6 @@ message DownstreamTlsContext {
}

// [#proto-status: experimental]
// [#not-implemented-hide:]
message SdsSecretConfig {
// Name (FQDN, UUID, SPKI, SHA256, etc.) by which the secret can be uniquely referred to.
// When both name and config are specified, then secret can be fetched and/or reloaded via SDS.
Expand All @@ -312,7 +311,6 @@ message SdsSecretConfig {
}

// [#proto-status: experimental]
// [#not-implemented-hide:]
message Secret {
// Name (FQDN, UUID, SPKI, SHA256, etc.) by which the secret can be uniquely referred to.
string name = 1;
Expand Down
16 changes: 16 additions & 0 deletions include/envoy/secret/secret_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,22 @@ class SecretManager {
virtual TlsCertificateConfigProviderSharedPtr findOrCreateTlsCertificateProvider(
const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context) PURE;

/**
* Finds and returns a dynamic secret provider associated to SDS config. Create
* a new one if such provider does not exist.
*
* @param config_source a protobuf message object containing a SDS config source.
* @param config_name a name that uniquely refers to the SDS config source.
* @param secret_provider_context context that provides components for creating and initializing
* secret provider.
* @return CertificateValidationContextConfigProviderSharedPtr the dynamic certificate validation
* context secret provider.
*/
virtual CertificateValidationContextConfigProviderSharedPtr
findOrCreateCertificateValidationContextProvider(
const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context) PURE;
};

} // namespace Secret
Expand Down
2 changes: 2 additions & 0 deletions source/common/secret/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,14 @@ envoy_cc_library(
"//include/envoy/local_info:local_info_interface",
"//include/envoy/runtime:runtime_interface",
"//include/envoy/secret:secret_provider_interface",
"//include/envoy/server:transport_socket_config_interface",
"//include/envoy/stats:stats_interface",
"//source/common/common:callback_impl_lib",
"//source/common/common:cleanup_lib",
"//source/common/config:resources_lib",
"//source/common/config:subscription_factory_lib",
"//source/common/protobuf:utility_lib",
"//source/common/ssl:certificate_validation_context_config_impl_lib",
"//source/common/ssl:tls_certificate_config_impl_lib",
],
)
12 changes: 4 additions & 8 deletions source/common/secret/sds_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,15 @@
#include "common/config/resources.h"
#include "common/config/subscription_factory.h"
#include "common/protobuf/utility.h"
#include "common/ssl/tls_certificate_config_impl.h"

namespace Envoy {
namespace Secret {

SdsApi::SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher,
Runtime::RandomGenerator& random, Stats::Store& stats,
Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager,
const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name,
std::function<void()> destructor_cb)
const envoy::api::v2::core::ConfigSource& sds_config,
const std::string& sds_config_name, std::function<void()> destructor_cb)
: local_info_(local_info), dispatcher_(dispatcher), random_(random), stats_(stats),
cluster_manager_(cluster_manager), sds_config_(sds_config), sds_config_name_(sds_config_name),
secret_hash_(0), clean_up_(destructor_cb) {
Expand Down Expand Up @@ -60,12 +59,9 @@ void SdsApi::onConfigUpdate(const ResourceVector& resources, const std::string&)
}

const uint64_t new_hash = MessageUtil::hash(secret);
if (new_hash != secret_hash_ &&
secret.type_case() == envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate) {
if (new_hash != secret_hash_) {
secret_hash_ = new_hash;
tls_certificate_secrets_ =
std::make_unique<Ssl::TlsCertificateConfigImpl>(secret.tls_certificate());

setSecret(secret);
update_callback_manager_.runCallbacks();
}

Expand Down
108 changes: 96 additions & 12 deletions source/common/secret/sds_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@
#include "envoy/runtime/runtime.h"
#include "envoy/secret/secret_callbacks.h"
#include "envoy/secret/secret_provider.h"
#include "envoy/server/transport_socket_config.h"
#include "envoy/stats/stats.h"
#include "envoy/upstream/cluster_manager.h"

#include "common/common/callback_impl.h"
#include "common/common/cleanup.h"
#include "common/ssl/certificate_validation_context_config_impl.h"
#include "common/ssl/tls_certificate_config_impl.h"

namespace Envoy {
namespace Secret {
Expand All @@ -24,13 +27,12 @@ namespace Secret {
* SDS API implementation that fetches secrets from SDS server via Subscription.
*/
class SdsApi : public Init::Target,
public TlsCertificateConfigProvider,
public Config::SubscriptionCallbacks<envoy::api::v2::auth::Secret> {
public:
SdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher,
Runtime::RandomGenerator& random, Stats::Store& stats,
Upstream::ClusterManager& cluster_manager, Init::Manager& init_manager,
const envoy::api::v2::core::ConfigSource& sds_config, std::string sds_config_name,
const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name,
std::function<void()> destructor_cb);

// Init::Target
Expand All @@ -43,14 +45,10 @@ class SdsApi : public Init::Target,
return MessageUtil::anyConvert<envoy::api::v2::auth::Secret>(resource).name();
}

// SecretProvider
const Ssl::TlsCertificateConfig* secret() const override {
return tls_certificate_secrets_.get();
}

Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override {
return update_callback_manager_.add(callback);
}
protected:
// Creates new secrets.
virtual void setSecret(const envoy::api::v2::auth::Secret&) PURE;
Common::CallbackManager<> update_callback_manager_;

private:
void runInitializeCallbackIfAny();
Expand All @@ -68,11 +66,97 @@ class SdsApi : public Init::Target,

uint64_t secret_hash_;
Cleanup clean_up_;
};

typedef std::shared_ptr<SdsApi> SdsApiSharedPtr;

/**
* TlsCertificateSdsApi implementation maintains and updates dynamic TLS certificate secrets.
*/
class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider {
public:
static SdsApiSharedPtr
create(Server::Configuration::TransportSocketFactoryContext& secret_provider_context,
const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name,
std::function<void()> destructor_cb) {
return std::make_shared<TlsCertificateSdsApi>(
secret_provider_context.localInfo(), secret_provider_context.dispatcher(),
secret_provider_context.random(), secret_provider_context.stats(),
secret_provider_context.clusterManager(), *secret_provider_context.initManager(),
sds_config, sds_config_name, destructor_cb);
}

TlsCertificateSdsApi(const LocalInfo::LocalInfo& local_info, Event::Dispatcher& dispatcher,
Runtime::RandomGenerator& random, 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, std::function<void()> destructor_cb)
: SdsApi(local_info, dispatcher, random, stats, cluster_manager, init_manager, sds_config,
sds_config_name, destructor_cb) {}

// SecretProvider
const Ssl::TlsCertificateConfig* secret() const override {
return tls_certificate_secrets_.get();
}
Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override {
return update_callback_manager_.add(callback);
}

protected:
void setSecret(const envoy::api::v2::auth::Secret& secret) override {
tls_certificate_secrets_ =
std::make_unique<Ssl::TlsCertificateConfigImpl>(secret.tls_certificate());
}

private:
Ssl::TlsCertificateConfigPtr tls_certificate_secrets_;
Common::CallbackManager<> update_callback_manager_;
};

typedef std::unique_ptr<SdsApi> SdsApiPtr;
/**
* CertificateValidationContextSdsApi implementation maintains and updates dynamic certificate
* validation context secrets.
*/
class CertificateValidationContextSdsApi : public SdsApi,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public CertificateValidationContextConfigProvider {
public:
static SdsApiSharedPtr
create(Server::Configuration::TransportSocketFactoryContext& secret_provider_context,
const envoy::api::v2::core::ConfigSource& sds_config, const std::string& sds_config_name,
std::function<void()> destructor_cb) {
return std::make_shared<CertificateValidationContextSdsApi>(
secret_provider_context.localInfo(), secret_provider_context.dispatcher(),
secret_provider_context.random(), secret_provider_context.stats(),
secret_provider_context.clusterManager(), *secret_provider_context.initManager(),
sds_config, sds_config_name, destructor_cb);
}
CertificateValidationContextSdsApi(const LocalInfo::LocalInfo& local_info,
Event::Dispatcher& dispatcher,
Runtime::RandomGenerator& random, Stats::Store& stats,
Upstream::ClusterManager& cluster_manager,
Init::Manager& init_manager,
const envoy::api::v2::core::ConfigSource& sds_config,
std::string sds_config_name,
std::function<void()> destructor_cb)
: SdsApi(local_info, dispatcher, random, stats, cluster_manager, init_manager, sds_config,
sds_config_name, destructor_cb) {}

// SecretProvider
const Ssl::CertificateValidationContextConfig* secret() const override {
return certificate_validation_context_secrets_.get();
}
Common::CallbackHandle* addUpdateCallback(std::function<void()> callback) override {
return update_callback_manager_.add(callback);
}

protected:
void setSecret(const envoy::api::v2::auth::Secret& secret) override {
certificate_validation_context_secrets_ =
std::make_unique<Ssl::CertificateValidationContextConfigImpl>(secret.validation_context());
}

private:
Ssl::CertificateValidationContextConfigPtr certificate_validation_context_secrets_;
};

} // namespace Secret
} // namespace Envoy
44 changes: 33 additions & 11 deletions source/common/secret/secret_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,31 +71,53 @@ void SecretManagerImpl::removeDynamicSecretProvider(const std::string& map_key)
ASSERT(num_deleted == 1, "");
}

TlsCertificateConfigProviderSharedPtr SecretManagerImpl::findOrCreateTlsCertificateProvider(
SdsApiSharedPtr SecretManagerImpl::findOrCreate(
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) {
const std::string map_key = sds_config_source.SerializeAsString() + config_name;

TlsCertificateConfigProviderSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock();
SdsApiSharedPtr secret_provider = dynamic_secret_providers_[map_key].lock();
if (!secret_provider) {
ASSERT(secret_provider_context.initManager() != nullptr);

// SdsApi is owned by ListenerImpl and ClusterInfo which are destroyed before
// SecretManagerImpl. It is safe to invoke this callback at the destructor of SdsApi.
std::function<void()> unregister_secret_provider = [map_key, this]() {
removeDynamicSecretProvider(map_key);
};

secret_provider = std::make_shared<SdsApi>(
secret_provider_context.localInfo(), secret_provider_context.dispatcher(),
secret_provider_context.random(), secret_provider_context.stats(),
secret_provider_context.clusterManager(), *secret_provider_context.initManager(),
sds_config_source, config_name, unregister_secret_provider);
secret_provider = create_fn(unregister_secret_provider);
dynamic_secret_providers_[map_key] = secret_provider;
}

return secret_provider;
}

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 = [&secret_provider_context, &sds_config_source, &config_name](
std::function<void()> unregister_secret_provider) -> SdsApiSharedPtr {
ASSERT(secret_provider_context.initManager() != nullptr);
return TlsCertificateSdsApi::create(secret_provider_context, sds_config_source, config_name,
unregister_secret_provider);
};
SdsApiSharedPtr secret_provider = findOrCreate(sds_config_source, config_name, create_fn);

return std::dynamic_pointer_cast<TlsCertificateConfigProvider>(secret_provider);
}

CertificateValidationContextConfigProviderSharedPtr
SecretManagerImpl::findOrCreateCertificateValidationContextProvider(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context) {
auto create_fn = [&secret_provider_context, &sds_config_source, &config_name](
std::function<void()> unregister_secret_provider) -> SdsApiSharedPtr {
ASSERT(secret_provider_context.initManager() != nullptr);
return CertificateValidationContextSdsApi::create(secret_provider_context, sds_config_source,
config_name, unregister_secret_provider);
};
SdsApiSharedPtr secret_provider = findOrCreate(sds_config_source, config_name, create_fn);

return std::dynamic_pointer_cast<CertificateValidationContextConfigProvider>(secret_provider);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

} // namespace Secret
} // namespace Envoy
15 changes: 12 additions & 3 deletions source/common/secret/secret_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "envoy/ssl/tls_certificate_config.h"

#include "common/common/logger.h"
#include "common/secret/sds_api.h"

namespace Envoy {
namespace Secret {
Expand All @@ -35,9 +36,18 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable<Logger::Id::sec
const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context) override;

CertificateValidationContextConfigProviderSharedPtr
findOrCreateCertificateValidationContextProvider(
const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name,
Server::Configuration::TransportSocketFactoryContext& secret_provider_context) override;

private:
// Remove dynamic secret provider which has been deleted.
// Removes dynamic secret provider which has been deleted.
void removeDynamicSecretProvider(const std::string& map_key);
// Finds or creates SdsApi object.
SdsApiSharedPtr findOrCreate(
const envoy::api::v2::core::ConfigSource& sds_config_source, const std::string& config_name,
std::function<SdsApiSharedPtr(std::function<void()> unregister_secret_provider)> create_fn);

// Manages pairs of secret name and TlsCertificateConfigProviderSharedPtr.
std::unordered_map<std::string, TlsCertificateConfigProviderSharedPtr>
Expand All @@ -48,8 +58,7 @@ class SecretManagerImpl : public SecretManager, Logger::Loggable<Logger::Id::sec
static_certificate_validation_context_providers_;

// map hash code of SDS config source and SdsApi object.
std::unordered_map<std::string, std::weak_ptr<TlsCertificateConfigProvider>>
dynamic_secret_providers_;
std::unordered_map<std::string, std::weak_ptr<SdsApi>> dynamic_secret_providers_;
};

} // namespace Secret
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,4 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl(
}

} // namespace Ssl
} // namespace Envoy
} // namespace Envoy
11 changes: 8 additions & 3 deletions source/common/ssl/context_config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ getCertificateValidationContextConfigProvider(
sds_secret_config.name()));
}
return secret_provider;
} else {
return factory_context.secretManager().findOrCreateCertificateValidationContextProvider(
sds_secret_config.sds_config(), sds_secret_config.name(), factory_context);
}
}
return nullptr;
Expand Down Expand Up @@ -98,7 +101,6 @@ ContextConfigImpl::ContextConfigImpl(
ecdh_curves_(StringUtil::nonEmptyStringOrDefault(
RepeatedPtrUtil::join(config.tls_params().ecdh_curves(), ":"), DEFAULT_ECDH_CURVES)),
tls_certficate_provider_(getTlsCertificateConfigProvider(config, factory_context)),
secret_update_callback_handle_(nullptr),
certficate_validation_context_provider_(
getCertificateValidationContextConfigProvider(config, factory_context)),
min_protocol_version_(
Expand All @@ -107,8 +109,11 @@ ContextConfigImpl::ContextConfigImpl(
TLS1_2_VERSION)) {}

ContextConfigImpl::~ContextConfigImpl() {
if (secret_update_callback_handle_) {
secret_update_callback_handle_->remove();
if (tc_update_callback_handle_) {
tc_update_callback_handle_->remove();
}
if (cvc_update_callback_handle_) {
cvc_update_callback_handle_->remove();
}
}

Expand Down
Loading