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..f22cd39703f2 --- /dev/null +++ b/.github/workflows/docs.yml @@ -0,0 +1,32 @@ +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 + 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 }} + 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/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/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 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/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/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 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) { 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/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 { 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/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, diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh old mode 100755 new mode 100644