http: downstream connect support #10720

alyssawilk · 2020-04-09T13:42:35Z

Using the upgrade path for CONNECT requests to allow proxying CONNECT payload.
This results in functionally correct proxying of CONNECT requests e2e, though it remains hidden and should not be used yet, as there are WIP improvements to security not yet landed.
Various tweaks to HTTP/1 and HTTP/2 codecs to get this working e2e
Changing the 400 for unsupported connects to 403, which seems more accurate.

Risk Level: Medium (mostly config guarded)
Testing: new unit tests, e2e tests for CONNECT proxying.
Docs Changes: in #10623, need more plus config examples
Release Notes: no
Part of #1630 #1451

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-04-14T16:12:37Z

I believe the clang-tidy error is unrelated - PTAL?

mattklein123

This is awesome. Very excited to see this taking shape. Generally looks great with a few small comments and questions. Thank you!

/wait

source/common/http/conn_manager_impl.cc

mattklein123 · 2020-04-14T17:32:42Z

source/common/http/conn_manager_impl.cc

@@ -2004,6 +2014,12 @@ bool ConnectionManagerImpl::ActiveStream::createFilterChain() {
 }
 bool upgrade_rejected = false;
 auto upgrade = request_headers_ ? request_headers_->Upgrade() : nullptr;
+ // Treat CONNECT requests as a special upgrade case.
+ if (!upgrade && request_headers_ && request_headers_->Method() &&


Is it possible to have headers but not method?

Not unless a filter removes the method for some reason, but given it's constant time I mildly prefer erring on the side of caution. I can take it out (and or revert the utility it landed in) if you prefer.

I'm fine either way. I thought this was before filters run but it's not a big deal either way.

mattklein123 · 2020-04-14T17:33:21Z

source/common/http/conn_manager_impl.cc

@@ -2004,6 +2014,12 @@ bool ConnectionManagerImpl::ActiveStream::createFilterChain() {
 }
 bool upgrade_rejected = false;
 auto upgrade = request_headers_ ? request_headers_->Upgrade() : nullptr;


nit: can you make this a non-auto type? It's a little confusing to read through and see what type this is.

source/common/http/http1/codec_impl.cc

mattklein123 · 2020-04-14T17:37:52Z

source/common/http/http1/codec_impl.h

@@ -72,6 +72,7 @@ class StreamEncoderImpl : public virtual StreamEncoder,
 const Network::Address::InstanceConstSharedPtr& connectionLocalAddress() override;

 void isResponseToHeadRequest(bool value) { is_response_to_head_request_ = value; }
+ void isResponseToConnectRequest(bool value) { is_response_to_connect_request_ = value; }


nit: I would probably do s/is/set in the method name and change the head request one also but up to you.

mattklein123 · 2020-04-14T17:38:52Z

source/common/http/http2/codec_impl.cc

@@ -149,6 +149,10 @@ void ConnectionImpl::ClientStreamImpl::encodeHeaders(const RequestHeaderMap& hea
 upgrade_type_ = std::string(headers.Upgrade()->value().getStringView());
 Http::Utility::transformUpgradeRequestFromH1toH2(*modified_headers);
 buildHeaders(final_headers, *modified_headers);
+ } else if (headers.Method() && headers.Method()->value() == "CONNECT") {
+ modified_headers = createHeaderMap<RequestHeaderMapImpl>(headers);
+ modified_headers->setProtocol("bytestream");


nit: "bytestream" is constant in headers.h? If possible for some of the more arcane parts of this would it be also possible to comment with the RFC section to reference?

mattklein123 · 2020-04-14T17:42:49Z

test/common/http/http1/codec_impl_test.cc

+ NiceMock<MockRequestDecoder> decoder;
+ EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
+
+ EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1);


nit: Times(1) is redundant here and elsewhere.

mattklein123 · 2020-04-14T17:43:46Z

test/common/http/http1/codec_impl_test.cc

+ EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder));
+
+ EXPECT_CALL(decoder, decodeHeaders_(_, false)).Times(1);
+ Buffer::OwnedImpl buffer("CONNECT / HTTP/1.1\r\n\r\n");


I'm not very familiar with CONNECT, but according to https://tools.ietf.org/html/rfc7231#section-4.3.6 it says that CONNECT requires the authority form. Should we be allowing a request like this?

No, we shouldn't. It gets by because of the if(is_connect) in ServerConnectionImpl::handlePath.
Fixing that requires fixing the URL parser to handle connect requests, and the audit of all the Path() calls in the code base. Ok with this and the updated TODO or would you prefer to land this code and the Path() work together?

I was thinking we could at least fix the codec in this PR and then put in the synthetic \ before dispatch into the HCM? WDYT?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-04-15T14:02:33Z

Fair compromise :-)

@dio this means I'm going to have to use the is_connect field in the http parser library. Can you make sure #10599 will handle the new functionality? Happy to brainstorm options with you.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

Thanks this is great. Some small comments. The codec changes are kind of scary. Is it possible to run the codec fuzzer locally a little bit to see if there are any obvious errors? I'm not sure if we should seed a CONNECT corpus? cc @asraa

/wait

mattklein123 · 2020-04-15T16:37:15Z

source/common/http/conn_manager_impl.cc

@@ -760,6 +760,12 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
 connection_manager_.read_callbacks_->connection().dispatcher());
 request_headers_ = std::move(headers);

+ // TODO(alyssawilk) remove this synthetic path in a follow-up PR, including
+ // auditing of empty path headers.
+ if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path()) {


Isn't path guaranteed by the RFC to be empty? Should we just universally set it and ASSERT that it's empty?

I believe it's allowed for HTTP/2
https://tools.ietf.org/html/draft-kinnear-httpbis-http2-transport-02

Can you comment about the h2 thing?

mattklein123 · 2020-04-15T16:37:38Z

source/common/http/conn_manager_impl.cc

- if (upgrade_rejected) {
- // Do not allow upgrades if the route does not support it.
+ if (hasCachedRoute()) {
+ if (upgrade_rejected) { // Do not allow upgrades if the route does not support it.


nit: move comment to next line.

mattklein123 · 2020-04-15T16:39:45Z

source/common/http/http1/codec_impl.cc

+ if (is_connect && !host) {
+ throw CodecClientException("Host must be specified for CONNECT requests");
+ } else if (!method || (!path && !is_connect)) {


This came up in @asraa's exception removal PR, but these exceptions have no purpose. I don't recall the history of why they are here but they are programming errors and will crash the server anyway, so perhaps just switch them to RELEASE_ASSERT or ASSERT now to avoid @asraa more hassle later? Up to you.

What did we decide here? Do you want to just ASSERT or RELEASE_ASSERT the new case vs. throw an exception?

mattklein123 · 2020-04-15T16:44:20Z

source/common/http/utility.h

@@ -101,7 +101,7 @@ namespace Utility {
 */
 class Url {
 public:
- bool initialize(absl::string_view absolute_url);
+ bool initialize(absl::string_view absolute_url, bool is_connect_request = false);


nit: I would probably have this be a non-default param given how much it changes the behavior, and possibly even an enum for clarity, but up to you.

mattklein123 · 2020-04-15T16:46:14Z

test/common/http/http1/codec_impl_test.cc

+ // Connect with body is technically illegal, but Envoy does not inspect the
+ // body to see if there is a non-zero byte chunk. It will instead pass it


Should we eventually block this? TODO? WDYT?

We'd have to basically peek at connect payload and reject if it's not an empty chunk. I'm happy to TODO if you think it's worth it

I'm fine either way. I don't know enough about the RFC here to have an opinion. cc @PiotrSikora

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

dio · 2020-04-16T07:08:07Z

@alyssawilk sure, so is_connect > 0 means to have the URL to be strictly checked. If there is no port in the URL (since CONNECT can only contain "hostname:port"), the schema will cause the http_parser_parse_url method to fail. Is that correct?

alyssawilk · 2020-04-16T13:40:01Z

Yeah, I think roughly we need a mode that validates we have authority:port. We've already got a utility for valid authority so shouldn't be too hard. I can also help out here if it'd be useful

…

On Thu, Apr 16, 2020 at 3:08 AM Dhi Aurrahman ***@***.***> wrote: @alyssawilk <https://github.com/alyssawilk> sure, so is_connect means to have the URL to be strictly checked. If there is no port in the URL (since CONNECT can only contain "hostname:port" <https://github.com/nodejs/http-parser/blob/7af127d3ac497bb036d3ecb4fb24bb5ee7ff4b5b/http_parser.c#L2503>), the schema will cause the http_parser_parse_url method to fail. Is that correct? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10720 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AELALPNOKXZMIJJ6V4AZKALRM2VGNANCNFSM4MEXT3LQ> .

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

Thanks, awesome, just pinging a few comments from before. Thank you!

/wait

mattklein123 · 2020-04-16T20:41:52Z

source/common/http/conn_manager_impl.cc

@@ -760,6 +760,12 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
 connection_manager_.read_callbacks_->connection().dispatcher());
 request_headers_ = std::move(headers);

+ // TODO(alyssawilk) remove this synthetic path in a follow-up PR, including
+ // auditing of empty path headers.
+ if (HeaderUtility::isConnect(*request_headers_) && !request_headers_->Path()) {


Can you comment about the h2 thing?

mattklein123 · 2020-04-16T20:42:02Z

source/common/http/conn_manager_impl.cc

- if (upgrade_rejected) {
- // Do not allow upgrades if the route does not support it.
+ if (hasCachedRoute()) {
+ if (upgrade_rejected) { // Do not allow upgrades if the route does not support it.


mattklein123 · 2020-04-16T20:42:44Z

source/common/http/http1/codec_impl.cc

+ if (is_connect && !host) {
+ throw CodecClientException("Host must be specified for CONNECT requests");
+ } else if (!method || (!path && !is_connect)) {


What did we decide here? Do you want to just ASSERT or RELEASE_ASSERT the new case vs. throw an exception?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-04-20T15:12:41Z

CI is happy and I think I addressed comments - PTAL!

mattklein123

Nice!

This should result in all Path() calls not altered in #10720 being safe for path-less CONNECT. The major change for this PR is that requests without a path will not be considered gRPC requests. They're still currently rejected at the HCM, but when they are allowed through they will simply not be gRPC rather than causing crashes. Risk Level: medium (L7 code refactor) Testing: new unit tests Docs Changes: n/a Release Notes: n/a Part of #1630 #1451 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: pengg <pengg@google.com>

…oxy#10851) This should result in all Path() calls not altered in envoyproxy#10720 being safe for path-less CONNECT. The major change for this PR is that requests without a path will not be considered gRPC requests. They're still currently rejected at the HCM, but when they are allowed through they will simply not be gRPC rather than causing crashes. Risk Level: medium (L7 code refactor) Testing: new unit tests Docs Changes: n/a Release Notes: n/a Part of envoyproxy#1630 envoyproxy#1451 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: pengg <pengg@google.com>

alyssawilk assigned lambdai and mattklein123 Apr 9, 2020

http: downstream connect support

dd36508

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the downstream_connect branch from b09df53 to dd36508 Compare April 9, 2020 13:43

alyssawilk added 3 commits April 9, 2020 10:45

Fixing spelling on my questions to Matt

0c06010

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'master' into downstream_connect

565d744

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

altering TODO

5b1a13a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 requested changes Apr 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 14, 2020

reviewer comments

b60e803

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Apr 14, 2020

mattklein123 added the waiting label Apr 15, 2020

alyssawilk added 2 commits April 15, 2020 10:04

Merge branch 'master' into downstream_connect

739ee00

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

fixed never-working connect parsing

74c9325

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Apr 15, 2020

cleanup + enabling a now working e2e test

bdba8ba

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 requested changes Apr 15, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 15, 2020

reviewer comments

2674cf6

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Apr 15, 2020

mattklein123 added the waiting label Apr 15, 2020

fuzzing requests and responses

fa76767

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Apr 15, 2020

mattklein123 added the waiting label Apr 15, 2020

repokitteh-read-only bot removed the waiting label Apr 16, 2020

preexisting clang_tidy :-(

e0725a1

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the downstream_connect branch from 8a80f90 to e0725a1 Compare April 16, 2020 14:25

fixing the fixes

6a3ca23

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the downstream_connect branch from b11da95 to 6a3ca23 Compare April 16, 2020 18:42

mattklein123 requested changes Apr 16, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 16, 2020

reviewer comments

f391f82

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Apr 16, 2020

spaces

840406e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk mentioned this pull request Apr 20, 2020

http: auditing Path() calls for safety with Pathless CONNECT #10851

Merged

mattklein123 approved these changes Apr 20, 2020

View reviewed changes

mattklein123 merged commit b3fe5f7 into envoyproxy:master Apr 20, 2020

alyssawilk mentioned this pull request Apr 21, 2020

router: adding CONNECT support to upstreams (not prod-ready) #10623

Merged

rgs1 mentioned this pull request Apr 22, 2020

Crash regression from #10362 #10901

Closed

penguingao pushed a commit to penguingao/envoy that referenced this pull request Apr 22, 2020

http: downstream connect support (envoyproxy#10720)

3494249

Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: pengg <pengg@google.com>

alyssawilk deleted the downstream_connect branch August 27, 2020 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: downstream connect support #10720

http: downstream connect support #10720

alyssawilk commented Apr 9, 2020 •

edited

Loading

alyssawilk commented Apr 14, 2020

mattklein123 left a comment

mattklein123 Apr 14, 2020

alyssawilk Apr 14, 2020

mattklein123 Apr 14, 2020

mattklein123 Apr 14, 2020

mattklein123 Apr 14, 2020

mattklein123 Apr 14, 2020

mattklein123 Apr 14, 2020

mattklein123 Apr 14, 2020

alyssawilk Apr 14, 2020

mattklein123 Apr 14, 2020

alyssawilk commented Apr 15, 2020

mattklein123 left a comment

mattklein123 Apr 15, 2020

alyssawilk Apr 15, 2020

mattklein123 Apr 16, 2020

mattklein123 Apr 15, 2020

mattklein123 Apr 16, 2020

mattklein123 Apr 15, 2020

mattklein123 Apr 16, 2020

mattklein123 Apr 15, 2020

mattklein123 Apr 15, 2020

alyssawilk Apr 15, 2020

mattklein123 Apr 15, 2020

dio commented Apr 16, 2020 •

edited

Loading

alyssawilk commented Apr 16, 2020 via email

mattklein123 left a comment

mattklein123 Apr 16, 2020

mattklein123 Apr 16, 2020

mattklein123 Apr 16, 2020

alyssawilk commented Apr 20, 2020

mattklein123 left a comment

		// Connect with body is technically illegal, but Envoy does not inspect the
		// body to see if there is a non-zero byte chunk. It will instead pass it

http: downstream connect support #10720

http: downstream connect support #10720

Conversation

alyssawilk commented Apr 9, 2020 • edited Loading

alyssawilk commented Apr 14, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented Apr 15, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dio commented Apr 16, 2020 • edited Loading

alyssawilk commented Apr 16, 2020 via email

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented Apr 20, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

alyssawilk commented Apr 9, 2020 •

edited

Loading

dio commented Apr 16, 2020 •

edited

Loading