Skip to content
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

Jump to bottom

http: fixing up CONNECT to be RFC compliant #21440

Merged
merged 6 commits into from
Jun 8, 2022
Merged

http: fixing up CONNECT to be RFC compliant #21440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f327436
http: fixing up CONNECT to be RFC compliant
alyssawilk May 25, 2022
f140470
docs fix
alyssawilk May 25, 2022
1022eea
test fixup
alyssawilk May 26, 2022
a87a0f9
Merge branch 'main' into extended
alyssawilk Jun 6, 2022
bb73c07
comments
alyssawilk Jun 6, 2022
0709f9e
comment
alyssawilk Jun 8, 2022
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
5 changes: 5 additions & 0 deletions changelogs/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ minor_behavior_changes:
``envoy.reloadable_features.lua_respond_with_send_local_reply`` to false.

bug_fixes:
- area: http
change: |
Fixed HTTP/2 CONNECT to be RFC compliant, rather than following the abandoned extended connect draft.
This behavioral change can be reverted by setting runtime guard ``envoy.reloadable_features.use_rfc_connect`` to false.

- area: runtime
change: |
Fixed a bug where was ``envoy.restart_features.no_runtime_singleton`` was inverted.
Expand Down
12 changes: 9 additions & 3 deletions source/common/http/http2/codec_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,15 @@ Status ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& h
// configurable if necessary.
// https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02
modified_headers = createHeaderMap<RequestHeaderMapImpl>(headers);
modified_headers->setProtocol(Headers::get().ProtocolValues.Bytestream);
if (!headers.Path()) {
modified_headers->setPath("/");
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_rfc_connect")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider moving the comment from line 296 down into the if blocks and editing it accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

modified_headers->removeScheme();
modified_headers->removePath();
modified_headers->removeProtocol();
} else {
modified_headers->setProtocol(Headers::get().ProtocolValues.Bytestream);
if (!headers.Path()) {
modified_headers->setPath("/");
}
}
encodeHeadersBase(*modified_headers, end_stream);
} else {
Expand Down
19 changes: 11 additions & 8 deletions source/common/quic/envoy_quic_client_stream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,17 @@ Http::Status EnvoyQuicClientStream::encodeHeaders(const Http::RequestHeaderMap&
auto spdy_headers = envoyHeadersToSpdyHeaderBlock(headers);
if (headers.Method()) {
if (headers.Method()->value() == "CONNECT") {
// It is a bytestream connect and should have :path and :protocol set accordingly
// As HTTP/1.1 does not require a path for CONNECT, we may have to add one
// if shifting codecs. For now, default to "/" - this can be made
// configurable if necessary.
// https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02
spdy_headers[":protocol"] = Http::Headers::get().ProtocolValues.Bytestream;
if (!headers.Path()) {
spdy_headers[":path"] = "/";
if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_rfc_connect")) {
spdy_headers.erase(":scheme");
spdy_headers.erase(":path");
spdy_headers.erase(":protocol");
} else {
// Legacy support for abandoned
// https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02
spdy_headers[":protocol"] = Http::Headers::get().ProtocolValues.Bytestream;
if (!headers.Path()) {
spdy_headers[":path"] = "/";
}
}
} else if (headers.Method()->value() == "HEAD") {
sent_head_request_ = true;
Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ RUNTIME_GUARD(envoy_reloadable_features_top_level_ecds_stats);
RUNTIME_GUARD(envoy_reloadable_features_udp_listener_updates_filter_chain_in_place);
RUNTIME_GUARD(envoy_reloadable_features_update_expected_rq_timeout_on_retry);
RUNTIME_GUARD(envoy_reloadable_features_update_grpc_response_error_tag);
RUNTIME_GUARD(envoy_reloadable_features_use_rfc_connect);
RUNTIME_GUARD(envoy_reloadable_features_validate_connect);
RUNTIME_GUARD(envoy_restart_features_explicit_wildcard_resource);
RUNTIME_GUARD(envoy_restart_features_remove_runtime_singleton);
Expand Down
9 changes: 6 additions & 3 deletions source/common/tcp_proxy/upstream.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,11 +278,14 @@ void Http2Upstream::setRequestEncoder(Http::RequestEncoder& request_encoder, boo
auto headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({
{Http::Headers::get().Method, config_.usePost() ? "POST" : "CONNECT"},
{Http::Headers::get().Host, config_.hostname()},
{Http::Headers::get().Path, "/"},
{Http::Headers::get().Scheme, scheme},
});

if (!config_.usePost()) {
if (config_.usePost()) {
headers->addReference(Http::Headers::get().Path, "/");
headers->addReference(Http::Headers::get().Scheme, scheme);
} else if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.use_rfc_connect")) {
headers->addReference(Http::Headers::get().Path, "/");
headers->addReference(Http::Headers::get().Scheme, scheme);
headers->addReference(Http::Headers::get().Protocol,
Http::Headers::get().ProtocolValues.Bytestream);
}
Expand Down
28 changes: 27 additions & 1 deletion test/common/http/http2/codec_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3613,7 +3613,8 @@ TEST_P(Http2CodecImplTest, ResetStreamCausesOutboundFlood) {
}

// CONNECT without upgrade type gets tagged with "bytestream"
TEST_P(Http2CodecImplTest, ConnectTest) {
TEST_P(Http2CodecImplTest, ConnectTestOld) {
scoped_runtime_.mergeValues({{"envoy.reloadable_features.use_rfc_connect", "false"}});
client_http2_options_.set_allow_connect(true);
server_http2_options_.set_allow_connect(true);
initialize();
Expand All @@ -3639,6 +3640,31 @@ TEST_P(Http2CodecImplTest, ConnectTest) {
driveToCompletion();
}

TEST_P(Http2CodecImplTest, ConnectTest) {
client_http2_options_.set_allow_connect(true);
server_http2_options_.set_allow_connect(true);
initialize();
MockStreamCallbacks callbacks;
request_encoder_->getStream().addCallbacks(callbacks);

TestRequestHeaderMapImpl request_headers;
HttpTestUtility::addDefaultHeaders(request_headers);
request_headers.setReferenceKey(Headers::get().Method, Http::Headers::get().MethodValues.Connect);
request_headers.setReferenceKey(Headers::get().Protocol, "bytestream");
TestRequestHeaderMapImpl expected_headers;
expected_headers.setReferenceKey(Headers::get().Host, "host");
expected_headers.setReferenceKey(Headers::get().Method,
Http::Headers::get().MethodValues.Connect);
EXPECT_CALL(request_decoder_, decodeHeaders_(HeaderMapEqual(&expected_headers), false));
EXPECT_TRUE(request_encoder_->encodeHeaders(request_headers, false).ok());
driveToCompletion();

EXPECT_CALL(callbacks, onResetStream(StreamResetReason::ConnectError, _));
EXPECT_CALL(server_stream_callbacks_, onResetStream(StreamResetReason::ConnectError, _));
response_encoder_->getStream().resetStream(StreamResetReason::ConnectError);
driveToCompletion();
}

TEST_P(Http2CodecImplTest, ShouldWaitForDeferredBodyToProcessBeforeProcessingTrailers) {
// We must initialize before dtor, otherwise we'll touch uninitialized
// members in dtor.
Expand Down
1 change: 1 addition & 0 deletions test/common/tcp_proxy/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,5 +81,6 @@ envoy_cc_test(
"//test/mocks/http:http_mocks",
"//test/mocks/tcp:tcp_mocks",
"//test/mocks/upstream:cluster_manager_mocks",
"//test/test_common:test_runtime_lib",
],
)
50 changes: 17 additions & 33 deletions test/common/tcp_proxy/upstream_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "test/mocks/tcp/mocks.h"
#include "test/test_common/environment.h"
#include "test/test_common/network_utility.h"
#include "test/test_common/test_runtime.h"

#include "gmock/gmock.h"
#include "gtest/gtest.h"
Expand Down Expand Up @@ -223,7 +224,10 @@ template <typename T> class HttpUpstreamRequestEncoderTest : public testing::Tes

TYPED_TEST_SUITE(HttpUpstreamRequestEncoderTest, Implementations);

TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoder) {
TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderOld) {
TestScopedRuntime scoped_runtime;
scoped_runtime.mergeValues({{"envoy.reloadable_features.use_rfc_connect", "false"}});

this->setupUpstream();
std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers;
expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({
Expand All @@ -243,6 +247,18 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoder) {
this->upstream_->setRequestEncoder(this->encoder_, false);
}

TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoder) {
this->setupUpstream();
std::unique_ptr<Http::RequestHeaderMapImpl> expected_headers;
expected_headers = Http::createHeaderMap<Http::RequestHeaderMapImpl>({
{Http::Headers::get().Method, "CONNECT"},
{Http::Headers::get().Host, this->config_->hostname()},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great, thanks for the fix!

One possibly concern is that in the current code we are using a header to replace Host, which allows dynamic metadata (

TunnelingConfig: &tcp.TcpProxy_TunnelingConfig{
    Hostname: "ignored",
    HeadersToAdd: []*core.HeaderValueOption{
        {Header: &core.HeaderValue{Key: "x-destination", Value: "%DYNAMIC_METADATA([\"tunnel\", \"address\"])%"}},
    },
},

).

Now we would want to use Hostname to follow the spec, but that is only a static string.

Seems like something that could be expanded in a followup though @lambdai

Copy link
Contributor

@kyessenov kyessenov May 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@jewertow This is an instance when we need metadata for hostname. Not sure if you still want to complete #21067, but we'll probably add metadata support.

});

EXPECT_CALL(this->encoder_, encodeHeaders(HeaderMapEqualRef(expected_headers.get()), false));
this->upstream_->setRequestEncoder(this->encoder_, false);
}

TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderUsePost) {
this->config_message_.set_use_post(true);
this->setupUpstream();
Expand Down Expand Up @@ -287,14 +303,6 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeaders) {
{Http::Headers::get().Host, this->config_->hostname()},
});

if (this->is_http2_) {
expected_headers->setReferenceKey(Http::Headers::get().Path, "/");
expected_headers->setReferenceKey(Http::Headers::get().Scheme,
Http::Headers::get().SchemeValues.Http);
expected_headers->setReferenceKey(Http::Headers::get().Protocol,
Http::Headers::get().ProtocolValues.Bytestream);
}

expected_headers->setCopy(Http::LowerCaseString("header0"), "value0");
expected_headers->addCopy(Http::LowerCaseString("header1"), "value1");
expected_headers->addCopy(Http::LowerCaseString("header1"), "value2");
Expand Down Expand Up @@ -323,25 +331,9 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, ConfigReuse) {
{Http::Headers::get().Host, this->config_->hostname()},
});

if (this->is_http2_) {
expected_headers->setReferenceKey(Http::Headers::get().Path, "/");
expected_headers->setReferenceKey(Http::Headers::get().Scheme,
Http::Headers::get().SchemeValues.Http);
expected_headers->setReferenceKey(Http::Headers::get().Protocol,
Http::Headers::get().ProtocolValues.Bytestream);
}

expected_headers->setCopy(Http::LowerCaseString("key"), "value1");
expected_headers->addCopy(Http::LowerCaseString("key"), "value2");

if (this->is_http2_) {
expected_headers->setReferenceKey(Http::Headers::get().Path, "/");
expected_headers->setReferenceKey(Http::Headers::get().Scheme,
Http::Headers::get().SchemeValues.Http);
expected_headers->setReferenceKey(Http::Headers::get().Protocol,
Http::Headers::get().ProtocolValues.Bytestream);
}

expected_headers->setCopy(Http::LowerCaseString("key"), "value1");
expected_headers->addCopy(Http::LowerCaseString("key"), "value2");

Expand Down Expand Up @@ -382,14 +374,6 @@ TYPED_TEST(HttpUpstreamRequestEncoderTest, RequestEncoderHeadersWithDownstreamIn
{Http::Headers::get().Host, this->config_->hostname()},
});

if (this->is_http2_) {
expected_headers->setReferenceKey(Http::Headers::get().Path, "/");
expected_headers->setReferenceKey(Http::Headers::get().Scheme,
Http::Headers::get().SchemeValues.Http);
expected_headers->setReferenceKey(Http::Headers::get().Protocol,
Http::Headers::get().ProtocolValues.Bytestream);
}

expected_headers->setCopy(Http::LowerCaseString("header0"), "value0");
expected_headers->addCopy(Http::LowerCaseString("downstream_local_port"), "80");
auto ip_versions = TestEnvironment::getIpVersionsForTest();
Expand Down
7 changes: 1 addition & 6 deletions test/extensions/filters/http/compressor/compressor_integration_tests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,12 +268,7 @@ TEST_P(CompressorProxyingConnectIntegrationTest, ProxyConnect) {
RELEASE_ASSERT(result, result.message());
ASSERT_TRUE(upstream_request_->waitForHeadersComplete());
EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Method)[0]->value(), "CONNECT");
if (upstreamProtocol() == Http::CodecType::HTTP1) {
EXPECT_TRUE(upstream_request_->headers().get(Http::Headers::get().Protocol).empty());
} else {
EXPECT_EQ(upstream_request_->headers().get(Http::Headers::get().Protocol)[0]->value(),
"bytestream");
}
EXPECT_TRUE(upstream_request_->headers().get(Http::Headers::get().Protocol).empty());

// Send response headers
upstream_request_->encodeHeaders(default_response_headers_, false);
Expand Down
15 changes: 5 additions & 10 deletions test/extensions/filters/http/jwt_authn/filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,16 +303,11 @@ TEST_P(LocalJwksIntegrationTest, ConnectRequestWithRegExMatch) {
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());
}
// Because CONNECT requests do not include a path, they will fail
// to find a route match and return a 404.
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is a change in behavior that is visible to this filter. Presumably that's ok (and expected). Is there any need to call this out in release notes, or anything? Probably not, since they already mention the flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that's my thought

ASSERT_TRUE(response->waitForEndStream());
ASSERT_TRUE(response->complete());
EXPECT_EQ("404", response->headers().getStatusValue());
}

// The test case with a fake upstream for remote Jwks server.
Expand Down
2 changes: 1 addition & 1 deletion test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,7 +1397,7 @@ envoy_cc_test(
data = [
"//test/config/integration/certs",
],
shard_count = 3,
shard_count = 7,
deps = [
":http_integration_lib",
":http_protocol_integration_lib",
Expand Down
19 changes: 8 additions & 11 deletions test/integration/protocol_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2733,19 +2733,15 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectIsBlocked) {
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());
EXPECT_EQ("404", response->headers().getStatusValue());
EXPECT_TRUE(response->complete());
} else {
ASSERT_TRUE(response->waitForReset());
ASSERT_TRUE(codec_client_->waitForDisconnect());
}
// Because CONNECT requests do not include a path, they will fail
// to find a route match and return a 404.
ASSERT_TRUE(response->waitForEndStream());
EXPECT_EQ("404", response->headers().getStatusValue());
EXPECT_TRUE(response->complete());
}

TEST_P(DownstreamProtocolIntegrationTest, ExtendedConnectIsBlocked) {
config_helper_.addRuntimeOverride("envoy.reloadable_features.use_rfc_connect", "false");
if (downstreamProtocol() == Http::CodecType::HTTP1) {
return;
}
Expand Down Expand Up @@ -2785,7 +2781,8 @@ TEST_P(DownstreamProtocolIntegrationTest, ConnectStreamRejection) {
auto response = codec_client_->makeHeaderOnlyRequest(
Http::TestRequestHeaderMapImpl{{":method", "CONNECT"}, {":authority", "sni.lyft.com"}});

ASSERT_TRUE(response->waitForReset());
ASSERT_TRUE(response->waitForEndStream());
EXPECT_EQ("404", response->headers().getStatusValue());
EXPECT_FALSE(codec_client_->disconnected());
}

Expand Down
Loading