Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception. #12351

sbordet · 2024-10-07T21:56:03Z

Introduced "maxResponseHeadersSize" parameter to AbstractProxyServlet.
Introduced HttpGenerator.maxResponseHeadersSize and added checks to not exceed it.
Fixed HttpParser to generate a 400 in case HttpParser.maxHeaderBytes are exceeded for a response.
HTTP2Flusher now resets streams in case of failures.
Removed cases in HTTP2Session where a GOAWAY frame was generated with lastStreamId=0. GOAWAY.lastStreamId=0 means that not a single request was processed by the server, which was obviously incorrect.
Written tests for both ProxyHandler and ProxyServlet about max response headers size exceeded.

…el clients. Fixed initialization for HTTP/2 and HTTP/3. Signed-off-by: Simone Bordet <[email protected]>

Signed-off-by: Simone Bordet <[email protected]>

…licit. Signed-off-by: Simone Bordet <[email protected]>

…nternally it throws "Response header too large" exception. * Introduced "maxResponseHeadersSize" parameter to AbstractProxyServlet. * Introduced HttpGenerator.maxResponseHeadersSize and added checks to not exceed it. * Fixed HttpParser to generate a 400 in case HttpParser.maxHeaderBytes are exceeded for a response. * HTTP2Flusher now resets streams in case of failures. * Removed cases in HTTP2Session where a GOAWAY frame was generated with lastStreamId=0. GOAWAY.lastStreamId=0 means that not a single request was processed by the server, which was obviously incorrect. * Written tests for both ProxyHandler and ProxyServlet about max response headers size exceeded. Signed-off-by: Simone Bordet <[email protected]>

lorban · 2024-10-08T08:24:29Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java

    private boolean _needCRLF = false;
+    private int _maxHeaderBytes;


I wonder if 0 is a sensible default, even if we always call the setter before using the generator.

We did not check for response headers max size before, so 0 keeps the original behavior.

lorban · 2024-10-08T08:29:49Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java

+                if (_requestParser)
+                    throw new BadMessageException(header ? HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431 : HttpStatus.PAYLOAD_TOO_LARGE_413);
+                // There is no equivalent of 431 for response headers.
+                throw new BadMessageException(HttpStatus.BAD_REQUEST_400, "Response Header Fields Too Large");


It's not a 400 error either as 4xx codes indicate client errors but here it really is a server error. 500 looks like a more correct response code IMHO.

Unfortunately BadMessageException only allows 4xx error codes.

I'll check if we can throw a different exception.

It is possible to throw a different exception, HttpException.RuntimeException, but this is a client parsing a response: should it really generate a 500 Internal Server Error (emphasis on "Server")?

The server could generate a large response; it's the client that cannot parse it, so it's not technically a server error in HTTP/1.1 where the client cannot inform the server about how much it can parse.

@gregw thoughts?

lorban · 2024-10-08T08:34:59Z

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java

@@ -776,7 +778,7 @@ public Action process() throws Exception
                    case HEADER_OVERFLOW:
                    {
                        if (_header.capacity() >= getHttpConfiguration().getResponseHeaderSize())
-                            throw new HttpException.RuntimeException(INTERNAL_SERVER_ERROR_500, "Response header too large");
+                            throw new HttpException.RuntimeException(INTERNAL_SERVER_ERROR_500, "Response Header Fields Too Large");


Here, the status is (IMHO correctly) set to 500 when headers are too large, unlike H2.

Not sure what you mean here? H2 does not generate an HTTP response (it just fails the connection).

I wrote H2, I meant the parser. Silly me.

lorban · 2024-10-08T08:38:41Z

jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ReverseProxyTest.java

+            .timeout(5, TimeUnit.SECONDS)
+            .send();
+
+//        assertTrue(serverToProxyFailureLatch.await(5, TimeUnit.SECONDS));


Delete this comment?

It should be restored!

Signed-off-by: Simone Bordet <[email protected]>

…se-headers-too-large'. Signed-off-by: Simone Bordet <[email protected]>

gregw

Why not allocate a headers buffer of the appropriate size instead of adding a new counter?

gregw · 2024-10-29T05:14:19Z

jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java

+    public int getMaxHeaderBytes()
+    {
+        return _maxHeaderBytes;
+    }
+
+    public void setMaxHeaderBytes(int maxHeaderBytes)
+    {
+        _maxHeaderBytes = maxHeaderBytes;
+    }


This is kind of a duplicate in the server as we already have HttpConfiguration.getResponseHeaderSize(), and we have use overflowing of that buffer to enforce a header size.

So why are we duplicating this mechanism? Can't the client just allocate a buffer of a specific size when it is asked for a HEADERs buffer?

lorban

Besides wiring HttpConfiguration.getResponseHeaderSize() like Greg suggested, LGTM.

sbordet · 2024-10-29T09:04:26Z

Why not allocate a headers buffer of the appropriate size instead of adding a new counter?

Because we cannot rely on buffer capacity to enforce max header size.
When a buffer is acquired from the pool, its capacity is rounded up to the next bucket.

So If I configure maxHeaderBytes=1200, I'm actually able to stuff up to 2048 bytes, which breaks HTTP/2 and HTTP/3 clients. This is because it is the client telling the server how much the client can receive via a SETTINGS frame.

So we need a mechanism that is independent of the buffer capacity.

gregw · 2024-10-29T09:12:54Z

So we need a mechanism that is independent of the buffer capacity.

I agree that h2/h3 need greater accuracy than can be achieved with a buffer from a pool. I also see you have wired up the HttpConfiguration to the generator, so I think that is OK.... although if the buffer size is exact, it will probably overflow before the limit check is applied.

Signed-off-by: Simone Bordet <[email protected]>

…se-headers-too-large'. Signed-off-by: Simone Bordet <[email protected]>

sbordet added 4 commits October 4, 2024 18:24

Fixes #12348 - HttpClientTransportDynamic does not initialize low-lev…

3f903e2

…el clients. Fixed initialization for HTTP/2 and HTTP/3. Signed-off-by: Simone Bordet <[email protected]>

Updates after review.

e5a672c

Signed-off-by: Simone Bordet <[email protected]>

Fixed test that was relying on default configuration, rather than exp…

53dff62

…licit. Signed-off-by: Simone Bordet <[email protected]>

sbordet requested review from gregw and lorban October 7, 2024 21:56

sbordet linked an issue Oct 7, 2024 that may be closed by this pull request

AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception #5685

Closed

sbordet mentioned this pull request Oct 7, 2024

AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception #5685

Closed

lorban requested changes Oct 8, 2024

View reviewed changes

sbordet added 2 commits October 8, 2024 11:08

Updates from review.

af31788

Signed-off-by: Simone Bordet <[email protected]>

Updates from review.

8a8ba79

Signed-off-by: Simone Bordet <[email protected]>

sbordet requested a review from lorban October 8, 2024 17:13

Base automatically changed from fix/jetty-12.0.x/12348/httpclienttransportdynamic-config to jetty-12.0.x October 10, 2024 15:03

Merged branch 'jetty-12.0.x' into 'fix/jetty-12.0.x/5685/proxy-respon…

e5a89c3

…se-headers-too-large'. Signed-off-by: Simone Bordet <[email protected]>

gregw requested changes Oct 29, 2024

View reviewed changes

lorban requested changes Oct 29, 2024

View reviewed changes

sbordet requested review from lorban and gregw October 29, 2024 09:04

Simplified server-side response header allocation for HTTP/1.1.

92c516e

Signed-off-by: Simone Bordet <[email protected]>

gregw approved these changes Oct 29, 2024

View reviewed changes

lorban approved these changes Oct 30, 2024

View reviewed changes

Merged branch 'jetty-12.0.x' into 'fix/jetty-12.0.x/5685/proxy-respon…

9fb5bd8

…se-headers-too-large'. Signed-off-by: Simone Bordet <[email protected]>

sbordet merged commit 85ce152 into jetty-12.0.x Oct 30, 2024
2 of 4 checks passed

sbordet deleted the fix/jetty-12.0.x/5685/proxy-response-headers-too-large branch October 30, 2024 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception. #12351

Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception. #12351

sbordet commented Oct 7, 2024

lorban Oct 8, 2024

sbordet Oct 8, 2024

lorban Oct 8, 2024

sbordet Oct 8, 2024

sbordet Oct 8, 2024

lorban Oct 8, 2024

sbordet Oct 8, 2024

lorban Oct 8, 2024

lorban Oct 8, 2024

sbordet Oct 8, 2024

gregw left a comment

gregw Oct 29, 2024

lorban left a comment

sbordet commented Oct 29, 2024

gregw commented Oct 29, 2024

		private boolean _needCRLF = false;
		private int _maxHeaderBytes;

Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception. #12351

Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when internally it throws "Response header too large" exception. #12351

Conversation

sbordet commented Oct 7, 2024

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

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorban left a comment

Choose a reason for hiding this comment

sbordet commented Oct 29, 2024

gregw commented Oct 29, 2024