http3: support Http3Options for downstream #15753

danzh2010 · 2021-03-30T03:21:44Z

Commit Message: allow HCM to config Http3Options and use it with other HCM configs, i.e. max_request_headers_kb and headers_with_underscores_action, to setup QuicHttpServerConnectionImpl. And support these configurations in QUIC. Currently the only Http3Options configuration is override_stream_error_on_invalid_http_message.

Additional Description: added Http3 codec stats. Pass it along with Http3Options and other HCM configs.

Risk Level: low
Testing: enable related tests in quic_protocol_integration_test
Docs Changes: updated docs/root/configuration/http/http_conn_man/response_code_details.rst

Part of #12930 #2557

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only · 2021-03-30T03:21:53Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15753 was opened by danzh2010.

see: more, trace.

alyssawilk

Nice change!

Once you sort out docs CI, let's check coverage on this one and make sure we've got corner cases tested.

alyssawilk · 2021-03-30T14:27:33Z

source/common/quic/envoy_quic_server_stream.cc

+}
+
+HeaderValidationResult
+EnvoyQuicServerStream::checkHeaderNameForUnderscores(const std::string& header_name) {


can we factor the Http2 code into HeaderUtility or not enough overlap? or maybe at least stick it in the utility directory, unit terst, and we can use when we switch codec libraries.

I moved this method to HeaderUtility, but using it in h2 codec doesn't save many lines, so I didn't use it there.

source/common/quic/envoy_quic_server_stream.h

alyssawilk · 2021-03-30T14:32:01Z

source/common/quic/envoy_quic_stream.h

+    return HeaderValidationResult::ACCEPT;
+  }
+
+  HeaderValidationResult validateContentLength(absl::string_view header_value,


Ditto, consider moving to utility

danzh2010 · 2021-03-30T16:59:32Z

api/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto

@@ -325,6 +325,10 @@ message HttpConnectionManager {
  config.core.v3.Http2ProtocolOptions http2_protocol_options = 9
      [(udpa.annotations.security).configure_for_untrusted_downstream = true];

+  // Additional HTTP/3 settings that are passed directly to the HTTP/3 codec.
+  config.core.v3.Http3ProtocolOptions http3_protocol_options = 44
+      [(udpa.annotations.security).configure_for_untrusted_downstream = true];


@htuch The doc CI fails because this field is annotated with udpa.annotations.security, but doesn't have description in docs/protodoc_manifest.yaml. Do I need to document override_stream_error_on_invalid_http_message, the only field of Http3ProtocolOptions, there? http2_protocol_options doesn't have documentation about its similar fields in protodoc_manifest.yaml.

If there is anything security related, e.g. buffering, update the example manifest YAML as per offline discussion. If not, feel free to drop the security annotation.

alyssawilk · 2021-03-30T17:14:07Z

Try hiding the field and see if that fixes docs build - it's probably trying to generate docs for the (not hidden) field refering to the (hidden) proto and having problems.
Also please main merge to pick up the Matt moving-all-the-things PR

Signed-off-by: Dan Zhang <danzh@google.com>

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

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-03-31T15:09:23Z

/retest

repokitteh-read-only · 2021-03-31T15:14:52Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15753 (comment) was created by @danzh2010.

see: more, trace.

htuch

/lgtm api

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk

Cool, just a few more nits and I think this one's good to go

source/common/http/header_utility.h

alyssawilk · 2021-04-01T12:39:12Z

source/common/quic/envoy_quic_stream.h

+  Http::HeaderUtility::HeaderValidationResult
+  validateHeader(const std::string& header_name, absl::string_view header_value) override {
+    bool override_stream_error_on_invalid_http_message =
+        http3_options_.has_override_stream_error_on_invalid_http_message() &&


Given we're cloning this, what do you think of just always setting it in initializeAndValidate?

Makes sense, then we don't need to check http3_options_.has_override_stream_error_on_invalid_http_message() here.

source/common/quic/quic_filter_manager_connection_impl.h

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-04-01T21:03:56Z

/retest

repokitteh-read-only · 2021-04-01T21:04:00Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15753 (comment) was created by @danzh2010.

see: more, trace.

mattklein123

Thanks really awesome to see this all coming together. A few comments from a quick pass.

/wait

mattklein123 · 2021-04-01T22:34:04Z

source/common/quic/envoy_quic_server_stream.cc

+  // closed.
+  static constexpr absl::string_view invalid_http_header = "http3.invalid_header_field";
+  // The size of headers (or trailers) exceeded the configured limits.
+  static constexpr absl::string_view headers_too_large = "http3.headers.too.large";


nit: it's kind of strange that this one is uses dot separation and the others are using underscores? Can we be consistent? Is this copied from elsewhere?

I saw http2.invalid.header.field and thought any error from external library should use ., no?

I think this is probably just a typo. IMO I would make it consistent with the others.

mattklein123 · 2021-04-01T22:39:14Z

include/envoy/upstream/upstream.h

@@ -995,6 +995,11 @@ class ClusterInfo {
   */
  virtual Http::Http2::CodecStats& http2CodecStats() const PURE;

+  /**
+   * @return the Http2 Codec Stats.


Suggested change

* @return the Http2 Codec Stats.

* @return the Http3 Codec Stats.

mattklein123 · 2021-04-01T22:41:33Z

source/common/quic/envoy_quic_server_session.cc

+    ENVOY_CONN_LOG(error, "Attempt to create stream {} before HCM filter is initialized.", *this,
+                   id);
+    return nullptr;


Can this actually happen? If it can, can this log spam?

It will be an implementation error if this happens.

OK then please use either ASSERT or ENVOY_BUG depending on what you want to do in terms of providing testing. I don't think kind of error handling makes sense.

mattklein123 · 2021-04-01T22:48:09Z

source/common/http/header_utility.cc

@@ -333,5 +333,57 @@ bool HeaderUtility::isModifiableHeader(absl::string_view header) {
          !absl::EqualsIgnoreCase(header, Headers::get().HostLegacy.get()));
 }

+HeaderUtility::HeaderValidationResult HeaderUtility::checkHeaderNameForUnderscores(


It kills me that this code is now copied into a third place when we already effectively have it in both the H1 and H2 codec. This is really scary and security sensitive stuff and I feel we shouldn't be making the problem worse (it's in the opposite direction of #10646). Is there any work we can do to share some of this code now? Maybe we can do a different PR to add this helper and call it somehow from both other codecs?

I don't want to touch any code in production in this PR because that needs feature protection. Once this PR lands, the other two codec can switch to this utility function.

OK fair enough. Can you at least add comments and put TODOs in the other codec places which reference this code and the tracking issue for merging?

cc @yanavlasov might be a good starter project for one of the envoy-sec folks

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2021-04-02T22:46:30Z

I refactored the stream error handling a bit. PTAL

mattklein123

Thanks LGTM, will defer to @alyssawilk to review the most recent set of changes and merge. Thank you!

mattklein123 · 2021-04-04T18:49:22Z

source/common/quic/envoy_quic_server_session.cc

@@ -44,8 +44,10 @@ quic::QuicSpdyStream* EnvoyQuicServerSession::CreateIncomingStream(quic::QuicStr
    return nullptr;
  }
  if (!codec_stats_.has_value() || !http3_options_.has_value()) {
-    ENVOY_CONN_LOG(error, "Attempt to create stream {} before HCM filter is initialized.", *this,
-                   id);
+    ENVOY_BUG(false,


Do you have a test for this ENVOY_BUG? If not just make it an assert.

As long as our implementation is correct, it shouldn't be reached. So I don't know how to trigger it in a meaningful test right now. But I'd like to keep it in production, in case we miss some QUICHE behavior change that could occassionally lead to this in the future.

But how will you know the error checking will work if we reach it in production? :) In general we require testing for ENVOY_BUG type logic, so I would still prefer that you switch this to assert if you can't reasonably test it.

Okay, I added a unit test in envoy_quic_server_session_test to hit this ENVOY_BUG just to check the ENVOY_BUG works as intended in debug and release build.

danzh2010 · 2021-04-05T03:13:57Z

/retest

repokitteh-read-only · 2021-04-05T03:14:00Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15753 (comment) was created by @danzh2010.

see: more, trace.

alyssawilk

Nice! just one more nit and good to go.

alyssawilk · 2021-04-05T13:10:53Z

source/common/http/header_utility.cc

@@ -333,5 +333,57 @@ bool HeaderUtility::isModifiableHeader(absl::string_view header) {
          !absl::EqualsIgnoreCase(header, Headers::get().HostLegacy.get()));
 }

+HeaderUtility::HeaderValidationResult HeaderUtility::checkHeaderNameForUnderscores(


cc @yanavlasov might be a good starter project for one of the envoy-sec folks

alyssawilk · 2021-04-05T13:13:53Z

source/common/quic/envoy_quic_server_stream.cc

  if (Http::HeaderUtility::requestHeadersValid(*headers) != absl::nullopt) {
-    stream_delegate()->OnStreamError(quic::QUIC_HTTP_FRAME_ERROR, "Invalid headers");
+    details_ = Http3ResponseCodeDetailValues::invalid_http_header;
+    onStreamError(!http3_options_.override_stream_error_on_invalid_http_message().value());


what do you think of making this a default argument? If you'd prefer to be explicit I'd mildly prefer a helper function for brevity.

I'd prefer having explicit argument. I changed the function to take an optional bool which if absent means checking override_stream_error_on_invalid_http_message from h3 options.

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk · 2021-04-05T18:16:58Z

source/common/quic/envoy_quic_server_stream.h

@@ -91,7 +91,7 @@ class EnvoyQuicServerStream : public quic::QuicSpdyServerStreamBase,

  // Either reset the stream or close the connection according to
  // close_connection_upon_invalid_header.
-  void onStreamError(bool close_connection_upon_invalid_header);
+  void onStreamError(absl::optional<bool> should_close_connection);


update comment, please?

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk · 2021-04-06T12:38:10Z

API hasn't changed since the LGTM so I'm going to go ahead and merge.

danzh1989 added 4 commits March 29, 2021 00:26

make test pass

0898c3e

Signed-off-by: Dan Zhang <danzh@google.com>

plumb thru http3 options and hcm config

c70d7cc

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into h3options

53695e7

Signed-off-by: Dan Zhang <danzh@google.com>

enable tests

215f63e

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from alyssawilk and mattklein123 as code owners March 30, 2021 03:21

repokitteh-read-only bot added the api label Mar 30, 2021

repokitteh-read-only bot assigned htuch Mar 30, 2021

danzh2010 changed the title ~~quiche: support Http3Options for downstream~~ http3: support Http3Options for downstream Mar 30, 2021

alyssawilk reviewed Mar 30, 2021

View reviewed changes

danzh2010 mentioned this pull request Mar 30, 2021

quic: turn up status tests #15648

Merged

danzh2010 commented Mar 30, 2021

View reviewed changes

danzh1989 added 2 commits March 30, 2021 13:17

fis protodoc

f8b7f03

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into h3options

6dd9ca1

Signed-off-by: Dan Zhang <danzh@google.com>

htuch reviewed Mar 31, 2021

View reviewed changes

api/envoy/config/core/v3/protocol.proto Show resolved Hide resolved

danzh1989 added 4 commits March 30, 2021 22:22

move some checks to HeaderUtility

720d792

Signed-off-by: Dan Zhang <danzh@google.com>

protofmt

85b7003

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into h3options

f21f032

Signed-off-by: Dan Zhang <danzh@google.com>

fix test

e55bd8a

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk self-assigned this Mar 31, 2021

htuch reviewed Mar 31, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Mar 31, 2021

Merge branch 'master' into h3options

f35ef44

Signed-off-by: Dan Zhang <danzh@google.com>

repokitteh-read-only bot added the api label Apr 1, 2021

remove codec factory

5004e70

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk reviewed Apr 1, 2021

View reviewed changes

danzh1989 added 2 commits April 1, 2021 11:48

address comment

511e79a

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into h3options

c255c5d

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk previously approved these changes Apr 1, 2021

View reviewed changes

alyssawilk assigned mattklein123 Apr 1, 2021

danzh2010 mentioned this pull request Apr 1, 2021

quiche: Fix crash in Quic listener upon request without authority #15799

Merged

mattklein123 requested changes Apr 1, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 1, 2021

danzh1989 added 2 commits April 2, 2021 14:24

Merge branch 'master' into h3options

c860abf

Signed-off-by: Dan Zhang <danzh@google.com>

address comments

c4e80a1

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed alyssawilk’s stale review via c4e80a1 April 2, 2021 19:10

repokitteh-read-only bot removed the waiting label Apr 2, 2021

enable more protocol test with client stream fix

7f01805

Signed-off-by: Dan Zhang <danzh@google.com>

mattklein123 previously approved these changes Apr 4, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Apr 4, 2021

alyssawilk reviewed Apr 5, 2021

View reviewed changes

onStreamError

5577f48

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed mattklein123’s stale review via 5577f48 April 5, 2021 18:11

repokitteh-read-only bot added the api label Apr 5, 2021

Merge branch 'master' into h3options

5d6362e

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk reviewed Apr 5, 2021

View reviewed changes

comments

255c01b

Signed-off-by: Dan Zhang <danzh@google.com>

alyssawilk merged commit c93d486 into envoyproxy:main Apr 6, 2021

	* @return the Http2 Codec Stats.
	* @return the Http3 Codec Stats.

http3: support Http3Options for downstream #15753

http3: support Http3Options for downstream #15753

Conversation

danzh2010 commented Mar 30, 2021 • edited Loading

repokitteh-read-only bot commented Mar 30, 2021

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

danzh2010 Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented Mar 30, 2021

danzh2010 commented Mar 31, 2021

repokitteh-read-only bot commented Mar 31, 2021

htuch left a 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

danzh2010 commented Apr 1, 2021

repokitteh-read-only bot commented Apr 1, 2021

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

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

danzh2010 commented Apr 2, 2021

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

danzh2010 commented Apr 5, 2021

repokitteh-read-only bot commented Apr 5, 2021

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 commented Apr 6, 2021

danzh2010 commented Mar 30, 2021 •

edited

Loading

danzh2010 Mar 30, 2021 •

edited

Loading