From 614d03aa9b96d492271eda2a670390996e466179 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Fri, 9 Dec 2022 10:32:33 -0500 Subject: [PATCH 1/7] envoy: creating a stripped main base without validation server (#24294) Splitting main common base into a third class, to allow for a main base without the validation server Risk Level: Medium (added fast fail for main common base validate to mittigate) Testing: updated tests Docs Changes: n/a Release Notes: did not add Signed-off-by: Alyssa Wilk --- mobile/library/common/BUILD | 4 +- mobile/library/common/engine_common.cc | 4 +- mobile/library/common/engine_common.h | 12 +- source/exe/BUILD | 54 +++++++- source/exe/main_common.cc | 149 +--------------------- source/exe/main_common.h | 54 +------- source/exe/stripped_main_base.cc | 169 +++++++++++++++++++++++++ source/exe/stripped_main_base.h | 86 +++++++++++++ test/per_file_coverage.sh | 0 9 files changed, 327 insertions(+), 205 deletions(-) create mode 100644 source/exe/stripped_main_base.cc create mode 100644 source/exe/stripped_main_base.h mode change 100755 => 100644 test/per_file_coverage.sh diff --git a/mobile/library/common/BUILD b/mobile/library/common/BUILD index 0466044dc7f7..55ec285fd010 100644 --- a/mobile/library/common/BUILD +++ b/mobile/library/common/BUILD @@ -53,7 +53,7 @@ envoy_cc_library( "@envoy//source/common/common:minimal_logger_lib", "@envoy//source/common/common:random_generator_lib", "@envoy//source/common/runtime:runtime_lib", - "@envoy//source/exe:main_common_lib", + "@envoy//source/exe:stripped_main_base_lib", ] + select({ "@envoy//bazel:disable_signal_trace": [], "//conditions:default": [ @@ -67,6 +67,6 @@ envoy_cc_library( repository = "@envoy", deps = [ ":engine_common_lib", - "@envoy//source/exe:envoy_main_common_lib", + "@envoy//source/exe:envoy_stripped_main_base_lib", ], ) diff --git a/mobile/library/common/engine_common.cc b/mobile/library/common/engine_common.cc index b20b31f147b2..c11b166b5785 100644 --- a/mobile/library/common/engine_common.cc +++ b/mobile/library/common/engine_common.cc @@ -5,8 +5,10 @@ namespace Envoy { +std::string hotRestartVersion(bool) { return "disabled"; } + EngineCommon::EngineCommon(int argc, const char* const* argv) - : options_(argc, argv, &MainCommon::hotRestartVersion, spdlog::level::info), + : options_(argc, argv, &hotRestartVersion, spdlog::level::info), base_(options_, real_time_system_, default_listener_hooks_, prod_component_factory_, std::make_unique(), std::make_unique(), nullptr) { diff --git a/mobile/library/common/engine_common.h b/mobile/library/common/engine_common.h index c979737c7a9a..bae7acd5fb8c 100644 --- a/mobile/library/common/engine_common.h +++ b/mobile/library/common/engine_common.h @@ -4,8 +4,8 @@ #include "envoy/server/instance.h" #include "source/common/event/real_time_system.h" -#include "source/exe/main_common.h" #include "source/exe/platform_impl.h" +#include "source/exe/stripped_main_base.h" #include "source/server/listener_hooks.h" #include "source/server/options_impl.h" @@ -18,12 +18,15 @@ namespace Envoy { /** * This class is used instead of Envoy::MainCommon to customize logic for the Envoy Mobile setting. - * It largely leverages Envoy::MainCommonBase. + * It largely leverages Envoy::StrippedMainBase. */ class EngineCommon { public: EngineCommon(int argc, const char* const* argv); - bool run() { return base_.run(); } + bool run() { + base_.runServer(); + return true; + } /** * @return a pointer to the server instance, or nullptr if initialized into @@ -38,13 +41,12 @@ class EngineCommon { Envoy::SignalAction handle_sigs_; Envoy::TerminateHandler log_on_terminate_; #endif - Thread::MainThread register_main_thread_; Envoy::OptionsImpl options_; Event::RealTimeSystem real_time_system_; // NO_CHECK_FORMAT(real_time) DefaultListenerHooks default_listener_hooks_; ProdComponentFactory prod_component_factory_; - MainCommonBase base_; + StrippedMainBase base_; }; } // namespace Envoy diff --git a/source/exe/BUILD b/source/exe/BUILD index 390bd8a4d84d..a34d06a04d2e 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -61,10 +61,40 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "stripped_main_base_lib", + srcs = ["stripped_main_base.cc"], + hdrs = ["stripped_main_base.h"], + deps = [ + ":envoy_common_lib", + ":platform_impl_lib", + ":process_wide_lib", + "//source/common/thread_local:thread_local_lib", + "//source/common/api:os_sys_calls_lib", + "//source/common/common:compiler_requirements_lib", + "//source/common/common:perf_annotation_lib", + "//source/common/grpc:google_grpc_context_lib", + "//source/server:hot_restart_lib", + "//source/server:hot_restart_nop_lib", + ] + select({ + "//bazel:disable_signal_trace": [], + "//conditions:default": [ + "//source/common/signal:sigaction_lib", + ":terminate_handler_lib", + ], + }), +) + envoy_cc_library( name = "main_common_lib", - srcs = ["main_common.cc"], - hdrs = ["main_common.h"], + srcs = [ + "main_common.cc", + "stripped_main_base.cc", + ], + hdrs = [ + "main_common.h", + "stripped_main_base.h", + ], deps = [ ":envoy_common_lib", ":platform_impl_lib", @@ -85,6 +115,7 @@ envoy_cc_library( }), ) +# provides a library target for Envoy server builds with the versioning information set up correctly. envoy_cc_library( name = "envoy_main_common_lib", deps = [ @@ -96,6 +127,15 @@ envoy_cc_library( ], ) +# provides a library target for Envoy Mobile builds with the versioning information set up correctly. +envoy_cc_library( + name = "envoy_stripped_main_base_lib", + deps = [ + ":stripped_main_base_lib", + "//source/common/version:version_linkstamp", + ], +) + envoy_cc_library( name = "envoy_common_with_core_extensions_lib", deps = [ @@ -117,8 +157,14 @@ envoy_cc_library( envoy_cc_library( name = "envoy_main_common_with_core_extensions_lib", - srcs = ["main_common.cc"], - hdrs = ["main_common.h"], + srcs = [ + "main_common.cc", + "stripped_main_base.cc", + ], + hdrs = [ + "main_common.h", + "stripped_main_base.h", + ], deps = [ ":envoy_common_with_core_extensions_lib", ":platform_impl_lib", diff --git a/source/exe/main_common.cc b/source/exe/main_common.cc index ea84f4037ba6..ba0edd482cac 100644 --- a/source/exe/main_common.cc +++ b/source/exe/main_common.cc @@ -29,154 +29,15 @@ namespace Envoy { -Server::DrainManagerPtr ProdComponentFactory::createDrainManager(Server::Instance& server) { - // The global drain manager only triggers on listener modification, which effectively is - // hot restart at the global level. The per-listener drain managers decide whether to - // to include /healthcheck/fail status. - return std::make_unique( - server, envoy::config::listener::v3::Listener::MODIFY_ONLY, server.dispatcher()); -} - -Runtime::LoaderPtr ProdComponentFactory::createRuntime(Server::Instance& server, - Server::Configuration::Initial& config) { - return Server::InstanceUtil::createRuntime(server, config); -} - -MainCommonBase::MainCommonBase(const Server::Options& options, Event::TimeSystem& time_system, - ListenerHooks& listener_hooks, - Server::ComponentFactory& component_factory, - std::unique_ptr platform_impl, - std::unique_ptr&& random_generator, - std::unique_ptr process_context) - : platform_impl_(std::move(platform_impl)), options_(options), - component_factory_(component_factory), stats_allocator_(symbol_table_) { - // Process the option to disable extensions as early as possible, - // before we do any configuration loading. - OptionsImpl::disableExtensions(options.disabledExtensions()); - - // Enable core dumps as early as possible. - if (options_.coreDumpEnabled()) { - const auto ret = platform_impl_->enableCoreDump(); - if (ret) { - ENVOY_LOG_MISC(info, "core dump enabled"); - } else { - ENVOY_LOG_MISC(warn, "failed to enable core dump"); - } - } - - switch (options_.mode()) { - case Server::Mode::InitOnly: - case Server::Mode::Serve: { - configureHotRestarter(*random_generator); - - tls_ = std::make_unique(); - Thread::BasicLockable& log_lock = restarter_->logLock(); - Thread::BasicLockable& access_log_lock = restarter_->accessLogLock(); - auto local_address = Network::Utility::getLocalAddress(options_.localAddressIpVersion()); - logging_context_ = std::make_unique(options_.logLevel(), options_.logFormat(), - log_lock, options_.logFormatEscaped(), - options_.enableFineGrainLogging()); - - configureComponentLogLevels(); - - // Provide consistent behavior for out-of-memory, regardless of whether it occurs in a try/catch - // block or not. - std::set_new_handler([]() { PANIC("out of memory"); }); - - stats_store_ = std::make_unique(stats_allocator_); - - server_ = std::make_unique( - *init_manager_, options_, time_system, local_address, listener_hooks, *restarter_, - *stats_store_, access_log_lock, component_factory, std::move(random_generator), *tls_, - platform_impl_->threadFactory(), platform_impl_->fileSystem(), std::move(process_context)); - - break; - } - case Server::Mode::Validate: - restarter_ = std::make_unique(); - logging_context_ = - std::make_unique(options_.logLevel(), options_.logFormat(), - restarter_->logLock(), options_.logFormatEscaped()); - break; - } -} - -void MainCommonBase::configureComponentLogLevels() { - for (auto& component_log_level : options_.componentLogLevels()) { - Logger::Logger* logger_to_change = Logger::Registry::logger(component_log_level.first); - ASSERT(logger_to_change); - logger_to_change->setLevel(component_log_level.second); - } -} - -void MainCommonBase::configureHotRestarter(Random::RandomGenerator& random_generator) { -#ifdef ENVOY_HOT_RESTART - if (!options_.hotRestartDisabled()) { - uint32_t base_id = options_.baseId(); - - if (options_.useDynamicBaseId()) { - ASSERT(options_.restartEpoch() == 0, "cannot use dynamic base id during hot restart"); - - std::unique_ptr restarter; - - // Try 100 times to get an unused base ID and then give up under the assumption - // that some other problem has occurred to prevent binding the domain socket. - for (int i = 0; i < 100 && restarter == nullptr; i++) { - // HotRestartImpl is going to multiply this value by 10, so leave head room. - base_id = static_cast(random_generator.random()) & 0x0FFFFFFF; - - TRY_ASSERT_MAIN_THREAD { - restarter = std::make_unique(base_id, 0, options_.socketPath(), - options_.socketMode()); - } - END_TRY - catch (Server::HotRestartDomainSocketInUseException& ex) { - // No luck, try again. - ENVOY_LOG_MISC(debug, "dynamic base id: {}", ex.what()); - } - } - - if (restarter == nullptr) { - throw EnvoyException("unable to select a dynamic base id"); - } - - restarter_.swap(restarter); - } else { - restarter_ = std::make_unique( - base_id, options_.restartEpoch(), options_.socketPath(), options_.socketMode()); - } - - // Write the base-id to the requested path whether we selected it - // dynamically or not. - if (!options_.baseIdPath().empty()) { - std::ofstream base_id_out_file(options_.baseIdPath()); - if (!base_id_out_file) { - ENVOY_LOG_MISC(critical, "cannot open base id output file {} for writing.", - options_.baseIdPath()); - } else { - base_id_out_file << base_id; - } - } - } -#else - UNREFERENCED_PARAMETER(random_generator); -#endif - - if (restarter_ == nullptr) { - restarter_ = std::make_unique(); - } -} - bool MainCommonBase::run() { switch (options_.mode()) { case Server::Mode::Serve: - server_->run(); + runServer(); return true; - case Server::Mode::Validate: { - auto local_address = Network::Utility::getLocalAddress(options_.localAddressIpVersion()); - return Server::validateConfig(options_, local_address, component_factory_, - platform_impl_->threadFactory(), platform_impl_->fileSystem()); - } + case Server::Mode::Validate: + return Server::validateConfig( + options_, Network::Utility::getLocalAddress(options_.localAddressIpVersion()), + component_factory_, platform_impl_->threadFactory(), platform_impl_->fileSystem()); case Server::Mode::InitOnly: PERF_DUMP(); return true; diff --git a/source/exe/main_common.h b/source/exe/main_common.h index f8dff2b70212..f101e578986a 100644 --- a/source/exe/main_common.h +++ b/source/exe/main_common.h @@ -11,6 +11,7 @@ #include "source/common/stats/thread_local_store.h" #include "source/common/thread_local/thread_local_impl.h" #include "source/exe/process_wide.h" +#include "source/exe/stripped_main_base.h" #include "source/server/listener_hooks.h" #include "source/server/options_impl.h" #include "source/server/server.h" @@ -22,29 +23,12 @@ namespace Envoy { -class ProdComponentFactory : public Server::ComponentFactory { +class MainCommonBase : public StrippedMainBase { public: - // Server::DrainManagerFactory - Server::DrainManagerPtr createDrainManager(Server::Instance& server) override; - Runtime::LoaderPtr createRuntime(Server::Instance& server, - Server::Configuration::Initial& config) override; -}; - -class MainCommonBase { -public: - // Consumer must guarantee that all passed references are alive until this object is - // destructed. - MainCommonBase(const Server::Options& options, Event::TimeSystem& time_system, - ListenerHooks& listener_hooks, Server::ComponentFactory& component_factory, - std::unique_ptr platform_impl, - std::unique_ptr&& random_generator, - std::unique_ptr process_context); + using StrippedMainBase::StrippedMainBase; bool run(); - // Will be null if options.mode() == Server::Mode::Validate - Server::Instance* server() { return server_.get(); } - #ifdef ENVOY_ADMIN_FUNCTIONALITY using AdminRequestFn = std::function; @@ -64,38 +48,10 @@ class MainCommonBase { void adminRequest(absl::string_view path_and_query, absl::string_view method, const AdminRequestFn& handler); #endif - -protected: - std::unique_ptr platform_impl_; - ProcessWide process_wide_; // Process-wide state setup/teardown (excluding grpc). - // We instantiate this class regardless of ENVOY_GOOGLE_GRPC, to avoid having - // an ifdef in a header file exposed in a C++ library. It is too easy to have - // the ifdef be inconsistent across build-system boundaries. - Grpc::GoogleGrpcContext google_grpc_context_; - const Envoy::Server::Options& options_; - Server::ComponentFactory& component_factory_; - Stats::SymbolTableImpl symbol_table_; - Stats::AllocatorImpl stats_allocator_; - - ThreadLocal::InstanceImplPtr tls_; - std::unique_ptr restarter_; - Stats::ThreadLocalStoreImplPtr stats_store_; - std::unique_ptr logging_context_; - std::unique_ptr init_manager_{std::make_unique("Server")}; - std::unique_ptr server_; - -private: - void configureComponentLogLevels(); - void configureHotRestarter(Random::RandomGenerator& random_generator); - - // Declaring main thread here allows custom integrations to instantiate - // MainCommonBase directly, with environment-specific dependency injection. - // Note that MainThread must also be declared in MainCommon. - Thread::MainThread main_thread_; }; -// TODO(jmarantz): consider removing this class; I think it'd be more useful to -// go through MainCommonBase directly. +// This is separate from MainCommonBase for legacy reasons: sufficient +// downstream tests use one or the other that resolving is deemed problematic. class MainCommon { public: // Hook to run after a server is created. diff --git a/source/exe/stripped_main_base.cc b/source/exe/stripped_main_base.cc new file mode 100644 index 000000000000..9d532a993b90 --- /dev/null +++ b/source/exe/stripped_main_base.cc @@ -0,0 +1,169 @@ +#include "source/exe/stripped_main_base.h" + +#include +#include +#include +#include + +#include "envoy/config/listener/v3/listener.pb.h" + +#include "source/common/common/compiler_requirements.h" +#include "source/common/common/logger.h" +#include "source/common/common/perf_annotation.h" +#include "source/common/network/utility.h" +#include "source/common/stats/thread_local_store.h" +#include "source/exe/platform_impl.h" +#include "source/server/drain_manager_impl.h" +#include "source/server/hot_restart_nop_impl.h" +#include "source/server/listener_hooks.h" +#include "source/server/options_impl.h" +#include "source/server/server.h" + +#include "absl/debugging/symbolize.h" +#include "absl/strings/str_split.h" + +#ifdef ENVOY_HOT_RESTART +#include "source/server/hot_restart_impl.h" +#endif + +namespace Envoy { + +Server::DrainManagerPtr ProdComponentFactory::createDrainManager(Server::Instance& server) { + // The global drain manager only triggers on listener modification, which effectively is + // hot restart at the global level. The per-listener drain managers decide whether to + // to include /healthcheck/fail status. + return std::make_unique( + server, envoy::config::listener::v3::Listener::MODIFY_ONLY, server.dispatcher()); +} + +Runtime::LoaderPtr ProdComponentFactory::createRuntime(Server::Instance& server, + Server::Configuration::Initial& config) { + return Server::InstanceUtil::createRuntime(server, config); +} + +StrippedMainBase::StrippedMainBase(const Server::Options& options, Event::TimeSystem& time_system, + ListenerHooks& listener_hooks, + Server::ComponentFactory& component_factory, + std::unique_ptr platform_impl, + std::unique_ptr&& random_generator, + std::unique_ptr process_context) + : platform_impl_(std::move(platform_impl)), options_(options), + component_factory_(component_factory), stats_allocator_(symbol_table_) { + // Process the option to disable extensions as early as possible, + // before we do any configuration loading. + OptionsImpl::disableExtensions(options.disabledExtensions()); + + // Enable core dumps as early as possible. + if (options_.coreDumpEnabled()) { + const auto ret = platform_impl_->enableCoreDump(); + if (ret) { + ENVOY_LOG_MISC(info, "core dump enabled"); + } else { + ENVOY_LOG_MISC(warn, "failed to enable core dump"); + } + } + + switch (options_.mode()) { + case Server::Mode::InitOnly: + case Server::Mode::Serve: { + configureHotRestarter(*random_generator); + + tls_ = std::make_unique(); + Thread::BasicLockable& log_lock = restarter_->logLock(); + Thread::BasicLockable& access_log_lock = restarter_->accessLogLock(); + auto local_address = Network::Utility::getLocalAddress(options_.localAddressIpVersion()); + logging_context_ = std::make_unique(options_.logLevel(), options_.logFormat(), + log_lock, options_.logFormatEscaped(), + options_.enableFineGrainLogging()); + + configureComponentLogLevels(); + + // Provide consistent behavior for out-of-memory, regardless of whether it occurs in a try/catch + // block or not. + std::set_new_handler([]() { PANIC("out of memory"); }); + + stats_store_ = std::make_unique(stats_allocator_); + + server_ = std::make_unique( + *init_manager_, options_, time_system, local_address, listener_hooks, *restarter_, + *stats_store_, access_log_lock, component_factory, std::move(random_generator), *tls_, + platform_impl_->threadFactory(), platform_impl_->fileSystem(), std::move(process_context)); + + break; + } + case Server::Mode::Validate: + restarter_ = std::make_unique(); + logging_context_ = + std::make_unique(options_.logLevel(), options_.logFormat(), + restarter_->logLock(), options_.logFormatEscaped()); + break; + } +} + +void StrippedMainBase::configureComponentLogLevels() { + for (auto& component_log_level : options_.componentLogLevels()) { + Logger::Logger* logger_to_change = Logger::Registry::logger(component_log_level.first); + ASSERT(logger_to_change); + logger_to_change->setLevel(component_log_level.second); + } +} + +void StrippedMainBase::configureHotRestarter(Random::RandomGenerator& random_generator) { +#ifdef ENVOY_HOT_RESTART + if (!options_.hotRestartDisabled()) { + uint32_t base_id = options_.baseId(); + + if (options_.useDynamicBaseId()) { + ASSERT(options_.restartEpoch() == 0, "cannot use dynamic base id during hot restart"); + + std::unique_ptr restarter; + + // Try 100 times to get an unused base ID and then give up under the assumption + // that some other problem has occurred to prevent binding the domain socket. + for (int i = 0; i < 100 && restarter == nullptr; i++) { + // HotRestartImpl is going to multiply this value by 10, so leave head room. + base_id = static_cast(random_generator.random()) & 0x0FFFFFFF; + + TRY_ASSERT_MAIN_THREAD { + restarter = std::make_unique(base_id, 0, options_.socketPath(), + options_.socketMode()); + } + END_TRY + catch (Server::HotRestartDomainSocketInUseException& ex) { + // No luck, try again. + ENVOY_LOG_MISC(debug, "dynamic base id: {}", ex.what()); + } + } + + if (restarter == nullptr) { + throw EnvoyException("unable to select a dynamic base id"); + } + + restarter_.swap(restarter); + } else { + restarter_ = std::make_unique( + base_id, options_.restartEpoch(), options_.socketPath(), options_.socketMode()); + } + + // Write the base-id to the requested path whether we selected it + // dynamically or not. + if (!options_.baseIdPath().empty()) { + std::ofstream base_id_out_file(options_.baseIdPath()); + if (!base_id_out_file) { + ENVOY_LOG_MISC(critical, "cannot open base id output file {} for writing.", + options_.baseIdPath()); + } else { + base_id_out_file << base_id; + } + } + } +#else + UNREFERENCED_PARAMETER(random_generator); +#endif + + if (restarter_ == nullptr) { + restarter_ = std::make_unique(); + } +} + +} // namespace Envoy diff --git a/source/exe/stripped_main_base.h b/source/exe/stripped_main_base.h new file mode 100644 index 000000000000..10a2bcf0f73b --- /dev/null +++ b/source/exe/stripped_main_base.h @@ -0,0 +1,86 @@ +#pragma once + +#include "envoy/event/timer.h" +#include "envoy/runtime/runtime.h" +#include "envoy/server/platform.h" + +#include "source/common/common/thread.h" +#include "source/common/event/real_time_system.h" +#include "source/common/grpc/google_grpc_context.h" +#include "source/common/stats/symbol_table.h" +#include "source/common/stats/thread_local_store.h" +#include "source/common/thread_local/thread_local_impl.h" +#include "source/exe/process_wide.h" +#include "source/server/listener_hooks.h" +#include "source/server/options_impl.h" +#include "source/server/server.h" + +#ifdef ENVOY_HANDLE_SIGNALS +#include "source/common/signal/signal_action.h" +#include "source/exe/terminate_handler.h" +#endif + +namespace Envoy { + +class ProdComponentFactory : public Server::ComponentFactory { +public: + // Server::DrainManagerFactory + Server::DrainManagerPtr createDrainManager(Server::Instance& server) override; + Runtime::LoaderPtr createRuntime(Server::Instance& server, + Server::Configuration::Initial& config) override; +}; + +// This is the common main between Envoy and Envoy mobile. +// It is stripped down to functionality required by Envoy Mobile: anything +// server-specific should live in MainCommonBase or MainCommon which remain +// separate for legacy reasons. +class StrippedMainBase { +public: + static std::string hotRestartVersion(bool hot_restart_enabled); + + // Consumer must guarantee that all passed references are alive until this object is + // destructed. + StrippedMainBase(const Server::Options& options, Event::TimeSystem& time_system, + ListenerHooks& listener_hooks, Server::ComponentFactory& component_factory, + std::unique_ptr platform_impl, + std::unique_ptr&& random_generator, + std::unique_ptr process_context); + + void runServer() { + ASSERT(options_.mode() == Server::Mode::Serve); + server_->run(); + } + + // Will be null if options.mode() == Server::Mode::Validate + Server::Instance* server() { return server_.get(); } + +protected: + std::unique_ptr platform_impl_; + ProcessWide process_wide_; // Process-wide state setup/teardown (excluding grpc). + // We instantiate this class regardless of ENVOY_GOOGLE_GRPC, to avoid having + // an ifdef in a header file exposed in a C++ library. It is too easy to have + // the ifdef be inconsistent across build-system boundaries. + Grpc::GoogleGrpcContext google_grpc_context_; + const Envoy::Server::Options& options_; + Server::ComponentFactory& component_factory_; + Stats::SymbolTableImpl symbol_table_; + Stats::AllocatorImpl stats_allocator_; + + ThreadLocal::InstanceImplPtr tls_; + std::unique_ptr restarter_; + Stats::ThreadLocalStoreImplPtr stats_store_; + std::unique_ptr logging_context_; + std::unique_ptr init_manager_{std::make_unique("Server")}; + std::unique_ptr server_; + +private: + void configureComponentLogLevels(); + void configureHotRestarter(Random::RandomGenerator& random_generator); + + // Declaring main thread here allows custom integrations to instantiate + // StrippedMainBase directly, with environment-specific dependency injection. + // Note that MainThread must also be declared in MainCommon. + Thread::MainThread main_thread_; +}; + +} // namespace Envoy diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh old mode 100755 new mode 100644 From 00a343c3496de7ad774c6b42eadea4a1e5b7e251 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Fri, 9 Dec 2022 10:34:56 -0500 Subject: [PATCH 2/7] mobile: Enable BaseClientIntegrationTest to bypass stream metrics checks (#24461) Some integration tests do not send data plane requests (we are building one such test now), so optionally allow those tests to skip stream metrics validation, controlled by a test knob. Signed-off-by: Ali Beyad --- .../test/common/integration/base_client_integration_test.cc | 4 +++- mobile/test/common/integration/base_client_integration_test.h | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/mobile/test/common/integration/base_client_integration_test.cc b/mobile/test/common/integration/base_client_integration_test.cc index 7ac8701454d2..19c108d3d7eb 100644 --- a/mobile/test/common/integration/base_client_integration_test.cc +++ b/mobile/test/common/integration/base_client_integration_test.cc @@ -108,7 +108,9 @@ void BaseClientIntegrationTest::initialize() { }); stream_prototype_->setOnComplete( [this](envoy_stream_intel, envoy_final_stream_intel final_intel) { - validateStreamIntel(final_intel, expect_dns_, upstream_tls_, cc_.on_complete_calls == 0); + if (expect_data_streams_) { + validateStreamIntel(final_intel, expect_dns_, upstream_tls_, cc_.on_complete_calls == 0); + } cc_.on_complete_received_byte_count = final_intel.received_byte_count; cc_.on_complete_calls++; cc_.terminal_callback->setReady(); diff --git a/mobile/test/common/integration/base_client_integration_test.h b/mobile/test/common/integration/base_client_integration_test.h index 8e233f3e134c..4cd159028013 100644 --- a/mobile/test/common/integration/base_client_integration_test.h +++ b/mobile/test/common/integration/base_client_integration_test.h @@ -61,6 +61,8 @@ class BaseClientIntegrationTest : public BaseIntegrationTest, public Platform::E bool explicit_flow_control_ = false; bool expect_dns_ = true; bool override_builder_config_ = false; + // True if data plane requests are expected in the test; false otherwise. + bool expect_data_streams_ = true; }; } // namespace Envoy From 59c0e37fd6b9524c85096a1335d3acce043d3abb Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Fri, 9 Dec 2022 10:57:23 -0500 Subject: [PATCH 3/7] Fix broken format strings (#24462) Risk Level: Low Testing: Unit Tests Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Yan Avlasov yavlasov@google.com Signed-off-by: Yan Avlasov --- source/common/http/http2/codec_impl.cc | 2 +- source/common/network/io_socket_handle_impl.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index e03f8e097e71..6d3c0cf793ee 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -911,7 +911,7 @@ ConnectionImpl::~ConnectionImpl() { void ConnectionImpl::sendKeepalive() { ASSERT(keepalive_timeout_timer_); if (keepalive_timeout_timer_->enabled()) { - ENVOY_CONN_LOG(trace, "Skipping PING: already awaiting PING ACK {}", connection_); + ENVOY_CONN_LOG(trace, "Skipping PING: already awaiting PING ACK", connection_); return; } diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 9f4ba719a66e..918b3fe08ef5 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -551,7 +551,7 @@ Address::InstanceConstSharedPtr IoSocketHandleImpl::peerAddress() { os_sys_calls.getpeername(fd_, reinterpret_cast(&ss), &ss_len); if (result.return_value_ != 0) { throw EnvoyException( - fmt::format("getpeername failed for '{}': {}", errorDetails(result.errno_))); + fmt::format("getpeername failed for '{}': {}", fd_, errorDetails(result.errno_))); } if (ss_len == udsAddressLength() && ss.ss_family == AF_UNIX) { From 20bc862384191e62357f08b53580d553ab4a8c0b Mon Sep 17 00:00:00 2001 From: "Adi (Suissa) Peleg" Date: Fri, 9 Dec 2022 11:21:31 -0500 Subject: [PATCH 4/7] stats: setting default value for returned values (#24454) Signed-off-by: Adi Suissa-Peleg --- source/common/stats/histogram_impl.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source/common/stats/histogram_impl.h b/source/common/stats/histogram_impl.h index 191626a9c4f9..23a2adaf23c2 100644 --- a/source/common/stats/histogram_impl.h +++ b/source/common/stats/histogram_impl.h @@ -68,9 +68,9 @@ class HistogramStatisticsImpl final : public HistogramStatistics, NonCopyable { ConstSupportedBuckets& supported_buckets_; std::vector computed_quantiles_; std::vector computed_buckets_; - uint64_t sample_count_; - double sample_sum_; - const Histogram::Unit unit_; + uint64_t sample_count_{0}; + double sample_sum_{0}; + const Histogram::Unit unit_{Histogram::Unit::Unspecified}; }; class HistogramImplHelper : public MetricImpl { From 55a4cc725f5c2a536aa6afa193839d65d2b1a4cc Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Fri, 9 Dec 2022 08:52:35 -0800 Subject: [PATCH 5/7] Simplify async certificate validation (#24297) Signed-off-by: Ryan Hamilton --- envoy/ssl/ssl_socket_extended_info.h | 8 ++- .../platform_bridge_cert_validator.cc | 26 ++++--- .../platform_bridge_cert_validator.h | 1 - .../platform_bridge_cert_validator_test.cc | 59 +++++++-------- .../common/quic/envoy_quic_proof_verifier.cc | 4 +- .../tls/cert_validator/cert_validator.h | 6 +- .../tls/cert_validator/default_validator.cc | 71 +++++++++---------- .../tls/cert_validator/default_validator.h | 6 +- .../cert_validator/spiffe/spiffe_validator.cc | 55 ++++++-------- .../cert_validator/spiffe/spiffe_validator.h | 4 +- .../transport_sockets/tls/context_impl.cc | 19 ++--- .../transport_sockets/tls/ssl_handshaker.cc | 4 +- .../transport_sockets/tls/ssl_handshaker.h | 4 +- .../cert_validator/default_validator_test.cc | 13 ++-- .../spiffe/spiffe_validator_test.cc | 67 +++++++++-------- .../cert_validator/timed_cert_validator.cc | 12 ++-- .../tls/cert_validator/timed_cert_validator.h | 1 - 17 files changed, 166 insertions(+), 194 deletions(-) diff --git a/envoy/ssl/ssl_socket_extended_info.h b/envoy/ssl/ssl_socket_extended_info.h index a28b7f723379..478fd07791f9 100644 --- a/envoy/ssl/ssl_socket_extended_info.h +++ b/envoy/ssl/ssl_socket_extended_info.h @@ -32,11 +32,15 @@ class ValidateResultCallback { /** * Called when the asynchronous cert validation completes. * @param succeeded true if the validation succeeds + * @param detailed_status detailed status of the underlying validation. Depending on the + * validation configuration, `succeeded` may be true but `detailed_status` might + * indicate a failure. This detailed status can be used to inform routing + * decisions. * @param error_details failure details, only used if the validation fails. * @param tls_alert the TLS error related to the failure, only used if the validation fails. */ - virtual void onCertValidationResult(bool succeeded, const std::string& error_details, - uint8_t tls_alert) PURE; + virtual void onCertValidationResult(bool succeeded, ClientValidationStatus detailed_status, + const std::string& error_details, uint8_t tls_alert) PURE; }; using ValidateResultCallbackPtr = std::unique_ptr; diff --git a/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.cc b/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.cc index c396092fa627..11f909a26ea5 100644 --- a/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.cc +++ b/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.cc @@ -35,28 +35,22 @@ PlatformBridgeCertValidator::~PlatformBridgeCertValidator() { ValidationResults PlatformBridgeCertValidator::doVerifyCertChain( STACK_OF(X509) & cert_chain, Ssl::ValidateResultCallbackPtr callback, - Ssl::SslExtendedSocketInfo* ssl_extended_info, const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options, SSL_CTX& /*ssl_ctx*/, const CertValidator::ExtraValidationContext& /*validation_context*/, bool is_server, absl::string_view hostname) { ASSERT(!is_server); if (sk_X509_num(&cert_chain) == 0) { - if (ssl_extended_info) { - ssl_extended_info->setCertificateValidationStatus( - Envoy::Ssl::ClientValidationStatus::NotValidated); - } const char* error = "verify cert chain failed: empty cert chain."; stats_.fail_verify_error_.inc(); ENVOY_LOG(debug, error); - return {ValidationResults::ValidationStatus::Failed, absl::nullopt, error}; - } - if (callback == nullptr) { - callback = ssl_extended_info->createValidateResultCallback(); + return {ValidationResults::ValidationStatus::Failed, + Envoy::Ssl::ClientValidationStatus::NotValidated, absl::nullopt, error}; } if (callback == nullptr) { IS_ENVOY_BUG("No callback specified"); const char* error = "verify cert chain failed: no callback specified."; - return {ValidationResults::ValidationStatus::Failed, absl::nullopt, error}; + return {ValidationResults::ValidationStatus::Failed, + Envoy::Ssl::ClientValidationStatus::NotValidated, absl::nullopt, error}; } std::vector certs; @@ -94,8 +88,8 @@ ValidationResults PlatformBridgeCertValidator::doVerifyCertChain( std::string(host), std::move(subject_alt_names), this); std::thread::id thread_id = job.validation_thread_.get_id(); validation_jobs_[thread_id] = std::move(job); - - return {ValidationResults::ValidationStatus::Pending, absl::nullopt, absl::nullopt}; + return {ValidationResults::ValidationStatus::Pending, + Envoy::Ssl::ClientValidationStatus::NotValidated, absl::nullopt, absl::nullopt}; } void PlatformBridgeCertValidator::verifyCertChainByPlatform( @@ -172,17 +166,21 @@ void PlatformBridgeCertValidator::onVerificationComplete(std::thread::id thread_ ValidationJob& job = job_handle.mapped(); job.validation_thread_.join(); + Ssl::ClientValidationStatus detailed_status = Envoy::Ssl::ClientValidationStatus::NotValidated; switch (failure_type) { case ValidationFailureType::SUCCESS: + detailed_status = Envoy::Ssl::ClientValidationStatus::Validated; break; case ValidationFailureType::FAIL_VERIFY_ERROR: + detailed_status = Envoy::Ssl::ClientValidationStatus::Failed; stats_.fail_verify_error_.inc(); case ValidationFailureType::FAIL_VERIFY_SAN: + detailed_status = Envoy::Ssl::ClientValidationStatus::Failed; stats_.fail_verify_san_.inc(); } - job.result_callback_->onCertValidationResult(allow_untrusted_certificate_ || success, error, - tls_alert); + job.result_callback_->onCertValidationResult(allow_untrusted_certificate_ || success, + detailed_status, error, tls_alert); ENVOY_LOG(trace, "Finished platform cert validation for {}, post result callback to network thread", hostname); diff --git a/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h b/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h index 54f4f8740371..8ece9b644dab 100644 --- a/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h +++ b/mobile/library/common/extensions/cert_validator/platform_bridge/platform_bridge_cert_validator.h @@ -52,7 +52,6 @@ class PlatformBridgeCertValidator : public CertValidator, Logger::Loggable callback_; @@ -174,8 +174,8 @@ TEST_P(PlatformBridgeCertValidatorTest, NoCallback) { EXPECT_ENVOY_BUG( { validator.doVerifyCertChain(*cert_chain, Ssl::ValidateResultCallbackPtr(), - &ssl_extended_info_, transport_socket_options_, *ssl_ctx_, - validation_context_, is_server_, hostname); + transport_socket_options_, *ssl_ctx_, validation_context_, + is_server_, hostname); }, "No callback specified"); } @@ -187,15 +187,14 @@ TEST_P(PlatformBridgeCertValidatorTest, EmptyCertChain) { bssl::UniquePtr cert_chain(sk_X509_new_null()); std::string hostname = "www.example.com"; - ValidationResults results = validator.doVerifyCertChain( - *cert_chain, std::move(callback_), &ssl_extended_info_, transport_socket_options_, *ssl_ctx_, - validation_context_, is_server_, hostname); + ValidationResults results = + validator.doVerifyCertChain(*cert_chain, std::move(callback_), transport_socket_options_, + *ssl_ctx_, validation_context_, is_server_, hostname); EXPECT_EQ(ValidationResults::ValidationStatus::Failed, results.status); EXPECT_FALSE(results.tls_alert.has_value()); ASSERT_TRUE(results.error_details.has_value()); EXPECT_EQ("verify cert chain failed: empty cert chain.", results.error_details.value()); - EXPECT_EQ(Envoy::Ssl::ClientValidationStatus::NotValidated, - ssl_extended_info_.certificateValidationStatus()); + EXPECT_EQ(Envoy::Ssl::ClientValidationStatus::NotValidated, results.detailed_status); } TEST_P(PlatformBridgeCertValidatorTest, ValidCertificate) { @@ -211,15 +210,17 @@ TEST_P(PlatformBridgeCertValidatorTest, ValidCertificate) { auto& callback_ref = *callback_; EXPECT_CALL(callback_ref, dispatcher()).WillRepeatedly(ReturnRef(*dispatcher_)); - ValidationResults results = validator.doVerifyCertChain( - *cert_chain, std::move(callback_), &ssl_extended_info_, transport_socket_options_, *ssl_ctx_, - validation_context_, is_server_, hostname); + ValidationResults results = + validator.doVerifyCertChain(*cert_chain, std::move(callback_), transport_socket_options_, + *ssl_ctx_, validation_context_, is_server_, hostname); EXPECT_EQ(ValidationResults::ValidationStatus::Pending, results.status); - EXPECT_CALL(callback_ref, onCertValidationResult(true, "", 46)).WillOnce(Invoke([this]() { - EXPECT_EQ(main_thread_id_, std::this_thread::get_id()); - dispatcher_->exit(); - })); + EXPECT_CALL(callback_ref, + onCertValidationResult(true, Envoy::Ssl::ClientValidationStatus::Validated, "", 46)) + .WillOnce(Invoke([this]() { + EXPECT_EQ(main_thread_id_, std::this_thread::get_id()); + dispatcher_->exit(); + })); EXPECT_FALSE(waitForDispatcherToExit()); } @@ -236,14 +237,14 @@ TEST_P(PlatformBridgeCertValidatorTest, ValidCertificateButInvalidSni) { auto& callback_ref = *callback_; EXPECT_CALL(callback_ref, dispatcher()).WillRepeatedly(ReturnRef(*dispatcher_)); - ValidationResults results = validator.doVerifyCertChain( - *cert_chain, std::move(callback_), &ssl_extended_info_, transport_socket_options_, *ssl_ctx_, - validation_context_, is_server_, hostname); + ValidationResults results = + validator.doVerifyCertChain(*cert_chain, std::move(callback_), transport_socket_options_, + *ssl_ctx_, validation_context_, is_server_, hostname); EXPECT_EQ(ValidationResults::ValidationStatus::Pending, results.status); EXPECT_CALL(callback_ref, onCertValidationResult( - acceptInvalidCertificates(), + acceptInvalidCertificates(), Envoy::Ssl::ClientValidationStatus::Failed, "PlatformBridgeCertValidator_verifySubjectAltName failed: SNI mismatch.", SSL_AD_BAD_CERTIFICATE)) .WillOnce(Invoke([this]() { dispatcher_->exit(); })); @@ -268,15 +269,15 @@ TEST_P(PlatformBridgeCertValidatorTest, ValidCertificateSniOverride) { transport_socket_options_ = std::make_shared("", std::move(subject_alt_names)); - ValidationResults results = validator.doVerifyCertChain( - *cert_chain, std::move(callback_), &ssl_extended_info_, transport_socket_options_, *ssl_ctx_, - validation_context_, is_server_, hostname); + ValidationResults results = + validator.doVerifyCertChain(*cert_chain, std::move(callback_), transport_socket_options_, + *ssl_ctx_, validation_context_, is_server_, hostname); EXPECT_EQ(ValidationResults::ValidationStatus::Pending, results.status); // The cert will be validated against the overridden name not the invalid name "server2". - EXPECT_CALL(callback_ref, onCertValidationResult(true, "", 46)).WillOnce(Invoke([this]() { - dispatcher_->exit(); - })); + EXPECT_CALL(callback_ref, + onCertValidationResult(true, Envoy::Ssl::ClientValidationStatus::Validated, "", 46)) + .WillOnce(Invoke([this]() { dispatcher_->exit(); })); EXPECT_FALSE(waitForDispatcherToExit()); } @@ -294,9 +295,9 @@ TEST_P(PlatformBridgeCertValidatorTest, DeletedWithValidationPending) { auto& callback_ref = *callback_; EXPECT_CALL(callback_ref, dispatcher()).WillRepeatedly(ReturnRef(*dispatcher_)); - ValidationResults results = validator->doVerifyCertChain( - *cert_chain, std::move(callback_), &ssl_extended_info_, transport_socket_options_, *ssl_ctx_, - validation_context_, is_server_, hostname); + ValidationResults results = + validator->doVerifyCertChain(*cert_chain, std::move(callback_), transport_socket_options_, + *ssl_ctx_, validation_context_, is_server_, hostname); EXPECT_EQ(ValidationResults::ValidationStatus::Pending, results.status); validator.reset(); diff --git a/source/common/quic/envoy_quic_proof_verifier.cc b/source/common/quic/envoy_quic_proof_verifier.cc index 2c243c5b581d..0f2404e90132 100644 --- a/source/common/quic/envoy_quic_proof_verifier.cc +++ b/source/common/quic/envoy_quic_proof_verifier.cc @@ -41,8 +41,8 @@ class QuicValidateResultCallback : public Ssl::ValidateResultCallback { Event::Dispatcher& dispatcher() override { return dispatcher_; } - void onCertValidationResult(bool succeeded, const std::string& error_details, - uint8_t /*tls_alert*/) override { + void onCertValidationResult(bool succeeded, Ssl::ClientValidationStatus /*detailed_status*/, + const std::string& error_details, uint8_t /*tls_alert*/) override { if (!succeeded) { std::unique_ptr details = std::make_unique(false); quic_callback_->Run(succeeded, error_details, &details); diff --git a/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h b/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h index fa1ec346d090..0034cc7dff02 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h +++ b/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h @@ -34,6 +34,9 @@ struct ValidationResults { // If the value is Pending, the validation is asynchronous. // If the value is Failed, refer to tls_alert and error_details for detailed error messages. ValidationStatus status; + // Detailed status of the underlying validation. Depending on the validation configuration, + // `status` may be valid but `detailed_status` might not be. + Envoy::Ssl::ClientValidationStatus detailed_status; // The TLS alert used to interpret validation error if the validation failed. absl::optional tls_alert; // The detailed error messages populated during validation. @@ -76,8 +79,6 @@ class CertValidator { * @param callback called after the asynchronous validation finishes to handle the result. Must * outlive this call if it returns Pending. Not used if doing synchronous verification. If not * provided and the validation is asynchronous, ssl_extended_info will create one. - * @param ssl_extended_info the info for creating async validation result callback if needed, - * tracking the validation and storing the result. * @param transport_socket_options config options to validate cert, might short live the * validation if it is asynchronous. * @param ssl_ctx the config context this validation should use. @@ -87,7 +88,6 @@ class CertValidator { */ virtual ValidationResults doVerifyCertChain(STACK_OF(X509)& cert_chain, Ssl::ValidateResultCallbackPtr callback, - Ssl::SslExtendedSocketInfo* ssl_extended_info, const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options, SSL_CTX& ssl_ctx, const ExtraValidationContext& validation_context, bool is_server, absl::string_view host_name) PURE; diff --git a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc index 30866141b468..06f4e9a0fef0 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc +++ b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc @@ -211,8 +211,14 @@ int DefaultCertValidator::doSynchronousVerifyCertChain( return allow_untrusted_certificate_ ? 1 : ret; } } - if (!verifyCertAndUpdateStatus(ssl_extended_info, &leaf_cert, transport_socket_options, nullptr, - nullptr)) { + Envoy::Ssl::ClientValidationStatus detailed_status = + Envoy::Ssl::ClientValidationStatus::NotValidated; + bool success = verifyCertAndUpdateStatus(&leaf_cert, transport_socket_options, detailed_status, + nullptr, nullptr); + if (ssl_extended_info) { + ssl_extended_info->setCertificateValidationStatus(detailed_status); + } + if (!success) { X509_STORE_CTX_set_error(store_ctx, X509_V_ERR_APPLICATION_VERIFICATION); return 0; } @@ -220,8 +226,8 @@ int DefaultCertValidator::doSynchronousVerifyCertChain( } bool DefaultCertValidator::verifyCertAndUpdateStatus( - Ssl::SslExtendedSocketInfo* ssl_extended_info, X509* leaf_cert, - const Network::TransportSocketOptions* transport_socket_options, std::string* error_details, + X509* leaf_cert, const Network::TransportSocketOptions* transport_socket_options, + Envoy::Ssl::ClientValidationStatus& detailed_status, std::string* error_details, uint8_t* out_alert) { Envoy::Ssl::ClientValidationStatus validated = verifyCertificate(leaf_cert, @@ -230,13 +236,9 @@ bool DefaultCertValidator::verifyCertAndUpdateStatus( : std::vector{}, subject_alt_name_matchers_, error_details, out_alert); - if (ssl_extended_info) { - if (ssl_extended_info->certificateValidationStatus() == - Envoy::Ssl::ClientValidationStatus::NotValidated) { - ssl_extended_info->setCertificateValidationStatus(validated); - } else if (validated != Envoy::Ssl::ClientValidationStatus::NotValidated) { - ssl_extended_info->setCertificateValidationStatus(validated); - } + if (detailed_status == Envoy::Ssl::ClientValidationStatus::NotValidated || + validated != Envoy::Ssl::ClientValidationStatus::NotValidated) { + detailed_status = validated; } // If `trusted_ca` exists, it is already verified in the code above. Thus, we just need to make @@ -310,20 +312,18 @@ DefaultCertValidator::verifyCertificate(X509* cert, const std::vectorsetCertificateValidationStatus( - Envoy::Ssl::ClientValidationStatus::NotValidated); - } stats_.fail_verify_error_.inc(); const char* error = "verify cert failed: empty cert chain"; ENVOY_LOG(debug, error); - return {ValidationResults::ValidationStatus::Failed, absl::nullopt, error}; + return {ValidationResults::ValidationStatus::Failed, + Envoy::Ssl::ClientValidationStatus::NotValidated, absl::nullopt, error}; } + Envoy::Ssl::ClientValidationStatus detailed_status = + Envoy::Ssl::ClientValidationStatus::NotValidated; X509* leaf_cert = sk_X509_value(&cert_chain, 0); ASSERT(leaf_cert); if (verify_trusted_ca_) { @@ -340,44 +340,37 @@ ValidationResults DefaultCertValidator::doVerifyCertChain( SSL_CTX_get0_param(&ssl_ctx))) { OPENSSL_PUT_ERROR(SSL, ERR_R_X509_LIB); const char* error = "verify cert failed: init and setup X509_STORE_CTX"; - onVerifyError(ssl_extended_info, error); - return {ValidationResults::ValidationStatus::Failed, absl::nullopt, error}; + stats_.fail_verify_error_.inc(); + ENVOY_LOG(debug, error); + return {ValidationResults::ValidationStatus::Failed, + Envoy::Ssl::ClientValidationStatus::Failed, absl::nullopt, error}; } const bool verify_succeeded = (X509_verify_cert(ctx.get()) == 1); if (!verify_succeeded) { const std::string error = absl::StrCat("verify cert failed: ", Utility::getX509VerificationErrorInfo(ctx.get())); - onVerifyError(ssl_extended_info, error); + stats_.fail_verify_error_.inc(); + ENVOY_LOG(debug, error); if (allow_untrusted_certificate_) { - return ValidationResults{ValidationResults::ValidationStatus::Successful, absl::nullopt, + return ValidationResults{ValidationResults::ValidationStatus::Successful, + Envoy::Ssl::ClientValidationStatus::Failed, absl::nullopt, absl::nullopt}; } return {ValidationResults::ValidationStatus::Failed, + Envoy::Ssl::ClientValidationStatus::Failed, SSL_alert_from_verify_result(X509_STORE_CTX_get_error(ctx.get())), error}; } - if (ssl_extended_info) { - ssl_extended_info->setCertificateValidationStatus( - Envoy::Ssl::ClientValidationStatus::Validated); - } + detailed_status = Envoy::Ssl::ClientValidationStatus::Validated; } std::string error_details; uint8_t tls_alert = SSL_AD_CERTIFICATE_UNKNOWN; - const bool succeeded = verifyCertAndUpdateStatus( - ssl_extended_info, leaf_cert, transport_socket_options.get(), &error_details, &tls_alert); + const bool succeeded = verifyCertAndUpdateStatus(leaf_cert, transport_socket_options.get(), + detailed_status, &error_details, &tls_alert); return succeeded ? ValidationResults{ValidationResults::ValidationStatus::Successful, - absl::nullopt, absl::nullopt} - : ValidationResults{ValidationResults::ValidationStatus::Failed, tls_alert, - error_details}; -} - -void DefaultCertValidator::onVerifyError(Ssl::SslExtendedSocketInfo* ssl_extended_info, - absl::string_view error) { - if (ssl_extended_info) { - ssl_extended_info->setCertificateValidationStatus(Envoy::Ssl::ClientValidationStatus::Failed); - } - stats_.fail_verify_error_.inc(); - ENVOY_LOG(debug, error); + detailed_status, absl::nullopt, absl::nullopt} + : ValidationResults{ValidationResults::ValidationStatus::Failed, detailed_status, + tls_alert, error_details}; } bool DefaultCertValidator::verifySubjectAltName(X509* cert, diff --git a/source/extensions/transport_sockets/tls/cert_validator/default_validator.h b/source/extensions/transport_sockets/tls/cert_validator/default_validator.h index 3f9ad85ff8b0..06ad4b7f887e 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/default_validator.h +++ b/source/extensions/transport_sockets/tls/cert_validator/default_validator.h @@ -48,7 +48,6 @@ class DefaultCertValidator : public CertValidator, Logger::Loggable& subject_alt_name_matchers); private: - bool verifyCertAndUpdateStatus(Ssl::SslExtendedSocketInfo* ssl_extended_info, X509* leaf_cert, + bool verifyCertAndUpdateStatus(X509* leaf_cert, const Network::TransportSocketOptions* transport_socket_options, + Envoy::Ssl::ClientValidationStatus& detailed_status, std::string* error_details, uint8_t* out_alert); - void onVerifyError(Ssl::SslExtendedSocketInfo* ssl_extended_info, absl::string_view error); - const Envoy::Ssl::CertificateValidationContextConfig* config_; SslStats& stats_; TimeSource& time_source_; diff --git a/source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.cc b/source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.cc index 9b281de8c238..b1b001238fae 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.cc +++ b/source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.cc @@ -148,19 +148,21 @@ int SPIFFEValidator::doSynchronousVerifyCertChain(X509_STORE_CTX* store_ctx, STACK_OF(X509)* cert_chain = X509_STORE_CTX_get0_untrusted(store_ctx); X509_VERIFY_PARAM* verify_param = X509_STORE_CTX_get0_param(store_ctx); std::string error_details; - return verifyCertChainUsingTrustBundleStore(ssl_extended_info, leaf_cert, cert_chain, - verify_param, error_details) - ? 1 - : 0; + bool verified = + verifyCertChainUsingTrustBundleStore(leaf_cert, cert_chain, verify_param, error_details); + if (ssl_extended_info) { + ssl_extended_info->setCertificateValidationStatus( + verified ? Envoy::Ssl::ClientValidationStatus::Validated + : Envoy::Ssl::ClientValidationStatus::Failed); + } + return verified ? 1 : 0; } -bool SPIFFEValidator::verifyCertChainUsingTrustBundleStore( - Ssl::SslExtendedSocketInfo* ssl_extended_info, X509& leaf_cert, STACK_OF(X509)* cert_chain, - X509_VERIFY_PARAM* verify_param, std::string& error_details) { +bool SPIFFEValidator::verifyCertChainUsingTrustBundleStore(X509& leaf_cert, + STACK_OF(X509)* cert_chain, + X509_VERIFY_PARAM* verify_param, + std::string& error_details) { if (!SPIFFEValidator::certificatePrecheck(&leaf_cert)) { - if (ssl_extended_info) { - ssl_extended_info->setCertificateValidationStatus(Envoy::Ssl::ClientValidationStatus::Failed); - } error_details = "verify cert failed: cert precheck"; stats_.fail_verify_error_.inc(); return false; @@ -168,9 +170,6 @@ bool SPIFFEValidator::verifyCertChainUsingTrustBundleStore( auto trust_bundle = getTrustBundleStore(&leaf_cert); if (!trust_bundle) { - if (ssl_extended_info) { - ssl_extended_info->setCertificateValidationStatus(Envoy::Ssl::ClientValidationStatus::Failed); - } error_details = "verify cert failed: no trust bundle store"; stats_.fail_verify_error_.inc(); return false; @@ -189,9 +188,6 @@ bool SPIFFEValidator::verifyCertChainUsingTrustBundleStore( } auto ret = X509_verify_cert(new_store_ctx.get()); if (!ret) { - if (ssl_extended_info) { - ssl_extended_info->setCertificateValidationStatus(Envoy::Ssl::ClientValidationStatus::Failed); - } error_details = absl::StrCat("verify cert failed: ", Utility::getX509VerificationErrorInfo(new_store_ctx.get())); stats_.fail_verify_error_.inc(); @@ -204,37 +200,30 @@ bool SPIFFEValidator::verifyCertChainUsingTrustBundleStore( error_details = "verify cert failed: SAN match"; stats_.fail_verify_san_.inc(); } - if (ssl_extended_info) { - ssl_extended_info->setCertificateValidationStatus( - san_match ? Envoy::Ssl::ClientValidationStatus::Validated - : Envoy::Ssl::ClientValidationStatus::Failed); - } return san_match; } ValidationResults SPIFFEValidator::doVerifyCertChain( STACK_OF(X509)& cert_chain, Ssl::ValidateResultCallbackPtr /*callback*/, - Ssl::SslExtendedSocketInfo* ssl_extended_info, const Network::TransportSocketOptionsConstSharedPtr& /*transport_socket_options*/, SSL_CTX& ssl_ctx, const CertValidator::ExtraValidationContext& /*validation_context*/, bool /*is_server*/, absl::string_view /*host_name*/) { if (sk_X509_num(&cert_chain) == 0) { - if (ssl_extended_info) { - ssl_extended_info->setCertificateValidationStatus( - Envoy::Ssl::ClientValidationStatus::NotValidated); - } stats_.fail_verify_error_.inc(); - return {ValidationResults::ValidationStatus::Failed, absl::nullopt, + return {ValidationResults::ValidationStatus::Failed, + Envoy::Ssl::ClientValidationStatus::NotValidated, absl::nullopt, "verify cert failed: empty cert chain"}; } X509* leaf_cert = sk_X509_value(&cert_chain, 0); std::string error_details; - return verifyCertChainUsingTrustBundleStore(ssl_extended_info, *leaf_cert, &cert_chain, - SSL_CTX_get0_param(&ssl_ctx), error_details) - ? ValidationResults{ValidationResults::ValidationStatus::Successful, absl::nullopt, - absl::nullopt} - : ValidationResults{ValidationResults::ValidationStatus::Failed, absl::nullopt, - error_details}; + bool verified = verifyCertChainUsingTrustBundleStore(*leaf_cert, &cert_chain, + SSL_CTX_get0_param(&ssl_ctx), error_details); + return verified ? ValidationResults{ValidationResults::ValidationStatus::Successful, + Envoy::Ssl::ClientValidationStatus::Validated, absl::nullopt, + absl::nullopt} + : ValidationResults{ValidationResults::ValidationStatus::Failed, + Envoy::Ssl::ClientValidationStatus::Failed, absl::nullopt, + error_details}; } X509_STORE* SPIFFEValidator::getTrustBundleStore(X509* leaf_cert) { diff --git a/source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.h b/source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.h index 122f6d56b6af..d252620d2e02 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.h +++ b/source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.h @@ -46,7 +46,6 @@ class SPIFFEValidator : public CertValidator { const Network::TransportSocketOptions* transport_socket_options) override; ValidationResults doVerifyCertChain(STACK_OF(X509)& cert_chain, Ssl::ValidateResultCallbackPtr callback, - Ssl::SslExtendedSocketInfo* ssl_extended_info, const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options, SSL_CTX& ssl_ctx, const CertValidator::ExtraValidationContext& validation_context, bool is_server, @@ -72,8 +71,7 @@ class SPIFFEValidator : public CertValidator { bool matchSubjectAltName(X509& leaf_cert); private: - bool verifyCertChainUsingTrustBundleStore(Ssl::SslExtendedSocketInfo* ssl_extended_info, - X509& leaf_cert, STACK_OF(X509)* cert_chain, + bool verifyCertChainUsingTrustBundleStore(X509& leaf_cert, STACK_OF(X509)* cert_chain, X509_VERIFY_PARAM* verify_param, std::string& error_details); diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index d80649fd069e..062694496453 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -505,16 +505,16 @@ ValidationResults ContextImpl::customVerifyCertChain( extended_socket_info->setCertificateValidationStatus(Ssl::ClientValidationStatus::NotValidated); stats_.fail_verify_error_.inc(); ENVOY_LOG(debug, "verify cert failed: no cert chain"); - return {ValidationResults::ValidationStatus::Failed, SSL_AD_INTERNAL_ERROR, absl::nullopt}; + return {ValidationResults::ValidationStatus::Failed, Ssl::ClientValidationStatus::NotValidated, + SSL_AD_INTERNAL_ERROR, absl::nullopt}; } ASSERT(cert_validator_); const char* host_name = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); - // Do not provide async callback here, but defer its creation to extended_socket_info if the - // validation is async. ValidationResults result = cert_validator_->doVerifyCertChain( - *cert_chain, nullptr, extended_socket_info, transport_socket_options, *SSL_get_SSL_CTX(ssl), - {}, SSL_is_server(ssl), absl::NullSafeStringView(host_name)); + *cert_chain, extended_socket_info->createValidateResultCallback(), transport_socket_options, + *SSL_get_SSL_CTX(ssl), {}, SSL_is_server(ssl), absl::NullSafeStringView(host_name)); if (result.status != ValidationResults::ValidationStatus::Pending) { + extended_socket_info->setCertificateValidationStatus(result.detailed_status); extended_socket_info->onCertificateValidationCompleted( result.status == ValidationResults::ValidationStatus::Successful); } @@ -1405,11 +1405,12 @@ ValidationResults ContextImpl::customVerifyCertChainForQuic( SSL_CTX* ssl_ctx = tls_contexts_[0].ssl_ctx_.get(); if (SSL_CTX_get_verify_mode(ssl_ctx) == SSL_VERIFY_NONE) { // Skip validation if the TLS is configured SSL_VERIFY_NONE. - return {ValidationResults::ValidationStatus::Successful, absl::nullopt, absl::nullopt}; + return {ValidationResults::ValidationStatus::Successful, + Envoy::Ssl::ClientValidationStatus::NotValidated, absl::nullopt, absl::nullopt}; } - ValidationResults result = cert_validator_->doVerifyCertChain( - cert_chain, std::move(callback), /*extended_socket_info=*/nullptr, transport_socket_options, - *ssl_ctx, validation_context, is_server, host_name); + ValidationResults result = + cert_validator_->doVerifyCertChain(cert_chain, std::move(callback), transport_socket_options, + *ssl_ctx, validation_context, is_server, host_name); return result; } diff --git a/source/extensions/transport_sockets/tls/ssl_handshaker.cc b/source/extensions/transport_sockets/tls/ssl_handshaker.cc index adb3176050e9..3528f5f5f1df 100644 --- a/source/extensions/transport_sockets/tls/ssl_handshaker.cc +++ b/source/extensions/transport_sockets/tls/ssl_handshaker.cc @@ -18,13 +18,13 @@ namespace Tls { void ValidateResultCallbackImpl::onSslHandshakeCancelled() { extended_socket_info_.reset(); } void ValidateResultCallbackImpl::onCertValidationResult(bool succeeded, + Ssl::ClientValidationStatus detailed_status, const std::string& /*error_details*/, uint8_t tls_alert) { if (!extended_socket_info_.has_value()) { return; } - extended_socket_info_->setCertificateValidationStatus( - succeeded ? Ssl::ClientValidationStatus::Validated : Ssl::ClientValidationStatus::Failed); + extended_socket_info_->setCertificateValidationStatus(detailed_status); extended_socket_info_->setCertificateValidationAlert(tls_alert); extended_socket_info_->onCertificateValidationCompleted(succeeded); } diff --git a/source/extensions/transport_sockets/tls/ssl_handshaker.h b/source/extensions/transport_sockets/tls/ssl_handshaker.h index 47119eb46489..56b05ed94d1e 100644 --- a/source/extensions/transport_sockets/tls/ssl_handshaker.h +++ b/source/extensions/transport_sockets/tls/ssl_handshaker.h @@ -40,8 +40,8 @@ class ValidateResultCallbackImpl : public Ssl::ValidateResultCallback { Event::Dispatcher& dispatcher() override { return dispatcher_; } - void onCertValidationResult(bool succeeded, const std::string& error_details, - uint8_t tls_alert) override; + void onCertValidationResult(bool succeeded, Ssl::ClientValidationStatus detailed_status, + const std::string& error_details, uint8_t tls_alert) override; void onSslHandshakeCancelled(); diff --git a/test/extensions/transport_sockets/tls/cert_validator/default_validator_test.cc b/test/extensions/transport_sockets/tls/cert_validator/default_validator_test.cc index 26ac3c4648e3..54ce3db9f7fc 100644 --- a/test/extensions/transport_sockets/tls/cert_validator/default_validator_test.cc +++ b/test/extensions/transport_sockets/tls/cert_validator/default_validator_test.cc @@ -192,7 +192,6 @@ TEST(DefaultCertValidatorTest, TestCertificateVerificationWithNoValidationContex EXPECT_EQ(ValidationResults::ValidationStatus::Failed, default_validator ->doVerifyCertChain(*cert_chain, /*callback=*/nullptr, - /*ssl_extended_info=*/nullptr, /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") .status); } @@ -209,13 +208,11 @@ TEST(DefaultCertValidatorTest, TestCertificateVerificationWithEmptyCertChain) { SSLContextPtr ssl_ctx = SSL_CTX_new(TLS_method()); bssl::UniquePtr cert_chain(sk_X509_new_null()); TestSslExtendedSocketInfo extended_socket_info; - EXPECT_EQ(ValidationResults::ValidationStatus::Failed, - default_validator - ->doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &extended_socket_info, - /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") - .status); - EXPECT_EQ(extended_socket_info.certificateValidationStatus(), - Ssl::ClientValidationStatus::NotValidated); + ValidationResults results = default_validator->doVerifyCertChain( + *cert_chain, /*callback=*/nullptr, + /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, ""); + EXPECT_EQ(ValidationResults::ValidationStatus::Failed, results.status); + EXPECT_EQ(Ssl::ClientValidationStatus::NotValidated, results.detailed_status); } TEST(DefaultCertValidatorTest, NoSanInCert) { diff --git a/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_test.cc b/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_test.cc index 15abe8b7973b..906e297f74a3 100644 --- a/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_test.cc +++ b/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_test.cc @@ -241,13 +241,12 @@ TEST_P(TestSPIFFEValidator, TestDoVerifyCertChainWithEmptyChain) { TestSslExtendedSocketInfo info; SSLContextPtr ssl_ctx = SSL_CTX_new(TLS_method()); bssl::UniquePtr cert_chain(sk_X509_new_null()); - EXPECT_EQ(ValidationResults::ValidationStatus::Failed, - validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, - /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") - .status); + ValidationResults results = + validator().doVerifyCertChain(*cert_chain, info.createValidateResultCallback(), + /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, ""); + EXPECT_EQ(ValidationResults::ValidationStatus::Failed, results.status); + EXPECT_EQ(Envoy::Ssl::ClientValidationStatus::NotValidated, results.detailed_status); EXPECT_EQ(1, stats().fail_verify_error_.value()); - EXPECT_EQ(info.certificateValidationStatus(), Envoy::Ssl::ClientValidationStatus::NotValidated); } TEST_P(TestSPIFFEValidator, TestDoVerifyCertChainPrecheckFailure) { @@ -260,17 +259,17 @@ TEST_P(TestSPIFFEValidator, TestDoVerifyCertChainPrecheckFailure) { SSLContextPtr ssl_ctx = SSL_CTX_new(TLS_method()); bssl::UniquePtr cert_chain(sk_X509_new_null()); sk_X509_push(cert_chain.get(), cert.release()); - EXPECT_EQ(ValidationResults::ValidationStatus::Failed, - validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, - /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") - .status); + ValidationResults results = validator().doVerifyCertChain( + *cert_chain, info.createValidateResultCallback(), + /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, ""); + EXPECT_EQ(ValidationResults::ValidationStatus::Failed, results.status); + EXPECT_EQ(Envoy::Ssl::ClientValidationStatus::Failed, results.detailed_status); } else { X509StoreContextPtr store_ctx = X509_STORE_CTX_new(); EXPECT_FALSE(validator().doSynchronousVerifyCertChain(store_ctx.get(), &info, *cert, nullptr)); + EXPECT_EQ(info.certificateValidationStatus(), Envoy::Ssl::ClientValidationStatus::Failed); } EXPECT_EQ(1, stats().fail_verify_error_.value()); - EXPECT_EQ(info.certificateValidationStatus(), Envoy::Ssl::ClientValidationStatus::Failed); } TEST_P(TestSPIFFEValidator, TestDoVerifyCertChainSingleTrustDomain) { @@ -295,7 +294,7 @@ name: envoy.tls.cert_validator.spiffe sk_X509_push(cert_chain.get(), cert.release()); EXPECT_EQ(ValidationResults::ValidationStatus::Successful, validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, + .doVerifyCertChain(*cert_chain, info.createValidateResultCallback(), /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") .status); } else { @@ -312,7 +311,7 @@ name: envoy.tls.cert_validator.spiffe sk_X509_push(cert_chain.get(), cert.release()); EXPECT_EQ(ValidationResults::ValidationStatus::Failed, validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, + .doVerifyCertChain(*cert_chain, info.createValidateResultCallback(), /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") .status); } else { @@ -330,7 +329,7 @@ name: envoy.tls.cert_validator.spiffe sk_X509_push(cert_chain.get(), cert.release()); EXPECT_EQ(ValidationResults::ValidationStatus::Failed, validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, + .doVerifyCertChain(*cert_chain, info.createValidateResultCallback(), /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") .status); } else { @@ -369,7 +368,7 @@ name: envoy.tls.cert_validator.spiffe sk_X509_push(cert_chain.get(), cert.release()); EXPECT_EQ(ValidationResults::ValidationStatus::Successful, validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, + .doVerifyCertChain(*cert_chain, info.createValidateResultCallback(), /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") .status); } else { @@ -385,7 +384,7 @@ name: envoy.tls.cert_validator.spiffe sk_X509_push(cert_chain.get(), cert.release()); EXPECT_EQ(ValidationResults::ValidationStatus::Successful, validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, + .doVerifyCertChain(*cert_chain, info.createValidateResultCallback(), /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") .status); } else { @@ -403,7 +402,7 @@ name: envoy.tls.cert_validator.spiffe sk_X509_push(cert_chain.get(), cert.release()); EXPECT_EQ(ValidationResults::ValidationStatus::Failed, validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, + .doVerifyCertChain(*cert_chain, info.createValidateResultCallback(), /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") .status); } else { @@ -421,7 +420,7 @@ name: envoy.tls.cert_validator.spiffe sk_X509_push(cert_chain.get(), cert.release()); EXPECT_EQ(ValidationResults::ValidationStatus::Failed, validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, + .doVerifyCertChain(*cert_chain, info.createValidateResultCallback(), /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") .status); } else { @@ -459,7 +458,7 @@ name: envoy.tls.cert_validator.spiffe sk_X509_push(cert_chain.get(), cert.release()); EXPECT_EQ(ValidationResults::ValidationStatus::Successful, validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, + .doVerifyCertChain(*cert_chain, info.createValidateResultCallback(), /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") .status); } else { @@ -501,16 +500,15 @@ name: envoy.tls.cert_validator.spiffe setSanMatchers({matcher}); initialize(config); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.tls_async_cert_validation")) { - EXPECT_EQ(ValidationResults::ValidationStatus::Successful, - validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, - /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, - "") - .status); + ValidationResults results = validator().doVerifyCertChain( + *cert_chain, info.createValidateResultCallback(), + /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, ""); + EXPECT_EQ(ValidationResults::ValidationStatus::Successful, results.status); + EXPECT_EQ(Envoy::Ssl::ClientValidationStatus::Validated, results.detailed_status); } else { EXPECT_TRUE(validator().doSynchronousVerifyCertChain(store_ctx.get(), &info, *cert, nullptr)); + EXPECT_EQ(info.certificateValidationStatus(), Envoy::Ssl::ClientValidationStatus::Validated); } - EXPECT_EQ(info.certificateValidationStatus(), Envoy::Ssl::ClientValidationStatus::Validated); } { envoy::type::matcher::v3::StringMatcher matcher; @@ -518,17 +516,16 @@ name: envoy.tls.cert_validator.spiffe setSanMatchers({matcher}); initialize(config); if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.tls_async_cert_validation")) { - EXPECT_EQ(ValidationResults::ValidationStatus::Failed, - validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, - /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, - "") - .status); + ValidationResults results = validator().doVerifyCertChain( + *cert_chain, info.createValidateResultCallback(), + /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, ""); + EXPECT_EQ(ValidationResults::ValidationStatus::Failed, results.status); + EXPECT_EQ(Envoy::Ssl::ClientValidationStatus::Failed, results.detailed_status); } else { EXPECT_FALSE( validator().doSynchronousVerifyCertChain(store_ctx.get(), &info, *cert, nullptr)); + EXPECT_EQ(info.certificateValidationStatus(), Envoy::Ssl::ClientValidationStatus::Failed); } - EXPECT_EQ(info.certificateValidationStatus(), Envoy::Ssl::ClientValidationStatus::Failed); EXPECT_EQ(1, stats().fail_verify_san_.value()); stats().fail_verify_san_.reset(); } @@ -561,7 +558,7 @@ name: envoy.tls.cert_validator.spiffe sk_X509_push(cert_chain.get(), intermediate_ca_cert.release()); EXPECT_EQ(ValidationResults::ValidationStatus::Successful, validator() - .doVerifyCertChain(*cert_chain, /*callback=*/nullptr, &info, + .doVerifyCertChain(*cert_chain, info.createValidateResultCallback(), /*transport_socket_options=*/nullptr, *ssl_ctx, {}, false, "") .status); } else { diff --git a/test/extensions/transport_sockets/tls/cert_validator/timed_cert_validator.cc b/test/extensions/transport_sockets/tls/cert_validator/timed_cert_validator.cc index 7675970802b5..2f83bf14fd13 100644 --- a/test/extensions/transport_sockets/tls/cert_validator/timed_cert_validator.cc +++ b/test/extensions/transport_sockets/tls/cert_validator/timed_cert_validator.cc @@ -13,14 +13,10 @@ namespace Tls { ValidationResults TimedCertValidator::doVerifyCertChain( STACK_OF(X509)& cert_chain, Ssl::ValidateResultCallbackPtr callback, - Ssl::SslExtendedSocketInfo* ssl_extended_info, const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options, SSL_CTX& ssl_ctx, const CertValidator::ExtraValidationContext& /*validation_context*/, bool is_server, absl::string_view host_name) { - if (callback == nullptr) { - ASSERT(ssl_extended_info); - callback = ssl_extended_info->createValidateResultCallback(); - } + ASSERT(callback != nullptr); ASSERT(callback_ == nullptr); callback_ = std::move(callback); if (expected_host_name_.has_value()) { @@ -47,14 +43,16 @@ ValidationResults TimedCertValidator::doVerifyCertChain( } } ValidationResults result = DefaultCertValidator::doVerifyCertChain( - *certs, nullptr, nullptr, transport_socket_options, ssl_ctx, {}, is_server, host); + *certs, nullptr, transport_socket_options, ssl_ctx, {}, is_server, host); callback_->onCertValidationResult( result.status == ValidationResults::ValidationStatus::Successful, + result.detailed_status, (result.error_details.has_value() ? result.error_details.value() : ""), (result.tls_alert.has_value() ? result.tls_alert.value() : SSL_AD_CERTIFICATE_UNKNOWN)); }); validation_timer_->enableTimer(validation_time_out_ms_); - return {ValidationResults::ValidationStatus::Pending, absl::nullopt, absl::nullopt}; + return {ValidationResults::ValidationStatus::Pending, + Envoy::Ssl::ClientValidationStatus::NotValidated, absl::nullopt, absl::nullopt}; } REGISTER_FACTORY(TimedCertValidatorFactory, CertValidatorFactory); diff --git a/test/extensions/transport_sockets/tls/cert_validator/timed_cert_validator.h b/test/extensions/transport_sockets/tls/cert_validator/timed_cert_validator.h index 6d3c7ef382d2..890a76a48c6e 100644 --- a/test/extensions/transport_sockets/tls/cert_validator/timed_cert_validator.h +++ b/test/extensions/transport_sockets/tls/cert_validator/timed_cert_validator.h @@ -29,7 +29,6 @@ class TimedCertValidator : public DefaultCertValidator { ValidationResults doVerifyCertChain(STACK_OF(X509)& cert_chain, Ssl::ValidateResultCallbackPtr callback, - Ssl::SslExtendedSocketInfo* ssl_extended_info, const Network::TransportSocketOptionsConstSharedPtr& transport_socket_options, SSL_CTX& ssl_ctx, const CertValidator::ExtraValidationContext& validation_context, bool is_server, From 1eb014a18ee51ea35ca7216384da6f17585d5384 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Fri, 9 Dec 2022 12:29:31 -0500 Subject: [PATCH 6/7] ci: move mobile docs job to Github Actions (#24447) This is the only use of CircleCI in this repo, so removing this helps us remove it altogether. As part of this change, also stop publishing docs when tags are pushed. Commit Message: Additional Description: Risk Level: Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] Signed-off-by: JP Simard --- .circleci/config.yml | 32 ++++---------------------------- .github/workflows/docs.yml | 31 +++++++++++++++++++++++++++++++ mobile/ci/mac_ci_setup.sh | 6 ------ mobile/docs/build.sh | 10 +++++----- mobile/docs/publish.sh | 10 +++++----- 5 files changed, 45 insertions(+), 44 deletions(-) create mode 100644 .github/workflows/docs.yml diff --git a/.circleci/config.yml b/.circleci/config.yml index 002d8c6ad8a9..70b2da1448ce 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,32 +1,8 @@ version: 2.1 -executors: - ubuntu-build: - description: "A regular build executor based on ubuntu image" - docker: - - image: envoyproxy/envoy-build-ubuntu:b0ff77ae3f25b0bf595f9b8bba46b489723ab446 - # TODO(mattklein123): Get xlarge class enabled - resource_class: medium - working_directory: /source - jobs: - docs: - executor: ubuntu-build + build: + docker: + - image: debian:bullseye-slim steps: - - checkout - - run: mobile/docs/build.sh - - add_ssh_keys: - fingerprints: - - "33:78:4d:5c:bd:62:2e:43:9d:79:2c:3e:dc:45:c0:98" - - run: mobile/docs/publish.sh - - store_artifacts: - path: generated/docs - -workflows: - version: 2 - all: - jobs: - - docs: - filters: - tags: - only: /^v.*/ + - run: echo "no circle on this branch" diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml new file mode 100644 index 000000000000..ea8072202e09 --- /dev/null +++ b/.github/workflows/docs.yml @@ -0,0 +1,31 @@ +name: mobile_docs + +on: + push: + branches: + - main + pull_request: + +jobs: + docs: + runs-on: ubuntu-20.04 + timeout-minutes: 20 + container: + image: envoyproxy/envoy-build-ubuntu:b0ff77ae3f25b0bf595f9b8bba46b489723ab446 + steps: + - uses: actions/checkout@v3 + - name: Add safe directory + run: git config --global --add safe.directory "$GITHUB_WORKSPACE" + - name: Generate docs + run: mobile/docs/build.sh + - name: Set up deploy key + uses: shimataro/ssh-key-action@193316a178ec055fcc7b018f7f76bbf64085c628 + with: + key: ${{ secrets.ENVOY_MOBILE_WEBSITE_DEPLOY_KEY }} + known_hosts: unnecessary + - name: Publish docs + run: mobile/docs/publish.sh + - uses: actions/upload-artifact@v3 + with: + name: docs + path: generated/docs diff --git a/mobile/ci/mac_ci_setup.sh b/mobile/ci/mac_ci_setup.sh index 58d07b29b8f6..e2d6381cc7db 100755 --- a/mobile/ci/mac_ci_setup.sh +++ b/mobile/ci/mac_ci_setup.sh @@ -35,12 +35,6 @@ do is_installed "${DEP}" || install "${DEP}" done -if [ -n "$CIRCLECI" ]; then - # bazel uses jgit internally and the default circle-ci .gitconfig says to - # convert https://github.com to ssh://git@github.com, which jgit does not support. - mv ~/.gitconfig ~/.gitconfig_save -fi - ./bazelw version pip3 install slackclient diff --git a/mobile/docs/build.sh b/mobile/docs/build.sh index 2913d5c6ad8b..141547d3b9e2 100755 --- a/mobile/docs/build.sh +++ b/mobile/docs/build.sh @@ -11,12 +11,12 @@ set -e # Docs for release tags are reserved for vX.Y.Z versions. # vX.Y.Z.ddmmyy do not publish tagged docs. VERSION_NUMBER=$(cat mobile/VERSION) -if [[ -n "$CIRCLE_TAG" ]] && [[ "${VERSION_NUMBER}" =~ ^[0-9]+\.[0-9]+\.[0-9]$ ]] +if [[ "$GITHUB_REF_TYPE" == "tag" ]] && [[ "${VERSION_NUMBER}" =~ ^[0-9]+\.[0-9]+\.[0-9]$ ]] then # Check the git tag matches the version number in the VERSION file. - if [ "v${VERSION_NUMBER}" != "${CIRCLE_TAG}" ]; then + if [ "v${VERSION_NUMBER}" != "${GITHUB_REF_NAME}" ]; then echo "Given git tag does not match the VERSION file content:" - echo "${CIRCLE_TAG} vs $(cat mobile/VERSION)" + echo "${GITHUB_REF_NAME} vs $(cat mobile/VERSION)" exit 1 fi # Check the version_history.rst contains current release version. @@ -24,9 +24,9 @@ then || (echo "Git tag not found in version_history.rst" && exit 1) # Now that we now there is a match, we can use the tag. - export ENVOY_DOCS_VERSION_STRING="tag-$CIRCLE_TAG" + export ENVOY_DOCS_VERSION_STRING="tag-$GITHUB_REF_NAME" export ENVOY_DOCS_RELEASE_LEVEL=tagged - export ENVOY_BLOB_SHA="$CIRCLE_TAG" + export ENVOY_BLOB_SHA="$GITHUB_REF_NAME" else BUILD_SHA=$(git rev-parse HEAD) export ENVOY_DOCS_VERSION_STRING="${VERSION_NUMBER}"-"${BUILD_SHA:0:6}" diff --git a/mobile/docs/publish.sh b/mobile/docs/publish.sh index 521261741665..dc821274f934 100755 --- a/mobile/docs/publish.sh +++ b/mobile/docs/publish.sh @@ -1,7 +1,7 @@ #!/bin/bash -# This is run on every commit that CircleCI picks up. It assumes that docs have already been built -# via docs/build.sh. The push behavior differs depending on the nature of the commit: +# This is run on every commit that GitHub Actions picks up. It assumes that docs have already been +# built via docs/build.sh. The push behavior differs depending on the nature of the commit: # * Tag commit (e.g. v1.6.0): pushes docs to versioned location. # * Main commit: pushes docs to latest. Note that envoy-mobile.github.io uses `master` rather than # `main` because using `main` as the default branch currently results in 404s. @@ -13,10 +13,10 @@ DOCS_DIR=generated/docs CHECKOUT_DIR=../envoy-mobile-docs BUILD_SHA="$(git rev-parse HEAD)" -if [ -n "$CIRCLE_TAG" ] +if [ "$GITHUB_REF_TYPE" == "tag" ] then - PUBLISH_DIR="$CHECKOUT_DIR"/docs/envoy-mobile/"$CIRCLE_TAG" -elif [ -z "$CIRCLE_PULL_REQUEST" ] && [ "$CIRCLE_BRANCH" == "main" ] + PUBLISH_DIR="$CHECKOUT_DIR"/docs/envoy-mobile/"$GITHUB_REF_NAME" +elif [ "$GITHUB_REF_NAME" == "main" ] then PUBLISH_DIR="$CHECKOUT_DIR"/docs/envoy-mobile/latest else From 086bc865801f2be0c0ac937658115798d9e48682 Mon Sep 17 00:00:00 2001 From: JP Simard Date: Fri, 9 Dec 2022 16:32:18 -0500 Subject: [PATCH 7/7] ci: only set up mobile docs deploy key for main repo (#24476) Skip this step for forks, since secrets aren't available to PRs from forks. Signed-off-by: JP Simard --- .github/workflows/docs.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index ea8072202e09..f22cd39703f2 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -19,6 +19,7 @@ jobs: - name: Generate docs run: mobile/docs/build.sh - name: Set up deploy key + if: github.event.pull_request.head.repo.full_name == github.repository uses: shimataro/ssh-key-action@193316a178ec055fcc7b018f7f76bbf64085c628 with: key: ${{ secrets.ENVOY_MOBILE_WEBSITE_DEPLOY_KEY }}