Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
32f1d5d
sds config dump single cmt due to dco...
Jun 22, 2019
c017b07
spelling check.
Jun 22, 2019
4269751
fix the sds_api_test.
Jun 24, 2019
a8e2724
fix mock test and config mocks comments.
Jun 24, 2019
fabae9e
rewording config_dump and secret manager impl todo.
Jun 24, 2019
350e7c8
fix all tests in context_impl_test by changing to impl.
Jun 25, 2019
a24bcc8
fix spelling...
Jun 25, 2019
d5e329b
fix the clang tidy and ssl_socket mock setup.
Jun 25, 2019
9343b62
fix double reset issue. testing rocks!
Jun 25, 2019
3e7bdb2
Merge branch 'master' of https://github.com/envoyproxy/envoy into sds…
Jul 9, 2019
4bedd26
Merge branch 'master' of https://github.com/envoyproxy/envoy into sds…
Jul 10, 2019
7082ff7
change to add warming secrets.
Jul 11, 2019
f72888b
warming case unit test added.
Jul 11, 2019
d68b3f5
simplify context var.
Jul 11, 2019
40f83ed
change to pw and key redacted
Jul 12, 2019
022db7b
wip finish test case for static, fail...
Jul 12, 2019
da5d481
all tests pass, update to set_name for all.
Jul 15, 2019
742fa01
one more test for static validaiton context
Jul 15, 2019
6771fdd
Merge branch 'master' of https://github.com/envoyproxy/envoy into sds…
Jul 15, 2019
ec6449e
remove threadmodel todo question.
Jul 15, 2019
51a64ad
add integration test for sds config dump.
Jul 21, 2019
f75d116
Merge branch 'master' of https://github.com/envoyproxy/envoy into sds…
Jul 21, 2019
44ace74
clang format fix.
Jul 22, 2019
031da5c
update comments in configdump.proto.
Jul 22, 2019
aef1787
address comments.
Jul 22, 2019
67373f2
format fix.
Jul 22, 2019
62b9635
address comments, not redact for filename.
Jul 22, 2019
9fb8f42
get rid of uni ptr get.
Jul 22, 2019
5130216
add test for filename no redact.
Jul 23, 2019
e0f12dd
update the version history.
Jul 23, 2019
52938d1
lint fix and use the right rst syntax.
Jul 24, 2019
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
1 change: 1 addition & 0 deletions api/envoy/admin/v2alpha/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ api_proto_library_internal(
"//envoy/api/v2:lds",
"//envoy/api/v2:rds",
"//envoy/api/v2:srds",
"//envoy/api/v2/auth:cert",
"//envoy/config/bootstrap/v2:bootstrap",
],
)
Expand Down
29 changes: 29 additions & 0 deletions api/envoy/admin/v2alpha/config_dump.proto
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ option java_outer_classname = "ConfigDumpProto";
option java_multiple_files = true;
option java_package = "io.envoyproxy.envoy.admin.v2alpha";

import "envoy/api/v2/auth/cert.proto";
import "envoy/api/v2/cds.proto";
import "envoy/api/v2/lds.proto";
import "envoy/api/v2/rds.proto";
Expand Down Expand Up @@ -219,3 +220,31 @@ message ScopedRoutesConfigDump {
repeated DynamicScopedRouteConfigs dynamic_scoped_route_configs = 2
[(gogoproto.nullable) = false];
}

// Envoys SDS implementation fills this message with all secrets fetched dynamically via SDS.
message SecretsConfigDump {
// DynamicSecret contains secret information fetched via SDS.
Comment thread
incfly marked this conversation as resolved.
// The security sensitive information will trimmed, for example, private key.
Comment thread
incfly marked this conversation as resolved.
Outdated
message DynamicSecret {
// The name assigned to the secret.
string name = 1;

// This is the per-resource version information.
string version_info = 2;

// The timestamp when the secret was last updated.
google.protobuf.Timestamp last_updated = 3;

// The actual secret information. Security sensitive information is stripped.
// For example, private key in a TLS cert.
Comment thread
incfly marked this conversation as resolved.
Outdated
envoy.api.v2.auth.Secret secret = 4;
}

// The dynamically loaded active secrets. These are secrets that are available to service
// clusters or listeners.
repeated DynamicSecret dynamic_active_secrets = 1;

// The dynamically loaded warming secrets. These are secrets that are currently undergoing
// warming in preparation to service clusters or listeners.
repeated DynamicSecret dynamic_warming_secrets = 2;
}
Comment thread
incfly marked this conversation as resolved.
1 change: 1 addition & 0 deletions source/common/secret/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ envoy_cc_library(
"//include/envoy/server:transport_socket_config_interface",
"//source/common/common:assert_lib",
"//source/common/common:minimal_logger_lib",
"@envoy_api//envoy/admin/v2alpha:config_dump_cc",
"@envoy_api//envoy/api/v2/auth:cert_cc",
],
)
Expand Down
13 changes: 9 additions & 4 deletions source/common/secret/sds_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@ namespace Envoy {
namespace Secret {

SdsApi::SdsApi(envoy::api::v2::core::ConfigSource sds_config, absl::string_view sds_config_name,
Config::SubscriptionFactory& subscription_factory,
Config::SubscriptionFactory& subscription_factory, TimeSource& time_source,
ProtobufMessage::ValidationVisitor& validation_visitor, Stats::Store& stats,
Init::Manager& init_manager, std::function<void()> destructor_cb)
: init_target_(fmt::format("SdsApi {}", sds_config_name), [this] { initialize(); }),
stats_(stats), sds_config_(std::move(sds_config)), sds_config_name_(sds_config_name),
secret_hash_(0), clean_up_(std::move(destructor_cb)), validation_visitor_(validation_visitor),
subscription_factory_(subscription_factory) {
subscription_factory_(subscription_factory),
time_source_(time_source), secret_data_{sds_config_name_, "uninitialized",
time_source_.systemTime()} {
// TODO(JimmyCYJ): Implement chained_init_manager, so that multiple init_manager
// can be chained together to behave as one init_manager. In that way, we let
// two listeners which share same SdsApi to register at separate init managers, and
Expand All @@ -26,7 +28,7 @@ SdsApi::SdsApi(envoy::api::v2::core::ConfigSource sds_config, absl::string_view
}

void SdsApi::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt::Any>& resources,
const std::string&) {
const std::string& version_info) {
validateUpdateSize(resources.size());
auto secret =
MessageUtil::anyConvert<envoy::api::v2::auth::Secret>(resources[0], validation_visitor_);
Expand All @@ -44,7 +46,8 @@ void SdsApi::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt::Any>&
setSecret(secret);
update_callback_manager_.runCallbacks();
}

secret_data_.last_updated_ = time_source_.systemTime();
secret_data_.version_info_ = version_info;
init_target_.ready();
}

Expand Down Expand Up @@ -79,5 +82,7 @@ void SdsApi::initialize() {
subscription_->start({sds_config_name_});
}

SdsApi::SecretData SdsApi::secretData() { return secret_data_; }

} // namespace Secret
} // namespace Envoy
27 changes: 21 additions & 6 deletions source/common/secret/sds_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,21 @@ namespace Secret {
*/
class SdsApi : public Config::SubscriptionCallbacks {
public:
struct SecretData {
Comment thread
incfly marked this conversation as resolved.
const std::string resource_name;
Comment thread
incfly marked this conversation as resolved.
Outdated
std::string version_info_;
SystemTime last_updated_;
};

SdsApi(envoy::api::v2::core::ConfigSource sds_config, absl::string_view sds_config_name,
Config::SubscriptionFactory& subscription_factory,
Config::SubscriptionFactory& subscription_factory, TimeSource& time_source,
ProtobufMessage::ValidationVisitor& validation_visitor, Stats::Store& stats,
Init::Manager& init_manager, std::function<void()> destructor_cb);

// TODO: what's the thread model of the admin::config_dump vs xDS config updates?
// any lock mechanism needed?
SecretData secretData();

protected:
// Creates new secrets.
virtual void setSecret(const envoy::api::v2::auth::Secret&) PURE;
Expand Down Expand Up @@ -68,6 +78,8 @@ class SdsApi : public Config::SubscriptionCallbacks {
Cleanup clean_up_;
ProtobufMessage::ValidationVisitor& validation_visitor_;
Config::SubscriptionFactory& subscription_factory_;
TimeSource& time_source_;
SecretData secret_data_;
};

class TlsCertificateSdsApi;
Expand All @@ -90,17 +102,18 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider
Config::Utility::checkLocalInfo("TlsCertificateSdsApi", secret_provider_context.localInfo());
return std::make_shared<TlsCertificateSdsApi>(
sds_config, sds_config_name, secret_provider_context.clusterManager().subscriptionFactory(),
secret_provider_context.dispatcher().timeSource(),
secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(),
*secret_provider_context.initManager(), destructor_cb);
}

TlsCertificateSdsApi(const envoy::api::v2::core::ConfigSource& sds_config,
const std::string& sds_config_name,
Config::SubscriptionFactory& subscription_factory,
Config::SubscriptionFactory& subscription_factory, TimeSource& time_source,
ProtobufMessage::ValidationVisitor& validation_visitor, Stats::Store& stats,
Init::Manager& init_manager, std::function<void()> destructor_cb)
: SdsApi(sds_config, sds_config_name, subscription_factory, validation_visitor, stats,
init_manager, std::move(destructor_cb)) {}
: SdsApi(sds_config, sds_config_name, subscription_factory, time_source, validation_visitor,
stats, init_manager, std::move(destructor_cb)) {}

// SecretProvider
const envoy::api::v2::auth::TlsCertificate* secret() const override {
Expand Down Expand Up @@ -138,17 +151,19 @@ class CertificateValidationContextSdsApi : public SdsApi,
secret_provider_context.localInfo());
return std::make_shared<CertificateValidationContextSdsApi>(
sds_config, sds_config_name, secret_provider_context.clusterManager().subscriptionFactory(),
secret_provider_context.dispatcher().timeSource(),
secret_provider_context.messageValidationVisitor(), secret_provider_context.stats(),
*secret_provider_context.initManager(), destructor_cb);
}
CertificateValidationContextSdsApi(const envoy::api::v2::core::ConfigSource& sds_config,
const std::string& sds_config_name,
Config::SubscriptionFactory& subscription_factory,
TimeSource& time_source,
ProtobufMessage::ValidationVisitor& validation_visitor,
Stats::Store& stats, Init::Manager& init_manager,
std::function<void()> destructor_cb)
: SdsApi(sds_config, sds_config_name, subscription_factory, validation_visitor, stats,
init_manager, std::move(destructor_cb)) {}
: SdsApi(sds_config, sds_config_name, subscription_factory, time_source, validation_visitor,
stats, init_manager, std::move(destructor_cb)) {}

// SecretProvider
const envoy::api::v2::auth::CertificateValidationContext* secret() const override {
Expand Down
61 changes: 61 additions & 0 deletions source/common/secret/secret_manager_impl.cc
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
#include "common/secret/secret_manager_impl.h"

#include "envoy/admin/v2alpha/config_dump.pb.h"
#include "envoy/common/exception.h"

#include "common/common/assert.h"
#include "common/common/logger.h"
#include "common/secret/sds_api.h"
#include "common/secret/secret_provider_impl.h"
#include "common/ssl/certificate_validation_context_config_impl.h"
Expand All @@ -11,6 +13,9 @@
namespace Envoy {
namespace Secret {

SecretManagerImpl::SecretManagerImpl(Server::ConfigTracker& config_tracker)
: config_tracker_entry_(config_tracker.add("secrets", [this] { return dumpSecretConfigs(); })) {
}
void SecretManagerImpl::addStaticSecret(const envoy::api::v2::auth::Secret& secret) {
switch (secret.type_case()) {
case envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate: {
Expand Down Expand Up @@ -79,5 +84,61 @@ SecretManagerImpl::findOrCreateCertificateValidationContextProvider(
secret_provider_context);
}

ProtobufTypes::MessagePtr SecretManagerImpl::dumpSecretConfigs() {
auto config_dump = std::make_unique<envoy::admin::v2alpha::SecretsConfigDump>();
auto providers = certificate_providers_.allSecretProviders();
for (const auto& cert_secrets : providers) {
const auto& secret_data = cert_secrets->secretData();
const auto& tls_cert = cert_secrets->secret();
::envoy::admin::v2alpha::SecretsConfigDump_DynamicSecret* dump_secret;
bool secret_ready = tls_cert != nullptr;
Comment thread
incfly marked this conversation as resolved.
Outdated
if (secret_ready) {
dump_secret = config_dump->mutable_dynamic_active_secrets()->Add();
} else {
dump_secret = config_dump->mutable_dynamic_warming_secrets()->Add();
}
auto secret = dump_secret->mutable_secret();

ProtobufWkt::Timestamp last_updated_ts;
TimestampUtil::systemClockToTimestamp(secret_data.last_updated_, last_updated_ts);
dump_secret->set_version_info(secret_data.version_info_);
*dump_secret->mutable_last_updated() = last_updated_ts;
secret->set_name(secret_data.resource_name);
if (secret_ready) {
auto tls_certificate = secret->mutable_tls_certificate();
tls_certificate->MergeFrom(*tls_cert);
// We clear private key and password to avoid information leaking.j
Comment thread
incfly marked this conversation as resolved.
Outdated
// TODO(incfly): switch to more generic scrubbing mechanism once
// https://github.com/envoyproxy/envoy/issues/4757 is resolved.
tls_certificate->clear_private_key();
tls_certificate->clear_password();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps set those to "[redacted]" (for non-empty values) to make it possible to distinguish between value being removed for security reasons vs not being set in the first place?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, while you're at it, could you make similar change to LDS and CDS config dumps? Separate PR is fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean? The inlined_string typed key/password from tls_certificate used in LDS/CDS dump?

}
}

// Handling validation Context provided via SDS.
auto context_secret_provider = validation_context_providers_.allSecretProviders();
Comment thread
incfly marked this conversation as resolved.
Outdated
for (const auto& validation_context_secret : context_secret_provider) {
const auto& secret_data = validation_context_secret->secretData();
const auto& validation_context = validation_context_secret->secret();
::envoy::admin::v2alpha::SecretsConfigDump_DynamicSecret* dump_secret;
bool secret_ready = validation_context != nullptr;
if (secret_ready) {
dump_secret = config_dump->mutable_dynamic_active_secrets()->Add();
} else {
dump_secret = config_dump->mutable_dynamic_warming_secrets()->Add();
}
auto secret = dump_secret->mutable_secret();
ProtobufWkt::Timestamp last_updated_ts;
TimestampUtil::systemClockToTimestamp(secret_data.last_updated_, last_updated_ts);
dump_secret->set_version_info(secret_data.version_info_);
*dump_secret->mutable_last_updated() = last_updated_ts;
secret->set_name(secret_data.resource_name);
if (secret_ready) {
secret->mutable_validation_context()->MergeFrom(*validation_context);
}
}
return config_dump;
}

} // namespace Secret
} // namespace Envoy
18 changes: 17 additions & 1 deletion source/common/secret/secret_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
namespace Envoy {
namespace Secret {

class SecretManagerImpl : public SecretManager {
class SecretManagerImpl : public SecretManager, public Logger::Loggable<Logger::Id::secret> {
Comment thread
incfly marked this conversation as resolved.
Outdated
public:
SecretManagerImpl(Server::ConfigTracker& config_tracker);
void addStaticSecret(const envoy::api::v2::auth::Secret& secret) override;

TlsCertificateConfigProviderSharedPtr
Expand All @@ -42,6 +43,8 @@ class SecretManagerImpl : public SecretManager {
Server::Configuration::TransportSocketFactoryContext& secret_provider_context) override;

private:
ProtobufTypes::MessagePtr dumpSecretConfigs();

template <class SecretType>
class DynamicSecretProviders : public Logger::Loggable<Logger::Id::secret> {
public:
Expand All @@ -68,6 +71,17 @@ class SecretManagerImpl : public SecretManager {
return secret_provider;
}

std::vector<std::shared_ptr<SecretType>> allSecretProviders() {
std::vector<std::shared_ptr<SecretType>> providers;
for (const auto& secret_entry : dynamic_secret_providers_) {
Comment thread
incfly marked this conversation as resolved.
std::shared_ptr<SecretType> secret_provider = secret_entry.second.lock();
if (secret_provider) {
providers.push_back(std::move(secret_provider));
}
}
return providers;
}

private:
// Removes dynamic secret provider which has been deleted.
void removeDynamicSecretProvider(const std::string& map_key) {
Expand All @@ -91,6 +105,8 @@ class SecretManagerImpl : public SecretManager {
// map hash code of SDS config source and SdsApi object.
DynamicSecretProviders<TlsCertificateSdsApi> certificate_providers_;
DynamicSecretProviders<CertificateValidationContextSdsApi> validation_context_providers_;

Server::ConfigTracker::EntryOwnerPtr config_tracker_entry_;
};

} // namespace Secret
Expand Down
2 changes: 1 addition & 1 deletion source/server/config_validation/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ void ValidationInstance::initialize(const Options& options,
listener_manager_ = std::make_unique<ListenerManagerImpl>(*this, *this, *this, false);
thread_local_.registerThread(*dispatcher_, true);
runtime_loader_ = component_factory.createRuntime(*this, initial_config);
secret_manager_ = std::make_unique<Secret::SecretManagerImpl>();
secret_manager_ = std::make_unique<Secret::SecretManagerImpl>(admin().getConfigTracker());
ssl_context_manager_ =
createContextManager(Ssl::ContextManagerFactory::name(), api_->timeSource());
cluster_manager_factory_ = std::make_unique<Upstream::ValidationClusterManagerFactory>(
Expand Down
5 changes: 3 additions & 2 deletions source/server/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,7 @@ InstanceImpl::InstanceImpl(const Options& options, Event::TimeSystem& time_syste
ThreadLocal::Instance& tls, Thread::ThreadFactory& thread_factory,
Filesystem::Instance& file_system,
std::unique_ptr<ProcessContext> process_context)
: secret_manager_(std::make_unique<Secret::SecretManagerImpl>()), shutdown_(false),
options_(options), time_source_(time_system), restarter_(restarter),
: shutdown_(false), options_(options), time_source_(time_system), restarter_(restarter),
start_time_(time(nullptr)), original_start_time_(start_time_), stats_store_(store),
thread_local_(tls), api_(new Api::Impl(thread_factory, store, time_system, file_system)),
dispatcher_(api_->allocateDispatcher()),
Expand Down Expand Up @@ -334,6 +333,8 @@ void InstanceImpl::initialize(const Options& options,

loadServerFlags(initial_config.flagsPath());

secret_manager_ = std::make_unique<Secret::SecretManagerImpl>(admin_->getConfigTracker());

// Initialize the overload manager early so other modules can register for actions.
overload_manager_ = std::make_unique<OverloadManagerImpl>(
*dispatcher_, stats_store_, thread_local_, bootstrap_.overload_manager(),
Expand Down
1 change: 1 addition & 0 deletions test/common/secret/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ envoy_cc_test(
"//test/mocks/server:server_mocks",
"//test/test_common:environment_lib",
"//test/test_common:registry_lib",
"//test/test_common:simulated_time_system_lib",
"//test/test_common:utility_lib",
],
)
Expand Down
Loading