From e21ed32394ba9bddfe5880c6197c7f516ba52535 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 14 Dec 2021 12:13:29 -0500 Subject: [PATCH 1/3] alpn: fixing config validation for socket matchers Signed-off-by: Alyssa Wilk --- docs/root/version_history/current.rst | 3 +- envoy/upstream/host_description.h | 5 + source/common/runtime/runtime_features.cc | 1 + .../upstream/transport_socket_match_impl.h | 12 ++ source/common/upstream/upstream_impl.cc | 18 ++- .../upstream/transport_socket_matcher_test.cc | 68 +++++++- test/common/upstream/upstream_impl_test.cc | 151 ++++++++++++++++++ test/mocks/upstream/transport_socket_match.h | 1 + 8 files changed, 250 insertions(+), 9 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 30c930490f58..ee8dc294b17d 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -1,10 +1,11 @@ -1.21.0 (Pending) +g.21.0 (Pending) ================ Incompatible Behavior Changes ----------------------------- *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* +* auto_config: :ref:`auto_config: ` now verifies that any transport sockets configured via :ref:`transport_socket_matches ` support ALPN. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.correctly_validate_alpn`` to false.. * xds: ``*`` became a reserved name for a wildcard resource that can be subscribed to and unsubscribed from at any time. This is a requirement for implementing the on-demand xDSes (like on-demand CDS) that can subscribe to specific resources next to their wildcard subscription. If such xDS is subscribed to both wildcard resource and to other specific resource, then in stream reconnection scenario, the xDS will not send an empty initial request, but a request containing ``*`` for wildcard subscription and the rest of the resources the xDS is subscribed to. If the xDS is only subscribed to wildcard resource, it will try to send a legacy wildcard request. This behavior implements the recent changes in :ref:`xDS protocol ` and can be temporarily reverted by setting the ``envoy.restart_features.explicit_wildcard_resource`` runtime guard to false. Minor Behavior Changes diff --git a/envoy/upstream/host_description.h b/envoy/upstream/host_description.h index 2a958d6ab2e8..d43193f67a42 100644 --- a/envoy/upstream/host_description.h +++ b/envoy/upstream/host_description.h @@ -195,6 +195,11 @@ class TransportSocketMatcher { * @return the match information of the transport socket selected. */ virtual MatchData resolve(const envoy::config::core::v3::Metadata* metadata) const PURE; + + /* + * return true if all matches support ALPN, false otherwise. + */ + virtual bool allMatchesSupportAlpn() const PURE; }; using TransportSocketMatcherPtr = std::unique_ptr; diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 67410b41205d..18d7d384abd3 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -59,6 +59,7 @@ constexpr const char* runtime_features[] = { "envoy.reloadable_features.allow_response_for_timeout", "envoy.reloadable_features.conn_pool_delete_when_idle", "envoy.reloadable_features.correct_scheme_and_xfp", + "envoy.reloadable_features.correctly_validate_alpn", "envoy.reloadable_features.disable_tls_inspector_injection", "envoy.reloadable_features.fix_added_trailers", "envoy.reloadable_features.grpc_bridge_stats_disabled", diff --git a/source/common/upstream/transport_socket_match_impl.h b/source/common/upstream/transport_socket_match_impl.h index 83ffb41774c9..6d9f665c8b82 100644 --- a/source/common/upstream/transport_socket_match_impl.h +++ b/source/common/upstream/transport_socket_match_impl.h @@ -40,6 +40,18 @@ class TransportSocketMatcherImpl : public Logger::Loggable MatchData resolve(const envoy::config::core::v3::Metadata* metadata) const override; + bool allMatchesSupportAlpn() const override { + if (!default_match_.factory->supportsAlpn()) { + return false; + } + for (const auto& match : matches_) { + if (!match.factory->supportsAlpn()) { + return false; + } + } + return true; + } + protected: TransportSocketMatchStats generateStats(const std::string& prefix); Stats::Scope& stats_scope_; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 4a57f8d18b20..a0860d423518 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -1049,6 +1049,7 @@ ClusterImplBase::ClusterImplBase( auto socket_matcher = std::make_unique( cluster.transport_socket_matches(), factory_context, socket_factory, *stats_scope); + const bool matcher_supports_alpn = socket_matcher->allMatchesSupportAlpn(); auto& dispatcher = factory_context.mainThreadDispatcher(); info_ = std::shared_ptr( new ClusterInfoImpl(cluster, factory_context.clusterManager().bindConfig(), runtime, @@ -1060,11 +1061,18 @@ ClusterImplBase::ClusterImplBase( std::unique_ptr(self)); }); - if ((info_->features() & ClusterInfoImpl::Features::USE_ALPN) && - !raw_factory_pointer->supportsAlpn()) { - throw EnvoyException( - fmt::format("ALPN configured for cluster {} which has a non-ALPN transport socket: {}", - cluster.name(), cluster.DebugString())); + if ((info_->features() & ClusterInfoImpl::Features::USE_ALPN)) { + if (!raw_factory_pointer->supportsAlpn()) { + throw EnvoyException( + fmt::format("ALPN configured for cluster {} which has a non-ALPN transport socket: {}", + cluster.name(), cluster.DebugString())); + } + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.correctly_validate_alpn") && + !matcher_supports_alpn) { + throw EnvoyException(fmt::format( + "ALPN configured for cluster {} which has a non-ALPN transport socket matcher: {}", + cluster.name(), cluster.DebugString())); + } } if (info_->features() & ClusterInfoImpl::Features::HTTP3) { diff --git a/test/common/upstream/transport_socket_matcher_test.cc b/test/common/upstream/transport_socket_matcher_test.cc index 2bedc726d5e8..ac087888f7fb 100644 --- a/test/common/upstream/transport_socket_matcher_test.cc +++ b/test/common/upstream/transport_socket_matcher_test.cc @@ -31,11 +31,16 @@ class FakeTransportSocketFactory : public Network::TransportSocketFactory { public: MOCK_METHOD(bool, implementsSecureTransport, (), (const)); MOCK_METHOD(bool, usesProxyProtocolOptions, (), (const)); + MOCK_METHOD(bool, supportsAlpn, (), (const)); MOCK_METHOD(Network::TransportSocketPtr, createTransportSocket, (Network::TransportSocketOptionsConstSharedPtr), (const)); - FakeTransportSocketFactory(std::string id) : id_(std::move(id)) {} + FakeTransportSocketFactory(std::string id, bool alpn) : supports_alpn_(alpn), id_(std::move(id)) { + ON_CALL(*this, supportsAlpn).WillByDefault(Invoke([this]() { return supports_alpn_; })); + } std::string id() const { return id_; } + bool supports_alpn_; + private: const std::string id_; }; @@ -58,7 +63,7 @@ class FooTransportSocketFactory if (!node.id().empty()) { id = node.id(); } - return std::make_unique(id); + return std::make_unique>(id, supports_alpn_); } ProtobufTypes::MessagePtr createEmptyConfigProto() override { @@ -66,12 +71,14 @@ class FooTransportSocketFactory } std::string name() const override { return "foo"; } + bool supports_alpn_{}; }; class TransportSocketMatcherTest : public testing::Test { public: TransportSocketMatcherTest() - : registration_(factory_), mock_default_factory_(new FakeTransportSocketFactory("default")), + : registration_(factory_), + mock_default_factory_(new NiceMock("default", false)), stats_scope_(stats_store_.createScope("transport_socket_match.test")) {} void init(const std::vector& match_yaml) { @@ -116,6 +123,61 @@ name: "enableFooSocket" envoy::config::core::v3::Metadata metadata; validate(metadata, "default"); + + // Neither the defaults nor matcher support ALPN. + EXPECT_FALSE(matcher_->allMatchesSupportAlpn()); +} + +TEST_F(TransportSocketMatcherTest, AlpnSupport) { + mock_default_factory_.reset(new NiceMock("default", true)); + factory_.supports_alpn_ = true; + init({R"EOF( +name: "enableFooSocket" +match: + hasSidecar: "true" +transport_socket: + name: "foo" + typed_config: + "@type": type.googleapis.com/envoy.config.core.v3.Node + id: "abc" + )EOF"}); + + // Both default and matcher support ALPN + EXPECT_TRUE(matcher_->allMatchesSupportAlpn()); +} + +TEST_F(TransportSocketMatcherTest, NoDefaultAlpnSupport) { + factory_.supports_alpn_ = true; + init({R"EOF( +name: "enableFooSocket" +match: + hasSidecar: "true" +transport_socket: + name: "foo" + typed_config: + "@type": type.googleapis.com/envoy.config.core.v3.Node + id: "abc" + )EOF"}); + + // The default doesn't support ALPN, matchers do. + EXPECT_FALSE(matcher_->allMatchesSupportAlpn()); +} + +TEST_F(TransportSocketMatcherTest, NoMatcherAlpnSupport) { + mock_default_factory_.reset(new NiceMock("default", true)); + init({R"EOF( +name: "enableFooSocket" +match: + hasSidecar: "true" +transport_socket: + name: "foo" + typed_config: + "@type": type.googleapis.com/envoy.config.core.v3.Node + id: "abc" + )EOF"}); + + // The default doesn't support ALPN, matchers do. + EXPECT_FALSE(matcher_->allMatchesSupportAlpn()); } TEST_F(TransportSocketMatcherTest, BasicMatch) { diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index f413d4cfd8a3..970789578562 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -3801,6 +3801,157 @@ TEST_F(ClusterInfoImplTest, Http3BadConfig) { } #endif // ENVOY_ENABLE_QUIC +TEST_F(ClusterInfoImplTest, Http2Auto) { + const std::string yaml = TestEnvironment::substitute(R"EOF( + name: name + connect_timeout: 0.25s + type: STRICT_DNS + lb_policy: MAGLEV + load_assignment: + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: foo.bar.com + port_value: 443 + transport_socket: + name: envoy.transport_sockets.tls + typed_config: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext + 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" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + 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); + ASSERT_TRUE(cluster1->info()->idleTimeout().has_value()); + EXPECT_EQ(std::chrono::hours(1), cluster1->info()->idleTimeout().value()); + + const std::string auto_http2 = R"EOF( + typed_extension_protocol_options: + envoy.extensions.upstreams.http.v3.HttpProtocolOptions: + "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions + auto_config: + http2_protocol_options: {} + common_http_protocol_options: + idle_timeout: 1s + )EOF"; + + auto auto_h2 = makeCluster(yaml + auto_http2); + EXPECT_EQ(Http::Protocol::Http2, + auto_h2->info()->upstreamHttpProtocol({Http::Protocol::Http10})[0]); +} + +TEST_F(ClusterInfoImplTest, Http2AutoNoAlpn) { + const std::string yaml = TestEnvironment::substitute(R"EOF( + name: name + connect_timeout: 0.25s + type: STRICT_DNS + lb_policy: MAGLEV + load_assignment: + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: foo.bar.com + port_value: 443 + )EOF", + Network::Address::IpVersion::v4); + + auto cluster1 = makeCluster(yaml); + ASSERT_TRUE(cluster1->info()->idleTimeout().has_value()); + EXPECT_EQ(std::chrono::hours(1), cluster1->info()->idleTimeout().value()); + + const std::string auto_http2 = R"EOF( + typed_extension_protocol_options: + envoy.extensions.upstreams.http.v3.HttpProtocolOptions: + "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions + auto_config: + http2_protocol_options: {} + common_http_protocol_options: + idle_timeout: 1s + )EOF"; + + EXPECT_THROW_WITH_REGEX(makeCluster(yaml + auto_http2), EnvoyException, + ".*which has a non-ALPN transport socket:.*"); +} + +TEST_F(ClusterInfoImplTest, Http2AutoWithNonAlpnMatcher) { + const std::string yaml = TestEnvironment::substitute(R"EOF( + name: name + connect_timeout: 0.25s + type: STRICT_DNS + lb_policy: MAGLEV + load_assignment: + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: foo.bar.com + port_value: 443 + transport_socket_matches: + - match: + name: insecure-mode + transport_socket: + name: envoy.transport_sockets.raw_buffer + transport_socket: + name: envoy.transport_sockets.tls + typed_config: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext + 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" + validation_context: + trusted_ca: + filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/ca_cert.pem" + 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); + ASSERT_TRUE(cluster1->info()->idleTimeout().has_value()); + EXPECT_EQ(std::chrono::hours(1), cluster1->info()->idleTimeout().value()); + + const std::string auto_http2 = R"EOF( + typed_extension_protocol_options: + envoy.extensions.upstreams.http.v3.HttpProtocolOptions: + "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions + auto_config: + http2_protocol_options: {} + common_http_protocol_options: + idle_timeout: 1s + )EOF"; + + EXPECT_THROW_WITH_REGEX(makeCluster(yaml + auto_http2), EnvoyException, + ".*which has a non-ALPN transport socket matcher:.*"); +} + // Validate empty singleton for HostsPerLocalityImpl. TEST(HostsPerLocalityImpl, Empty) { EXPECT_FALSE(HostsPerLocalityImpl::empty()->hasLocalLocality()); diff --git a/test/mocks/upstream/transport_socket_match.h b/test/mocks/upstream/transport_socket_match.h index 55bacbde270c..5dd6b52758e8 100644 --- a/test/mocks/upstream/transport_socket_match.h +++ b/test/mocks/upstream/transport_socket_match.h @@ -19,6 +19,7 @@ class MockTransportSocketMatcher : public TransportSocketMatcher { ~MockTransportSocketMatcher() override; MOCK_METHOD(TransportSocketMatcher::MatchData, resolve, (const envoy::config::core::v3::Metadata*), (const)); + MOCK_METHOD(bool, allMatchesSupportAlpn, (), (const)); Network::TransportSocketFactoryPtr socket_factory_; Stats::TestUtil::TestStore stats_store_; From c677a1e9aeb03c87b93e86b2d5b1088dd6c10ecc Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 14 Dec 2021 14:09:58 -0500 Subject: [PATCH 2/3] oops indeed Signed-off-by: Alyssa Wilk --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index ee8dc294b17d..6d21427a6078 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -1,4 +1,4 @@ -g.21.0 (Pending) +1.21.0 (Pending) ================ Incompatible Behavior Changes From a49a4a8cadb29c625572ee92829ebcc378b83c9f Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 15 Dec 2021 09:12:38 -0500 Subject: [PATCH 3/3] tidy Signed-off-by: Alyssa Wilk --- test/common/upstream/transport_socket_matcher_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/common/upstream/transport_socket_matcher_test.cc b/test/common/upstream/transport_socket_matcher_test.cc index ac087888f7fb..4a3f8af0690c 100644 --- a/test/common/upstream/transport_socket_matcher_test.cc +++ b/test/common/upstream/transport_socket_matcher_test.cc @@ -1,3 +1,4 @@ +#include #include #include @@ -129,7 +130,7 @@ name: "enableFooSocket" } TEST_F(TransportSocketMatcherTest, AlpnSupport) { - mock_default_factory_.reset(new NiceMock("default", true)); + mock_default_factory_ = std::make_unique>("default", true); factory_.supports_alpn_ = true; init({R"EOF( name: "enableFooSocket" @@ -164,7 +165,7 @@ name: "enableFooSocket" } TEST_F(TransportSocketMatcherTest, NoMatcherAlpnSupport) { - mock_default_factory_.reset(new NiceMock("default", true)); + mock_default_factory_ = std::make_unique>("default", true); init({R"EOF( name: "enableFooSocket" match: