[WIP] new listener filter for terminating HTTP/1 CONNECT(#19077) by liverbirdkte · Pull Request #19346 · envoyproxy/envoy

liverbirdkte · 2021-12-23T10:38:04Z

Add a new listener filter to handle CONNECT request, only HTTP/1.x is supported.

inspect if incoming request is HTTP CONNECT
drain CONNECT request data out of queue
terminate HTTP CONNECT by writing a response to the client
set server names using request url in CONNECT request

It seems the behavior of draining CONNECT data and writing response may have an impact on other listener filters in the loop. Is there any way to exit the listener filter iteration? Or I should put this listener filter as the last one in the config.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2021-12-23T10:38:07Z

Hi @liverbirdkte, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #19346 was opened by liverbirdkte.

see: more, trace.

repokitteh-read-only · 2021-12-23T10:38:11Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #19346 was opened by liverbirdkte.

see: more, trace.

rojkov

Nice! I added a few comments.

Also this PR needs docs and tests.

rojkov · 2021-12-23T11:26:05Z

Per https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

Suggested change

static absl::string_view response_body = " 200 Connection Established\r\n\r\n";

static constexpr absl::string_view response_body = " 200 Connection Established\r\n\r\n";

And instead of static we prefer anonymous namespaces.

got it, will use anonymous namespace instead.

rojkov · 2021-12-23T11:37:51Z

The switch above is meant to be fully exhaustive. We'd better detect a missing case in it if we extend ParseState. Better replace NOT_REACHED_GCOVR_EXCL_LINE with ENVOY_BUG().

Do you mean adding a default for switch to handle missing cases or to extend ParseState by adding a new status？

No need to add default as some compilers can generate a warning at compile time about non-exhaustive switch'es.

If ParseState is ever extended with a new value then this switch will become non-exhaustive. So, it'll be possible to reach NOT_REACHED_GCOVR_EXCL_LINE. And that would be a bug most probably. Unfortunately reaching NOT_REACHED_GCOVR_EXCL_LINE doesn't trigger a bug nor prints a warning. Thus it's better to replace it with ENVOY_BUG().

rojkov · 2021-12-23T12:28:11Z

nit: no need to instantiate a string_view here.

rojkov · 2021-12-27T08:32:11Z

/wait

adisuissa

Thanks for working on this, left an API comment.

adisuissa · 2021-12-28T15:43:42Z

I don't think such a type exists.
If there is no v2 predecessor, you should remove the annotation.

sure, will remove this.

liverbirdkte · 2022-01-04T01:42:54Z

Sorry for the late response, thanks for the comments, I'll submit a new version soon. The patch is for issue #19077.

liverbirdkte · 2022-01-05T08:23:49Z

start working on tests and docs, will update when code is ready

rojkov · 2022-01-05T13:13:07Z

/wait

liverbirdkte · 2022-01-18T11:38:32Z

Add unittest, looking at how to write fuzz tests..

Add a new listener filter to handle CONNECT request, only HTTP/1.x is supported. 1. inspect if incoming request is HTTP CONNECT 2. drain CONNECT request queue 3. terminate HTTP CONNECT by writing a response to the client 4. set server names using request url in CONNECT request Signed-off-by: leizhang <lei.a.zhang@intel.com>

Signed-off-by: leizhang <lei.a.zhang@intel.com>

repokitteh-read-only · 2022-01-20T09:02:36Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).

🐱

Caused by: #19346 was synchronize by liverbirdkte.

see: more, trace.

rojkov · 2022-01-20T09:13:34Z

Please never use git force-push. If the PR is not ready for review you can turn it into a Draft.

liverbirdkte · 2022-01-20T09:24:03Z

OK, I was fixing some CI failures, saw the DOC issue, so I added sign-off and force push as the instruction on the page told me to.

Also didn't mean to modify unrelated jwt_authn proto, but git hook format checker won't get passed unless I apply these changes.

liverbirdkte · 2022-01-20T09:29:05Z

Thanks, I will convert it back when it's ready.

Signed-off-by: leizhang <lei.a.zhang@intel.com>

adisuissa

/lgtm api
left a minor comment.

adisuissa · 2022-01-26T02:51:17Z

+# connect handler
+/*/extensions/filters/listener/connect_handler @rojkov @alyssawilk


Please move up to the extensions part.

rojkov

Thank you! Looks good. I added a few questions.

rojkov · 2022-01-26T09:58:23Z

+  static constexpr absl::string_view HTTP1_CONNECT_PREFACE = "CONNECT";
+  Buffer::OwnedImpl resp_buf_{};
+  absl::string_view protocol_;
+  absl::string_view request_url_;


Is it used?

rojkov · 2022-01-26T10:00:50Z

+          return ParseState::Error;
+        }
+        // terminate CONNECT request
+        resp_buf_.add(protocol_);


It seems there's no need to make protocol_ a member of the class.

rojkov · 2022-01-26T13:54:26Z

+http_parser_settings Filter::settings_{
+    nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
+};


Can it be just a compilation unit-wide constexpr?

rojkov · 2022-01-26T13:55:30Z

+thread_local uint8_t Filter::buf_[Config::MAX_INSPECT_SIZE];
+
+namespace {
+constexpr absl::string_view response_body = " 200 Connection Established\r\n\r\n";


According to STYLE.md this should be

Suggested change

constexpr absl::string_view response_body = " 200 Connection Established\r\n\r\n";

constexpr absl::string_view ResponseBody = " 200 Connection Established\r\n\r\n";

rojkov · 2022-01-26T14:04:16Z

+  if (result.return_value_ == 0) {
+    return ParseState::Error;
+  }
+  return parseConnect(absl::string_view(reinterpret_cast<const char*>(buf_), result.return_value_));


No need to instantiate a temp string_view here.

rojkov · 2022-01-26T14:13:08Z

+          return ParseState::Done;
+        }
+
+        std::vector<absl::string_view> fields = absl::StrSplit(data, absl::ByAnyChar(" :"));


Would it be more efficient to use StringUtil::cropLeft() and StringUtil::cropRight() for server name extraction?

rojkov · 2022-01-26T14:22:58Z

 intra
 ints
 invariance
+iov


This is probably not needed.

I added this because my local checker could not get passed, removing this to see if CI job works well.

Indeed the check fails. This is weird as I used iov in my PRs and everything was ok. And it's all over the code base. Looks like the code is treated as a comment by the spellchecker. @phlax could you please have a look?

rojkov · 2022-01-31T08:05:25Z

/wait

Signed-off-by: leizhang <lei.a.zhang@intel.com>

rojkov · 2022-02-09T08:11:39Z

+constexpr http_parser_settings Settings{
+    nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr, nullptr,
+};
+}


nit:

Suggested change

}

} // namespace

rojkov · 2022-02-09T08:44:34Z

+    #TODO(davinci26): The test passes on Windows *but* http inspector
+    # *used* to rely on Event::FileTriggerType::Edge and we got away with it


Is this comment relevant to connect_handler?

rojkov · 2022-02-09T08:49:30Z

+envoy_cc_fuzz_test(
+    name = "connect_handler_fuzz_test",
+    srcs = ["connect_handler_fuzz_test.cc"],
+    corpus = "connect_handler_corpus",
+    deps = [
+        "//source/extensions/filters/listener/connect_handler:connect_handler_lib",
+        "//test/extensions/filters/listener/common/fuzz:listener_filter_fuzzer_lib",
+    ],
+)


This target seems to be missing in the PR.

adisuissa

Thanks!
Left a small comment on the doc.
/lgtm api
I suggest adding an integration test.

adisuissa · 2022-02-09T15:04:51Z

+Connect Handler
+===============
+
+Connect Handler listener filter allows detecting whether the incoming request appears to be HTTP/1.x CONNECT, and if it is CONNECT, it removes the CONNECT by reading data from socket and responding with a 200 status code to indicate a connection to remote server is established. It also extracts remote server url from the CONNECT. This can be used to select a


Suggested change

Connect Handler listener filter allows detecting whether the incoming request appears to be HTTP/1.x CONNECT, and if it is CONNECT, it removes the CONNECT by reading data from socket and responding with a 200 status code to indicate a connection to remote server is established. It also extracts remote server url from the CONNECT. This can be used to select a

Connect Handler listener filter allows detecting whether the incoming request appears to be HTTP/1.x CONNECT, and if it is CONNECT, it removes the CONNECT by reading data from the socket and responding with a 200 status code to indicate a connection to the remote server is established. It also extracts the remote server url from the CONNECT. This can be used to select a

rojkov · 2022-02-11T09:15:48Z

/wait

github-actions · 2022-03-13T12:01:11Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2022-03-20T12:03:12Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

repokitteh-read-only Bot added the api label Dec 23, 2021

repokitteh-read-only Bot assigned adisuissa Dec 23, 2021

rojkov reviewed Dec 23, 2021

View reviewed changes

rojkov self-assigned this Dec 23, 2021

repokitteh-read-only Bot added the waiting label Dec 27, 2021

adisuissa reviewed Dec 28, 2021

View reviewed changes

repokitteh-read-only Bot removed the waiting label Jan 5, 2022

repokitteh-read-only Bot added the waiting label Jan 5, 2022

repokitteh-read-only Bot removed the waiting label Jan 18, 2022

leizhang added 4 commits January 20, 2022 06:28

Address review comments

a03f825

Signed-off-by: leizhang <lei.a.zhang@intel.com>

Add unittest

7ece110

Signed-off-by: leizhang <lei.a.zhang@intel.com>

Fix format and DCO

04203ad

Signed-off-by: leizhang <lei.a.zhang@intel.com>

liverbirdkte force-pushed the connect_handler branch from 7ae9503 to 04203ad Compare January 20, 2022 09:02

repokitteh-read-only Bot added the v2-freeze label Jan 20, 2022

liverbirdkte marked this pull request as draft January 20, 2022 09:28

leizhang added 2 commits January 25, 2022 08:48

Add docs

65c58f8

Signed-off-by: leizhang <lei.a.zhang@intel.com>

Merge branch 'main' into connect_handler

50caaf9

liverbirdkte marked this pull request as ready for review January 26, 2022 01:57

adisuissa reviewed Jan 26, 2022

View reviewed changes

repokitteh-read-only Bot removed the api label Jan 26, 2022

rojkov reviewed Jan 26, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Jan 31, 2022

Merge branch 'main' into connect_handler

8a3c35b

repokitteh-read-only Bot removed the waiting label Feb 8, 2022

liverbirdkte marked this pull request as draft February 8, 2022 11:01

lei zhang added 2 commits February 8, 2022 22:21

Merge branch 'envoyproxy:main' into connect_handler

acc042f

Address comments

4115216

Signed-off-by: leizhang <lei.a.zhang@intel.com>

liverbirdkte marked this pull request as ready for review February 8, 2022 14:57

rojkov reviewed Feb 9, 2022

View reviewed changes

adisuissa reviewed Feb 9, 2022

View reviewed changes

repokitteh-read-only Bot added the waiting label Feb 11, 2022

github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 13, 2022

github-actions Bot closed this Mar 20, 2022

LuyaoZhong mentioned this pull request Jul 20, 2022

TLS bumping: decrypting communications between internal and external services #18928

Open

	static absl::string_view response_body = " 200 Connection Established\r\n\r\n";
	static constexpr absl::string_view response_body = " 200 Connection Established\r\n\r\n";

		# connect handler
		/*/extensions/filters/listener/connect_handler @rojkov @alyssawilk

	constexpr absl::string_view response_body = " 200 Connection Established\r\n\r\n";
	constexpr absl::string_view ResponseBody = " 200 Connection Established\r\n\r\n";

		#TODO(davinci26): The test passes on Windows but http inspector
		# used to rely on Event::FileTriggerType::Edge and we got away with it

	Connect Handler listener filter allows detecting whether the incoming request appears to be HTTP/1.x CONNECT, and if it is CONNECT, it removes the CONNECT by reading data from socket and responding with a 200 status code to indicate a connection to remote server is established. It also extracts remote server url from the CONNECT. This can be used to select a
	Connect Handler listener filter allows detecting whether the incoming request appears to be HTTP/1.x CONNECT, and if it is CONNECT, it removes the CONNECT by reading data from the socket and responding with a 200 status code to indicate a connection to the remote server is established. It also extracts the remote server url from the CONNECT. This can be used to select a

Conversation

liverbirdkte commented Dec 23, 2021

Uh oh!

repokitteh-read-only Bot commented Dec 23, 2021

Uh oh!

repokitteh-read-only Bot commented Dec 23, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojkov commented Dec 27, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liverbirdkte commented Jan 4, 2022

Uh oh!

liverbirdkte commented Jan 5, 2022

Uh oh!

rojkov commented Jan 5, 2022

Uh oh!

liverbirdkte commented Jan 18, 2022

Uh oh!

repokitteh-read-only Bot commented Jan 20, 2022

Uh oh!

rojkov commented Jan 20, 2022

Uh oh!

liverbirdkte commented Jan 20, 2022

Uh oh!

liverbirdkte commented Jan 20, 2022

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!