From 3f903e2af98351ac03069df7bcec294dca0f7a7c Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 4 Oct 2024 17:47:54 +0200 Subject: [PATCH 1/7] Fixes #12348 - HttpClientTransportDynamic does not initialize low-level clients. Fixed initialization for HTTP/2 and HTTP/3. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/client/HttpClient.java | 9 +++ .../jetty/client/HttpClientTransport.java | 3 +- .../transport/HttpClientTransportDynamic.java | 11 ++++ .../ClientConnectionFactoryOverHTTP2.java | 18 +++++- .../HttpClientTransportOverHTTP2.java | 29 +++++----- .../HttpClientTransportOverHTTP2Test.java | 24 +++++++- .../ClientConnectionFactoryOverHTTP3.java | 20 ++++++- .../HttpClientTransportOverHTTP3.java | 33 ++++++----- .../HttpClientTransportOverHTTP3Test.java | 57 +++++++++++++++++++ 9 files changed, 168 insertions(+), 36 deletions(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java index b6c40a2eb1da..dd06d78a5459 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java @@ -1147,4 +1147,13 @@ public void close() throws Exception { stop(); } + + /** + *

Implementations of this interface are made aware of + * the {@code HttpClient} instance while it is starting.

+ */ + public interface Aware + { + void setHttpClient(HttpClient httpClient); + } } diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClientTransport.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClientTransport.java index 45451fa2ead9..1867c7285c5d 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClientTransport.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClientTransport.java @@ -31,7 +31,7 @@ * but the HTTP exchange may also be carried using the FCGI protocol, the HTTP/2 protocol or, * in future, other protocols. */ -public interface HttpClientTransport extends ClientConnectionFactory +public interface HttpClientTransport extends ClientConnectionFactory, HttpClient.Aware { public static final String HTTP_DESTINATION_CONTEXT_KEY = "org.eclipse.jetty.client.destination"; public static final String HTTP_CONNECTION_PROMISE_CONTEXT_KEY = "org.eclipse.jetty.client.connection.promise"; @@ -45,6 +45,7 @@ public interface HttpClientTransport extends ClientConnectionFactory * * @param client the {@link HttpClient} that uses this transport. */ + @Override public void setHttpClient(HttpClient client); /** diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpClientTransportDynamic.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpClientTransportDynamic.java index 31f2d5fbcf3e..b9457e1b5bae 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpClientTransportDynamic.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpClientTransportDynamic.java @@ -25,6 +25,7 @@ import org.eclipse.jetty.alpn.client.ALPNClientConnectionFactory; import org.eclipse.jetty.client.AbstractConnectorHttpClientTransport; import org.eclipse.jetty.client.Destination; +import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.HttpClientTransport; import org.eclipse.jetty.client.MultiplexConnectionPool; import org.eclipse.jetty.client.Origin; @@ -129,6 +130,16 @@ private static ClientConnector findClientConnector(ClientConnectionFactory.Info[ .orElseGet(ClientConnector::new); } + @Override + public void setHttpClient(HttpClient client) + { + super.setHttpClient(client); + infos.stream() + .filter(info -> info instanceof HttpClient.Aware) + .map(HttpClient.Aware.class::cast) + .forEach(info -> info.setHttpClient(getHttpClient())); + } + @Override public Origin newOrigin(Request request) { diff --git a/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/ClientConnectionFactoryOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/ClientConnectionFactoryOverHTTP2.java index 2958bf141ced..fde87e53cbd0 100644 --- a/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/ClientConnectionFactoryOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/ClientConnectionFactoryOverHTTP2.java @@ -19,6 +19,7 @@ import java.util.Map; import org.eclipse.jetty.client.Connection; +import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.HttpClientTransport; import org.eclipse.jetty.client.transport.HttpClientConnectionFactory; import org.eclipse.jetty.client.transport.HttpClientTransportDynamic; @@ -35,7 +36,7 @@ import org.eclipse.jetty.util.Promise; import org.eclipse.jetty.util.component.ContainerLifeCycle; -public class ClientConnectionFactoryOverHTTP2 extends ContainerLifeCycle implements ClientConnectionFactory +public class ClientConnectionFactoryOverHTTP2 extends ContainerLifeCycle implements ClientConnectionFactory, HttpClient.Aware { private final ClientConnectionFactory factory = new HTTP2ClientConnectionFactory(); private final HTTP2Client http2Client; @@ -46,6 +47,12 @@ public ClientConnectionFactoryOverHTTP2(HTTP2Client http2Client) installBean(http2Client); } + @Override + public void setHttpClient(HttpClient httpClient) + { + HttpClientTransportOverHTTP2.configure(httpClient, http2Client); + } + @Override public org.eclipse.jetty.io.Connection newConnection(EndPoint endPoint, Map context) throws IOException { @@ -61,7 +68,7 @@ public org.eclipse.jetty.io.Connection newConnection(EndPoint endPoint, Map protocols = List.of("h2", "h2c"); private static final List h2c = List.of("h2c"); @@ -71,6 +78,13 @@ public HTTP2(HTTP2Client http2Client) super(new ClientConnectionFactoryOverHTTP2(http2Client)); } + @Override + public void setHttpClient(HttpClient httpClient) + { + ClientConnectionFactoryOverHTTP2 factory = (ClientConnectionFactoryOverHTTP2)getClientConnectionFactory(); + factory.setHttpClient(httpClient); + } + @Override public List getProtocols(boolean secure) { diff --git a/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/HttpClientTransportOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/HttpClientTransportOverHTTP2.java index a5178b522418..9cb8b7cc8d21 100644 --- a/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/HttpClientTransportOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/HttpClientTransportOverHTTP2.java @@ -87,22 +87,25 @@ public void setUseALPN(boolean useALPN) protected void doStart() throws Exception { if (!http2Client.isStarted()) - { - HttpClient httpClient = getHttpClient(); - http2Client.setExecutor(httpClient.getExecutor()); - http2Client.setScheduler(httpClient.getScheduler()); - http2Client.setByteBufferPool(httpClient.getByteBufferPool()); - http2Client.setConnectTimeout(httpClient.getConnectTimeout()); - http2Client.setIdleTimeout(httpClient.getIdleTimeout()); - http2Client.setInputBufferSize(httpClient.getResponseBufferSize()); - http2Client.setUseInputDirectByteBuffers(httpClient.isUseInputDirectByteBuffers()); - http2Client.setUseOutputDirectByteBuffers(httpClient.isUseOutputDirectByteBuffers()); - http2Client.setConnectBlocking(httpClient.isConnectBlocking()); - http2Client.setBindAddress(httpClient.getBindAddress()); - } + configure(getHttpClient(), getHTTP2Client()); super.doStart(); } + static void configure(HttpClient httpClient, HTTP2Client http2Client) + { + http2Client.setExecutor(httpClient.getExecutor()); + http2Client.setScheduler(httpClient.getScheduler()); + http2Client.setByteBufferPool(httpClient.getByteBufferPool()); + http2Client.setConnectTimeout(httpClient.getConnectTimeout()); + http2Client.setIdleTimeout(httpClient.getIdleTimeout()); + http2Client.setInputBufferSize(httpClient.getResponseBufferSize()); + http2Client.setUseInputDirectByteBuffers(httpClient.isUseInputDirectByteBuffers()); + http2Client.setUseOutputDirectByteBuffers(httpClient.isUseOutputDirectByteBuffers()); + http2Client.setConnectBlocking(httpClient.isConnectBlocking()); + http2Client.setBindAddress(httpClient.getBindAddress()); + http2Client.setMaxResponseHeadersSize(httpClient.getMaxResponseHeadersSize()); + } + @Override public Origin newOrigin(Request request) { diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HttpClientTransportOverHTTP2Test.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HttpClientTransportOverHTTP2Test.java index 2d5254ad0956..43cf76a981f5 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HttpClientTransportOverHTTP2Test.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HttpClientTransportOverHTTP2Test.java @@ -40,11 +40,13 @@ import org.eclipse.jetty.client.ContentResponse; import org.eclipse.jetty.client.Destination; import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.HttpClientTransport; import org.eclipse.jetty.client.HttpProxy; import org.eclipse.jetty.client.InputStreamResponseListener; import org.eclipse.jetty.client.Origin; import org.eclipse.jetty.client.Response; import org.eclipse.jetty.client.Result; +import org.eclipse.jetty.client.transport.HttpClientTransportDynamic; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; @@ -60,6 +62,7 @@ import org.eclipse.jetty.http2.api.Stream; import org.eclipse.jetty.http2.api.server.ServerSessionListener; import org.eclipse.jetty.http2.client.HTTP2Client; +import org.eclipse.jetty.http2.client.transport.ClientConnectionFactoryOverHTTP2; import org.eclipse.jetty.http2.client.transport.HttpClientTransportOverHTTP2; import org.eclipse.jetty.http2.client.transport.internal.HttpChannelOverHTTP2; import org.eclipse.jetty.http2.client.transport.internal.HttpConnectionOverHTTP2; @@ -105,10 +108,24 @@ public class HttpClientTransportOverHTTP2Test extends AbstractTest { @Test - public void testPropertiesAreForwarded() throws Exception + public void testPropertiesAreForwardedOverHTTP2() throws Exception { - HTTP2Client http2Client = new HTTP2Client(); - try (HttpClient httpClient = new HttpClient(new HttpClientTransportOverHTTP2(http2Client))) + ClientConnector clientConnector = new ClientConnector(); + HTTP2Client http2Client = new HTTP2Client(clientConnector); + testPropertiesAreForwarded(http2Client, new HttpClientTransportOverHTTP2(http2Client)); + } + + @Test + public void testPropertiesAreForwardedDynamic() throws Exception + { + ClientConnector clientConnector = new ClientConnector(); + HTTP2Client http2Client = new HTTP2Client(clientConnector); + testPropertiesAreForwarded(http2Client, new HttpClientTransportDynamic(clientConnector, new ClientConnectionFactoryOverHTTP2.HTTP2(http2Client))); + } + + private void testPropertiesAreForwarded(HTTP2Client http2Client, HttpClientTransport httpClientTransport) throws Exception + { + try (HttpClient httpClient = new HttpClient(httpClientTransport)) { Executor executor = new QueuedThreadPool(); httpClient.setExecutor(executor); @@ -127,6 +144,7 @@ public void testPropertiesAreForwarded() throws Exception assertEquals(httpClient.getIdleTimeout(), http2Client.getIdleTimeout()); assertEquals(httpClient.isUseInputDirectByteBuffers(), http2Client.isUseInputDirectByteBuffers()); assertEquals(httpClient.isUseOutputDirectByteBuffers(), http2Client.isUseOutputDirectByteBuffers()); + assertEquals(httpClient.getMaxResponseHeadersSize(), http2Client.getMaxResponseHeadersSize()); } assertTrue(http2Client.isStopped()); } diff --git a/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/ClientConnectionFactoryOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/ClientConnectionFactoryOverHTTP3.java index a27ee788578f..75ab0b7f0e33 100644 --- a/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/ClientConnectionFactoryOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/ClientConnectionFactoryOverHTTP3.java @@ -16,6 +16,7 @@ import java.util.List; import java.util.Map; +import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.transport.HttpClientConnectionFactory; import org.eclipse.jetty.client.transport.HttpClientTransportDynamic; import org.eclipse.jetty.http3.client.HTTP3Client; @@ -29,15 +30,23 @@ import org.eclipse.jetty.quic.common.QuicSession; import org.eclipse.jetty.util.component.ContainerLifeCycle; -public class ClientConnectionFactoryOverHTTP3 extends ContainerLifeCycle implements ClientConnectionFactory +public class ClientConnectionFactoryOverHTTP3 extends ContainerLifeCycle implements ClientConnectionFactory, HttpClient.Aware { private final HTTP3ClientConnectionFactory factory = new HTTP3ClientConnectionFactory(); + private final HTTP3Client http3Client; public ClientConnectionFactoryOverHTTP3(HTTP3Client http3Client) { + this.http3Client = http3Client; installBean(http3Client); } + @Override + public void setHttpClient(HttpClient httpClient) + { + HttpClientTransportOverHTTP3.configure(httpClient, http3Client); + } + @Override public org.eclipse.jetty.io.Connection newConnection(EndPoint endPoint, Map context) { @@ -49,7 +58,7 @@ public org.eclipse.jetty.io.Connection newConnection(EndPoint endPoint, Map protocols = List.of("h3"); @@ -61,6 +70,13 @@ public HTTP3(HTTP3Client client) http3Client = client; } + @Override + public void setHttpClient(HttpClient httpClient) + { + ClientConnectionFactoryOverHTTP3 factory = (ClientConnectionFactoryOverHTTP3)getClientConnectionFactory(); + factory.setHttpClient(httpClient); + } + public HTTP3Client getHTTP3Client() { return http3Client; diff --git a/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/HttpClientTransportOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/HttpClientTransportOverHTTP3.java index 490ae2970f48..750d70caa8ae 100644 --- a/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/HttpClientTransportOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/HttpClientTransportOverHTTP3.java @@ -66,24 +66,27 @@ public HTTP3Client getHTTP3Client() protected void doStart() throws Exception { if (!http3Client.isStarted()) - { - HttpClient httpClient = getHttpClient(); - ClientConnector clientConnector = this.http3Client.getClientConnector(); - clientConnector.setExecutor(httpClient.getExecutor()); - clientConnector.setScheduler(httpClient.getScheduler()); - clientConnector.setByteBufferPool(httpClient.getByteBufferPool()); - clientConnector.setConnectTimeout(Duration.ofMillis(httpClient.getConnectTimeout())); - clientConnector.setConnectBlocking(httpClient.isConnectBlocking()); - clientConnector.setBindAddress(httpClient.getBindAddress()); - clientConnector.setIdleTimeout(Duration.ofMillis(httpClient.getIdleTimeout())); - HTTP3Configuration configuration = http3Client.getHTTP3Configuration(); - configuration.setInputBufferSize(httpClient.getResponseBufferSize()); - configuration.setUseInputDirectByteBuffers(httpClient.isUseInputDirectByteBuffers()); - configuration.setUseOutputDirectByteBuffers(httpClient.isUseOutputDirectByteBuffers()); - } + configure(getHttpClient(), http3Client); super.doStart(); } + static void configure(HttpClient httpClient, HTTP3Client http3Client) + { + ClientConnector clientConnector = http3Client.getClientConnector(); + clientConnector.setExecutor(httpClient.getExecutor()); + clientConnector.setScheduler(httpClient.getScheduler()); + clientConnector.setByteBufferPool(httpClient.getByteBufferPool()); + clientConnector.setConnectTimeout(Duration.ofMillis(httpClient.getConnectTimeout())); + clientConnector.setConnectBlocking(httpClient.isConnectBlocking()); + clientConnector.setBindAddress(httpClient.getBindAddress()); + clientConnector.setIdleTimeout(Duration.ofMillis(httpClient.getIdleTimeout())); + HTTP3Configuration configuration = http3Client.getHTTP3Configuration(); + configuration.setInputBufferSize(httpClient.getResponseBufferSize()); + configuration.setUseInputDirectByteBuffers(httpClient.isUseInputDirectByteBuffers()); + configuration.setUseOutputDirectByteBuffers(httpClient.isUseOutputDirectByteBuffers()); + configuration.setMaxResponseHeadersSize(httpClient.getMaxResponseHeadersSize()); + } + @Override public Origin newOrigin(Request request) { diff --git a/jetty-core/jetty-http3/jetty-http3-tests/src/test/java/org/eclipse/jetty/http3/tests/HttpClientTransportOverHTTP3Test.java b/jetty-core/jetty-http3/jetty-http3-tests/src/test/java/org/eclipse/jetty/http3/tests/HttpClientTransportOverHTTP3Test.java index a77146f6b25f..efb7c3442470 100644 --- a/jetty-core/jetty-http3/jetty-http3-tests/src/test/java/org/eclipse/jetty/http3/tests/HttpClientTransportOverHTTP3Test.java +++ b/jetty-core/jetty-http3/jetty-http3-tests/src/test/java/org/eclipse/jetty/http3/tests/HttpClientTransportOverHTTP3Test.java @@ -16,30 +16,87 @@ import java.nio.ByteBuffer; import java.util.Arrays; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import org.eclipse.jetty.client.ContentResponse; +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.client.HttpClientTransport; import org.eclipse.jetty.client.Response; +import org.eclipse.jetty.client.transport.HttpClientTransportDynamic; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpVersion; +import org.eclipse.jetty.http3.HTTP3Configuration; +import org.eclipse.jetty.http3.client.HTTP3Client; +import org.eclipse.jetty.http3.client.transport.ClientConnectionFactoryOverHTTP3; +import org.eclipse.jetty.http3.client.transport.HttpClientTransportOverHTTP3; +import org.eclipse.jetty.io.ClientConnector; import org.eclipse.jetty.io.Content; +import org.eclipse.jetty.quic.client.ClientQuicConfiguration; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.util.Callback; +import org.eclipse.jetty.util.ssl.SslContextFactory; +import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; public class HttpClientTransportOverHTTP3Test extends AbstractClientServerTest { + @Test + public void testPropertiesAreForwardedOverHTTP3() throws Exception + { + ClientConnector clientConnector = new ClientConnector(); + HTTP3Client http3Client = new HTTP3Client(new ClientQuicConfiguration(new SslContextFactory.Client(), null), clientConnector); + testPropertiesAreForwarded(http3Client, new HttpClientTransportOverHTTP3(http3Client)); + } + + @Test + public void testPropertiesAreForwardedDynamic() throws Exception + { + ClientConnector clientConnector = new ClientConnector(); + HTTP3Client http3Client = new HTTP3Client(new ClientQuicConfiguration(new SslContextFactory.Client(), null), clientConnector); + testPropertiesAreForwarded(http3Client, new HttpClientTransportDynamic(clientConnector, new ClientConnectionFactoryOverHTTP3.HTTP3(http3Client))); + } + + private void testPropertiesAreForwarded(HTTP3Client http3Client, HttpClientTransport httpClientTransport) throws Exception + { + try (HttpClient httpClient = new HttpClient(httpClientTransport)) + { + Executor executor = new QueuedThreadPool(); + httpClient.setExecutor(executor); + httpClient.setConnectTimeout(13); + httpClient.setIdleTimeout(17); + httpClient.setUseInputDirectByteBuffers(false); + httpClient.setUseOutputDirectByteBuffers(false); + + httpClient.start(); + + assertTrue(http3Client.isStarted()); + ClientConnector clientConnector = http3Client.getClientConnector(); + assertSame(httpClient.getExecutor(), clientConnector.getExecutor()); + assertSame(httpClient.getScheduler(), clientConnector.getScheduler()); + assertSame(httpClient.getByteBufferPool(), clientConnector.getByteBufferPool()); + assertEquals(httpClient.getConnectTimeout(), clientConnector.getConnectTimeout().toMillis()); + assertEquals(httpClient.getIdleTimeout(), clientConnector.getIdleTimeout().toMillis()); + HTTP3Configuration http3Configuration = http3Client.getHTTP3Configuration(); + assertEquals(httpClient.isUseInputDirectByteBuffers(), http3Configuration.isUseInputDirectByteBuffers()); + assertEquals(httpClient.isUseOutputDirectByteBuffers(), http3Configuration.isUseOutputDirectByteBuffers()); + assertEquals(httpClient.getMaxResponseHeadersSize(), http3Configuration.getMaxResponseHeadersSize()); + } + assertTrue(http3Client.isStopped()); + } + @Test public void testRequestHasHTTP3Version() throws Exception { From e5a672c6037a100035a010df9db4973cde401a2d Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sun, 6 Oct 2024 15:44:25 +0200 Subject: [PATCH 2/7] Updates after review. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/client/HttpClient.java | 6 ++-- .../transport/HttpClientTransportDynamic.java | 31 ++++++------------- .../ClientConnectionFactoryOverHTTP2.java | 9 +----- .../ClientConnectionFactoryOverHTTP3.java | 9 +----- .../jetty/io/ClientConnectionFactory.java | 2 +- .../util/component/ContainerLifeCycle.java | 4 +-- 6 files changed, 18 insertions(+), 43 deletions(-) diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java index dd06d78a5459..5592dd13bccd 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/HttpClient.java @@ -218,7 +218,7 @@ protected void doStart() throws Exception if (cookieStore == null) cookieStore = new HttpCookieStore.Default(); - transport.setHttpClient(this); + getContainedBeans(Aware.class).forEach(bean -> bean.setHttpClient(this)); super.doStart(); @@ -1149,8 +1149,8 @@ public void close() throws Exception } /** - *

Implementations of this interface are made aware of - * the {@code HttpClient} instance while it is starting.

+ *

Descendant beans of {@code HttpClient} that implement this interface + * are made aware of the {@code HttpClient} instance while it is starting.

*/ public interface Aware { diff --git a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpClientTransportDynamic.java b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpClientTransportDynamic.java index b9457e1b5bae..b52d262b4127 100644 --- a/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpClientTransportDynamic.java +++ b/jetty-core/jetty-client/src/main/java/org/eclipse/jetty/client/transport/HttpClientTransportDynamic.java @@ -25,7 +25,6 @@ import org.eclipse.jetty.alpn.client.ALPNClientConnectionFactory; import org.eclipse.jetty.client.AbstractConnectorHttpClientTransport; import org.eclipse.jetty.client.Destination; -import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.HttpClientTransport; import org.eclipse.jetty.client.MultiplexConnectionPool; import org.eclipse.jetty.client.Origin; @@ -82,7 +81,7 @@ public class HttpClientTransportDynamic extends AbstractConnectorHttpClientTrans { private static final Logger LOG = LoggerFactory.getLogger(HttpClientTransportDynamic.class); - private final List infos; + private final List clientConnectionFactoryInfos; /** * Creates a dynamic transport that speaks only HTTP/1.1. @@ -115,8 +114,8 @@ public HttpClientTransportDynamic(ClientConnectionFactory.Info... infos) public HttpClientTransportDynamic(ClientConnector connector, ClientConnectionFactory.Info... infos) { super(connector); - this.infos = infos.length == 0 ? List.of(HttpClientConnectionFactory.HTTP11) : List.of(infos); - this.infos.forEach(this::installBean); + this.clientConnectionFactoryInfos = infos.length == 0 ? List.of(HttpClientConnectionFactory.HTTP11) : List.of(infos); + this.clientConnectionFactoryInfos.forEach(this::installBean); setConnectionPoolFactory(destination -> new MultiplexConnectionPool(destination, destination.getHttpClient().getMaxConnectionsPerDestination(), 1) ); @@ -130,16 +129,6 @@ private static ClientConnector findClientConnector(ClientConnectionFactory.Info[ .orElseGet(ClientConnector::new); } - @Override - public void setHttpClient(HttpClient client) - { - super.setHttpClient(client); - infos.stream() - .filter(info -> info instanceof HttpClient.Aware) - .map(HttpClient.Aware.class::cast) - .forEach(info -> info.setHttpClient(getHttpClient())); - } - @Override public Origin newOrigin(Request request) { @@ -152,7 +141,7 @@ public Origin newOrigin(Request request) { HttpVersion version = request.getVersion(); List wanted = toProtocols(version); - for (Info info : infos) + for (Info info : clientConnectionFactoryInfos) { // Find the first protocol that matches the version. List protocols = info.getProtocols(secure); @@ -175,7 +164,7 @@ public Origin newOrigin(Request request) } else { - Info preferredInfo = infos.get(0); + Info preferredInfo = clientConnectionFactoryInfos.get(0); if (secure) { if (preferredInfo.getProtocols(true).contains("h3")) @@ -189,7 +178,7 @@ public Origin newOrigin(Request request) // If the preferred protocol is not HTTP/3, then // must be excluded since it won't be compatible // with the other HTTP versions due to UDP vs TCP. - for (Info info : infos) + for (Info info : clientConnectionFactoryInfos) { if (info.getProtocols(true).contains("h3")) continue; @@ -211,7 +200,7 @@ else if (matches == 1) else { // Pick the first that allows non-secure. - for (Info info : infos) + for (Info info : clientConnectionFactoryInfos) { if (info.getProtocols(false).contains("h3")) continue; @@ -260,7 +249,7 @@ public org.eclipse.jetty.io.Connection newConnection(EndPoint endPoint, Map findClientConnectionFactoryInfo(List protocols, boolean secure) { - return infos.stream() + return clientConnectionFactoryInfos.stream() .filter(info -> info.matches(protocols, secure)) .findFirst(); } diff --git a/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/ClientConnectionFactoryOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/ClientConnectionFactoryOverHTTP2.java index fde87e53cbd0..7c285a78ce07 100644 --- a/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/ClientConnectionFactoryOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-client-transport/src/main/java/org/eclipse/jetty/http2/client/transport/ClientConnectionFactoryOverHTTP2.java @@ -68,7 +68,7 @@ public org.eclipse.jetty.io.Connection newConnection(EndPoint endPoint, Map protocols = List.of("h2", "h2c"); private static final List h2c = List.of("h2c"); @@ -78,13 +78,6 @@ public HTTP2(HTTP2Client http2Client) super(new ClientConnectionFactoryOverHTTP2(http2Client)); } - @Override - public void setHttpClient(HttpClient httpClient) - { - ClientConnectionFactoryOverHTTP2 factory = (ClientConnectionFactoryOverHTTP2)getClientConnectionFactory(); - factory.setHttpClient(httpClient); - } - @Override public List getProtocols(boolean secure) { diff --git a/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/ClientConnectionFactoryOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/ClientConnectionFactoryOverHTTP3.java index 75ab0b7f0e33..6ffa5fab4078 100644 --- a/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/ClientConnectionFactoryOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-client-transport/src/main/java/org/eclipse/jetty/http3/client/transport/ClientConnectionFactoryOverHTTP3.java @@ -58,7 +58,7 @@ public org.eclipse.jetty.io.Connection newConnection(EndPoint endPoint, Map protocols = List.of("h3"); @@ -70,13 +70,6 @@ public HTTP3(HTTP3Client client) http3Client = client; } - @Override - public void setHttpClient(HttpClient httpClient) - { - ClientConnectionFactoryOverHTTP3 factory = (ClientConnectionFactoryOverHTTP3)getClientConnectionFactory(); - factory.setHttpClient(httpClient); - } - public HTTP3Client getHTTP3Client() { return http3Client; diff --git a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnectionFactory.java b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnectionFactory.java index 82a29c2cbffe..df3fd5dc47a7 100644 --- a/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnectionFactory.java +++ b/jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/ClientConnectionFactory.java @@ -73,7 +73,7 @@ public abstract static class Info extends ContainerLifeCycle public Info(ClientConnectionFactory factory) { this.factory = factory; - addBean(factory); + installBean(factory); } /** diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/ContainerLifeCycle.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/ContainerLifeCycle.java index 36fad2ee2e36..904cdad1b851 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/ContainerLifeCycle.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/component/ContainerLifeCycle.java @@ -19,7 +19,7 @@ import java.util.Collection; import java.util.Collections; import java.util.EventListener; -import java.util.HashSet; +import java.util.LinkedHashSet; import java.util.List; import java.util.Objects; import java.util.Set; @@ -882,7 +882,7 @@ public void updateBeans(final Collection oldBeans, final Collection newBea @Override public Collection getContainedBeans(Class clazz) { - Set beans = new HashSet<>(); + Set beans = new LinkedHashSet<>(); getContainedBeans(clazz, beans); return beans; } From 53dff628a95d509529abc4cdab97dd5ac8e005b4 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sun, 6 Oct 2024 17:20:41 +0200 Subject: [PATCH 3/7] Fixed test that was relying on default configuration, rather than explicit. Signed-off-by: Simone Bordet --- .../test/java/org/eclipse/jetty/http2/tests/HTTP2Test.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2Test.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2Test.java index 3593d0fcd6eb..b4a25d8f7710 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2Test.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2Test.java @@ -1068,6 +1068,7 @@ public void onClose(Session session, GoAwayFrame frame, Callback callback) @Test public void testServerSendsLargeHeader() throws Exception { + int maxResponseHeadersSize = 8 * 1024; CompletableFuture serverFailureFuture = new CompletableFuture<>(); CompletableFuture serverCloseReasonFuture = new CompletableFuture<>(); start(new ServerSessionListener() @@ -1076,9 +1077,9 @@ public void testServerSendsLargeHeader() throws Exception public Stream.Listener onNewStream(Stream stream, HeadersFrame frame) { HTTP2Session session = (HTTP2Session)stream.getSession(); - session.getGenerator().getHpackEncoder().setMaxHeaderListSize(1024 * 1024); + session.getGenerator().getHpackEncoder().setMaxHeaderListSize(2 * maxResponseHeadersSize); - String value = "x".repeat(8 * 1024); + String value = "x".repeat(maxResponseHeadersSize); HttpFields fields = HttpFields.build().put("custom", value); MetaData.Response response = new MetaData.Response(HttpStatus.OK_200, null, HttpVersion.HTTP_2, fields); stream.headers(new HeadersFrame(stream.getId(), response, null, true)); @@ -1100,6 +1101,7 @@ public void onClose(Session session, GoAwayFrame frame, Callback callback) } }); + http2Client.setMaxResponseHeadersSize(maxResponseHeadersSize); CompletableFuture clientFailureFuture = new CompletableFuture<>(); CompletableFuture clientCloseReasonFuture = new CompletableFuture<>(); Session.Listener listener = new Session.Listener() From ad00741f1782a0a93600523ac05dc173b5c9de17 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 7 Oct 2024 23:54:32 +0200 Subject: [PATCH 4/7] Fixes #5685 - AsyncProxyServlet calls onProxyResponseSuccess() when internally 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 --- .../org/eclipse/jetty/http/HttpGenerator.java | 65 ++-- .../org/eclipse/jetty/http/HttpParser.java | 10 +- .../org/eclipse/jetty/http2/HTTP2Session.java | 30 +- .../org/eclipse/jetty/http2/HTTP2Stream.java | 6 + .../jetty/http2/internal/HTTP2Flusher.java | 82 +++-- .../AbstractHTTP2ServerConnectionFactory.java | 1 + .../jetty/proxy/AbstractProxyTest.java | 19 +- .../eclipse/jetty/proxy/ReverseProxyTest.java | 309 +++++++++++++++++- .../jetty/server/internal/HttpConnection.java | 6 +- .../eclipse/jetty/server/LargeHeaderTest.java | 2 +- .../ee10/proxy/AbstractProxyServlet.java | 13 +- .../jetty/ee10/proxy/ProxyServletTest.java | 78 ++++- 12 files changed, 510 insertions(+), 111 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java index 595fb5f5df5e..f59f3005525e 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java @@ -41,9 +41,14 @@ public class HttpGenerator private static final Logger LOG = LoggerFactory.getLogger(HttpGenerator.class); public static final boolean __STRICT = Boolean.getBoolean("org.eclipse.jetty.http.HttpGenerator.STRICT"); - private static final byte[] __colon_space = new byte[]{':', ' '}; public static final MetaData.Response CONTINUE_100_INFO = new MetaData.Response(100, null, HttpVersion.HTTP_1_1, HttpFields.EMPTY); + private static final Index ASSUMED_CONTENT_METHODS = new Index.Builder() + .caseSensitive(false) + .with(HttpMethod.POST.asString(), Boolean.TRUE) + .with(HttpMethod.PUT.asString(), Boolean.TRUE) + .build(); + public static final int CHUNK_SIZE = 12; // states public enum State @@ -68,25 +73,14 @@ public enum Result DONE // The current phase of generation is complete } - // other statics - public static final int CHUNK_SIZE = 12; - private State _state = State.START; private EndOfContent _endOfContent = EndOfContent.UNKNOWN_CONTENT; private MetaData _info; - private long _contentPrepared = 0; private boolean _noContentResponse = false; private Boolean _persistent = null; - - private static final Index ASSUMED_CONTENT_METHODS = new Index.Builder() - .caseSensitive(false) - .with(HttpMethod.POST.asString(), Boolean.TRUE) - .with(HttpMethod.PUT.asString(), Boolean.TRUE) - .build(); - - // data private boolean _needCRLF = false; + private int _maxHeaderBytes; public HttpGenerator() { @@ -103,6 +97,16 @@ public void reset() _needCRLF = false; } + public int getMaxHeaderBytes() + { + return _maxHeaderBytes; + } + + public void setMaxHeaderBytes(int maxHeaderBytes) + { + _maxHeaderBytes = maxHeaderBytes; + } + public State getState() { return _state; @@ -590,28 +594,28 @@ private void generateHeaders(ByteBuffer header, ByteBuffer content, boolean last HttpField field = fields.getField(f); HttpHeader h = field.getHeader(); if (h == null) + { putTo(field, header); + } else { switch (h) { - case CONTENT_LENGTH: + case CONTENT_LENGTH -> + { if (contentLength < 0) contentLength = field.getLongValue(); else if (contentLength != field.getLongValue()) throw new HttpException.RuntimeException(INTERNAL_SERVER_ERROR_500, String.format("Incorrect Content-Length %d!=%d", contentLength, field.getLongValue())); contentLengthField = true; - break; - - case CONTENT_TYPE: + } + case CONTENT_TYPE -> { // write the field to the header contentType = true; putTo(field, header); - break; } - - case TRANSFER_ENCODING: + case TRANSFER_ENCODING -> { if (http11) { @@ -620,10 +624,8 @@ else if (contentLength != field.getLongValue()) transferEncoding = field; chunkedHint = field.contains(HttpHeaderValue.CHUNKED.asString()); } - break; } - - case CONNECTION: + case CONNECTION -> { String value = field.getValue(); @@ -677,13 +679,11 @@ else if (field.contains(HttpHeaderValue.KEEP_ALIVE.asString())) { putTo(field, header); } - break; } - - default: - putTo(field, header); + default -> putTo(field, header); } } + checkMaxHeaderBytes(header); } } @@ -798,12 +798,23 @@ else if (response != null) // end the header. header.put(HttpTokens.CRLF); + + checkMaxHeaderBytes(header); + } + + private void checkMaxHeaderBytes(ByteBuffer header) + { + int maxHeaderBytes = getMaxHeaderBytes(); + if (maxHeaderBytes > 0 && header.position() > maxHeaderBytes) + throw new BufferOverflowException(); } private static void putContentLength(ByteBuffer header, long contentLength) { if (contentLength == 0) + { header.put(CONTENT_LENGTH_0); + } else { header.put(HttpHeader.CONTENT_LENGTH.getBytesColonSpace()); diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index e1b037cda648..a398889c4005 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -1259,10 +1259,12 @@ protected boolean parseFields(ByteBuffer buffer) if (_maxHeaderBytes > 0 && ++_headerBytes > _maxHeaderBytes) { boolean header = _state == State.HEADER; - LOG.warn("{} is too large {}>{}", header ? "Header" : "Trailer", _headerBytes, _maxHeaderBytes); - throw new BadMessageException(header - ? HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431 - : HttpStatus.PAYLOAD_TOO_LARGE_413); + if (debugEnabled) + LOG.debug("{} is too large {}>{}", header ? "Header" : "Trailer", _headerBytes, _maxHeaderBytes); + 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"); } switch (_fieldState) diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java index 7aec552d8723..43e71f9be644 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Session.java @@ -740,11 +740,6 @@ public boolean goAway(GoAwayFrame frame, Callback callback) } private GoAwayFrame newGoAwayFrame(int error, String reason) - { - return newGoAwayFrame(getLastRemoteStreamId(), error, reason); - } - - private GoAwayFrame newGoAwayFrame(int lastRemoteStreamId, int error, String reason) { byte[] payload = null; if (reason != null) @@ -753,7 +748,7 @@ private GoAwayFrame newGoAwayFrame(int lastRemoteStreamId, int error, String rea reason = reason.substring(0, Math.min(reason.length(), 32)); payload = reason.getBytes(StandardCharsets.UTF_8); } - return new GoAwayFrame(lastRemoteStreamId, error, payload); + return new GoAwayFrame(getLastRemoteStreamId(), error, payload); } @Override @@ -1267,15 +1262,20 @@ boolean hasHighPriority() return false; } - @Override - public void failed(Throwable x) + public void closeAndFail(Throwable failure) { if (stream != null) { stream.close(); stream.getSession().removeStream(stream); } - super.failed(x); + failed(failure); + } + + public void resetAndFail(Throwable x) + { + if (stream != null) + stream.reset(new ResetFrame(stream.getId(), ErrorCode.CANCEL_STREAM_ERROR.code), Callback.from(() -> failed(x))); } /** @@ -1808,10 +1808,8 @@ private void onGoAway(GoAwayFrame frame) if (failStreams) { - // Must compare the lastStreamId only with local streams. - // For example, a client that sent request with streamId=137 may send a GOAWAY(4), - // where streamId=4 is the last stream pushed by the server to the client. - // The server must not compare its local streamId=4 with remote streamId=137. + // The lastStreamId carried by the GOAWAY is that of a local stream, + // so the lastStreamId must be compared only to local streams ids. Predicate failIf = stream -> stream.isLocal() && stream.getId() > frame.getLastStreamId(); failStreams(failIf, "closing", false); } @@ -1839,7 +1837,7 @@ private void onShutdown() case REMOTELY_CLOSED -> { closed = CloseState.CLOSING; - GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason); + GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); zeroStreamsAction = () -> terminate(goAwayFrame); failure = new ClosedChannelException(); failStreams = true; @@ -1869,7 +1867,7 @@ private void onShutdown() } else { - GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason); + GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); abort(reason, cause, Callback.from(() -> terminate(goAwayFrame))); } } @@ -1998,7 +1996,7 @@ private void onWriteFailure(Throwable x) closed = CloseState.CLOSING; zeroStreamsAction = () -> { - GoAwayFrame goAwayFrame = newGoAwayFrame(0, ErrorCode.NO_ERROR.code, reason); + GoAwayFrame goAwayFrame = newGoAwayFrame(ErrorCode.NO_ERROR.code, reason); terminate(goAwayFrame); }; failure = x; diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java index aa91f4d37c4b..d6490012a7db 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2Stream.java @@ -179,9 +179,15 @@ public void reset(ResetFrame frame, Callback callback) } session.dataConsumed(this, flowControlLength); if (resetFailure != null) + { + close(); + session.removeStream(this); callback.failed(resetFailure); + } else + { session.reset(this, frame, callback); + } } private boolean startWrite(Callback callback) diff --git a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java index 505ca16da587..47cd60246e1e 100644 --- a/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java +++ b/jetty-core/jetty-http2/jetty-http2-common/src/main/java/org/eclipse/jetty/http2/internal/HTTP2Flusher.java @@ -98,7 +98,7 @@ public boolean prepend(HTTP2Session.Entry entry) return true; } } - closed(entry, closed); + entry.closeAndFail(closed); return false; } @@ -108,9 +108,13 @@ public boolean append(HTTP2Session.Entry entry) try (AutoLock ignored = lock.lock()) { closed = terminated; - // If it was not possible to HPACK encode, then allow to send a GOAWAY. - if (closed instanceof HpackException.SessionException && entry.frame().getType() == FrameType.GO_AWAY) - closed = null; + // If it was not possible to HPACK encode, then allow to send RST_STREAM and GOAWAY. + if (closed instanceof HpackException.SessionException) + { + FrameType frameType = entry.frame().getType(); + if (frameType == FrameType.RST_STREAM || frameType == FrameType.GO_AWAY) + closed = null; + } if (closed == null) { entries.offer(entry); @@ -119,7 +123,7 @@ public boolean append(HTTP2Session.Entry entry) return true; } } - closed(entry, closed); + entry.closeAndFail(closed); return false; } @@ -137,7 +141,7 @@ public boolean append(List list) return true; } } - list.forEach(entry -> closed(entry, closed)); + list.forEach(entry -> entry.closeAndFail(closed)); return false; } @@ -171,11 +175,18 @@ protected Action process() throws Throwable if (terminated instanceof HpackException.SessionException) { HTTP2Session.Entry entry = entries.peek(); - if (entry != null && entry.frame().getType() == FrameType.GO_AWAY) + if (entry != null) { - // Allow a SessionException to be processed once to send a GOAWAY. - terminated = new ClosedChannelException().initCause(terminated); - rethrow = false; + FrameType frameType = entry.frame().getType(); + if (frameType == FrameType.RST_STREAM || frameType == FrameType.GO_AWAY) + { + rethrow = false; + if (frameType == FrameType.GO_AWAY) + { + // Allow a SessionException to be processed once to send a GOAWAY. + terminated = new ClosedChannelException().initCause(terminated); + } + } } } if (rethrow) @@ -222,7 +233,7 @@ protected Action process() throws Throwable { if (LOG.isDebugEnabled()) LOG.debug("Dropped {}", entry); - entry.failed(new EofException("dropped")); + entry.closeAndFail(new EofException("dropped")); pending.remove(); continue; } @@ -262,7 +273,7 @@ protected Action process() throws Throwable { if (LOG.isDebugEnabled()) LOG.debug("Failure generating {}", entry, failure); - entry.failed(failure); + entry.resetAndFail(failure); pending.remove(); } catch (HpackException.SessionException failure) @@ -365,23 +376,39 @@ protected void onCompleteSuccess() protected void onCompleteFailure(Throwable x) { accumulator.release(); - Throwable closed = fail(x); - // If the failure came from within the - // flusher, we need to close the connection. + + Throwable closed; + Set allEntries; + try (AutoLock ignored = lock.lock()) + { + closed = terminated; + terminated = x; + if (LOG.isDebugEnabled()) + LOG.debug(String.format("%s, entries processed/pending/queued=%d/%d/%d", + closed != null ? "Closing" : "Failing", + processedEntries.size(), + pendingEntries.size(), + entries.size()), x); + allEntries = new HashSet<>(entries); + entries.clear(); + } + allEntries.addAll(processedEntries); + processedEntries.clear(); + allEntries.addAll(pendingEntries); + pendingEntries.clear(); + + // If the failure comes from within the flusher, + // fail the current streams and close the connection. if (closed == null) session.onWriteFailure(x); + + allEntries.forEach(entry -> entry.closeAndFail(x)); } private void onSessionFailure(Throwable x) { accumulator.release(); - Throwable closed = fail(x); - if (closed == null) - session.close(ErrorCode.COMPRESSION_ERROR.code, null, NOOP); - } - private Throwable fail(Throwable x) - { Throwable closed; Set allEntries; try (AutoLock ignored = lock.lock()) @@ -397,13 +424,15 @@ private Throwable fail(Throwable x) allEntries = new HashSet<>(entries); entries.clear(); } - allEntries.addAll(processedEntries); processedEntries.clear(); allEntries.addAll(pendingEntries); pendingEntries.clear(); - allEntries.forEach(entry -> entry.failed(x)); - return closed; + + allEntries.forEach(entry -> entry.resetAndFail(x)); + + if (closed == null) + session.close(ErrorCode.COMPRESSION_ERROR.code, null, NOOP); } public void terminate(Throwable cause) @@ -420,11 +449,6 @@ public void terminate(Throwable cause) iterate(); } - private void closed(HTTP2Session.Entry entry, Throwable failure) - { - entry.failed(failure); - } - @Override public String dump() { diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/AbstractHTTP2ServerConnectionFactory.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/AbstractHTTP2ServerConnectionFactory.java index 3b69140b4d76..08dd3a493231 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/AbstractHTTP2ServerConnectionFactory.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/AbstractHTTP2ServerConnectionFactory.java @@ -303,6 +303,7 @@ public Connection newConnection(Connector connector, EndPoint endPoint) ServerSessionListener listener = newSessionListener(connector, endPoint); Generator generator = new Generator(connector.getByteBufferPool(), isUseOutputDirectByteBuffers(), getMaxHeaderBlockFragment()); + generator.getHpackEncoder().setMaxHeaderListSize(getHttpConfiguration().getResponseHeaderSize()); FlowControlStrategy flowControl = getFlowControlStrategyFactory().newFlowControlStrategy(); ServerParser parser = newServerParser(connector, getRateControlFactory().newRateControl(endPoint)); diff --git a/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/AbstractProxyTest.java b/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/AbstractProxyTest.java index 7a552f1043ce..e8bc31456a3a 100644 --- a/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/AbstractProxyTest.java +++ b/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/AbstractProxyTest.java @@ -14,6 +14,7 @@ package org.eclipse.jetty.proxy; import java.util.List; +import java.util.function.Consumer; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.transport.HttpClientConnectionFactory; @@ -39,6 +40,8 @@ public static List httpVersions() return List.of(HttpVersion.HTTP_1_1, HttpVersion.HTTP_2); } + protected HttpConfiguration serverHttpConfig = new HttpConfiguration(); + protected HttpConfiguration proxyHttpConfig = new HttpConfiguration(); protected Server server; protected ServerConnector serverConnector; protected Server proxy; @@ -46,6 +49,11 @@ public static List httpVersions() protected HttpClient client; protected void startClient() throws Exception + { + startClient(client -> {}); + } + + protected void startClient(Consumer configurator) throws Exception { ClientConnector clientConnector = new ClientConnector(); QueuedThreadPool clientThreads = new QueuedThreadPool(); @@ -53,6 +61,7 @@ protected void startClient() throws Exception clientConnector.setExecutor(clientThreads); HTTP2Client http2Client = new HTTP2Client(clientConnector); client = new HttpClient(new HttpClientTransportDynamic(clientConnector, HttpClientConnectionFactory.HTTP11, new ClientConnectionFactoryOverHTTP2.HTTP2(http2Client))); + configurator.accept(client); client.start(); } @@ -61,10 +70,9 @@ protected void startProxy(ProxyHandler handler) throws Exception QueuedThreadPool proxyPool = new QueuedThreadPool(); proxyPool.setName("proxy"); proxy = new Server(proxyPool); - HttpConfiguration configuration = new HttpConfiguration(); - configuration.setSendDateHeader(false); - configuration.setSendServerVersion(false); - proxyConnector = new ServerConnector(proxy, 1, 1, new HttpConnectionFactory(configuration), new HTTP2CServerConnectionFactory(configuration)); + proxyHttpConfig.setSendDateHeader(false); + proxyHttpConfig.setSendServerVersion(false); + proxyConnector = new ServerConnector(proxy, 1, 1, new HttpConnectionFactory(proxyHttpConfig), new HTTP2CServerConnectionFactory(proxyHttpConfig)); proxy.addConnector(proxyConnector); proxy.setHandler(handler); proxy.start(); @@ -75,8 +83,7 @@ protected void startServer(Handler handler) throws Exception QueuedThreadPool serverPool = new QueuedThreadPool(); serverPool.setName("server"); server = new Server(serverPool); - HttpConfiguration httpConfig = new HttpConfiguration(); - serverConnector = new ServerConnector(server, 1, 1, new HttpConnectionFactory(httpConfig), new HTTP2CServerConnectionFactory(httpConfig)); + serverConnector = new ServerConnector(server, 1, 1, new HttpConnectionFactory(serverHttpConfig), new HTTP2CServerConnectionFactory(serverHttpConfig)); server.addConnector(serverConnector); server.setHandler(handler); server.start(); diff --git a/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ReverseProxyTest.java b/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ReverseProxyTest.java index 9cc328cbe58f..4e76141fbdb3 100644 --- a/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ReverseProxyTest.java +++ b/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ReverseProxyTest.java @@ -13,13 +13,18 @@ package org.eclipse.jetty.proxy; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import org.eclipse.jetty.client.CompletableResponseListener; import org.eclipse.jetty.client.ContentResponse; import org.eclipse.jetty.client.HttpClient; import org.eclipse.jetty.client.StringRequestContent; import org.eclipse.jetty.client.transport.HttpClientConnectionFactory; import org.eclipse.jetty.client.transport.HttpClientTransportDynamic; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http2.client.HTTP2Client; @@ -35,6 +40,10 @@ import org.junit.jupiter.params.provider.MethodSource; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; public class ReverseProxyTest extends AbstractProxyTest { @@ -62,12 +71,7 @@ public boolean handle(Request request, Response response, Callback callback) thr @Override protected HttpClient newHttpClient() { - ClientConnector proxyClientConnector = new ClientConnector(); - QueuedThreadPool proxyClientThreads = new QueuedThreadPool(); - proxyClientThreads.setName("proxy-client"); - proxyClientConnector.setExecutor(proxyClientThreads); - HTTP2Client proxyHTTP2Client = new HTTP2Client(proxyClientConnector); - return new HttpClient(new HttpClientTransportDynamic(proxyClientConnector, HttpClientConnectionFactory.HTTP11, new ClientConnectionFactoryOverHTTP2.HTTP2(proxyHTTP2Client))); + return newProxyHttpClient(); } @Override @@ -111,12 +115,7 @@ public boolean handle(Request request, Response response, Callback callback) @Override protected HttpClient newHttpClient() { - ClientConnector proxyClientConnector = new ClientConnector(); - QueuedThreadPool proxyClientThreads = new QueuedThreadPool(); - proxyClientThreads.setName("proxy-client"); - proxyClientConnector.setExecutor(proxyClientThreads); - HTTP2Client proxyHTTP2Client = new HTTP2Client(proxyClientConnector); - return new HttpClient(new HttpClientTransportDynamic(proxyClientConnector, HttpClientConnectionFactory.HTTP11, new ClientConnectionFactoryOverHTTP2.HTTP2(proxyHTTP2Client))); + return newProxyHttpClient(); } @Override @@ -138,4 +137,290 @@ protected org.eclipse.jetty.client.Request newProxyToServerRequest(Request clien assertEquals(200, response.getStatus()); assertEquals("", response.getHeaders().get(emptyHeaderName)); } + + @ParameterizedTest + @MethodSource("httpVersions") + public void testServerResponseHeadersTooLargeForServerConfiguration(HttpVersion httpVersion) throws Exception + { + // Server is not able to write response and aborts. + // Proxy sees the abort and sends 502 to client. + + int maxResponseHeadersSize = 256; + serverHttpConfig.setResponseHeaderSize(maxResponseHeadersSize); + startServer(new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) + { + response.getHeaders().put("X-Large", "A".repeat(maxResponseHeadersSize)); + + // With HTTP/1.1, calling response.write() would fail the Handler callback + // which would trigger ErrorHandler and result in a 500 to the proxy. +// response.write(true, null, callback); + + // With HTTP/1.1, succeeding the callback before the actual last write + // results in skipping the ErrorHandler and aborting the response and + // the connection, which the proxy interprets as a 502. + // HTTP/2 always behaves by aborting the connection. + callback.succeeded(); + return true; + } + }); + + CountDownLatch serverToProxyFailureLatch = new CountDownLatch(1); + startProxy(new ProxyHandler.Reverse(clientToProxyRequest -> + HttpURI.build(clientToProxyRequest.getHttpURI()).port(serverConnector.getLocalPort())) + { + @Override + protected HttpClient newHttpClient() + { + return newProxyHttpClient(); + } + + @Override + protected org.eclipse.jetty.client.Request newProxyToServerRequest(Request clientToProxyRequest, HttpURI newHttpURI) + { + // Use the client to proxy protocol also from the proxy to server. + return super.newProxyToServerRequest(clientToProxyRequest, newHttpURI) + .version(httpVersion); + } + + @Override + protected void onServerToProxyResponseFailure(Request clientToProxyRequest, org.eclipse.jetty.client.Request proxyToServerRequest, org.eclipse.jetty.client.Response serverToProxyResponse, Response proxyToClientResponse, Callback proxyToClientCallback, Throwable failure) + { + serverToProxyFailureLatch.countDown(); + super.onServerToProxyResponseFailure(clientToProxyRequest, proxyToServerRequest, serverToProxyResponse, proxyToClientResponse, proxyToClientCallback, failure); + } + }); + + startClient(); + + ContentResponse response = client.newRequest("localhost", proxyConnector.getLocalPort()) + .version(httpVersion) + .timeout(5, TimeUnit.SECONDS) + .send(); + +// assertTrue(serverToProxyFailureLatch.await(5, TimeUnit.SECONDS)); + assertEquals(HttpStatus.BAD_GATEWAY_502, response.getStatus()); + } + + @ParameterizedTest + @MethodSource("httpVersions") + public void testServerResponseHeadersTooLargeForProxyConfiguration(HttpVersion httpVersion) throws Exception + { + // Server is able to write the response. + // Proxy cannot parse the response from server, fails and sends 502 to client. + + int maxResponseHeadersSize = 256; + startServer(new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) + { + response.getHeaders().put("X-Large", "A".repeat(maxResponseHeadersSize)); + callback.succeeded(); + return true; + } + }); + + CountDownLatch serverToProxyFailureLatch = new CountDownLatch(1); + startProxy(new ProxyHandler.Reverse(clientToProxyRequest -> + HttpURI.build(clientToProxyRequest.getHttpURI()).port(serverConnector.getLocalPort())) + { + @Override + protected HttpClient newHttpClient() + { + HttpClient httpClient = newProxyHttpClient(); + httpClient.setMaxResponseHeadersSize(maxResponseHeadersSize); + return httpClient; + } + + @Override + protected org.eclipse.jetty.client.Request newProxyToServerRequest(Request clientToProxyRequest, HttpURI newHttpURI) + { + // Use the client to proxy protocol also from the proxy to server. + return super.newProxyToServerRequest(clientToProxyRequest, newHttpURI) + .version(httpVersion); + } + + @Override + protected void onServerToProxyResponseFailure(Request clientToProxyRequest, org.eclipse.jetty.client.Request proxyToServerRequest, org.eclipse.jetty.client.Response serverToProxyResponse, Response proxyToClientResponse, Callback proxyToClientCallback, Throwable failure) + { + serverToProxyFailureLatch.countDown(); + super.onServerToProxyResponseFailure(clientToProxyRequest, proxyToServerRequest, serverToProxyResponse, proxyToClientResponse, proxyToClientCallback, failure); + } + }); + + startClient(); + + ContentResponse response = client.newRequest("localhost", proxyConnector.getLocalPort()) + .version(httpVersion) + .timeout(5, TimeUnit.SECONDS) + .send(); + + assertTrue(serverToProxyFailureLatch.await(5, TimeUnit.SECONDS)); + assertEquals(HttpStatus.BAD_GATEWAY_502, response.getStatus()); + } + + @ParameterizedTest + @MethodSource("httpVersions") + public void testProxyResponseHeadersTooLargeForProxyConfiguration(HttpVersion httpVersion) throws Exception + { + // Proxy client receives response from server. + // Proxy server is not able to write the response to client. + + int maxResponseHeadersSize = 256; + startServer(new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) + { + callback.succeeded(); + return true; + } + }); + + CountDownLatch proxyToClientFailureLatch = new CountDownLatch(1); + proxyHttpConfig.setResponseHeaderSize(maxResponseHeadersSize); + startProxy(new ProxyHandler.Reverse(clientToProxyRequest -> + HttpURI.build(clientToProxyRequest.getHttpURI()).port(serverConnector.getLocalPort())) + { + @Override + protected HttpClient newHttpClient() + { + return newProxyHttpClient(); + } + + @Override + protected org.eclipse.jetty.client.Request newProxyToServerRequest(Request clientToProxyRequest, HttpURI newHttpURI) + { + // Use the client to proxy protocol also from the proxy to server. + return super.newProxyToServerRequest(clientToProxyRequest, newHttpURI) + .version(httpVersion); + } + + @Override + protected org.eclipse.jetty.client.Response.CompleteListener newServerToProxyResponseListener(Request clientToProxyRequest, org.eclipse.jetty.client.Request proxyToServerRequest, Response proxyToClientResponse, Callback proxyToClientCallback) + { + return new ProxyResponseListener(clientToProxyRequest, proxyToServerRequest, proxyToClientResponse, proxyToClientCallback) + { + @Override + public void onHeaders(org.eclipse.jetty.client.Response serverToProxyResponse) + { + proxyToClientResponse.getHeaders().put("X-Large", "A".repeat(maxResponseHeadersSize)); + super.onHeaders(serverToProxyResponse); + } + }; + } + + @Override + protected void onProxyToClientResponseFailure(Request clientToProxyRequest, org.eclipse.jetty.client.Request proxyToServerRequest, org.eclipse.jetty.client.Response serverToProxyResponse, Response proxyToClientResponse, Callback proxyToClientCallback, Throwable failure) + { + proxyToClientFailureLatch.countDown(); + super.onProxyToClientResponseFailure(clientToProxyRequest, proxyToServerRequest, serverToProxyResponse, proxyToClientResponse, proxyToClientCallback, failure); + } + }); + + startClient(); + + var request = client.newRequest("localhost", proxyConnector.getLocalPort()) + .version(httpVersion) + .timeout(5, TimeUnit.SECONDS); + CompletableFuture completable = new CompletableResponseListener(request).send(); + + assertTrue(proxyToClientFailureLatch.await(5, TimeUnit.SECONDS)); + + completable.handle((response, failure) -> + { + switch (httpVersion) + { + case HTTP_1_1 -> + { + // HTTP/1.1 fails to generate the response, but does not commit, + // so it is able to write an error response to the client. + + assertNotNull(response); + assertNull(failure); + assertEquals(HttpStatus.INTERNAL_SERVER_ERROR_500, response.getStatus()); + } + case HTTP_2 -> + { + // HTTP/2 fails to generate the response, sends a GOAWAY, + // and the client aborts the response. + assertNull(response); + assertNotNull(failure); + } + } + return null; + }).get(5, TimeUnit.SECONDS); + } + + @ParameterizedTest + @MethodSource("httpVersions") + public void testProxyResponseHeadersTooLargeForClientConfiguration(HttpVersion httpVersion) throws Exception + { + int maxResponseHeadersSize = 256; + startServer(new Handler.Abstract() + { + @Override + public boolean handle(Request request, Response response, Callback callback) + { + callback.succeeded(); + return true; + } + }); + + startProxy(new ProxyHandler.Reverse(clientToProxyRequest -> + HttpURI.build(clientToProxyRequest.getHttpURI()).port(serverConnector.getLocalPort())) + { + @Override + protected HttpClient newHttpClient() + { + return newProxyHttpClient(); + } + + @Override + protected org.eclipse.jetty.client.Request newProxyToServerRequest(Request clientToProxyRequest, HttpURI newHttpURI) + { + // Use the client to proxy protocol also from the proxy to server. + return super.newProxyToServerRequest(clientToProxyRequest, newHttpURI) + .version(httpVersion); + } + + @Override + protected org.eclipse.jetty.client.Response.CompleteListener newServerToProxyResponseListener(Request clientToProxyRequest, org.eclipse.jetty.client.Request proxyToServerRequest, Response proxyToClientResponse, Callback proxyToClientCallback) + { + return new ProxyResponseListener(clientToProxyRequest, proxyToServerRequest, proxyToClientResponse, proxyToClientCallback) + { + @Override + public void onHeaders(org.eclipse.jetty.client.Response serverToProxyResponse) + { + proxyToClientResponse.getHeaders().put("X-Large", "A".repeat(maxResponseHeadersSize)); + super.onHeaders(serverToProxyResponse); + } + }; + } + }); + + startClient(client -> client.setMaxResponseHeadersSize(maxResponseHeadersSize)); + + CountDownLatch responseFailureLatch = new CountDownLatch(1); + assertThrows(ExecutionException.class, () -> client.newRequest("localhost", proxyConnector.getLocalPort()) + .version(httpVersion) + .onResponseFailure((r, x) -> responseFailureLatch.countDown()) + .timeout(5, TimeUnit.SECONDS) + .send()); + + assertTrue(responseFailureLatch.await(5, TimeUnit.SECONDS)); + } + + private static HttpClient newProxyHttpClient() + { + ClientConnector proxyClientConnector = new ClientConnector(); + QueuedThreadPool proxyClientThreads = new QueuedThreadPool(); + proxyClientThreads.setName("proxy-client"); + proxyClientConnector.setExecutor(proxyClientThreads); + HTTP2Client proxyHTTP2Client = new HTTP2Client(proxyClientConnector); + return new HttpClient(new HttpClientTransportDynamic(proxyClientConnector, HttpClientConnectionFactory.HTTP11, new ClientConnectionFactoryOverHTTP2.HTTP2(proxyHTTP2Client))); + } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 37fe59479e59..784fbbd3a1bf 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -171,7 +171,9 @@ public boolean isRecordHttpComplianceViolations() protected HttpGenerator newHttpGenerator() { - return new HttpGenerator(); + HttpGenerator generator = new HttpGenerator(); + generator.setMaxHeaderBytes(getHttpConfiguration().getResponseHeaderSize()); + return generator; } protected HttpParser newHttpParser(HttpCompliance compliance) @@ -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"); releaseHeader(); _header = _bufferPool.acquire(getHttpConfiguration().getResponseHeaderSize(), useDirectByteBuffers); continue; diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/LargeHeaderTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/LargeHeaderTest.java index 975642701ac1..f5364bd84014 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/LargeHeaderTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/LargeHeaderTest.java @@ -53,7 +53,7 @@ public class LargeHeaderTest { private static final Logger LOG = LoggerFactory.getLogger(LargeHeaderTest.class); - private static final String EXPECTED_ERROR_TEXT = "

HTTP ERROR 500 Response header too large

"; + private static final String EXPECTED_ERROR_TEXT = "

HTTP ERROR 500 Response Header Fields Too Large

"; private Server server; @BeforeEach diff --git a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AbstractProxyServlet.java b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AbstractProxyServlet.java index 3bebd249a616..dc04437b5c10 100644 --- a/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AbstractProxyServlet.java +++ b/jetty-ee10/jetty-ee10-proxy/src/main/java/org/eclipse/jetty/ee10/proxy/AbstractProxyServlet.java @@ -255,6 +255,11 @@ protected Logger createLogger() * The response buffer size, see {@link HttpClient#setResponseBufferSize(int)} * * + * maxResponseHeadersSize + * HttpClient's default + * The maximum response headers size, see {@link HttpClient#setMaxResponseHeadersSize(int)} + * + * * selectors * cores / 2 * The number of NIO selectors used by {@link HttpClient} @@ -322,6 +327,10 @@ protected HttpClient createHttpClient() throws ServletException if (value != null) client.setResponseBufferSize(Integer.parseInt(value)); + value = config.getInitParameter("maxResponseHeadersSize"); + if (value != null) + client.setMaxResponseHeadersSize(Integer.parseInt(value)); + try { client.start(); @@ -715,7 +724,7 @@ protected void onProxyResponseSuccess(HttpServletRequest clientRequest, HttpServ protected void onProxyResponseFailure(HttpServletRequest clientRequest, HttpServletResponse proxyResponse, Response serverResponse, Throwable failure) { if (_log.isDebugEnabled()) - _log.debug(getRequestId(clientRequest) + " proxying failed", failure); + _log.debug("{} proxying failed", getRequestId(clientRequest), failure); int status = proxyResponseStatus(failure); int serverStatus = serverResponse == null ? status : serverResponse.getStatus(); @@ -812,7 +821,7 @@ protected void init(ServletConfig config) throws ServletException _prefix = _prefix == null ? contextPath : (contextPath + _prefix); if (proxyServlet._log.isDebugEnabled()) - proxyServlet._log.debug(config.getServletName() + " @ " + _prefix + " to " + _proxyTo); + proxyServlet._log.debug("{} @ {} to {}", config.getServletName(), _prefix, _proxyTo); } protected String rewriteTarget(HttpServletRequest request) diff --git a/jetty-ee10/jetty-ee10-proxy/src/test/java/org/eclipse/jetty/ee10/proxy/ProxyServletTest.java b/jetty-ee10/jetty-ee10-proxy/src/test/java/org/eclipse/jetty/ee10/proxy/ProxyServletTest.java index f98ca0d4a350..0804d020b198 100644 --- a/jetty-ee10/jetty-ee10-proxy/src/test/java/org/eclipse/jetty/ee10/proxy/ProxyServletTest.java +++ b/jetty-ee10/jetty-ee10-proxy/src/test/java/org/eclipse/jetty/ee10/proxy/ProxyServletTest.java @@ -127,32 +127,32 @@ public static Stream impls() ).map(Arguments::of); } - private HttpClient client; - private Proxy clientProxy; + private HttpConfiguration httpConfig = new HttpConfiguration(); + private Server server; + private ServerConnector serverConnector; + private ServerConnector tlsServerConnector; private Server proxy; private ServerConnector proxyConnector; private ServletContextHandler proxyContext; private AbstractProxyServlet proxyServlet; - private Server server; - private ServerConnector serverConnector; - private ServerConnector tlsServerConnector; + private HttpClient client; + private Proxy clientProxy; private void startServer(HttpServlet servlet) throws Exception { QueuedThreadPool serverPool = new QueuedThreadPool(); serverPool.setName("server"); server = new Server(serverPool); - serverConnector = new ServerConnector(server); + HttpConnectionFactory h1 = new HttpConnectionFactory(httpConfig); + serverConnector = new ServerConnector(server, 1, 1, h1); server.addConnector(serverConnector); SslContextFactory.Server sslContextFactory = new SslContextFactory.Server(); String keyStorePath = MavenTestingUtils.getTestResourceFile("server_keystore.p12").getAbsolutePath(); sslContextFactory.setKeyStorePath(keyStorePath); sslContextFactory.setKeyStorePassword("storepwd"); - tlsServerConnector = new ServerConnector(server, new SslConnectionFactory( - sslContextFactory, - HttpVersion.HTTP_1_1.asString()), - new HttpConnectionFactory()); + SslConnectionFactory ssl = new SslConnectionFactory(sslContextFactory, HttpVersion.HTTP_1_1.asString()); + tlsServerConnector = new ServerConnector(server, 1, 1, ssl, h1); server.addConnector(tlsServerConnector); ServletContextHandler appCtx = new ServletContextHandler("/", true, false); @@ -1749,9 +1749,9 @@ protected void service(HttpServletRequest request, HttpServletResponse response) Host: $A Expect: 100-Continue Transfer-Encoding: chunked - + 0 - + """; } else @@ -1782,4 +1782,58 @@ protected void service(HttpServletRequest request, HttpServletResponse response) } } } + + @ParameterizedTest + @MethodSource("transparentImpls") + public void testServerResponseHeadersTooLargeForServerConfiguration(AbstractProxyServlet proxyServletClass) throws Exception + { + int maxResponseHeadersSize = 256; + httpConfig.setResponseHeaderSize(maxResponseHeadersSize); + startServer(new HttpServlet() + { + @Override + protected void service(HttpServletRequest request, HttpServletResponse response) + { + response.setHeader("X-Large", "A".repeat(maxResponseHeadersSize)); + } + }); + Map initParams = Map.of( + "proxyTo", "http://localhost:" + serverConnector.getLocalPort() + ); + startProxy(proxyServletClass, initParams); + startClient(); + + ContentResponse response = client.newRequest("localhost", proxyConnector.getLocalPort()) + .timeout(5, TimeUnit.SECONDS) + .send(); + + assertEquals(HttpStatus.INTERNAL_SERVER_ERROR_500, response.getStatus()); + } + + @ParameterizedTest + @MethodSource("transparentImpls") + public void testServerResponseHeadersTooLargeForProxyConfiguration(AbstractProxyServlet proxyServletClass) throws Exception + { + int maxResponseHeadersSize = 256; + startServer(new HttpServlet() + { + @Override + protected void service(HttpServletRequest request, HttpServletResponse response) + { + response.setHeader("X-Large", "A".repeat(maxResponseHeadersSize)); + } + }); + Map initParams = Map.of( + "proxyTo", "http://localhost:" + serverConnector.getLocalPort(), + "maxResponseHeadersSize", String.valueOf(maxResponseHeadersSize) + ); + startProxy(proxyServletClass, initParams); + startClient(); + + ContentResponse response = client.newRequest("localhost", proxyConnector.getLocalPort()) + .timeout(5, TimeUnit.SECONDS) + .send(); + + assertEquals(HttpStatus.BAD_GATEWAY_502, response.getStatus()); + } } From af317882a2f0db18cea1fe3fa992449f4e59fe4f Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 8 Oct 2024 11:08:03 +0200 Subject: [PATCH 5/7] Updates from review. Signed-off-by: Simone Bordet --- .../main/java/org/eclipse/jetty/http/BadMessageException.java | 2 +- .../src/test/java/org/eclipse/jetty/proxy/ReverseProxyTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java index 10368cc3e73b..ceadf7bb2475 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/BadMessageException.java @@ -49,6 +49,6 @@ public BadMessageException(int code, String reason) public BadMessageException(int code, String reason, Throwable cause) { super(code, reason, cause); - assert code >= 400 && code < 500; + assert HttpStatus.isClientError(code); } } diff --git a/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ReverseProxyTest.java b/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ReverseProxyTest.java index 4e76141fbdb3..95c19c81619a 100644 --- a/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ReverseProxyTest.java +++ b/jetty-core/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ReverseProxyTest.java @@ -200,7 +200,7 @@ protected void onServerToProxyResponseFailure(Request clientToProxyRequest, org. .timeout(5, TimeUnit.SECONDS) .send(); -// assertTrue(serverToProxyFailureLatch.await(5, TimeUnit.SECONDS)); + assertTrue(serverToProxyFailureLatch.await(5, TimeUnit.SECONDS)); assertEquals(HttpStatus.BAD_GATEWAY_502, response.getStatus()); } From 8a8ba7950424dfb6f0701f81461c7637a8a438c4 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 8 Oct 2024 19:13:48 +0200 Subject: [PATCH 6/7] Updates from review. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/http/HttpParser.java | 39 +++++++++++++++---- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index a398889c4005..e7ab9eaeea24 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -676,7 +676,10 @@ private void quickStart(ByteBuffer buffer) if (_maxHeaderBytes > 0 && ++_headerBytes > _maxHeaderBytes) { LOG.warn("padding is too large >{}", _maxHeaderBytes); - throw new BadMessageException(HttpStatus.BAD_REQUEST_400); + if (_requestParser) + throw new BadMessageException(HttpStatus.BAD_REQUEST_400); + else + throw new HttpException.RuntimeException(_responseStatus, "Bad Response"); } } } @@ -740,10 +743,15 @@ private boolean parseLine(ByteBuffer buffer) else { if (_requestParser) + { LOG.warn("request is too large >{}", _maxHeaderBytes); + throw new BadMessageException(HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431); + } else + { LOG.warn("response is too large >{}", _maxHeaderBytes); - throw new BadMessageException(HttpStatus.REQUEST_HEADER_FIELDS_TOO_LARGE_431); + throw new HttpException.RuntimeException(_responseStatus, "Response Header Bytes Too Large"); + } } } @@ -865,7 +873,10 @@ else if (Violation.CASE_INSENSITIVE_METHOD.isAllowedBy(_complianceMode)) break; default: - throw new BadMessageException(_requestParser ? "No URI" : "No Status"); + if (_requestParser) + throw new BadMessageException("No URI"); + else + throw new HttpException.RuntimeException(_responseStatus, "No Status"); } break; @@ -1264,7 +1275,7 @@ protected boolean parseFields(ByteBuffer buffer) 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"); + throw new HttpException.RuntimeException(_responseStatus, "Response Header Fields Too Large"); } switch (_fieldState) @@ -1744,7 +1755,10 @@ else if (isTerminated()) if (debugEnabled) LOG.debug("{} EOF in {}", this, _state); setState(State.CLOSED); - _handler.badMessage(new BadMessageException(HttpStatus.BAD_REQUEST_400)); + if (_requestParser) + _handler.badMessage(new BadMessageException(HttpStatus.BAD_REQUEST_400, "Early EOF")); + else + _handler.badMessage(new HttpException.RuntimeException(_responseStatus, "Early EOF")); break; } } @@ -1752,9 +1766,18 @@ else if (isTerminated()) catch (Throwable x) { BufferUtil.clear(buffer); - HttpException bad = x instanceof HttpException http - ? http - : new BadMessageException(_requestParser ? "Bad Request" : "Bad Response", x); + HttpException bad; + if (x instanceof HttpException http) + { + bad = http; + } + else + { + if (_requestParser) + bad = new BadMessageException("Bad Request", x); + else + bad = new HttpException.RuntimeException(_responseStatus, "Bad Response", x); + } badMessage(bad); } return false; From 92c516e24c1fb7fbd23f668e15035e1ca9f6ee8c Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 29 Oct 2024 10:27:26 +0100 Subject: [PATCH 7/7] Simplified server-side response header allocation for HTTP/1.1. Signed-off-by: Simone Bordet --- .../eclipse/jetty/server/internal/HttpConnection.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 2b5ae5defe94..73bd0d34ea77 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -770,16 +770,12 @@ public Action process() throws Exception case NEED_HEADER: { - _header = _bufferPool.acquire(Math.min(getHttpConfiguration().getResponseHeaderSize(), getHttpConfiguration().getOutputBufferSize()), useDirectByteBuffers); + _header = _bufferPool.acquire(getHttpConfiguration().getResponseHeaderSize(), useDirectByteBuffers); continue; } case HEADER_OVERFLOW: { - if (_header.capacity() >= getHttpConfiguration().getResponseHeaderSize()) - throw new HttpException.RuntimeException(INTERNAL_SERVER_ERROR_500, "Response Header Fields Too Large"); - releaseHeader(); - _header = _bufferPool.acquire(getHttpConfiguration().getResponseHeaderSize(), useDirectByteBuffers); - continue; + throw new HttpException.RuntimeException(INTERNAL_SERVER_ERROR_500, "Response Header Fields Too Large"); } case NEED_CHUNK: { @@ -915,7 +911,7 @@ protected void onCompleteSuccess() } @Override - public void onCompleteFailure(final Throwable x) + public void onCompleteFailure(Throwable x) { failedCallback(release(), x); }