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
10 changes: 9 additions & 1 deletion include/envoy/secret/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,19 @@ load(

envoy_package()

envoy_cc_library(
name = "secret_provider_interface",
hdrs = ["secret_provider.h"],
deps = [
"//include/envoy/ssl:tls_certificate_config_interface",
],
)

envoy_cc_library(
name = "secret_manager_interface",
hdrs = ["secret_manager.h"],
deps = [
"//include/envoy/ssl:tls_certificate_config_interface",
":secret_provider_interface",
"@envoy_api//envoy/api/v2/auth:cert_cc",
],
)
23 changes: 16 additions & 7 deletions include/envoy/secret/secret_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
#include <string>

#include "envoy/api/v2/auth/cert.pb.h"
#include "envoy/ssl/tls_certificate_config.h"
#include "envoy/secret/secret_provider.h"

namespace Envoy {
namespace Secret {
Expand All @@ -18,16 +18,25 @@ class SecretManager {
virtual ~SecretManager() {}

/**
* @param secret a protobuf message of envoy::api::v2::auth::Secret.
* @throw an EnvoyException if the secret is invalid or not supported.
* @param add a static secret from envoy::api::v2::auth::Secret.
* @throw an EnvoyException if the secret is invalid or not supported, or there is duplicate.
*/
virtual void addOrUpdateSecret(const envoy::api::v2::auth::Secret& secret) PURE;
virtual void addStaticSecret(const envoy::api::v2::auth::Secret& secret) PURE;

/**
* @param name a name of the Ssl::TlsCertificateConfig.
* @return the TlsCertificate secret. Returns nullptr if the secret is not found.
* @param name a name of the static TlsCertificateConfigProvider.
* @return the TlsCertificateConfigProviderSharedPtr. Returns nullptr if the static secret is not
* found.
*/
virtual const Ssl::TlsCertificateConfig* findTlsCertificate(const std::string& name) const PURE;
virtual TlsCertificateConfigProviderSharedPtr
findStaticTlsCertificateProvider(const std::string& name) const PURE;

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

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.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

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.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@qiwzhang qiwzhang Aug 10, 2018

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.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah I see. OK.

const envoy::api::v2::auth::TlsCertificate& tls_certificate) PURE;
};

} // namespace Secret
Expand Down
28 changes: 28 additions & 0 deletions include/envoy/secret/secret_provider.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#pragma once

#include "envoy/common/pure.h"
#include "envoy/ssl/tls_certificate_config.h"

namespace Envoy {
namespace Secret {

/**
* A secret provider for each kind of secret.
*/
template <class SecretType> class SecretProvider {
public:
virtual ~SecretProvider() {}

/**
* @return the secret. Returns nullptr if the secret is not ready.
*/
virtual const SecretType* secret() const PURE;

// TODO(lizan): Add more methods for dynamic secret provider.
};

typedef SecretProvider<Ssl::TlsCertificateConfig> TlsCertificateConfigProvider;
typedef std::shared_ptr<TlsCertificateConfigProvider> TlsCertificateConfigProviderSharedPtr;

} // namespace Secret
} // namespace Envoy
1 change: 1 addition & 0 deletions include/envoy/ssl/context_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ class ContextConfig {
*/
virtual const std::string& certificateRevocationListPath() const PURE;

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

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.

+1 for this change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

/**
* @return The certificate chain used to identify the local side.
*/
Expand Down
12 changes: 12 additions & 0 deletions include/envoy/ssl/tls_certificate_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,22 @@ class TlsCertificateConfig {
*/
virtual const std::string& certificateChain() const PURE;

/**
* @return Path of the certificate chain used to identify the local side or "<inline>"
* if the certificate chain was inlined.
*/
virtual const std::string& certificateChainPath() const PURE;

/**
* @return a string of private key
*/
virtual const std::string& privateKey() const PURE;

/**
* @return Path of the private key used to identify the local side or "<inline>"
* if the private key was inlined.
*/
virtual const std::string& privateKeyPath() const PURE;
};

typedef std::unique_ptr<TlsCertificateConfig> TlsCertificateConfigPtr;
Expand Down
7 changes: 4 additions & 3 deletions source/common/config/datasource.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,10 @@ std::string read(const envoy::api::v2::core::DataSource& source, bool allow_empt
}
}

std::string getPath(const envoy::api::v2::core::DataSource& source) {
return source.specifier_case() == envoy::api::v2::core::DataSource::kFilename ? source.filename()
: "";
absl::optional<std::string> getPath(const envoy::api::v2::core::DataSource& source) {
return source.specifier_case() == envoy::api::v2::core::DataSource::kFilename
? absl::make_optional(source.filename())
: absl::nullopt;
}

} // namespace DataSource
Expand Down
6 changes: 4 additions & 2 deletions source/common/config/datasource.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

#include "envoy/api/v2/core/base.pb.h"

#include "absl/types/optional.h"

namespace Envoy {
namespace Config {
namespace DataSource {
Expand All @@ -17,9 +19,9 @@ 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.
* @return absl::optional<std::string> path to DataSource if a filename, otherwise absl::nullopt.
*/
std::string getPath(const envoy::api::v2::core::DataSource& source);
absl::optional<std::string> getPath(const envoy::api::v2::core::DataSource& source);

} // namespace DataSource
} // namespace Config
Expand Down
11 changes: 11 additions & 0 deletions source/common/secret/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,19 @@ envoy_cc_library(
srcs = ["secret_manager_impl.cc"],
hdrs = ["secret_manager_impl.h"],
deps = [
":secret_provider_impl_lib",
"//include/envoy/secret:secret_manager_interface",
"//source/common/common:minimal_logger_lib",
"@envoy_api//envoy/api/v2/auth:cert_cc",
],
)

envoy_cc_library(
name = "secret_provider_impl_lib",
srcs = ["secret_provider_impl.cc"],
hdrs = ["secret_provider_impl.h"],
deps = [
"//include/envoy/secret:secret_provider_interface",
"//source/common/ssl:tls_certificate_config_impl_lib",
"@envoy_api//envoy/api/v2/auth:cert_cc",
],
Expand Down
29 changes: 21 additions & 8 deletions source/common/secret/secret_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,39 @@

#include "envoy/common/exception.h"

#include "common/common/assert.h"
#include "common/secret/secret_provider_impl.h"
#include "common/ssl/tls_certificate_config_impl.h"

namespace Envoy {
namespace Secret {

void SecretManagerImpl::addOrUpdateSecret(const envoy::api::v2::auth::Secret& secret) {
void SecretManagerImpl::addStaticSecret(const envoy::api::v2::auth::Secret& secret) {
switch (secret.type_case()) {
case envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate:
tls_certificate_secrets_[secret.name()] =
std::make_unique<Ssl::TlsCertificateConfigImpl>(secret.tls_certificate());
case envoy::api::v2::auth::Secret::TypeCase::kTlsCertificate: {
auto secret_provider =
std::make_shared<TlsCertificateConfigProviderImpl>(secret.tls_certificate());
if (!static_tls_certificate_providers_.insert(std::make_pair(secret.name(), secret_provider))
.second) {
throw EnvoyException(
fmt::format("Duplicate static TlsCertificate secret name {}", secret.name()));
}
break;
}
default:
throw EnvoyException("Secret type not implemented");
}
}

const Ssl::TlsCertificateConfig*
SecretManagerImpl::findTlsCertificate(const std::string& name) const {
auto secret = tls_certificate_secrets_.find(name);
return (secret != tls_certificate_secrets_.end()) ? secret->second.get() : nullptr;
TlsCertificateConfigProviderSharedPtr
SecretManagerImpl::findStaticTlsCertificateProvider(const std::string& name) const {
auto secret = static_tls_certificate_providers_.find(name);
return (secret != static_tls_certificate_providers_.end()) ? secret->second : nullptr;
}

TlsCertificateConfigProviderSharedPtr SecretManagerImpl::createInlineTlsCertificateProvider(
const envoy::api::v2::auth::TlsCertificate& tls_certificate) {
return std::make_shared<TlsCertificateConfigProviderImpl>(tls_certificate);
}

} // namespace Secret
Expand Down
11 changes: 8 additions & 3 deletions source/common/secret/secret_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <unordered_map>

#include "envoy/secret/secret_manager.h"
#include "envoy/secret/secret_provider.h"
#include "envoy/ssl/tls_certificate_config.h"

#include "common/common/logger.h"
Expand All @@ -12,11 +13,15 @@ namespace Secret {

class SecretManagerImpl : public SecretManager, Logger::Loggable<Logger::Id::upstream> {
public:
void addOrUpdateSecret(const envoy::api::v2::auth::Secret& secret) override;
const Ssl::TlsCertificateConfig* findTlsCertificate(const std::string& name) const override;
void addStaticSecret(const envoy::api::v2::auth::Secret& secret) override;
TlsCertificateConfigProviderSharedPtr
findStaticTlsCertificateProvider(const std::string& name) const override;
TlsCertificateConfigProviderSharedPtr createInlineTlsCertificateProvider(
const envoy::api::v2::auth::TlsCertificate& tls_certificate) override;

private:
std::unordered_map<std::string, Ssl::TlsCertificateConfigPtr> tls_certificate_secrets_;
std::unordered_map<std::string, TlsCertificateConfigProviderSharedPtr>
static_tls_certificate_providers_;
};

} // namespace Secret
Expand Down
14 changes: 14 additions & 0 deletions source/common/secret/secret_provider_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#include "common/secret/secret_provider_impl.h"

#include "common/common/assert.h"
#include "common/ssl/tls_certificate_config_impl.h"

namespace Envoy {
namespace Secret {

TlsCertificateConfigProviderImpl::TlsCertificateConfigProviderImpl(
const envoy::api::v2::auth::TlsCertificate& tls_certificate)
: tls_certificate_(std::make_unique<Ssl::TlsCertificateConfigImpl>(tls_certificate)) {}

} // namespace Secret
} // namespace Envoy
21 changes: 21 additions & 0 deletions source/common/secret/secret_provider_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#pragma once

#include "envoy/api/v2/auth/cert.pb.h"
#include "envoy/secret/secret_provider.h"
#include "envoy/ssl/tls_certificate_config.h"

namespace Envoy {
namespace Secret {

class TlsCertificateConfigProviderImpl : public TlsCertificateConfigProvider {
public:
TlsCertificateConfigProviderImpl(const envoy::api::v2::auth::TlsCertificate& tls_certificate);

const Ssl::TlsCertificateConfig* secret() const override { return tls_certificate_.get(); }

private:
Ssl::TlsCertificateConfigPtr tls_certificate_;
};

} // namespace Secret
} // namespace Envoy
2 changes: 2 additions & 0 deletions source/common/ssl/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ envoy_cc_library(
],
deps = [
"//include/envoy/secret:secret_manager_interface",
"//include/envoy/secret:secret_provider_interface",
"//include/envoy/ssl:context_config_interface",
"//source/common/common:assert_lib",
"//source/common/common:empty_string",
Expand Down Expand Up @@ -78,6 +79,7 @@ envoy_cc_library(
hdrs = ["tls_certificate_config_impl.h"],
deps = [
"//include/envoy/ssl:tls_certificate_config_interface",
"//source/common/common:empty_string",
"//source/common/config:datasource_lib",
"@envoy_api//envoy/api/v2/auth:cert_cc",
],
Expand Down
63 changes: 23 additions & 40 deletions source/common/ssl/context_config_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,28 @@ namespace Ssl {

namespace {

std::string readConfig(
const envoy::api::v2::auth::CommonTlsContext& config, Secret::SecretManager& secret_manager,
const std::function<std::string(const envoy::api::v2::auth::TlsCertificate& tls_certificate)>&
read_inline_config,
const std::function<std::string(const Ssl::TlsCertificateConfig& secret)>& read_secret) {
Secret::TlsCertificateConfigProviderSharedPtr
getTlsCertificateConfigProvider(const envoy::api::v2::auth::CommonTlsContext& config,
Secret::SecretManager& secret_manager) {
if (!config.tls_certificates().empty()) {
return read_inline_config(config.tls_certificates()[0]);
} else if (!config.tls_certificate_sds_secret_configs().empty()) {
auto name = config.tls_certificate_sds_secret_configs()[0].name();
const Ssl::TlsCertificateConfig* secret = secret_manager.findTlsCertificate(name);
if (!secret) {
throw EnvoyException(fmt::format("Static secret is not defined: {}", name));
const auto& tls_certificate = config.tls_certificates(0);
if (!tls_certificate.has_certificate_chain() && !tls_certificate.has_private_key()) {
return nullptr;
}
return read_secret(*secret);
} else {
return EMPTY_STRING;
return secret_manager.createInlineTlsCertificateProvider(config.tls_certificates(0));
}
if (!config.tls_certificate_sds_secret_configs().empty()) {
const auto& sds_secret_config = config.tls_certificate_sds_secret_configs(0);

auto secret_provider =
secret_manager.findStaticTlsCertificateProvider(sds_secret_config.name());
if (!secret_provider) {
throw EnvoyException(
fmt::format("Static secret is not defined: {}", sds_secret_config.name()));
}
return secret_provider;
}
return nullptr;
}

} // namespace
Expand Down Expand Up @@ -62,35 +67,13 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::auth::CommonTlsContex
ecdh_curves_(StringUtil::nonEmptyStringOrDefault(
RepeatedPtrUtil::join(config.tls_params().ecdh_curves(), ":"), DEFAULT_ECDH_CURVES)),
ca_cert_(Config::DataSource::read(config.validation_context().trusted_ca(), true)),
ca_cert_path_(Config::DataSource::getPath(config.validation_context().trusted_ca())),
ca_cert_path_(Config::DataSource::getPath(config.validation_context().trusted_ca())
.value_or(EMPTY_STRING)),
certificate_revocation_list_(
Config::DataSource::read(config.validation_context().crl(), true)),
certificate_revocation_list_path_(
Config::DataSource::getPath(config.validation_context().crl())),
cert_chain_(readConfig(
config, secret_manager,
[](const envoy::api::v2::auth::TlsCertificate& tls_certificate) -> std::string {
return Config::DataSource::read(tls_certificate.certificate_chain(), true);
},
[](const Ssl::TlsCertificateConfig& secret) -> std::string {
return secret.certificateChain();
})),
cert_chain_path_(
config.tls_certificates().empty()
? ""
: Config::DataSource::getPath(config.tls_certificates()[0].certificate_chain())),
private_key_(readConfig(
config, secret_manager,
[](const envoy::api::v2::auth::TlsCertificate& tls_certificate) -> std::string {
return Config::DataSource::read(tls_certificate.private_key(), true);
},
[](const Ssl::TlsCertificateConfig& secret) -> std::string {
return secret.privateKey();
})),
private_key_path_(
config.tls_certificates().empty()
? ""
: Config::DataSource::getPath(config.tls_certificates()[0].private_key())),
Config::DataSource::getPath(config.validation_context().crl()).value_or(EMPTY_STRING)),
tls_certficate_provider_(getTlsCertificateConfigProvider(config, secret_manager)),
verify_subject_alt_name_list_(config.validation_context().verify_subject_alt_name().begin(),
config.validation_context().verify_subject_alt_name().end()),
verify_certificate_hash_list_(config.validation_context().verify_certificate_hash().begin(),
Expand Down
Loading