diff --git a/api/envoy/config/core/v3/base.proto b/api/envoy/config/core/v3/base.proto index 4d7d69fae70bd..5b5339ea5bc5d 100644 --- a/api/envoy/config/core/v3/base.proto +++ b/api/envoy/config/core/v3/base.proto @@ -313,6 +313,13 @@ message HeaderMap { repeated HeaderValue headers = 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 { + // Directory path to watch. + string path = 1 [(validate.rules).string = {min_len: 1}]; +} + // 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..27b0b356b1a79 100644 --- a/api/envoy/config/core/v4alpha/base.proto +++ b/api/envoy/config/core/v4alpha/base.proto @@ -308,6 +308,16 @@ message HeaderMap { repeated HeaderValue headers = 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"; + + // Directory path to watch. + string path = 1 [(validate.rules).string = {min_len: 1}]; +} + // 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..587e3271836b1 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -128,16 +128,36 @@ 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 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). 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 // ` field. This can't be // marked as ``oneof`` due to API compatibility reasons. Setting both :ref:`private_key @@ -191,7 +211,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 +253,21 @@ 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 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 // 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..b2fa6f672628c 100644 --- a/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v4alpha/common.proto @@ -129,17 +129,37 @@ 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 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). 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 // ` field. This can't be // marked as ``oneof`` due to API compatibility reasons. Setting both :ref:`private_key @@ -193,7 +213,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 +255,21 @@ 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 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 // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate // matches one of the specified values. diff --git a/docs/root/configuration/security/secret.rst b/docs/root/configuration/security/secret.rst index 087cf388b759e..fdbc88242d021 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 @@ -239,6 +261,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_directory*. 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 ---------- @@ -261,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/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. diff --git a/generated_api_shadow/envoy/config/core/v3/base.proto b/generated_api_shadow/envoy/config/core/v3/base.proto index 35b015710e5cb..1184c89de6e22 100644 --- a/generated_api_shadow/envoy/config/core/v3/base.proto +++ b/generated_api_shadow/envoy/config/core/v3/base.proto @@ -311,6 +311,13 @@ message HeaderMap { repeated HeaderValue headers = 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 { + // Directory path to watch. + string path = 1 [(validate.rules).string = {min_len: 1}]; +} + // 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..95ca4f77a2bcd 100644 --- a/generated_api_shadow/envoy/config/core/v4alpha/base.proto +++ b/generated_api_shadow/envoy/config/core/v4alpha/base.proto @@ -315,6 +315,16 @@ message HeaderMap { repeated HeaderValue headers = 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"; + + // Directory path to watch. + string path = 1 [(validate.rules).string = {min_len: 1}]; +} + // 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..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 @@ -127,16 +127,36 @@ 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 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). 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 // ` field. This can't be // marked as ``oneof`` due to API compatibility reasons. Setting both :ref:`private_key @@ -190,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.api.v2.auth.CertificateValidationContext"; @@ -230,8 +250,21 @@ 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 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 // 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..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 @@ -129,17 +129,37 @@ 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 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). 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 // ` field. This can't be // marked as ``oneof`` due to API compatibility reasons. Setting both :ref:`private_key @@ -193,7 +213,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 +255,21 @@ 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 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 // SHA-256 of the DER-encoded Subject Public Key Information (SPKI) of the presented certificate // matches one of the specified values. diff --git a/source/common/config/BUILD b/source/common/config/BUILD index cf1ac31142ffb..7c5460df96bb7 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -444,6 +444,17 @@ 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: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..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_directory.h b/source/common/config/watched_directory.h new file mode 100644 index 0000000000000..34e965a0b4df0 --- /dev/null +++ b/source/common/config/watched_directory.h @@ -0,0 +1,28 @@ +#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::WatchedDirectory. +class WatchedDirectory { +public: + using Callback = std::function; + + WatchedDirectory(const envoy::config::core::v3::WatchedDirectory& config, + Event::Dispatcher& dispatcher); + + void setCallback(Callback cb) { cb_ = cb; } + +private: + std::unique_ptr watcher_; + Callback cb_; +}; + +using WatchedDirectoryPtr = std::unique_ptr; + +} // namespace Config +} // namespace Envoy 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..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,16 +23,17 @@ 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), + 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", - 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( - 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 @@ -36,6 +41,48 @@ 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() { + // 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(); + } +} + void SdsApi::onConfigUpdate(const std::vector& resources, const std::string& version_info) { validateUpdateSize(resources.size()); @@ -47,35 +94,40 @@ 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); secret_hash_ = new_hash; setSecret(secret); + const auto files = loadFiles(); + files_hash_ = getHashForFiles(files); + resolveSecret(files); 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(); + // Either we have a watched path 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]() { 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(); @@ -114,36 +166,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 (tls_certificate_secrets_ && tls_certificate_secrets_->has_certificate_chain() && - 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(tls_certificate_secrets_->certificate_chain().filename()); + files.push_back(sds_tls_certificate_secrets_->certificate_chain().filename()); } - if (tls_certificate_secrets_ && tls_certificate_secrets_->has_private_key() && - 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(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 (certificate_validation_context_secrets_ && - certificate_validation_context_secrets_->has_trusted_ca() && - 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(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 52b78a37a49ba..1c58feb6c5bc3 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" @@ -30,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. */ @@ -60,8 +73,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(const FileContentMap& /*files*/){}; virtual void validateConfig(const envoy::extensions::transport_sockets::tls::v3::Secret&) PURE; Common::CallbackManager<> update_callback_manager_; @@ -74,15 +92,26 @@ 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; + + void resolveDataSource(const FileContentMap& files, + envoy::config::core::v3::DataSource& data_source); Init::TargetImpl init_target_; + Event::Dispatcher& dispatcher_; + Api::Api& api_; 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(); + 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_; @@ -94,8 +123,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 +169,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 +185,37 @@ 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()); + resolved_tls_certificate_secrets_ = nullptr; + if (secret.tls_certificate().has_watched_directory()) { + watched_directory_ = std::make_unique( + secret.tls_certificate().watched_directory(), dispatcher_); + } else { + watched_directory_.reset(); + } + } + void resolveSecret(const FileContentMap& files) override { + resolved_tls_certificate_secrets_ = + std::make_unique( + *sds_tls_certificate_secrets_); + // We replace path based secrets with inlined secrets on update. + 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(); } private: - TlsCertificatePtr tls_certificate_secrets_; + // Path to watch for rotation. + Config::WatchedDirectoryPtr watched_directory_; + // TlsCertificate according to SDS source. + TlsCertificatePtr sds_tls_certificate_secrets_; + // TlsCertificate after reloading. Path based certificates are inlined for + // future read consistency. + TlsCertificatePtr resolved_tls_certificate_secrets_; }; /** @@ -205,7 +254,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 +272,26 @@ 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()); + resolved_certificate_validation_context_secrets_ = nullptr; + if (secret.validation_context().has_watched_directory()) { + watched_directory_ = std::make_unique( + secret.validation_context().watched_directory(), dispatcher_); + } else { + watched_directory_.reset(); + } + } + + 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_); + // We replace path based secrets with inlined secrets on update. + resolveDataSource(files, + *resolved_certificate_validation_context_secrets_->mutable_trusted_ca()); } void @@ -233,9 +299,16 @@ 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 rotation. + Config::WatchedDirectoryPtr watched_directory_; + // CertificateValidationContext according to SDS source; + CertificateValidationContextPtr sds_certificate_validation_context_secrets_; + // CertificateValidationContext after resolving paths via watched_directory_. + 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_; @@ -306,6 +379,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_; @@ -346,7 +420,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); @@ -359,17 +433,19 @@ 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 { validation_callback_manager_.runCallbacks(secret.generic_secret()); } std::vector getDataSourceFilenames() override; + Config::WatchedDirectory* getWatchedDirectory() override { return nullptr; } private: - GenericSecretPtr generic_secret; + GenericSecretPtr generic_secret_; Common::CallbackManager validation_callback_manager_; }; 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..8988306d4ad4d --- /dev/null +++ b/test/common/config/watched_directory_test.cc @@ -0,0 +1,33 @@ +#include "envoy/filesystem/watcher.h" + +#include "common/config/watched_directory.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 diff --git a/test/common/secret/BUILD b/test/common/secret/BUILD index 48572641a39b2..6161773b7b8c2 100644 --- a/test/common/secret/BUILD +++ b/test/common/secret/BUILD @@ -43,12 +43,15 @@ 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", "//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 8d5cdb00294af..12386bbf535e6 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -12,29 +12,38 @@ #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" #include "gmock/gmock.h" #include "gtest/gtest.h" using ::testing::_; +using ::testing::InSequence; using ::testing::Invoke; using ::testing::InvokeWithoutArgs; +using ::testing::NiceMock; +using ::testing::Return; +using ::testing::Throw; 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() { @@ -51,19 +60,21 @@ class SdsApiTest : public testing::Test { Event::GlobalTimeSystem time_system_; Init::TargetHandlePtr init_target_handle_; Event::DispatcherPtr dispatcher_; + Stats::TestUtil::TestStore stats_; }; +class SdsApiTest : public testing::Test, public SdsApiTestBase {}; + // Validate that SdsApi object is created and initialized successfully. 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(); } @@ -72,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: @@ -90,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; @@ -113,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)); } @@ -122,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 { @@ -131,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; @@ -180,15 +188,317 @@ 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_; +}; + +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_, stats_, + []() {}, mock_dispatcher_, *api_); + sds_api_->registerInitTarget(init_manager_); + initialize(); + handle_ = sds_api_->addUpdateCallback([this]() { secret_callback_.onAddOrUpdateSecret(); }); + } + + ~TlsCertificateSdsRotationApiTest() override { 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_, stats_, + []() {}, mock_dispatcher_, *api_); + sds_api_->registerInitTarget(init_manager_); + initialize(); + handle_ = sds_api_->addUpdateCallback([this]() { secret_callback_.onAddOrUpdateSecret(); }); + } + + ~CertificateValidationContextSdsRotationApiTest() override { 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()); +} + +// 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; + 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 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", + 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, + 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); } @@ -202,6 +512,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::WatchedDirectory* getWatchedDirectory() override { return nullptr; } NiceMock validation_visitor_; }; @@ -214,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")); @@ -238,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; @@ -284,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; @@ -339,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; @@ -428,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; @@ -474,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(); @@ -490,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 = @@ -521,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/BUILD b/test/integration/BUILD index db3e796099c6e..43dfa373074d2 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -1139,8 +1139,13 @@ envoy_cc_test( "sds_dynamic_integration_test.cc", ], data = [ + "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", diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index 2a44fc58dc86d..f686a0946fb43 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -139,6 +139,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 @@ -201,6 +202,103 @@ 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_directory = tls_certificate->mutable_watched_directory(); + watched_directory->set_path(TestEnvironment::temporaryPath("root")); + 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))); + +// Validate that a basic key-cert rotation works via symlink rename. +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); + // 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) { @@ -214,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, @@ -231,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. @@ -241,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 { @@ -369,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. @@ -386,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 @@ -409,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 @@ -433,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. @@ -504,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. @@ -527,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 new file mode 100755 index 0000000000000..2e5f30d88751d --- /dev/null +++ b/test/integration/sds_dynamic_key_rotation_setup.sh @@ -0,0 +1,23 @@ +#!/bin/bash + +set -e + +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 +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 +ln -sf "${EMPTY_KEYCERT}" "${ROOT}"/empty diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index adb3e8b4de7c5..08c04e889c743 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -379,6 +379,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 1e0d25cd57f9f..6b4abcd7c63c8 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -1034,6 +1034,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);