Envoy: http/1.1: make HTTP_MAX_HEADER_SIZE configurable

[javiervazquezh]

Title: http/1.1: make HTTP_MAX_HEADER_SIZE configurable

Description:

http_parser v2.9.0 supports changing the the max header size at runtime. It'd be great to make this value configurable so it can be increased/decreased without having to fork the library.

[optional Relevant Links:]

https://github.com/nodejs/http-parser/commit/0ae8d93f7335c0279f37b5ca5c26ea881ac17586

[alyssawilk]

Conveniently Auni was about to start working on this, so I'm assigning it his way :-)

[auni53]

Just started on this today, you're in luck.

TODO, apply option to...

• [ ] envoy check in conn_manager
• [ ] nghttp2
• [ ] http_parser

[auni53]

@alyssawilk @mattklein123, cc @AndresGuedez

Need some help debugging a weird case. This is a little bit convoluted so let me try and be thorough.

The current nghttp2 max header limit is 64K, as set by "NGHTTP2_MAX_HEADERSLEN". I confirmed that sending a request whose size exceeds 64K does indeed trigger that check, and closes the stream with error code 0x07 (NGHTTP2_REFUSED_STREAM). I confirmed that sending a request with ~62K of headers does not trigger any errors and the request is decoded.

HOWEVER, sending a request with a byte size of ~63K fails and resets the stream, with a different error. Instead of getting 0x07, I get 0x02 (NGHTTP2_INTERNAL_ERROR). Through trial and error, I determined this internal error is caused by the check at nghttp2_session.c:6036, with this weird-as-hell NGHTTP2_ERR_TEMPORAL_CALLBACK that I don't understand. This occurs with both a single 63K header, as well as with 63 separate 1K headers.

Furthermore, when I disable the correct max header blocks length check (which returns 0x07) and send a 64K request, I expect the codec to correctly process it. Instead, it exhibits the same behaviour as the 63K check with a 0x02 error and the temporal callback stuff.

Sooooo basically, there's some other check that's being triggered by requests that are under the indicated max request headers limit. The call stack in nghttp2 isn't trivially interpretable so I'm very confused with respect to diagnosing where this error is coming from. Andres also suggests that it could be an issue with how Envoy is invoking the nghttp2 library, and not the library itself.

Thoughts?

[mattklein123]

@auni53 sorry would need to debug to see what is going on. I don't think it's failing at nghttp2_session.c:6036 as that is in the push promise handling code which we aren't using. Can you potentially provide a callstack of the failure?

[auni53]

Test I added to http2/codec_impl_test.cc:

TEST_P(Http2CodecImplTest, TestLargeRequestSingleLargeHeaderUnderLimitAccepted) {
  initialize();                                                                  

  TestHeaderMapImpl request_headers;                                             
  HttpTestUtility::addDefaultHeaders(request_headers);                           
  std::string long_string = std::string(63 * 1024, 'q');                         
  request_headers.addCopy("big", long_string);                                   

  EXPECT_CALL(server_stream_callbacks_, onResetStream(_)).Times(0);              
  request_encoder_->encodeHeaders(request_headers, true);                        
}                                                                                

(call stack and test output coming up)

[mattklein123]

Anyway I think you are hitting this logic block: https://github.com/envoyproxy/envoy/blob/master/source/common/http/http2/codec_impl.cc#L647, and we are failing during receiving a header frame.

[javiervazquezh]

By reading #5654 it seems like there is a possibility to cap the header size to 64Kb.

Our use case requires to up the limit to 100Kb, it'd be great to not limit the header size to 64Kb. Is it possible to set the knob default value to 64KB and leave it to the user decide the actual max size?

[auni53]

The limit is constrained by the codec limits, so it wouldn't work correctly even if the user bumped it up. I'm working on bumping up the codec limits after this though, so you'll be able to bump it pass past 100kb once i'm done with all of my work.

[auni53]

TODO(me): pass the configured limits onto the individual codec checks.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.