codec: Raise max_request_headers_kb limit to 96 KiB by aunu53 · Pull Request #5859 · envoyproxy/envoy

aunu53 · 2019-02-06T14:43:29Z

Signed-off-by: Auni Ahsan auni@google.com

Bump up max configurable max_request_headers_kb to 96 KiB.
Add a check to http1/codec_impl.cc for headers size.
Raise the default library limits in http_parser (at compilation) and nghttp2 (at options) to 0x7fffffff, so we'll rely on our own codec check.

Risk Level: Medium.
Testing: Moved all the large request headers tests to ProtocolIntegrationTest.

Part of #5626.

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-02-06T19:29:35Z

What's missing this PR is an actual runtime call of http_parser_set_max_header_size. That will go in the Http conn manager config, but it's impossible to test AFAIK because the proto validator limit is lower than the http_parser default limit. When the limit is bumped past 80kb, i'll add an http1 integration test and a settings call to conn manager/config.cc.

Signed-off-by: Auni Ahsan <auni@google.com>

alyssawilk · 2019-02-07T18:17:42Z

Hey Auni, as discussed offline, ping me when CI is happy?

/wait

aunu53 · 2019-02-13T15:57:19Z

Hi! I need some input on how to resolve this. Essentially, my test is failing because the nghttp2 library max request headers check, which I now manually configure, trips differently from the existing codec check. The codec check at codec_impl.cc:563 will check if the headers exceed the limit and reset the stream if so, while the nghttp2 library check will just flat-out close the stream.

How should I resolve this? Two ideas:

In the onStreamClose callback check if the nghttp2 limit was reached and interrupt with a reset stream if so. (Not sure if this is possible or logical.)
Set the nghttp2 limit to something arbitrarily higher and solely rely on our own check, whose behaviour we're in more control of. I was thinking either the max configurable limit from envoy; though since that would have to be edited twice each time the config limit is changed, maybe the configured limit + 1 would work.

Thoughts?

aunu53 · 2019-02-13T16:34:27Z

Ahh, some more details. The library check triggers before any header bytes are dispatched to the library, so server_callbacks_.newStream() is never called (and so it's never reset, which is why my test fails). So yeah again, I could configure the library check up high as I suggested to never get hit, or we could decide that behaviour is useful.

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-02-13T16:57:23Z

OK so I went with an arbitrarily hard-coded nghttp2 max-header-len setting of 100kb. This can be easily changed or be configurable later, but this changes no envoy behaviour. (Both before and after this PR, the nghttp2 max check will not ever be triggered.)

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-02-13T20:23:05Z

/retest

repokitteh-read-only · 2019-02-13T20:23:09Z

🔨 rebuilding ci/circleci: mac (failed build)

🐱

Caused by: a #5859 (comment) was created by @auni53.

see: more, trace.

alyssawilk

Thanks for continuing to plug away at these!

source/common/http/http2/codec_impl.h

alyssawilk · 2019-02-13T20:31:36Z

source/common/http/http2/codec_impl.cc

  // of kept alive HTTP/2 connections.
  nghttp2_option_set_no_closed_streams(options_, 1);
  nghttp2_option_set_no_auto_window_update(options_, 1);
+  nghttp2_option_set_max_send_header_block_length(options_,


Isn't this the setting for the size of an individual header frame? I think we want to change the total allowed headers, not the per-frame limits.

IIRC this is not very well named, but I think this does actually set the header send limit, and the library will chunk it into frames using continuation frames if necessary. @auni53 to check me here.

Could be. I'm not seeing any other size-related fields to tweak. I'd checked
https://nghttp2.org/documentation/nghttp2_option_set_max_send_header_block_length.html
"This option sets the maximum length of header block (a set of header fields per one HEADERS frame) to send. "
It was the "per one HEADERS frame" that got me thinking it might be the wrong field.
in code it's used in prep_frame which I thought was after the chunking but I could easily be misremembering. I'm happy to let Auni do the code sleuthing though :-)

Yeeee, I'm pretty certain this is the right call. Also because I did this TDD-style and the testing behaviour checks out.

test/common/http/http1/codec_impl_test.cc

alyssawilk · 2019-02-13T20:34:09Z

test/common/http/http2/codec_impl_test.cc

+  request_encoder_->encodeHeaders(request_headers, true);
+}
+
+TEST_P(Http2CodecImplTest, TestSingleLargeHeadersWayOverDefaultCodecLimit) {


If your goal is to test an unexpectedly large header frame, you could have several keys with fairly long strings.
It makes sense to both limit the size of overall headers as well as the size of any individual header, and I think rejecting a single header key or value of 65536 bytes is totally reasonable :-)

Already had a test with several long strings! But I rearranged/reworded this stuff. Discussed offline, and I'm going to turn up this test to fail on single-large-header and document the nghttp2 limit in the max_request_headers_kb api.

mattklein123

Thanks for working on this, some comments/questions.

/wait

mattklein123 · 2019-02-13T21:16:11Z

source/common/http/http2/codec_impl.cc

  // of kept alive HTTP/2 connections.
  nghttp2_option_set_no_closed_streams(options_, 1);
  nghttp2_option_set_no_auto_window_update(options_, 1);
+  nghttp2_option_set_max_send_header_block_length(options_,


IIRC this is not very well named, but I think this does actually set the header send limit, and the library will chunk it into frames using continuation frames if necessary. @auni53 to check me here.

source/common/http/http2/codec_impl.h

test/common/http/http1/codec_impl_test.cc

Signed-off-by: Auni Ahsan <auni@google.com>

This reverts commit 999b247. Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-02-19T14:39:24Z

Hi @mattklein123, working on some changes to this. I'm having an issue with the http codec configuration I could use some input on.

Basically, to set the http parser limits you need to call http_parser_set_max_header_size(x). What I'm finding from my testing is that this setting persists across test runs, i.e. making that call in one http1/codec_impl test affects the success of a different test. I had thought previously that each envoy codec would be instantiating its http parser separately, but it looks like these settings are global. Does that make sense?

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-03-04T17:03:40Z

Oky I changed the ClientConnectionImpl max response headers from a default parameter to a class constant, added a test for large response headers rejection, and tweaked the API comment.

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-03-04T18:08:25Z

Ok, updated with Matt's suggestion on the API commenting + added a note on nghttp2 to the test file. I got a mysql_integration_test failure, hoping that was a flake bc idk how I'd have touched that; will look further into it if that persists.

alyssawilk · 2019-03-04T18:23:47Z

Yep, that's a known and fixed flake. If you merge master you'll pick up the fix

Signed-off-by: Auni Ahsan <auni@google.com>

mattklein123

Thank you LGTM assuming it passes @alyssawilk muster!

alyssawilk

Looking great - just a few more minor nits from me and you should be good to go.

alyssawilk · 2019-03-04T21:45:02Z

source/common/http/http1/codec_impl.cc

  header_parsing_state_ = HeaderParsingState::Value;
  current_header_value_.append(data, length);
+
+  uint32_t total =


const uint32_t?

alyssawilk · 2019-03-04T21:47:52Z

source/common/http/http1/codec_impl.h

  // Set true between receiving 100-Continue headers and receiving the spurious onMessageComplete.
  bool ignore_message_complete_for_100_continue_{};
+
+  // The default limit of 80 KiB is the vanilla http_parser behaviour.


The default limit is 80Kb (Kilobytes) not KiB (Kibibytes), no?

Wow, I forgot kibibytes were a thing. In any case, I checked http_parser.h:63, looks like it's indeed KiB.

Oh jeez, trusted the http_parser issue which was incorrect - looks like they were saying KB when they meant KiB. You can ignore this one.

alyssawilk · 2019-03-04T21:50:55Z

bazel/external/http-parser.BUILD

        "http_parser.h",
    ],
    hdrs = ["http_parser.h"],
+    copts = ["-DHTTP_MAX_HEADER_SIZE=0x7fffffff"],


Actually sorry, on second thought I'm having a moment of paranoia. We don't have anywhere else in the Envoy code base where we use http_parser so this is 100% OK today but what do we think of adding a limit just in case? If we added a 200KB limit or some such it'd be very safely above our window such that we should never hit the http_parser limits in the codec, without allowing total infinite memory if someone else uses http parser.
Thoughts?

alyssawilk · 2019-03-04T21:54:35Z

source/common/http/http2/codec_impl.cc


+  // The max send header block length is configured to an arbitrarily high number so as to never
+  // trigger the check within nghttp2, as we check request headers length in codec_impl::saveHeader.
+  nghttp2_option_set_max_send_header_block_length(options_, 0x7fffffff);


Ditto here on limits

alyssawilk

Bah, click fail

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-03-05T18:13:58Z

/retest

repokitteh-read-only · 2019-03-05T18:14:02Z

🔨 rebuilding ci/circleci: asan (failed build)

🐱

Caused by: a #5859 (comment) was created by @auni53.

see: more, trace.

Bump up max configurable max_request_headers_kb to 96 KiB. Add a check to http1/codec_impl.cc for headers size. Raise the default library limits in http_parser nghttp2 so we'll rely on our own codec check. Risk Level: Medium. Testing: Moved all the large request headers tests to ProtocolIntegrationTest. Part of envoyproxy#5626. Signed-off-by: Auni Ahsan <auni@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

…iB (#15921) Commit Message: Raise max configurable max_request_headers_kb limit to 8192 KiB (8MiB) from 96 KiB in http connection manager protobuf. Additional Description: Added/Updated relevant unit tests and integration tests. This change will allow increasing max_request_headers_kb to 8MiB from http connection manager's configuration. The change is mainly updating the limit in a validation check in the protobuf. Also, the old (merged) PR #5859 is read, no nghttp2 library-related issues are observed on raising the max_request_headers_kb beyond 96 KiB. Risk Level: Low Testing: Unit, Integration, Manual Docs Changes: Inline in proto file. Signed-off-by: Anirudha Singh <ps.anirudha@gmail.com>

…iB (envoyproxy#15921) Commit Message: Raise max configurable max_request_headers_kb limit to 8192 KiB (8MiB) from 96 KiB in http connection manager protobuf. Additional Description: Added/Updated relevant unit tests and integration tests. This change will allow increasing max_request_headers_kb to 8MiB from http connection manager's configuration. The change is mainly updating the limit in a validation check in the protobuf. Also, the old (merged) PR envoyproxy#5859 is read, no nghttp2 library-related issues are observed on raising the max_request_headers_kb beyond 96 KiB. Risk Level: Low Testing: Unit, Integration, Manual Docs Changes: Inline in proto file. Signed-off-by: Anirudha Singh <ps.anirudha@gmail.com> Signed-off-by: Gokul Nair <gnair@twitter.com>

ramaraochavali · 2023-04-11T06:28:58Z

@mattklein123 @alyssawilk Sorry to tag on a old issue. Since it has context - Couple of questions

Would it be OK to make the default value read from a runtime config instead of hardcoding it to 60Kib?
If we introduce a runtime config - Can the default of the runtime config be changed to a higher value? What would be the considerations (security , memory etc) for choosing that default in Envoy?

mattklein123 · 2023-04-11T15:21:46Z

cc @RyanTheOptimist @yanavlasov @KBaichoo also on ^

RyanTheOptimist · 2023-04-11T20:33:11Z

@ramaraochavali Can you file an issue about there were we can discuss the details? I think that's probably better than a 4 year old already committed PR :) Please feel free to link to this PR or any related issues there.

ramaraochavali · 2023-04-12T07:24:15Z

@RyanTheOptimist created #26690

aunu53 changed the title ~~codec: Configure codec library max request header limits~~ codec: Apply max request header limits settings to codec libraries' configurations Feb 6, 2019

Configure codec library max request header limits

fdd1442

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 force-pushed the maxer branch from 7d09b11 to fdd1442 Compare February 6, 2019 14:45

Add hd to dictionary

932eaf0

Signed-off-by: Auni Ahsan <auni@google.com>

htuch assigned alyssawilk Feb 6, 2019

htuch requested a review from alyssawilk February 6, 2019 15:35

Undo http1 codec changes

999b247

Signed-off-by: Auni Ahsan <auni@google.com>

set http_parser limit in test

6e2e092

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 force-pushed the maxer branch from 3c70ec9 to 6e2e092 Compare February 6, 2019 19:30

repokitteh-read-only bot added the waiting label Feb 7, 2019

mattklein123 self-assigned this Feb 7, 2019

Remove http2options for nghttp2 check

1ba5e19

Signed-off-by: Auni Ahsan <auni@google.com>

repokitteh-read-only bot removed the waiting label Feb 13, 2019

Fix format

51e8fb7

Signed-off-by: Auni Ahsan <auni@google.com>

alyssawilk reviewed Feb 13, 2019

View reviewed changes

mattklein123 requested changes Feb 13, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 13, 2019

aunu53 added 5 commits February 14, 2019 12:03

add comment

37d6900

Signed-off-by: Auni Ahsan <auni@google.com>

Clean up codec tests and bump limit to 96

8c1be4a

Signed-off-by: Auni Ahsan <auni@google.com>

Merge remote-tracking branch 'envoy/master' into maxer

d3e146d

Signed-off-by: Auni Ahsan <auni@google.com>

Add protocol tests

d14f019

Signed-off-by: Auni Ahsan <auni@google.com>

Revert "Undo http1 codec changes"

7fae643

This reverts commit 999b247. Signed-off-by: Auni Ahsan <auni@google.com>

repokitteh-read-only bot removed the waiting label Mar 4, 2019

pr comments

9a20bb5

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 added 2 commits March 4, 2019 12:15

fix fuzz test

bfbeed4

Signed-off-by: Auni Ahsan <auni@google.com>

update comments

e28fcdb

Signed-off-by: Auni Ahsan <auni@google.com>

Merge remote-tracking branch 'envoy/master' into maxer

be85a3f

Signed-off-by: Auni Ahsan <auni@google.com>

mattklein123 previously approved these changes Mar 4, 2019

View reviewed changes

alyssawilk approved these changes Mar 4, 2019

View reviewed changes

alyssawilk suggested changes Mar 4, 2019

View reviewed changes

mattklein123 added the waiting label Mar 5, 2019

pr change

b5ba8ef

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 dismissed mattklein123’s stale review via b5ba8ef March 5, 2019 17:04

repokitteh-read-only bot removed the waiting label Mar 5, 2019

alyssawilk approved these changes Mar 5, 2019

View reviewed changes

alyssawilk merged commit df3d47f into envoyproxy:master Mar 5, 2019

aunu53 deleted the maxer branch March 10, 2020 21:06

esmet mentioned this pull request Dec 10, 2020

Question: Should the maximum size of response headers be configurable? #14370

Closed

anirudhaps mentioned this pull request Apr 12, 2021

http connection manager proto: raise max_request_headers_kb limit to 8192 KiB (8MiB) from 96 KiB #15921

Merged

ramaraochavali mentioned this pull request Apr 12, 2023

Allow max header size configurable via runtime config #26690

Closed

Conversation

aunu53 commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aunu53 commented Feb 6, 2019

Uh oh!

alyssawilk commented Feb 7, 2019

Uh oh!

aunu53 commented Feb 13, 2019

Uh oh!

aunu53 commented Feb 13, 2019

Uh oh!

aunu53 commented Feb 13, 2019

Uh oh!

aunu53 commented Feb 13, 2019

Uh oh!

repokitteh-read-only bot commented Feb 13, 2019

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aunu53 commented Feb 19, 2019

Uh oh!

aunu53 commented Mar 4, 2019

Uh oh!

aunu53 commented Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Mar 4, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk 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!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

aunu53 commented Mar 5, 2019

Uh oh!

repokitteh-read-only bot commented Mar 5, 2019

Uh oh!

ramaraochavali commented Apr 11, 2023

Uh oh!

aunu53 commented Feb 6, 2019 •

edited

Loading

aunu53 commented Mar 4, 2019 •

edited

Loading