http3: respecting header number limits #15970

alyssawilk · 2021-04-14T14:46:05Z

Respecting upstream and downstream caps on number of headers for HTTP/3

Risk Level: n/a (http/3)
Testing: turned up integration tests
Docs Changes: n/a
Release Notes: n/a
#14829 among others.

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

danzh2010

Thanks for fixing more tests! A few thoughts on the details.

danzh2010 · 2021-04-14T15:02:02Z

test/integration/protocol_integration_test.cc

@@ -1600,7 +1600,6 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestHeadersAccepted) {
 TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) {
  // QUICHE doesn't limit number of headers.
  EXCLUDE_DOWNSTREAM_HTTP3


The upstream http3 test failure is irrelevant to this fix right? I doubt current fix works for trailers.

Ah yeah, I should fix that.

danzh2010 · 2021-04-14T15:03:56Z

test/integration/multiplexed_upstream_integration_test.cc

@@ -47,12 +47,10 @@ TEST_P(Http2UpstreamIntegrationTest, RouterHeaderOnlyRequestAndResponseNoBuffer)
 }

 TEST_P(Http2UpstreamIntegrationTest, RouterUpstreamDisconnectBeforeRequestcomplete) {
-  EXCLUDE_UPSTREAM_HTTP3; // Close loop.


How are these tests related to max headers count?

They're not, I was just enabling excluded tests until I found ones that didn't work =P

danzh2010 · 2021-04-14T15:13:35Z

source/common/quic/envoy_quic_utils.h

+static constexpr absl::string_view invalid_underscore = "http3.unexpected_underscore";
+static constexpr absl::string_view too_many_http_headers = "http3.too_many_http_headers";


Maybe add/reuse those already defined in Http3ResponseCodeDetailValues? You may need to move it around.

danzh2010 · 2021-04-14T15:14:44Z

source/common/quic/envoy_quic_utils.h

    Http::HeaderUtility::HeaderValidationResult result =
        validator.validateHeader(entry.first, entry.second);
    switch (result) {
    case Http::HeaderUtility::HeaderValidationResult::REJECT:
+      details = invalid_underscore;


This details is already assigned in server stream's validateHeader().

danzh2010 · 2021-04-14T15:21:54Z

source/common/quic/envoy_quic_utils.h

-                                             HeaderValidator& validator) {
+std::unique_ptr<T>
+quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, HeaderValidator& validator,
+                          uint32_t max_headers_allowed, absl::string_view& details) {


Is it because QuicHeaderList doesn't expose size() interface that we need max_headers_allowed to be passed into this function?

I slightly prefer checking it in QUICHE and have something like OnHeadersTooLarge() or simply add QuicHeaderList::size() instead of iterating over the header list again.

100% prefer checking in QUICHE and rejecting early, vs rejecting late in the game, but I think this is still better than nothing because it protects Envoy filters against too many headers. What do you think about landing this as-is and moving where we enforce once someone on quic-dev adds the knob we'd need?

danzh2010 · 2021-04-14T15:26:40Z

source/common/quic/envoy_quic_client_stream.cc

  if (headers == nullptr) {
-    onStreamError(close_connection_upon_invalid_header_);
+    onStreamError(close_connection_upon_invalid_header_, quic::QUIC_STREAM_INTERNAL_ERROR);


This is not negotiated in initial SETTINGS. I think it should be QUIC_BAD_APPLICATION_PAYLOAD or at least something else other than QUIC_STREAM_INTERNAL_ERROR?

Ah, so the quiche code maps bad application payload to Http::StreamResetReason::ProtocolError which is only for spec violations. We differentiate between envoy rejecting something which it is configured to reject from actual standards violantion by response code (503/502) so we need to have a different code for header limit checks. I don't care which but it has to translate to something which isn't an Envoy ProtocolError

How about QUIC_STREAM_EXCESSIVE_LOAD ?

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

danzh2010 · 2021-04-15T21:20:43Z

source/common/quic/envoy_quic_utils.h

-                                             HeaderValidator& validator) {
+std::unique_ptr<T>
+quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, HeaderValidator& validator,
+                          uint32_t max_headers_allowed, absl::string_view& details) {


danzh2010 · 2021-04-15T21:29:03Z

source/common/quic/envoy_quic_client_stream.cc

  if (headers == nullptr) {
-    onStreamError(close_connection_upon_invalid_header_);
+    onStreamError(close_connection_upon_invalid_header_, quic::QUIC_STREAM_INTERNAL_ERROR);


How about QUIC_STREAM_EXCESSIVE_LOAD ?

danzh2010 · 2021-04-15T21:31:03Z

source/common/quic/envoy_quic_server_stream.cc

+    if (received_trailers().size() > filterManagerConnection()->maxIncomingHeadersCount()) {
+      details_ = Http3ResponseCodeDetailValues::too_many_trailers;
+      onStreamError(close_connection_upon_invalid_header_);
+      return;
+    }


Same for client stream?

danzh2010 · 2021-04-15T21:33:40Z

test/integration/protocol_integration_test.cc

@@ -1622,13 +1617,12 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyRequestTrailersRejected) {
    EXPECT_TRUE(response->complete());
    EXPECT_EQ("431", response->headers().getStatusValue());
  } else {
-    ASSERT_TRUE(response->waitForReset());


should this be reverted?

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

snowp

Seems right to me, just one tiny nit

snowp · 2021-04-28T00:07:33Z

source/common/quic/envoy_quic_utils.h

+  static constexpr absl::string_view remote_refused = "http3.remote_refuse";
+  // The peer reset the stream.
+  static constexpr absl::string_view remote_reset = "http3.remote_reset";
+  // Too many trailers were sent


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

alyssawilk · 2021-04-28T15:07:42Z

Looks like an asan flake in a new test
/wait

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

alyssawilk · 2021-04-29T18:48:34Z

woot! period added, merge conflicts sorted, test flake fixed :-)

snowp

Thanks!

Respecting upstream and downstream caps on number of headers for HTTP/3 Risk Level: n/a (http/3) Testing: turned up integration tests Docs Changes: n/a Release Notes: n/a envoyproxy#14829 among others. Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Gokul Nair <gnair@twitter.com>

http3: respecting header number limits

8690f06

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

alyssawilk assigned danzh2010 Apr 14, 2021

alyssawilk requested a review from mattklein123 as a code owner April 14, 2021 14:46

maybe turning up more tests?

5ef16a3

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

danzh2010 reviewed Apr 14, 2021

View reviewed changes

alyssawilk added 6 commits April 14, 2021 12:45

enforcing trailer limits, addressing comments

c84e9b1

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

fix_test

05ada94

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

last test

ae6d313

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

Merge branch 'main' into header_limits

687f2f8

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

Merge branch 'main' into header_limits

c7d8001

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

Merge branch 'main' into header_limits

1baddbc

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

danzh2010 reviewed Apr 15, 2021

View reviewed changes

Merge branch 'main' into header_limits

5dff671

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

alyssawilk added the waiting label Apr 16, 2021

alyssawilk added 2 commits April 26, 2021 12:51

Merge branch 'main' into header_limits

73104c8

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

comments

0a1895e

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

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

alyssawilk added 2 commits April 26, 2021 16:47

speeling

4dc82ff

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

Merge branch 'main' into header_limits

485a234

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

alyssawilk assigned snowp Apr 27, 2021

snowp reviewed Apr 28, 2021

View reviewed changes

.

e0aeb76

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

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

danzh2010 previously approved these changes Apr 28, 2021

View reviewed changes

fix asan?

6525ecf

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

alyssawilk dismissed danzh2010’s stale review via 6525ecf April 28, 2021 16:16

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

alyssawilk added 2 commits April 28, 2021 15:10

Merge branch 'main' into header_limits

2d29b48

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

Merge branch 'main' into header_limits

96ddf39

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

snowp approved these changes May 3, 2021

View reviewed changes

alyssawilk merged commit 6a2e112 into envoyproxy:main May 3, 2021

alyssawilk deleted the header_limits branch August 18, 2021 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http3: respecting header number limits #15970

http3: respecting header number limits #15970

alyssawilk commented Apr 14, 2021

danzh2010 left a comment

danzh2010 Apr 14, 2021

alyssawilk Apr 14, 2021

danzh2010 Apr 14, 2021

alyssawilk Apr 14, 2021

danzh2010 Apr 14, 2021

danzh2010 Apr 14, 2021 •

edited

Loading

danzh2010 Apr 14, 2021 •

edited

Loading

alyssawilk Apr 14, 2021

danzh2010 Apr 15, 2021

danzh2010 Apr 14, 2021

alyssawilk Apr 14, 2021

danzh2010 Apr 15, 2021

danzh2010 Apr 15, 2021

danzh2010 Apr 15, 2021

danzh2010 Apr 15, 2021

danzh2010 Apr 15, 2021

snowp left a comment

snowp Apr 28, 2021

alyssawilk commented Apr 28, 2021

alyssawilk commented Apr 29, 2021

snowp left a comment

		static constexpr absl::string_view invalid_underscore = "http3.unexpected_underscore";
		static constexpr absl::string_view too_many_http_headers = "http3.too_many_http_headers";

http3: respecting header number limits #15970

http3: respecting header number limits #15970

Conversation

alyssawilk commented Apr 14, 2021

danzh2010 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

danzh2010 Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

danzh2010 Apr 14, 2021 • edited Loading

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

snowp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented Apr 28, 2021

alyssawilk commented Apr 29, 2021

snowp left a comment

Choose a reason for hiding this comment

danzh2010 Apr 14, 2021 •

edited

Loading

danzh2010 Apr 14, 2021 •

edited

Loading