From 08d0c5112c69ef862024215785df18f55aec55b5 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Sat, 24 Oct 2020 22:23:56 +0000 Subject: [PATCH 01/14] baseline from GH Signed-off-by: Harvey Tuch --- api/envoy/config/core/v3/base.proto | 11 +++ api/envoy/config/core/v4alpha/base.proto | 14 ++++ .../transport_sockets/tls/v3/common.proto | 31 ++++++- .../tls/v4alpha/common.proto | 31 ++++++- .../envoy/config/core/v3/base.proto | 11 +++ .../envoy/config/core/v4alpha/base.proto | 14 ++++ .../transport_sockets/tls/v3/common.proto | 31 ++++++- .../tls/v4alpha/common.proto | 31 ++++++- include/envoy/filesystem/filesystem.h | 19 ++++- source/common/config/BUILD | 12 +++ source/common/config/watched_directory.cc | 43 ++++++++++ source/common/config/watched_directory.h | 40 +++++++++ .../filesystem/posix/filesystem_impl.cc | 23 +++++ .../common/filesystem/posix/filesystem_impl.h | 2 + source/common/secret/BUILD | 1 + source/common/secret/sds_api.cc | 84 +++++++++++-------- source/common/secret/sds_api.h | 70 ++++++++++++++-- test/integration/BUILD | 1 + .../sds_dynamic_integration_test.cc | 68 ++++++++++++++- .../sds_dynamic_key_rotation_setup.sh | 20 +++++ test/mocks/filesystem/mocks.h | 2 + 21 files changed, 505 insertions(+), 54 deletions(-) create mode 100644 source/common/config/watched_directory.cc create mode 100644 source/common/config/watched_directory.h create mode 100755 test/integration/sds_dynamic_key_rotation_setup.sh diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index 4d7d69fae70bd..53cf4a81ae5db 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -313,6 +313,17 @@ message HeaderMap { repeated HeaderValue headers = 1; } +// A directory that is watched for changes, e.g. by inotify on Linux. +message WatchedDirectory { + // Path to watch. Any move events in this directory will trigger an update. + string root = 1 [(validate.rules).string = {min_len: 1}]; + + // A subdirectory from *root* containing watched files. This will be resolved + // via readlink(2) of *root*/*subdirectory* on an inotify event prior to + // reading files. + string subdirectory = 2; +} + // Data source consisting of either a file or an inline value. message DataSource { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.DataSource"; diff --git a/api/envoy/config/core/v4alpha/base.proto b/api/envoy/config/core/v4alpha/base.proto index dc1104a219b79..a0df4f31884aa 100644 --- a/api/envoy/config/core/v4alpha/base.proto +++ b/api/envoy/config/core/v4alpha/base.proto @@ -308,6 +308,20 @@ message HeaderMap { repeated HeaderValue headers = 1; } +// A directory that is watched for changes, e.g. by inotify on Linux. +message WatchedDirectory { + option (udpa.annotations.versioning).previous_message_type = + "envoy.config.core.v3.WatchedDirectory"; + + // Path to watch. Any move events in this directory will trigger an update. + string root = 1 [(validate.rules).string = {min_len: 1}]; + + // A subdirectory from *root* containing watched files. This will be resolved + // via readlink(2) of *root*/*subdirectory* on an inotify event prior to + // reading files. + string subdirectory = 2; +} + // Data source consisting of either a file or an inline value. message DataSource { option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.DataSource"; diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index 2f45a754416b7..d6f83a4792ad0 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -128,16 +128,33 @@ message PrivateKeyProvider { } } -// [#next-free-field: 7] +// [#next-free-field: 8] message TlsCertificate { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.TlsCertificate"; // The TLS certificate chain. + // + // If *certificate_chain* is a filesystem path, a watch will be added to the + // parent directory for any file moves to support rotation. This currently + // only applies to dynamic secrets, when the *TlsCertificate* is delivered via + // SDS. config.core.v3.DataSource certificate_chain = 1; // The TLS private key. + // + // If *private_key* is a filesystem path, a watch will be added to the parent + // directory for any file moves to support rotation. This currently only + // applies to dynamic secrets, when the *TlsCertificate* is delivered via SDS. config.core.v3.DataSource private_key = 2 [(udpa.annotations.sensitive) = true]; + // If specified, updates of file-based *certificate_chain* and *private_key* + // sources will be triggered by move events in this directory. The + // certificate/key pair will use a consistent evaluation of the watched path's + // real path. This overrides any filesystem watch implied by the path in + // *certificate_chain* and *private_key*. This is recommended for atomic + // symlink-based key/cert pair rotation. + config.core.v3.WatchedDirectory watched_directory = 7; + // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be // marked as ``oneof`` due to API compatibility reasons. Setting both :ref:`private_key @@ -191,7 +208,7 @@ message TlsSessionTicketKeys { [(validate.rules).repeated = {min_items: 1}, (udpa.annotations.sensitive) = true]; } -// [#next-free-field: 11] +// [#next-free-field: 12] message CertificateValidationContext { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.CertificateValidationContext"; @@ -233,8 +250,18 @@ message CertificateValidationContext { // // See :ref:`the TLS overview ` for a list of common // system CA locations. + // + // If *trusted_ca* is a filesystem path, a watch will be added to the parent + // directory for any file moves to support rotation. This currently only + // applies to dynamic secrets, when the *CertificateValidationContext* is + // delivered via SDS. config.core.v3.DataSource trusted_ca = 1; + // If specified, updates of a file-based *trusted_ca* source will be + // triggered by move events in this directory. This overrides any filesystem + // watch implied by the path in *trusted_ca*. + config.core.v3.WatchedDirectory watched_directory = 11; + // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate // matches one of the specified values. diff --git a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto index 1382863cb804c..d15c533f01c86 100644 --- a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -129,17 +129,34 @@ message PrivateKeyProvider { } } -// [#next-free-field: 7] +// [#next-free-field: 8] message TlsCertificate { option (udpa.annotations.versioning).previous_message_type = "envoy.extensions.transport_sockets.tls.v3.TlsCertificate"; // The TLS certificate chain. + // + // If *certificate_chain* is a filesystem path, a watch will be added to the + // parent directory for any file moves to support rotation. This currently + // only applies to dynamic secrets, when the *TlsCertificate* is delivered via + // SDS. config.core.v4alpha.DataSource certificate_chain = 1; // The TLS private key. + // + // If *private_key* is a filesystem path, a watch will be added to the parent + // directory for any file moves to support rotation. This currently only + // applies to dynamic secrets, when the *TlsCertificate* is delivered via SDS. config.core.v4alpha.DataSource private_key = 2 [(udpa.annotations.sensitive) = true]; + // If specified, updates of file-based *certificate_chain* and *private_key* + // sources will be triggered by move events in this directory. The + // certificate/key pair will use a consistent evaluation of the watched path's + // real path. This overrides any filesystem watch implied by the path in + // *certificate_chain* and *private_key*. This is recommended for atomic + // symlink-based key/cert pair rotation. + config.core.v4alpha.WatchedDirectory watched_directory = 7; + // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be // marked as ``oneof`` due to API compatibility reasons. Setting both :ref:`private_key @@ -193,7 +210,7 @@ message TlsSessionTicketKeys { [(validate.rules).repeated = {min_items: 1}, (udpa.annotations.sensitive) = true]; } -// [#next-free-field: 11] +// [#next-free-field: 12] message CertificateValidationContext { option (udpa.annotations.versioning).previous_message_type = "envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext"; @@ -235,8 +252,18 @@ message CertificateValidationContext { // // See :ref:`the TLS overview ` for a list of common // system CA locations. + // + // If *trusted_ca* is a filesystem path, a watch will be added to the parent + // directory for any file moves to support rotation. This currently only + // applies to dynamic secrets, when the *CertificateValidationContext* is + // delivered via SDS. config.core.v4alpha.DataSource trusted_ca = 1; + // If specified, updates of a file-based *trusted_ca* source will be + // triggered by move events in this directory. This overrides any filesystem + // watch implied by the path in *trusted_ca*. + config.core.v4alpha.WatchedDirectory watched_directory = 11; + // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate // matches one of the specified values. diff --git a/generated_api_shadow/envoy/config/core/v3/base.proto b/generated_api_shadow/envoy/config/core/v3/base.proto index 35b015710e5cb..acad68f9c2eeb 100644 --- a/generated_api_shadow/envoy/config/core/v3/base.proto +++ b/generated_api_shadow/envoy/config/core/v3/base.proto @@ -311,6 +311,17 @@ message HeaderMap { repeated HeaderValue headers = 1; } +// A directory that is watched for changes, e.g. by inotify on Linux. +message WatchedDirectory { + // Path to watch. Any move events in this directory will trigger an update. + string root = 1 [(validate.rules).string = {min_len: 1}]; + + // A subdirectory from *root* containing watched files. This will be resolved + // via readlink(2) of *root*/*subdirectory* on an inotify event prior to + // reading files. + string subdirectory = 2; +} + // Data source consisting of either a file or an inline value. message DataSource { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.DataSource"; diff --git a/generated_api_shadow/envoy/config/core/v4alpha/base.proto b/generated_api_shadow/envoy/config/core/v4alpha/base.proto index 03fcc5a461e0c..aa6d50c6d416c 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/base.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/base.proto @@ -315,6 +315,20 @@ message HeaderMap { repeated HeaderValue headers = 1; } +// A directory that is watched for changes, e.g. by inotify on Linux. +message WatchedDirectory { + option (udpa.annotations.versioning).previous_message_type = + "envoy.config.core.v3.WatchedDirectory"; + + // Path to watch. Any move events in this directory will trigger an update. + string root = 1 [(validate.rules).string = {min_len: 1}]; + + // A subdirectory from *root* containing watched files. This will be resolved + // via readlink(2) of *root*/*subdirectory* on an inotify event prior to + // reading files. + string subdirectory = 2; +} + // Data source consisting of either a file or an inline value. message DataSource { option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.DataSource"; diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto index da834e8afc548..4dfec5abf94b5 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -127,16 +127,33 @@ message PrivateKeyProvider { } } -// [#next-free-field: 7] +// [#next-free-field: 8] message TlsCertificate { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.TlsCertificate"; // The TLS certificate chain. + // + // If *certificate_chain* is a filesystem path, a watch will be added to the + // parent directory for any file moves to support rotation. This currently + // only applies to dynamic secrets, when the *TlsCertificate* is delivered via + // SDS. config.core.v3.DataSource certificate_chain = 1; // The TLS private key. + // + // If *private_key* is a filesystem path, a watch will be added to the parent + // directory for any file moves to support rotation. This currently only + // applies to dynamic secrets, when the *TlsCertificate* is delivered via SDS. config.core.v3.DataSource private_key = 2 [(udpa.annotations.sensitive) = true]; + // If specified, updates of file-based *certificate_chain* and *private_key* + // sources will be triggered by move events in this directory. The + // certificate/key pair will use a consistent evaluation of the watched path's + // real path. This overrides any filesystem watch implied by the path in + // *certificate_chain* and *private_key*. This is recommended for atomic + // symlink-based key/cert pair rotation. + config.core.v3.WatchedDirectory watched_directory = 7; + // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be // marked as ``oneof`` due to API compatibility reasons. Setting both :ref:`private_key @@ -190,7 +207,7 @@ message TlsSessionTicketKeys { [(validate.rules).repeated = {min_items: 1}, (udpa.annotations.sensitive) = true]; } -// [#next-free-field: 11] +// [#next-free-field: 12] message CertificateValidationContext { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.CertificateValidationContext"; @@ -230,8 +247,18 @@ message CertificateValidationContext { // // See :ref:`the TLS overview ` for a list of common // system CA locations. + // + // If *trusted_ca* is a filesystem path, a watch will be added to the parent + // directory for any file moves to support rotation. This currently only + // applies to dynamic secrets, when the *CertificateValidationContext* is + // delivered via SDS. config.core.v3.DataSource trusted_ca = 1; + // If specified, updates of a file-based *trusted_ca* source will be + // triggered by move events in this directory. This overrides any filesystem + // watch implied by the path in *trusted_ca*. + config.core.v3.WatchedDirectory watched_directory = 11; + // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate // matches one of the specified values. diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto index 1382863cb804c..d15c533f01c86 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -129,17 +129,34 @@ message PrivateKeyProvider { } } -// [#next-free-field: 7] +// [#next-free-field: 8] message TlsCertificate { option (udpa.annotations.versioning).previous_message_type = "envoy.extensions.transport_sockets.tls.v3.TlsCertificate"; // The TLS certificate chain. + // + // If *certificate_chain* is a filesystem path, a watch will be added to the + // parent directory for any file moves to support rotation. This currently + // only applies to dynamic secrets, when the *TlsCertificate* is delivered via + // SDS. config.core.v4alpha.DataSource certificate_chain = 1; // The TLS private key. + // + // If *private_key* is a filesystem path, a watch will be added to the parent + // directory for any file moves to support rotation. This currently only + // applies to dynamic secrets, when the *TlsCertificate* is delivered via SDS. config.core.v4alpha.DataSource private_key = 2 [(udpa.annotations.sensitive) = true]; + // If specified, updates of file-based *certificate_chain* and *private_key* + // sources will be triggered by move events in this directory. The + // certificate/key pair will use a consistent evaluation of the watched path's + // real path. This overrides any filesystem watch implied by the path in + // *certificate_chain* and *private_key*. This is recommended for atomic + // symlink-based key/cert pair rotation. + config.core.v4alpha.WatchedDirectory watched_directory = 7; + // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be // marked as ``oneof`` due to API compatibility reasons. Setting both :ref:`private_key @@ -193,7 +210,7 @@ message TlsSessionTicketKeys { [(validate.rules).repeated = {min_items: 1}, (udpa.annotations.sensitive) = true]; } -// [#next-free-field: 11] +// [#next-free-field: 12] message CertificateValidationContext { option (udpa.annotations.versioning).previous_message_type = "envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext"; @@ -235,8 +252,18 @@ message CertificateValidationContext { // // See :ref:`the TLS overview ` for a list of common // system CA locations. + // + // If *trusted_ca* is a filesystem path, a watch will be added to the parent + // directory for any file moves to support rotation. This currently only + // applies to dynamic secrets, when the *CertificateValidationContext* is + // delivered via SDS. config.core.v4alpha.DataSource trusted_ca = 1; + // If specified, updates of a file-based *trusted_ca* source will be + // triggered by move events in this directory. This overrides any filesystem + // watch implied by the path in *trusted_ca*. + config.core.v4alpha.WatchedDirectory watched_directory = 11; + // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate // matches one of the specified values. diff --git a/include/envoy/filesystem/filesystem.h b/include/envoy/filesystem/filesystem.h index 09764415b6a99..5edde8541c3c6 100644 --- a/include/envoy/filesystem/filesystem.h +++ b/include/envoy/filesystem/filesystem.h @@ -112,12 +112,29 @@ class Instance { virtual std::string fileReadToEnd(const std::string& path) PURE; /** - * @path file path to split + * @param path file path to split * @return PathSplitResult containing the parent directory of the input path and the file name * @note will throw an exception if path does not contain any path separator character */ virtual PathSplitResult splitPathFromFilename(absl::string_view path) PURE; + /** + * @param path1 first path component. + * @param path2 second path component. + * @return composite path with the second path component joined to the first. + */ + virtual std::string joinPath(absl::string_view path1, absl::string_view path2) PURE; + + /** + * Resolve a symlink path. + * @param path to resolve. + * @return resolved path. If there is an error, an exception is thrown, unless + * the error is a result of path not being a symlink, in which case + * the supplied path is returned. + * @throw EnvoyException if the path can't be resolved. + */ + virtual std::string readSymlink(absl::string_view path) PURE; + /** * Determine if the path is on a list of paths Envoy will refuse to access. This * is a basic sanity check for users, denying some clearly bad paths. Paths diff --git a/source/common/config/BUILD b/source/common/config/BUILD index cf1ac31142ffb..642b40575a3f9 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -444,6 +444,18 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "watched_directory_lib", + srcs = ["watched_directory.cc"], + hdrs = ["watched_directory.h"], + deps = [ + "//include/envoy/event:dispatcher_interface", + "//include/envoy/filesystem:filesystem_interface", + "//include/envoy/filesystem:watcher_interface", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + ], +) + envoy_cc_library( name = "well_known_names", srcs = ["well_known_names.cc"], diff --git a/source/common/config/watched_directory.cc b/source/common/config/watched_directory.cc new file mode 100644 index 0000000000000..6c5de1aff0661 --- /dev/null +++ b/source/common/config/watched_directory.cc @@ -0,0 +1,43 @@ +#include "common/config/watched_directory.h" + +namespace Envoy { +namespace Config { + +WatchedDirectory::WatchedDirectory(const envoy::config::core::v3::WatchedDirectory& config, + Event::Dispatcher& dispatcher, Filesystem::Instance& filesystem) + : filesystem_(filesystem), + literal_path_(filesystem.joinPath(config.root(), config.subdirectory())) { + resolvePath(); + watcher_ = dispatcher.createFilesystemWatcher(); + watcher_->addWatch(absl::StrCat(config.root(), "/"), Filesystem::Watcher::Events::MovedTo, + [this](uint32_t) { + resolvePath(); + cb_(); + }); +} + +void WatchedDirectory::resolveDataSourcePath(envoy::config::core::v3::DataSource& data_source) { + if (data_source.filename().empty() || resolved_path_.empty()) { + return; + } + try { + const auto result = filesystem_.splitPathFromFilename(data_source.filename()); + if (result.directory_ == literal_path_) { + data_source.set_filename(filesystem_.joinPath(resolved_path_, result.file_)); + } + } catch (EnvoyException& e) { + ENVOY_LOG_MISC(warn, "Unable to split path {}: {}", data_source.filename(), e.what()); + } +} + +void WatchedDirectory::resolvePath() { + try { + resolved_path_ = filesystem_.readSymlink(literal_path_); + } catch (EnvoyException& e) { + resolved_path_ = ""; + ENVOY_LOG_MISC(warn, "Unable to resolve path {}: {}", literal_path_, e.what()); + } +} + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/watched_directory.h b/source/common/config/watched_directory.h new file mode 100644 index 0000000000000..ed1ce34dee25d --- /dev/null +++ b/source/common/config/watched_directory.h @@ -0,0 +1,40 @@ +#pragma once + +#include "envoy/config/core/v3/base.pb.h" +#include "envoy/event/dispatcher.h" +#include "envoy/filesystem/filesystem.h" +#include "envoy/filesystem/watcher.h" + +namespace Envoy { +namespace Config { + +// Implement the common functionality of +// envoy::config::core::v3::WatchedDirectory. +class WatchedDirectory { +public: + using Callback = std::function; + + WatchedDirectory(const envoy::config::core::v3::WatchedDirectory& config, + Event::Dispatcher& dispatcher, Filesystem::Instance& filesystem); + + void setCallback(Callback cb) { cb_ = cb; } + // Update the path in a DataSource if its directory matches the watched + // directory. + void resolveDataSourcePath(envoy::config::core::v3::DataSource& data_source); + +private: + void resolvePath(); + + Filesystem::Instance& filesystem_; + // The root/subdirectory path from the WatchedDirectory. + const std::string literal_path_; + // The resolved path after a move event in the root directory. + std::string resolved_path_; + std::unique_ptr watcher_; + Callback cb_; +}; + +using WatchedDirectoryPtr = std::unique_ptr; + +} // namespace Config +} // namespace Envoy diff --git a/source/common/filesystem/posix/filesystem_impl.cc b/source/common/filesystem/posix/filesystem_impl.cc index 5f69e98e764bf..91ddc91b8b6d8 100644 --- a/source/common/filesystem/posix/filesystem_impl.cc +++ b/source/common/filesystem/posix/filesystem_impl.cc @@ -131,6 +131,29 @@ PathSplitResult InstanceImplPosix::splitPathFromFilename(absl::string_view path) return {path.substr(0, last_slash), name}; } +std::string InstanceImplPosix::joinPath(absl::string_view path1, absl::string_view path2) { + if (path2.empty()) { + return std::string(path1); + } + return absl::StrCat(path1, "/", path2); +} + +std::string InstanceImplPosix::readSymlink(absl::string_view path) { + std::string null_terminated_path{path}; + char resolved_path[PATH_MAX]; + const ssize_t result = ::readlink(null_terminated_path.data(), resolved_path, PATH_MAX); + if (result == -1) { + // If it's not a symlink, just return it, e.g. it might be simply + // directory. + if (errno == EINVAL) { + return std::string(path); + } + throw EnvoyException( + fmt::format("Unable to resolve symlink {}: {}", path, errorDetails(errno))); + } + return std::string(resolved_path, result); +} + bool InstanceImplPosix::illegalPath(const std::string& path) { // Special case, allow /dev/fd/* access here so that config can be passed in a // file descriptor from a bootstrap script via exec. The reason we do this diff --git a/source/common/filesystem/posix/filesystem_impl.h b/source/common/filesystem/posix/filesystem_impl.h index 4f81255be2225..ffc4230e86775 100644 --- a/source/common/filesystem/posix/filesystem_impl.h +++ b/source/common/filesystem/posix/filesystem_impl.h @@ -39,6 +39,8 @@ class InstanceImplPosix : public Instance { ssize_t fileSize(const std::string& path) override; std::string fileReadToEnd(const std::string& path) override; PathSplitResult splitPathFromFilename(absl::string_view path) override; + std::string joinPath(absl::string_view path1, absl::string_view path2) override; + std::string readSymlink(absl::string_view path) override; bool illegalPath(const std::string& path) override; private: diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index f486c2f7ce8ed..84defb7ef88bd 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -58,6 +58,7 @@ envoy_cc_library( "//source/common/config:api_version_lib", "//source/common/config:subscription_base_interface", "//source/common/config:utility_lib", + "//source/common/config:watched_directory_lib", "//source/common/init:target_lib", "//source/common/protobuf:utility_lib", "//source/common/ssl:certificate_validation_context_config_impl_lib", diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 163b91a13a316..1b9279781b002 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -19,12 +19,11 @@ SdsApi::SdsApi(envoy::config::core::v3::ConfigSource sds_config, absl::string_vi : Envoy::Config::SubscriptionBase( sds_config.resource_api_version(), validation_visitor, "name"), 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)), + dispatcher_(dispatcher), api_(api), stats_(stats), sds_config_(std::move(sds_config)), + sds_config_name_(sds_config_name), secret_hash_(0), clean_up_(std::move(destructor_cb)), subscription_factory_(subscription_factory), time_source_(time_source), secret_data_{sds_config_name_, "uninitialized", - time_source_.systemTime()}, - dispatcher_(dispatcher), api_(api) { + time_source_.systemTime()} { const auto resource_name = getResourceName(); // This has to happen here (rather than in initialize()) as it can throw exceptions. subscription_ = subscription_factory_.subscriptionFromConfigSource( @@ -36,6 +35,14 @@ SdsApi::SdsApi(envoy::config::core::v3::ConfigSource sds_config, absl::string_vi // each init manager has a chance to initialize its targets. } +void SdsApi::onWatchUpdate() { + const uint64_t new_hash = getHashForFiles(); + if (new_hash != files_hash_) { + update_callback_manager_.runCallbacks(); + files_hash_ = new_hash; + } +} + void SdsApi::onConfigUpdate(const std::vector& resources, const std::string& version_info) { validateUpdateSize(resources.size()); @@ -47,7 +54,7 @@ void SdsApi::onConfigUpdate(const std::vector& resou fmt::format("Unexpected SDS secret (expecting {}): {}", sds_config_name_, secret.name())); } - uint64_t new_hash = MessageUtil::hash(secret); + const uint64_t new_hash = MessageUtil::hash(secret); if (new_hash != secret_hash_) { validateConfig(secret); @@ -55,27 +62,33 @@ void SdsApi::onConfigUpdate(const std::vector& resou setSecret(secret); update_callback_manager_.runCallbacks(); - // List DataSources that refer to files - auto files = getDataSourceFilenames(); - if (!files.empty()) { - // Create new watch, also destroys the old watch if any. - watcher_ = dispatcher_.createFilesystemWatcher(); - files_hash_ = getHashForFiles(); - for (auto const& filename : files) { - // Watch for directory instead of file. This allows users to do atomic renames - // on directory level (e.g. Kubernetes secret update). - const auto result = api_.fileSystem().splitPathFromFilename(filename); - watcher_->addWatch(absl::StrCat(result.directory_, "/"), - Filesystem::Watcher::Events::MovedTo, [this](uint32_t) { - uint64_t new_hash = getHashForFiles(); - if (new_hash != files_hash_) { - update_callback_manager_.runCallbacks(); - files_hash_ = new_hash; - } - }); - } + auto* watched_directory = getWatchedDirectory(); + files_hash_ = getHashForFiles(); + // Either we have a watched directory and can defer the watch monitoring to + // a WatchedDirectory object, or we need to implement per-file watches + // in the else clause. + if (watched_directory != nullptr) { + watched_directory->setCallback([this]() { + resolveSecret(); + onWatchUpdate(); + }); } else { - watcher_.reset(); // Destroy the old watch if any + // List DataSources that refer to files + auto files = getDataSourceFilenames(); + if (!files.empty()) { + // Create new watch, also destroys the old watch if any. + watcher_ = dispatcher_.createFilesystemWatcher(); + for (auto const& filename : files) { + // Watch for directory instead of file. This allows users to do atomic renames + // on directory level (e.g. Kubernetes secret update). + const auto result = api_.fileSystem().splitPathFromFilename(filename); + watcher_->addWatch(absl::StrCat(result.directory_, "/"), + Filesystem::Watcher::Events::MovedTo, + [this](uint32_t) { onWatchUpdate(); }); + } + } else { + watcher_.reset(); // Destroy the old watch if any + } } } secret_data_.last_updated_ = time_source_.systemTime(); @@ -124,26 +137,27 @@ uint64_t SdsApi::getHashForFiles() { std::vector TlsCertificateSdsApi::getDataSourceFilenames() { std::vector files; - if (tls_certificate_secrets_ && tls_certificate_secrets_->has_certificate_chain() && - tls_certificate_secrets_->certificate_chain().specifier_case() == + if (resolved_tls_certificate_secrets_ && + resolved_tls_certificate_secrets_->has_certificate_chain() && + resolved_tls_certificate_secrets_->certificate_chain().specifier_case() == envoy::config::core::v3::DataSource::SpecifierCase::kFilename) { - files.push_back(tls_certificate_secrets_->certificate_chain().filename()); + files.push_back(resolved_tls_certificate_secrets_->certificate_chain().filename()); } - if (tls_certificate_secrets_ && tls_certificate_secrets_->has_private_key() && - tls_certificate_secrets_->private_key().specifier_case() == + if (resolved_tls_certificate_secrets_ && resolved_tls_certificate_secrets_->has_private_key() && + resolved_tls_certificate_secrets_->private_key().specifier_case() == envoy::config::core::v3::DataSource::SpecifierCase::kFilename) { - files.push_back(tls_certificate_secrets_->private_key().filename()); + files.push_back(resolved_tls_certificate_secrets_->private_key().filename()); } return files; } std::vector CertificateValidationContextSdsApi::getDataSourceFilenames() { std::vector files; - if (certificate_validation_context_secrets_ && - certificate_validation_context_secrets_->has_trusted_ca() && - certificate_validation_context_secrets_->trusted_ca().specifier_case() == + if (resolved_certificate_validation_context_secrets_ && + resolved_certificate_validation_context_secrets_->has_trusted_ca() && + resolved_certificate_validation_context_secrets_->trusted_ca().specifier_case() == envoy::config::core::v3::DataSource::SpecifierCase::kFilename) { - files.push_back(certificate_validation_context_secrets_->trusted_ca().filename()); + files.push_back(resolved_certificate_validation_context_secrets_->trusted_ca().filename()); } return files; } diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 52b78a37a49ba..53b99ac723d13 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -23,6 +23,7 @@ #include "common/common/cleanup.h" #include "common/config/subscription_base.h" #include "common/config/utility.h" +#include "common/config/watched_directory.h" #include "common/init/target_impl.h" #include "common/ssl/certificate_validation_context_config_impl.h" #include "common/ssl/tls_certificate_config_impl.h" @@ -62,6 +63,8 @@ class SdsApi : public Envoy::Config::SubscriptionBase< protected: // Creates new secrets. virtual void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret&) PURE; + // Refresh secrets, e.g. re-resolve symlinks in secret paths. + virtual void resolveSecret(){}; virtual void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret&) PURE; Common::CallbackManager<> update_callback_manager_; @@ -74,13 +77,18 @@ class SdsApi : public Envoy::Config::SubscriptionBase< void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException* e) override; virtual std::vector getDataSourceFilenames() PURE; + virtual Config::WatchedDirectory* getWatchedDirectory() PURE; Init::TargetImpl init_target_; + Event::Dispatcher& dispatcher_; + Api::Api& api_; private: void validateUpdateSize(int num_resources); void initialize(); uint64_t getHashForFiles(); + // Invoked for filesystem watches on update. + void onWatchUpdate(); Stats::Store& stats_; @@ -94,8 +102,6 @@ class SdsApi : public Envoy::Config::SubscriptionBase< Config::SubscriptionFactory& subscription_factory_; TimeSource& time_source_; SecretData secret_data_; - Event::Dispatcher& dispatcher_; - Api::Api& api_; std::unique_ptr watcher_; bool registered_init_target_{false}; }; @@ -142,7 +148,7 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider // SecretProvider const envoy::extensions::transport_sockets::tls::v3::TlsCertificate* secret() const override { - return tls_certificate_secrets_.get(); + return resolved_tls_certificate_secrets_.get(); } Common::CallbackHandle* addValidationCallback( std::function) @@ -158,15 +164,39 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider protected: void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) override { - tls_certificate_secrets_ = + sds_tls_certificate_secrets_ = std::make_unique( secret.tls_certificate()); + if (secret.tls_certificate().has_watched_directory()) { + watched_directory_ = std::make_unique( + secret.tls_certificate().watched_directory(), dispatcher_, api_.fileSystem()); + } else { + watched_directory_.reset(); + } + resolveSecret(); + } + void resolveSecret() override { + resolved_tls_certificate_secrets_ = + std::make_unique( + *sds_tls_certificate_secrets_); + if (watched_directory_ != nullptr) { + watched_directory_->resolveDataSourcePath( + *resolved_tls_certificate_secrets_->mutable_certificate_chain()); + watched_directory_->resolveDataSourcePath( + *resolved_tls_certificate_secrets_->mutable_private_key()); + } } void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret&) override {} std::vector getDataSourceFilenames() override; + Config::WatchedDirectory* getWatchedDirectory() override { return watched_directory_.get(); } private: - TlsCertificatePtr tls_certificate_secrets_; + // Directory to watch for symlink rotation. + Config::WatchedDirectoryPtr watched_directory_; + // TlsCertificate according to SDS source. + TlsCertificatePtr sds_tls_certificate_secrets_; + // TlsCertificate after resolving paths via WatchedDirectory. + TlsCertificatePtr resolved_tls_certificate_secrets_; }; /** @@ -205,7 +235,7 @@ class CertificateValidationContextSdsApi : public SdsApi, // SecretProvider const envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext* secret() const override { - return certificate_validation_context_secrets_.get(); + return resolved_certificate_validation_context_secrets_.get(); } Common::CallbackHandle* addUpdateCallback(std::function callback) override { if (secret()) { @@ -223,9 +253,25 @@ class CertificateValidationContextSdsApi : public SdsApi, protected: void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) override { - certificate_validation_context_secrets_ = std::make_unique< + sds_certificate_validation_context_secrets_ = std::make_unique< envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext>( secret.validation_context()); + if (secret.validation_context().has_watched_directory()) { + watched_directory_ = std::make_unique( + secret.validation_context().watched_directory(), dispatcher_, api_.fileSystem()); + } else { + watched_directory_.reset(); + } + resolveSecret(); + } + void resolveSecret() override { + resolved_certificate_validation_context_secrets_ = std::make_unique< + envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext>( + *sds_certificate_validation_context_secrets_); + if (watched_directory_ != nullptr) { + watched_directory_->resolveDataSourcePath( + *resolved_certificate_validation_context_secrets_->mutable_trusted_ca()); + } } void @@ -233,9 +279,15 @@ class CertificateValidationContextSdsApi : public SdsApi, validation_callback_manager_.runCallbacks(secret.validation_context()); } std::vector getDataSourceFilenames() override; + Config::WatchedDirectory* getWatchedDirectory() override { return watched_directory_.get(); } private: - CertificateValidationContextPtr certificate_validation_context_secrets_; + // Directory to watch for symlink rotation. + Config::WatchedDirectoryPtr watched_directory_; + // CertificateValidationContext according to SDS source; + CertificateValidationContextPtr sds_certificate_validation_context_secrets_; + // CertificateValidationContext after resolving paths via WatchedDirectory. + CertificateValidationContextPtr resolved_certificate_validation_context_secrets_; Common::CallbackManager< const envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext&> validation_callback_manager_; @@ -306,6 +358,7 @@ class TlsSessionTicketKeysSdsApi : public SdsApi, public TlsSessionTicketKeysCon validation_callback_manager_.runCallbacks(secret.session_ticket_keys()); } std::vector getDataSourceFilenames() override; + Config::WatchedDirectory* getWatchedDirectory() override { return nullptr; } private: Secret::TlsSessionTicketKeysPtr tls_session_ticket_keys_; @@ -367,6 +420,7 @@ class GenericSecretSdsApi : public SdsApi, public GenericSecretConfigProvider { validation_callback_manager_.runCallbacks(secret.generic_secret()); } std::vector getDataSourceFilenames() override; + Config::WatchedDirectory* getWatchedDirectory() override { return nullptr; } private: GenericSecretPtr generic_secret; diff --git a/test/integration/BUILD b/test/integration/BUILD index 8cf3bfcda25e1..08d449b3dc36a 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1139,6 +1139,7 @@ envoy_cc_test( "sds_dynamic_integration_test.cc", ], data = [ + "sds_dynamic_key_rotation_setup.sh", "//test/config/integration/certs", ], deps = [ diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index 4e5bc1ddebf5a..ef5ffae6bdb3c 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -64,6 +64,9 @@ class SdsDynamicIntegrationBaseTest : public Grpc::GrpcClientIntegrationParamTes const std::string& secret_name) { secret_config->set_name(secret_name); auto* config_source = secret_config->mutable_sds_config(); + if (v3_resource_api_) { + config_source->set_resource_api_version(envoy::config::core::v3::ApiVersion::V3); + } auto* api_config_source = config_source->mutable_api_config_source(); api_config_source->set_api_type(envoy::config::core::v3::ApiConfigSource::GRPC); auto* grpc_service = api_config_source->add_grpc_services(); @@ -122,8 +125,14 @@ class SdsDynamicIntegrationBaseTest : public Grpc::GrpcClientIntegrationParamTes void sendSdsResponse(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) { API_NO_BOOST(envoy::api::v2::DiscoveryResponse) discovery_response; discovery_response.set_version_info("1"); - discovery_response.set_type_url(Config::TypeUrl::get().Secret); - discovery_response.add_resources()->PackFrom(API_DOWNGRADE(secret)); + if (!v3_resource_api_) { + discovery_response.set_type_url(Config::TypeUrl::get().Secret); + discovery_response.add_resources()->PackFrom(API_DOWNGRADE(secret)); + } else { + discovery_response.set_type_url( + "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.Secret"); + discovery_response.add_resources()->PackFrom(secret); + } xds_stream_->sendGrpcMessage(discovery_response); } @@ -138,6 +147,7 @@ class SdsDynamicIntegrationBaseTest : public Grpc::GrpcClientIntegrationParamTes const std::string server_cert_; const std::string validation_secret_; const std::string client_cert_; + bool v3_resource_api_{false}; }; // Downstream SDS integration test: static Listener with ssl cert from SDS @@ -200,6 +210,60 @@ class SdsDynamicDownstreamIntegrationTest : public SdsDynamicIntegrationBaseTest INSTANTIATE_TEST_SUITE_P(IpVersionsClientType, SdsDynamicDownstreamIntegrationTest, GRPC_CLIENT_INTEGRATION_PARAMS); +class SdsDynamicKeyRotationIntegrationTest : public SdsDynamicDownstreamIntegrationTest { +protected: + envoy::extensions::transport_sockets::tls::v3::Secret getCurrentServerSecret() { + envoy::extensions::transport_sockets::tls::v3::Secret secret; + secret.set_name(server_cert_); + auto* tls_certificate = secret.mutable_tls_certificate(); + tls_certificate->mutable_certificate_chain()->set_filename( + TestEnvironment::temporaryPath("root/current/servercert.pem")); + tls_certificate->mutable_private_key()->set_filename( + TestEnvironment::temporaryPath("root/current/serverkey.pem")); + auto* watched_dir = tls_certificate->mutable_watched_directory(); + watched_dir->set_root(TestEnvironment::temporaryPath("root")); + watched_dir->set_subdirectory("current"); + return secret; + } +}; + +// We don't care about multiple gRPC types here, Envoy gRPC is fine, the +// interest is on the filesystem. +INSTANTIATE_TEST_SUITE_P( + IpVersionsClientType, SdsDynamicKeyRotationIntegrationTest, + testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + testing::Values(Grpc::ClientType::EnvoyGrpc))); + +TEST_P(SdsDynamicKeyRotationIntegrationTest, BasicRotation) { + v3_resource_api_ = true; + TestEnvironment::exec( + {TestEnvironment::runfilesPath("test/integration/sds_dynamic_key_rotation_setup.sh")}); + + on_server_init_function_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getCurrentServerSecret()); + }; + initialize(); + + // Initial update from filesystem. + test_server_->waitForCounterGe( + listenerStatPrefix("server_ssl_socket_factory.ssl_context_update_by_sds"), 1); + + ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { + return makeSslClientConnection(); + }; + // First request with server{cert,key}.pem. + testRouterHeaderOnlyRequestAndResponse(&creator); + cleanupUpstreamAndDownstream(); + // Rotate. + TestEnvironment::renameFile(TestEnvironment::temporaryPath("root/new"), + TestEnvironment::temporaryPath("root/current")); + test_server_->waitForCounterGe( + listenerStatPrefix("server_ssl_socket_factory.ssl_context_update_by_sds"), 2); + // First request with server_ecda{cert,key}.pem. + testRouterHeaderOnlyRequestAndResponse(&creator); +} + // A test that SDS server send a good server secret for a static listener. // The first ssl request should be OK. TEST_P(SdsDynamicDownstreamIntegrationTest, BasicSuccess) { diff --git a/test/integration/sds_dynamic_key_rotation_setup.sh b/test/integration/sds_dynamic_key_rotation_setup.sh new file mode 100755 index 0000000000000..89132a52f4c39 --- /dev/null +++ b/test/integration/sds_dynamic_key_rotation_setup.sh @@ -0,0 +1,20 @@ +#!/bin/bash + +set -e + +TEST_CERTS=test/config/integration/certs + +ROOT="${TEST_TMPDIR}"/root +SERVER_KEYCERT="${ROOT}"/server +SERVER_ECDSA_KEYCERT="${ROOT}"/server_ecdsa + +rm -rf "${ROOT}" +mkdir -p "${SERVER_KEYCERT}" +mkdir -p "${SERVER_ECDSA_KEYCERT}" + +cp -f "${TEST_CERTS}"/server{cert,key}.pem "${SERVER_KEYCERT}" +cp -f "${TEST_CERTS}"/server_ecdsacert.pem "${SERVER_ECDSA_KEYCERT}"/servercert.pem +cp -f "${TEST_CERTS}"/server_ecdsakey.pem "${SERVER_ECDSA_KEYCERT}"/serverkey.pem + +ln -sf "${SERVER_KEYCERT}" "${ROOT}"/current +ln -sf "${SERVER_ECDSA_KEYCERT}" "${ROOT}"/new diff --git a/test/mocks/filesystem/mocks.h b/test/mocks/filesystem/mocks.h index b6fca8ed4af1c..fc50098abedbc 100644 --- a/test/mocks/filesystem/mocks.h +++ b/test/mocks/filesystem/mocks.h @@ -53,6 +53,8 @@ class MockInstance : public Instance { MOCK_METHOD(ssize_t, fileSize, (const std::string&)); MOCK_METHOD(std::string, fileReadToEnd, (const std::string&)); MOCK_METHOD(PathSplitResult, splitPathFromFilename, (absl::string_view)); + MOCK_METHOD(std::string, joinPath, (absl::string_view, absl::string_view)); + MOCK_METHOD(std::string, readSymlink, (absl::string_view)); MOCK_METHOD(bool, illegalPath, (const std::string&)); }; From d3c88a5341b1534891eaf4376f9848a1409de682 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 26 Oct 2020 23:28:00 +0000 Subject: [PATCH 02/14] Fine grained WatchedPath control, hash-based file atomic reads. Signed-off-by: Harvey Tuch --- api/envoy/config/core/v3/base.proto | 25 +++--- api/envoy/config/core/v4alpha/base.proto | 26 +++--- .../transport_sockets/tls/v3/common.proto | 17 ++-- .../tls/v4alpha/common.proto | 17 ++-- .../envoy/config/core/v3/base.proto | 25 +++--- .../envoy/config/core/v4alpha/base.proto | 26 +++--- .../transport_sockets/tls/v3/common.proto | 17 ++-- .../tls/v4alpha/common.proto | 17 ++-- source/common/config/BUILD | 7 +- source/common/config/watched_directory.cc | 43 ---------- source/common/config/watched_directory.h | 40 ---------- source/common/config/watched_path.cc | 30 +++++++ source/common/config/watched_path.h | 27 +++++++ source/common/secret/BUILD | 2 +- source/common/secret/sds_api.cc | 77 ++++++++++++------ source/common/secret/sds_api.h | 79 +++++++++++-------- test/common/secret/sds_api_test.cc | 1 + .../sds_dynamic_integration_test.cc | 5 +- 18 files changed, 253 insertions(+), 228 deletions(-) delete mode 100644 source/common/config/watched_directory.cc delete mode 100644 source/common/config/watched_directory.h create mode 100644 source/common/config/watched_path.cc create mode 100644 source/common/config/watched_path.h diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index 53cf4a81ae5db..ca950062b2a1d 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -313,15 +313,22 @@ message HeaderMap { repeated HeaderValue headers = 1; } -// A directory that is watched for changes, e.g. by inotify on Linux. -message WatchedDirectory { - // Path to watch. Any move events in this directory will trigger an update. - string root = 1 [(validate.rules).string = {min_len: 1}]; - - // A subdirectory from *root* containing watched files. This will be resolved - // via readlink(2) of *root*/*subdirectory* on an inotify event prior to - // reading files. - string subdirectory = 2; +// A path that is watched for changes, e.g. by inotify on Linux. +message WatchedPath { + enum Event { + // Generated for the directory containing the new filename when a file is + // renamed. + MOVED_TO = 0; + + // File was modified, e.g. write(2), truncate(2). + MODIFIED = 1; + } + + // Path to watch. + string path = 1 [(validate.rules).string = {min_len: 1}]; + + // Event to watch on *path*. + Event event = 2 [(validate.rules).enum = {defined_only: true}]; } // Data source consisting of either a file or an inline value. diff --git a/api/envoy/config/core/v4alpha/base.proto b/api/envoy/config/core/v4alpha/base.proto index a0df4f31884aa..aab29fd050d12 100644 --- a/api/envoy/config/core/v4alpha/base.proto +++ b/api/envoy/config/core/v4alpha/base.proto @@ -308,18 +308,24 @@ message HeaderMap { repeated HeaderValue headers = 1; } -// A directory that is watched for changes, e.g. by inotify on Linux. -message WatchedDirectory { - option (udpa.annotations.versioning).previous_message_type = - "envoy.config.core.v3.WatchedDirectory"; +// A path that is watched for changes, e.g. by inotify on Linux. +message WatchedPath { + option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.WatchedPath"; + + enum Event { + // Generated for the directory containing the new filename when a file is + // renamed. + MOVED_TO = 0; + + // File was modified, e.g. write(2), truncate(2). + MODIFIED = 1; + } - // Path to watch. Any move events in this directory will trigger an update. - string root = 1 [(validate.rules).string = {min_len: 1}]; + // Path to watch. + string path = 1 [(validate.rules).string = {min_len: 1}]; - // A subdirectory from *root* containing watched files. This will be resolved - // via readlink(2) of *root*/*subdirectory* on an inotify event prior to - // reading files. - string subdirectory = 2; + // Event to watch on *path*. + Event event = 2 [(validate.rules).enum = {defined_only: true}]; } // Data source consisting of either a file or an inline value. diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index d6f83a4792ad0..21a87e86585f0 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -148,12 +148,10 @@ message TlsCertificate { config.core.v3.DataSource private_key = 2 [(udpa.annotations.sensitive) = true]; // If specified, updates of file-based *certificate_chain* and *private_key* - // sources will be triggered by move events in this directory. The - // certificate/key pair will use a consistent evaluation of the watched path's - // real path. This overrides any filesystem watch implied by the path in - // *certificate_chain* and *private_key*. This is recommended for atomic - // symlink-based key/cert pair rotation. - config.core.v3.WatchedDirectory watched_directory = 7; + // sources will be triggered by this watch. The certificate/key pair will be + // read together and validated for atomic read consistency (i.e. no intervening + // modification occurred between cert/key read). + config.core.v3.WatchedPath watched_path = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be @@ -257,10 +255,9 @@ message CertificateValidationContext { // delivered via SDS. config.core.v3.DataSource trusted_ca = 1; - // If specified, updates of a file-based *trusted_ca* source will be - // triggered by move events in this directory. This overrides any filesystem - // watch implied by the path in *trusted_ca*. - config.core.v3.WatchedDirectory watched_directory = 11; + // If specified, updates of a file-based *trusted_ca* + // source will be triggered by this watch. + config.core.v3.WatchedPath watched_path = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate diff --git a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto index d15c533f01c86..2740357ccca0a 100644 --- a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -150,12 +150,10 @@ message TlsCertificate { config.core.v4alpha.DataSource private_key = 2 [(udpa.annotations.sensitive) = true]; // If specified, updates of file-based *certificate_chain* and *private_key* - // sources will be triggered by move events in this directory. The - // certificate/key pair will use a consistent evaluation of the watched path's - // real path. This overrides any filesystem watch implied by the path in - // *certificate_chain* and *private_key*. This is recommended for atomic - // symlink-based key/cert pair rotation. - config.core.v4alpha.WatchedDirectory watched_directory = 7; + // sources will be triggered by this watch. The certificate/key pair will be + // read together and validated for atomic read consistency (i.e. no intervening + // modification occurred between cert/key read). + config.core.v4alpha.WatchedPath watched_path = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be @@ -259,10 +257,9 @@ message CertificateValidationContext { // delivered via SDS. config.core.v4alpha.DataSource trusted_ca = 1; - // If specified, updates of a file-based *trusted_ca* source will be - // triggered by move events in this directory. This overrides any filesystem - // watch implied by the path in *trusted_ca*. - config.core.v4alpha.WatchedDirectory watched_directory = 11; + // If specified, updates of a file-based *trusted_ca* + // source will be triggered by this watch. + config.core.v4alpha.WatchedPath watched_path = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate diff --git a/generated_api_shadow/envoy/config/core/v3/base.proto b/generated_api_shadow/envoy/config/core/v3/base.proto index acad68f9c2eeb..09c71c5a35764 100644 --- a/generated_api_shadow/envoy/config/core/v3/base.proto +++ b/generated_api_shadow/envoy/config/core/v3/base.proto @@ -311,15 +311,22 @@ message HeaderMap { repeated HeaderValue headers = 1; } -// A directory that is watched for changes, e.g. by inotify on Linux. -message WatchedDirectory { - // Path to watch. Any move events in this directory will trigger an update. - string root = 1 [(validate.rules).string = {min_len: 1}]; - - // A subdirectory from *root* containing watched files. This will be resolved - // via readlink(2) of *root*/*subdirectory* on an inotify event prior to - // reading files. - string subdirectory = 2; +// A path that is watched for changes, e.g. by inotify on Linux. +message WatchedPath { + enum Event { + // Generated for the directory containing the new filename when a file is + // renamed. + MOVED_TO = 0; + + // File was modified, e.g. write(2), truncate(2). + MODIFIED = 1; + } + + // Path to watch. + string path = 1 [(validate.rules).string = {min_len: 1}]; + + // Event to watch on *path*. + Event event = 2 [(validate.rules).enum = {defined_only: true}]; } // Data source consisting of either a file or an inline value. diff --git a/generated_api_shadow/envoy/config/core/v4alpha/base.proto b/generated_api_shadow/envoy/config/core/v4alpha/base.proto index aa6d50c6d416c..b6c2d99d14953 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/base.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/base.proto @@ -315,18 +315,24 @@ message HeaderMap { repeated HeaderValue headers = 1; } -// A directory that is watched for changes, e.g. by inotify on Linux. -message WatchedDirectory { - option (udpa.annotations.versioning).previous_message_type = - "envoy.config.core.v3.WatchedDirectory"; +// A path that is watched for changes, e.g. by inotify on Linux. +message WatchedPath { + option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.WatchedPath"; + + enum Event { + // Generated for the directory containing the new filename when a file is + // renamed. + MOVED_TO = 0; + + // File was modified, e.g. write(2), truncate(2). + MODIFIED = 1; + } - // Path to watch. Any move events in this directory will trigger an update. - string root = 1 [(validate.rules).string = {min_len: 1}]; + // Path to watch. + string path = 1 [(validate.rules).string = {min_len: 1}]; - // A subdirectory from *root* containing watched files. This will be resolved - // via readlink(2) of *root*/*subdirectory* on an inotify event prior to - // reading files. - string subdirectory = 2; + // Event to watch on *path*. + Event event = 2 [(validate.rules).enum = {defined_only: true}]; } // Data source consisting of either a file or an inline value. diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto index 4dfec5abf94b5..6113b57fffcf4 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -147,12 +147,10 @@ message TlsCertificate { config.core.v3.DataSource private_key = 2 [(udpa.annotations.sensitive) = true]; // If specified, updates of file-based *certificate_chain* and *private_key* - // sources will be triggered by move events in this directory. The - // certificate/key pair will use a consistent evaluation of the watched path's - // real path. This overrides any filesystem watch implied by the path in - // *certificate_chain* and *private_key*. This is recommended for atomic - // symlink-based key/cert pair rotation. - config.core.v3.WatchedDirectory watched_directory = 7; + // sources will be triggered by this watch. The certificate/key pair will be + // read together and validated for atomic read consistency (i.e. no intervening + // modification occurred between cert/key read). + config.core.v3.WatchedPath watched_path = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be @@ -254,10 +252,9 @@ message CertificateValidationContext { // delivered via SDS. config.core.v3.DataSource trusted_ca = 1; - // If specified, updates of a file-based *trusted_ca* source will be - // triggered by move events in this directory. This overrides any filesystem - // watch implied by the path in *trusted_ca*. - config.core.v3.WatchedDirectory watched_directory = 11; + // If specified, updates of a file-based *trusted_ca* + // source will be triggered by this watch. + config.core.v3.WatchedPath watched_path = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto index d15c533f01c86..2740357ccca0a 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -150,12 +150,10 @@ message TlsCertificate { config.core.v4alpha.DataSource private_key = 2 [(udpa.annotations.sensitive) = true]; // If specified, updates of file-based *certificate_chain* and *private_key* - // sources will be triggered by move events in this directory. The - // certificate/key pair will use a consistent evaluation of the watched path's - // real path. This overrides any filesystem watch implied by the path in - // *certificate_chain* and *private_key*. This is recommended for atomic - // symlink-based key/cert pair rotation. - config.core.v4alpha.WatchedDirectory watched_directory = 7; + // sources will be triggered by this watch. The certificate/key pair will be + // read together and validated for atomic read consistency (i.e. no intervening + // modification occurred between cert/key read). + config.core.v4alpha.WatchedPath watched_path = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be @@ -259,10 +257,9 @@ message CertificateValidationContext { // delivered via SDS. config.core.v4alpha.DataSource trusted_ca = 1; - // If specified, updates of a file-based *trusted_ca* source will be - // triggered by move events in this directory. This overrides any filesystem - // watch implied by the path in *trusted_ca*. - config.core.v4alpha.WatchedDirectory watched_directory = 11; + // If specified, updates of a file-based *trusted_ca* + // source will be triggered by this watch. + config.core.v4alpha.WatchedPath watched_path = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 642b40575a3f9..01fc3fb21e84e 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -445,12 +445,11 @@ envoy_cc_library( ) envoy_cc_library( - name = "watched_directory_lib", - srcs = ["watched_directory.cc"], - hdrs = ["watched_directory.h"], + name = "watched_path_lib", + srcs = ["watched_path.cc"], + hdrs = ["watched_path.h"], deps = [ "//include/envoy/event:dispatcher_interface", - "//include/envoy/filesystem:filesystem_interface", "//include/envoy/filesystem:watcher_interface", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], diff --git a/source/common/config/watched_directory.cc b/source/common/config/watched_directory.cc deleted file mode 100644 index 6c5de1aff0661..0000000000000 --- a/source/common/config/watched_directory.cc +++ /dev/null @@ -1,43 +0,0 @@ -#include "common/config/watched_directory.h" - -namespace Envoy { -namespace Config { - -WatchedDirectory::WatchedDirectory(const envoy::config::core::v3::WatchedDirectory& config, - Event::Dispatcher& dispatcher, Filesystem::Instance& filesystem) - : filesystem_(filesystem), - literal_path_(filesystem.joinPath(config.root(), config.subdirectory())) { - resolvePath(); - watcher_ = dispatcher.createFilesystemWatcher(); - watcher_->addWatch(absl::StrCat(config.root(), "/"), Filesystem::Watcher::Events::MovedTo, - [this](uint32_t) { - resolvePath(); - cb_(); - }); -} - -void WatchedDirectory::resolveDataSourcePath(envoy::config::core::v3::DataSource& data_source) { - if (data_source.filename().empty() || resolved_path_.empty()) { - return; - } - try { - const auto result = filesystem_.splitPathFromFilename(data_source.filename()); - if (result.directory_ == literal_path_) { - data_source.set_filename(filesystem_.joinPath(resolved_path_, result.file_)); - } - } catch (EnvoyException& e) { - ENVOY_LOG_MISC(warn, "Unable to split path {}: {}", data_source.filename(), e.what()); - } -} - -void WatchedDirectory::resolvePath() { - try { - resolved_path_ = filesystem_.readSymlink(literal_path_); - } catch (EnvoyException& e) { - resolved_path_ = ""; - ENVOY_LOG_MISC(warn, "Unable to resolve path {}: {}", literal_path_, e.what()); - } -} - -} // namespace Config -} // namespace Envoy diff --git a/source/common/config/watched_directory.h b/source/common/config/watched_directory.h deleted file mode 100644 index ed1ce34dee25d..0000000000000 --- a/source/common/config/watched_directory.h +++ /dev/null @@ -1,40 +0,0 @@ -#pragma once - -#include "envoy/config/core/v3/base.pb.h" -#include "envoy/event/dispatcher.h" -#include "envoy/filesystem/filesystem.h" -#include "envoy/filesystem/watcher.h" - -namespace Envoy { -namespace Config { - -// Implement the common functionality of -// envoy::config::core::v3::WatchedDirectory. -class WatchedDirectory { -public: - using Callback = std::function; - - WatchedDirectory(const envoy::config::core::v3::WatchedDirectory& config, - Event::Dispatcher& dispatcher, Filesystem::Instance& filesystem); - - void setCallback(Callback cb) { cb_ = cb; } - // Update the path in a DataSource if its directory matches the watched - // directory. - void resolveDataSourcePath(envoy::config::core::v3::DataSource& data_source); - -private: - void resolvePath(); - - Filesystem::Instance& filesystem_; - // The root/subdirectory path from the WatchedDirectory. - const std::string literal_path_; - // The resolved path after a move event in the root directory. - std::string resolved_path_; - std::unique_ptr watcher_; - Callback cb_; -}; - -using WatchedDirectoryPtr = std::unique_ptr; - -} // namespace Config -} // namespace Envoy diff --git a/source/common/config/watched_path.cc b/source/common/config/watched_path.cc new file mode 100644 index 0000000000000..31f31a80f2ae0 --- /dev/null +++ b/source/common/config/watched_path.cc @@ -0,0 +1,30 @@ +#include "common/config/watched_path.h" + +namespace Envoy { +namespace Config { + +namespace { + +uint32_t watchedPathEventToWatcherEvent(envoy::config::core::v3::WatchedPath::Event event) { + switch (event) { + case envoy::config::core::v3::WatchedPath::MOVED_TO: + return Filesystem::Watcher::Events::MovedTo; + case envoy::config::core::v3::WatchedPath::MODIFIED: + return Filesystem::Watcher::Events::Modified; + + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } +} + +} // namespace + +WatchedPath::WatchedPath(const envoy::config::core::v3::WatchedPath& config, + Event::Dispatcher& dispatcher) { + watcher_ = dispatcher.createFilesystemWatcher(); + watcher_->addWatch(config.path(), watchedPathEventToWatcherEvent(config.event()), + [this](uint32_t) { cb_(); }); +} + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/watched_path.h b/source/common/config/watched_path.h new file mode 100644 index 0000000000000..2031e798d512e --- /dev/null +++ b/source/common/config/watched_path.h @@ -0,0 +1,27 @@ +#pragma once + +#include "envoy/config/core/v3/base.pb.h" +#include "envoy/event/dispatcher.h" +#include "envoy/filesystem/watcher.h" + +namespace Envoy { +namespace Config { + +// Implement the common functionality of envoy::config::core::v3::WatchedPath. +class WatchedPath { +public: + using Callback = std::function; + + WatchedPath(const envoy::config::core::v3::WatchedPath& config, Event::Dispatcher& dispatcher); + + void setCallback(Callback cb) { cb_ = cb; } + +private: + std::unique_ptr watcher_; + Callback cb_; +}; + +using WatchedPathPtr = std::unique_ptr; + +} // namespace Config +} // namespace Envoy diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 84defb7ef88bd..63fbcf20b5643 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -58,7 +58,7 @@ envoy_cc_library( "//source/common/config:api_version_lib", "//source/common/config:subscription_base_interface", "//source/common/config:utility_lib", - "//source/common/config:watched_directory_lib", + "//source/common/config:watched_path_lib", "//source/common/init:target_lib", "//source/common/protobuf:utility_lib", "//source/common/ssl:certificate_validation_context_config_impl_lib", diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 1b9279781b002..47e64d050ecb9 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -35,9 +35,30 @@ SdsApi::SdsApi(envoy::config::core::v3::ConfigSource sds_config, absl::string_vi // each init manager has a chance to initialize its targets. } +void SdsApi::resolveDataSource(const FileContentMap& files, + envoy::config::core::v3::DataSource& data_source) { + if (data_source.specifier_case() == + envoy::config::core::v3::DataSource::SpecifierCase::kFilename) { + const std::string& content = files.at(data_source.filename()); + data_source.set_inline_bytes(content); + } +} + void SdsApi::onWatchUpdate() { - const uint64_t new_hash = getHashForFiles(); + // Obtain a stable set of files. If a rotation happens while we're eading, + // then we need to try again. + uint64_t prev_hash = 0; + FileContentMap files = loadFiles(); + uint64_t next_hash = getHashForFiles(files); + // TODO(htuch): bound this so we don't run forever. DO NOT SUBMIT until fixed. + while (next_hash != prev_hash) { + files = loadFiles(); + prev_hash = next_hash; + next_hash = getHashForFiles(files); + } + const uint64_t new_hash = next_hash; if (new_hash != files_hash_) { + resolveSecret(files); update_callback_manager_.runCallbacks(); files_hash_ = new_hash; } @@ -60,18 +81,17 @@ void SdsApi::onConfigUpdate(const std::vector& resou validateConfig(secret); secret_hash_ = new_hash; setSecret(secret); + const auto files = loadFiles(); + files_hash_ = getHashForFiles(files); + resolveSecret(files); update_callback_manager_.runCallbacks(); - auto* watched_directory = getWatchedDirectory(); - files_hash_ = getHashForFiles(); - // Either we have a watched directory and can defer the watch monitoring to - // a WatchedDirectory object, or we need to implement per-file watches - // in the else clause. - if (watched_directory != nullptr) { - watched_directory->setCallback([this]() { - resolveSecret(); - onWatchUpdate(); - }); + auto* watched_path = getWatchedPath(); + // Either we have a watched path and can defer the watch monitoring to a + // WatchedPath object, or we need to implement per-file watches in the else + // clause. + if (watched_path != nullptr) { + watched_path->setCallback([this]() { onWatchUpdate(); }); } else { // List DataSources that refer to files auto files = getDataSourceFilenames(); @@ -127,37 +147,44 @@ void SdsApi::initialize() { SdsApi::SecretData SdsApi::secretData() { return secret_data_; } -uint64_t SdsApi::getHashForFiles() { - uint64_t hash = 0; +SdsApi::FileContentMap SdsApi::loadFiles() { + FileContentMap files; for (auto const& filename : getDataSourceFilenames()) { - hash = HashUtil::xxHash64(api_.fileSystem().fileReadToEnd(filename), hash); + files[filename] = api_.fileSystem().fileReadToEnd(filename); + } + return files; +} + +uint64_t SdsApi::getHashForFiles(const FileContentMap& files) { + uint64_t hash = 0; + for (const auto& it : files) { + hash = HashUtil::xxHash64(it.second, hash); } return hash; } std::vector TlsCertificateSdsApi::getDataSourceFilenames() { std::vector files; - if (resolved_tls_certificate_secrets_ && - resolved_tls_certificate_secrets_->has_certificate_chain() && - resolved_tls_certificate_secrets_->certificate_chain().specifier_case() == + if (sds_tls_certificate_secrets_ && sds_tls_certificate_secrets_->has_certificate_chain() && + sds_tls_certificate_secrets_->certificate_chain().specifier_case() == envoy::config::core::v3::DataSource::SpecifierCase::kFilename) { - files.push_back(resolved_tls_certificate_secrets_->certificate_chain().filename()); + files.push_back(sds_tls_certificate_secrets_->certificate_chain().filename()); } - if (resolved_tls_certificate_secrets_ && resolved_tls_certificate_secrets_->has_private_key() && - resolved_tls_certificate_secrets_->private_key().specifier_case() == + if (sds_tls_certificate_secrets_ && sds_tls_certificate_secrets_->has_private_key() && + sds_tls_certificate_secrets_->private_key().specifier_case() == envoy::config::core::v3::DataSource::SpecifierCase::kFilename) { - files.push_back(resolved_tls_certificate_secrets_->private_key().filename()); + files.push_back(sds_tls_certificate_secrets_->private_key().filename()); } return files; } std::vector CertificateValidationContextSdsApi::getDataSourceFilenames() { std::vector files; - if (resolved_certificate_validation_context_secrets_ && - resolved_certificate_validation_context_secrets_->has_trusted_ca() && - resolved_certificate_validation_context_secrets_->trusted_ca().specifier_case() == + if (sds_certificate_validation_context_secrets_ && + sds_certificate_validation_context_secrets_->has_trusted_ca() && + sds_certificate_validation_context_secrets_->trusted_ca().specifier_case() == envoy::config::core::v3::DataSource::SpecifierCase::kFilename) { - files.push_back(resolved_certificate_validation_context_secrets_->trusted_ca().filename()); + files.push_back(sds_certificate_validation_context_secrets_->trusted_ca().filename()); } return files; } diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 53b99ac723d13..05e51f7ef8d71 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -23,7 +23,7 @@ #include "common/common/cleanup.h" #include "common/config/subscription_base.h" #include "common/config/utility.h" -#include "common/config/watched_directory.h" +#include "common/config/watched_path.h" #include "common/init/target_impl.h" #include "common/ssl/certificate_validation_context_config_impl.h" #include "common/ssl/tls_certificate_config_impl.h" @@ -61,10 +61,13 @@ class SdsApi : public Envoy::Config::SubscriptionBase< } protected: + // Ordered for hash stability. + using FileContentMap = std::map; + // Creates new secrets. virtual void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret&) PURE; // Refresh secrets, e.g. re-resolve symlinks in secret paths. - virtual void resolveSecret(){}; + virtual void resolveSecret(const FileContentMap& /*files*/){}; virtual void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret&) PURE; Common::CallbackManager<> update_callback_manager_; @@ -77,7 +80,10 @@ class SdsApi : public Envoy::Config::SubscriptionBase< void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException* e) override; virtual std::vector getDataSourceFilenames() PURE; - virtual Config::WatchedDirectory* getWatchedDirectory() PURE; + virtual Config::WatchedPath* getWatchedPath() PURE; + + void resolveDataSource(const FileContentMap& files, + envoy::config::core::v3::DataSource& data_source); Init::TargetImpl init_target_; Event::Dispatcher& dispatcher_; @@ -86,7 +92,8 @@ class SdsApi : public Envoy::Config::SubscriptionBase< private: void validateUpdateSize(int num_resources); void initialize(); - uint64_t getHashForFiles(); + FileContentMap loadFiles(); + uint64_t getHashForFiles(const FileContentMap& files); // Invoked for filesystem watches on update. void onWatchUpdate(); @@ -167,35 +174,35 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider sds_tls_certificate_secrets_ = std::make_unique( secret.tls_certificate()); - if (secret.tls_certificate().has_watched_directory()) { - watched_directory_ = std::make_unique( - secret.tls_certificate().watched_directory(), dispatcher_, api_.fileSystem()); + resolved_tls_certificate_secrets_ = nullptr; + if (secret.tls_certificate().has_watched_path()) { + watched_path_ = std::make_unique(secret.tls_certificate().watched_path(), + dispatcher_); } else { - watched_directory_.reset(); + watched_path_.reset(); } - resolveSecret(); } - void resolveSecret() override { + void resolveSecret(const FileContentMap& files) override { resolved_tls_certificate_secrets_ = std::make_unique( *sds_tls_certificate_secrets_); - if (watched_directory_ != nullptr) { - watched_directory_->resolveDataSourcePath( - *resolved_tls_certificate_secrets_->mutable_certificate_chain()); - watched_directory_->resolveDataSourcePath( - *resolved_tls_certificate_secrets_->mutable_private_key()); + // We replace path based secrets with inlined secrets on update. + if (watched_path_ != nullptr) { + resolveDataSource(files, *resolved_tls_certificate_secrets_->mutable_certificate_chain()); + resolveDataSource(files, *resolved_tls_certificate_secrets_->mutable_private_key()); } } void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret&) override {} std::vector getDataSourceFilenames() override; - Config::WatchedDirectory* getWatchedDirectory() override { return watched_directory_.get(); } + Config::WatchedPath* getWatchedPath() override { return watched_path_.get(); } private: - // Directory to watch for symlink rotation. - Config::WatchedDirectoryPtr watched_directory_; + // Path to watch for rotation. + Config::WatchedPathPtr watched_path_; // TlsCertificate according to SDS source. TlsCertificatePtr sds_tls_certificate_secrets_; - // TlsCertificate after resolving paths via WatchedDirectory. + // TlsCertificate after reloading. Path based certificates are inlined for + // future read consistency. TlsCertificatePtr resolved_tls_certificate_secrets_; }; @@ -256,21 +263,24 @@ class CertificateValidationContextSdsApi : public SdsApi, sds_certificate_validation_context_secrets_ = std::make_unique< envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext>( secret.validation_context()); - if (secret.validation_context().has_watched_directory()) { - watched_directory_ = std::make_unique( - secret.validation_context().watched_directory(), dispatcher_, api_.fileSystem()); + resolved_certificate_validation_context_secrets_ = nullptr; + if (secret.validation_context().has_watched_path()) { + watched_path_ = std::make_unique( + secret.validation_context().watched_path(), dispatcher_); } else { - watched_directory_.reset(); + watched_path_.reset(); } - resolveSecret(); } - void resolveSecret() override { + + void resolveSecret(const FileContentMap& files) override { + // Copy existing CertificateValidationContext. resolved_certificate_validation_context_secrets_ = std::make_unique< envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext>( *sds_certificate_validation_context_secrets_); - if (watched_directory_ != nullptr) { - watched_directory_->resolveDataSourcePath( - *resolved_certificate_validation_context_secrets_->mutable_trusted_ca()); + // We replace path based secrets with inlined secrets on update. + if (watched_path_ != nullptr) { + resolveDataSource(files, + *resolved_certificate_validation_context_secrets_->mutable_trusted_ca()); } } @@ -279,15 +289,16 @@ class CertificateValidationContextSdsApi : public SdsApi, validation_callback_manager_.runCallbacks(secret.validation_context()); } std::vector getDataSourceFilenames() override; - Config::WatchedDirectory* getWatchedDirectory() override { return watched_directory_.get(); } + Config::WatchedPath* getWatchedPath() override { return watched_path_.get(); } private: - // Directory to watch for symlink rotation. - Config::WatchedDirectoryPtr watched_directory_; + // Directory to watch for rotation. + Config::WatchedPathPtr watched_path_; // CertificateValidationContext according to SDS source; CertificateValidationContextPtr sds_certificate_validation_context_secrets_; - // CertificateValidationContext after resolving paths via WatchedDirectory. + // CertificateValidationContext after resolving paths via watched_path_. CertificateValidationContextPtr resolved_certificate_validation_context_secrets_; + // Path based certificates are inlined for future read consistency. Common::CallbackManager< const envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext&> validation_callback_manager_; @@ -358,7 +369,7 @@ class TlsSessionTicketKeysSdsApi : public SdsApi, public TlsSessionTicketKeysCon validation_callback_manager_.runCallbacks(secret.session_ticket_keys()); } std::vector getDataSourceFilenames() override; - Config::WatchedDirectory* getWatchedDirectory() override { return nullptr; } + Config::WatchedPath* getWatchedPath() override { return nullptr; } private: Secret::TlsSessionTicketKeysPtr tls_session_ticket_keys_; @@ -420,7 +431,7 @@ class GenericSecretSdsApi : public SdsApi, public GenericSecretConfigProvider { validation_callback_manager_.runCallbacks(secret.generic_secret()); } std::vector getDataSourceFilenames() override; - Config::WatchedDirectory* getWatchedDirectory() override { return nullptr; } + Config::WatchedPath* getWatchedPath() override { return nullptr; } private: GenericSecretPtr generic_secret; diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 8d5cdb00294af..b7ea73978fa33 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -202,6 +202,7 @@ class PartialMockSds : public SdsApi { void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret&) override {} void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret&) override {} std::vector getDataSourceFilenames() override { return {}; } + Config::WatchedPath* getWatchedPath() override { return nullptr; } NiceMock validation_visitor_; }; diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index ef5ffae6bdb3c..bfb6dcc9c9d13 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -220,9 +220,8 @@ class SdsDynamicKeyRotationIntegrationTest : public SdsDynamicDownstreamIntegrat TestEnvironment::temporaryPath("root/current/servercert.pem")); tls_certificate->mutable_private_key()->set_filename( TestEnvironment::temporaryPath("root/current/serverkey.pem")); - auto* watched_dir = tls_certificate->mutable_watched_directory(); - watched_dir->set_root(TestEnvironment::temporaryPath("root")); - watched_dir->set_subdirectory("current"); + auto* watched_path = tls_certificate->mutable_watched_path(); + watched_path->set_path(TestEnvironment::temporaryPath("root/")); return secret; } }; From ce847fd27f4f9370f97a41a9bb9b55ee89edb4da Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 27 Oct 2020 11:54:49 +0000 Subject: [PATCH 03/14] Revert filesystem changes. Signed-off-by: Harvey Tuch --- include/envoy/filesystem/filesystem.h | 19 +-------------- .../filesystem/posix/filesystem_impl.cc | 23 ------------------- .../common/filesystem/posix/filesystem_impl.h | 2 -- test/mocks/filesystem/mocks.h | 2 -- 4 files changed, 1 insertion(+), 45 deletions(-) diff --git a/include/envoy/filesystem/filesystem.h b/include/envoy/filesystem/filesystem.h index 5edde8541c3c6..09764415b6a99 100644 --- a/include/envoy/filesystem/filesystem.h +++ b/include/envoy/filesystem/filesystem.h @@ -112,29 +112,12 @@ class Instance { virtual std::string fileReadToEnd(const std::string& path) PURE; /** - * @param path file path to split + * @path file path to split * @return PathSplitResult containing the parent directory of the input path and the file name * @note will throw an exception if path does not contain any path separator character */ virtual PathSplitResult splitPathFromFilename(absl::string_view path) PURE; - /** - * @param path1 first path component. - * @param path2 second path component. - * @return composite path with the second path component joined to the first. - */ - virtual std::string joinPath(absl::string_view path1, absl::string_view path2) PURE; - - /** - * Resolve a symlink path. - * @param path to resolve. - * @return resolved path. If there is an error, an exception is thrown, unless - * the error is a result of path not being a symlink, in which case - * the supplied path is returned. - * @throw EnvoyException if the path can't be resolved. - */ - virtual std::string readSymlink(absl::string_view path) PURE; - /** * Determine if the path is on a list of paths Envoy will refuse to access. This * is a basic sanity check for users, denying some clearly bad paths. Paths diff --git a/source/common/filesystem/posix/filesystem_impl.cc b/source/common/filesystem/posix/filesystem_impl.cc index 91ddc91b8b6d8..5f69e98e764bf 100644 --- a/source/common/filesystem/posix/filesystem_impl.cc +++ b/source/common/filesystem/posix/filesystem_impl.cc @@ -131,29 +131,6 @@ PathSplitResult InstanceImplPosix::splitPathFromFilename(absl::string_view path) return {path.substr(0, last_slash), name}; } -std::string InstanceImplPosix::joinPath(absl::string_view path1, absl::string_view path2) { - if (path2.empty()) { - return std::string(path1); - } - return absl::StrCat(path1, "/", path2); -} - -std::string InstanceImplPosix::readSymlink(absl::string_view path) { - std::string null_terminated_path{path}; - char resolved_path[PATH_MAX]; - const ssize_t result = ::readlink(null_terminated_path.data(), resolved_path, PATH_MAX); - if (result == -1) { - // If it's not a symlink, just return it, e.g. it might be simply - // directory. - if (errno == EINVAL) { - return std::string(path); - } - throw EnvoyException( - fmt::format("Unable to resolve symlink {}: {}", path, errorDetails(errno))); - } - return std::string(resolved_path, result); -} - bool InstanceImplPosix::illegalPath(const std::string& path) { // Special case, allow /dev/fd/* access here so that config can be passed in a // file descriptor from a bootstrap script via exec. The reason we do this diff --git a/source/common/filesystem/posix/filesystem_impl.h b/source/common/filesystem/posix/filesystem_impl.h index ffc4230e86775..4f81255be2225 100644 --- a/source/common/filesystem/posix/filesystem_impl.h +++ b/source/common/filesystem/posix/filesystem_impl.h @@ -39,8 +39,6 @@ class InstanceImplPosix : public Instance { ssize_t fileSize(const std::string& path) override; std::string fileReadToEnd(const std::string& path) override; PathSplitResult splitPathFromFilename(absl::string_view path) override; - std::string joinPath(absl::string_view path1, absl::string_view path2) override; - std::string readSymlink(absl::string_view path) override; bool illegalPath(const std::string& path) override; private: diff --git a/test/mocks/filesystem/mocks.h b/test/mocks/filesystem/mocks.h index fc50098abedbc..b6fca8ed4af1c 100644 --- a/test/mocks/filesystem/mocks.h +++ b/test/mocks/filesystem/mocks.h @@ -53,8 +53,6 @@ class MockInstance : public Instance { MOCK_METHOD(ssize_t, fileSize, (const std::string&)); MOCK_METHOD(std::string, fileReadToEnd, (const std::string&)); MOCK_METHOD(PathSplitResult, splitPathFromFilename, (absl::string_view)); - MOCK_METHOD(std::string, joinPath, (absl::string_view, absl::string_view)); - MOCK_METHOD(std::string, readSymlink, (absl::string_view)); MOCK_METHOD(bool, illegalPath, (const std::string&)); }; From cbe018bfaf4f02ee1a6c5700af6b341a439bcf87 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 27 Oct 2020 12:05:01 +0000 Subject: [PATCH 04/14] Always load secrets inline, even with non-watched path. Bound retries. Fix format. Signed-off-by: Harvey Tuch --- source/common/secret/sds_api.cc | 11 ++++++++--- source/common/secret/sds_api.h | 12 ++++-------- test/integration/sds_dynamic_integration_test.cc | 2 +- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 47e64d050ecb9..ae52c28d4f40c 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -45,17 +45,22 @@ void SdsApi::resolveDataSource(const FileContentMap& files, } void SdsApi::onWatchUpdate() { - // Obtain a stable set of files. If a rotation happens while we're eading, + // Obtain a stable set of files. If a rotation happens while we're reading, // then we need to try again. uint64_t prev_hash = 0; FileContentMap files = loadFiles(); uint64_t next_hash = getHashForFiles(files); - // TODO(htuch): bound this so we don't run forever. DO NOT SUBMIT until fixed. - while (next_hash != prev_hash) { + const uint64_t MaxBoundedRetries = 5; + for (uint64_t bounded_retries = MaxBoundedRetries; next_hash != prev_hash && bounded_retries > 0; + --bounded_retries) { files = loadFiles(); prev_hash = next_hash; next_hash = getHashForFiles(files); } + if (next_hash != prev_hash) { + ENVOY_LOG_MISC(warn, "Unable to refresh secrets due to > {} non-atomic rotations observed", + MaxBoundedRetries); + } const uint64_t new_hash = next_hash; if (new_hash != files_hash_) { resolveSecret(files); diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 05e51f7ef8d71..e5784648b764f 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -187,10 +187,8 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider std::make_unique( *sds_tls_certificate_secrets_); // We replace path based secrets with inlined secrets on update. - if (watched_path_ != nullptr) { - resolveDataSource(files, *resolved_tls_certificate_secrets_->mutable_certificate_chain()); - resolveDataSource(files, *resolved_tls_certificate_secrets_->mutable_private_key()); - } + resolveDataSource(files, *resolved_tls_certificate_secrets_->mutable_certificate_chain()); + resolveDataSource(files, *resolved_tls_certificate_secrets_->mutable_private_key()); } void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret&) override {} std::vector getDataSourceFilenames() override; @@ -278,10 +276,8 @@ class CertificateValidationContextSdsApi : public SdsApi, envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext>( *sds_certificate_validation_context_secrets_); // We replace path based secrets with inlined secrets on update. - if (watched_path_ != nullptr) { - resolveDataSource(files, - *resolved_certificate_validation_context_secrets_->mutable_trusted_ca()); - } + resolveDataSource(files, + *resolved_certificate_validation_context_secrets_->mutable_trusted_ca()); } void diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index bfb6dcc9c9d13..a1b0b409e68b4 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -259,7 +259,7 @@ TEST_P(SdsDynamicKeyRotationIntegrationTest, BasicRotation) { TestEnvironment::temporaryPath("root/current")); test_server_->waitForCounterGe( listenerStatPrefix("server_ssl_socket_factory.ssl_context_update_by_sds"), 2); - // First request with server_ecda{cert,key}.pem. + // First request with server_ecdsa{cert,key}.pem. testRouterHeaderOnlyRequestAndResponse(&creator); } From 6ae5d801b6386a049c44979706625f5b0da44a06 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 27 Oct 2020 19:20:38 -0400 Subject: [PATCH 05/14] Back to WatchedDirectory. Signed-off-by: Harvey Tuch --- api/envoy/config/core/v3/base.proto | 19 +++-------- api/envoy/config/core/v4alpha/base.proto | 22 ++++-------- .../transport_sockets/tls/v3/common.proto | 8 ++--- .../tls/v4alpha/common.proto | 8 ++--- .../envoy/config/core/v3/base.proto | 19 +++-------- .../envoy/config/core/v4alpha/base.proto | 22 ++++-------- .../transport_sockets/tls/v3/common.proto | 8 ++--- .../tls/v4alpha/common.proto | 8 ++--- source/common/config/BUILD | 6 ++-- source/common/config/watched_directory.cc | 14 ++++++++ .../{watched_path.h => watched_directory.h} | 9 ++--- source/common/config/watched_path.cc | 30 ---------------- source/common/secret/BUILD | 2 +- source/common/secret/sds_api.cc | 8 ++--- source/common/secret/sds_api.h | 34 +++++++++---------- test/common/secret/sds_api_test.cc | 2 +- .../sds_dynamic_integration_test.cc | 4 +-- 17 files changed, 83 insertions(+), 140 deletions(-) create mode 100644 source/common/config/watched_directory.cc rename source/common/config/{watched_path.h => watched_directory.h} (65%) delete mode 100644 source/common/config/watched_path.cc diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index ca950062b2a1d..5b5339ea5bc5d 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -313,22 +313,11 @@ message HeaderMap { repeated HeaderValue headers = 1; } -// A path that is watched for changes, e.g. by inotify on Linux. -message WatchedPath { - enum Event { - // Generated for the directory containing the new filename when a file is - // renamed. - MOVED_TO = 0; - - // File was modified, e.g. write(2), truncate(2). - MODIFIED = 1; - } - - // Path to watch. +// A directory that is watched for changes, e.g. by inotify on Linux. Move/rename +// events inside this directory trigger the watch. +message WatchedDirectory { + // Directory path to watch. string path = 1 [(validate.rules).string = {min_len: 1}]; - - // Event to watch on *path*. - Event event = 2 [(validate.rules).enum = {defined_only: true}]; } // Data source consisting of either a file or an inline value. diff --git a/api/envoy/config/core/v4alpha/base.proto b/api/envoy/config/core/v4alpha/base.proto index aab29fd050d12..27b0b356b1a79 100644 --- a/api/envoy/config/core/v4alpha/base.proto +++ b/api/envoy/config/core/v4alpha/base.proto @@ -308,24 +308,14 @@ message HeaderMap { repeated HeaderValue headers = 1; } -// A path that is watched for changes, e.g. by inotify on Linux. -message WatchedPath { - option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.WatchedPath"; - - enum Event { - // Generated for the directory containing the new filename when a file is - // renamed. - MOVED_TO = 0; - - // File was modified, e.g. write(2), truncate(2). - MODIFIED = 1; - } +// A directory that is watched for changes, e.g. by inotify on Linux. Move/rename +// events inside this directory trigger the watch. +message WatchedDirectory { + option (udpa.annotations.versioning).previous_message_type = + "envoy.config.core.v3.WatchedDirectory"; - // Path to watch. + // Directory path to watch. string path = 1 [(validate.rules).string = {min_len: 1}]; - - // Event to watch on *path*. - Event event = 2 [(validate.rules).enum = {defined_only: true}]; } // Data source consisting of either a file or an inline value. diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index 21a87e86585f0..18b5c1c178eec 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -151,7 +151,7 @@ message TlsCertificate { // sources will be triggered by this watch. The certificate/key pair will be // read together and validated for atomic read consistency (i.e. no intervening // modification occurred between cert/key read). - config.core.v3.WatchedPath watched_path = 7; + config.core.v3.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be @@ -255,9 +255,9 @@ message CertificateValidationContext { // delivered via SDS. config.core.v3.DataSource trusted_ca = 1; - // If specified, updates of a file-based *trusted_ca* - // source will be triggered by this watch. - config.core.v3.WatchedPath watched_path = 11; + // If specified, updates of a file-based *trusted_ca* source will be triggered + // by this watch. + config.core.v3.WatchedDirectory watched_directory = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate diff --git a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto index 2740357ccca0a..7216807afe4de 100644 --- a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -153,7 +153,7 @@ message TlsCertificate { // sources will be triggered by this watch. The certificate/key pair will be // read together and validated for atomic read consistency (i.e. no intervening // modification occurred between cert/key read). - config.core.v4alpha.WatchedPath watched_path = 7; + config.core.v4alpha.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be @@ -257,9 +257,9 @@ message CertificateValidationContext { // delivered via SDS. config.core.v4alpha.DataSource trusted_ca = 1; - // If specified, updates of a file-based *trusted_ca* - // source will be triggered by this watch. - config.core.v4alpha.WatchedPath watched_path = 11; + // If specified, updates of a file-based *trusted_ca* source will be triggered + // by this watch. + config.core.v4alpha.WatchedDirectory watched_directory = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate diff --git a/generated_api_shadow/envoy/config/core/v3/base.proto b/generated_api_shadow/envoy/config/core/v3/base.proto index 09c71c5a35764..1184c89de6e22 100644 --- a/generated_api_shadow/envoy/config/core/v3/base.proto +++ b/generated_api_shadow/envoy/config/core/v3/base.proto @@ -311,22 +311,11 @@ message HeaderMap { repeated HeaderValue headers = 1; } -// A path that is watched for changes, e.g. by inotify on Linux. -message WatchedPath { - enum Event { - // Generated for the directory containing the new filename when a file is - // renamed. - MOVED_TO = 0; - - // File was modified, e.g. write(2), truncate(2). - MODIFIED = 1; - } - - // Path to watch. +// A directory that is watched for changes, e.g. by inotify on Linux. Move/rename +// events inside this directory trigger the watch. +message WatchedDirectory { + // Directory path to watch. string path = 1 [(validate.rules).string = {min_len: 1}]; - - // Event to watch on *path*. - Event event = 2 [(validate.rules).enum = {defined_only: true}]; } // Data source consisting of either a file or an inline value. diff --git a/generated_api_shadow/envoy/config/core/v4alpha/base.proto b/generated_api_shadow/envoy/config/core/v4alpha/base.proto index b6c2d99d14953..95ca4f77a2bcd 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/base.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/base.proto @@ -315,24 +315,14 @@ message HeaderMap { repeated HeaderValue headers = 1; } -// A path that is watched for changes, e.g. by inotify on Linux. -message WatchedPath { - option (udpa.annotations.versioning).previous_message_type = "envoy.config.core.v3.WatchedPath"; - - enum Event { - // Generated for the directory containing the new filename when a file is - // renamed. - MOVED_TO = 0; - - // File was modified, e.g. write(2), truncate(2). - MODIFIED = 1; - } +// A directory that is watched for changes, e.g. by inotify on Linux. Move/rename +// events inside this directory trigger the watch. +message WatchedDirectory { + option (udpa.annotations.versioning).previous_message_type = + "envoy.config.core.v3.WatchedDirectory"; - // Path to watch. + // Directory path to watch. string path = 1 [(validate.rules).string = {min_len: 1}]; - - // Event to watch on *path*. - Event event = 2 [(validate.rules).enum = {defined_only: true}]; } // Data source consisting of either a file or an inline value. diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto index 6113b57fffcf4..ddcf26a1b94c8 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -150,7 +150,7 @@ message TlsCertificate { // sources will be triggered by this watch. The certificate/key pair will be // read together and validated for atomic read consistency (i.e. no intervening // modification occurred between cert/key read). - config.core.v3.WatchedPath watched_path = 7; + config.core.v3.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be @@ -252,9 +252,9 @@ message CertificateValidationContext { // delivered via SDS. config.core.v3.DataSource trusted_ca = 1; - // If specified, updates of a file-based *trusted_ca* - // source will be triggered by this watch. - config.core.v3.WatchedPath watched_path = 11; + // If specified, updates of a file-based *trusted_ca* source will be triggered + // by this watch. + config.core.v3.WatchedDirectory watched_directory = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto index 2740357ccca0a..7216807afe4de 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -153,7 +153,7 @@ message TlsCertificate { // sources will be triggered by this watch. The certificate/key pair will be // read together and validated for atomic read consistency (i.e. no intervening // modification occurred between cert/key read). - config.core.v4alpha.WatchedPath watched_path = 7; + config.core.v4alpha.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key // ` field. This can't be @@ -257,9 +257,9 @@ message CertificateValidationContext { // delivered via SDS. config.core.v4alpha.DataSource trusted_ca = 1; - // If specified, updates of a file-based *trusted_ca* - // source will be triggered by this watch. - config.core.v4alpha.WatchedPath watched_path = 11; + // If specified, updates of a file-based *trusted_ca* source will be triggered + // by this watch. + config.core.v4alpha.WatchedDirectory watched_directory = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 01fc3fb21e84e..7c5460df96bb7 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -445,9 +445,9 @@ envoy_cc_library( ) envoy_cc_library( - name = "watched_path_lib", - srcs = ["watched_path.cc"], - hdrs = ["watched_path.h"], + name = "watched_directory_lib", + srcs = ["watched_directory.cc"], + hdrs = ["watched_directory.h"], deps = [ "//include/envoy/event:dispatcher_interface", "//include/envoy/filesystem:watcher_interface", diff --git a/source/common/config/watched_directory.cc b/source/common/config/watched_directory.cc new file mode 100644 index 0000000000000..c3843ee3f71c7 --- /dev/null +++ b/source/common/config/watched_directory.cc @@ -0,0 +1,14 @@ +#include "common/config/watched_directory.h" + +namespace Envoy { +namespace Config { + +WatchedDirectory::WatchedDirectory(const envoy::config::core::v3::WatchedDirectory& config, + Event::Dispatcher& dispatcher) { + watcher_ = dispatcher.createFilesystemWatcher(); + watcher_->addWatch(absl::StrCat(config.path(), "/"), Filesystem::Watcher::Events::MovedTo, + [this](uint32_t) { cb_(); }); +} + +} // namespace Config +} // namespace Envoy diff --git a/source/common/config/watched_path.h b/source/common/config/watched_directory.h similarity index 65% rename from source/common/config/watched_path.h rename to source/common/config/watched_directory.h index 2031e798d512e..34e965a0b4df0 100644 --- a/source/common/config/watched_path.h +++ b/source/common/config/watched_directory.h @@ -7,12 +7,13 @@ namespace Envoy { namespace Config { -// Implement the common functionality of envoy::config::core::v3::WatchedPath. -class WatchedPath { +// Implement the common functionality of envoy::config::core::v3::WatchedDirectory. +class WatchedDirectory { public: using Callback = std::function; - WatchedPath(const envoy::config::core::v3::WatchedPath& config, Event::Dispatcher& dispatcher); + WatchedDirectory(const envoy::config::core::v3::WatchedDirectory& config, + Event::Dispatcher& dispatcher); void setCallback(Callback cb) { cb_ = cb; } @@ -21,7 +22,7 @@ class WatchedPath { Callback cb_; }; -using WatchedPathPtr = std::unique_ptr; +using WatchedDirectoryPtr = std::unique_ptr; } // namespace Config } // namespace Envoy diff --git a/source/common/config/watched_path.cc b/source/common/config/watched_path.cc deleted file mode 100644 index 31f31a80f2ae0..0000000000000 --- a/source/common/config/watched_path.cc +++ /dev/null @@ -1,30 +0,0 @@ -#include "common/config/watched_path.h" - -namespace Envoy { -namespace Config { - -namespace { - -uint32_t watchedPathEventToWatcherEvent(envoy::config::core::v3::WatchedPath::Event event) { - switch (event) { - case envoy::config::core::v3::WatchedPath::MOVED_TO: - return Filesystem::Watcher::Events::MovedTo; - case envoy::config::core::v3::WatchedPath::MODIFIED: - return Filesystem::Watcher::Events::Modified; - - default: - NOT_REACHED_GCOVR_EXCL_LINE; - } -} - -} // namespace - -WatchedPath::WatchedPath(const envoy::config::core::v3::WatchedPath& config, - Event::Dispatcher& dispatcher) { - watcher_ = dispatcher.createFilesystemWatcher(); - watcher_->addWatch(config.path(), watchedPathEventToWatcherEvent(config.event()), - [this](uint32_t) { cb_(); }); -} - -} // namespace Config -} // namespace Envoy diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 63fbcf20b5643..84defb7ef88bd 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -58,7 +58,7 @@ envoy_cc_library( "//source/common/config:api_version_lib", "//source/common/config:subscription_base_interface", "//source/common/config:utility_lib", - "//source/common/config:watched_path_lib", + "//source/common/config:watched_directory_lib", "//source/common/init:target_lib", "//source/common/protobuf:utility_lib", "//source/common/ssl:certificate_validation_context_config_impl_lib", diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index ae52c28d4f40c..04eeb48ca9b93 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -91,12 +91,12 @@ void SdsApi::onConfigUpdate(const std::vector& resou resolveSecret(files); update_callback_manager_.runCallbacks(); - auto* watched_path = getWatchedPath(); + auto* watched_directory = getWatchedDirectory(); // Either we have a watched path and can defer the watch monitoring to a - // WatchedPath object, or we need to implement per-file watches in the else + // WatchedDirectory object, or we need to implement per-file watches in the else // clause. - if (watched_path != nullptr) { - watched_path->setCallback([this]() { onWatchUpdate(); }); + if (watched_directory != nullptr) { + watched_directory->setCallback([this]() { onWatchUpdate(); }); } else { // List DataSources that refer to files auto files = getDataSourceFilenames(); diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index e5784648b764f..87916f836adb7 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -23,7 +23,7 @@ #include "common/common/cleanup.h" #include "common/config/subscription_base.h" #include "common/config/utility.h" -#include "common/config/watched_path.h" +#include "common/config/watched_directory.h" #include "common/init/target_impl.h" #include "common/ssl/certificate_validation_context_config_impl.h" #include "common/ssl/tls_certificate_config_impl.h" @@ -80,7 +80,7 @@ class SdsApi : public Envoy::Config::SubscriptionBase< void onConfigUpdateFailed(Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException* e) override; virtual std::vector getDataSourceFilenames() PURE; - virtual Config::WatchedPath* getWatchedPath() PURE; + virtual Config::WatchedDirectory* getWatchedDirectory() PURE; void resolveDataSource(const FileContentMap& files, envoy::config::core::v3::DataSource& data_source); @@ -175,11 +175,11 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider std::make_unique( secret.tls_certificate()); resolved_tls_certificate_secrets_ = nullptr; - if (secret.tls_certificate().has_watched_path()) { - watched_path_ = std::make_unique(secret.tls_certificate().watched_path(), - dispatcher_); + if (secret.tls_certificate().has_watched_directory()) { + watched_directory_ = std::make_unique( + secret.tls_certificate().watched_directory(), dispatcher_); } else { - watched_path_.reset(); + watched_directory_.reset(); } } void resolveSecret(const FileContentMap& files) override { @@ -192,11 +192,11 @@ class TlsCertificateSdsApi : public SdsApi, public TlsCertificateConfigProvider } void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret&) override {} std::vector getDataSourceFilenames() override; - Config::WatchedPath* getWatchedPath() override { return watched_path_.get(); } + Config::WatchedDirectory* getWatchedDirectory() override { return watched_directory_.get(); } private: // Path to watch for rotation. - Config::WatchedPathPtr watched_path_; + Config::WatchedDirectoryPtr watched_directory_; // TlsCertificate according to SDS source. TlsCertificatePtr sds_tls_certificate_secrets_; // TlsCertificate after reloading. Path based certificates are inlined for @@ -262,11 +262,11 @@ class CertificateValidationContextSdsApi : public SdsApi, envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext>( secret.validation_context()); resolved_certificate_validation_context_secrets_ = nullptr; - if (secret.validation_context().has_watched_path()) { - watched_path_ = std::make_unique( - secret.validation_context().watched_path(), dispatcher_); + if (secret.validation_context().has_watched_directory()) { + watched_directory_ = std::make_unique( + secret.validation_context().watched_directory(), dispatcher_); } else { - watched_path_.reset(); + watched_directory_.reset(); } } @@ -285,14 +285,14 @@ class CertificateValidationContextSdsApi : public SdsApi, validation_callback_manager_.runCallbacks(secret.validation_context()); } std::vector getDataSourceFilenames() override; - Config::WatchedPath* getWatchedPath() override { return watched_path_.get(); } + Config::WatchedDirectory* getWatchedDirectory() override { return watched_directory_.get(); } private: // Directory to watch for rotation. - Config::WatchedPathPtr watched_path_; + Config::WatchedDirectoryPtr watched_directory_; // CertificateValidationContext according to SDS source; CertificateValidationContextPtr sds_certificate_validation_context_secrets_; - // CertificateValidationContext after resolving paths via watched_path_. + // CertificateValidationContext after resolving paths via watched_directory_. CertificateValidationContextPtr resolved_certificate_validation_context_secrets_; // Path based certificates are inlined for future read consistency. Common::CallbackManager< @@ -365,7 +365,7 @@ class TlsSessionTicketKeysSdsApi : public SdsApi, public TlsSessionTicketKeysCon validation_callback_manager_.runCallbacks(secret.session_ticket_keys()); } std::vector getDataSourceFilenames() override; - Config::WatchedPath* getWatchedPath() override { return nullptr; } + Config::WatchedDirectory* getWatchedDirectory() override { return nullptr; } private: Secret::TlsSessionTicketKeysPtr tls_session_ticket_keys_; @@ -427,7 +427,7 @@ class GenericSecretSdsApi : public SdsApi, public GenericSecretConfigProvider { validation_callback_manager_.runCallbacks(secret.generic_secret()); } std::vector getDataSourceFilenames() override; - Config::WatchedPath* getWatchedPath() override { return nullptr; } + Config::WatchedDirectory* getWatchedDirectory() override { return nullptr; } private: GenericSecretPtr generic_secret; diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index b7ea73978fa33..6d44709b3c78b 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -202,7 +202,7 @@ class PartialMockSds : public SdsApi { void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret&) override {} void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret&) override {} std::vector getDataSourceFilenames() override { return {}; } - Config::WatchedPath* getWatchedPath() override { return nullptr; } + Config::WatchedDirectory* getWatchedDirectory() override { return nullptr; } NiceMock validation_visitor_; }; diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index a1b0b409e68b4..6c4dedaf57684 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -220,8 +220,8 @@ class SdsDynamicKeyRotationIntegrationTest : public SdsDynamicDownstreamIntegrat TestEnvironment::temporaryPath("root/current/servercert.pem")); tls_certificate->mutable_private_key()->set_filename( TestEnvironment::temporaryPath("root/current/serverkey.pem")); - auto* watched_path = tls_certificate->mutable_watched_path(); - watched_path->set_path(TestEnvironment::temporaryPath("root/")); + auto* watched_directory = tls_certificate->mutable_watched_directory(); + watched_directory->set_path(TestEnvironment::temporaryPath("root")); return secret; } }; From e6fca97821d31f4316892358945b0d4239e36561 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 10 Nov 2020 16:40:04 -0500 Subject: [PATCH 06/14] Clarify how cert/key consistency is achieved (hashe comparisons). Signed-off-by: Harvey Tuch --- api/envoy/extensions/transport_sockets/tls/v3/common.proto | 3 ++- .../extensions/transport_sockets/tls/v4alpha/common.proto | 3 ++- .../envoy/extensions/transport_sockets/tls/v3/common.proto | 3 ++- .../extensions/transport_sockets/tls/v4alpha/common.proto | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index 18b5c1c178eec..31ef951e6707c 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -150,7 +150,8 @@ message TlsCertificate { // If specified, updates of file-based *certificate_chain* and *private_key* // sources will be triggered by this watch. The certificate/key pair will be // read together and validated for atomic read consistency (i.e. no intervening - // modification occurred between cert/key read). + // modification occurred between cert/key read, verified by file hash + // comparisons). config.core.v3.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key diff --git a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto index 7216807afe4de..1c34fc92ca1c0 100644 --- a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -152,7 +152,8 @@ message TlsCertificate { // If specified, updates of file-based *certificate_chain* and *private_key* // sources will be triggered by this watch. The certificate/key pair will be // read together and validated for atomic read consistency (i.e. no intervening - // modification occurred between cert/key read). + // modification occurred between cert/key read, verified by file hash + // comparisons). config.core.v4alpha.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto index ddcf26a1b94c8..f41892d21c4b0 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -149,7 +149,8 @@ message TlsCertificate { // If specified, updates of file-based *certificate_chain* and *private_key* // sources will be triggered by this watch. The certificate/key pair will be // read together and validated for atomic read consistency (i.e. no intervening - // modification occurred between cert/key read). + // modification occurred between cert/key read, verified by file hash + // comparisons). config.core.v3.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto index 7216807afe4de..1c34fc92ca1c0 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -152,7 +152,8 @@ message TlsCertificate { // If specified, updates of file-based *certificate_chain* and *private_key* // sources will be triggered by this watch. The certificate/key pair will be // read together and validated for atomic read consistency (i.e. no intervening - // modification occurred between cert/key read). + // modification occurred between cert/key read, verified by file hash + // comparisons). config.core.v4alpha.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key From e8c318187352f62edc0c45ef2697e308d2b20d17 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 10 Nov 2020 16:53:08 -0500 Subject: [PATCH 07/14] WatchedDirectory unit test. Signed-off-by: Harvey Tuch --- test/common/config/BUILD | 10 ++++++ test/common/config/watched_directory_test.cc | 33 ++++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 test/common/config/watched_directory_test.cc diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 52c508569ec61..c95c4b8bb72e2 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -502,3 +502,13 @@ envoy_cc_test( "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", ], ) + +envoy_cc_test( + name = "watched_directory_test", + srcs = ["watched_directory_test.cc"], + deps = [ + "//source/common/config:watched_directory_lib", + "//test/mocks/event:event_mocks", + "//test/mocks/filesystem:filesystem_mocks", + ], +) diff --git a/test/common/config/watched_directory_test.cc b/test/common/config/watched_directory_test.cc new file mode 100644 index 0000000000000..59ed7f107bd48 --- /dev/null +++ b/test/common/config/watched_directory_test.cc @@ -0,0 +1,33 @@ +#include "common/config/watched_directory.h" + +#include "envoy/filesystem/watcher.h" + +#include "test/mocks/event/mocks.h" +#include "test/mocks/filesystem/mocks.h" + +#include "gtest/gtest.h" + +using testing::Return; +using testing::SaveArg; + +namespace Envoy { +namespace Config { + +TEST(WatchedDirectory, All) { + Event::MockDispatcher dispatcher; + envoy::config::core::v3::WatchedDirectory config; + config.set_path("foo/bar"); + auto* watcher = new Filesystem::MockWatcher(); + EXPECT_CALL(dispatcher, createFilesystemWatcher_()).WillOnce(Return(watcher)); + Filesystem::Watcher::OnChangedCb cb; + EXPECT_CALL(*watcher, addWatch("foo/bar/", Filesystem::Watcher::Events::MovedTo, _)) + .WillOnce(SaveArg<2>(&cb)); + WatchedDirectory wd(config, dispatcher); + bool called = false; + wd.setCallback([&called] { called = true; }); + cb(Filesystem::Watcher::Events::MovedTo); + EXPECT_TRUE(called); +} + +} // namespace Config +} // namespace Envoy From c5ed1a15c935987e9590e5222222b7a3161781f0 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 10 Nov 2020 19:36:38 -0500 Subject: [PATCH 08/14] SdsApi unit tests. Signed-off-by: Harvey Tuch --- source/common/secret/sds_api.cc | 3 +- test/common/config/watched_directory_test.cc | 4 +- test/common/secret/BUILD | 1 + test/common/secret/sds_api_test.cc | 300 ++++++++++++++++++- test/test_common/utility.cc | 4 + test/test_common/utility.h | 1 + 6 files changed, 307 insertions(+), 6 deletions(-) diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index 04eeb48ca9b93..fe71bf64f1310 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -58,7 +58,8 @@ void SdsApi::onWatchUpdate() { next_hash = getHashForFiles(files); } if (next_hash != prev_hash) { - ENVOY_LOG_MISC(warn, "Unable to refresh secrets due to > {} non-atomic rotations observed", + ENVOY_LOG_MISC(warn, + "Unable to atomically refresh secrets due to > {} non-atomic rotations observed", MaxBoundedRetries); } const uint64_t new_hash = next_hash; diff --git a/test/common/config/watched_directory_test.cc b/test/common/config/watched_directory_test.cc index 59ed7f107bd48..8988306d4ad4d 100644 --- a/test/common/config/watched_directory_test.cc +++ b/test/common/config/watched_directory_test.cc @@ -1,7 +1,7 @@ -#include "common/config/watched_directory.h" - #include "envoy/filesystem/watcher.h" +#include "common/config/watched_directory.h" + #include "test/mocks/event/mocks.h" #include "test/mocks/filesystem/mocks.h" diff --git a/test/common/secret/BUILD b/test/common/secret/BUILD index 48572641a39b2..c341f6394c2a9 100644 --- a/test/common/secret/BUILD +++ b/test/common/secret/BUILD @@ -49,6 +49,7 @@ envoy_cc_test( "//test/mocks/secret:secret_mocks", "//test/mocks/server:instance_mocks", "//test/test_common:environment_lib", + "//test/test_common:logging_lib", "//test/test_common:registry_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 6d44709b3c78b..b57def8502f06 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -18,23 +18,28 @@ #include "test/mocks/secret/mocks.h" #include "test/mocks/server/instance.h" #include "test/test_common/environment.h" +#include "test/test_common/logging.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" using ::testing::_; +using ::testing::InSequence; using ::testing::Invoke; using ::testing::InvokeWithoutArgs; +using ::testing::Return; namespace Envoy { namespace Secret { namespace { -class SdsApiTest : public testing::Test { +class SdsApiTestBase { protected: - SdsApiTest() - : api_(Api::createApiForTest()), dispatcher_(api_->allocateDispatcher("test_thread")) {} + SdsApiTestBase() { + api_ = Api::createApiForTest(); + dispatcher_ = api_->allocateDispatcher("test_thread"); + } void initialize() { init_target_handle_->initialize(init_watcher_); } void setupMocks() { @@ -53,6 +58,8 @@ class SdsApiTest : public testing::Test { Event::DispatcherPtr dispatcher_; }; +class SdsApiTest : public testing::Test, public SdsApiTestBase {}; + // Validate that SdsApi object is created and initialized successfully. TEST_F(SdsApiTest, BasicTest) { ::testing::InSequence s; @@ -180,6 +187,293 @@ TEST_F(SdsApiTest, DynamicTlsCertificateUpdateSuccess) { handle->remove(); } +class SdsRotationApiTest : public SdsApiTestBase { +protected: + SdsRotationApiTest() { + api_ = Api::createApiForTest(filesystem_); + setupMocks(); + EXPECT_CALL(filesystem_, splitPathFromFilename(_)) + .WillRepeatedly(Invoke([](absl::string_view path) -> Filesystem::PathSplitResult { + return Filesystem::fileSystemForTest().splitPathFromFilename(path); + })); + } + + Secret::MockSecretCallbacks secret_callback_; + Common::CallbackHandle* handle_{}; + std::vector watch_cbs_; + Event::MockDispatcher mock_dispatcher_; + Filesystem::MockInstance filesystem_; + NiceMock server_; +}; + +class TlsCertificateSdsRotationApiTest : public testing::TestWithParam, + public SdsRotationApiTest { +protected: + TlsCertificateSdsRotationApiTest() + : watched_directory_(GetParam()), cert_path_("/foo/bar/cert.pem"), + key_path_("/foo/bar/key.pem"), expected_watch_path_("/foo/bar/"), trigger_path_("/foo") { + envoy::config::core::v3::ConfigSource config_source; + sds_api_ = std::make_unique( + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, + server_.stats(), []() {}, mock_dispatcher_, *api_); + sds_api_->registerInitTarget(init_manager_); + initialize(); + handle_ = sds_api_->addUpdateCallback([this]() { secret_callback_.onAddOrUpdateSecret(); }); + } + + ~TlsCertificateSdsRotationApiTest() { handle_->remove(); } + + void onConfigUpdate(const std::string& cert_value, const std::string& key_value) { + const std::string yaml = fmt::format( + R"EOF( + name: "abc.com" + tls_certificate: + certificate_chain: + filename: "{}" + private_key: + filename: "{}" + )EOF", + cert_path_, key_path_); + envoy::extensions::transport_sockets::tls::v3::Secret typed_secret; + TestUtility::loadFromYaml(yaml, typed_secret); + if (watched_directory_) { + typed_secret.mutable_tls_certificate()->mutable_watched_directory()->set_path(trigger_path_); + } + const auto decoded_resources = TestUtility::decodeResources({typed_secret}); + + auto* watcher = new Filesystem::MockWatcher(); + if (watched_directory_) { + EXPECT_CALL(mock_dispatcher_, createFilesystemWatcher_()).WillOnce(Return(watcher)); + EXPECT_CALL(*watcher, addWatch(trigger_path_ + "/", Filesystem::Watcher::Events::MovedTo, _)) + .WillOnce( + Invoke([this](absl::string_view, uint32_t, Filesystem::Watcher::OnChangedCb cb) { + watch_cbs_.push_back(cb); + })); + EXPECT_CALL(filesystem_, fileReadToEnd(cert_path_)).WillOnce(Return(cert_value)); + EXPECT_CALL(filesystem_, fileReadToEnd(key_path_)).WillOnce(Return(key_value)); + EXPECT_CALL(secret_callback_, onAddOrUpdateSecret()); + } else { + EXPECT_CALL(filesystem_, fileReadToEnd(cert_path_)).WillOnce(Return(cert_value)); + EXPECT_CALL(filesystem_, fileReadToEnd(key_path_)).WillOnce(Return(key_value)); + EXPECT_CALL(secret_callback_, onAddOrUpdateSecret()); + EXPECT_CALL(mock_dispatcher_, createFilesystemWatcher_()).WillOnce(Return(watcher)); + EXPECT_CALL(*watcher, addWatch(expected_watch_path_, Filesystem::Watcher::Events::MovedTo, _)) + .Times(2) + .WillRepeatedly( + Invoke([this](absl::string_view, uint32_t, Filesystem::Watcher::OnChangedCb cb) { + watch_cbs_.push_back(cb); + })); + } + subscription_factory_.callbacks_->onConfigUpdate(decoded_resources.refvec_, ""); + } + + const bool watched_directory_; + std::string cert_path_; + std::string key_path_; + std::string expected_watch_path_; + std::string trigger_path_; + std::unique_ptr sds_api_; +}; + +INSTANTIATE_TEST_SUITE_P(TlsCertificateSdsRotationApiTestParams, TlsCertificateSdsRotationApiTest, + testing::Values(false, true)); + +class CertificateValidationContextSdsRotationApiTest : public testing::TestWithParam, + public SdsRotationApiTest { +protected: + CertificateValidationContextSdsRotationApiTest() { + envoy::config::core::v3::ConfigSource config_source; + sds_api_ = std::make_unique( + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, + server_.stats(), []() {}, mock_dispatcher_, *api_); + sds_api_->registerInitTarget(init_manager_); + initialize(); + handle_ = sds_api_->addUpdateCallback([this]() { secret_callback_.onAddOrUpdateSecret(); }); + } + + ~CertificateValidationContextSdsRotationApiTest() { handle_->remove(); } + + void onConfigUpdate(const std::string& trusted_ca_path, const std::string& trusted_ca_value, + const std::string& watch_path) { + const std::string yaml = fmt::format( + R"EOF( + name: "abc.com" + validation_context: + trusted_ca: + filename: "{}" + allow_expired_certificate: true + )EOF", + trusted_ca_path); + envoy::extensions::transport_sockets::tls::v3::Secret typed_secret; + TestUtility::loadFromYaml(yaml, typed_secret); + const auto decoded_resources = TestUtility::decodeResources({typed_secret}); + + auto* watcher = new Filesystem::MockWatcher(); + EXPECT_CALL(filesystem_, fileReadToEnd(trusted_ca_path)).WillOnce(Return(trusted_ca_value)); + EXPECT_CALL(secret_callback_, onAddOrUpdateSecret()); + EXPECT_CALL(mock_dispatcher_, createFilesystemWatcher_()).WillOnce(Return(watcher)); + EXPECT_CALL(*watcher, addWatch(watch_path, Filesystem::Watcher::Events::MovedTo, _)) + .WillOnce(Invoke([this](absl::string_view, uint32_t, Filesystem::Watcher::OnChangedCb cb) { + watch_cbs_.push_back(cb); + })); + subscription_factory_.callbacks_->onConfigUpdate(decoded_resources.refvec_, ""); + } + + std::unique_ptr sds_api_; +}; + +INSTANTIATE_TEST_SUITE_P(CertificateValidationContextSdsRotationApiTestParams, + CertificateValidationContextSdsRotationApiTest, + testing::Values(false, true)); + +// Initial onConfigUpdate() of TlsCertificate secret. +TEST_P(TlsCertificateSdsRotationApiTest, InitialUpdate) { + InSequence s; + onConfigUpdate("a", "b"); + + const auto& secret = *sds_api_->secret(); + EXPECT_EQ("a", secret.certificate_chain().inline_bytes()); + EXPECT_EQ("b", secret.private_key().inline_bytes()); +} + +// Two distinct updates with onConfigUpdate() of TlsCertificate secret. +TEST_P(TlsCertificateSdsRotationApiTest, MultiUpdate) { + InSequence s; + onConfigUpdate("a", "b"); + { + const auto& secret = *sds_api_->secret(); + EXPECT_EQ("a", secret.certificate_chain().inline_bytes()); + EXPECT_EQ("b", secret.private_key().inline_bytes()); + } + + cert_path_ = "/new/foo/bar/cert.pem"; + key_path_ = "/new/foo/bar/key.pem"; + expected_watch_path_ = "/new/foo/bar/"; + onConfigUpdate("c", "d"); + { + const auto& secret = *sds_api_->secret(); + EXPECT_EQ("c", secret.certificate_chain().inline_bytes()); + EXPECT_EQ("d", secret.private_key().inline_bytes()); + } +} + +// Watch trigger without file change has no effect. +TEST_P(TlsCertificateSdsRotationApiTest, NopWatchTrigger) { + InSequence s; + onConfigUpdate("a", "b"); + + for (const auto& cb : watch_cbs_) { + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("a")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("b")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("a")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("b")); + cb(Filesystem::Watcher::Events::MovedTo); + } + + const auto& secret = *sds_api_->secret(); + EXPECT_EQ("a", secret.certificate_chain().inline_bytes()); + EXPECT_EQ("b", secret.private_key().inline_bytes()); +} + +// Basic rotation of TlsCertificate. +TEST_P(TlsCertificateSdsRotationApiTest, RotationWatchTrigger) { + InSequence s; + onConfigUpdate("a", "b"); + + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("c")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("c")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(secret_callback_, onAddOrUpdateSecret()); + watch_cbs_[0](Filesystem::Watcher::Events::MovedTo); + if (!watched_directory_) { + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("c")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("c")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + watch_cbs_[1](Filesystem::Watcher::Events::MovedTo); + } + + const auto& secret = *sds_api_->secret(); + EXPECT_EQ("c", secret.certificate_chain().inline_bytes()); + EXPECT_EQ("d", secret.private_key().inline_bytes()); +} + +// Basic rotation of CertificateValidationContext. +TEST_P(CertificateValidationContextSdsRotationApiTest, CertificateValidationContext) { + InSequence s; + onConfigUpdate("/foo/bar/ca.pem", "a", "/foo/bar/"); + + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/ca.pem")).WillOnce(Return("c")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/ca.pem")).WillOnce(Return("c")); + EXPECT_CALL(secret_callback_, onAddOrUpdateSecret()); + watch_cbs_[0](Filesystem::Watcher::Events::MovedTo); + + const auto& secret = *sds_api_->secret(); + EXPECT_EQ("c", secret.trusted_ca().inline_bytes()); +} + +// Hash consistency verification prevents races. +TEST_P(TlsCertificateSdsRotationApiTest, RotationConsistency) { + InSequence s; + onConfigUpdate("a", "b"); + + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("a")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("c")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("c")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(secret_callback_, onAddOrUpdateSecret()); + watch_cbs_[0](Filesystem::Watcher::Events::MovedTo); + if (!watched_directory_) { + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("c")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("c")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + watch_cbs_[1](Filesystem::Watcher::Events::MovedTo); + } + + const auto& secret = *sds_api_->secret(); + EXPECT_EQ("c", secret.certificate_chain().inline_bytes()); + EXPECT_EQ("d", secret.private_key().inline_bytes()); +} + +// Hash consistency verification failure, no callback. +TEST_P(TlsCertificateSdsRotationApiTest, RotationConsistencyExhaustion) { + InSequence s; + onConfigUpdate("a", "b"); + + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("a")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("c")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("e")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("f")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("f")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("g")); + // We've exhaused the bounded retries, but continue with the non-atomic rotation. + EXPECT_CALL(secret_callback_, onAddOrUpdateSecret()); + EXPECT_LOG_CONTAINS( + "warn", "Unable to atomically refresh secrets due to > 5 non-atomic rotations observed", + watch_cbs_[0](Filesystem::Watcher::Events::MovedTo)); + if (!watched_directory_) { + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("f")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("g")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("f")); + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("g")); + watch_cbs_[1](Filesystem::Watcher::Events::MovedTo); + } + + const auto& secret = *sds_api_->secret(); + EXPECT_EQ("f", secret.certificate_chain().inline_bytes()); + EXPECT_EQ("g", secret.private_key().inline_bytes()); +} + class PartialMockSds : public SdsApi { public: PartialMockSds(NiceMock& server, NiceMock& init_manager, diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 928cb440c5f2f..7c00b2e91e147 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -428,6 +428,10 @@ ApiPtr createApiForTest() { Filesystem::fileSystemForTest()); } +ApiPtr createApiForTest(Filesystem::Instance& filesystem) { + return std::make_unique(Thread::threadFactoryForTest(), filesystem); +} + ApiPtr createApiForTest(Random::RandomGenerator& random) { return std::make_unique(Thread::threadFactoryForTest(), Filesystem::fileSystemForTest(), nullptr, nullptr, &random); diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 3dc15c8ccac03..2f3ce54d7c840 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -1041,6 +1041,7 @@ makeHeaderMap(const std::initializer_list>& namespace Api { ApiPtr createApiForTest(); +ApiPtr createApiForTest(Filesystem::Instance& filesystem); ApiPtr createApiForTest(Random::RandomGenerator& random); ApiPtr createApiForTest(Stats::Store& stat_store); ApiPtr createApiForTest(Stats::Store& stat_store, Random::RandomGenerator& random); From 2282367c57b3923e57ec21af19120330e8b550ea Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 11 Nov 2020 20:18:03 -0500 Subject: [PATCH 09/14] Release notes and docs. Signed-off-by: Harvey Tuch --- docs/root/configuration/security/secret.rst | 33 +++++++++++++++++++++ docs/root/version_history/current.rst | 3 ++ 2 files changed, 36 insertions(+) diff --git a/docs/root/configuration/security/secret.rst b/docs/root/configuration/security/secret.rst index 087cf388b759e..6fd47d986ef57 100644 --- a/docs/root/configuration/security/secret.rst +++ b/docs/root/configuration/security/secret.rst @@ -239,6 +239,39 @@ Path to CA certificate bundle for validating the xDS server certificate is given trusted_ca: filename: /certs/cacert.pem +In the above example, a watch will be established on ``/certs``. File movement in this directory +will trigger an update. An alternative common key rotation scheme that provides improved atomicity +is to establish an active symlink ``/certs/current`` and use an atomic move operation to replace the +symlink. The watch in this case needs to be on the certificate's grandparent directory. Envoy +supports this scheme via the use of *watched_path*. Continuing the above examples: + +.. code-block:: yaml + + resources: + - "@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.Secret" + tls_certificate: + certificate_chain: + filename: /certs/current/sds_cert.pem + private_key: + filename: /certs/current/sds_key.pem + watched_directory: + path: /certs + +.. code-block:: yaml + + resources: + - "@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.Secret" + validation_context: + trusted_ca: + filename: /certs/current/cacert.pem + watched_directory: + path: /certs + +Secret rotation can be performed with: + +.. code-block:: bash + + ln -s /certs/new && mv -Tf /certs/new /certs/current Statistics ---------- diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 3a4dd5dffb4c5..5b06a70c96638 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -61,6 +61,9 @@ New Features * overload: add :ref:`envoy.overload_actions.reduce_timeouts ` overload action to enable scaling timeouts down with load. * ratelimit: added support for use of various :ref:`metadata ` as a ratelimit action. * ratelimit: added :ref:`disable_x_envoy_ratelimited_header ` option to disable `X-Envoy-RateLimited` header. +* sds: improved support for atomic :ref:`key rotations ` and added configurable rotation triggers for + :ref:`TlsCertificate ` and + :ref:`CertificateValidationContext `. * tcp: added a new :ref:`envoy.overload_actions.reject_incoming_connections ` action to reject incoming TCP connections. * tls: added support for RSA certificates with 4096-bit keys in FIPS mode. * tracing: added SkyWalking tracer. From f7b7d94573412f54a8e9a6fdf11c529464cdabd8 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 12 Nov 2020 09:20:22 -0500 Subject: [PATCH 10/14] Typo. Signed-off-by: Harvey Tuch --- test/common/secret/sds_api_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index b57def8502f06..927966813f8f6 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -456,7 +456,7 @@ TEST_P(TlsCertificateSdsRotationApiTest, RotationConsistencyExhaustion) { EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("d")); EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")).WillOnce(Return("f")); EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/key.pem")).WillOnce(Return("g")); - // We've exhaused the bounded retries, but continue with the non-atomic rotation. + // We've exhausted the bounded retries, but continue with the non-atomic rotation. EXPECT_CALL(secret_callback_, onAddOrUpdateSecret()); EXPECT_LOG_CONTAINS( "warn", "Unable to atomically refresh secrets due to > 5 non-atomic rotations observed", From da64842f85508205095c855e08627d36e26b1f00 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 12 Nov 2020 13:28:09 -0500 Subject: [PATCH 11/14] Fix clang-tidy. Signed-off-by: Harvey Tuch --- source/common/secret/sds_api.h | 6 +++--- test/common/secret/sds_api_test.cc | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 87916f836adb7..385a4716cb74e 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -406,7 +406,7 @@ class GenericSecretSdsApi : public SdsApi, public GenericSecretConfigProvider { // SecretProvider const envoy::extensions::transport_sockets::tls::v3::GenericSecret* secret() const override { - return generic_secret.get(); + return generic_secret_.get(); } Common::CallbackHandle* addUpdateCallback(std::function callback) override { return update_callback_manager_.add(callback); @@ -419,7 +419,7 @@ class GenericSecretSdsApi : public SdsApi, public GenericSecretConfigProvider { protected: void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) override { - generic_secret = std::make_unique( + generic_secret_ = std::make_unique( secret.generic_secret()); } void @@ -430,7 +430,7 @@ class GenericSecretSdsApi : public SdsApi, public GenericSecretConfigProvider { Config::WatchedDirectory* getWatchedDirectory() override { return nullptr; } private: - GenericSecretPtr generic_secret; + GenericSecretPtr generic_secret_; Common::CallbackManager validation_callback_manager_; }; diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 927966813f8f6..5d5584c059216 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -221,7 +221,7 @@ class TlsCertificateSdsRotationApiTest : public testing::TestWithParam, handle_ = sds_api_->addUpdateCallback([this]() { secret_callback_.onAddOrUpdateSecret(); }); } - ~TlsCertificateSdsRotationApiTest() { handle_->remove(); } + ~TlsCertificateSdsRotationApiTest() override { handle_->remove(); } void onConfigUpdate(const std::string& cert_value, const std::string& key_value) { const std::string yaml = fmt::format( @@ -291,7 +291,7 @@ class CertificateValidationContextSdsRotationApiTest : public testing::TestWithP handle_ = sds_api_->addUpdateCallback([this]() { secret_callback_.onAddOrUpdateSecret(); }); } - ~CertificateValidationContextSdsRotationApiTest() { handle_->remove(); } + ~CertificateValidationContextSdsRotationApiTest() override { handle_->remove(); } void onConfigUpdate(const std::string& trusted_ca_path, const std::string& trusted_ca_value, const std::string& watch_path) { From 2dae3bea9130cd5eaa04ec69a60d39128b5ba41f Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 12 Nov 2020 14:47:48 -0500 Subject: [PATCH 12/14] fails_on_windows tag added (after consulting @wrowe) Signed-off-by: Harvey Tuch --- source/common/secret/sds_api.h | 5 +++-- test/integration/BUILD | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index 385a4716cb74e..d7a3293a31cbc 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -419,8 +419,9 @@ class GenericSecretSdsApi : public SdsApi, public GenericSecretConfigProvider { protected: void setSecret(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) override { - generic_secret_ = std::make_unique( - secret.generic_secret()); + generic_secret_ = + std::make_unique( + secret.generic_secret()); } void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) override { diff --git a/test/integration/BUILD b/test/integration/BUILD index aa0cd5a566c46..43dfa373074d2 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1142,6 +1142,10 @@ envoy_cc_test( "sds_dynamic_key_rotation_setup.sh", "//test/config/integration/certs", ], + # TODO(envoyproxy/windows-dev): The key rotation in SdsDynamicKeyRotationIntegrationTest via + # TestEnvironment::renameFile() fails on Windows. The renameFile() implementation does not + # correctly handle symlinks. + tags = ["fails_on_windows"], deps = [ ":http_integration_lib", "//source/common/config:api_version_lib", From 6b5f720841c89133a6973d084b69180b825558c6 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 12 Nov 2020 17:10:06 -0500 Subject: [PATCH 13/14] Strengthen docs. Signed-off-by: Harvey Tuch --- .../transport_sockets/tls/v3/common.proto | 16 +++++++++---- .../tls/v4alpha/common.proto | 16 +++++++++---- docs/root/configuration/security/secret.rst | 24 ++++++++++++++++++- .../transport_sockets/tls/v3/common.proto | 16 +++++++++---- .../tls/v4alpha/common.proto | 16 +++++++++---- 5 files changed, 71 insertions(+), 17 deletions(-) diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index 31ef951e6707c..587e3271836b1 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -149,9 +149,13 @@ message TlsCertificate { // If specified, updates of file-based *certificate_chain* and *private_key* // sources will be triggered by this watch. The certificate/key pair will be - // read together and validated for atomic read consistency (i.e. no intervening - // modification occurred between cert/key read, verified by file hash - // comparisons). + // read together and validated for atomic read consistency (i.e. no + // intervening modification occurred between cert/key read, verified by file + // hash comparisons). This allows explicit control over the path watched, by + // default the parent directories of the filesystem paths in + // *certificate_chain* and *private_key* are watched if this field is not + // specified. This only applies when a *TlsCertificate* is delivered by SDS + // with references to filesystem paths. config.core.v3.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key @@ -257,7 +261,11 @@ message CertificateValidationContext { config.core.v3.DataSource trusted_ca = 1; // If specified, updates of a file-based *trusted_ca* source will be triggered - // by this watch. + // by this watch. This allows explicit control over the path watched, by + // default the parent directory of the filesystem path in *trusted_ca* is + // watched if this field is not specified. This only applies when a + // *CertificateValidationContext* is delivered by SDS with references to + // filesystem paths. config.core.v3.WatchedDirectory watched_directory = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the diff --git a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto index 1c34fc92ca1c0..b2fa6f672628c 100644 --- a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -151,9 +151,13 @@ message TlsCertificate { // If specified, updates of file-based *certificate_chain* and *private_key* // sources will be triggered by this watch. The certificate/key pair will be - // read together and validated for atomic read consistency (i.e. no intervening - // modification occurred between cert/key read, verified by file hash - // comparisons). + // read together and validated for atomic read consistency (i.e. no + // intervening modification occurred between cert/key read, verified by file + // hash comparisons). This allows explicit control over the path watched, by + // default the parent directories of the filesystem paths in + // *certificate_chain* and *private_key* are watched if this field is not + // specified. This only applies when a *TlsCertificate* is delivered by SDS + // with references to filesystem paths. config.core.v4alpha.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key @@ -259,7 +263,11 @@ message CertificateValidationContext { config.core.v4alpha.DataSource trusted_ca = 1; // If specified, updates of a file-based *trusted_ca* source will be triggered - // by this watch. + // by this watch. This allows explicit control over the path watched, by + // default the parent directory of the filesystem path in *trusted_ca* is + // watched if this field is not specified. This only applies when a + // *CertificateValidationContext* is delivered by SDS with references to + // filesystem paths. config.core.v4alpha.WatchedDirectory watched_directory = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the diff --git a/docs/root/configuration/security/secret.rst b/docs/root/configuration/security/secret.rst index 6fd47d986ef57..082a7b89c236b 100644 --- a/docs/root/configuration/security/secret.rst +++ b/docs/root/configuration/security/secret.rst @@ -33,6 +33,26 @@ SDS Configuration *SdsSecretConfig* is used in two fields in :ref:`CommonTlsContext `. The first field is *tls_certificate_sds_secret_configs* to use SDS to get :ref:`TlsCertificate `. The second field is *validation_context_sds_secret_config* to use SDS to get :ref:`CertificateValidationContext `. +.. _sds_key_rotation: + +Key rotation +------------ + +It's usually preferrable to perform key rotation via gRPC SDS, but when this is not possible or +desired (e.g. during bootstrap of SDS credentials), SDS allows for filesystem rotation when secrets +refer to filesystem paths. This currently is supported for the following secret types: + +* :ref:`TlsCertificate ` +* :ref:`CertificateValidationContext ` + +By default, directories containing secrets are watched for filesystem move events. Explicit control over +the watched directory is possible by specifying a *watched_directory* path in :ref:`TlsCertificate +` and +:ref:`CertificateValidationContext +`. + +An example of key rotation is provided :ref:`below `. + Example one: static_resource ----------------------------- @@ -211,9 +231,11 @@ In contrast, :ref:`sds_server_example` requires a restart to reload xDS certific "@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext" common_tls_context: tls_certificate_sds_secret_configs: + name: tls_sds sds_config: path: /etc/envoy/tls_certificate_sds_secret.yaml validation_context_sds_secret_config: + name: validation_context_sds sds_config: path: /etc/envoy/validation_context_sds_secret.yaml @@ -243,7 +265,7 @@ In the above example, a watch will be established on ``/certs``. File movement i will trigger an update. An alternative common key rotation scheme that provides improved atomicity is to establish an active symlink ``/certs/current`` and use an atomic move operation to replace the symlink. The watch in this case needs to be on the certificate's grandparent directory. Envoy -supports this scheme via the use of *watched_path*. Continuing the above examples: +supports this scheme via the use of *watched_directory*. Continuing the above examples: .. code-block:: yaml diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto index f41892d21c4b0..c5452fced643d 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -148,9 +148,13 @@ message TlsCertificate { // If specified, updates of file-based *certificate_chain* and *private_key* // sources will be triggered by this watch. The certificate/key pair will be - // read together and validated for atomic read consistency (i.e. no intervening - // modification occurred between cert/key read, verified by file hash - // comparisons). + // read together and validated for atomic read consistency (i.e. no + // intervening modification occurred between cert/key read, verified by file + // hash comparisons). This allows explicit control over the path watched, by + // default the parent directories of the filesystem paths in + // *certificate_chain* and *private_key* are watched if this field is not + // specified. This only applies when a *TlsCertificate* is delivered by SDS + // with references to filesystem paths. config.core.v3.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key @@ -254,7 +258,11 @@ message CertificateValidationContext { config.core.v3.DataSource trusted_ca = 1; // If specified, updates of a file-based *trusted_ca* source will be triggered - // by this watch. + // by this watch. This allows explicit control over the path watched, by + // default the parent directory of the filesystem path in *trusted_ca* is + // watched if this field is not specified. This only applies when a + // *CertificateValidationContext* is delivered by SDS with references to + // filesystem paths. config.core.v3.WatchedDirectory watched_directory = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the diff --git a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto index 1c34fc92ca1c0..b2fa6f672628c 100644 --- a/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/generated_api_shadow/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -151,9 +151,13 @@ message TlsCertificate { // If specified, updates of file-based *certificate_chain* and *private_key* // sources will be triggered by this watch. The certificate/key pair will be - // read together and validated for atomic read consistency (i.e. no intervening - // modification occurred between cert/key read, verified by file hash - // comparisons). + // read together and validated for atomic read consistency (i.e. no + // intervening modification occurred between cert/key read, verified by file + // hash comparisons). This allows explicit control over the path watched, by + // default the parent directories of the filesystem paths in + // *certificate_chain* and *private_key* are watched if this field is not + // specified. This only applies when a *TlsCertificate* is delivered by SDS + // with references to filesystem paths. config.core.v4alpha.WatchedDirectory watched_directory = 7; // BoringSSL private key method provider. This is an alternative to :ref:`private_key @@ -259,7 +263,11 @@ message CertificateValidationContext { config.core.v4alpha.DataSource trusted_ca = 1; // If specified, updates of a file-based *trusted_ca* source will be triggered - // by this watch. + // by this watch. This allows explicit control over the path watched, by + // default the parent directory of the filesystem path in *trusted_ca* is + // watched if this field is not specified. This only applies when a + // *CertificateValidationContext* is delivered by SDS with references to + // filesystem paths. config.core.v4alpha.WatchedDirectory watched_directory = 11; // An optional list of base64-encoded SHA-256 hashes. If specified, Envoy will verify that the From 4ef3f139337d85ee7aadc315d87f1d8916477d87 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 12 Nov 2020 18:18:33 -0500 Subject: [PATCH 14/14] Fix SDS stats and avoid crashing on empty directory rotations. Signed-off-by: Harvey Tuch --- docs/root/configuration/security/secret.rst | 9 ++ source/common/secret/sds_api.cc | 61 +++++++----- source/common/secret/sds_api.h | 16 +++- test/common/secret/BUILD | 4 +- test/common/secret/sds_api_test.cc | 94 ++++++++++--------- .../sds_dynamic_integration_test.cc | 84 +++++++++++++++++ .../sds_dynamic_key_rotation_setup.sh | 3 + 7 files changed, 202 insertions(+), 69 deletions(-) diff --git a/docs/root/configuration/security/secret.rst b/docs/root/configuration/security/secret.rst index 082a7b89c236b..fdbc88242d021 100644 --- a/docs/root/configuration/security/secret.rst +++ b/docs/root/configuration/security/secret.rst @@ -316,3 +316,12 @@ For upstream clusters, they are in the *cluster..client_ssl_socket ssl_context_update_by_sds, Total number of ssl context has been updated. upstream_context_secrets_not_ready, Total number of upstream connections reset due to empty ssl certificate. + +SDS has a :ref:`statistics ` tree rooted in the *sds..* +namespace. In addition, the following statistics are tracked in this namespace: + +.. csv-table:: + :header: Name, Description + :widths: 1, 2 + + key_rotation_failed, Total number of filesystem key rotations that failed outside of an SDS update. diff --git a/source/common/secret/sds_api.cc b/source/common/secret/sds_api.cc index fe71bf64f1310..3f319f03e638b 100644 --- a/source/common/secret/sds_api.cc +++ b/source/common/secret/sds_api.cc @@ -12,6 +12,10 @@ namespace Envoy { namespace Secret { +SdsApiStats SdsApi::generateStats(Stats::Scope& scope) { + return {ALL_SDS_API_STATS(POOL_COUNTER(scope))}; +} + SdsApi::SdsApi(envoy::config::core::v3::ConfigSource sds_config, absl::string_view sds_config_name, Config::SubscriptionFactory& subscription_factory, TimeSource& time_source, ProtobufMessage::ValidationVisitor& validation_visitor, Stats::Store& stats, @@ -19,7 +23,9 @@ SdsApi::SdsApi(envoy::config::core::v3::ConfigSource sds_config, absl::string_vi : Envoy::Config::SubscriptionBase( sds_config.resource_api_version(), validation_visitor, "name"), init_target_(fmt::format("SdsApi {}", sds_config_name), [this] { initialize(); }), - dispatcher_(dispatcher), api_(api), stats_(stats), sds_config_(std::move(sds_config)), + dispatcher_(dispatcher), api_(api), + scope_(stats.createScope(absl::StrCat("sds.", sds_config_name, "."))), + sds_api_stats_(generateStats(*scope_)), sds_config_(std::move(sds_config)), sds_config_name_(sds_config_name), secret_hash_(0), clean_up_(std::move(destructor_cb)), subscription_factory_(subscription_factory), time_source_(time_source), secret_data_{sds_config_name_, "uninitialized", @@ -27,7 +33,7 @@ SdsApi::SdsApi(envoy::config::core::v3::ConfigSource sds_config, absl::string_vi const auto resource_name = getResourceName(); // This has to happen here (rather than in initialize()) as it can throw exceptions. subscription_ = subscription_factory_.subscriptionFromConfigSource( - sds_config_, Grpc::Common::typeUrl(resource_name), stats_, *this, resource_decoder_); + sds_config_, Grpc::Common::typeUrl(resource_name), *scope_, *this, resource_decoder_); // 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 @@ -45,28 +51,35 @@ void SdsApi::resolveDataSource(const FileContentMap& files, } void SdsApi::onWatchUpdate() { - // Obtain a stable set of files. If a rotation happens while we're reading, - // then we need to try again. - uint64_t prev_hash = 0; - FileContentMap files = loadFiles(); - uint64_t next_hash = getHashForFiles(files); - const uint64_t MaxBoundedRetries = 5; - for (uint64_t bounded_retries = MaxBoundedRetries; next_hash != prev_hash && bounded_retries > 0; - --bounded_retries) { - files = loadFiles(); - prev_hash = next_hash; - next_hash = getHashForFiles(files); - } - if (next_hash != prev_hash) { - ENVOY_LOG_MISC(warn, - "Unable to atomically refresh secrets due to > {} non-atomic rotations observed", - MaxBoundedRetries); - } - const uint64_t new_hash = next_hash; - if (new_hash != files_hash_) { - resolveSecret(files); - update_callback_manager_.runCallbacks(); - files_hash_ = new_hash; + // Filesystem reads and update callbacks can fail if the key material is missing or bad. We're not + // under an onConfigUpdate() context, so we need to catch these cases explicitly here. + try { + // Obtain a stable set of files. If a rotation happens while we're reading, + // then we need to try again. + uint64_t prev_hash = 0; + FileContentMap files = loadFiles(); + uint64_t next_hash = getHashForFiles(files); + const uint64_t MaxBoundedRetries = 5; + for (uint64_t bounded_retries = MaxBoundedRetries; + next_hash != prev_hash && bounded_retries > 0; --bounded_retries) { + files = loadFiles(); + prev_hash = next_hash; + next_hash = getHashForFiles(files); + } + if (next_hash != prev_hash) { + ENVOY_LOG_MISC( + warn, "Unable to atomically refresh secrets due to > {} non-atomic rotations observed", + MaxBoundedRetries); + } + const uint64_t new_hash = next_hash; + if (new_hash != files_hash_) { + resolveSecret(files); + update_callback_manager_.runCallbacks(); + files_hash_ = new_hash; + } + } catch (const EnvoyException& e) { + ENVOY_LOG_MISC(warn, fmt::format("Failed to reload certificates: {}", e.what())); + sds_api_stats_.key_rotation_failed_.inc(); } } diff --git a/source/common/secret/sds_api.h b/source/common/secret/sds_api.h index d7a3293a31cbc..1c58feb6c5bc3 100644 --- a/source/common/secret/sds_api.h +++ b/source/common/secret/sds_api.h @@ -31,6 +31,18 @@ namespace Envoy { namespace Secret { +/** + * All SDS API. @see stats_macros.h + */ +#define ALL_SDS_API_STATS(COUNTER) COUNTER(key_rotation_failed) + +/** + * Struct definition for all SDS API stats. @see stats_macros.h + */ +struct SdsApiStats { + ALL_SDS_API_STATS(GENERATE_COUNTER_STRUCT) +}; + /** * SDS API implementation that fetches secrets from SDS server via Subscription. */ @@ -96,8 +108,10 @@ class SdsApi : public Envoy::Config::SubscriptionBase< uint64_t getHashForFiles(const FileContentMap& files); // Invoked for filesystem watches on update. void onWatchUpdate(); + SdsApiStats generateStats(Stats::Scope& scope); - Stats::Store& stats_; + Stats::ScopePtr scope_; + SdsApiStats sds_api_stats_; const envoy::config::core::v3::ConfigSource sds_config_; Config::SubscriptionPtr subscription_; diff --git a/test/common/secret/BUILD b/test/common/secret/BUILD index c341f6394c2a9..6161773b7b8c2 100644 --- a/test/common/secret/BUILD +++ b/test/common/secret/BUILD @@ -43,11 +43,13 @@ envoy_cc_test( "//source/common/secret:sds_api_lib", "//source/common/ssl:certificate_validation_context_config_impl_lib", "//source/common/ssl:tls_certificate_config_impl_lib", + "//test/common/stats:stat_test_utility_lib", + "//test/mocks/config:config_mocks", + "//test/mocks/filesystem:filesystem_mocks", "//test/mocks/grpc:grpc_mocks", "//test/mocks/init:init_mocks", "//test/mocks/protobuf:protobuf_mocks", "//test/mocks/secret:secret_mocks", - "//test/mocks/server:instance_mocks", "//test/test_common:environment_lib", "//test/test_common:logging_lib", "//test/test_common:registry_lib", diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index 5d5584c059216..12386bbf535e6 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -12,11 +12,13 @@ #include "common/ssl/certificate_validation_context_config_impl.h" #include "common/ssl/tls_certificate_config_impl.h" +#include "test/common/stats/stat_test_utility.h" +#include "test/mocks/config/mocks.h" +#include "test/mocks/filesystem/mocks.h" #include "test/mocks/grpc/mocks.h" #include "test/mocks/init/mocks.h" #include "test/mocks/protobuf/mocks.h" #include "test/mocks/secret/mocks.h" -#include "test/mocks/server/instance.h" #include "test/test_common/environment.h" #include "test/test_common/logging.h" #include "test/test_common/utility.h" @@ -28,7 +30,9 @@ using ::testing::_; using ::testing::InSequence; using ::testing::Invoke; using ::testing::InvokeWithoutArgs; +using ::testing::NiceMock; using ::testing::Return; +using ::testing::Throw; namespace Envoy { namespace Secret { @@ -56,6 +60,7 @@ class SdsApiTestBase { Event::GlobalTimeSystem time_system_; Init::TargetHandlePtr init_target_handle_; Event::DispatcherPtr dispatcher_; + Stats::TestUtil::TestStore stats_; }; class SdsApiTest : public testing::Test, public SdsApiTestBase {}; @@ -64,13 +69,12 @@ class SdsApiTest : public testing::Test, public SdsApiTestBase {}; TEST_F(SdsApiTest, BasicTest) { ::testing::InSequence s; const envoy::service::secret::v3::SdsDummy dummy; - NiceMock server; envoy::config::core::v3::ConfigSource config_source; setupMocks(); TlsCertificateSdsApi sds_api( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server.stats(), []() {}, *dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, *dispatcher_, *api_); sds_api.registerInitTarget(init_manager_); initialize(); } @@ -79,7 +83,6 @@ TEST_F(SdsApiTest, BasicTest) { // has been already initialized. This is a regression test for // https://github.com/envoyproxy/envoy/issues/12013 TEST_F(SdsApiTest, InitManagerInitialised) { - NiceMock server; std::string sds_config = R"EOF( resources: @@ -97,7 +100,7 @@ TEST_F(SdsApiTest, InitManagerInitialised) { NiceMock callbacks; TestUtility::TestOpaqueResourceDecoderImpl resource_decoder("name"); - Config::SubscriptionStats stats(Config::Utility::generateStats(server.stats())); + Config::SubscriptionStats stats(Config::Utility::generateStats(stats_)); NiceMock validation_visitor; envoy::config::core::v3::ConfigSource config_source; @@ -120,8 +123,8 @@ TEST_F(SdsApiTest, InitManagerInitialised) { EXPECT_EQ(Init::Manager::State::Initializing, init_manager.state()); TlsCertificateSdsApi sds_api( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server.stats(), []() {}, *dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, *dispatcher_, *api_); EXPECT_NO_THROW(sds_api.registerInitTarget(init_manager)); } @@ -129,7 +132,6 @@ TEST_F(SdsApiTest, InitManagerInitialised) { // https://github.com/envoyproxy/envoy/issues/10976. TEST_F(SdsApiTest, BadConfigSource) { ::testing::InSequence s; - NiceMock server; envoy::config::core::v3::ConfigSource config_source; EXPECT_CALL(subscription_factory_, subscriptionFromConfigSource(_, _, _, _, _)) .WillOnce(InvokeWithoutArgs([]() -> Config::SubscriptionPtr { @@ -138,19 +140,18 @@ TEST_F(SdsApiTest, BadConfigSource) { })); EXPECT_THROW_WITH_MESSAGE(TlsCertificateSdsApi( config_source, "abc.com", subscription_factory_, time_system_, - validation_visitor_, server.stats(), []() {}, *dispatcher_, *api_), + validation_visitor_, stats_, []() {}, *dispatcher_, *api_), EnvoyException, "bad config"); } // Validate that TlsCertificateSdsApi updates secrets successfully if a good secret // is passed to onConfigUpdate(). TEST_F(SdsApiTest, DynamicTlsCertificateUpdateSuccess) { - NiceMock server; envoy::config::core::v3::ConfigSource config_source; setupMocks(); TlsCertificateSdsApi sds_api( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server.stats(), []() {}, *dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, *dispatcher_, *api_); sds_api.registerInitTarget(init_manager_); initialize(); NiceMock secret_callback; @@ -203,7 +204,6 @@ class SdsRotationApiTest : public SdsApiTestBase { std::vector watch_cbs_; Event::MockDispatcher mock_dispatcher_; Filesystem::MockInstance filesystem_; - NiceMock server_; }; class TlsCertificateSdsRotationApiTest : public testing::TestWithParam, @@ -214,8 +214,8 @@ class TlsCertificateSdsRotationApiTest : public testing::TestWithParam, key_path_("/foo/bar/key.pem"), expected_watch_path_("/foo/bar/"), trigger_path_("/foo") { envoy::config::core::v3::ConfigSource config_source; sds_api_ = std::make_unique( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server_.stats(), []() {}, mock_dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, mock_dispatcher_, *api_); sds_api_->registerInitTarget(init_manager_); initialize(); handle_ = sds_api_->addUpdateCallback([this]() { secret_callback_.onAddOrUpdateSecret(); }); @@ -284,8 +284,8 @@ class CertificateValidationContextSdsRotationApiTest : public testing::TestWithP CertificateValidationContextSdsRotationApiTest() { envoy::config::core::v3::ConfigSource config_source; sds_api_ = std::make_unique( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server_.stats(), []() {}, mock_dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, mock_dispatcher_, *api_); sds_api_->registerInitTarget(init_manager_); initialize(); handle_ = sds_api_->addUpdateCallback([this]() { secret_callback_.onAddOrUpdateSecret(); }); @@ -399,6 +399,22 @@ TEST_P(TlsCertificateSdsRotationApiTest, RotationWatchTrigger) { EXPECT_EQ("d", secret.private_key().inline_bytes()); } +// Failed rotation of TlsCertificate. +TEST_P(TlsCertificateSdsRotationApiTest, FailedRotation) { + InSequence s; + onConfigUpdate("a", "b"); + + EXPECT_CALL(filesystem_, fileReadToEnd("/foo/bar/cert.pem")) + .WillOnce(Throw(EnvoyException("fail"))); + EXPECT_LOG_CONTAINS("warn", "Failed to reload certificates: ", + watch_cbs_[0](Filesystem::Watcher::Events::MovedTo)); + EXPECT_EQ(1U, stats_.counter("sds.abc.com.key_rotation_failed").value()); + + const auto& secret = *sds_api_->secret(); + EXPECT_EQ("a", secret.certificate_chain().inline_bytes()); + EXPECT_EQ("b", secret.private_key().inline_bytes()); +} + // Basic rotation of CertificateValidationContext. TEST_P(CertificateValidationContextSdsRotationApiTest, CertificateValidationContext) { InSequence s; @@ -476,13 +492,13 @@ TEST_P(TlsCertificateSdsRotationApiTest, RotationConsistencyExhaustion) { class PartialMockSds : public SdsApi { public: - PartialMockSds(NiceMock& server, NiceMock& init_manager, + PartialMockSds(Stats::Store& stats, NiceMock& init_manager, envoy::config::core::v3::ConfigSource& config_source, Config::SubscriptionFactory& subscription_factory, TimeSource& time_source, Event::Dispatcher& dispatcher, Api::Api& api) : SdsApi( - config_source, "abc.com", subscription_factory, time_source, validation_visitor_, - server.stats(), []() {}, dispatcher, api) { + config_source, "abc.com", subscription_factory, time_source, validation_visitor_, stats, + []() {}, dispatcher, api) { registerInitTarget(init_manager); } @@ -509,11 +525,10 @@ TEST_F(SdsApiTest, Delta) { Config::DecodedResourceImpl resource(std::move(secret), "name", {}, "version1"); std::vector resources{resource}; - NiceMock server; envoy::config::core::v3::ConfigSource config_source; Event::GlobalTimeSystem time_system; setupMocks(); - PartialMockSds sds(server, init_manager_, config_source, subscription_factory_, time_system, + PartialMockSds sds(stats_, init_manager_, config_source, subscription_factory_, time_system, *dispatcher_, *api_); initialize(); EXPECT_CALL(sds, onConfigUpdate(DecodedResourcesEq(resources), "version1")); @@ -533,12 +548,11 @@ TEST_F(SdsApiTest, Delta) { // Tests SDS's use of the delta variant of onConfigUpdate(). TEST_F(SdsApiTest, DeltaUpdateSuccess) { - NiceMock server; envoy::config::core::v3::ConfigSource config_source; setupMocks(); TlsCertificateSdsApi sds_api( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server.stats(), []() {}, *dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, *dispatcher_, *api_); sds_api.registerInitTarget(init_manager_); NiceMock secret_callback; @@ -579,12 +593,11 @@ TEST_F(SdsApiTest, DeltaUpdateSuccess) { // Validate that CertificateValidationContextSdsApi updates secrets successfully if // a good secret is passed to onConfigUpdate(). TEST_F(SdsApiTest, DynamicCertificateValidationContextUpdateSuccess) { - NiceMock server; envoy::config::core::v3::ConfigSource config_source; setupMocks(); CertificateValidationContextSdsApi sds_api( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server.stats(), []() {}, *dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, *dispatcher_, *api_); sds_api.registerInitTarget(init_manager_); NiceMock secret_callback; @@ -634,12 +647,11 @@ class MockCvcValidationCallback : public CvcValidationCallback { // a good secret is passed to onConfigUpdate(), and that merged CertificateValidationContext // provides correct information. TEST_F(SdsApiTest, DefaultCertificateValidationContextTest) { - NiceMock server; envoy::config::core::v3::ConfigSource config_source; setupMocks(); CertificateValidationContextSdsApi sds_api( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server.stats(), []() {}, *dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, *dispatcher_, *api_); sds_api.registerInitTarget(init_manager_); NiceMock secret_callback; @@ -723,12 +735,11 @@ class MockGenericSecretValidationCallback : public GenericSecretValidationCallba // Validate that GenericSecretSdsApi updates secrets successfully if // a good secret is passed to onConfigUpdate(). TEST_F(SdsApiTest, GenericSecretSdsApiTest) { - NiceMock server; envoy::config::core::v3::ConfigSource config_source; setupMocks(); GenericSecretSdsApi sds_api( config_source, "encryption_key", subscription_factory_, time_system_, validation_visitor_, - server.stats(), []() {}, *dispatcher_, *api_); + stats_, []() {}, *dispatcher_, *api_); sds_api.registerInitTarget(init_manager_); NiceMock secret_callback; @@ -769,12 +780,11 @@ name: "encryption_key" // Validate that SdsApi throws exception if an empty secret is passed to onConfigUpdate(). TEST_F(SdsApiTest, EmptyResource) { - NiceMock server; envoy::config::core::v3::ConfigSource config_source; setupMocks(); TlsCertificateSdsApi sds_api( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server.stats(), []() {}, *dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, *dispatcher_, *api_); sds_api.registerInitTarget(init_manager_); initialize(); @@ -785,12 +795,11 @@ TEST_F(SdsApiTest, EmptyResource) { // Validate that SdsApi throws exception if multiple secrets are passed to onConfigUpdate(). TEST_F(SdsApiTest, SecretUpdateWrongSize) { - NiceMock server; envoy::config::core::v3::ConfigSource config_source; setupMocks(); TlsCertificateSdsApi sds_api( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server.stats(), []() {}, *dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, *dispatcher_, *api_); sds_api.registerInitTarget(init_manager_); std::string yaml = @@ -816,12 +825,11 @@ TEST_F(SdsApiTest, SecretUpdateWrongSize) { // Validate that SdsApi throws exception if secret name passed to onConfigUpdate() // does not match configured name. TEST_F(SdsApiTest, SecretUpdateWrongSecretName) { - NiceMock server; envoy::config::core::v3::ConfigSource config_source; setupMocks(); TlsCertificateSdsApi sds_api( - config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, - server.stats(), []() {}, *dispatcher_, *api_); + config_source, "abc.com", subscription_factory_, time_system_, validation_visitor_, stats_, + []() {}, *dispatcher_, *api_); sds_api.registerInitTarget(init_manager_); std::string yaml = diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index 7c83cd65fdf10..f686a0946fb43 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -225,6 +225,7 @@ INSTANTIATE_TEST_SUITE_P( testing::Combine(testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), testing::Values(Grpc::ClientType::EnvoyGrpc))); +// Validate that a basic key-cert rotation works via symlink rename. TEST_P(SdsDynamicKeyRotationIntegrationTest, BasicRotation) { v3_resource_api_ = true; TestEnvironment::exec( @@ -251,10 +252,53 @@ TEST_P(SdsDynamicKeyRotationIntegrationTest, BasicRotation) { TestEnvironment::temporaryPath("root/current")); test_server_->waitForCounterGe( listenerStatPrefix("server_ssl_socket_factory.ssl_context_update_by_sds"), 2); + // The rotation is not a SDS attempt, so no change to these stats. + EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.server_cert.update_rejected")->value()); + // First request with server_ecdsa{cert,key}.pem. testRouterHeaderOnlyRequestAndResponse(&creator); } +// Validate that rotating to a directory with missing certs is handled. +TEST_P(SdsDynamicKeyRotationIntegrationTest, EmptyRotation) { + v3_resource_api_ = true; + TestEnvironment::exec( + {TestEnvironment::runfilesPath("test/integration/sds_dynamic_key_rotation_setup.sh")}); + + on_server_init_function_ = [this]() { + createSdsStream(*(fake_upstreams_[1])); + sendSdsResponse(getCurrentServerSecret()); + }; + initialize(); + + // Initial update from filesystem. + test_server_->waitForCounterGe( + listenerStatPrefix("server_ssl_socket_factory.ssl_context_update_by_sds"), 1); + + ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { + return makeSslClientConnection(); + }; + // First request with server{cert,key}.pem. + testRouterHeaderOnlyRequestAndResponse(&creator); + cleanupUpstreamAndDownstream(); + + // Rotate to an empty directory, this should fail. + TestEnvironment::renameFile(TestEnvironment::temporaryPath("root/empty"), + TestEnvironment::temporaryPath("root/current")); + test_server_->waitForCounterEq("sds.server_cert.key_rotation_failed", 1); + EXPECT_EQ(1, + test_server_ + ->counter(listenerStatPrefix("server_ssl_socket_factory.ssl_context_update_by_sds")) + ->value()); + // The rotation is not a SDS attempt, so no change to these stats. + EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.server_cert.update_rejected")->value()); + + // Requests continue to work with key/cert pair. + testRouterHeaderOnlyRequestAndResponse(&creator); +} + // A test that SDS server send a good server secret for a static listener. // The first ssl request should be OK. TEST_P(SdsDynamicDownstreamIntegrationTest, BasicSuccess) { @@ -268,6 +312,10 @@ TEST_P(SdsDynamicDownstreamIntegrationTest, BasicSuccess) { return makeSslClientConnection(); }; testRouterHeaderOnlyRequestAndResponse(&creator); + + // Success + EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.server_cert.update_rejected")->value()); } // A test that SDS server send a bad secret for a static listener, @@ -285,6 +333,10 @@ TEST_P(SdsDynamicDownstreamIntegrationTest, WrongSecretFirst) { EXPECT_FALSE(codec_client_->connected()); codec_client_->connection()->close(Network::ConnectionCloseType::NoFlush); + // Failure + EXPECT_EQ(0, test_server_->counter("sds.server_cert.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_rejected")->value()); + sendSdsResponse(getServerSecret()); // Wait for ssl_context_updated_by_sds counter. @@ -295,6 +347,10 @@ TEST_P(SdsDynamicDownstreamIntegrationTest, WrongSecretFirst) { return makeSslClientConnection(); }; testRouterHeaderOnlyRequestAndResponse(&creator); + + // Success + EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("sds.server_cert.update_rejected")->value()); } class SdsDynamicDownstreamCertValidationContextTest : public SdsDynamicDownstreamIntegrationTest { @@ -423,6 +479,10 @@ TEST_P(SdsDynamicDownstreamCertValidationContextTest, BasicSuccess) { return makeSslClientConnection(); }; testRouterHeaderOnlyRequestAndResponse(&creator); + + // Success + EXPECT_EQ(1, test_server_->counter("sds.validation_secret.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.validation_secret.update_rejected")->value()); } // A test that SDS server sends a certificate validation context for a static listener. @@ -440,6 +500,10 @@ TEST_P(SdsDynamicDownstreamCertValidationContextTest, CombinedCertValidationCont return makeSslClientConnection(); }; testRouterHeaderOnlyRequestAndResponse(&creator); + + // Success + EXPECT_EQ(1, test_server_->counter("sds.validation_secret.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.validation_secret.update_rejected")->value()); } // A test that verifies that both: static cluster and LDS listener are updated when using @@ -463,6 +527,10 @@ TEST_P(SdsDynamicDownstreamCertValidationContextTest, BasicWithSharedSecret) { return makeSslClientConnection(); }; testRouterHeaderOnlyRequestAndResponse(&creator); + + // Success + EXPECT_EQ(1, test_server_->counter("sds.validation_secret.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.validation_secret.update_rejected")->value()); } // A test that verifies that both: static cluster and LDS listener are updated when using @@ -487,6 +555,10 @@ TEST_P(SdsDynamicDownstreamCertValidationContextTest, CombinedValidationContextW return makeSslClientConnection(); }; testRouterHeaderOnlyRequestAndResponse(&creator); + + // Success + EXPECT_EQ(1, test_server_->counter("sds.validation_secret.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.validation_secret.update_rejected")->value()); } // Upstream SDS integration test: a static cluster has ssl cert from SDS. @@ -558,6 +630,10 @@ TEST_P(SdsDynamicUpstreamIntegrationTest, BasicSuccess) { "cluster.cluster_0.client_ssl_socket_factory.ssl_context_update_by_sds", 1); testRouterHeaderOnlyRequestAndResponse(); + + // Success + EXPECT_EQ(1, test_server_->counter("sds.client_cert.update_success")->value()); + EXPECT_EQ(0, test_server_->counter("sds.client_cert.update_rejected")->value()); } // To test a static cluster with sds. SDS send a bad client secret first. @@ -581,11 +657,19 @@ TEST_P(SdsDynamicUpstreamIntegrationTest, WrongSecretFirst) { ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection)); ASSERT_TRUE(fake_upstream_connection->waitForDisconnect()); + // Failure + EXPECT_EQ(0, test_server_->counter("sds.client_cert.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("sds.client_cert.update_rejected")->value()); + sendSdsResponse(getClientSecret()); test_server_->waitForCounterGe( "cluster.cluster_0.client_ssl_socket_factory.ssl_context_update_by_sds", 1); testRouterHeaderOnlyRequestAndResponse(); + + // Success + EXPECT_EQ(1, test_server_->counter("sds.client_cert.update_success")->value()); + EXPECT_EQ(1, test_server_->counter("sds.client_cert.update_rejected")->value()); } } // namespace Ssl diff --git a/test/integration/sds_dynamic_key_rotation_setup.sh b/test/integration/sds_dynamic_key_rotation_setup.sh index 89132a52f4c39..2e5f30d88751d 100755 --- a/test/integration/sds_dynamic_key_rotation_setup.sh +++ b/test/integration/sds_dynamic_key_rotation_setup.sh @@ -7,10 +7,12 @@ TEST_CERTS=test/config/integration/certs ROOT="${TEST_TMPDIR}"/root SERVER_KEYCERT="${ROOT}"/server SERVER_ECDSA_KEYCERT="${ROOT}"/server_ecdsa +EMPTY_KEYCERT="${ROOT}"/empty_keycert rm -rf "${ROOT}" mkdir -p "${SERVER_KEYCERT}" mkdir -p "${SERVER_ECDSA_KEYCERT}" +mkdir -p "${EMPTY_KEYCERT}" cp -f "${TEST_CERTS}"/server{cert,key}.pem "${SERVER_KEYCERT}" cp -f "${TEST_CERTS}"/server_ecdsacert.pem "${SERVER_ECDSA_KEYCERT}"/servercert.pem @@ -18,3 +20,4 @@ cp -f "${TEST_CERTS}"/server_ecdsakey.pem "${SERVER_ECDSA_KEYCERT}"/serverkey.pe ln -sf "${SERVER_KEYCERT}" "${ROOT}"/current ln -sf "${SERVER_ECDSA_KEYCERT}" "${ROOT}"/new +ln -sf "${EMPTY_KEYCERT}" "${ROOT}"/empty