http: stream error on invalid HTTP messaging for HTTP/1 #11884

rulex123 · 2020-07-03T14:38:55Z

In this PR:

HTTP/1.1 API is extended to allow configuration of behaviour on invalid HTTP messaging;
codec interface is extended so that the HCM can pull value for new config from it;
logic for behaviour on invalid HTTP messaging added in http: stream error on invalid messaging #11748 is updated to account for the new HTTP/1.1 option.
Helps with http: require stream_error_on_invalid_http_messaging to be set for every HCM #9846.

Risk Level: high (HCM changes)
Testing: new unit tests and integration tests
Docs Changes: n/a
Release Notes: updated
Runtime guard: envoy.reloadable_features.hcm_stream_error_on_invalid_message (already introduced by #11748)

repokitteh-read-only · 2020-07-03T14:39:03Z

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).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #11884 was opened by rulex123.

see: more, trace.

alyssawilk

Yep, good start! Next you'll want to make your new function available from the Http Connection Manager, since it only has the abstract APIs. You could also start on the codec tests paralleling mine for HTTP/2 for which value takes precedence. right now you can obviously only test yours being on and off, but once my PR lands we can do the full 4x combinations.

alyssawilk · 2020-07-06T16:31:33Z

api/envoy/api/v2/core/protocol.proto

@@ -142,6 +142,10 @@ message Http1ProtocolOptions {
  //   - Not a response to a HEAD request.
  //   - The content length header is not present.
  bool enable_trailers = 5;
+
+  // [#not-implemented-hide:]
+  // Overrides envoy's behavior on invalid HTTP messaging configured in the hcm


you'll want to ref link to the actual hcm field - you can crib that notation from my PR as well :-)

alyssawilk · 2020-07-06T16:32:27Z

source/common/http/http1/codec_impl.h

@@ -128,8 +131,13 @@ class ResponseEncoderImpl : public StreamEncoderImpl, public ResponseEncoder {
  void encodeHeaders(const ResponseHeaderMap& headers, bool end_stream) override;
  void encodeTrailers(const ResponseTrailerMap& trailers) override { encodeTrailersBase(trailers); }

+  absl::optional<bool> streamErrorOnInvalidHTTPMessage() override {


HTTP -> Http ?

source/common/http/http2/codec_impl.h

…o, extend codec interface so that hcm can pull this new option from the codec. Signed-off-by: Erica Manno <[email protected]>

Signed-off-by: Erica Manno <[email protected]>

…ng for HTTP/1.1 Signed-off-by: Erica Manno <[email protected]>

Signed-off-by: Erica Manno <[email protected]>

rulex123 · 2020-07-20T15:08:43Z

@alyssawilk I think this is ready for another look.

One question though: do we want to update the old code path for sending errors from the codec as well (

envoy/source/common/http/http1/codec_impl.cc

Line 969 in bfab9ed

void ServerConnectionImpl::sendProtocolErrorOld(absl::string_view details) {

) to terminate the connection or not based on the new config param?

alyssawilk

Looking good!

I think your PR needs a format check, and it'd be good to have an integration test Luckily I think Piotr wrote most of one for you
DownstreamProtocolIntegrationTest,.InvalidContentLengthAllowed
tests that for http/2 the config override results in stream reset, for HTTP/1 it results in disconnect.

You will need to change it to set the HTTP/1.1 override as well, and perhaps change the invalid message from one with invalid content length (which will trigger before the headers are complete) to some other messaging error - maybe remove the host?

alyssawilk · 2020-07-20T15:23:38Z

api/envoy/config/core/v3/protocol.proto

@@ -157,6 +157,11 @@ message Http1ProtocolOptions {
  //   - Not a response to a HEAD request.
  //   - The content length header is not present.
  bool enable_trailers = 5;
+
+  // Allows invalid HTTP messaging. When this option is disabled (default), then Envoy will terminate


Hm, when this option is disabled, Envoy will terminate the connection, but I think you should remove reference to default behavior, since the default behavior is actually to ignore this field and use the HCM value.

alyssawilk · 2020-07-20T15:25:31Z

source/common/http/conn_manager_impl.cc

-      !connection_manager_.config_.streamErrorOnInvalidHttpMessaging() &&
-      code == Http::Code::BadRequest && connection_manager_.codec_->protocol() < Protocol::Http2) {
+      code == Http::Code::BadRequest && connection_manager_.codec_->protocol() < Protocol::Http2 &&
+      !Utility::streamErrorOnInvalidHttpMessage(


If this function is only used in one place, I'd be mildly inclined to make this utility in an unnamed namespace in the cc file, and unit test the HCM by setting explicit configs. WDYT?

alyssawilk · 2020-07-20T15:28:05Z

source/common/http/http2/codec_impl.h

+    absl::optional<bool> stream_error_on_invalid_http_message_;
+
+    absl::optional<bool> streamErrorOnInvalidHttpMessage() const override {
+      return stream_error_on_invalid_http_message_;


Do we need to latch this, or can we just return parent_.stream_error_on_invalid_http_messaging_ here?

Also let's make sure this and the HTTP/1.1 function have unit tests, even if they're pretty trivial.

Signed-off-by: Erica Manno <[email protected]>

mattklein123

Thanks this LGTM modulo the comment about clarifying what is supported downstream and upstream. @alyssawilk is back from vacation so I will defer to her for final review. Thank you!

mattklein123 · 2020-08-17T17:17:15Z

include/envoy/http/codec.h

+  // Behaviour on invalid HTTP messaging:
+  // - if set and true, the HTTP/1.1 connection is left open (where possible)
+  // - if set and false, the HTTP/1.1 connection is terminated
+  // - if not set, fall back to the HCM behaviour on invalid HTTP messaging


Yeah I think it would be good to call this out more in the comments and the proto documentation. The proto is a generic message that is used for both upstream and downstream, so I think it would be good to clarify that it only applies right now for downstream?

Signed-off-by: Erica Manno <[email protected]>

rulex123 · 2020-08-24T18:02:36Z

@alyssawilk ping: any further feedback beside what Matt said?

alyssawilk

Oh ugh I'm so sorry, I thought I sent these comments out last week! thanks for the ping and sorry for the lag and late cross-codec-behavior comment

alyssawilk · 2020-08-18T17:08:07Z

api/envoy/config/core/v3/protocol.proto

+  // open where possible.
+  // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging
+  // <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>`;
+  // if not set, this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging


I think you can leave out the "if not set" because the sentence above makes it clear that otherwise the HCM value is used.

source/common/http/http1/codec_impl_legacy.h

Signed-off-by: Erica Manno <[email protected]>

rulex123 · 2020-08-26T19:05:19Z

@alyssawilk no worries! I have reworked the code a bit to take into account your cross-codec-behavior comment. Please take another look when you get a chance :-)

alyssawilk

Awesome, thanks for your patience with the last minute thought! sending a few more ideas your way

alyssawilk · 2020-08-26T19:52:08Z

api/envoy/config/core/v3/protocol.proto

@@ -164,7 +164,7 @@ message Http1ProtocolOptions {
  // open where possible.
  // If set, this overrides any HCM :ref:`stream_error_on_invalid_http_messaging
  // <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_error_on_invalid_http_message>`;
-  // if not set, this is overridden by any HCM :ref:`stream_error_on_invalid_http_messaging


sorry, for any ocnfusion, I was suggesting taking out this line and following.

alyssawilk · 2020-08-26T19:52:26Z

include/envoy/http/codec.h

@@ -147,7 +147,7 @@ class ResponseEncoder : public virtual StreamEncoder {
   * Indicates whether invalid HTTP messaging should be handled with a stream error or a connection
   * error; if value is missing, then HCM drives behaviour on invalid HTTP messaging.


update comment to remove "if value is missing, then HCM drives behaviour on invalid HTTP messaging."

alyssawilk · 2020-08-26T19:54:36Z

source/common/http/utility.cc

+Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config,
+                            bool hcm_stream_error_set,
+                            const Protobuf::BoolValue& hcm_stream_error) {
+  Http1Settings ret = parseHttp1Settings(config);


I think this function just return the override_stream_error if set, and if not returns the HCM value no?

Hm, isn't it the case that when this function is called the HCM value hasn't been initialised to its default value yet (in case of no explicit configuration)? See

envoy/source/extensions/filters/network/http_connection_manager/config.cc

Lines 233 to 234 in c555587

stream_error_on_invalid_http_messaging_(

PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, stream_error_on_invalid_http_message, false)),

I guess we could move that initialisation so that it happens earlier, and then simplify this function as per your comment. WDYT?

ah, I see what your concern is, but I believe (and you can unit test) that the default value for BoolValue is false, and the default for stream_error_on_invalid_http_messaging_ is false, so you should just be able to set the default to config.override_stream_error_on_invalid_http_message().value() even if it's not set.

Signed-off-by: Erica Manno <[email protected]>

alyssawilk

Looks great - just a few final comments :-)

alyssawilk · 2020-08-27T20:57:56Z

source/common/http/utility.cc

+Utility::parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOptions& config,
+                            bool hcm_stream_error_set,
+                            const Protobuf::BoolValue& hcm_stream_error) {
+  Http1Settings ret = parseHttp1Settings(config);


ah, I see what your concern is, but I believe (and you can unit test) that the default value for BoolValue is false, and the default for stream_error_on_invalid_http_messaging_ is false, so you should just be able to set the default to config.override_stream_error_on_invalid_http_message().value() even if it's not set.

alyssawilk · 2020-08-27T21:01:40Z

test/common/http/conn_manager_impl_test.cc

@@ -2131,6 +2131,63 @@ TEST_F(HttpConnectionManagerImplTest, TestAccessLogWithInvalidRequest) {
  conn_manager_->onData(fake_input, false);
 }

+class StreamErrorOnInvalidHttpMessageTest : public HttpConnectionManagerImplTest {
+public:
+  void sendInvalidRequestAndVerifyConnectionState(bool codec_stream_error,


Since these are always opposite I think you can just have one boolean, stream_error_in_invalid_messaging and make sure that the connection is terminated if !stream_error

alyssawilk · 2020-08-27T21:05:00Z

test/integration/protocol_integration_test.cc

+// HTTP message (missing :host header)
+TEST_P(DownstreamProtocolIntegrationTest,
+       Http1ConnectionIsLeftOpenIfHCMStreamErrorIsFalseAndOverrideIsTrue) {
+  if (downstreamProtocol() == Http::CodecClient::Type::HTTP2) {


I think you can move these to
TEST_P(IntegrationTest,...)
and then it won't kick off a test run for downstream HTTP/2

Signed-off-by: Erica Manno <[email protected]>

rulex123 · 2020-09-01T16:40:23Z

@alyssawilk ping: any further comments or feedback? Would love to wrap this up this week if possible :-)

alyssawilk · 2020-09-01T19:24:12Z

over to Matt for API sign off.

mattklein123

Thank you, especially for the lengthy review cycle. This stuff is tricky to get right!

repokitteh-read-only bot added v2-freeze api labels Jul 3, 2020

alyssawilk self-assigned this Jul 6, 2020

alyssawilk reviewed Jul 6, 2020

View reviewed changes

alyssawilk added the waiting label Jul 7, 2020

repokitteh-read-only bot removed the waiting label Jul 9, 2020

rulex123 added 9 commits July 13, 2020 17:26

Add config for behavior on HTTP invalid message to HTTP1 options. Als…

ee7cb0d

…o, extend codec interface so that hcm can pull this new option from the codec. Signed-off-by: Erica Manno <[email protected]>

Correct typo

b04b98e

Signed-off-by: Erica Manno <[email protected]>

Shadow API

46768be

Signed-off-by: Erica Manno <[email protected]>

Extend codec to return behavior on invalid HTTP messaging

d1bf85d

Signed-off-by: Erica Manno <[email protected]>

Update API documentation

2d7e93d

Signed-off-by: Erica Manno <[email protected]>

Add TODO

86031f0

Signed-off-by: Erica Manno <[email protected]>

Add utility function that determines behavior on invalid HTTP messagi…

397e369

…ng for HTTP/1.1 Signed-off-by: Erica Manno <[email protected]>

Format code

c139ff1

Signed-off-by: Erica Manno <[email protected]>

Fix format

2d6cac8

Signed-off-by: Erica Manno <[email protected]>

rulex123 force-pushed the behavior-on-invalid-http-message branch from 26b7a81 to 2d6cac8 Compare July 13, 2020 16:35

rulex123 added 8 commits July 17, 2020 17:54

Add const

d30e854

Signed-off-by: Erica Manno <[email protected]>

Plug logic into HCM

aa421fd

Signed-off-by: Erica Manno <[email protected]>

Format

9718246

Signed-off-by: Erica Manno <[email protected]>

Add unit test for HCM logic

cc33ccf

Signed-off-by: Erica Manno <[email protected]>

Merge branch 'master' into behavior-on-invalid-http-message

cab6751

Signed-off-by: Erica Manno <[email protected]>

Add unit tests for HCM

00814d2

Signed-off-by: Erica Manno <[email protected]>

Update API to remove hide-implementation annotation

7f61bdf

Signed-off-by: Erica Manno <[email protected]>

Remove changes from v2 API

908bdfc

Signed-off-by: Erica Manno <[email protected]>

rulex123 changed the title ~~http: config for behaviour on invalid HTTP message for HTTP/1.1~~ http: stream error on invalid HTTP messaging for HTTP/1 Jul 20, 2020

rulex123 marked this pull request as ready for review July 20, 2020 15:02

rulex123 requested a review from mattklein123 as a code owner July 20, 2020 15:02

alyssawilk reviewed Jul 20, 2020

View reviewed changes

More code review feedback

a35bada

Signed-off-by: Erica Manno <[email protected]>

repokitteh-read-only bot removed the waiting label Aug 17, 2020

mattklein123 previously approved these changes Aug 17, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Aug 17, 2020

mattklein123 removed their assignment Aug 17, 2020

Merge branch 'master' into behavior-on-invalid-http-message

b037f64

Signed-off-by: Erica Manno <[email protected]>

rulex123 dismissed mattklein123’s stale review via b037f64 August 24, 2020 14:37

repokitteh-read-only bot added the api label Aug 24, 2020

alyssawilk reviewed Aug 24, 2020

View reviewed changes

rulex123 added 3 commits August 26, 2020 19:45

Have logic that determines stream error on codec instantiation

16414a0

Signed-off-by: Erica Manno <[email protected]>

Unit test for init logic of stream_error

4bb2a86

Signed-off-by: Erica Manno <[email protected]>

Update doc

1410972

Signed-off-by: Erica Manno <[email protected]>

alyssawilk reviewed Aug 26, 2020

View reviewed changes

Address feedback and fix CI

5e63612

Signed-off-by: Erica Manno <[email protected]>

alyssawilk reviewed Aug 27, 2020

View reviewed changes

rulex123 added 5 commits August 28, 2020 12:28

Address feedback

85e57c4

Signed-off-by: Erica Manno <[email protected]>

Merge branch 'master' into behavior-on-invalid-http-message

3dd2c3d

Signed-off-by: Erica Manno <[email protected]>

Fix unit test

464a5c7

Signed-off-by: Erica Manno <[email protected]>

Fix test

e663b45

Signed-off-by: Erica Manno <[email protected]>

Merge branch 'master' into behavior-on-invalid-http-message

d92640d

Signed-off-by: Erica Manno <[email protected]>

alyssawilk approved these changes Sep 1, 2020

View reviewed changes

alyssawilk assigned mattklein123 Sep 1, 2020

mattklein123 approved these changes Sep 1, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 1, 2020

mattklein123 merged commit 52ec66f into envoyproxy:master Sep 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: stream error on invalid HTTP messaging for HTTP/1 #11884

http: stream error on invalid HTTP messaging for HTTP/1 #11884

rulex123 commented Jul 3, 2020 •

edited

Loading

repokitteh-read-only bot commented Jul 3, 2020

alyssawilk left a comment

alyssawilk Jul 6, 2020

alyssawilk Jul 6, 2020

rulex123 commented Jul 20, 2020

alyssawilk left a comment

alyssawilk Jul 20, 2020

alyssawilk Jul 20, 2020

alyssawilk Jul 29, 2020

alyssawilk Jul 20, 2020

mattklein123 left a comment

mattklein123 Aug 17, 2020

rulex123 commented Aug 24, 2020

alyssawilk left a comment

alyssawilk Aug 18, 2020

rulex123 commented Aug 26, 2020

alyssawilk left a comment

alyssawilk Aug 26, 2020

alyssawilk Aug 26, 2020

alyssawilk Aug 26, 2020

rulex123 Aug 27, 2020

alyssawilk Aug 27, 2020

alyssawilk left a comment

alyssawilk Aug 27, 2020

alyssawilk Aug 27, 2020

alyssawilk Aug 27, 2020

rulex123 commented Sep 1, 2020

alyssawilk commented Sep 1, 2020

mattklein123 left a comment

		@@ -147,7 +147,7 @@ class ResponseEncoder : public virtual StreamEncoder {
		* Indicates whether invalid HTTP messaging should be handled with a stream error or a connection
		* error; if value is missing, then HCM drives behaviour on invalid HTTP messaging.

	stream_error_on_invalid_http_messaging_(
	PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, stream_error_on_invalid_http_message, false)),

http: stream error on invalid HTTP messaging for HTTP/1 #11884

http: stream error on invalid HTTP messaging for HTTP/1 #11884

Conversation

rulex123 commented Jul 3, 2020 • edited Loading

repokitteh-read-only bot commented Jul 3, 2020

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rulex123 commented Jul 20, 2020

alyssawilk 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

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rulex123 commented Aug 24, 2020

alyssawilk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rulex123 commented Aug 26, 2020

alyssawilk 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

alyssawilk 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

rulex123 commented Sep 1, 2020

alyssawilk commented Sep 1, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

rulex123 commented Jul 3, 2020 •

edited

Loading