From f6f7ea0c804d5b6804bd21657c827308a2d141cc Mon Sep 17 00:00:00 2001 From: yanavlasov Date: Thu, 17 Feb 2022 11:36:35 -0500 Subject: [PATCH 01/15] bazel: copy .bazelversion for envoy filter examples (#18730) (#20010) This file needs to be copied otherwise bazelisk picks whichever current version is out there which could break with any release Signed-off-by: Keith Smiley Signed-off-by: Yan Avlasov Co-authored-by: Keith Smiley --- ci/filter_example_setup.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ci/filter_example_setup.sh b/ci/filter_example_setup.sh index 5ef74fa49119..2447a79e41d4 100644 --- a/ci/filter_example_setup.sh +++ b/ci/filter_example_setup.sh @@ -25,6 +25,8 @@ sed -e "s|{ENVOY_SRCDIR}|${ENVOY_SRCDIR}|" "${ENVOY_SRCDIR}"/ci/WORKSPACE.filter mkdir -p "${ENVOY_FILTER_EXAMPLE_SRCDIR}"/bazel ln -sf "${ENVOY_SRCDIR}"/bazel/get_workspace_status "${ENVOY_FILTER_EXAMPLE_SRCDIR}"/bazel/ cp -f "${ENVOY_SRCDIR}"/.bazelrc "${ENVOY_FILTER_EXAMPLE_SRCDIR}"/ +rm -f "${ENVOY_FILTER_EXAMPLE_SRCDIR}"/.bazelversion +cp -f "${ENVOY_SRCDIR}"/.bazelversion "${ENVOY_FILTER_EXAMPLE_SRCDIR}"/ cp -f "$(bazel info workspace)"/*.bazelrc "${ENVOY_FILTER_EXAMPLE_SRCDIR}"/ export FILTER_WORKSPACE_SET=1 From ae948a66804564932b350d771a65db5f7069dfb5 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 17 Feb 2022 21:30:46 +0100 Subject: [PATCH 02/15] Kick-off release 1.20.2 (#19886) * Kick-off release 1.20.2 Signed-off-by: Otto van der Schaaf --- VERSION | 2 +- docs/root/version_history/current.rst | 12 ++----- docs/root/version_history/v1.20.1.rst | 32 +++++++++++++++++++ docs/root/version_history/version_history.rst | 1 + 4 files changed, 36 insertions(+), 11 deletions(-) create mode 100644 docs/root/version_history/v1.20.1.rst diff --git a/VERSION b/VERSION index 0044d6cb9691..558dbb08f9e3 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.20.1 +1.20.2-dev diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 5ee3ba7bc0c2..0c1c97cb5d25 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -1,5 +1,5 @@ -1.20.1 (November 30, 2021) -========================== +1.20.2 (Pending) +======================== Incompatible Behavior Changes ----------------------------- @@ -9,18 +9,10 @@ Minor Behavior Changes ---------------------- *Changes that may cause incompatibilities for some users, but should not for most* -* config: the log message for "gRPC config stream closed" now uses the most recent error message, and reports seconds instead of milliseconds for how long the most recent status has been received. - Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* -* http: remove redundant Warn log in HTTP codec. -* listener: fix a crash when updating any listener that does not bind to port. -* listener: listener add can reuse the listener socket of a draining filter chain listener and fix the request lost. -* mac: fix crash on startup on macOS 12 by changing the default allocator. -* tcp: fixed a bug where upstream circuit breakers applied HTTP per-request bounds to TCP connections. - Removed Config or Runtime ------------------------- *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/docs/root/version_history/v1.20.1.rst b/docs/root/version_history/v1.20.1.rst new file mode 100644 index 000000000000..5ee3ba7bc0c2 --- /dev/null +++ b/docs/root/version_history/v1.20.1.rst @@ -0,0 +1,32 @@ +1.20.1 (November 30, 2021) +========================== + +Incompatible Behavior Changes +----------------------------- +*Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* + +Minor Behavior Changes +---------------------- +*Changes that may cause incompatibilities for some users, but should not for most* + +* config: the log message for "gRPC config stream closed" now uses the most recent error message, and reports seconds instead of milliseconds for how long the most recent status has been received. + +Bug Fixes +--------- +*Changes expected to improve the state of the world and are unlikely to have negative effects* + +* http: remove redundant Warn log in HTTP codec. +* listener: fix a crash when updating any listener that does not bind to port. +* listener: listener add can reuse the listener socket of a draining filter chain listener and fix the request lost. +* mac: fix crash on startup on macOS 12 by changing the default allocator. +* tcp: fixed a bug where upstream circuit breakers applied HTTP per-request bounds to TCP connections. + +Removed Config or Runtime +------------------------- +*Normally occurs at the end of the* :ref:`deprecation period ` + +New Features +------------ + +Deprecated +---------- diff --git a/docs/root/version_history/version_history.rst b/docs/root/version_history/version_history.rst index 7cca61970b5b..006173c8ade0 100644 --- a/docs/root/version_history/version_history.rst +++ b/docs/root/version_history/version_history.rst @@ -7,6 +7,7 @@ Version history :titlesonly: current + v1.20.1 v1.20.0 v1.19.1 v1.19.0 From f46c19d99cd7186781b6391cec310b93f76fae6b Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 18 Feb 2022 20:45:23 +0100 Subject: [PATCH 03/15] Fix for X.509 Extended Key Usage and Trust Purposes bypass (#20032) Backport for v1.20 of 351c0ca Signed-off-by: Otto van der Schaaf --- .../transport_sockets/tls/context_impl.cc | 8 ++ .../transport_sockets/tls/ssl_socket.cc | 2 +- .../transport_sockets/tls/ssl_socket.h | 3 - .../quic/envoy_quic_proof_verifier_test.cc | 80 ++++++++++++++++++- .../transport_sockets/tls/ssl_socket_test.cc | 7 +- tools/spelling/spelling_dictionary.txt | 3 + 6 files changed, 96 insertions(+), 7 deletions(-) diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc index 38ff08d9f51b..0afc83d5a67a 100644 --- a/source/extensions/transport_sockets/tls/context_impl.cc +++ b/source/extensions/transport_sockets/tls/context_impl.cc @@ -1171,6 +1171,14 @@ bool ContextImpl::verifyCertChain(X509& leaf_cert, STACK_OF(X509) & intermediate error_details = "Failed to verify certificate chain: X509_STORE_CTX_init"; return false; } + // Currently this method is only used to verify server certs, so hard-code "ssl_server" for now. + if (!X509_STORE_CTX_set_default(ctx.get(), "ssl_server") || + !X509_VERIFY_PARAM_set1(X509_STORE_CTX_get0_param(ctx.get()), + SSL_CTX_get0_param(const_cast(ssl_ctx)))) { + error_details = + "Failed to verify certificate chain: fail to setup X509_STORE_CTX or its param."; + return false; + } int res = cert_validator_->doVerifyCertChain(ctx.get(), nullptr, leaf_cert, nullptr); // If |SSL_VERIFY_NONE|, the error is non-fatal, but we keep the error details. diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index c5056786ee9a..31ed7fd52f01 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -164,7 +164,7 @@ Network::IoResult SslSocket::doRead(Buffer::Instance& read_buffer) { } void SslSocket::onPrivateKeyMethodComplete() { - ASSERT(isThreadSafe()); + ASSERT(callbacks_ != nullptr && callbacks_->connection().dispatcher().isThreadSafe()); ASSERT(info_->state() == Ssl::SocketState::HandshakeInProgress); // Resume handshake. diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h index 186bebbabc06..a6502cc9548c 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -86,9 +86,6 @@ class SslSocket : public Network::TransportSocket, void drainErrorQueue(); void shutdownSsl(); void shutdownBasic(); - bool isThreadSafe() const { - return callbacks_ != nullptr && callbacks_->connection().dispatcher().isThreadSafe(); - } const Network::TransportSocketOptionsConstSharedPtr transport_socket_options_; Network::TransportSocketCallbacks* callbacks_{}; diff --git a/test/common/quic/envoy_quic_proof_verifier_test.cc b/test/common/quic/envoy_quic_proof_verifier_test.cc index 21b7be3136a7..304488c49a66 100644 --- a/test/common/quic/envoy_quic_proof_verifier_test.cc +++ b/test/common/quic/envoy_quic_proof_verifier_test.cc @@ -7,6 +7,7 @@ #include "test/mocks/ssl/mocks.h" #include "test/mocks/stats/mocks.h" #include "test/test_common/test_time.h" +#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -78,7 +79,7 @@ class EnvoyQuicProofVerifierTest : public testing::Test { const std::string empty_string_; const std::vector empty_string_list_; const std::string cert_chain_{quic::test::kTestCertificateChainPem}; - const std::string root_ca_cert_; + std::string root_ca_cert_; const std::string leaf_cert_; const absl::optional custom_validator_config_{ absl::nullopt}; @@ -119,6 +120,12 @@ TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainFailureFromSsl) { error_details); } +TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainFailureInvalidCA) { + root_ca_cert_ = "invalid root CA"; + EXPECT_THROW_WITH_REGEX(configCertVerificationDetails(true), EnvoyException, + "Failed to load trusted CA certificates from"); +} + TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainFailureInvalidLeafCert) { configCertVerificationDetails(true); const std::string ocsp_response; @@ -192,5 +199,76 @@ VdGXMAjeXhnOnPvmDi5hUz/uvI+Pg6cNmUoCRwSCnK/DazhA EXPECT_EQ("Invalid leaf cert, only P-256 ECDSA certificates are supported", error_details); } +TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainFailureNonServerAuthEKU) { + // Override the CA cert with cert copied from test/config/integration/certs/cacert.pem. + root_ca_cert_ = R"(-----BEGIN CERTIFICATE----- +MIID3TCCAsWgAwIBAgIUdCu/mLip3X/We37vh3BA9u/nxakwDQYJKoZIhvcNAQEL +BQAwdjELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcM +DVNhbiBGcmFuY2lzY28xDTALBgNVBAoMBEx5ZnQxGTAXBgNVBAsMEEx5ZnQgRW5n +aW5lZXJpbmcxEDAOBgNVBAMMB1Rlc3QgQ0EwHhcNMjAwODA1MTkxNjAwWhcNMjIw +ODA1MTkxNjAwWjB2MQswCQYDVQQGEwJVUzETMBEGA1UECAwKQ2FsaWZvcm5pYTEW +MBQGA1UEBwwNU2FuIEZyYW5jaXNjbzENMAsGA1UECgwETHlmdDEZMBcGA1UECwwQ +THlmdCBFbmdpbmVlcmluZzEQMA4GA1UEAwwHVGVzdCBDQTCCASIwDQYJKoZIhvcN +AQEBBQADggEPADCCAQoCggEBALu2Ihi4DmaQG7zySZlWyM9SjxOXCI5840V7Hn0C +XoiI8sQQmKSC2YCzsaphQoJ0lXCi6Y47o5FkooYyLeNDQTGS0nh+IWm5RCyochtO +fnaKPv/hYxhpyFQEwkJkbF1Zt1s6j2rq5MzmbWZx090uXZEE82DNZ9QJaMPu6VWt +iwGoGoS5HF5HNlUVxLNUsklNH0ZfDafR7/LC2ty1vO1c6EJ6yCGiyJZZ7Ilbz27Q +HPAUd8CcDNKCHZDoMWkLSLN3Nj1MvPVZ5HDsHiNHXthP+zV8FQtloAuZ8Srsmlyg +rJREkc7gF3f6HrH5ShNhsRFFc53NUjDbYZuha1u4hiOE8lcCAwEAAaNjMGEwDwYD +VR0TAQH/BAUwAwEB/zAOBgNVHQ8BAf8EBAMCAQYwHQYDVR0OBBYEFJZL2ixTtL6V +xpNz4qekny4NchiHMB8GA1UdIwQYMBaAFJZL2ixTtL6VxpNz4qekny4NchiHMA0G +CSqGSIb3DQEBCwUAA4IBAQAcgG+AaCdrUFEVJDn9UsO7zqzQ3c1VOp+WAtAU8OQK +Oc4vJYVVKpDs8OZFxmukCeqm1gz2zDeH7TfgCs5UnLtkplx1YO1bd9qvserJVHiD +LAK+Yl24ZEbrHPaq0zI1RLchqYUOGWmi51pcXi1gsfc8DQ3GqIXoai6kYJeV3jFJ +jxpQSR32nx6oNN/6kVKlgmBjlWrOy7JyDXGim6Z97TzmS6Clctewmw/5gZ9g+M8e +g0ZdFbFkNUjzSNm44hiDX8nR6yJRn+gLaARaJvp1dnT+MlvofZuER17WYKH4OyMs +ie3qKR3an4KC20CtFbpZfv540BVuTTOCtQ5xqZ/LTE78 +-----END CERTIFICATE-----)"; + configCertVerificationDetails(true); + const std::string ocsp_response; + const std::string cert_sct; + std::string error_details; + // This is a cert generated with the test/config/integration/certs/certs.sh. And the config that + // used to generate this cert is same as test/config/integration/certs/servercert.cfg but with + // 'extKeyUsage: clientAuth'. + const std::string certs{R"(-----BEGIN CERTIFICATE----- +MIIEYjCCA0qgAwIBAgIUWzmfQSTX8xfzUzdByjCjCJN8E/wwDQYJKoZIhvcNAQEL +BQAwdjELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcM +DVNhbiBGcmFuY2lzY28xDTALBgNVBAoMBEx5ZnQxGTAXBgNVBAsMEEx5ZnQgRW5n +aW5lZXJpbmcxEDAOBgNVBAMMB1Rlc3QgQ0EwHhcNMjEwOTI5MTY0NTM3WhcNMjMw +OTI5MTY0NTM3WjCBpjELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWEx +FjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDTALBgNVBAoMBEx5ZnQxGTAXBgNVBAsM +EEx5ZnQgRW5naW5lZXJpbmcxGjAYBgNVBAMMEVRlc3QgQmFja2VuZCBUZWFtMSQw +IgYJKoZIhvcNAQkBFhViYWNrZW5kLXRlYW1AbHlmdC5jb20wggEiMA0GCSqGSIb3 +DQEBAQUAA4IBDwAwggEKAoIBAQC9JgaI7hxjPM0tsUna/QmivBdKbCrLnLW9Teak +RH/Ebg68ovyvrRIlybDT6XhKi+iVpzVY9kqxhGHgrFDgGLBakVMiYJ5EjIgHfoo4 +UUAHwIYbunJluYCgANzpprBsvTC/yFYDVMqUrjvwHsoYYVm36io994k9+t813b70 +o0l7/PraBsKkz8NcY2V2mrd/yHn/0HAhv3hl6iiJme9yURuDYQrae2ACSrQtsbel +KwdZ/Re71Z1awz0OQmAjMa2HuCop+Q/1QLnqBekT5+DH1qKUzJ3Jkq6NRkERXOpi +87j04rtCBteCogrO67qnuBZ2lH3jYEMb+lQdLkyNMLltBSdLAgMBAAGjgbYwgbMw +DAYDVR0TAQH/BAIwADALBgNVHQ8EBAMCBeAwEwYDVR0lBAwwCgYIKwYBBQUHAwIw +QQYDVR0RBDowOIYec3BpZmZlOi8vbHlmdC5jb20vYmFja2VuZC10ZWFtgghseWZ0 +LmNvbYIMd3d3Lmx5ZnQuY29tMB0GA1UdDgQWBBTZdxNltzTEpl+A1UpK8BsxkkIG +hjAfBgNVHSMEGDAWgBSWS9osU7S+lcaTc+KnpJ8uDXIYhzANBgkqhkiG9w0BAQsF +AAOCAQEAhiXkQJZ53L3uoQMX6xNhAFThomirnLm2RT10kPIbr5mmf3wcR8+EKrWX +dWCj56bk1tSDbQZqx33DSGbhvNaydggbo69Pkie5b7J9O7AWzT21NME6Jis9hHED +VUI63L+7SgJ2oZs0o8xccUaLFeknuNdQL4qUEwhMwCC8kYLz+c6g0qwDwZi1MtdL +YR4qm2S6KveVPGzBHpUjfWf/whSCM3JN5Fm8gWfC6d6XEYz6z1dZrj3lpwmhRgF6 +Wb72f68jzCQ3BFqKRFsJI2xz3EP6PoQ+e6EQjMpjQLomxIhIN/aTsgrKwA5wf6vQ +ZCFbredVxDBZuoVsfrKPSQa407Jj1Q== +-----END CERTIFICATE-----)"}; + std::stringstream pem_stream(certs); + std::vector chain = quic::CertificateView::LoadPemFromStream(&pem_stream); + std::unique_ptr cert_view = + quic::CertificateView::ParseSingleCertificate(chain[0]); + ASSERT(cert_view); + EXPECT_EQ(quic::QUIC_FAILURE, + verifier_->VerifyCertChain("lyft.com", 54321, chain, ocsp_response, cert_sct, nullptr, + &error_details, nullptr, nullptr, nullptr)); + EXPECT_EQ("X509_verify_cert: certificate verification error at depth 0: unsupported certificate " + "purpose", + error_details); +} + } // namespace Quic } // namespace Envoy diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 24b2e1422c7e..c85ae51bfac6 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -658,6 +658,7 @@ void testUtilV2(const TestUtilOptionsV2& options) { ServerSslSocketFactory server_ssl_socket_factory(std::move(server_cfg), manager, server_stats_store, server_names); + EXPECT_FALSE(server_ssl_socket_factory.usesProxyProtocolOptions()); Event::DispatcherPtr dispatcher(server_api->allocateDispatcher("test_thread")); auto socket = std::make_shared( @@ -702,9 +703,11 @@ void testUtilV2(const TestUtilOptionsV2& options) { ? options.transportSocketOptions()->serverNameOverride().value() : options.clientCtxProto().sni(); socket->setRequestedServerName(sni); + Network::TransportSocketPtr transport_socket = + server_ssl_socket_factory.createTransportSocket(nullptr); + EXPECT_FALSE(transport_socket->startSecureTransport()); server_connection = dispatcher->createServerConnection( - std::move(socket), server_ssl_socket_factory.createTransportSocket(nullptr), - stream_info); + std::move(socket), std::move(transport_socket), stream_info); server_connection->addConnectionCallbacks(server_connection_callbacks); })); diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index f89846c06b98..377c19089494 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -481,6 +481,7 @@ bursty bytecode bytestream bytestring +cacert cacheable cacheability callee @@ -497,6 +498,7 @@ canonicalizer canonicalizing cardinality casted +cfg charset checkin checksum @@ -1084,6 +1086,7 @@ sendto serializable serializer serv +servercert setenv setsockopt sig From 1f8111b6f6ef299c5090d0a013e41333a2a677a7 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Sat, 19 Feb 2022 01:13:30 +0100 Subject: [PATCH 04/15] Fix for X.509 subjectAltName matching (and nameConstraint) bypass (#19873) Backport for v1.20 of bb95af848c939cfe5b5ee33c5b1770558077e64e. Signed-off-by: Otto van der Schaaf --- .../transport_sockets/tls/v3/common.proto | 41 ++++++++-- .../tls/v3/tls_spiffe_validator_config.proto | 2 +- configs/envoy_double_proxy.template.yaml | 12 ++- .../envoy_service_to_service.template.yaml | 12 ++- .../arch_overview/security/_include/ssl.yaml | 6 +- .../root/intro/arch_overview/security/ssl.rst | 2 +- .../_include/envoy-demo-tls-client-auth.yaml | 6 +- .../_include/envoy-demo-tls-validation.yaml | 6 +- docs/root/start/quick-start/securing.rst | 12 +-- docs/root/version_history/current.rst | 5 ++ .../certificate_validation_context_config.h | 3 +- examples/double-proxy/envoy-backend.yaml | 6 +- examples/double-proxy/envoy-frontend.yaml | 6 +- ...tificate_validation_context_config_impl.cc | 32 +++++++- ...rtificate_validation_context_config_impl.h | 9 ++- .../tls/cert_validator/BUILD | 6 ++ .../tls/cert_validator/default_validator.cc | 28 +++---- .../tls/cert_validator/default_validator.h | 18 ++--- .../tls/cert_validator/san_matcher.cc | 55 +++++++++++++ .../tls/cert_validator/san_matcher.h | 51 ++++++++++++ .../cert_validator/spiffe/spiffe_validator.cc | 21 ++--- .../cert_validator/spiffe/spiffe_validator.h | 4 +- .../quic/envoy_quic_proof_source_test.cc | 3 +- .../quic/envoy_quic_proof_verifier_test.cc | 3 +- test/common/secret/sds_api_test.cc | 18 ++++- .../common/secret/secret_manager_impl_test.cc | 37 +++++++++ test/common/upstream/upstream_impl_test.cc | 40 +++++++--- test/config/utility.cc | 4 +- test/config/utility.h | 7 +- .../tls/cert_validator/BUILD | 13 ++++ .../cert_validator/default_validator_test.cc | 77 +++++++++++++------ .../tls/cert_validator/san_matcher_test.cc | 57 ++++++++++++++ .../spiffe_validator_integration_test.cc | 28 ++++++- .../spiffe_validator_integration_test.h | 3 +- .../spiffe/spiffe_validator_test.cc | 28 ++++++- .../tls/cert_validator/test_common.h | 8 +- .../tls/context_impl_test.cc | 35 ++++++++- .../transport_sockets/tls/ssl_socket_test.cc | 30 +++++--- test/integration/ads_integration.cc | 5 +- test/integration/ssl_utility.cc | 19 ++++- test/integration/xfcc_integration_test.cc | 34 +++++--- test/mocks/ssl/mocks.h | 5 +- test/per_file_coverage.sh | 4 +- .../listener_manager_impl_quic_only_test.cc | 40 +++++++--- 44 files changed, 668 insertions(+), 173 deletions(-) create mode 100644 source/extensions/transport_sockets/tls/cert_validator/san_matcher.cc create mode 100644 source/extensions/transport_sockets/tls/cert_validator/san_matcher.h create mode 100644 test/extensions/transport_sockets/tls/cert_validator/san_matcher_test.cc diff --git a/api/envoy/extensions/transport_sockets/tls/v3/common.proto b/api/envoy/extensions/transport_sockets/tls/v3/common.proto index 82dcb37cd7ca..ebed0e520b2f 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/common.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/common.proto @@ -9,6 +9,7 @@ import "envoy/type/matcher/v3/string.proto"; import "google/protobuf/any.proto"; import "google/protobuf/wrappers.proto"; +import "envoy/annotations/deprecation.proto"; import "udpa/annotations/migrate.proto"; import "udpa/annotations/sensitive.proto"; import "udpa/annotations/status.proto"; @@ -253,7 +254,26 @@ message CertificateProviderPluginInstance { string certificate_name = 2; } -// [#next-free-field: 14] +// Matcher for subject alternative names, to match both type and value of the SAN. +message SubjectAltNameMatcher { + // Indicates the choice of GeneralName as defined in section 4.2.1.5 of RFC 5280 to match + // against. + enum SanType { + SAN_TYPE_UNSPECIFIED = 0; + EMAIL = 1; + DNS = 2; + URI = 3; + IP_ADDRESS = 4; + } + + // Specification of type of SAN. Note that the default enum value is an invalid choice. + SanType san_type = 1 [(validate.rules).enum = {defined_only: true not_in: 0}]; + + // Matcher for SAN value. + type.matcher.v3.StringMatcher matcher = 2 [(validate.rules).message = {required: true}]; +} + +// [#next-free-field: 16] message CertificateValidationContext { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.CertificateValidationContext"; @@ -283,8 +303,8 @@ message CertificateValidationContext { // `, // :ref:`verify_certificate_hash // `, or - // :ref:`match_subject_alt_names - // `) is also + // :ref:`match_typed_subject_alt_names + // `) is also // specified. // // It can optionally contain certificate revocation lists, in which case Envoy will verify @@ -388,6 +408,8 @@ message CertificateValidationContext { // An optional list of Subject Alternative name matchers. If specified, Envoy will verify that the // Subject Alternative Name of the presented certificate matches one of the specified matchers. + // The matching uses "any" semantics, that is to say, the SAN is verified if at least one matcher is + // matched. // // When a certificate has wildcard DNS SAN entries, to match a specific client, it should be // configured with exact match type in the :ref:`string matcher `. @@ -396,15 +418,22 @@ message CertificateValidationContext { // // .. code-block:: yaml // - // match_subject_alt_names: - // exact: "api.example.com" + // match_typed_subject_alt_names: + // - san_type: DNS + // matcher: + // exact: "api.example.com" // // .. attention:: // // Subject Alternative Names are easily spoofable and verifying only them is insecure, // therefore this option must be used together with :ref:`trusted_ca // `. - repeated type.matcher.v3.StringMatcher match_subject_alt_names = 9; + repeated SubjectAltNameMatcher match_typed_subject_alt_names = 15; + + // This field is deprecated in favor of ref:`match_typed_subject_alt_names + // ` + repeated type.matcher.v3.StringMatcher match_subject_alt_names = 9 + [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"]; // [#not-implemented-hide:] Must present signed certificate time-stamp. google.protobuf.BoolValue require_signed_certificate_timestamp = 6; diff --git a/api/envoy/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto b/api/envoy/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto index cfb5e5c07e90..382fe985daff 100644 --- a/api/envoy/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto +++ b/api/envoy/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.proto @@ -42,7 +42,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // Note that SPIFFE validator inherits and uses the following options from :ref:`CertificateValidationContext `. // // - :ref:`allow_expired_certificate ` to allow expired certificates. -// - :ref:`match_subject_alt_names ` to match **URI** SAN of certificates. Unlike the default validator, SPIFFE validator only matches **URI** SAN (which equals to SVID in SPIFFE terminology) and ignore other SAN types. +// - :ref:`match_typed_subject_alt_names ` to match **URI** SAN of certificates. Unlike the default validator, SPIFFE validator only matches **URI** SAN (which equals to SVID in SPIFFE terminology) and ignore other SAN types. // message SPIFFECertValidatorConfig { message TrustDomain { diff --git a/configs/envoy_double_proxy.template.yaml b/configs/envoy_double_proxy.template.yaml index b574d6a518c5..56f4d17ad201 100644 --- a/configs/envoy_double_proxy.template.yaml +++ b/configs/envoy_double_proxy.template.yaml @@ -149,8 +149,10 @@ static_resources: validation_context: trusted_ca: filename: certs/cacert.pem - match_subject_alt_names: - exact: "front-proxy.yourcompany.net" + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: "front-proxy.yourcompany.net" typed_extension_protocol_options: envoy.extensions.upstreams.http.v3.HttpProtocolOptions: "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions @@ -188,8 +190,10 @@ static_resources: validation_context: trusted_ca: filename: certs/cacert.pem - match_subject_alt_names: - exact: "collector-grpc.lightstep.com" + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: "collector-grpc.lightstep.com" flags_path: "/etc/envoy/flags" stats_sinks: - name: envoy.stat_sinks.statsd diff --git a/configs/envoy_service_to_service.template.yaml b/configs/envoy_service_to_service.template.yaml index 6ec2f0bde905..7bdd6a4d0fad 100644 --- a/configs/envoy_service_to_service.template.yaml +++ b/configs/envoy_service_to_service.template.yaml @@ -350,8 +350,10 @@ static_resources: trusted_ca: filename: certs/cacert.pem {% if host.get('verify_subject_alt_name', False) %} - match_subject_alt_names: - exact: "{{host['verify_subject_alt_name'] }}" + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: "{{host['verify_subject_alt_name'] }}" {% endif %} {% if host.get('sni', False) %} sni: "{{ host['sni'] }}" @@ -520,8 +522,10 @@ static_resources: validation_context: trusted_ca: filename: certs/cacert.pem - match_subject_alt_names: - exact: "collector-grpc.lightstep.com" + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: "collector-grpc.lightstep.com" - name: cds_cluster connect_timeout: 0.25s type: STRICT_DNS diff --git a/docs/root/intro/arch_overview/security/_include/ssl.yaml b/docs/root/intro/arch_overview/security/_include/ssl.yaml index 6f666e4d92a5..00bd6083239b 100644 --- a/docs/root/intro/arch_overview/security/_include/ssl.yaml +++ b/docs/root/intro/arch_overview/security/_include/ssl.yaml @@ -50,7 +50,9 @@ static_resources: private_key: {"filename": "certs/serverkey.pem"} ocsp_staple: {"filename": "certs/server_ocsp_resp.der"} validation_context: - match_subject_alt_names: - - exact: "foo" + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: "foo" trusted_ca: filename: /etc/ssl/certs/ca-certificates.crt diff --git a/docs/root/intro/arch_overview/security/ssl.rst b/docs/root/intro/arch_overview/security/ssl.rst index e1192833232c..8878c1faf419 100644 --- a/docs/root/intro/arch_overview/security/ssl.rst +++ b/docs/root/intro/arch_overview/security/ssl.rst @@ -76,7 +76,7 @@ Example configuration */etc/ssl/certs/ca-certificates.crt* is the default path for the system CA bundle on Debian systems. :ref:`trusted_ca ` along with -:ref:`match_subject_alt_names ` +:ref:`match_typed_subject_alt_names ` makes Envoy verify the server identity of *127.0.0.1:1234* as "foo" in the same way as e.g. cURL does on standard Debian installations. Common paths for system CA bundles on Linux and BSD are: diff --git a/docs/root/start/quick-start/_include/envoy-demo-tls-client-auth.yaml b/docs/root/start/quick-start/_include/envoy-demo-tls-client-auth.yaml index 84367c637f68..dd48870fbda6 100644 --- a/docs/root/start/quick-start/_include/envoy-demo-tls-client-auth.yaml +++ b/docs/root/start/quick-start/_include/envoy-demo-tls-client-auth.yaml @@ -34,8 +34,10 @@ static_resources: validation_context: trusted_ca: filename: certs/cacert.pem - match_subject_alt_names: - - exact: proxy-postgres-frontend.example.com + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: proxy-postgres-frontend.example.com tls_certificates: - certificate_chain: filename: certs/servercert.pem diff --git a/docs/root/start/quick-start/_include/envoy-demo-tls-validation.yaml b/docs/root/start/quick-start/_include/envoy-demo-tls-validation.yaml index b9ad6cc0635e..054f79e55f36 100644 --- a/docs/root/start/quick-start/_include/envoy-demo-tls-validation.yaml +++ b/docs/root/start/quick-start/_include/envoy-demo-tls-validation.yaml @@ -48,5 +48,7 @@ static_resources: validation_context: trusted_ca: filename: certs/cacert.pem - match_subject_alt_names: - - exact: proxy-postgres-backend.example.com + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: proxy-postgres-backend.example.com diff --git a/docs/root/start/quick-start/securing.rst b/docs/root/start/quick-start/securing.rst index ccfd6bd0ce06..35ff830450a6 100644 --- a/docs/root/start/quick-start/securing.rst +++ b/docs/root/start/quick-start/securing.rst @@ -100,7 +100,7 @@ certificate is valid for. .. note:: If the "Subject Alternative Names" for a certificate are for a wildcard domain, eg ``*.example.com``, - this is what you should use when matching with ``match_subject_alt_names``. + this is what you should use when matching with ``match_typed_subject_alt_names``. .. note:: @@ -122,20 +122,20 @@ and specify a mutually trusted certificate authority: :language: yaml :linenos: :lineno-start: 27 - :lines: 27-39 + :lines: 27-41 :emphasize-lines: 6, 8-10 :caption: :download:`envoy-demo-tls-client-auth.yaml <_include/envoy-demo-tls-client-auth.yaml>` You can further restrict the authentication of connecting clients by specifying the allowed "Subject Alternative Names" in -:ref:`match_subject_alt_names `, +:ref:`match_typed_subject_alt_names `, similar to validating upstream certificates :ref:`described above `. .. literalinclude:: _include/envoy-demo-tls-client-auth.yaml :language: yaml :linenos: :lineno-start: 27 - :lines: 27-39 + :lines: 27-41 :emphasize-lines: 7, 11-12 :caption: :download:`envoy-demo-tls-client-auth.yaml <_include/envoy-demo-tls-client-auth.yaml>` @@ -154,8 +154,8 @@ When connecting to an upstream with client certificates you can set them as foll .. literalinclude:: _include/envoy-demo-tls-client-auth.yaml :language: yaml :linenos: - :lineno-start: 44 - :lines: 44-68 + :lineno-start: 46 + :lines: 46-70 :emphasize-lines: 20-25 :caption: :download:`envoy-demo-tls-client-auth.yaml <_include/envoy-demo-tls-client-auth.yaml>` diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 0c1c97cb5d25..6d6be3ba7085 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -20,5 +20,10 @@ Removed Config or Runtime New Features ------------ ++* tls: added support for :ref:`match_typed_subject_alt_names ` for subject alternative names to enforce specifying the subject alternative name type for the matcher to prevent matching against an unintended type in the certificate. + + Deprecated ---------- + ++* tls: :ref:`match_subject_alt_names ` has been deprecated in favor of the :ref:`match_typed_subject_alt_names `. diff --git a/envoy/ssl/certificate_validation_context_config.h b/envoy/ssl/certificate_validation_context_config.h index 544215a73dae..187a4ff07c4d 100644 --- a/envoy/ssl/certificate_validation_context_config.h +++ b/envoy/ssl/certificate_validation_context_config.h @@ -7,6 +7,7 @@ #include "envoy/api/api.h" #include "envoy/common/pure.h" #include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h" +#include "envoy/extensions/transport_sockets/tls/v3/common.pb.h" #include "envoy/type/matcher/v3/string.pb.h" #include "absl/types/optional.h" @@ -43,7 +44,7 @@ class CertificateValidationContextConfig { /** * @return The subject alt name matchers to be verified, if enabled. */ - virtual const std::vector& + virtual const std::vector& subjectAltNameMatchers() const PURE; /** diff --git a/examples/double-proxy/envoy-backend.yaml b/examples/double-proxy/envoy-backend.yaml index 07cc1a7905f1..0636354c3771 100644 --- a/examples/double-proxy/envoy-backend.yaml +++ b/examples/double-proxy/envoy-backend.yaml @@ -26,8 +26,10 @@ static_resources: private_key: filename: certs/serverkey.pem validation_context: - match_subject_alt_names: - - exact: proxy-postgres-frontend.example.com + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: proxy-postgres-frontend.example.com trusted_ca: filename: certs/cacert.pem diff --git a/examples/double-proxy/envoy-frontend.yaml b/examples/double-proxy/envoy-frontend.yaml index 37acbf334124..21fa643e62ed 100644 --- a/examples/double-proxy/envoy-frontend.yaml +++ b/examples/double-proxy/envoy-frontend.yaml @@ -36,7 +36,9 @@ static_resources: private_key: filename: certs/clientkey.pem validation_context: - match_subject_alt_names: - - exact: proxy-postgres-backend.example.com + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: proxy-postgres-backend.example.com trusted_ca: filename: certs/cacert.pem diff --git a/source/common/ssl/certificate_validation_context_config_impl.cc b/source/common/ssl/certificate_validation_context_config_impl.cc index 40dc20f6ef3a..ee7e2d171233 100644 --- a/source/common/ssl/certificate_validation_context_config_impl.cc +++ b/source/common/ssl/certificate_validation_context_config_impl.cc @@ -22,8 +22,7 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl( certificate_revocation_list_path_( Config::DataSource::getPath(config.crl()) .value_or(certificate_revocation_list_.empty() ? EMPTY_STRING : INLINE_STRING)), - subject_alt_name_matchers_(config.match_subject_alt_names().begin(), - config.match_subject_alt_names().end()), + subject_alt_name_matchers_(getSubjectAltNameMatchers(config)), verify_certificate_hash_list_(config.verify_certificate_hash().begin(), config.verify_certificate_hash().end()), verify_certificate_spki_list_(config.verify_certificate_spki().begin(), @@ -51,5 +50,34 @@ CertificateValidationContextConfigImpl::CertificateValidationContextConfigImpl( } } +std::vector +CertificateValidationContextConfigImpl::getSubjectAltNameMatchers( + const envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext& config) { + if (!config.match_typed_subject_alt_names().empty() && + !config.match_subject_alt_names().empty()) { + throw EnvoyException("SAN-based verification using both match_typed_subject_alt_names and " + "the deprecated match_subject_alt_names is not allowed"); + } + std::vector + subject_alt_name_matchers(config.match_typed_subject_alt_names().begin(), + config.match_typed_subject_alt_names().end()); + // Handle deprecated string type san matchers without san type specified, by + // creating a matcher for each supported type. + for (const envoy::type::matcher::v3::StringMatcher& matcher : config.match_subject_alt_names()) { + static constexpr std::array< + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::SanType, 4> + san_types{envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS, + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::URI, + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::EMAIL, + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::IP_ADDRESS}; + for (const auto san_type : san_types) { + subject_alt_name_matchers.emplace_back(); + subject_alt_name_matchers.back().set_san_type(san_type); + *subject_alt_name_matchers.back().mutable_matcher() = matcher; + } + } + return subject_alt_name_matchers; +} + } // namespace Ssl } // namespace Envoy diff --git a/source/common/ssl/certificate_validation_context_config_impl.h b/source/common/ssl/certificate_validation_context_config_impl.h index 7cf045a35184..f58ac1ae0a54 100644 --- a/source/common/ssl/certificate_validation_context_config_impl.h +++ b/source/common/ssl/certificate_validation_context_config_impl.h @@ -4,6 +4,7 @@ #include "envoy/api/api.h" #include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h" +#include "envoy/extensions/transport_sockets/tls/v3/common.pb.h" #include "envoy/ssl/certificate_validation_context_config.h" #include "envoy/type/matcher/v3/string.pb.h" @@ -24,7 +25,7 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte const std::string& certificateRevocationListPath() const final { return certificate_revocation_list_path_; } - const std::vector& + const std::vector& subjectAltNameMatchers() const override { return subject_alt_name_matchers_; } @@ -49,11 +50,15 @@ class CertificateValidationContextConfigImpl : public CertificateValidationConte Api::Api& api() const override { return api_; } private: + static std::vector + getSubjectAltNameMatchers( + const envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext& config); const std::string ca_cert_; const std::string ca_cert_path_; const std::string certificate_revocation_list_; const std::string certificate_revocation_list_path_; - const std::vector subject_alt_name_matchers_; + const std::vector + subject_alt_name_matchers_; const std::vector verify_certificate_hash_list_; const std::vector verify_certificate_spki_list_; const bool allow_expired_certificate_; diff --git a/source/extensions/transport_sockets/tls/cert_validator/BUILD b/source/extensions/transport_sockets/tls/cert_validator/BUILD index ce92df41e80c..93f745f8faa3 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/BUILD +++ b/source/extensions/transport_sockets/tls/cert_validator/BUILD @@ -13,12 +13,14 @@ envoy_cc_library( srcs = [ "default_validator.cc", "factory.cc", + "san_matcher.cc", "utility.cc", ], hdrs = [ "cert_validator.h", "default_validator.h", "factory.h", + "san_matcher.h", "utility.h", ], external_deps = [ @@ -35,9 +37,13 @@ envoy_cc_library( "//source/common/common:hex_lib", "//source/common/common:minimal_logger_lib", "//source/common/common:utility_lib", + "//source/common/config:utility_lib", "//source/common/stats:symbol_table_lib", "//source/common/stats:utility_lib", "//source/extensions/transport_sockets/tls:stats_lib", "//source/extensions/transport_sockets/tls:utility_lib", + "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto", + "@envoy_api//envoy/type/matcher/v3:pkg_cc_proto", ], ) 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 369c2aa7375b..1af0c588f45d 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc +++ b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc @@ -18,6 +18,7 @@ #include "source/common/common/hex.h" #include "source/common/common/matchers.h" #include "source/common/common/utility.h" +#include "source/common/config/utility.h" #include "source/common/network/address_impl.h" #include "source/common/protobuf/utility.h" #include "source/common/runtime/runtime_features.h" @@ -144,9 +145,9 @@ int DefaultCertValidator::initializeSslContexts(std::vector contexts, const Envoy::Ssl::CertificateValidationContextConfig* cert_validation_config = config_; if (cert_validation_config != nullptr) { if (!cert_validation_config->subjectAltNameMatchers().empty()) { - for (const envoy::type::matcher::v3::StringMatcher& matcher : + for (const envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher& matcher : cert_validation_config->subjectAltNameMatchers()) { - subject_alt_name_matchers_.push_back(Matchers::StringMatcherImpl(matcher)); + subject_alt_name_matchers_.emplace_back(createStringSanMatcher(matcher)); } verify_mode = verify_mode_validation_context; } @@ -218,8 +219,8 @@ int DefaultCertValidator::doVerifyCertChain( // If `trusted_ca` exists, it is already verified in the code above. Thus, we just need to make // sure the verification for other validation context configurations doesn't fail (i.e. either - // `NotValidated` or `Validated`). If `trusted_ca` doesn't exist, we will need to make sure other - // configurations are verified and the verification succeed. + // `NotValidated` or `Validated`). If `trusted_ca` doesn't exist, we will need to make sure + // other configurations are verified and the verification succeed. int validation_status = verify_trusted_ca_ ? validated != Envoy::Ssl::ClientValidationStatus::Failed : validated == Envoy::Ssl::ClientValidationStatus::Validated; @@ -229,8 +230,7 @@ int DefaultCertValidator::doVerifyCertChain( Envoy::Ssl::ClientValidationStatus DefaultCertValidator::verifyCertificate( X509* cert, const std::vector& verify_san_list, - const std::vector>& - subject_alt_name_matchers) { + const std::vector& subject_alt_name_matchers) { Envoy::Ssl::ClientValidationStatus validated = Envoy::Ssl::ClientValidationStatus::NotValidated; if (!verify_san_list.empty()) { @@ -309,23 +309,15 @@ bool DefaultCertValidator::dnsNameMatch(const absl::string_view dns_name, } bool DefaultCertValidator::matchSubjectAltName( - X509* cert, - const std::vector>& - subject_alt_name_matchers) { + X509* cert, const std::vector& subject_alt_name_matchers) { bssl::UniquePtr san_names( static_cast(X509_get_ext_d2i(cert, NID_subject_alt_name, nullptr, nullptr))); if (san_names == nullptr) { return false; } - for (const GENERAL_NAME* general_name : san_names.get()) { - const std::string san = Utility::generalNameAsString(general_name); - for (auto& config_san_matcher : subject_alt_name_matchers) { - // For DNS SAN, if the StringMatcher type is exact, we have to follow DNS matching semantics. - if (general_name->type == GEN_DNS && - config_san_matcher.matcher().match_pattern_case() == - envoy::type::matcher::v3::StringMatcher::MatchPatternCase::kExact - ? dnsNameMatch(config_san_matcher.matcher().exact(), absl::string_view(san)) - : config_san_matcher.match(san)) { + for (const auto& config_san_matcher : subject_alt_name_matchers) { + for (const GENERAL_NAME* general_name : san_names.get()) { + if (config_san_matcher->match(general_name)) { return true; } } 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 163eb0118d42..6a0a4ce6165e 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/default_validator.h +++ b/source/extensions/transport_sockets/tls/cert_validator/default_validator.h @@ -4,6 +4,7 @@ #include #include #include +#include #include #include "envoy/common/pure.h" @@ -18,6 +19,7 @@ #include "source/common/common/matchers.h" #include "source/common/stats/symbol_table_impl.h" #include "source/extensions/transport_sockets/tls/cert_validator/cert_validator.h" +#include "source/extensions/transport_sockets/tls/cert_validator/san_matcher.h" #include "source/extensions/transport_sockets/tls/stats.h" #include "absl/synchronization/mutex.h" @@ -53,10 +55,9 @@ class DefaultCertValidator : public CertValidator, Logger::Loggable& verify_san_list, - const std::vector>& - subject_alt_name_matchers); + Envoy::Ssl::ClientValidationStatus + verifyCertificate(X509* cert, const std::vector& verify_san_list, + const std::vector& subject_alt_name_matchers); /** * Verifies certificate hash for pinning. The hash is a hex-encoded SHA-256 of the DER-encoded @@ -103,10 +104,8 @@ class DefaultCertValidator : public CertValidator, Logger::Loggable>& - subject_alt_name_matchers); + static bool matchSubjectAltName(X509* cert, + const std::vector& subject_alt_name_matchers); private: const Envoy::Ssl::CertificateValidationContextConfig* config_; @@ -116,8 +115,7 @@ class DefaultCertValidator : public CertValidator, Logger::Loggable ca_cert_; std::string ca_file_path_; - std::vector> - subject_alt_name_matchers_; + std::vector subject_alt_name_matchers_; std::vector> verify_certificate_hash_list_; std::vector> verify_certificate_spki_list_; bool verify_trusted_ca_{false}; diff --git a/source/extensions/transport_sockets/tls/cert_validator/san_matcher.cc b/source/extensions/transport_sockets/tls/cert_validator/san_matcher.cc new file mode 100644 index 000000000000..2158a268f472 --- /dev/null +++ b/source/extensions/transport_sockets/tls/cert_validator/san_matcher.cc @@ -0,0 +1,55 @@ +#include "source/extensions/transport_sockets/tls/cert_validator/san_matcher.h" + +#include + +#include "envoy/config/core/v3/extension.pb.h" +#include "envoy/extensions/transport_sockets/tls/v3/common.pb.h" +#include "envoy/registry/registry.h" +#include "envoy/ssl/certificate_validation_context_config.h" + +#include "source/extensions/transport_sockets/tls/cert_validator/default_validator.h" +#include "source/extensions/transport_sockets/tls/utility.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { + +bool StringSanMatcher::match(const GENERAL_NAME* general_name) const { + if (general_name->type != general_name_type_) { + return false; + } + // For DNS SAN, if the StringMatcher type is exact, we have to follow DNS matching semantics. + const std::string san = Utility::generalNameAsString(general_name); + return general_name->type == GEN_DNS && + matcher_.matcher().match_pattern_case() == + envoy::type::matcher::v3::StringMatcher::MatchPatternCase::kExact + ? Extensions::TransportSockets::Tls::DefaultCertValidator::dnsNameMatch( + matcher_.matcher().exact(), absl::string_view(san)) + : matcher_.match(san); +} + +SanMatcherPtr createStringSanMatcher( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher const& matcher) { + // Verify that a new san type has not been added. + static_assert(envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::SanType_MAX == + 4); + + switch (matcher.san_type()) { + case envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS: + return SanMatcherPtr{std::make_unique(GEN_DNS, matcher.matcher())}; + case envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::EMAIL: + return SanMatcherPtr{std::make_unique(GEN_EMAIL, matcher.matcher())}; + case envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::URI: + return SanMatcherPtr{std::make_unique(GEN_URI, matcher.matcher())}; + case envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::IP_ADDRESS: + return SanMatcherPtr{std::make_unique(GEN_IPADD, matcher.matcher())}; + default: + NOT_REACHED_GCOVR_EXCL_LINE; + } +} + +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/transport_sockets/tls/cert_validator/san_matcher.h b/source/extensions/transport_sockets/tls/cert_validator/san_matcher.h new file mode 100644 index 000000000000..5c2141f4b570 --- /dev/null +++ b/source/extensions/transport_sockets/tls/cert_validator/san_matcher.h @@ -0,0 +1,51 @@ +#pragma once + +#include + +#include "envoy/config/core/v3/extension.pb.h" +#include "envoy/extensions/transport_sockets/tls/v3/common.pb.h" +#include "envoy/ssl/certificate_validation_context_config.h" +#include "envoy/type/matcher/v3/string.pb.h" + +#include "source/common/common/hash.h" +#include "source/common/common/matchers.h" +#include "source/common/protobuf/protobuf.h" +#include "source/extensions/transport_sockets/tls/utility.h" + +#include "openssl/x509v3.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { + +/** Interface to verify if there is a match in a list of subject alternative + * names. + */ +class SanMatcher { +public: + virtual bool match(GENERAL_NAME const*) const PURE; + virtual ~SanMatcher() = default; +}; + +using SanMatcherPtr = std::unique_ptr; + +class StringSanMatcher : public SanMatcher { +public: + bool match(const GENERAL_NAME* general_name) const override; + ~StringSanMatcher() override = default; + StringSanMatcher(int general_name_type, envoy::type::matcher::v3::StringMatcher matcher) + : general_name_type_(general_name_type), matcher_(matcher) {} + +private: + const int general_name_type_; + const Matchers::StringMatcherImpl matcher_; +}; + +SanMatcherPtr createStringSanMatcher( + const envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher& matcher); + +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy 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 63df8297c76d..c3b8d338fcae 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 @@ -1,5 +1,6 @@ #include "source/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator.h" +#include "envoy/extensions/transport_sockets/tls/v3/common.pb.h" #include "envoy/extensions/transport_sockets/tls/v3/tls_spiffe_validator_config.pb.h" #include "envoy/network/transport_socket.h" #include "envoy/registry/registry.h" @@ -37,7 +38,14 @@ SPIFFEValidator::SPIFFEValidator(const Envoy::Ssl::CertificateValidationContextC if (!config->subjectAltNameMatchers().empty()) { for (const auto& matcher : config->subjectAltNameMatchers()) { - subject_alt_name_matchers_.push_back(Matchers::StringMatcherImpl(matcher)); + if (matcher.san_type() == + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::URI) { + // Only match against URI SAN since SPIFFE specification does not restrict values in other + // SAN types. See the discussion: https://github.com/envoyproxy/envoy/issues/15392 + // TODO(pradeepcrao): Throw an exception when a non-URI matcher is encountered after the + // deprecated field match_subject_alt_names is removed + subject_alt_name_matchers_.emplace_back(createStringSanMatcher(matcher)); + } } } @@ -224,15 +232,10 @@ bool SPIFFEValidator::matchSubjectAltName(X509& leaf_cert) { ASSERT(san_names != nullptr, "san_names should have at least one name after SPIFFE cert validation"); - // Only match against URI SAN since SPIFFE specification does not restrict values in other SAN - // types. See the discussion: https://github.com/envoyproxy/envoy/issues/15392 for (const GENERAL_NAME* general_name : san_names.get()) { - if (general_name->type == GEN_URI) { - const std::string san = Utility::generalNameAsString(general_name); - for (const auto& config_san_matcher : subject_alt_name_matchers_) { - if (config_san_matcher.match(san)) { - return true; - } + for (const auto& config_san_matcher : subject_alt_name_matchers_) { + if (config_san_matcher->match(general_name)) { + return true; } } } 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 b4cc068e908e..669d77aec7d0 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 @@ -17,6 +17,7 @@ #include "source/common/common/matchers.h" #include "source/common/stats/symbol_table_impl.h" #include "source/extensions/transport_sockets/tls/cert_validator/cert_validator.h" +#include "source/extensions/transport_sockets/tls/cert_validator/san_matcher.h" #include "source/extensions/transport_sockets/tls/stats.h" #include "openssl/ssl.h" @@ -67,8 +68,7 @@ class SPIFFEValidator : public CertValidator { bool allow_expired_certificate_{false}; std::vector> ca_certs_; std::string ca_file_name_; - std::vector> - subject_alt_name_matchers_{}; + std::vector subject_alt_name_matchers_{}; absl::flat_hash_map trust_bundle_stores_; SslStats& stats_; diff --git a/test/common/quic/envoy_quic_proof_source_test.cc b/test/common/quic/envoy_quic_proof_source_test.cc index 9ef2e3fe5470..4e27141773ee 100644 --- a/test/common/quic/envoy_quic_proof_source_test.cc +++ b/test/common/quic/envoy_quic_proof_source_test.cc @@ -58,7 +58,8 @@ class SignatureVerifier { ON_CALL(cert_validation_ctx_config_, certificateRevocationListPath()) .WillByDefault(ReturnRef(path_string)); const std::vector empty_string_list; - const std::vector san_matchers; + const std::vector + san_matchers; ON_CALL(cert_validation_ctx_config_, subjectAltNameMatchers()) .WillByDefault(ReturnRef(san_matchers)); ON_CALL(cert_validation_ctx_config_, verifyCertificateHashList()) diff --git a/test/common/quic/envoy_quic_proof_verifier_test.cc b/test/common/quic/envoy_quic_proof_verifier_test.cc index 304488c49a66..c6b075fe03ea 100644 --- a/test/common/quic/envoy_quic_proof_verifier_test.cc +++ b/test/common/quic/envoy_quic_proof_verifier_test.cc @@ -75,7 +75,8 @@ class EnvoyQuicProofVerifierTest : public testing::Test { const std::string path_string_{"some_path"}; const std::string alpn_{"h2,http/1.1"}; const std::string sig_algs_{"rsa_pss_rsae_sha256"}; - const std::vector san_matchers_; + const std::vector + san_matchers_; const std::string empty_string_; const std::vector empty_string_list_; const std::string cert_chain_{quic::test::kTestCertificateChainPem}; diff --git a/test/common/secret/sds_api_test.cc b/test/common/secret/sds_api_test.cc index a0cea30a2f8a..f91734a588b9 100644 --- a/test/common/secret/sds_api_test.cc +++ b/test/common/secret/sds_api_test.cc @@ -665,7 +665,10 @@ TEST_F(SdsApiTest, DefaultCertificateValidationContextTest) { dynamic_cvc->set_allow_expired_certificate(false); dynamic_cvc->mutable_trusted_ca()->set_filename(TestEnvironment::substitute( "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem")); - dynamic_cvc->add_match_subject_alt_names()->set_exact("second san"); + auto* san_matcher = dynamic_cvc->add_match_typed_subject_alt_names(); + san_matcher->mutable_matcher()->set_exact("second san"); + san_matcher->set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS); const std::string dynamic_verify_certificate_spki = "QGJRPdmx/r5EGOFLb2MTiZp2isyC0Whht7iazhzXaCM="; dynamic_cvc->add_verify_certificate_spki(dynamic_verify_certificate_spki); @@ -681,7 +684,10 @@ TEST_F(SdsApiTest, DefaultCertificateValidationContextTest) { envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext default_cvc; default_cvc.set_allow_expired_certificate(true); default_cvc.mutable_trusted_ca()->set_inline_bytes("fake trusted ca"); - default_cvc.add_match_subject_alt_names()->set_exact("first san"); + san_matcher = default_cvc.add_match_typed_subject_alt_names(); + san_matcher->mutable_matcher()->set_exact("first san"); + san_matcher->set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS); default_cvc.add_verify_certificate_hash(default_verify_certificate_hash); envoy::extensions::transport_sockets::tls::v3::CertificateValidationContext merged_cvc = default_cvc; @@ -697,8 +703,12 @@ TEST_F(SdsApiTest, DefaultCertificateValidationContextTest) { cvc_config.caCert()); // Verify that repeated fields are concatenated. EXPECT_EQ(2, cvc_config.subjectAltNameMatchers().size()); - EXPECT_EQ("first san", cvc_config.subjectAltNameMatchers()[0].exact()); - EXPECT_EQ("second san", cvc_config.subjectAltNameMatchers()[1].exact()); + EXPECT_EQ("first san", cvc_config.subjectAltNameMatchers()[0].matcher().exact()); + EXPECT_EQ(envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS, + cvc_config.subjectAltNameMatchers()[0].san_type()); + EXPECT_EQ("second san", cvc_config.subjectAltNameMatchers()[1].matcher().exact()); + EXPECT_EQ(envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS, + cvc_config.subjectAltNameMatchers()[1].san_type()); // Verify that if dynamic CertificateValidationContext does not set certificate hash list, the new // secret contains hash list from default CertificateValidationContext. EXPECT_EQ(1, cvc_config.verifyCertificateHashList().size()); diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index a73f143ba861..1109b7621256 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -1128,6 +1128,43 @@ name: "abc.com" EnvoyException, "Failed to load private key provider: test"); } +// Verify that using the match_subject_alt_names will result in a typed matcher, one for each of +// DNS, URI, EMAIL and IP_ADDRESS. +// TODO(pradeepcrao): Delete this test once the deprecated field is removed. +TEST_F(SecretManagerImplTest, DeprecatedSanMatcher) { + envoy::extensions::transport_sockets::tls::v3::Secret secret_config; + const std::string yaml = + R"EOF( + name: "abc.com" + validation_context: + trusted_ca: { filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" } + allow_expired_certificate: true + match_subject_alt_names: + exact: "example.foo" + )EOF"; + TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), secret_config); + std::unique_ptr secret_manager(new SecretManagerImpl(config_tracker_)); + secret_manager->addStaticSecret(secret_config); + + ASSERT_EQ(secret_manager->findStaticCertificateValidationContextProvider("undefined"), nullptr); + ASSERT_NE(secret_manager->findStaticCertificateValidationContextProvider("abc.com"), nullptr); + Ssl::CertificateValidationContextConfigImpl cvc_config( + *secret_manager->findStaticCertificateValidationContextProvider("abc.com")->secret(), *api_); + EXPECT_EQ(cvc_config.subjectAltNameMatchers().size(), 4); + EXPECT_EQ("example.foo", cvc_config.subjectAltNameMatchers()[0].matcher().exact()); + EXPECT_EQ(envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS, + cvc_config.subjectAltNameMatchers()[0].san_type()); + EXPECT_EQ("example.foo", cvc_config.subjectAltNameMatchers()[1].matcher().exact()); + EXPECT_EQ(envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::URI, + cvc_config.subjectAltNameMatchers()[1].san_type()); + EXPECT_EQ("example.foo", cvc_config.subjectAltNameMatchers()[2].matcher().exact()); + EXPECT_EQ(envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::EMAIL, + cvc_config.subjectAltNameMatchers()[2].san_type()); + EXPECT_EQ("example.foo", cvc_config.subjectAltNameMatchers()[3].matcher().exact()); + EXPECT_EQ(envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::IP_ADDRESS, + cvc_config.subjectAltNameMatchers()[3].san_type()); +} + } // namespace } // namespace Secret } // namespace Envoy diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index 79efee3c96b9..b608854e352b 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -3393,9 +3393,13 @@ TEST_F(ClusterInfoImplTest, Http3) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - - exact: localhost - - exact: 127.0.0.1 + match_typed_subject_alt_names: + - matcher: + exact: localhost + san_type: URI + - matcher: + exact: 127.0.0.1 + san_type: IP_ADDRESS )EOF", Network::Address::IpVersion::v4); auto cluster1 = makeCluster(yaml); @@ -3466,9 +3470,13 @@ TEST_F(ClusterInfoImplTest, Http3BadConfig) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - - exact: localhost - - exact: 127.0.0.1 + match_typed_subject_alt_names: + - matcher: + exact: localhost + san_type: URI + - matcher: + exact: 127.0.0.1 + san_type: IP_ADDRESS typed_extension_protocol_options: envoy.extensions.upstreams.http.v3.HttpProtocolOptions: "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions @@ -3511,9 +3519,13 @@ TEST_F(ClusterInfoImplTest, Http3Auto) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - - exact: localhost - - exact: 127.0.0.1 + match_typed_subject_alt_names: + - matcher: + exact: localhost + san_type: URI + - matcher: + exact: 127.0.0.1 + san_type: IP_ADDRESS )EOF", Network::Address::IpVersion::v4); @@ -3570,9 +3582,13 @@ TEST_F(ClusterInfoImplTest, UseDownstreamHttpProtocolWithoutDowngrade) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - - exact: localhost - - exact: 127.0.0.1 + match_typed_subject_alt_names: + - matcher: + exact: localhost + san_type: URI + - matcher: + exact: 127.0.0.1 + san_type: IP_ADDRESS typed_extension_protocol_options: envoy.extensions.upstreams.http.v3.HttpProtocolOptions: "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions diff --git a/test/config/utility.cc b/test/config/utility.cc index a2d7c9df517c..e3c360f7168c 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -1191,8 +1191,8 @@ void ConfigHelper::initializeTls( } } if (!options.san_matchers_.empty()) { - *validation_context->mutable_match_subject_alt_names() = {options.san_matchers_.begin(), - options.san_matchers_.end()}; + *validation_context->mutable_match_typed_subject_alt_names() = {options.san_matchers_.begin(), + options.san_matchers_.end()}; } } diff --git a/test/config/utility.h b/test/config/utility.h index f188f8ea1bcd..6c310ec3cf36 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -14,6 +14,7 @@ #include "envoy/config/route/v3/route_components.pb.h" #include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" #include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h" +#include "envoy/extensions/transport_sockets/tls/v3/common.pb.h" #include "envoy/extensions/upstreams/http/v3/http_protocol_options.pb.h" #include "envoy/http/codes.h" @@ -80,7 +81,8 @@ class ConfigHelper { } ServerSslOptions& - setSanMatchers(std::vector san_matchers) { + setSanMatchers(std::vector + san_matchers) { san_matchers_ = san_matchers; return *this; } @@ -94,7 +96,8 @@ class ConfigHelper { bool ocsp_staple_required_{false}; bool tlsv1_3_{false}; bool expect_client_ecdsa_cert_{false}; - std::vector san_matchers_{}; + std::vector + san_matchers_{}; }; // Set up basic config, using the specified IpVersion for all connections: listeners, upstream, diff --git a/test/extensions/transport_sockets/tls/cert_validator/BUILD b/test/extensions/transport_sockets/tls/cert_validator/BUILD index 7fd2c97084a1..9d880d51fa05 100644 --- a/test/extensions/transport_sockets/tls/cert_validator/BUILD +++ b/test/extensions/transport_sockets/tls/cert_validator/BUILD @@ -57,3 +57,16 @@ envoy_cc_test_library( "//test/test_common:utility_lib", ], ) + +envoy_cc_test( + name = "san_matcher_test", + srcs = [ + "san_matcher_test.cc", + ], + deps = [ + "//source/common/protobuf:utility_lib", + "//source/extensions/transport_sockets/tls/cert_validator:cert_validator_lib", + "//test/test_common:utility_lib", + "@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto", + ], +) 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 68925f05930d..2842c642e959 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 @@ -2,6 +2,7 @@ #include #include "source/extensions/transport_sockets/tls/cert_validator/default_validator.h" +#include "source/extensions/transport_sockets/tls/cert_validator/san_matcher.h" #include "test/extensions/transport_sockets/tls/ssl_test_utility.h" #include "test/test_common/environment.h" @@ -42,22 +43,33 @@ TEST(DefaultCertValidatorTest, TestMatchSubjectAltNameDNSMatched) { bssl::UniquePtr cert = readCertFromFile(TestEnvironment::substitute( "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns_cert.pem")); envoy::type::matcher::v3::StringMatcher matcher; - matcher.MergeFrom(TestUtility::createRegexMatcher(".*.example.com")); - std::vector> - subject_alt_name_matchers; - subject_alt_name_matchers.push_back(Matchers::StringMatcherImpl(matcher)); + matcher.MergeFrom(TestUtility::createRegexMatcher(R"raw([^.]*\.example.com)raw")); + std::vector subject_alt_name_matchers; + subject_alt_name_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_DNS, matcher)}); EXPECT_TRUE(DefaultCertValidator::matchSubjectAltName(cert.get(), subject_alt_name_matchers)); } +TEST(DefaultCertValidatorTest, TestMatchSubjectAltNameIncorrectTypeMatched) { + bssl::UniquePtr cert = readCertFromFile(TestEnvironment::substitute( + "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns_cert.pem")); + envoy::type::matcher::v3::StringMatcher matcher; + matcher.MergeFrom(TestUtility::createRegexMatcher(R"raw([^.]*\.example.com)raw")); + std::vector subject_alt_name_matchers; + subject_alt_name_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_URI, matcher)}); + EXPECT_FALSE(DefaultCertValidator::matchSubjectAltName(cert.get(), subject_alt_name_matchers)); +} + TEST(DefaultCertValidatorTest, TestMatchSubjectAltNameWildcardDNSMatched) { bssl::UniquePtr cert = readCertFromFile(TestEnvironment::substitute( "{{ test_rundir " "}}/test/extensions/transport_sockets/tls/test_data/san_multiple_dns_cert.pem")); envoy::type::matcher::v3::StringMatcher matcher; matcher.set_exact("api.example.com"); - std::vector> - subject_alt_name_matchers; - subject_alt_name_matchers.push_back(Matchers::StringMatcherImpl(matcher)); + std::vector subject_alt_name_matchers; + subject_alt_name_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_DNS, matcher)}); EXPECT_TRUE(DefaultCertValidator::matchSubjectAltName(cert.get(), subject_alt_name_matchers)); } @@ -68,9 +80,9 @@ TEST(DefaultCertValidatorTest, TestMultiLevelMatch) { "}}/test/extensions/transport_sockets/tls/test_data/san_multiple_dns_cert.pem")); envoy::type::matcher::v3::StringMatcher matcher; matcher.set_exact("foo.api.example.com"); - std::vector> - subject_alt_name_matchers; - subject_alt_name_matchers.push_back(Matchers::StringMatcherImpl(matcher)); + std::vector subject_alt_name_matchers; + subject_alt_name_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_DNS, matcher)}); EXPECT_FALSE(DefaultCertValidator::matchSubjectAltName(cert.get(), subject_alt_name_matchers)); } @@ -95,10 +107,10 @@ TEST(DefaultCertValidatorTest, TestMatchSubjectAltNameURIMatched) { bssl::UniquePtr cert = readCertFromFile(TestEnvironment::substitute( "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_cert.pem")); envoy::type::matcher::v3::StringMatcher matcher; - matcher.MergeFrom(TestUtility::createRegexMatcher("spiffe://lyft.com/.*-team")); - std::vector> - subject_alt_name_matchers; - subject_alt_name_matchers.push_back(Matchers::StringMatcherImpl(matcher)); + matcher.MergeFrom(TestUtility::createRegexMatcher(R"raw(spiffe://lyft.com/[^/]*-team)raw")); + std::vector subject_alt_name_matchers; + subject_alt_name_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_URI, matcher)}); EXPECT_TRUE(DefaultCertValidator::matchSubjectAltName(cert.get(), subject_alt_name_matchers)); } @@ -114,10 +126,16 @@ TEST(DefaultCertValidatorTest, TestMatchSubjectAltNameNotMatched) { bssl::UniquePtr cert = readCertFromFile(TestEnvironment::substitute( "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns_cert.pem")); envoy::type::matcher::v3::StringMatcher matcher; - matcher.MergeFrom(TestUtility::createRegexMatcher(".*.foo.com")); - std::vector> - subject_alt_name_matchers; - subject_alt_name_matchers.push_back(Matchers::StringMatcherImpl(matcher)); + matcher.MergeFrom(TestUtility::createRegexMatcher(R"raw([^.]*\.example\.net)raw")); + std::vector subject_alt_name_matchers; + subject_alt_name_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_DNS, matcher)}); + subject_alt_name_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_IPADD, matcher)}); + subject_alt_name_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_URI, matcher)}); + subject_alt_name_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_EMAIL, matcher)}); EXPECT_FALSE(DefaultCertValidator::matchSubjectAltName(cert.get(), subject_alt_name_matchers)); } @@ -133,18 +151,18 @@ TEST(DefaultCertValidatorTest, TestCertificateVerificationWithSANMatcher) { bssl::UniquePtr cert = readCertFromFile(TestEnvironment::substitute( "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns_cert.pem")); envoy::type::matcher::v3::StringMatcher matcher; - matcher.MergeFrom(TestUtility::createRegexMatcher(".*.example.com")); - std::vector> san_matchers; - san_matchers.push_back(Matchers::StringMatcherImpl(matcher)); + matcher.MergeFrom(TestUtility::createRegexMatcher(R"raw([^.]*\.example.com)raw")); + std::vector san_matchers; + san_matchers.push_back(SanMatcherPtr{std::make_unique(GEN_DNS, matcher)}); // Verify the certificate with correct SAN regex matcher. EXPECT_EQ(default_validator->verifyCertificate(cert.get(), /*verify_san_list=*/{}, san_matchers), Envoy::Ssl::ClientValidationStatus::Validated); EXPECT_EQ(stats.fail_verify_san_.value(), 0); matcher.MergeFrom(TestUtility::createExactMatcher("hello.example.com")); - std::vector> - invalid_san_matchers; - invalid_san_matchers.push_back(Matchers::StringMatcherImpl(matcher)); + std::vector invalid_san_matchers; + invalid_san_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_DNS, matcher)}); // Verify the certificate with incorrect SAN exact matcher. EXPECT_EQ(default_validator->verifyCertificate(cert.get(), /*verify_san_list=*/{}, invalid_san_matchers), @@ -172,6 +190,17 @@ TEST(DefaultCertValidatorTest, TestCertificateVerificationWithNoValidationContex 0); } +TEST(DefaultCertValidatorTest, NoSanInCert) { + bssl::UniquePtr cert = readCertFromFile(TestEnvironment::substitute( + "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/fake_ca_cert.pem")); + envoy::type::matcher::v3::StringMatcher matcher; + matcher.MergeFrom(TestUtility::createRegexMatcher(R"raw([^.]*\.example\.net)raw")); + std::vector subject_alt_name_matchers; + subject_alt_name_matchers.push_back( + SanMatcherPtr{std::make_unique(GEN_DNS, matcher)}); + EXPECT_FALSE(DefaultCertValidator::matchSubjectAltName(cert.get(), subject_alt_name_matchers)); +} + } // namespace Tls } // namespace TransportSockets } // namespace Extensions diff --git a/test/extensions/transport_sockets/tls/cert_validator/san_matcher_test.cc b/test/extensions/transport_sockets/tls/cert_validator/san_matcher_test.cc new file mode 100644 index 000000000000..b7729de8d41e --- /dev/null +++ b/test/extensions/transport_sockets/tls/cert_validator/san_matcher_test.cc @@ -0,0 +1,57 @@ +#include "envoy/extensions/transport_sockets/tls/v3/common.pb.validate.h" + +#include "source/common/protobuf/message_validator_impl.h" +#include "source/common/protobuf/utility.h" +#include "source/extensions/transport_sockets/tls/cert_validator/san_matcher.h" + +#include "test/test_common/utility.h" + +#include "gmock/gmock.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace TransportSockets { +namespace Tls { + +// Verify that we get a valid string san matcher for all valid san types. +TEST(SanMatcherConfigTest, TestValidSanType) { + // Iterate over all san type enums. + for (envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::SanType san_type = + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::SanType_MIN; + san_type <= + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::SanType_MAX; + san_type = static_cast< + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::SanType>( + static_cast(san_type + 1))) { + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher san_matcher; + san_matcher.mutable_matcher()->set_exact("foo.example"); + san_matcher.set_san_type(san_type); + if (san_type == envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher:: + SAN_TYPE_UNSPECIFIED) { + continue; + } else { + const SanMatcherPtr matcher = createStringSanMatcher(san_matcher); + EXPECT_NE(matcher.get(), nullptr); + // Verify that the message is valid. + TestUtility::validate(san_matcher); + } + } +} + +TEST(SanMatcherConfigTest, UnspecifiedSanType) { + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher san_matcher; + san_matcher.mutable_matcher()->set_exact("foo.example"); + // Do not set san_type + EXPECT_THROW_WITH_REGEX(TestUtility::validate(san_matcher), EnvoyException, + "Proto constraint validation failed"); + san_matcher.set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::SAN_TYPE_UNSPECIFIED); + EXPECT_THROW_WITH_REGEX(TestUtility::validate(san_matcher), EnvoyException, + "Proto constraint validation failed"); +} + +} // namespace Tls +} // namespace TransportSockets +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_integration_test.cc b/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_integration_test.cc index 47acf98c4f53..fe47ef14f6e3 100644 --- a/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_integration_test.cc +++ b/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_integration_test.cc @@ -53,6 +53,26 @@ void SslSPIFFECertValidatorIntegrationTest::checkVerifyErrorCouter(uint64_t valu counter->reset(); } +void SslSPIFFECertValidatorIntegrationTest::addStringMatcher( + const envoy::type::matcher::v3::StringMatcher& matcher) { + san_matchers_.emplace_back(); + *san_matchers_.back().mutable_matcher() = matcher; + san_matchers_.back().set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS); + san_matchers_.emplace_back(); + *san_matchers_.back().mutable_matcher() = matcher; + san_matchers_.back().set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::URI); + san_matchers_.emplace_back(); + *san_matchers_.back().mutable_matcher() = matcher; + san_matchers_.back().set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::EMAIL); + san_matchers_.emplace_back(); + *san_matchers_.back().mutable_matcher() = matcher; + san_matchers_.back().set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::IP_ADDRESS); +} + INSTANTIATE_TEST_SUITE_P( IpVersionsClientVersions, SslSPIFFECertValidatorIntegrationTest, testing::Combine( @@ -124,7 +144,7 @@ name: envoy.tls.cert_validator.spiffe envoy::type::matcher::v3::StringMatcher matcher; matcher.set_prefix("spiffe://lyft.com/"); - san_matchers_ = {matcher}; + addStringMatcher(matcher); ConnectionCreationFunction creator = [&]() -> Network::ClientConnectionPtr { return makeSslClientConnection({}); @@ -152,7 +172,7 @@ name: envoy.tls.cert_validator.spiffe matcher.set_prefix("spiffe://example.com/"); // The cert has "DNS.1 = lyft.com" but SPIFFE validator must ignore SAN types other than URI. matcher.set_prefix("www.lyft.com"); - san_matchers_ = {matcher}; + addStringMatcher(matcher); initialize(); auto conn = makeSslClientConnection({}); if (tls_version_ == envoy::extensions::transport_sockets::tls::v3::TlsParameters::TLSv1_2) { @@ -223,8 +243,8 @@ name: envoy.tls.cert_validator.spiffe checkVerifyErrorCouter(1); } -// clientcert.pem's san is "spiffe://lyft.com/frontend-team" but the corresponding trust bundle does -// not match with the client cert. So this should also be rejected. +// clientcert.pem's san is "spiffe://lyft.com/frontend-team" but the corresponding trust bundle +// does not match with the client cert. So this should also be rejected. TEST_P(SslSPIFFECertValidatorIntegrationTest, ServerRsaSPIFFEValidatorRejected2) { auto typed_conf = new envoy::config::core::v3::TypedExtensionConfig(); TestUtility::loadFromYaml(TestEnvironment::substitute(R"EOF( diff --git a/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_integration_test.h b/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_integration_test.h index b1e57169ea18..01d08f5a811d 100644 --- a/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_integration_test.h +++ b/test/extensions/transport_sockets/tls/cert_validator/spiffe/spiffe_validator_integration_test.h @@ -39,10 +39,11 @@ class SslSPIFFECertValidatorIntegrationTest } protected: + void addStringMatcher(envoy::type::matcher::v3::StringMatcher const& matcher); bool allow_expired_cert_{}; envoy::config::core::v3::TypedExtensionConfig* custom_validator_config_{nullptr}; std::unique_ptr context_manager_; - std::vector san_matchers_; + std::vector san_matchers_; const envoy::extensions::transport_sockets::tls::v3::TlsParameters::TlsProtocol tls_version_{ std::get<1>(GetParam())}; }; 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 cf77bdf3737a..c865b9f585f4 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 @@ -64,13 +64,34 @@ class TestSPIFFEValidator : public testing::Test { // Setter. void setAllowExpiredCertificate(bool val) { allow_expired_certificate_ = val; } void setSanMatchers(std::vector san_matchers) { - san_matchers_ = san_matchers; + san_matchers_.clear(); + for (auto& matcher : san_matchers) { + san_matchers_.emplace_back(); + san_matchers_.back().set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS); + *san_matchers_.back().mutable_matcher() = matcher; + + san_matchers_.emplace_back(); + san_matchers_.back().set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::URI); + *san_matchers_.back().mutable_matcher() = matcher; + + san_matchers_.emplace_back(); + san_matchers_.back().set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::EMAIL); + *san_matchers_.back().mutable_matcher() = matcher; + + san_matchers_.emplace_back(); + san_matchers_.back().set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::IP_ADDRESS); + *san_matchers_.back().mutable_matcher() = matcher; + } }; private: bool allow_expired_certificate_{false}; TestCertificateValidationContextConfigPtr config_; - std::vector san_matchers_{}; + std::vector san_matchers_{}; Stats::TestUtil::TestStore store_; SslStats stats_; Event::TestRealTimeSystem time_system_; @@ -193,7 +214,8 @@ TEST_F(TestSPIFFEValidator, TestGetTrustBundleStore) { // Non-SPIFFE SAN cert = readCertFromFile(TestEnvironment::substitute( - "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/non_spiffe_san_cert.pem")); + "{{ test_rundir " + "}}/test/extensions/transport_sockets/tls/test_data/non_spiffe_san_cert.pem")); EXPECT_FALSE(validator().getTrustBundleStore(cert.get())); // SPIFFE SAN diff --git a/test/extensions/transport_sockets/tls/cert_validator/test_common.h b/test/extensions/transport_sockets/tls/cert_validator/test_common.h index b958f1727207..cad834c9f259 100644 --- a/test/extensions/transport_sockets/tls/cert_validator/test_common.h +++ b/test/extensions/transport_sockets/tls/cert_validator/test_common.h @@ -33,7 +33,8 @@ class TestCertificateValidationContextConfig public: TestCertificateValidationContextConfig( envoy::config::core::v3::TypedExtensionConfig config, bool allow_expired_certificate = false, - std::vector san_matchers = {}) + std::vector + san_matchers = {}) : allow_expired_certificate_(allow_expired_certificate), api_(Api::createApiForTest()), custom_validator_config_(config), san_matchers_(san_matchers){}; TestCertificateValidationContextConfig() @@ -47,7 +48,7 @@ class TestCertificateValidationContextConfig const std::string& certificateRevocationListPath() const final { CONSTRUCT_ON_FIRST_USE(std::string, ""); } - const std::vector& + const std::vector& subjectAltNameMatchers() const override { return san_matchers_; } @@ -77,7 +78,8 @@ class TestCertificateValidationContextConfig bool allow_expired_certificate_{false}; Api::ApiPtr api_; const absl::optional custom_validator_config_; - const std::vector san_matchers_{}; + const std::vector + san_matchers_{}; }; } // namespace Tls diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 0967e77d25b0..8e4bfc66ef77 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -923,8 +923,10 @@ TEST_F(SslServerContextImplTicketTest, VerifySanWithNoCA) { private_key: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns_key.pem" validation_context: - match_subject_alt_names: - exact : "spiffe://lyft.com/testclient" + match_typed_subject_alt_names: + - san_type: URI + matcher: + exact: "spiffe://lyft.com/testclient" )EOF"; EXPECT_THROW_WITH_MESSAGE(loadConfigYaml(yaml), EnvoyException, "SAN-based verification of peer certificates without trusted CA " @@ -1769,6 +1771,35 @@ TEST_F(ServerContextConfigImplTest, PrivateKeyMethodLoadFailureBothKeyAndMethod) "Certificate configuration can't have both private_key and private_key_provider"); } +// Test that we don't allow specification of both typed and untyped matchers for +// sans. +TEST_F(ServerContextConfigImplTest, DeprecatedSanMatcher) { + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context; + NiceMock context_manager; + NiceMock private_key_method_manager; + auto private_key_method_provider_ptr = + std::make_shared>(); + const std::string yaml = + R"EOF( + common_tls_context: + validation_context: + trusted_ca: { filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" } + allow_expired_certificate: true + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: "foo.example" + match_subject_alt_names: + exact: "foo.example" + )EOF"; + TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); + + EXPECT_THROW_WITH_MESSAGE( + ServerContextConfigImpl server_context_config(tls_context, factory_context_), EnvoyException, + "SAN-based verification using both match_typed_subject_alt_names and " + "the deprecated match_subject_alt_names is not allowed"); +} + // Subclass ContextImpl so we can instantiate directly from tests, despite the // constructor being protected. class TestContextImpl : public ContextImpl { diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index c85ae51bfac6..52519f6df0fa 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -1076,8 +1076,10 @@ TEST_P(SslSocketTest, GetUriWithUriSan) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - exact: "spiffe://lyft.com/test-team" + match_typed_subject_alt_names: + - san_type: URI + matcher: + exact: "spiffe://lyft.com/test-team" )EOF"; TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, true, GetParam()); @@ -1092,8 +1094,10 @@ TEST_P(SslSocketTest, Ipv4San) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/config/integration/certs/upstreamcacert.pem" - match_subject_alt_names: - exact: "127.0.0.1" + match_typed_subject_alt_names: + - san_type: IP_ADDRESS + matcher: + exact: "127.0.0.1" )EOF"; const std::string server_ctx_yaml = R"EOF( @@ -1116,8 +1120,10 @@ TEST_P(SslSocketTest, Ipv6San) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/config/integration/certs/upstreamcacert.pem" - match_subject_alt_names: - exact: "::1" + match_typed_subject_alt_names: + - san_type: IP_ADDRESS + matcher: + exact: "::1" )EOF"; const std::string server_ctx_yaml = R"EOF( @@ -1519,8 +1525,10 @@ TEST_P(SslSocketTest, FailedClientAuthSanVerificationNoClientCert) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - exact: "example.com" + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: "example.com" )EOF"; TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, false, GetParam()); @@ -1547,8 +1555,10 @@ TEST_P(SslSocketTest, FailedClientAuthSanVerification) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - exact: "example.com" + match_typed_subject_alt_names: + - san_type: DNS + matcher: + exact: "example.com" )EOF"; TestUtilOptions test_options(client_ctx_yaml, server_ctx_yaml, false, GetParam()); diff --git a/test/integration/ads_integration.cc b/test/integration/ads_integration.cc index c1758c067006..08a8eafdaf19 100644 --- a/test/integration/ads_integration.cc +++ b/test/integration/ads_integration.cc @@ -136,7 +136,10 @@ void AdsIntegrationTest::initializeAds(const bool rate_limiting) { auto* validation_context = context.mutable_common_tls_context()->mutable_validation_context(); validation_context->mutable_trusted_ca()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/upstreamcacert.pem")); - validation_context->add_match_subject_alt_names()->set_suffix("lyft.com"); + auto* san_matcher = validation_context->add_match_typed_subject_alt_names(); + san_matcher->mutable_matcher()->set_suffix("lyft.com"); + san_matcher->set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS); if (clientType() == Grpc::ClientType::GoogleGrpc) { auto* google_grpc = grpc_service->mutable_google_grpc(); auto* ssl_creds = google_grpc->mutable_channel_credentials()->mutable_ssl_credentials(); diff --git a/test/integration/ssl_utility.cc b/test/integration/ssl_utility.cc index 36b93b6f81bc..c629d6f2e3c0 100644 --- a/test/integration/ssl_utility.cc +++ b/test/integration/ssl_utility.cc @@ -66,8 +66,23 @@ void initializeUpstreamTlsContextConfig( common_context->add_alpn_protocols(Http::Utility::AlpnNames::get().Http3); } if (!options.san_.empty()) { - common_context->mutable_validation_context()->add_match_subject_alt_names()->set_exact( - options.san_); + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher* matcher = + common_context->mutable_validation_context()->add_match_typed_subject_alt_names(); + matcher->mutable_matcher()->set_exact(options.san_); + matcher->set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS); + matcher = common_context->mutable_validation_context()->add_match_typed_subject_alt_names(); + matcher->mutable_matcher()->set_exact(options.san_); + matcher->set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::URI); + matcher = common_context->mutable_validation_context()->add_match_typed_subject_alt_names(); + matcher->mutable_matcher()->set_exact(options.san_); + matcher->set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::EMAIL); + matcher = common_context->mutable_validation_context()->add_match_typed_subject_alt_names(); + matcher->mutable_matcher()->set_exact(options.san_); + matcher->set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::IP_ADDRESS); } for (const std::string& cipher_suite : options.cipher_suites_) { common_context->mutable_tls_params()->add_cipher_suites(cipher_suite); diff --git a/test/integration/xfcc_integration_test.cc b/test/integration/xfcc_integration_test.cc index 19186660db21..0ce4d4a94213 100644 --- a/test/integration/xfcc_integration_test.cc +++ b/test/integration/xfcc_integration_test.cc @@ -6,6 +6,7 @@ #include "envoy/config/bootstrap/v3/bootstrap.pb.h" #include "envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.pb.h" #include "envoy/extensions/transport_sockets/tls/v3/cert.pb.h" +#include "envoy/extensions/transport_sockets/tls/v3/common.pb.h" #include "envoy/stats/scope.h" #include "source/common/event/dispatcher_impl.h" @@ -45,10 +46,16 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createClientSslContext(b validation_context: trusted_ca: filename: {{ test_rundir }}/test/config/integration/certs/cacert.pem - match_subject_alt_names: - exact: "spiffe://lyft.com/backend-team" - exact: "lyft.com" - exact: "www.lyft.com" + match_typed_subject_alt_names: + - san_type: URI + matcher: + exact: "spiffe://lyft.com/backend-team" + - san_type: DNS + matcher: + exact: "lyft.com" + - san_type: DNS + matcher: + exact: "www.lyft.com" )EOF"; const std::string yaml_mtls = R"EOF( @@ -56,10 +63,16 @@ Network::TransportSocketFactoryPtr XfccIntegrationTest::createClientSslContext(b validation_context: trusted_ca: filename: {{ test_rundir }}/test/config/integration/certs/cacert.pem - match_subject_alt_names: - exact: "spiffe://lyft.com/backend-team" - exact: "lyft.com" - exact: "www.lyft.com" + match_typed_subject_alt_names: + - san_type: URI + matcher: + exact: "spiffe://lyft.com/backend-team" + - san_type: DNS + matcher: + exact: "lyft.com" + - san_type: DNS + matcher: + exact: "www.lyft.com" tls_certificates: certificate_chain: filename: {{ test_rundir }}/test/config/integration/certs/clientcert.pem @@ -135,7 +148,10 @@ void XfccIntegrationTest::initialize() { auto* validation_context = context.mutable_common_tls_context()->mutable_validation_context(); validation_context->mutable_trusted_ca()->set_filename( TestEnvironment::runfilesPath("test/config/integration/certs/upstreamcacert.pem")); - validation_context->add_match_subject_alt_names()->set_suffix("lyft.com"); + auto* san_matcher = validation_context->add_match_typed_subject_alt_names(); + san_matcher->mutable_matcher()->set_suffix("lyft.com"); + san_matcher->set_san_type( + envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher::DNS); transport_socket->set_name("envoy.transport_sockets.tls"); transport_socket->mutable_typed_config()->PackFrom(context); }); diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index 43ad275fce27..e36b06fbd99d 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -156,8 +156,9 @@ class MockCertificateValidationContextConfig : public CertificateValidationConte MOCK_METHOD(const std::string&, caCertPath, (), (const)); MOCK_METHOD(const std::string&, certificateRevocationList, (), (const)); MOCK_METHOD(const std::string&, certificateRevocationListPath, (), (const)); - MOCK_METHOD(const std::vector&, subjectAltNameMatchers, - (), (const)); + MOCK_METHOD( + const std::vector&, + subjectAltNameMatchers, (), (const)); MOCK_METHOD(const std::vector&, verifyCertificateHashList, (), (const)); MOCK_METHOD(const std::vector&, verifyCertificateSpkiList, (), (const)); MOCK_METHOD(bool, allowExpiredCertificate, (), (const)); diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh index fb445b530565..a7603f4af675 100755 --- a/test/per_file_coverage.sh +++ b/test/per_file_coverage.sh @@ -65,9 +65,9 @@ declare -a KNOWN_LOW_COVERAGE=( "source/extensions/tracers/opencensus:94.8" "source/extensions/tracers/xray:96.2" "source/extensions/tracers/zipkin:96.1" -"source/extensions/transport_sockets:95.4" +"source/extensions/transport_sockets:95.3" "source/extensions/transport_sockets/tls:94.6" -"source/extensions/transport_sockets/tls/cert_validator:96.0" +"source/extensions/transport_sockets/tls/cert_validator:95.8" "source/extensions/transport_sockets/tls/ocsp:96.5" "source/extensions/transport_sockets/tls/private_key:77.8" "source/extensions/wasm_runtime/wamr:0.0" # Not enabled in coverage build diff --git a/test/server/listener_manager_impl_quic_only_test.cc b/test/server/listener_manager_impl_quic_only_test.cc index 84e1a3a200c9..f505b467c99d 100644 --- a/test/server/listener_manager_impl_quic_only_test.cc +++ b/test/server/listener_manager_impl_quic_only_test.cc @@ -60,9 +60,13 @@ TEST_F(ListenerManagerImplQuicOnlyTest, QuicListenerFactoryAndSslContext) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - - exact: localhost - - exact: 127.0.0.1 + match_typed_subject_alt_names: + - matcher: + exact: localhost + san_type: URI + - matcher: + exact: 127.0.0.1 + san_type: IP_ADDRESS udp_listener_config: quic_options: {} )EOF", @@ -161,9 +165,13 @@ TEST_F(ListenerManagerImplQuicOnlyTest, QuicListenerFactoryWithWrongTransportSoc validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - - exact: localhost - - exact: 127.0.0.1 + match_typed_subject_alt_names: + - matcher: + exact: localhost + san_type: URI + - matcher: + exact: 127.0.0.1 + san_type: IP_ADDRESS udp_listener_config: quic_options: {} )EOF", @@ -205,9 +213,13 @@ TEST_F(ListenerManagerImplQuicOnlyTest, QuicListenerFactoryWithWrongCodec) { validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - - exact: localhost - - exact: 127.0.0.1 + match_typed_subject_alt_names: + - matcher: + exact: localhost + san_type: URI + - matcher: + exact: 127.0.0.1 + san_type: IP_ADDRESS udp_listener_config: quic_options: {} )EOF", @@ -259,9 +271,13 @@ TEST_F(ListenerManagerImplQuicOnlyTest, QuicListenerFactoryWithConnectionBalence validation_context: trusted_ca: filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" - match_subject_alt_names: - - exact: localhost - - exact: 127.0.0.1 + match_typed_subject_alt_names: + - matcher: + exact: localhost + san_type: URI + - matcher: + exact: 127.0.0.1 + san_type: IP_ADDRESS udp_listener_config: quic_options: {} connection_balance_config: From c087cca67850abe2b5ad6170af2345bdcca7ba97 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 28 Jan 2022 14:15:39 +0100 Subject: [PATCH 05/15] [1.20] CVE-2021-43825 Response filter manager crash Signed-off-by: Otto van der Schaaf --- source/common/http/conn_manager_impl.cc | 6 +- source/common/http/filter_manager.cc | 15 +++- source/common/http/filter_manager.h | 16 ++-- test/integration/BUILD | 1 + test/integration/filters/BUILD | 14 ++++ .../filters/buffer_continue_filter.cc | 74 +++++++++++++++++++ test/integration/protocol_integration_test.cc | 52 +++++++++++++ 7 files changed, 165 insertions(+), 13 deletions(-) create mode 100644 test/integration/filters/buffer_continue_filter.cc diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 90f6be301b63..6530a473f230 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -197,8 +197,8 @@ void ConnectionManagerImpl::doEndStream(ActiveStream& stream) { // here is when Envoy "ends" the stream by calling recreateStream at which point recreateStream // explicitly nulls out response_encoder to avoid the downstream being notified of the // Envoy-internal stream instance being ended. - if (stream.response_encoder_ != nullptr && - (!stream.filter_manager_.remoteComplete() || !stream.state_.codec_saw_local_complete_)) { + if (stream.response_encoder_ != nullptr && (!stream.filter_manager_.remoteDecodeComplete() || + !stream.state_.codec_saw_local_complete_)) { // Indicate local is complete at this point so that if we reset during a continuation, we don't // raise further data or trailers. ENVOY_STREAM_LOG(debug, "doEndStream() resetting stream", stream); @@ -1416,7 +1416,7 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ResponseHeaderMap& heade // If we are destroying a stream before remote is complete and the connection does not support // multiplexing, we should disconnect since we don't want to wait around for the request to // finish. - if (!filter_manager_.remoteComplete()) { + if (!filter_manager_.remoteDecodeComplete()) { if (connection_manager_.codec_->protocol() < Protocol::Http2) { connection_manager_.drain_state_ = DrainState::Closing; } diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index fc15f8041f22..46e9fda6f6fb 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -303,6 +303,12 @@ bool ActiveStreamDecoderFilter::canContinue() { return !parent_.state_.local_complete_; } +bool ActiveStreamEncoderFilter::canContinue() { + // As with ActiveStreamDecoderFilter::canContinue() make sure we do not + // continue if a local reply has been sent. + return !parent_.state_.remote_encode_complete_; +} + Buffer::InstancePtr ActiveStreamDecoderFilter::createBuffer() { auto buffer = dispatcher().getWatermarkFactory().createBuffer( [this]() -> void { this->requestDataDrained(); }, @@ -316,7 +322,7 @@ Buffer::InstancePtr& ActiveStreamDecoderFilter::bufferedData() { return parent_.buffered_request_data_; } -bool ActiveStreamDecoderFilter::complete() { return parent_.state_.remote_complete_; } +bool ActiveStreamDecoderFilter::complete() { return parent_.state_.remote_decode_complete_; } void ActiveStreamDecoderFilter::doHeaders(bool end_stream) { parent_.decodeHeaders(this, *parent_.filter_manager_callbacks_.requestHeaders(), end_stream); @@ -828,8 +834,8 @@ void FilterManager::decodeMetadata(ActiveStreamDecoderFilter* filter, MetadataMa } void FilterManager::maybeEndDecode(bool end_stream) { - ASSERT(!state_.remote_complete_); - state_.remote_complete_ = end_stream; + ASSERT(!state_.remote_decode_complete_); + state_.remote_decode_complete_ = end_stream; if (end_stream) { stream_info_.onLastDownstreamRxByteReceived(); ENVOY_STREAM_LOG(debug, "request end stream", *this); @@ -1338,6 +1344,8 @@ void FilterManager::encodeTrailers(ActiveStreamEncoderFilter* filter, void FilterManager::maybeEndEncode(bool end_stream) { if (end_stream) { + ASSERT(!state_.remote_encode_complete_); + state_.remote_encode_complete_ = true; filter_manager_callbacks_.endStream(); } } @@ -1627,6 +1635,7 @@ Http1StreamEncoderOptionsOptRef ActiveStreamEncoderFilter::http1StreamEncoderOpt } void ActiveStreamEncoderFilter::responseDataTooLarge() { + ENVOY_STREAM_LOG(debug, "response data too large watermark exceeded", parent_); if (parent_.state_.encoder_filters_streaming_) { onEncoderFilterAboveWriteBufferHighWatermark(); } else { diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 633fa6862d3d..9d8718a48c6b 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -315,7 +315,7 @@ struct ActiveStreamEncoderFilter : public ActiveStreamFilterBase, : ActiveStreamFilterBase(parent, dual_filter, std::move(match_state)), handle_(filter) {} // ActiveStreamFilterBase - bool canContinue() override { return true; } + bool canContinue() override; Buffer::InstancePtr createBuffer() override; Buffer::InstancePtr& bufferedData() override; bool complete() override; @@ -907,7 +907,7 @@ class FilterManager : public ScopeTrackedObject, /** * Whether remote processing has been marked as complete. */ - bool remoteComplete() const { return state_.remote_complete_; } + bool remoteDecodeComplete() const { return state_.remote_decode_complete_; } /** * Instructs the FilterManager to not create a filter chain. This makes it possible to issue @@ -1059,14 +1059,16 @@ class FilterManager : public ScopeTrackedObject, struct State { State() - : remote_complete_(false), local_complete_(false), has_continue_headers_(false), - created_filter_chain_(false), is_head_request_(false), is_grpc_request_(false), - non_100_response_headers_encoded_(false), under_on_local_reply_(false), - decoder_filter_chain_aborted_(false), encoder_filter_chain_aborted_(false) {} + : remote_encode_complete_(false), remote_decode_complete_(false), local_complete_(false), + has_continue_headers_(false), created_filter_chain_(false), is_head_request_(false), + is_grpc_request_(false), non_100_response_headers_encoded_(false), + under_on_local_reply_(false), decoder_filter_chain_aborted_(false), + encoder_filter_chain_aborted_(false) {} uint32_t filter_call_state_{0}; - bool remote_complete_ : 1; + bool remote_encode_complete_ : 1; + bool remote_decode_complete_ : 1; bool local_complete_ : 1; // This indicates that local is complete prior to filter processing. // A filter can still stop the stream from being complete as seen // by the codec. diff --git a/test/integration/BUILD b/test/integration/BUILD index 5e631735ef7f..38b514c5496a 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -509,6 +509,7 @@ envoy_cc_test_library( "//source/extensions/filters/http/buffer:config", "//source/extensions/filters/http/health_check:config", "//test/common/http/http2:http2_frame", + "//test/integration/filters:buffer_continue_filter_lib", "//test/integration/filters:continue_after_local_reply_filter_lib", "//test/integration/filters:continue_headers_only_inject_body", "//test/integration/filters:encoder_decoder_buffer_filter_lib", diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 79e08f2533a4..beab9eccde08 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -514,6 +514,20 @@ envoy_cc_test_library( ], ) +envoy_cc_test_library( + name = "buffer_continue_filter_lib", + srcs = [ + "buffer_continue_filter.cc", + ], + deps = [ + "//envoy/http:filter_interface", + "//envoy/registry", + "//envoy/server:filter_config_interface", + "//source/extensions/filters/http/common:pass_through_filter_lib", + "//test/extensions/filters/http/common:empty_http_filter_config_lib", + ], +) + envoy_cc_test_library( name = "test_socket_interface_lib", srcs = [ diff --git a/test/integration/filters/buffer_continue_filter.cc b/test/integration/filters/buffer_continue_filter.cc new file mode 100644 index 000000000000..d5f5dadd690d --- /dev/null +++ b/test/integration/filters/buffer_continue_filter.cc @@ -0,0 +1,74 @@ +#include + +#include "envoy/http/filter.h" +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "source/extensions/filters/http/common/pass_through_filter.h" + +#include "test/extensions/filters/http/common/empty_http_filter_config.h" + +namespace Envoy { + +// A filter that buffers until the limit is reached and then continues. +class BufferContinueStreamFilter : public Http::PassThroughFilter { +public: + Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool end_stream) override { + return end_stream ? Http::FilterHeadersStatus::Continue + : Http::FilterHeadersStatus::StopIteration; + } + + Http::FilterDataStatus decodeData(Buffer::Instance&, bool end_stream) override { + return end_stream ? Http::FilterDataStatus::Continue + : Http::FilterDataStatus::StopIterationAndBuffer; + } + + Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap& headers, bool) override { + response_headers_ = &headers; + return Http::FilterHeadersStatus::StopIteration; + } + + Http::FilterDataStatus encodeData(Buffer::Instance& data, bool end_stream) override { + data_total_ += data.length(); + + const auto limit = encoder_callbacks_->encoderBufferLimit(); + const auto header_size = response_headers_->byteSize(); + + if (limit && header_size + data_total_ > limit) { + // Give up since we've reached the buffer limit, Envoy should generate + // a 500 since it couldn't finished encoding. + return Http::FilterDataStatus::Continue; + } + + encoder_callbacks_->addEncodedData(data, false); + + if (!end_stream) { + return Http::FilterDataStatus::StopIterationAndBuffer; + } + + return Http::FilterDataStatus::Continue; + } + +private: + Http::ResponseHeaderMap* response_headers_; + uint64_t data_total_{0}; +}; + +class BufferContinueFilterConfig : public Extensions::HttpFilters::Common::EmptyHttpFilterConfig { +public: + BufferContinueFilterConfig() : EmptyHttpFilterConfig("buffer-continue-filter") {} + + Http::FilterFactoryCb createFilter(const std::string&, + Server::Configuration::FactoryContext&) override { + return [](Http::FilterChainFactoryCallbacks& callbacks) -> void { + callbacks.addStreamFilter(std::make_shared<::Envoy::BufferContinueStreamFilter>()); + }; + } +}; + +// perform static registration +static Registry::RegisterFactory + register_; + +} // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index a33c7bc0e83d..1c949161ef08 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -3200,4 +3200,56 @@ TEST_P(ProtocolIntegrationTest, FragmentStrippedFromPathWithOverride) { EXPECT_EQ("200", response->headers().getStatusValue()); } +// Test buffering and then continuing after too many response bytes to buffer. +TEST_P(ProtocolIntegrationTest, BufferContinue) { + // Bytes sent is configured for http/2 flow control windows. + if (upstreamProtocol() != Http::CodecType::HTTP2) { + return; + } + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + auto* route_config = hcm.mutable_route_config(); + auto* virtual_host = route_config->mutable_virtual_hosts(0); + auto* header = virtual_host->mutable_response_headers_to_add()->Add()->mutable_header(); + header->set_key("foo"); + header->set_value("bar"); + }); + + useAccessLog(); + config_helper_.addFilter("{ name: buffer-continue-filter, typed_config: { \"@type\": " + "type.googleapis.com/google.protobuf.Empty } }"); + config_helper_.setBufferLimits(1024, 1024); + initialize(); + + // Send the request. + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = codec_client_->startRequest(default_request_headers_); + auto downstream_request = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + Buffer::OwnedImpl data("HTTP body content goes here"); + codec_client_->sendData(*downstream_request, data, true); + waitForNextUpstreamRequest(); + + // Send the response headers. + upstream_request_->encodeHeaders(default_response_headers_, false); + + // Now send an overly large response body. At some point, too much data will + // be buffered, the stream will be reset, and the connection will disconnect. + upstream_request_->encodeData(512, false); + upstream_request_->encodeData(1024 * 100, false); + + if (upstreamProtocol() == Http::CodecType::HTTP1) { + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + } else { + ASSERT_TRUE(upstream_request_->waitForReset()); + ASSERT_TRUE(fake_upstream_connection_->close()); + ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect()); + } + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("500", response->headers().getStatusValue()); +} + } // namespace Envoy From f0bb2219112d8cdb4c4e8b346834f962925362ca Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 28 Jan 2022 13:04:37 +0100 Subject: [PATCH 06/15] [1.20] CVE-2022-21655 Crash with direct_response Signed-off-by: Otto van der Schaaf --- docs/root/version_history/current.rst | 2 + source/common/router/router.cc | 3 +- test/common/router/router_test.cc | 33 +++++++++++++-- test/integration/redirect_integration_test.cc | 42 +++++++++++++++++++ 4 files changed, 76 insertions(+), 4 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 6d6be3ba7085..72c63856d6f3 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -13,6 +13,8 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* +* data plane: fix crash when internal redirect selects a route configured with direct response or redirect actions. + Removed Config or Runtime ------------------------- *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 054b6a6858ec..6bd25a5810b7 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -1624,7 +1624,8 @@ bool Filter::convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& do return false; } - const auto& route_name = route->routeEntry()->routeName(); + const auto& route_name = route->directResponseEntry() ? route->directResponseEntry()->routeName() + : route->routeEntry()->routeName(); for (const auto& predicate : policy.predicates()) { if (!predicate->acceptTargetRoute(*filter_state, route_name, !scheme_is_http, !target_is_http)) { diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 1ff5661d20e0..2fe493982ef8 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -55,6 +55,7 @@ using testing::AtLeast; using testing::Eq; using testing::InSequence; using testing::Invoke; +using testing::InvokeWithoutArgs; using testing::MockFunction; using testing::NiceMock; using testing::Property; @@ -323,7 +324,7 @@ TEST_F(RouterTest, MissingRequiredHeaders) { sendLocalReply(Http::Code::ServiceUnavailable, testing::Eq("missing required header: :method"), _, _, "filter_removed_required_request_headers{missing required header: :method}")) - .WillOnce(testing::InvokeWithoutArgs([] {})); + .WillOnce(InvokeWithoutArgs([] {})); router_.decodeHeaders(headers, true); router_.onDestroy(); } @@ -3775,8 +3776,7 @@ TEST_F(RouterTest, RetryUpstreamResetResponseStarted) { // Normally, sendLocalReply will actually send the reply, but in this case the // HCM will detect the headers have already been sent and not route through // the encoder again. - EXPECT_CALL(callbacks_, sendLocalReply(_, _, _, _, _)).WillOnce(testing::InvokeWithoutArgs([] { - })); + EXPECT_CALL(callbacks_, sendLocalReply(_, _, _, _, _)).WillOnce(InvokeWithoutArgs([] {})); encoder1.stream_.resetStream(Http::StreamResetReason::RemoteReset); // For normal HTTP, once we have a 200 we consider this a success, even if a // later reset occurs. @@ -4736,6 +4736,33 @@ TEST_F(RouterTest, HttpInternalRedirectSucceeded) { .value()); } +TEST_F(RouterTest, HttpInternalRedirectMatchedToDirectResponseSucceeded) { + NiceMock direct_response; + std::string route_name("route-test-name"); + EXPECT_CALL(direct_response, routeName()).WillOnce(ReturnRef(route_name)); + + enableRedirects(); + sendRequest(); + EXPECT_CALL(callbacks_, clearRouteCache()).WillOnce(InvokeWithoutArgs([&]() -> void { + // Direct message route should be matched after internal redirect + EXPECT_CALL(*callbacks_.route_, routeEntry()).WillRepeatedly(Return(nullptr)); + EXPECT_CALL(*callbacks_.route_, directResponseEntry()).WillRepeatedly(Return(&direct_response)); + })); + EXPECT_CALL(callbacks_, recreateStream(_)).WillOnce(Return(true)); + + response_decoder_->decodeHeaders(std::move(redirect_headers_), false); + EXPECT_EQ(1U, cm_.thread_local_cluster_.cluster_.info_->stats_store_ + .counter("upstream_internal_redirect_succeeded_total") + .value()); + + // In production, the HCM recreateStream would have called this. + router_.onDestroy(); + EXPECT_EQ(1, callbacks_.streamInfo() + .filterState() + ->getDataMutable("num_internal_redirects") + .value()); +} + TEST_F(RouterTest, InternalRedirectStripsFragment) { enableRedirects(); default_request_headers_.setForwardedProto("http"); diff --git a/test/integration/redirect_integration_test.cc b/test/integration/redirect_integration_test.cc index a88b83a1917b..813defeaba10 100644 --- a/test/integration/redirect_integration_test.cc +++ b/test/integration/redirect_integration_test.cc @@ -39,6 +39,15 @@ class RedirectIntegrationTest : public HttpProtocolIntegrationTest { ->set_value(3); config_helper_.addVirtualHost(handle_max_3_hop); + auto handle_by_direct_response = config_helper_.createVirtualHost("handle.direct.response"); + handle_by_direct_response.mutable_routes(0)->set_name("direct_response"); + handle_by_direct_response.mutable_routes(0)->mutable_direct_response()->set_status(204); + handle_by_direct_response.mutable_routes(0) + ->mutable_direct_response() + ->mutable_body() + ->set_inline_string(EMPTY_STRING); + config_helper_.addVirtualHost(handle_by_direct_response); + HttpProtocolIntegrationTest::initialize(); } @@ -597,6 +606,39 @@ TEST_P(RedirectIntegrationTest, InvalidRedirect) { response->headers().get(test_header_key_)[0]->value().getStringView()); } +TEST_P(RedirectIntegrationTest, InternalRedirectHandledByDirectResponse) { + useAccessLog("%RESPONSE_FLAGS% %RESPONSE_CODE% %RESPONSE_CODE_DETAILS% %RESP(test-header)%"); + // Validate that header sanitization is only called once. + config_helper_.addConfigModifier( + [](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) { hcm.set_via("via_value"); }); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + default_request_headers_.setHost("handle.internal.redirect"); + IntegrationStreamDecoderPtr response = + codec_client_->makeHeaderOnlyRequest(default_request_headers_); + + waitForNextUpstreamRequest(); + + redirect_response_.setLocation("http://handle.direct.response/"); + upstream_request_->encodeHeaders(redirect_response_, true); + + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("204", response->headers().getStatusValue()); + EXPECT_EQ(1, test_server_->counter("cluster.cluster_0.upstream_internal_redirect_succeeded_total") + ->value()); + // 302 was never returned downstream + EXPECT_EQ(0, test_server_->counter("http.config_test.downstream_rq_3xx")->value()); + EXPECT_EQ(1, test_server_->counter("http.config_test.downstream_rq_2xx")->value()); + EXPECT_THAT(waitForAccessLog(access_log_name_, 0), + HasSubstr("302 internal_redirect test-header-value\n")); + // No test header + EXPECT_THAT(waitForAccessLog(access_log_name_, 1), HasSubstr("204 direct_response -\n")); +} + INSTANTIATE_TEST_SUITE_P(Protocols, RedirectIntegrationTest, testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), HttpProtocolIntegrationTest::protocolTestParamsToString); From c00e556936c3aa631beff433b256789b283b89b9 Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Fri, 28 Jan 2022 12:37:15 +0100 Subject: [PATCH 07/15] [1.20] CVE-2022-21654 tls allows re-use when some cert validation settings have changed Signed-off-by: Otto van der Schaaf --- .../tls/cert_validator/cert_validator.h | 5 +- .../tls/cert_validator/default_validator.cc | 31 ++++ .../transport_sockets/tls/ssl_socket_test.cc | 158 ++++++++++++++++++ 3 files changed, 193 insertions(+), 1 deletion(-) 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 efec7cb3aa4e..8174b5d367e6 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h +++ b/source/extensions/transport_sockets/tls/cert_validator/cert_validator.h @@ -62,7 +62,10 @@ class CertValidator { bool handshaker_provides_certificates) PURE; /** - * Called when calculation hash for session context ids + * Called when calculation hash for session context ids. This hash MUST include all + * configuration used to validate a peer certificate, so that if this configuration + * is changed, sessions cannot be re-used and must be re-negotiated and re-validated + * using the new settings. * * @param md the store context * @param hash_buffer the buffer used for digest calculation 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 1af0c588f45d..74215f6900fe 100644 --- a/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc +++ b/source/extensions/transport_sockets/tls/cert_validator/default_validator.cc @@ -396,6 +396,37 @@ void DefaultCertValidator::updateDigestForSessionId(bssl::ScopedEVP_MD_CTX& md, sizeof(std::remove_reference::type::value_type)); RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); } + + rc = EVP_DigestUpdate(md.get(), &verify_trusted_ca_, sizeof(verify_trusted_ca_)); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + + if (config_ != nullptr) { + for (const auto& matcher : config_->subjectAltNameMatchers()) { + size_t hash = MessageUtil::hash(matcher); + rc = EVP_DigestUpdate(md.get(), &hash, sizeof(hash)); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + } + + const std::string& crl = config_->certificateRevocationList(); + if (!crl.empty()) { + rc = EVP_DigestUpdate(md.get(), crl.data(), crl.length()); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + } + + bool allow_expired = config_->allowExpiredCertificate(); + rc = EVP_DigestUpdate(md.get(), &allow_expired, sizeof(allow_expired)); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + + auto trust_chain_verification = config_->trustChainVerification(); + rc = EVP_DigestUpdate(md.get(), &trust_chain_verification, sizeof(trust_chain_verification)); + RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + + // Back porting note: onlyVerifyLeafCertificateCrl isn't implemented. + // Hence we comment these lines here to disable them. + // auto only_leaf_crl = config_->onlyVerifyLeafCertificateCrl(); + // rc = EVP_DigestUpdate(md.get(), &only_leaf_crl, sizeof(only_leaf_crl)); + // RELEASE_ASSERT(rc == 1, Utility::getLastCryptoError().value_or("")); + } } void DefaultCertValidator::addClientValidationContext(SSL_CTX* ctx, bool require_client_cert) { diff --git a/test/extensions/transport_sockets/tls/ssl_socket_test.cc b/test/extensions/transport_sockets/tls/ssl_socket_test.cc index 52519f6df0fa..8ed445eccf44 100644 --- a/test/extensions/transport_sockets/tls/ssl_socket_test.cc +++ b/test/extensions/transport_sockets/tls/ssl_socket_test.cc @@ -3260,6 +3260,164 @@ TEST_P(SslSocketTest, TicketSessionResumptionDifferentServerNames) { client_ctx_yaml, false, GetParam()); } +// Sessions cannot be resumed even though the server certificates are the same, +// because of the different `verify_certificate_hash` settings. +TEST_P(SslSocketTest, TicketSessionResumptionDifferentVerifyCertHash) { + const std::string server_ctx_yaml1 = absl::StrCat(R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + verify_certificate_hash: + - ")EOF", + TEST_SAN_URI_CERT_256_HASH, "\""); + + const std::string server_ctx_yaml2 = absl::StrCat(R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + verify_certificate_hash: + - "0000000000000000000000000000000000000000000000000000000000000000" + - ")EOF", + TEST_SAN_URI_CERT_256_HASH, "\""); + + const std::string client_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_key.pem" +)EOF"; + + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml1, {}, client_ctx_yaml, true, + GetParam()); + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml2, {}, client_ctx_yaml, false, + GetParam()); +} + +// Sessions cannot be resumed even though the server certificates are the same, +// because of the different `verify_certificate_spki` settings. +TEST_P(SslSocketTest, TicketSessionResumptionDifferentVerifyCertSpki) { + const std::string server_ctx_yaml1 = absl::StrCat(R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + verify_certificate_spki: + - ")EOF", + TEST_SAN_URI_CERT_SPKI, "\""); + + const std::string server_ctx_yaml2 = absl::StrCat(R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + verify_certificate_spki: + - "NvqYIYSbgK2vCJpQhObf77vv+bQWtc5ek5RIOwPiC9A=" + - ")EOF", + TEST_SAN_URI_CERT_SPKI, "\""); + + const std::string client_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_key.pem" +)EOF"; + + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml1, {}, client_ctx_yaml, true, + GetParam()); + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml2, {}, client_ctx_yaml, false, + GetParam()); +} + +// Sessions cannot be resumed even though the server certificates are the same, +// because of the different `match_subject_alt_names` settings. +TEST_P(SslSocketTest, TicketSessionResumptionDifferentMatchSAN) { + const std::string server_ctx_yaml1 = R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + match_subject_alt_names: + - exact: "spiffe://lyft.com/test-team" +)EOF"; + + const std::string server_ctx_yaml2 = R"EOF( + session_ticket_keys: + keys: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ticket_key_a" + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/unittest_key.pem" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + match_subject_alt_names: + - prefix: "spiffe://lyft.com/test-team" +")EOF"; + + const std::string client_ctx_yaml = R"EOF( + common_tls_context: + tls_certificates: + certificate_chain: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_cert.pem" + private_key: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_uri_key.pem" +)EOF"; + + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml1, {}, client_ctx_yaml, true, + GetParam()); + testTicketSessionResumption(server_ctx_yaml1, {}, server_ctx_yaml2, {}, client_ctx_yaml, false, + GetParam()); +} + // Sessions can be resumed because the server certificates are different but the CN/SANs and // issuer are identical TEST_P(SslSocketTest, TicketSessionResumptionDifferentServerCert) { From ccc19042a36f73f505317d31f033786735c6e966 Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Tue, 1 Feb 2022 18:41:58 +0000 Subject: [PATCH 08/15] [1.20] CVE-2021-43824 jwt_atuhn: fixed the crash when a CONNECT request is sent to JWT filter configured with regex match. Signed-off-by: Yan Avlasov --- docs/root/version_history/current.rst | 1 + .../filters/http/jwt_authn/matcher.cc | 3 ++ .../http/jwt_authn/filter_integration_test.cc | 27 ++++++++++++++++ .../filters/http/jwt_authn/test_common.h | 31 +++++++++++++++++++ 4 files changed, 62 insertions(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 72c63856d6f3..caba4b0b8b36 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -14,6 +14,7 @@ Bug Fixes *Changes expected to improve the state of the world and are unlikely to have negative effects* * data plane: fix crash when internal redirect selects a route configured with direct response or redirect actions. +* jwt_authn: fixed the crash when a CONNECT request is sent to JWT filter configured with regex match on the Host header. Removed Config or Runtime ------------------------- diff --git a/source/extensions/filters/http/jwt_authn/matcher.cc b/source/extensions/filters/http/jwt_authn/matcher.cc index 3439396a0fbe..b329851eb654 100644 --- a/source/extensions/filters/http/jwt_authn/matcher.cc +++ b/source/extensions/filters/http/jwt_authn/matcher.cc @@ -117,6 +117,9 @@ class RegexMatcherImpl : public BaseMatcherImpl { bool matches(const Http::RequestHeaderMap& headers) const override { if (BaseMatcherImpl::matchRoute(headers)) { + if (headers.Path() == nullptr) { + return false; + } const Http::HeaderString& path = headers.Path()->value(); const absl::string_view query_string = Http::Utility::findQueryStringStart(path); absl::string_view path_view = path.getStringView(); diff --git a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc index 98fea932bc86..46f5bbe589cf 100644 --- a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc @@ -326,6 +326,33 @@ TEST_P(LocalJwksIntegrationTest, FilterStateRequirement) { } } +// Verify that JWT config with RegEx matcher can handle CONNECT requests. +TEST_P(LocalJwksIntegrationTest, ConnectRequestWithRegExMatch) { + config_helper_.prependFilter(getAuthFilterConfig(ExampleConfigWithRegEx, true)); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto encoder_decoder = codec_client_->startRequest(Http::TestRequestHeaderMapImpl{ + {":method", "CONNECT"}, + {":authority", "host.com:80"}, + {"authorization", "Bearer " + std::string(GoodToken)}, + }); + request_encoder_ = &encoder_decoder.first; + auto response = std::move(encoder_decoder.second); + + if (downstreamProtocol() == Http::CodecType::HTTP1) { + // Because CONNECT requests for HTTP/1 do not include a path, they will fail + // to find a route match and return a 404. + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + EXPECT_EQ("404", response->headers().getStatusValue()); + } else { + ASSERT_TRUE(response->waitForReset()); + ASSERT_TRUE(codec_client_->waitForDisconnect()); + } +} + // The test case with a fake upstream for remote Jwks server. class RemoteJwksIntegrationTest : public HttpProtocolIntegrationTest { public: diff --git a/test/extensions/filters/http/jwt_authn/test_common.h b/test/extensions/filters/http/jwt_authn/test_common.h index 114ee7e5ac65..44a57320dc43 100644 --- a/test/extensions/filters/http/jwt_authn/test_common.h +++ b/test/extensions/filters/http/jwt_authn/test_common.h @@ -85,6 +85,37 @@ const char ExampleConfig[] = R"( bypass_cors_preflight: true )"; +const char ExampleConfigWithRegEx[] = R"( +providers: + example_provider: + issuer: https://example.com + audiences: + - example_service + - http://example_service1 + - https://example_service2/ + remote_jwks: + http_uri: + uri: https://pubkey_server/pubkey_path + cluster: pubkey_cluster + timeout: + seconds: 5 + cache_duration: + seconds: 600 + forward_payload_header: sec-istio-auth-userinfo +rules: +- match: + safe_regex: + google_re2: {} + regex: "/somethig/.*" + requires: + provider_name: "example_provider" +- match: + path: "/" + requires: + provider_name: "example_provider" +bypass_cors_preflight: true +)"; + // The name of provider for above config. const char ProviderName[] = "example_provider"; From ab57bddf3570f34a03e1ba8870338210a5fc299d Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Tue, 1 Feb 2022 18:43:46 +0000 Subject: [PATCH 09/15] [1.20] CVE-2021-43826 Signed-off-by: Yan Avlasov --- docs/root/version_history/current.rst | 1 + source/common/tcp_proxy/tcp_proxy.cc | 9 ++- source/common/tcp_proxy/tcp_proxy.h | 1 + .../tcp_tunneling_integration_test.cc | 63 +++++++++++++++++++ 4 files changed, 73 insertions(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index caba4b0b8b36..f47916e22290 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -15,6 +15,7 @@ Bug Fixes * data plane: fix crash when internal redirect selects a route configured with direct response or redirect actions. * jwt_authn: fixed the crash when a CONNECT request is sent to JWT filter configured with regex match on the Host header. +* tcp_proxy: fix a crash that occurs when configured for :ref:`upstream tunneling ` and the downstream connection disconnects while the the upstream connection or http/2 stream is still being established. Removed Config or Runtime ------------------------- diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index b22fa5e956e5..7d2870e7857b 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -531,6 +531,11 @@ Network::FilterStatus Filter::onNewConnection() { } void Filter::onDownstreamEvent(Network::ConnectionEvent event) { + if (event == Network::ConnectionEvent::LocalClose || + event == Network::ConnectionEvent::RemoteClose) { + downstream_closed_ = true; + } + if (upstream_) { Tcp::ConnectionPool::ConnectionDataPtr conn_data(upstream_->onDownstreamEvent(event)); if (conn_data != nullptr && @@ -579,7 +584,9 @@ void Filter::onUpstreamEvent(Network::ConnectionEvent event) { Upstream::Outlier::Result::LocalOriginConnectFailed); } - initializeUpstreamConnection(); + if (!downstream_closed_) { + initializeUpstreamConnection(); + } } else { if (read_callbacks_->connection().state() == Network::Connection::State::Open) { read_callbacks_->connection().close(Network::ConnectionCloseType::FlushWrite); diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h index 3b64e58f5add..07cfe49adac2 100644 --- a/source/common/tcp_proxy/tcp_proxy.h +++ b/source/common/tcp_proxy/tcp_proxy.h @@ -377,6 +377,7 @@ class Filter : public Network::ReadFilter, Network::Socket::OptionsSharedPtr upstream_options_; uint32_t connect_attempts_{}; bool connecting_{}; + bool downstream_closed_{}; }; // This class deals with an upstream connection that needs to finish flushing, when the downstream diff --git a/test/integration/tcp_tunneling_integration_test.cc b/test/integration/tcp_tunneling_integration_test.cc index ea3d79afd589..e89786b3dcd5 100644 --- a/test/integration/tcp_tunneling_integration_test.cc +++ b/test/integration/tcp_tunneling_integration_test.cc @@ -661,6 +661,69 @@ TEST_P(TcpTunnelingIntegrationTest, ResetStreamTest) { tcp_client->waitForDisconnect(); } +TEST_P(TcpTunnelingIntegrationTest, UpstreamConnectingDownstreamDisconnect) { + if (upstreamProtocol() == Http::CodecType::HTTP1) { + return; + } + +#if defined(WIN32) + // TODO(ggreenway): figure out why this test fails on Windows and remove this disable. + // Failing tests: + // IpAndHttpVersions/TcpTunnelingIntegrationTest.UpstreamConnectingDownstreamDisconnect/IPv4_HttpDownstream_Http3UpstreamBareHttp2, + // IpAndHttpVersions/TcpTunnelingIntegrationTest.UpstreamConnectingDownstreamDisconnect/IPv6_HttpDownstream_Http2UpstreamWrappedHttp2, + // Times out at the end of the test on `ASSERT_TRUE(upstream_request_->waitForReset());`. + return; +#endif + + config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void { + envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy proxy_config; + proxy_config.set_stat_prefix("tcp_stats"); + proxy_config.set_cluster("cluster_0"); + proxy_config.mutable_tunneling_config()->set_hostname("host.com:80"); + + // Enable retries. The crash is due to retrying after the downstream connection is closed, which + // can't occur if retries are not enabled. + proxy_config.mutable_max_connect_attempts()->set_value(2); + + auto* listeners = bootstrap.mutable_static_resources()->mutable_listeners(); + for (auto& listener : *listeners) { + if (listener.name() != "tcp_proxy") { + continue; + } + auto* filter_chain = listener.mutable_filter_chains(0); + auto* filter = filter_chain->mutable_filters(0); + filter->mutable_typed_config()->PackFrom(proxy_config); + + // Use TLS because it will respond to a TCP half-close during handshake by closing the + // connection. + envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context; + ConfigHelper::initializeTls({}, *tls_context.mutable_common_tls_context()); + filter_chain->mutable_transport_socket()->set_name("envoy.transport_sockets.tls"); + filter_chain->mutable_transport_socket()->mutable_typed_config()->PackFrom(tls_context); + + break; + } + }); + + enableHalfClose(false); + initialize(); + + IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("tcp_proxy")); + + // Wait for the request for a connection, but don't send a response back yet. This ensures that + // tcp_proxy is stuck in `connecting_`. + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + + // Close the client connection. The TLS transport socket will detect this even while + // `readDisable(true)` on the connection, and will raise a `RemoteClose` event. + tcp_client->close(); + + ASSERT_TRUE(upstream_request_->waitForReset()); + ASSERT_TRUE(fake_upstream_connection_->close()); +} + TEST_P(TcpTunnelingIntegrationTest, TestIdletimeoutWithLargeOutstandingData) { enableHalfClose(false); config_helper_.setBufferLimits(1024, 1024); From 4aaf9593152c6996b9da384c8918e9ad4f0abd4d Mon Sep 17 00:00:00 2001 From: Yan Avlasov Date: Tue, 1 Feb 2022 18:45:42 +0000 Subject: [PATCH 10/15] [1.20] CVE-2022-23606 Signed-off-by: Yan Avlasov --- docs/root/version_history/current.rst | 1 + source/common/conn_pool/BUILD | 1 + source/common/conn_pool/conn_pool_base.cc | 27 +++- source/common/conn_pool/conn_pool_base.h | 5 + test/config/utility.cc | 28 ++++ test/config/utility.h | 5 + test/integration/BUILD | 2 + test/integration/cds_integration_test.cc | 174 +++++++++++++++++++++- 8 files changed, 230 insertions(+), 13 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f47916e22290..8abda6992b9f 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -16,6 +16,7 @@ Bug Fixes * data plane: fix crash when internal redirect selects a route configured with direct response or redirect actions. * jwt_authn: fixed the crash when a CONNECT request is sent to JWT filter configured with regex match on the Host header. * tcp_proxy: fix a crash that occurs when configured for :ref:`upstream tunneling ` and the downstream connection disconnects while the the upstream connection or http/2 stream is still being established. +* upstream: fix stack overflow when a cluster with large number of idle connections is removed. Removed Config or Runtime ------------------------- diff --git a/source/common/conn_pool/BUILD b/source/common/conn_pool/BUILD index d526dfbf956b..048aa131fbb4 100644 --- a/source/common/conn_pool/BUILD +++ b/source/common/conn_pool/BUILD @@ -14,6 +14,7 @@ envoy_cc_library( hdrs = ["conn_pool_base.h"], deps = [ "//envoy/stats:timespan_interface", + "//source/common/common:debug_recursion_checker_lib", "//source/common/common:linked_object", "//source/common/stats:timespan_lib", "//source/common/upstream:upstream_lib", diff --git a/source/common/conn_pool/conn_pool_base.cc b/source/common/conn_pool/conn_pool_base.cc index a22f8d618c74..8e57ff8aaed0 100644 --- a/source/common/conn_pool/conn_pool_base.cc +++ b/source/common/conn_pool/conn_pool_base.cc @@ -1,6 +1,7 @@ #include "source/common/conn_pool/conn_pool_base.h" #include "source/common/common/assert.h" +#include "source/common/common/debug_recursion_checker.h" #include "source/common/network/transport_socket_options_impl.h" #include "source/common/runtime/runtime_features.h" #include "source/common/stats/timespan_impl.h" @@ -333,6 +334,8 @@ void ConnPoolImplBase::transitionActiveClientState(ActiveClient& client, void ConnPoolImplBase::addIdleCallbackImpl(Instance::IdleCb cb) { idle_callbacks_.push_back(cb); } void ConnPoolImplBase::closeIdleConnectionsForDrainingPool() { + Common::AutoDebugRecursionChecker assert_not_in(recursion_checker_); + // Create a separate list of elements to close to avoid mutate-while-iterating problems. std::list to_close; @@ -387,11 +390,7 @@ bool ConnPoolImplBase::isIdleImpl() const { connecting_clients_.empty(); } -void ConnPoolImplBase::checkForIdleAndCloseIdleConnsIfDraining() { - if (is_draining_for_deletion_) { - closeIdleConnectionsForDrainingPool(); - } - +void ConnPoolImplBase::checkForIdleAndNotify() { if (isIdleImpl()) { ENVOY_LOG(debug, "invoking idle callbacks - is_draining_for_deletion_={}", is_draining_for_deletion_); @@ -401,6 +400,14 @@ void ConnPoolImplBase::checkForIdleAndCloseIdleConnsIfDraining() { } } +void ConnPoolImplBase::checkForIdleAndCloseIdleConnsIfDraining() { + if (is_draining_for_deletion_) { + closeIdleConnectionsForDrainingPool(); + } + + checkForIdleAndNotify(); +} + void ConnPoolImplBase::onConnectionEvent(ActiveClient& client, absl::string_view failure_reason, Network::ConnectionEvent event) { if (client.state() == ActiveClient::State::CONNECTING) { @@ -459,7 +466,15 @@ void ConnPoolImplBase::onConnectionEvent(ActiveClient& client, absl::string_view dispatcher_.deferredDelete(client.removeFromList(owningList(client.state()))); - checkForIdleAndCloseIdleConnsIfDraining(); + // Check if the pool transitioned to idle state after removing closed client + // from one of the client tracking lists. + // There is no need to check if other connections are idle in a draining pool + // because the pool will close all idle connection when it is starting to + // drain. + // Trying to close other connections here can lead to deep recursion when + // a large number idle connections are closed at the start of pool drain. + // See CdsIntegrationTest.CdsClusterDownWithLotsOfIdleConnections for an example. + checkForIdleAndNotify(); client.setState(ActiveClient::State::CLOSED); diff --git a/source/common/conn_pool/conn_pool_base.h b/source/common/conn_pool/conn_pool_base.h index a6a4802a59f4..61ee7e999739 100644 --- a/source/common/conn_pool/conn_pool_base.h +++ b/source/common/conn_pool/conn_pool_base.h @@ -6,6 +6,7 @@ #include "envoy/stats/timespan.h" #include "envoy/upstream/cluster_manager.h" +#include "source/common/common/debug_recursion_checker.h" #include "source/common/common/dump_state_utils.h" #include "source/common/common/linked_object.h" @@ -201,6 +202,9 @@ class ConnPoolImplBase : protected Logger::Loggable { void onConnectionEvent(ActiveClient& client, absl::string_view failure_reason, Network::ConnectionEvent event); + // Check if the pool has gone idle and invoke idle notification callbacks. + void checkForIdleAndNotify(); + // See if the pool has gone idle. If we're draining, this will also close idle connections. void checkForIdleAndCloseIdleConnsIfDraining(); @@ -345,6 +349,7 @@ class ConnPoolImplBase : protected Logger::Loggable { void onUpstreamReady(); Event::SchedulableCallbackPtr upstream_ready_cb_; + Common::DebugRecursionChecker recursion_checker_; }; } // namespace ConnectionPool diff --git a/test/config/utility.cc b/test/config/utility.cc index e3c360f7168c..15c519013acb 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -432,6 +432,34 @@ envoy::config::cluster::v3::Cluster ConfigHelper::buildStaticCluster(const std:: name, name, address, port, lb_policy)); } +envoy::config::cluster::v3::Cluster ConfigHelper::buildH1ClusterWithHighCircuitBreakersLimits( + const std::string& name, int port, const std::string& address, const std::string& lb_policy) { + return TestUtility::parseYaml( + fmt::format(R"EOF( + name: {} + connect_timeout: 50s + type: STATIC + circuit_breakers: + thresholds: + - priority: DEFAULT + max_connections: 10000 + max_pending_requests: 10000 + max_requests: 10000 + max_retries: 10000 + load_assignment: + cluster_name: {} + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: {} + port_value: {} + lb_policy: {} + )EOF", + name, name, address, port, lb_policy)); +} + envoy::config::cluster::v3::Cluster ConfigHelper::buildCluster(const std::string& name, const std::string& lb_policy) { API_NO_BOOST(envoy::config::cluster::v3::Cluster) cluster; diff --git a/test/config/utility.h b/test/config/utility.h index 6c310ec3cf36..107f87b4f7c0 100644 --- a/test/config/utility.h +++ b/test/config/utility.h @@ -153,6 +153,11 @@ class ConfigHelper { buildStaticCluster(const std::string& name, int port, const std::string& address, const std::string& lb_policy = "ROUND_ROBIN"); + static envoy::config::cluster::v3::Cluster + buildH1ClusterWithHighCircuitBreakersLimits(const std::string& name, int port, + const std::string& address, + const std::string& lb_policy = "ROUND_ROBIN"); + // ADS configurations static envoy::config::cluster::v3::Cluster buildCluster(const std::string& name, const std::string& lb_policy = "ROUND_ROBIN"); diff --git a/test/integration/BUILD b/test/integration/BUILD index 38b514c5496a..8f0d1cf58c8e 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -111,10 +111,12 @@ envoy_proto_library( envoy_cc_test( name = "cds_integration_test", + size = "large", srcs = ["cds_integration_test.cc"], data = [ "//test/config/integration/certs", ], + shard_count = 4, deps = [ ":http_integration_lib", "//source/common/config:protobuf_link_hacks", diff --git a/test/integration/cds_integration_test.cc b/test/integration/cds_integration_test.cc index bbffa27e9569..ef1bdb976752 100644 --- a/test/integration/cds_integration_test.cc +++ b/test/integration/cds_integration_test.cc @@ -34,7 +34,8 @@ class CdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public Ht CdsIntegrationTest() : HttpIntegrationTest(Http::CodecType::HTTP2, ipVersion(), ConfigHelper::discoveredClustersBootstrap( - sotwOrDelta() == Grpc::SotwOrDelta::Sotw ? "GRPC" : "DELTA_GRPC")) { + sotwOrDelta() == Grpc::SotwOrDelta::Sotw ? "GRPC" : "DELTA_GRPC")), + cluster_creator_(&ConfigHelper::buildStaticCluster) { use_lds_ = false; sotw_or_delta_ = sotwOrDelta(); } @@ -72,14 +73,14 @@ class CdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public Ht // Create the regular (i.e. not an xDS server) upstreams. We create them manually here after // initialize() because finalize() expects all fake_upstreams_ to correspond to a static // cluster in the bootstrap config - which we don't want since we're testing dynamic CDS! - addFakeUpstream(Http::CodecType::HTTP2); - addFakeUpstream(Http::CodecType::HTTP2); - cluster1_ = ConfigHelper::buildStaticCluster( + addFakeUpstream(upstream_codec_type_); + addFakeUpstream(upstream_codec_type_); + cluster1_ = cluster_creator_( ClusterName1, fake_upstreams_[UpstreamIndex1]->localAddress()->ip()->port(), - Network::Test::getLoopbackAddressString(ipVersion())); - cluster2_ = ConfigHelper::buildStaticCluster( + Network::Test::getLoopbackAddressString(ipVersion()), "ROUND_ROBIN"); + cluster2_ = cluster_creator_( ClusterName2, fake_upstreams_[UpstreamIndex2]->localAddress()->ip()->port(), - Network::Test::getLoopbackAddressString(ipVersion())); + Network::Test::getLoopbackAddressString(ipVersion()), "ROUND_ROBIN"); // Let Envoy establish its connection to the CDS server. acceptXdsConnection(); @@ -126,6 +127,10 @@ class CdsIntegrationTest : public Grpc::DeltaSotwIntegrationParamTest, public Ht envoy::config::cluster::v3::Cluster cluster2_; // True if we decided not to run the test after all. bool test_skipped_{true}; + Http::CodecType upstream_codec_type_{Http::CodecType::HTTP2}; + std::function + cluster_creator_; }; INSTANTIATE_TEST_SUITE_P(IpVersionsClientTypeDelta, CdsIntegrationTest, @@ -301,5 +306,160 @@ TEST_P(CdsIntegrationTest, VersionsRememberedAfterReconnect) { ASSERT_TRUE(codec_client_->waitForDisconnect()); } +// This test verifies that Envoy can delete a cluster with a lot of idle connections. +// The original problem was recursive closure of idle connections that can run out +// of stack when there are a lot of idle connections. +TEST_P(CdsIntegrationTest, CdsClusterDownWithLotsOfIdleConnections) { + constexpr int num_requests = 2000; + // Make upstream H/1 so it creates connection for each request + upstream_codec_type_ = Http::CodecType::HTTP1; + // Relax default circuit breaker limits and timeouts so Envoy can accumulate a lot of idle + // connections + cluster_creator_ = &ConfigHelper::buildH1ClusterWithHighCircuitBreakersLimits; + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.mutable_route_config() + ->mutable_virtual_hosts(0) + ->mutable_routes(0) + ->mutable_route() + ->mutable_timeout() + ->set_seconds(600); + hcm.mutable_route_config() + ->mutable_virtual_hosts(0) + ->mutable_routes(0) + ->mutable_route() + ->mutable_idle_timeout() + ->set_seconds(600); + }); + initialize(); + std::vector responses; + std::vector upstream_connections; + std::vector upstream_requests; + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + // The first loop establishes a lot of open connections with active requests to upstream + for (int i = 0; i < num_requests; ++i) { + Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, + {":path", "/cluster1"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-lyft-user-id", absl::StrCat(i)}}; + + auto response = codec_client_->makeHeaderOnlyRequest(request_headers); + responses.push_back(std::move(response)); + + FakeHttpConnectionPtr fake_upstream_connection; + waitForNextUpstreamConnection({UpstreamIndex1}, TestUtility::DefaultTimeout, + fake_upstream_connection); + // Wait for the next stream on the upstream connection. + FakeStreamPtr upstream_request; + AssertionResult result = + fake_upstream_connection->waitForNewStream(*dispatcher_, upstream_request); + RELEASE_ASSERT(result, result.message()); + // Wait for the stream to be completely received. + result = upstream_request->waitForEndStream(*dispatcher_); + RELEASE_ASSERT(result, result.message()); + upstream_connections.push_back(std::move(fake_upstream_connection)); + upstream_requests.push_back(std::move(upstream_request)); + } + + // This loop completes all requests making the all upstream connections idle + for (int i = 0; i < num_requests; ++i) { + // Send response headers, and end_stream if there is no response body. + upstream_requests[i]->encodeHeaders(default_response_headers_, true); + // Wait for the response to be read by the codec client. + RELEASE_ASSERT(responses[i]->waitForEndStream(), "unexpected timeout"); + ASSERT_TRUE(responses[i]->complete()); + EXPECT_EQ("200", responses[i]->headers().getStatusValue()); + } + + test_server_->waitForCounterGe("cluster_manager.cluster_added", 1); + + // Tell Envoy that cluster_1 is gone. Envoy will try to close all idle connections + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "55", {}, {}, {})); + sendDiscoveryResponse(Config::TypeUrl::get().Cluster, {}, {}, + {ClusterName1}, "42"); + // We can continue the test once we're sure that Envoy's ClusterManager has made use of + // the DiscoveryResponse that says cluster_1 is gone. + test_server_->waitForCounterGe("cluster_manager.cluster_removed", 1); + + // If we made it this far then everything is ok. + for (int i = 0; i < num_requests; ++i) { + AssertionResult result = upstream_connections[i]->close(); + RELEASE_ASSERT(result, result.message()); + result = upstream_connections[i]->waitForDisconnect(); + RELEASE_ASSERT(result, result.message()); + } + upstream_connections.clear(); + cleanupUpstreamAndDownstream(); + ASSERT_TRUE(codec_client_->waitForDisconnect()); +} + +// This test verifies that Envoy can delete a cluster with a lot of connections in the connecting +// state and associated pending requests. The recursion guard in the +// ConnPoolImplBase::closeIdleConnectionsForDrainingPool() would fire if it was called recursively. +// +// Test is currently disabled as there is presently no reliable way of making upstream connections +// hang in connecting state. +TEST_P(CdsIntegrationTest, DISABLED_CdsClusterDownWithLotsOfConnectingConnections) { + // Use low number of pending connections to prevent bumping into the default + // limit of 128, since the upstream will be prevented below from + // accepting connections. + constexpr int num_requests = 64; + // Make upstream H/1 so it creates connection for each request + upstream_codec_type_ = Http::CodecType::HTTP1; + cluster_creator_ = &ConfigHelper::buildH1ClusterWithHighCircuitBreakersLimits; + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { + hcm.mutable_route_config() + ->mutable_virtual_hosts(0) + ->mutable_routes(0) + ->mutable_route() + ->mutable_timeout() + ->set_seconds(600); + hcm.mutable_route_config() + ->mutable_virtual_hosts(0) + ->mutable_routes(0) + ->mutable_route() + ->mutable_idle_timeout() + ->set_seconds(600); + }); + initialize(); + test_server_->waitForCounterGe("cluster_manager.cluster_added", 1); + std::vector responses; + codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); + // Stop upstream at UpstreamIndex1 dispatcher, to prevent it from accepting TCP connections. + // This will cause Envoy's connections to that upstream hang in the connecting state. + fake_upstreams_[UpstreamIndex1]->dispatcher()->exit(); + for (int i = 0; i < num_requests; ++i) { + Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, + {":path", "/cluster1"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-lyft-user-id", absl::StrCat(i)}}; + + auto response = codec_client_->makeHeaderOnlyRequest(request_headers); + responses.push_back(std::move(response)); + } + + // Wait for Envoy to try to establish all expected connections + test_server_->waitForCounterEq("cluster.cluster_1.upstream_cx_total", num_requests); + + // Tell Envoy that cluster_1 is gone. Envoy will try to close all pending connections + EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "55", {}, {}, {})); + sendDiscoveryResponse(Config::TypeUrl::get().Cluster, {}, {}, + {ClusterName1}, "42"); + // We can continue the test once we're sure that Envoy's ClusterManager has made use of + // the DiscoveryResponse that says cluster_1 is gone. + test_server_->waitForCounterGe("cluster_manager.cluster_removed", 1); + + cleanupUpstreamAndDownstream(); + ASSERT_TRUE(codec_client_->waitForDisconnect()); + // If we got here it means that the recursion guard in the + // ConnPoolImplBase::closeIdleConnectionsForDrainingPool() did not fire, which is what this test + // validates. +} + } // namespace } // namespace Envoy From 8d61f0641faae7fd54947c7e4edd691f178200e7 Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Fri, 4 Mar 2022 09:12:20 -0800 Subject: [PATCH 11/15] Clean up version number and history following the v1.20.2 release (#20082) Signed-off-by: Ryan Hamilton --- VERSION | 2 +- docs/root/version_history/current.rst | 12 +------ docs/root/version_history/v1.20.2.rst | 34 +++++++++++++++++++ docs/root/version_history/version_history.rst | 1 + 4 files changed, 37 insertions(+), 12 deletions(-) create mode 100644 docs/root/version_history/v1.20.2.rst diff --git a/VERSION b/VERSION index 558dbb08f9e3..03d6f0b2bc4f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.20.2-dev +1.20.3-dev diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 8abda6992b9f..f876f6cc1493 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -1,4 +1,4 @@ -1.20.2 (Pending) +1.20.3 (Pending) ======================== Incompatible Behavior Changes @@ -13,11 +13,6 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* -* data plane: fix crash when internal redirect selects a route configured with direct response or redirect actions. -* jwt_authn: fixed the crash when a CONNECT request is sent to JWT filter configured with regex match on the Host header. -* tcp_proxy: fix a crash that occurs when configured for :ref:`upstream tunneling ` and the downstream connection disconnects while the the upstream connection or http/2 stream is still being established. -* upstream: fix stack overflow when a cluster with large number of idle connections is removed. - Removed Config or Runtime ------------------------- *Normally occurs at the end of the* :ref:`deprecation period ` @@ -25,10 +20,5 @@ Removed Config or Runtime New Features ------------ -+* tls: added support for :ref:`match_typed_subject_alt_names ` for subject alternative names to enforce specifying the subject alternative name type for the matcher to prevent matching against an unintended type in the certificate. - - Deprecated ---------- - -+* tls: :ref:`match_subject_alt_names ` has been deprecated in favor of the :ref:`match_typed_subject_alt_names `. diff --git a/docs/root/version_history/v1.20.2.rst b/docs/root/version_history/v1.20.2.rst new file mode 100644 index 000000000000..ad8dc61a21d4 --- /dev/null +++ b/docs/root/version_history/v1.20.2.rst @@ -0,0 +1,34 @@ +1.20.2 (February 22, 2022) +========================== + +Incompatible Behavior Changes +----------------------------- +*Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* + +Minor Behavior Changes +---------------------- +*Changes that may cause incompatibilities for some users, but should not for most* + +Bug Fixes +--------- +*Changes expected to improve the state of the world and are unlikely to have negative effects* + +* data plane: fix crash when internal redirect selects a route configured with direct response or redirect actions. +* jwt_authn: fixed the crash when a CONNECT request is sent to JWT filter configured with regex match on the Host header. +* tcp_proxy: fix a crash that occurs when configured for :ref:`upstream tunneling ` and the downstream connection disconnects while the the upstream connection or http/2 stream is still being established. +* upstream: fix stack overflow when a cluster with large number of idle connections is removed. + +Removed Config or Runtime +------------------------- +*Normally occurs at the end of the* :ref:`deprecation period ` + +New Features +------------ + ++* tls: added support for :ref:`match_typed_subject_alt_names ` for subject alternative names to enforce specifying the subject alternative name type for the matcher to prevent matching against an unintended type in the certificate. + + +Deprecated +---------- + ++* tls: :ref:`match_subject_alt_names ` has been deprecated in favor of the :ref:`match_typed_subject_alt_names `. diff --git a/docs/root/version_history/version_history.rst b/docs/root/version_history/version_history.rst index 006173c8ade0..a485935830cc 100644 --- a/docs/root/version_history/version_history.rst +++ b/docs/root/version_history/version_history.rst @@ -7,6 +7,7 @@ Version history :titlesonly: current + v1.20.2 v1.20.1 v1.20.0 v1.19.1 From bef835c8f4a1f427f65f54bf96ad86387a65da1a Mon Sep 17 00:00:00 2001 From: Ryan Hamilton Date: Tue, 8 Mar 2022 12:18:05 -0800 Subject: [PATCH 12/15] Fix CI for release/v1.20 (#20221) Backport: examples: Fix flask import (#20038) Signed-off-by: Ryan Northey ryan@synca.io Signed-off-by: phlax phlax@users.noreply.github.com Backport: build(deps): bump github/codeql-action from 1.1.2 to 1.1.3 (#20107) Bumps github/codeql-action from 1.1.2 to 1.1.3. updated-dependencies: dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch Signed-off-by: dependabot[bot] support@github.com Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Ryan Hamilton rch@google.com Backport: ci: not using alpine image in examples (#19805) Signed-off-by: Lizan Zhou lizan@tetrate.io Signed-off-by: Ryan Hamilton rch@google.com --- .github/workflows/codeql-daily.yml | 4 ++-- .github/workflows/codeql-push.yml | 4 ++-- examples/brotli/Dockerfile-service | 4 ++-- examples/cache/Dockerfile-service | 2 +- examples/cors/backend/Dockerfile-service | 2 +- examples/cors/frontend/Dockerfile-service | 2 +- examples/csrf/crosssite/Dockerfile-service | 2 +- examples/csrf/samesite/Dockerfile-service | 2 +- examples/double-proxy/Dockerfile-app | 2 +- examples/ext_authz/upstream/service/Dockerfile | 2 +- examples/front-proxy/Dockerfile-jaeger-service | 6 +++--- examples/front-proxy/Dockerfile-service | 6 +++--- examples/gzip/Dockerfile-service | 4 ++-- examples/load-reporting-service/Dockerfile-http-server | 2 +- examples/win32-front-proxy/Dockerfile-service | 2 +- 15 files changed, 23 insertions(+), 23 deletions(-) diff --git a/.github/workflows/codeql-daily.yml b/.github/workflows/codeql-daily.yml index 358695c84f61..1badcf748209 100644 --- a/.github/workflows/codeql-daily.yml +++ b/.github/workflows/codeql-daily.yml @@ -26,7 +26,7 @@ jobs: # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL - uses: github/codeql-action/init@a66b44a48a44d63acf71688c56aa168ac171d4cf # v1 + uses: github/codeql-action/init@75f07e7ab2ee63cba88752d8c696324e4df67466 # Override language selection by uncommenting this and choosing your languages with: languages: cpp @@ -53,4 +53,4 @@ jobs: git clean -xdf - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@a66b44a48a44d63acf71688c56aa168ac171d4cf # v1 + uses: github/codeql-action/analyze@75f07e7ab2ee63cba88752d8c696324e4df67466 diff --git a/.github/workflows/codeql-push.yml b/.github/workflows/codeql-push.yml index d79e7a011d34..f3936374311b 100644 --- a/.github/workflows/codeql-push.yml +++ b/.github/workflows/codeql-push.yml @@ -34,7 +34,7 @@ jobs: # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL - uses: github/codeql-action/init@a66b44a48a44d63acf71688c56aa168ac171d4cf # v1 + uses: github/codeql-action/init@75f07e7ab2ee63cba88752d8c696324e4df67466 # Override language selection by uncommenting this and choosing your languages with: languages: cpp @@ -64,4 +64,4 @@ jobs: - name: Perform CodeQL Analysis if: env.BUILD_TARGETS != '' - uses: github/codeql-action/analyze@a66b44a48a44d63acf71688c56aa168ac171d4cf # v1 + uses: github/codeql-action/analyze@75f07e7ab2ee63cba88752d8c696324e4df67466 diff --git a/examples/brotli/Dockerfile-service b/examples/brotli/Dockerfile-service index 4712019f501b..c9103f91a32a 100644 --- a/examples/brotli/Dockerfile-service +++ b/examples/brotli/Dockerfile-service @@ -1,11 +1,11 @@ FROM debian:buster-slim RUN apt-get update \ - && apt-get install --no-install-recommends -y python3 python3-pip \ + && apt-get install --no-install-recommends -y python3 python3-pip python3-setuptools \ && apt-get autoremove -y && apt-get clean \ && rm -rf /tmp/* /var/tmp/* \ && rm -rf /var/lib/apt/lists/* -RUN pip3 install -q flask +RUN pip3 install -q Flask==2.0.3 RUN mkdir -p /code/data RUN dd if=/dev/zero of="/code/data/file.txt" bs=1024 count=10240 \ && dd if=/dev/zero of="/code/data/file.json" bs=1024 count=10240 diff --git a/examples/cache/Dockerfile-service b/examples/cache/Dockerfile-service index a05db06f5e24..6c9a7f939fe8 100644 --- a/examples/cache/Dockerfile-service +++ b/examples/cache/Dockerfile-service @@ -1,7 +1,7 @@ FROM alpine:latest RUN apk update && apk add py3-pip -RUN pip3 install -q Flask==0.11.1 pyyaml +RUN pip3 install -q Flask==2.0.3 pyyaml RUN mkdir /code COPY ./service.py /code CMD ["python3", "/code/service.py"] diff --git a/examples/cors/backend/Dockerfile-service b/examples/cors/backend/Dockerfile-service index 4b98a80704dc..e5fb609c8539 100644 --- a/examples/cors/backend/Dockerfile-service +++ b/examples/cors/backend/Dockerfile-service @@ -1,7 +1,7 @@ FROM alpine:latest RUN apk update && apk add py3-pip -RUN pip3 install -q Flask==0.11.1 +RUN pip3 install -q Flask==2.0.3 RUN mkdir /code ADD ./service.py /code/ diff --git a/examples/cors/frontend/Dockerfile-service b/examples/cors/frontend/Dockerfile-service index 85dba2382f99..704184b2ac3a 100644 --- a/examples/cors/frontend/Dockerfile-service +++ b/examples/cors/frontend/Dockerfile-service @@ -1,7 +1,7 @@ FROM alpine:latest RUN apk update && apk add py3-pip -RUN pip3 install -q Flask==0.11.1 +RUN pip3 install -q Flask==2.0.3 RUN mkdir /code ADD ./service.py ./index.html /code/ diff --git a/examples/csrf/crosssite/Dockerfile-service b/examples/csrf/crosssite/Dockerfile-service index 873f00e04246..60a57011cf80 100644 --- a/examples/csrf/crosssite/Dockerfile-service +++ b/examples/csrf/crosssite/Dockerfile-service @@ -1,7 +1,7 @@ FROM alpine:latest RUN apk update && apk add py3-pip -RUN pip3 install -q Flask==0.11.1 +RUN pip3 install -q Flask==2.0.3 RUN mkdir /code ADD ./crosssite/service.py ./index.html /code/ CMD ["python3", "/code/service.py"] diff --git a/examples/csrf/samesite/Dockerfile-service b/examples/csrf/samesite/Dockerfile-service index 17fd1f51a6dc..bc035fa1e564 100644 --- a/examples/csrf/samesite/Dockerfile-service +++ b/examples/csrf/samesite/Dockerfile-service @@ -1,7 +1,7 @@ FROM alpine:latest RUN apk update && apk add py3-pip -RUN pip3 install -q Flask==0.11.1 +RUN pip3 install -q Flask==2.0.3 RUN mkdir /code ADD ./samesite/service.py ./index.html /code/ CMD ["python3", "/code/service.py"] diff --git a/examples/double-proxy/Dockerfile-app b/examples/double-proxy/Dockerfile-app index f843db1f8a53..0e6f664dc63e 100644 --- a/examples/double-proxy/Dockerfile-app +++ b/examples/double-proxy/Dockerfile-app @@ -1,7 +1,7 @@ FROM python:3.8-alpine RUN apk update && apk add postgresql-dev gcc python3-dev musl-dev -RUN pip3 install -q Flask==0.11.1 requests==2.18.4 psycopg2-binary +RUN pip3 install -q Flask==2.0.3 requests==2.18.4 psycopg2-binary RUN mkdir /code ADD ./service.py /code ENTRYPOINT ["python3", "/code/service.py"] diff --git a/examples/ext_authz/upstream/service/Dockerfile b/examples/ext_authz/upstream/service/Dockerfile index 5f70f40aca7c..9cd53e82641d 100644 --- a/examples/ext_authz/upstream/service/Dockerfile +++ b/examples/ext_authz/upstream/service/Dockerfile @@ -1,5 +1,5 @@ FROM python:3-alpine -RUN pip3 install -q Flask==0.11.1 +RUN pip3 install -q Flask==2.0.3 COPY . ./app CMD ["python3", "/app/service/server.py"] diff --git a/examples/front-proxy/Dockerfile-jaeger-service b/examples/front-proxy/Dockerfile-jaeger-service index f8bbf13b03e0..4a38c6efff76 100644 --- a/examples/front-proxy/Dockerfile-jaeger-service +++ b/examples/front-proxy/Dockerfile-jaeger-service @@ -1,7 +1,7 @@ -FROM envoyproxy/envoy-alpine-dev:latest +FROM envoyproxy/envoy-dev:latest -RUN apk update && apk add py3-pip bash curl -RUN pip3 install -q Flask==0.11.1 requests==2.18.4 +RUN apt-get update && apt-get -q install --no-install-recommends -y python3-pip curl +RUN pip3 install -q Flask==2.0.3 requests==2.18.4 RUN mkdir /code ADD ./service.py /code ADD ./start_service.sh /usr/local/bin/start_service.sh diff --git a/examples/front-proxy/Dockerfile-service b/examples/front-proxy/Dockerfile-service index 0c2ae43024fa..86fdf2b667b4 100644 --- a/examples/front-proxy/Dockerfile-service +++ b/examples/front-proxy/Dockerfile-service @@ -1,7 +1,7 @@ -FROM envoyproxy/envoy-alpine-dev:latest +FROM envoyproxy/envoy-dev:latest -RUN apk update && apk add py3-pip bash curl -RUN pip3 install -q Flask==0.11.1 requests==2.18.4 +RUN apt-get update && apt-get -q install --no-install-recommends -y python3-pip curl +RUN pip3 install -q Flask==2.0.3 requests==2.18.4 RUN mkdir /code ADD ./service.py /code ADD ./start_service.sh /usr/local/bin/start_service.sh diff --git a/examples/gzip/Dockerfile-service b/examples/gzip/Dockerfile-service index 4712019f501b..c9103f91a32a 100644 --- a/examples/gzip/Dockerfile-service +++ b/examples/gzip/Dockerfile-service @@ -1,11 +1,11 @@ FROM debian:buster-slim RUN apt-get update \ - && apt-get install --no-install-recommends -y python3 python3-pip \ + && apt-get install --no-install-recommends -y python3 python3-pip python3-setuptools \ && apt-get autoremove -y && apt-get clean \ && rm -rf /tmp/* /var/tmp/* \ && rm -rf /var/lib/apt/lists/* -RUN pip3 install -q flask +RUN pip3 install -q Flask==2.0.3 RUN mkdir -p /code/data RUN dd if=/dev/zero of="/code/data/file.txt" bs=1024 count=10240 \ && dd if=/dev/zero of="/code/data/file.json" bs=1024 count=10240 diff --git a/examples/load-reporting-service/Dockerfile-http-server b/examples/load-reporting-service/Dockerfile-http-server index a63927fbd804..a1dc60c1d81f 100644 --- a/examples/load-reporting-service/Dockerfile-http-server +++ b/examples/load-reporting-service/Dockerfile-http-server @@ -1,7 +1,7 @@ FROM alpine:latest RUN apk update && apk add py3-pip -RUN pip3 install -q Flask==0.11.1 +RUN pip3 install -q Flask==2.0.3 RUN mkdir /code COPY ./service.py ./code diff --git a/examples/win32-front-proxy/Dockerfile-service b/examples/win32-front-proxy/Dockerfile-service index a347d8c3189f..925bf95e6c96 100644 --- a/examples/win32-front-proxy/Dockerfile-service +++ b/examples/win32-front-proxy/Dockerfile-service @@ -3,7 +3,7 @@ FROM envoyproxy/envoy-windows-dev:latest COPY ./setup_python.ps1 / RUN powershell.exe .\\setup_python.ps1 -RUN pip3 install -q Flask==0.11.1 requests==2.18.4 +RUN pip3 install -q Flask==2.0.3 requests==2.18.4 RUN powershell mkdir code ADD ./service.py ./code From 8511ebd8c97be2c1837b163f219e2b961233e00b Mon Sep 17 00:00:00 2001 From: Yuchen Dai Date: Tue, 15 Mar 2022 16:36:41 -0700 Subject: [PATCH 13/15] SDS: reduce cost of secret update (#20191) Backport of #19971 Signed-off-by: Yuchen Dai --- docs/root/version_history/current.rst | 2 + envoy/ssl/context_manager.h | 13 +- .../tls/context_manager_impl.cc | 55 +++---- .../tls/context_manager_impl.h | 14 +- .../transport_sockets/tls/ssl_socket.cc | 17 ++- .../transport_sockets/tls/ssl_socket.h | 4 + source/server/ssl_context_manager.cc | 12 +- test/common/http/conn_pool_grid_test.cc | 2 +- .../client_connection_factory_impl_test.cc | 3 +- .../quic/envoy_quic_proof_source_test.cc | 1 + .../quic/envoy_quic_proof_verifier_test.cc | 1 + .../dynamic_forward_proxy/cluster_test.cc | 2 +- .../tls/context_impl_test.cc | 143 +++++++++++------- test/mocks/ssl/mocks.h | 7 +- test/server/admin/server_info_handler_test.cc | 3 +- test/server/ssl_context_manager_test.cc | 7 +- 16 files changed, 162 insertions(+), 124 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index f876f6cc1493..4ed393252497 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -9,6 +9,8 @@ Minor Behavior Changes ---------------------- *Changes that may cause incompatibilities for some users, but should not for most* +* perf: ssl contexts are now tracked without scan based garbage collection and greatly improved the performance on secret update. + Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* diff --git a/envoy/ssl/context_manager.h b/envoy/ssl/context_manager.h index 0302642db8cf..bf3d16014bf7 100644 --- a/envoy/ssl/context_manager.h +++ b/envoy/ssl/context_manager.h @@ -22,17 +22,15 @@ class ContextManager { /** * Builds a ClientContext from a ClientContextConfig. */ - virtual ClientContextSharedPtr - createSslClientContext(Stats::Scope& scope, const ClientContextConfig& config, - Envoy::Ssl::ClientContextSharedPtr old_context) PURE; + virtual ClientContextSharedPtr createSslClientContext(Stats::Scope& scope, + const ClientContextConfig& config) PURE; /** * Builds a ServerContext from a ServerContextConfig. */ virtual ServerContextSharedPtr createSslServerContext(Stats::Scope& scope, const ServerContextConfig& config, - const std::vector& server_names, - Envoy::Ssl::ServerContextSharedPtr old_context) PURE; + const std::vector& server_names) PURE; /** * @return the number of days until the next certificate being managed will expire. @@ -55,6 +53,11 @@ class ContextManager { * expire, or `absl::nullopt` if no OCSP responses exist. */ virtual absl::optional secondsUntilFirstOcspResponseExpires() const PURE; + + /** + * Remove an existing ssl context. + */ + virtual void removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) PURE; }; using ContextManagerPtr = std::unique_ptr; diff --git a/source/extensions/transport_sockets/tls/context_manager_impl.cc b/source/extensions/transport_sockets/tls/context_manager_impl.cc index 7c83121892f2..09cfa3f71aa8 100644 --- a/source/extensions/transport_sockets/tls/context_manager_impl.cc +++ b/source/extensions/transport_sockets/tls/context_manager_impl.cc @@ -14,62 +14,42 @@ namespace Extensions { namespace TransportSockets { namespace Tls { +ContextManagerImpl::ContextManagerImpl(TimeSource& time_source) : time_source_(time_source) {} + ContextManagerImpl::~ContextManagerImpl() { - removeEmptyContexts(); KNOWN_ISSUE_ASSERT(contexts_.empty(), "https://github.com/envoyproxy/envoy/issues/10030"); } -void ContextManagerImpl::removeEmptyContexts() { - contexts_.remove_if([](const std::weak_ptr& n) { return n.expired(); }); -} - -void ContextManagerImpl::removeOldContext(std::shared_ptr old_context) { - if (old_context) { - contexts_.remove_if([old_context](const std::weak_ptr& n) { - std::shared_ptr sp = n.lock(); - if (sp) { - return old_context == sp; - } - return false; - }); - } -} - Envoy::Ssl::ClientContextSharedPtr ContextManagerImpl::createSslClientContext(Stats::Scope& scope, - const Envoy::Ssl::ClientContextConfig& config, - Envoy::Ssl::ClientContextSharedPtr old_context) { + const Envoy::Ssl::ClientContextConfig& config) { if (!config.isReady()) { return nullptr; } Envoy::Ssl::ClientContextSharedPtr context = std::make_shared(scope, config, time_source_); - removeOldContext(old_context); - removeEmptyContexts(); - contexts_.emplace_back(context); + contexts_.insert(context); return context; } -Envoy::Ssl::ServerContextSharedPtr ContextManagerImpl::createSslServerContext( - Stats::Scope& scope, const Envoy::Ssl::ServerContextConfig& config, - const std::vector& server_names, Envoy::Ssl::ServerContextSharedPtr old_context) { +Envoy::Ssl::ServerContextSharedPtr +ContextManagerImpl::createSslServerContext(Stats::Scope& scope, + const Envoy::Ssl::ServerContextConfig& config, + const std::vector& server_names) { if (!config.isReady()) { return nullptr; } Envoy::Ssl::ServerContextSharedPtr context = std::make_shared(scope, config, server_names, time_source_); - removeOldContext(old_context); - removeEmptyContexts(); - contexts_.emplace_back(context); + contexts_.insert(context); return context; } size_t ContextManagerImpl::daysUntilFirstCertExpires() const { size_t ret = std::numeric_limits::max(); - for (const auto& ctx_weak_ptr : contexts_) { - Envoy::Ssl::ContextSharedPtr context = ctx_weak_ptr.lock(); + for (const auto& context : contexts_) { if (context) { ret = std::min(context->daysUntilFirstCertExpires(), ret); } @@ -79,8 +59,7 @@ size_t ContextManagerImpl::daysUntilFirstCertExpires() const { absl::optional ContextManagerImpl::secondsUntilFirstOcspResponseExpires() const { absl::optional ret; - for (const auto& ctx_weak_ptr : contexts_) { - Envoy::Ssl::ContextSharedPtr context = ctx_weak_ptr.lock(); + for (const auto& context : contexts_) { if (context) { auto next_expiration = context->secondsUntilFirstOcspResponseExpires(); if (next_expiration) { @@ -93,14 +72,22 @@ absl::optional ContextManagerImpl::secondsUntilFirstOcspResponseExpire } void ContextManagerImpl::iterateContexts(std::function callback) { - for (const auto& ctx_weak_ptr : contexts_) { - Envoy::Ssl::ContextSharedPtr context = ctx_weak_ptr.lock(); + for (const auto& context : contexts_) { if (context) { callback(*context); } } } +void ContextManagerImpl::removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) { + if (old_context != nullptr) { + auto erased = contexts_.erase(old_context); + // The contexts is expected to be added before is removed. + // And the prod ssl factory implementation guarantees any context is removed exactly once. + ASSERT(erased == 1); + } +} + } // namespace Tls } // namespace TransportSockets } // namespace Extensions diff --git a/source/extensions/transport_sockets/tls/context_manager_impl.h b/source/extensions/transport_sockets/tls/context_manager_impl.h index eb8914fc0009..7a05015e682f 100644 --- a/source/extensions/transport_sockets/tls/context_manager_impl.h +++ b/source/extensions/transport_sockets/tls/context_manager_impl.h @@ -24,29 +24,27 @@ namespace Tls { */ class ContextManagerImpl final : public Envoy::Ssl::ContextManager { public: - ContextManagerImpl(TimeSource& time_source) : time_source_(time_source) {} + explicit ContextManagerImpl(TimeSource& time_source); ~ContextManagerImpl() override; // Ssl::ContextManager Ssl::ClientContextSharedPtr - createSslClientContext(Stats::Scope& scope, const Envoy::Ssl::ClientContextConfig& config, - Envoy::Ssl::ClientContextSharedPtr old_context) override; + createSslClientContext(Stats::Scope& scope, + const Envoy::Ssl::ClientContextConfig& config) override; Ssl::ServerContextSharedPtr createSslServerContext(Stats::Scope& scope, const Envoy::Ssl::ServerContextConfig& config, - const std::vector& server_names, - Envoy::Ssl::ServerContextSharedPtr old_context) override; + const std::vector& server_names) override; size_t daysUntilFirstCertExpires() const override; absl::optional secondsUntilFirstOcspResponseExpires() const override; void iterateContexts(std::function callback) override; Ssl::PrivateKeyMethodManager& privateKeyMethodManager() override { return private_key_method_manager_; }; + void removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) override; private: - void removeEmptyContexts(); - void removeOldContext(std::shared_ptr old_context); TimeSource& time_source_; - std::list> contexts_; + absl::flat_hash_set contexts_; PrivateKeyMethodManagerImpl private_key_method_manager_{}; }; diff --git a/source/extensions/transport_sockets/tls/ssl_socket.cc b/source/extensions/transport_sockets/tls/ssl_socket.cc index 31ed7fd52f01..b403afec7d39 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.cc +++ b/source/extensions/transport_sockets/tls/ssl_socket.cc @@ -356,10 +356,12 @@ ClientSslSocketFactory::ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPt Stats::Scope& stats_scope) : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("client", stats_scope)), config_(std::move(config)), - ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_, nullptr)) { + ssl_ctx_(manager_.createSslClientContext(stats_scope_, *config_)) { config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); }); } +ClientSslSocketFactory::~ClientSslSocketFactory() { manager_.removeContext(ssl_ctx_); } + Network::TransportSocketPtr ClientSslSocketFactory::createTransportSocket( Network::TransportSocketOptionsConstSharedPtr transport_socket_options) const { // onAddOrUpdateSecret() could be invoked in the middle of checking the existence of ssl_ctx and @@ -384,10 +386,12 @@ bool ClientSslSocketFactory::implementsSecureTransport() const { return true; } void ClientSslSocketFactory::onAddOrUpdateSecret() { ENVOY_LOG(debug, "Secret is updated."); + auto ctx = manager_.createSslClientContext(stats_scope_, *config_); { absl::WriterMutexLock l(&ssl_ctx_mu_); - ssl_ctx_ = manager_.createSslClientContext(stats_scope_, *config_, ssl_ctx_); + std::swap(ctx, ssl_ctx_); } + manager_.removeContext(ctx); stats_.ssl_context_update_by_sds_.inc(); } @@ -397,10 +401,12 @@ ServerSslSocketFactory::ServerSslSocketFactory(Envoy::Ssl::ServerContextConfigPt const std::vector& server_names) : manager_(manager), stats_scope_(stats_scope), stats_(generateStats("server", stats_scope)), config_(std::move(config)), server_names_(server_names), - ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_, nullptr)) { + ssl_ctx_(manager_.createSslServerContext(stats_scope_, *config_, server_names_)) { config_->setSecretUpdateCallback([this]() { onAddOrUpdateSecret(); }); } +ServerSslSocketFactory::~ServerSslSocketFactory() { manager_.removeContext(ssl_ctx_); } + Envoy::Ssl::ClientContextSharedPtr ClientSslSocketFactory::sslCtx() { absl::ReaderMutexLock l(&ssl_ctx_mu_); return ssl_ctx_; @@ -430,10 +436,13 @@ bool ServerSslSocketFactory::implementsSecureTransport() const { return true; } void ServerSslSocketFactory::onAddOrUpdateSecret() { ENVOY_LOG(debug, "Secret is updated."); + auto ctx = manager_.createSslServerContext(stats_scope_, *config_, server_names_); { absl::WriterMutexLock l(&ssl_ctx_mu_); - ssl_ctx_ = manager_.createSslServerContext(stats_scope_, *config_, server_names_, ssl_ctx_); + std::swap(ctx, ssl_ctx_); } + manager_.removeContext(ctx); + stats_.ssl_context_update_by_sds_.inc(); } diff --git a/source/extensions/transport_sockets/tls/ssl_socket.h b/source/extensions/transport_sockets/tls/ssl_socket.h index a6502cc9548c..4874b78b5585 100644 --- a/source/extensions/transport_sockets/tls/ssl_socket.h +++ b/source/extensions/transport_sockets/tls/ssl_socket.h @@ -103,6 +103,8 @@ class ClientSslSocketFactory : public Network::TransportSocketFactory, ClientSslSocketFactory(Envoy::Ssl::ClientContextConfigPtr config, Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope); + ~ClientSslSocketFactory() override; + Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsConstSharedPtr options) const override; bool implementsSecureTransport() const override; @@ -133,6 +135,8 @@ class ServerSslSocketFactory : public Network::TransportSocketFactory, Envoy::Ssl::ContextManager& manager, Stats::Scope& stats_scope, const std::vector& server_names); + ~ServerSslSocketFactory() override; + Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsConstSharedPtr options) const override; bool implementsSecureTransport() const override; diff --git a/source/server/ssl_context_manager.cc b/source/server/ssl_context_manager.cc index 0189da0c6d04..a1a9fc12c9d0 100644 --- a/source/server/ssl_context_manager.cc +++ b/source/server/ssl_context_manager.cc @@ -14,16 +14,14 @@ namespace Server { class SslContextManagerNoTlsStub final : public Envoy::Ssl::ContextManager { Ssl::ClientContextSharedPtr createSslClientContext(Stats::Scope& /* scope */, - const Envoy::Ssl::ClientContextConfig& /* config */, - Envoy::Ssl::ClientContextSharedPtr /* old_context */) override { + const Envoy::Ssl::ClientContextConfig& /* config */) override { throwException(); } Ssl::ServerContextSharedPtr createSslServerContext(Stats::Scope& /* scope */, const Envoy::Ssl::ServerContextConfig& /* config */, - const std::vector& /* server_names */, - Envoy::Ssl::ServerContextSharedPtr /* old_context */) override { + const std::vector& /* server_names */) override { throwException(); } @@ -36,6 +34,12 @@ class SslContextManagerNoTlsStub final : public Envoy::Ssl::ContextManager { Ssl::PrivateKeyMethodManager& privateKeyMethodManager() override { throwException(); } + void removeContext(const Envoy::Ssl::ContextSharedPtr& old_context) override { + if (old_context) { + throw EnvoyException("SSL is not supported in this configuration"); + } + } + private: [[noreturn]] void throwException() { throw EnvoyException("SSL is not supported in this configuration"); diff --git a/test/common/http/conn_pool_grid_test.cc b/test/common/http/conn_pool_grid_test.cc index 69528416002d..dd32fe08a911 100644 --- a/test/common/http/conn_pool_grid_test.cc +++ b/test/common/http/conn_pool_grid_test.cc @@ -691,7 +691,7 @@ TEST_F(ConnectivityGridTest, ConnectionCloseDuringCreation) { Envoy::Ssl::ClientContextConfigPtr config(new NiceMock()); NiceMock factory_context; Ssl::ClientContextSharedPtr ssl_context(new Ssl::MockClientContext()); - EXPECT_CALL(factory_context.context_manager_, createSslClientContext(_, _, _)) + EXPECT_CALL(factory_context.context_manager_, createSslClientContext(_, _)) .WillOnce(Return(ssl_context)); auto factory = std::make_unique(std::move(config), factory_context); diff --git a/test/common/quic/client_connection_factory_impl_test.cc b/test/common/quic/client_connection_factory_impl_test.cc index 97220f85244a..6dae4ee709d9 100644 --- a/test/common/quic/client_connection_factory_impl_test.cc +++ b/test/common/quic/client_connection_factory_impl_test.cc @@ -25,8 +25,7 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, public t Network::Test::getLoopbackAddressString(TestEnvironment::getIpVersionsForTest()[0]), ":30")); Ssl::ClientContextSharedPtr context{new Ssl::MockClientContext()}; - EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _, _)) - .WillOnce(Return(context)); + EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _)).WillOnce(Return(context)); factory_ = std::make_unique( std::unique_ptr( new NiceMock), diff --git a/test/common/quic/envoy_quic_proof_source_test.cc b/test/common/quic/envoy_quic_proof_source_test.cc index 4e27141773ee..40021edd52bf 100644 --- a/test/common/quic/envoy_quic_proof_source_test.cc +++ b/test/common/quic/envoy_quic_proof_source_test.cc @@ -101,6 +101,7 @@ class SignatureVerifier { NiceMock client_context_config_; NiceMock cert_validation_ctx_config_; std::unique_ptr verifier_; + NiceMock tls_context_manager_; }; class TestSignatureCallback : public quic::ProofSource::SignatureCallback { diff --git a/test/common/quic/envoy_quic_proof_verifier_test.cc b/test/common/quic/envoy_quic_proof_verifier_test.cc index c6b075fe03ea..6aa79b6ee4f8 100644 --- a/test/common/quic/envoy_quic_proof_verifier_test.cc +++ b/test/common/quic/envoy_quic_proof_verifier_test.cc @@ -89,6 +89,7 @@ class EnvoyQuicProofVerifierTest : public testing::Test { NiceMock client_context_config_; Ssl::MockCertificateValidationContextConfig cert_validation_ctx_config_; std::unique_ptr verifier_; + NiceMock tls_context_manager_; }; TEST_F(EnvoyQuicProofVerifierTest, VerifyCertChainSuccess) { diff --git a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc index 097dcf993cd4..eccd93e18699 100644 --- a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc +++ b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc @@ -43,7 +43,7 @@ class ClusterTest : public testing::Test, admin_, ssl_context_manager_, *scope, cm_, local_info_, dispatcher_, stats_store_, singleton_manager_, tls_, validation_visitor_, *api_, options_); if (uses_tls) { - EXPECT_CALL(ssl_context_manager_, createSslClientContext(_, _, _)); + EXPECT_CALL(ssl_context_manager_, createSslClientContext(_, _)); } EXPECT_CALL(*dns_cache_manager_, getCache(_)); // Below we return a nullptr handle which has no effect on the code under test but isn't diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 8e4bfc66ef77..023694982000 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -66,6 +66,7 @@ const std::vector& knownCipherSuites() { "PSK-AES256-CBC-SHA", "DES-CBC3-SHA"}); } + } // namespace class SslLibraryCipherSuiteSupport : public ::testing::TestWithParam {}; @@ -95,6 +96,22 @@ TEST_P(SslLibraryCipherSuiteSupport, CipherSuitesNotRemoved) { } class SslContextImplTest : public SslCertsTest { +public: + ABSL_MUST_USE_RESULT Cleanup cleanUpHelper(Envoy::Ssl::ClientContextSharedPtr& context) { + return Cleanup([&manager = manager_, &context]() { + if (context != nullptr) { + manager.removeContext(context); + } + }); + } + ABSL_MUST_USE_RESULT Cleanup cleanUpHelper(Envoy::Ssl::ServerContextSharedPtr& context) { + return Cleanup([&manager = manager_, &context]() { + if (context != nullptr) { + manager.removeContext(context); + } + }); + } + protected: Event::SimulatedTimeSystem time_system_; ContextManagerImpl manager_{time_system_}; @@ -111,7 +128,7 @@ TEST_F(SslContextImplTest, TestCipherSuites) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); EXPECT_THROW_WITH_MESSAGE( - manager_.createSslClientContext(store_, cfg, nullptr), EnvoyException, + manager_.createSslClientContext(store_, cfg), EnvoyException, "Failed to initialize cipher suites " "-ALL:+[AES128-SHA|BOGUS1-SHA256]:BOGUS2-SHA:AES256-SHA. The following " "ciphers were rejected when tried individually: BOGUS1-SHA256, BOGUS2-SHA"); @@ -131,8 +148,8 @@ TEST_F(SslContextImplTest, TestExpiringCert) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); - + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); // Calculate the days until test cert expires auto cert_expiry = TestUtility::parseTime(TEST_UNITTEST_CERT_NOT_AFTER, "%b %d %H:%M:%S %Y GMT"); int64_t days_until_expiry = absl::ToInt64Hours(cert_expiry - absl::Now()) / 24; @@ -152,7 +169,8 @@ TEST_F(SslContextImplTest, TestExpiredCert) { envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); EXPECT_EQ(0U, context->daysUntilFirstCertExpires()); } @@ -172,7 +190,7 @@ TEST_F(SslContextImplTest, TestContextUpdate) { envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context; TestUtility::loadFromYaml(TestEnvironment::substitute(expired_yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); EXPECT_EQ(manager_.daysUntilFirstCertExpires(), 0U); const std::string expiring_yaml = R"EOF( @@ -190,7 +208,8 @@ TEST_F(SslContextImplTest, TestContextUpdate) { ClientContextConfigImpl expiring_cfg(expiring_context, factory_context_); Envoy::Ssl::ClientContextSharedPtr new_context( - manager_.createSslClientContext(store_, expiring_cfg, context)); + manager_.createSslClientContext(store_, expiring_cfg)); + manager_.removeContext(context); // Validate that when the context is updated, daysUntilFirstCertExpires reflects the current // context expiry. @@ -201,8 +220,10 @@ TEST_F(SslContextImplTest, TestContextUpdate) { // Update the context again and validate daysUntilFirstCertExpires still reflects the current // expiry. - Envoy::Ssl::ClientContextSharedPtr updated_context( - manager_.createSslClientContext(store_, cfg, new_context)); + Envoy::Ssl::ClientContextSharedPtr updated_context(manager_.createSslClientContext(store_, cfg)); + manager_.removeContext(new_context); + auto cleanup = cleanUpHelper(updated_context); + EXPECT_EQ(updated_context->daysUntilFirstCertExpires(), 0U); EXPECT_EQ(manager_.daysUntilFirstCertExpires(), 0U); } @@ -224,7 +245,9 @@ TEST_F(SslContextImplTest, TestGetCertInformation) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); + // This is similar to the hack above, but right now we generate the ca_cert and it expires in 15 // days only in the first second that it's valid. We will partially match for up until Days until // Expiration: 1. @@ -274,7 +297,8 @@ TEST_F(SslContextImplTest, TestGetCertInformationWithSAN) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); std::string ca_cert_json = absl::StrCat(R"EOF({ "path": "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns3_cert.pem", "serial_number": ")EOF", @@ -328,7 +352,8 @@ TEST_F(SslContextImplTest, TestGetCertInformationWithIPSAN) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); std::string ca_cert_json = absl::StrCat(R"EOF({ "path": "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_ip_cert.pem", "serial_number": ")EOF", @@ -386,7 +411,9 @@ TEST_F(SslContextImplTest, TestGetCertInformationWithExpiration) { TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); ClientContextConfigImpl cfg(tls_context, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); + std::string ca_cert_json = absl::StrCat(R"EOF({ "path": "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/san_dns3_cert.pem", @@ -416,7 +443,8 @@ TEST_F(SslContextImplTest, TestGetCertInformationWithExpiration) { TEST_F(SslContextImplTest, TestNoCert) { envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext config; ClientContextConfigImpl cfg(config, factory_context_); - Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg, nullptr)); + Envoy::Ssl::ClientContextSharedPtr context(manager_.createSslClientContext(store_, cfg)); + auto cleanup = cleanUpHelper(context); EXPECT_EQ(nullptr, context->getCaCertInformation()); EXPECT_TRUE(context->getCertChainInformation().empty()); } @@ -438,9 +466,9 @@ TEST_F(SslContextImplTest, AtMostOneRsaCert) { )EOF"; TestUtility::loadFromYaml(TestEnvironment::substitute(tls_context_yaml), tls_context); ServerContextConfigImpl server_context_config(tls_context, factory_context_); - EXPECT_THROW_WITH_REGEX( - manager_.createSslServerContext(store_, server_context_config, {}, nullptr), EnvoyException, - "at most one certificate of a given type may be specified"); + EXPECT_THROW_WITH_REGEX(manager_.createSslServerContext(store_, server_context_config, {}), + EnvoyException, + "at most one certificate of a given type may be specified"); } // Multiple ECDSA certificates are rejected. @@ -460,9 +488,9 @@ TEST_F(SslContextImplTest, AtMostOneEcdsaCert) { )EOF"; TestUtility::loadFromYaml(TestEnvironment::substitute(tls_context_yaml), tls_context); ServerContextConfigImpl server_context_config(tls_context, factory_context_); - EXPECT_THROW_WITH_REGEX( - manager_.createSslServerContext(store_, server_context_config, {}, nullptr), EnvoyException, - "at most one certificate of a given type may be specified"); + EXPECT_THROW_WITH_REGEX(manager_.createSslServerContext(store_, server_context_config, {}), + EnvoyException, + "at most one certificate of a given type may be specified"); } // Certificates with no subject CN and no SANs are rejected. @@ -478,15 +506,14 @@ TEST_F(SslContextImplTest, MustHaveSubjectOrSAN) { )EOF"; TestUtility::loadFromYaml(TestEnvironment::substitute(tls_context_yaml), tls_context); ServerContextConfigImpl server_context_config(tls_context, factory_context_); - EXPECT_THROW_WITH_REGEX( - manager_.createSslServerContext(store_, server_context_config, {}, nullptr), EnvoyException, - "has neither subject CN nor SAN names"); + EXPECT_THROW_WITH_REGEX(manager_.createSslServerContext(store_, server_context_config, {}), + EnvoyException, "has neither subject CN nor SAN names"); } class SslServerContextImplOcspTest : public SslContextImplTest { public: Envoy::Ssl::ServerContextSharedPtr loadConfig(ServerContextConfigImpl& cfg) { - return manager_.createSslServerContext(store_, cfg, std::vector{}, nullptr); + return manager_.createSslServerContext(store_, cfg, std::vector{}); } Envoy::Ssl::ServerContextSharedPtr loadConfigYaml(const std::string& yaml) { @@ -509,7 +536,8 @@ TEST_F(SslServerContextImplOcspTest, TestFilenameOcspStapleConfigLoads) { filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/ocsp/test_data/good_ocsp_resp.der" ocsp_staple_policy: must_staple )EOF"; - loadConfigYaml(tls_context_yaml); + auto context = loadConfigYaml(tls_context_yaml); + auto cleanup = cleanUpHelper(context); } TEST_F(SslServerContextImplOcspTest, TestInlineBytesOcspStapleConfigLoads) { @@ -529,7 +557,8 @@ TEST_F(SslServerContextImplOcspTest, TestInlineBytesOcspStapleConfigLoads) { )EOF", base64_response); - loadConfigYaml(tls_context_yaml); + auto context = loadConfigYaml(tls_context_yaml); + auto cleanup = cleanUpHelper(context); } TEST_F(SslServerContextImplOcspTest, TestInlineStringOcspStapleConfigFails) { @@ -638,6 +667,7 @@ TEST_F(SslServerContextImplOcspTest, TestGetCertInformationWithOCSP) { envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context; TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), tls_context); auto context = loadConfigYaml(yaml); + auto cleanup = cleanUpHelper(context); constexpr absl::string_view this_update = "This Update: "; constexpr absl::string_view next_update = "Next Update: "; @@ -683,7 +713,8 @@ class SslServerContextImplTicketTest : public SslContextImplTest { public: void loadConfig(ServerContextConfigImpl& cfg) { Envoy::Ssl::ServerContextSharedPtr server_ctx( - manager_.createSslServerContext(store_, cfg, std::vector{}, nullptr)); + manager_.createSslServerContext(store_, cfg, std::vector{})); + auto cleanup = cleanUpHelper(server_ctx); } void loadConfigV2(envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext& cfg) { @@ -1002,7 +1033,19 @@ TEST_F(SslServerContextImplTicketTest, StatelessSessionResumptionEnabledWhenKeyI EXPECT_FALSE(server_context_config.disableStatelessSessionResumption()); } -class ClientContextConfigImplTest : public SslCertsTest {}; +class ClientContextConfigImplTest : public SslCertsTest { +public: + ABSL_MUST_USE_RESULT Cleanup cleanUpHelper(Envoy::Ssl::ClientContextSharedPtr& context) { + return Cleanup([&manager = manager_, &context]() { + if (context != nullptr) { + manager.removeContext(context); + } + }); + } + + Event::SimulatedTimeSystem time_system_; + ContextManagerImpl manager_{time_system_}; +}; // Validate that empty SNI (according to C string rules) fails config validation. TEST_F(ClientContextConfigImplTest, EmptyServerNameIndication) { @@ -1029,10 +1072,8 @@ TEST_F(ClientContextConfigImplTest, InvalidCertificateHash) { ->add_verify_certificate_hash("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); ClientContextConfigImpl client_context_config(tls_context, factory_context); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, "Invalid hex-encoded SHA-256 .*"); } @@ -1045,10 +1086,8 @@ TEST_F(ClientContextConfigImplTest, InvalidCertificateSpki) { // Not a base64-encoded string. ->add_verify_certificate_spki("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); ClientContextConfigImpl client_context_config(tls_context, factory_context); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, "Invalid base64-encoded SHA-256 .*"); } @@ -1064,10 +1103,9 @@ TEST_F(ClientContextConfigImplTest, RSA2048Cert) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), *tls_context.mutable_common_tls_context()->add_tls_certificates()); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - manager.createSslClientContext(store, client_context_config, nullptr); + auto context = manager_.createSslClientContext(store, client_context_config); + auto cleanup = cleanUpHelper(context); } // Validate that 1024-bit RSA certificates are rejected. @@ -1082,8 +1120,6 @@ TEST_F(ClientContextConfigImplTest, RSA1024Cert) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), *tls_context.mutable_common_tls_context()->add_tls_certificates()); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; std::string error_msg( @@ -1094,7 +1130,7 @@ TEST_F(ClientContextConfigImplTest, RSA1024Cert) { "with 2048-bit or larger keys are supported" #endif ); - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, error_msg); } @@ -1113,7 +1149,8 @@ TEST_F(ClientContextConfigImplTest, RSA3072Cert) { Event::SimulatedTimeSystem time_system; ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - manager.createSslClientContext(store, client_context_config, nullptr); + auto context = manager_.createSslClientContext(store, client_context_config); + auto cleanup = cleanUpHelper(context); } // Validate that 4096-bit RSA certificates load successfully. @@ -1128,10 +1165,9 @@ TEST_F(ClientContextConfigImplTest, RSA4096Cert) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), *tls_context.mutable_common_tls_context()->add_tls_certificates()); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - manager.createSslClientContext(store, client_context_config, nullptr); + auto context = manager_.createSslClientContext(store, client_context_config); + auto cleanup = cleanUpHelper(context); } // Validate that P256 ECDSA certs load. @@ -1146,10 +1182,9 @@ TEST_F(ClientContextConfigImplTest, P256EcdsaCert) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), *tls_context.mutable_common_tls_context()->add_tls_certificates()); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - manager.createSslClientContext(store, client_context_config, nullptr); + auto context = manager_.createSslClientContext(store, client_context_config); + auto cleanup = cleanUpHelper(context); } // Validate that non-P256 ECDSA certs are rejected. @@ -1164,10 +1199,8 @@ TEST_F(ClientContextConfigImplTest, NonP256EcdsaCert) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), *tls_context.mutable_common_tls_context()->add_tls_certificates()); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, "Failed to load certificate chain from .*selfsigned_ecdsa_p384_cert.pem, " "only P-256 ECDSA certificates are supported"); @@ -1370,10 +1403,8 @@ TEST_F(ClientContextConfigImplTest, PasswordNotSuppliedTlsCertificates) { factory_context_.secretManager().addStaticSecret(secret_config); ClientContextConfigImpl client_context_config(tls_context, factory_context_); - Event::SimulatedTimeSystem time_system; - ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, absl::StrCat("Failed to load private key from ", private_key_path)); } @@ -1615,8 +1646,8 @@ TEST_F(ServerContextConfigImplTest, TlsCertificateNonEmpty) { ContextManagerImpl manager(time_system); Stats::IsolatedStoreImpl store; EXPECT_THROW_WITH_MESSAGE( - Envoy::Ssl::ServerContextSharedPtr server_ctx(manager.createSslServerContext( - store, client_context_config, std::vector{}, nullptr)), + Envoy::Ssl::ServerContextSharedPtr server_ctx( + manager.createSslServerContext(store, client_context_config, std::vector{})), EnvoyException, "Server TlsCertificates must have a certificate specified"); } @@ -1713,8 +1744,8 @@ TEST_F(ServerContextConfigImplTest, PrivateKeyMethodLoadFailureNoMethod) { TestUtility::loadFromYaml(TestEnvironment::substitute(tls_context_yaml), tls_context); ServerContextConfigImpl server_context_config(tls_context, factory_context_); EXPECT_THROW_WITH_MESSAGE( - Envoy::Ssl::ServerContextSharedPtr server_ctx(manager.createSslServerContext( - store, server_context_config, std::vector{}, nullptr)), + Envoy::Ssl::ServerContextSharedPtr server_ctx( + manager.createSslServerContext(store, server_context_config, std::vector{})), EnvoyException, "Failed to get BoringSSL private key method from provider"); } diff --git a/test/mocks/ssl/mocks.h b/test/mocks/ssl/mocks.h index e36b06fbd99d..06d4d88b06fd 100644 --- a/test/mocks/ssl/mocks.h +++ b/test/mocks/ssl/mocks.h @@ -25,16 +25,15 @@ class MockContextManager : public ContextManager { ~MockContextManager() override; MOCK_METHOD(ClientContextSharedPtr, createSslClientContext, - (Stats::Scope & scope, const ClientContextConfig& config, - Envoy::Ssl::ClientContextSharedPtr old_context)); + (Stats::Scope & scope, const ClientContextConfig& config)); MOCK_METHOD(ServerContextSharedPtr, createSslServerContext, (Stats::Scope & stats, const ServerContextConfig& config, - const std::vector& server_names, - Envoy::Ssl::ServerContextSharedPtr old_context)); + const std::vector& server_names)); MOCK_METHOD(size_t, daysUntilFirstCertExpires, (), (const)); MOCK_METHOD(absl::optional, secondsUntilFirstOcspResponseExpires, (), (const)); MOCK_METHOD(void, iterateContexts, (std::function callback)); MOCK_METHOD(Ssl::PrivateKeyMethodManager&, privateKeyMethodManager, ()); + MOCK_METHOD(void, removeContext, (const Envoy::Ssl::ContextSharedPtr& old_context)); }; class MockConnectionInfo : public ConnectionInfo { diff --git a/test/server/admin/server_info_handler_test.cc b/test/server/admin/server_info_handler_test.cc index 9b7e0fa3643f..9d310832e309 100644 --- a/test/server/admin/server_info_handler_test.cc +++ b/test/server/admin/server_info_handler_test.cc @@ -28,7 +28,7 @@ TEST_P(AdminInstanceTest, ContextThatReturnsNullCertDetails) { Extensions::TransportSockets::Tls::ClientContextConfigImpl cfg(config, factory_context); Stats::IsolatedStoreImpl store; Envoy::Ssl::ClientContextSharedPtr client_ctx( - server_.sslContextManager().createSslClientContext(store, cfg, nullptr)); + server_.sslContextManager().createSslClientContext(store, cfg)); const std::string expected_empty_json = R"EOF({ "certificates": [ @@ -45,6 +45,7 @@ TEST_P(AdminInstanceTest, ContextThatReturnsNullCertDetails) { EXPECT_TRUE(client_ctx->getCertChainInformation().empty()); EXPECT_EQ(Http::Code::OK, getCallback("/certs", header_map, response)); EXPECT_EQ(expected_empty_json, response.toString()); + server_.sslContextManager().removeContext(client_ctx); } TEST_P(AdminInstanceTest, Memory) { diff --git a/test/server/ssl_context_manager_test.cc b/test/server/ssl_context_manager_test.cc index 70bc7d6c1a5c..f25dba343ded 100644 --- a/test/server/ssl_context_manager_test.cc +++ b/test/server/ssl_context_manager_test.cc @@ -23,11 +23,10 @@ TEST(SslContextManager, createStub) { // Check we've created a stub, not real manager. EXPECT_EQ(manager->daysUntilFirstCertExpires(), std::numeric_limits::max()); EXPECT_EQ(manager->secondsUntilFirstOcspResponseExpires(), absl::nullopt); - EXPECT_THROW_WITH_MESSAGE(manager->createSslClientContext(scope, client_config, nullptr), + EXPECT_THROW_WITH_MESSAGE(manager->createSslClientContext(scope, client_config), EnvoyException, + "SSL is not supported in this configuration"); + EXPECT_THROW_WITH_MESSAGE(manager->createSslServerContext(scope, server_config, server_names), EnvoyException, "SSL is not supported in this configuration"); - EXPECT_THROW_WITH_MESSAGE( - manager->createSslServerContext(scope, server_config, server_names, nullptr), EnvoyException, - "SSL is not supported in this configuration"); EXPECT_NO_THROW(manager->iterateContexts([](const Envoy::Ssl::Context&) -> void {})); } From 473c1dbfe0ad677930d11da58479efb5af59e4d3 Mon Sep 17 00:00:00 2001 From: Jonh Wendell Date: Tue, 22 Mar 2022 12:47:28 -0400 Subject: [PATCH 14/15] Bump gazelle to 0.24.0 (#20394) * Bump gazelle to 0.24.0 This bump fixes issues with go 1.16+ like this one: ``` DEBUG: /tmp/tmp.g1skLo1cY4/external/bazel_gazelle/internal/go_repository.bzl:189:18: com_github_lyft_protoc_gen_star: gazelle: finding module path for import github.com/stretchr/testify/assert: exit status 1: go: malformed module path "gazelle_remote_cache__\\n": invalid char '\\' gazelle: finding module path for import github.com/stretchr/testify/require: exit status 1: go: malformed module path "gazelle_remote_cache__\\n": invalid char '\\' ``` Signed-off-by: Jonh Wendell --- bazel/repository_locations.bzl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 899b16998138..bf1abb074293 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -15,10 +15,10 @@ REPOSITORY_LOCATIONS_SPEC = dict( project_name = "Gazelle", project_desc = "Bazel BUILD file generator for Go projects", project_url = "https://github.com/bazelbuild/bazel-gazelle", - version = "0.22.2", - sha256 = "b85f48fa105c4403326e9525ad2b2cc437babaa6e15a3fc0b1dbab0ab064bc7c", + version = "0.24.0", + sha256 = "de69a09dc70417580aabf20a28619bb3ef60d038470c7cf8442fafcf627c21cb", urls = ["https://github.com/bazelbuild/bazel-gazelle/releases/download/v{version}/bazel-gazelle-v{version}.tar.gz"], - release_date = "2020-10-02", + release_date = "2021-10-11", use_category = ["build"], ), bazel_toolchains = dict( @@ -963,7 +963,7 @@ REPOSITORY_LOCATIONS_SPEC = dict( urls = ["https://github.com/edenhill/librdkafka/archive/v{version}.tar.gz"], use_category = ["dataplane_ext"], extensions = ["envoy.filters.network.kafka_mesh"], - release_date = "2021-05-10", + release_date = "2021-05-06", cpe = "N/A", ), kafka_server_binary = dict( From 4495532c501e8683a478b37fcd749a927d5ee26f Mon Sep 17 00:00:00 2001 From: Otto van der Schaaf Date: Thu, 24 Mar 2022 09:57:14 +0100 Subject: [PATCH 15/15] Tweak to unbreak building context_impl_test.cc --- test/extensions/transport_sockets/tls/context_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc index 44eb15495df7..d3ef29a4159e 100644 --- a/test/extensions/transport_sockets/tls/context_impl_test.cc +++ b/test/extensions/transport_sockets/tls/context_impl_test.cc @@ -1134,7 +1134,7 @@ TEST_F(ClientContextConfigImplTest, RSA1024Cert) { "Failed to load certificate chain from .*selfsigned_rsa_1024_cert.pem, only RSA certificates " "with 2048-bit or larger keys are supported|Failed to load certificate chain from " ".*selfsigned_rsa_1024_cert.pem, please see log for details"); - EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config, nullptr), + EXPECT_THROW_WITH_REGEX(manager_.createSslClientContext(store, client_context_config), EnvoyException, error_msg); }