Skip to content

http3: validating codec #17452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alyssawilk merged 5 commits into from
Jul 28, 2021
Merged

http3: validating codec #17452

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0b16c30
http3: validating codec
alyssawilk Jul 22, 2021
ce3a6ed
comments
alyssawilk Jul 26, 2021
f6a603a
moving away from string compares
alyssawilk Jul 27, 2021
6f082a7
reinstating HCM checks
alyssawilk Jul 27, 2021
fa96d79
fix
alyssawilk Jul 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions source/server/listener_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,16 @@ Network::DrainableFilterChainSharedPtr ListenerFilterChainFactoryBuilder::buildF
"{}. \nUse QuicDownstreamTransport instead.",
transport_socket.DebugString()));
}
const std::string config_str =
filter_chain.filters_size() == 0 ? "" : filter_chain.filters(0).DebugString();

@mattklein123 mattklein123 Jul 27, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used anymore?

const std::string hcm_str =
"envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager";
if (is_quic && (filter_chain.filters().size() != 1 || !absl::StrContains(config_str, hcm_str) ||
!absl::StrContains(config_str, "codec_type: HTTP3"))) {

@danzh2010 danzh2010 Jul 23, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StrContains() is inefficient and this doesn't prevent other codec types from being misconfigured to HTTP3.
I have some thoughts about validating hcm and codec_type here:
We can compare filter_chain.filters(0).typed_config().type_url() here with "type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager" to validate HCM is the only network filter configured.

As to codec_type, the caller of HttpConnectionManagerConfig::createCodec() -- Http::ConnectionManagerImpl-- actually knows if the connection is quic or not. Can we modify createCodec() interface to take in a bool and always create a Http3 codec if it's true? If the pass-in value isn't consistent with configured codec type, i.e. createCodec(/is_quic/true) with codec_type_ != HTTP3 or createCodec(/is_quic/false) with codec_type_ == HTTP3, create whatever makes the most sense and warn about that.

@alyssawilk alyssawilk Jul 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switching to hcm type url makes sense.
I'm not worried about an expensive string compare for config load - it's infrequent and already expensive. I don't love doing StrContains because it's not exact, but I think it's better than adding a hard-dep from listener to HCM (will let them maintainer reviewer weigh in on that). I think ignoring configured codec type would not be Ok - it'd better to reject invalid config than accept, do something unexpeced and runtime-warn about it.

@mattklein123 mattklein123 Jul 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love doing the unpack twice, but why not unpack the HCM config into a real object, and then do the check in code? That would be a lot less hacky than doing str contains on a debug string?

Another option if we don't want to unpack twice. What about doing this check in the HCM config load itself? We could probably figure out a way to get back to the listener config and do the check there?

/wait-any

throw EnvoyException(fmt::format(
"error building network filter chain for quic listener: requires exactly one http_"
"connection_manager filter with an HTTP/3 codec."));
}
#else
// When QUIC is compiled out it should not be possible to configure either the QUIC transport
// socket or the QUIC listener and get to this point.
Expand Down
2 changes: 2 additions & 0 deletions test/server/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,8 @@ envoy_cc_test(
deps = [
":listener_manager_impl_test_lib",
":utility_lib",
"//source/extensions/filters/http/router:config",
"//source/extensions/request_id/uuid:config",
"//source/extensions/transport_sockets/raw_buffer:config",
"//source/extensions/transport_sockets/tls:config",
"//test/test_common:threadsafe_singleton_injector_lib",
Expand Down
65 changes: 64 additions & 1 deletion test/server/listener_manager_impl_quic_only_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,16 @@ TEST_F(ListenerManagerImplQuicOnlyTest, QuicListenerFactoryAndSslContext) {
filter_chains:
- filter_chain_match:
transport_protocol: "quic"
filters: []
filters:
- name: envoy.filters.network.http_connection_manager
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
codec_type: HTTP3
stat_prefix: hcm
route_config:
name: local_route
http_filters:
- name: envoy.filters.http.router
transport_socket:
name: envoy.transport_sockets.quic
typed_config:
Expand Down Expand Up @@ -171,6 +180,60 @@ TEST_F(ListenerManagerImplQuicOnlyTest, QuicListenerFactoryWithWrongTransportSoc
#endif
}

TEST_F(ListenerManagerImplQuicOnlyTest, QuicListenerFactoryWithWrongCodec) {
const std::string yaml = TestEnvironment::substitute(R"EOF(
address:
socket_address:
address: 127.0.0.1
protocol: UDP
port_value: 1234
filter_chains:
- filter_chain_match:
transport_protocol: "quic"
filters:
- name: envoy.filters.network.http_connection_manager
typed_config:
"@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
codec_type: HTTP2
stat_prefix: hcm
route_config:
name: local_route
http_filters:
- name: envoy.filters.http.router
transport_socket:
name: envoy.transport_sockets.quic
typed_config:
"@type": type.googleapis.com/envoy.extensions.transport_sockets.quic.v3.QuicDownstreamTransport
downstream_tls_context:
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_subject_alt_names:
- exact: localhost
- exact: 127.0.0.1
udp_listener_config:
quic_options: {}
)EOF",
Network::Address::IpVersion::v4);

envoy::config::listener::v3::Listener listener_proto = parseListenerFromV3Yaml(yaml);

#if defined(ENVOY_ENABLE_QUIC)
EXPECT_THROW_WITH_REGEX(manager_->addOrUpdateListener(listener_proto, "", true), EnvoyException,
"error building network filter chain for quic listener: requires exactly "
"one http_connection_manager filter with an HTTP/3 codec.");
#else
EXPECT_THROW_WITH_REGEX(manager_->addOrUpdateListener(listener_proto, "", true), EnvoyException,
"QUIC is configured but not enabled in the build.");
#endif
}

} // namespace
} // namespace Server
} // namespace Envoy