Fixes #5104 - AbstractProxyServlet include incorrect protocol version in Via header when accessed over H2.

* Introduced HttpFields.computeField() to put/append header values.
* Reworked AbstractProxyServlet.addViaHeader().

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
This commit is contained in:
Simone Bordet 2020-08-13 00:08:52 +02:00
parent b2ab05c44a
commit 79d340fdb6
6 changed files with 250 additions and 169 deletions

View File

@ -32,6 +32,7 @@ import java.util.NoSuchElementException;
import java.util.Objects;
import java.util.Set;
import java.util.StringTokenizer;
import java.util.function.BiFunction;
import java.util.function.ToIntFunction;
import java.util.stream.Stream;
@ -88,6 +89,115 @@ public class HttpFields implements Iterable<HttpField>
_size = fields._size;
}
/**
* <p>Computes a single field for the given HTTP header name and for existing fields with the same name.</p>
*
* <p>The compute function receives the field name and a list of fields with the same name
* so that their values can be used to compute the value of the field that is returned
* by the compute function.
* If the compute function returns {@code null}, the fields with the given name are removed.</p>
* <p>This method comes handy when you want to add an HTTP header if it does not exist,
* or add a value if the HTTP header already exists, similarly to
* {@link Map#compute(Object, BiFunction)}.</p>
*
* <p>This method can be used to {@link #put(HttpField) put} a new field (or blindly replace its value):</p>
* <pre>
* httpFields.computeField("X-New-Header",
* (name, fields) -> new HttpField(name, "NewValue"));
* </pre>
*
* <p>This method can be used to coalesce many fields into one:</p>
* <pre>
* // Input:
* GET / HTTP/1.1
* Host: localhost
* Cookie: foo=1
* Cookie: bar=2,baz=3
* User-Agent: Jetty
*
* // Computation:
* httpFields.computeField("Cookie", (name, fields) ->
* {
* // No cookies, nothing to do.
* if (fields == null)
* return null;
*
* // Coalesces all cookies.
* String coalesced = fields.stream()
* .flatMap(field -> Stream.of(field.getValues()))
* .collect(Collectors.joining(", "));
*
* // Returns a single Cookie header with all cookies.
* return new HttpField(name, coalesced);
* }
*
* // Output:
* GET / HTTP/1.1
* Host: localhost
* Cookie: foo=1, bar=2, baz=3
* User-Agent: Jetty
* </pre>
*
* <p>This method can be used to replace a field:</p>
* <pre>
* httpFields.computeField("X-Length", (name, fields) ->
* {
* if (fields == null)
* return null;
*
* // Get any value among the X-Length headers.
* String length = fields.stream()
* .map(HttpField::getValue)
* .findAny()
* .orElse("0");
*
* // Replace X-Length headers with X-Capacity header.
* return new HttpField("X-Capacity", length);
* });
* </pre>
*
* <p>This method can be used to remove a field:</p>
* <pre>
* httpFields.computeField("Connection", (name, fields) -> null);
* </pre>
*
* @param name the HTTP header name
* @param computeFn the compute function
*/
public void computeField(String name, BiFunction<String, List<HttpField>, HttpField> computeFn)
{
boolean found = false;
ListIterator<HttpField> iterator = listIterator();
while (iterator.hasNext())
{
HttpField field = iterator.next();
if (field.getName().equalsIgnoreCase(name))
{
if (found)
{
// Remove other headers with the same name, since
// we have computed one from all of them already.
iterator.remove();
}
else
{
found = true;
HttpField newField = computeFn.apply(name, Collections.unmodifiableList(getFields(name)));
if (newField == null)
iterator.remove();
else
iterator.set(newField);
}
}
}
if (!found)
{
HttpField newField = computeFn.apply(name, null);
if (newField != null)
put(newField);
}
}
public int size()
{
return _size;
@ -189,6 +299,22 @@ public class HttpFields implements Iterable<HttpField>
return fields == null ? Collections.emptyList() : fields;
}
public List<HttpField> getFields(String name)
{
List<HttpField> fields = null;
for (int i = 0; i < _size; i++)
{
HttpField f = _fields[i];
if (f.getName().equalsIgnoreCase(name))
{
if (fields == null)
fields = new ArrayList<>();
fields.add(f);
}
}
return fields == null ? Collections.emptyList() : fields;
}
public boolean contains(HttpField field)
{
for (int i = _size; i-- > 0; )

View File

@ -29,6 +29,8 @@ import java.util.Locale;
import java.util.Set;
import java.util.concurrent.Executor;
import java.util.concurrent.TimeoutException;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.servlet.AsyncContext;
import javax.servlet.ServletConfig;
import javax.servlet.ServletContext;
@ -48,6 +50,7 @@ import org.eclipse.jetty.http.HttpField;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpHeaderValue;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.util.HttpCookieStore;
import org.eclipse.jetty.util.ProcessorUtils;
import org.eclipse.jetty.util.StringUtil;
@ -109,7 +112,6 @@ public abstract class AbstractProxyServlet extends HttpServlet
private String _viaHost;
private HttpClient _client;
private long _timeout;
private boolean oldAddViaHeaderCalled;
@Override
public void init() throws ServletException
@ -168,9 +170,6 @@ public abstract class AbstractProxyServlet extends HttpServlet
public String getViaHost()
{
if (_viaHost == null)
_viaHost = viaHost();
return _viaHost;
}
@ -456,6 +455,14 @@ public abstract class AbstractProxyServlet extends HttpServlet
return HttpHeaderValue.CONTINUE.is(request.getHeader(HttpHeader.EXPECT.asString()));
}
protected Request newProxyRequest(HttpServletRequest request, String rewrittenTarget)
{
return getHttpClient().newRequest(rewrittenTarget)
.method(request.getMethod())
.version(HttpVersion.fromString(request.getProtocol()))
.attribute(CLIENT_REQUEST_ATTRIBUTE, request);
}
protected void copyRequestHeaders(HttpServletRequest clientRequest, Request proxyRequest)
{
// First clear possibly existing headers, as we are going to copy those from the client request.
@ -513,28 +520,28 @@ public abstract class AbstractProxyServlet extends HttpServlet
protected void addProxyHeaders(HttpServletRequest clientRequest, Request proxyRequest)
{
addViaHeader(clientRequest, proxyRequest);
addViaHeader(proxyRequest);
addXForwardedHeaders(clientRequest, proxyRequest);
}
/**
* Adds the HTTP Via header to the proxied request.
* Adds the HTTP {@code Via} header to the proxied request.
*
* @deprecated Use {@link #addViaHeader(HttpServletRequest, Request)} instead.
* @param proxyRequest the request being proxied
* @see #addViaHeader(HttpServletRequest, Request)
*/
@Deprecated
protected void addViaHeader(Request proxyRequest)
{
oldAddViaHeaderCalled = true;
HttpServletRequest clientRequest = (HttpServletRequest)proxyRequest.getAttributes().get(CLIENT_REQUEST_ATTRIBUTE);
addViaHeader(clientRequest, proxyRequest);
}
/**
* Adds the HTTP Via header to the proxied request, taking into account data present in the client request.
* This method considers the protocol of the client request when forming the proxied request. If it
* is HTTP, then the protocol name will not be included in the Via header that is sent by the proxy, and only
* <p>Adds the HTTP {@code Via} header to the proxied request, taking into account data present in the client request.</p>
* <p>This method considers the protocol of the client request when forming the proxied request. If it
* is HTTP, then the protocol name will not be included in the {@code Via} header that is sent by the proxy, and only
* the protocol version will be sent. If it is not, the entire protocol (name and version) will be included.
* If the client request includes a Via header, the result will be appended to that to form a chain.
* If the client request includes a {@code Via} header, the result will be appended to that to form a chain.</p>
*
* @param clientRequest the client request
* @param proxyRequest the request being proxied
@ -542,30 +549,25 @@ public abstract class AbstractProxyServlet extends HttpServlet
*/
protected void addViaHeader(HttpServletRequest clientRequest, Request proxyRequest)
{
// For backward compatibility reasons, call old, deprecated version of this method.
// If our flag isn't set, the deprecated method was overridden and we shouldn't do
// anything more.
oldAddViaHeaderCalled = false;
addViaHeader(proxyRequest);
if (!oldAddViaHeaderCalled)
return; // Old method was overridden, so bail out.
// Old version of this method wasn't overridden, so do the new logic instead.
String protocol = clientRequest.getProtocol();
String[] parts = protocol.split("/", 2);
String protocolName = parts.length == 2 && "HTTP".equals(parts[0]) ? parts[1] : protocol;
String viaHeaderValue = "";
String clientViaHeader = clientRequest.getHeader(HttpHeader.VIA.name());
if (clientViaHeader != null)
viaHeaderValue = clientViaHeader;
viaHeaderValue += protocolName + " " + getViaHost();
proxyRequest.header(HttpHeader.VIA, viaHeaderValue);
// Retain only the version if the protocol is HTTP.
String protocolPart = parts.length == 2 && "HTTP".equalsIgnoreCase(parts[0]) ? parts[1] : protocol;
String viaHeaderValue = protocolPart + " " + getViaHost();
proxyRequest.getHeaders().computeField(HttpHeader.VIA.asString(), (name, viaFields) ->
{
if (viaFields == null || viaFields.isEmpty())
return new HttpField(name, viaHeaderValue);
String separator = ", ";
String newValue = viaFields.stream()
.flatMap(field -> Stream.of(field.getValues()))
.filter(value -> !StringUtil.isBlank(value))
.collect(Collectors.joining(separator));
if (newValue.length() > 0)
newValue += separator;
newValue += viaHeaderValue;
return new HttpField(HttpHeader.VIA, newValue);
});
}
protected void addXForwardedHeaders(HttpServletRequest clientRequest, Request proxyRequest)

View File

@ -47,7 +47,6 @@ import org.eclipse.jetty.client.api.Response;
import org.eclipse.jetty.client.api.Result;
import org.eclipse.jetty.client.util.DeferredContentProvider;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.io.ByteBufferPool;
import org.eclipse.jetty.io.RuntimeIOException;
import org.eclipse.jetty.util.BufferUtil;
@ -92,9 +91,7 @@ public class AsyncMiddleManServlet extends AbstractProxyServlet
return;
}
final Request proxyRequest = getHttpClient().newRequest(rewrittenTarget)
.method(clientRequest.getMethod())
.version(HttpVersion.fromString(clientRequest.getProtocol()));
Request proxyRequest = newProxyRequest(clientRequest, rewrittenTarget);
copyRequestHeaders(clientRequest, proxyRequest);
@ -115,7 +112,6 @@ public class AsyncMiddleManServlet extends AbstractProxyServlet
if (expects100Continue(clientRequest))
{
proxyRequest.attribute(CLIENT_REQUEST_ATTRIBUTE, clientRequest);
proxyRequest.attribute(CONTINUE_ACTION_ATTRIBUTE, (Runnable)() ->
{
try

View File

@ -38,7 +38,6 @@ import org.eclipse.jetty.client.api.Response;
import org.eclipse.jetty.client.api.Result;
import org.eclipse.jetty.client.util.DeferredContentProvider;
import org.eclipse.jetty.client.util.InputStreamContentProvider;
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.IteratingCallback;
@ -76,9 +75,7 @@ public class ProxyServlet extends AbstractProxyServlet
return;
}
final Request proxyRequest = getHttpClient().newRequest(rewrittenTarget)
.method(request.getMethod())
.version(HttpVersion.fromString(request.getProtocol()));
Request proxyRequest = newProxyRequest(request, rewrittenTarget);
copyRequestHeaders(request, proxyRequest);
@ -95,7 +92,6 @@ public class ProxyServlet extends AbstractProxyServlet
{
DeferredContentProvider deferred = new DeferredContentProvider();
proxyRequest.content(deferred);
proxyRequest.attribute(CLIENT_REQUEST_ATTRIBUTE, request);
proxyRequest.attribute(CONTINUE_ACTION_ATTRIBUTE, (Runnable)() ->
{
try

View File

@ -26,8 +26,6 @@ import java.io.OutputStream;
import java.io.PrintWriter;
import java.net.ConnectException;
import java.net.HttpCookie;
import java.net.InetAddress;
import java.net.URI;
import java.nio.ByteBuffer;
import java.nio.file.Files;
import java.nio.file.Path;
@ -60,12 +58,12 @@ import javax.servlet.ServletResponse;
import javax.servlet.http.Cookie;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletRequestWrapper;
import javax.servlet.http.HttpServletResponse;
import org.eclipse.jetty.client.DuplexConnectionPool;
import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.client.HttpProxy;
import org.eclipse.jetty.client.HttpRequest;
import org.eclipse.jetty.client.api.ContentResponse;
import org.eclipse.jetty.client.api.Request;
import org.eclipse.jetty.client.api.Response;
@ -90,15 +88,18 @@ import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
import org.eclipse.jetty.util.IO;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.thread.QueuedThreadPool;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.junit.jupiter.params.provider.ValueSource;
import static org.eclipse.jetty.http.HttpFieldsMatchers.containsHeader;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.junit.jupiter.api.Assertions.assertArrayEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
@ -119,42 +120,6 @@ public class ProxyServletTest
).map(Arguments::of);
}
public static Stream<Arguments> implsWithProtocols()
{
String[] protocols = {"HTTP/1.1", "HTTP/2.0", "OTHER/0.9"};
return impls()
.flatMap(impl -> Arrays.stream(protocols)
.flatMap(p -> Stream.of(Arguments.of(impl.get()[0], p))));
}
public static Stream<Arguments> subclassesWithProtocols()
{
ProxyServlet subclass1 = new ProxyServlet()
{
@Override
protected void addViaHeader(Request proxyRequest)
{
System.err.println("addViaHeader called: " + proxyRequest);
super.addViaHeader(proxyRequest);
}
};
String proto = "MY_GOOD_PROTO/0.8";
ProxyServlet subclass2 = new ProxyServlet()
{
@Override
protected void addViaHeader(Request proxyRequest)
{
proxyRequest.header(HttpHeader.VIA, proto + " " + getViaHost());
}
};
return Stream.of(
Arguments.of(subclass1, "1.1"), // HTTP 1.1 used by this proxy (w/ the connector created in startServer)
Arguments.of(subclass2, proto)
);
}
private HttpClient client;
private Server proxy;
private ServerConnector proxyConnector;
@ -185,8 +150,7 @@ public class ProxyServletTest
private void startProxy(Class<? extends ProxyServlet> proxyServletClass, Map<String, String> initParams) throws Exception
{
proxyServlet = proxyServletClass.getDeclaredConstructor().newInstance();
startProxy(proxyServlet, initParams);
startProxy(proxyServletClass.getConstructor().newInstance(), initParams);
}
private void startProxy(AbstractProxyServlet proxyServlet, Map<String, String> initParams) throws Exception
@ -205,6 +169,7 @@ public class ProxyServletTest
proxy.addConnector(proxyConnector);
proxyContext = new ServletContextHandler(proxy, "/", true, false);
this.proxyServlet = proxyServlet;
ServletHolder proxyServletHolder = new ServletHolder(proxyServlet);
proxyServletHolder.setInitParameters(initParams);
proxyContext.addServlet(proxyServletHolder, "/*");
@ -228,26 +193,6 @@ public class ProxyServletTest
return result;
}
private static HttpServletRequest mockClientRequest(String protocol)
{
return new org.eclipse.jetty.server.Request(null, null)
{
@Override
public String getProtocol()
{
return protocol;
}
};
}
private static HttpRequest mockProxyRequest()
{
return new HttpRequest(new HttpClient(), null, URI.create("https://example.com"))
{
};
}
@AfterEach
public void dispose() throws Exception
{
@ -609,50 +554,21 @@ public class ProxyServletTest
ContentResponse response = client.GET("http://localhost:" + serverConnector.getLocalPort());
assertThat("Response expected to contain content of X-Forwarded-Host Header from the request",
response.getContentAsString(),
Matchers.equalTo("localhost:" + serverConnector.getLocalPort()));
}
@ParameterizedTest
@MethodSource("subclassesWithProtocols")
public void testInheritance(ProxyServlet derivedProxyServlet, String protocol) throws Exception
{
startServer(new HttpServlet()
{
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException
{
PrintWriter writer = resp.getWriter();
writer.write(req.getHeader("Via"));
writer.flush();
}
});
String viaHost = "my-good-via-host.example.org";
startProxy(derivedProxyServlet, Collections.singletonMap("viaHost", viaHost));
startClient();
HttpRequest proxyRequest = mockProxyRequest();
derivedProxyServlet.addViaHeader(proxyRequest);
ContentResponse response = client.GET("http://localhost:" + serverConnector.getLocalPort());
String expectedVia = protocol + " " + viaHost;
assertThat("Response expected to contain content of Via Header from the request",
response.getContentAsString(),
Matchers.equalTo(expectedVia));
equalTo("localhost:" + serverConnector.getLocalPort()));
}
@ParameterizedTest
@MethodSource("impls")
public void testProxyViaHeaderIsPresent(Class<? extends ProxyServlet> proxyServletClass) throws Exception
public void testProxyViaHeaderIsAdded(Class<? extends ProxyServlet> proxyServletClass) throws Exception
{
startServer(new HttpServlet()
startServer(new EmptyHttpServlet()
{
@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException
protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException
{
PrintWriter writer = resp.getWriter();
writer.write(req.getHeader("Via"));
writer.flush();
PrintWriter writer = response.getWriter();
List<String> viaValues = Collections.list(request.getHeaders("Via"));
writer.write(String.join(", ", viaValues));
}
});
String viaHost = "my-good-via-host.example.org";
@ -660,29 +576,76 @@ public class ProxyServletTest
startClient();
ContentResponse response = client.GET("http://localhost:" + serverConnector.getLocalPort());
assertThat("Response expected to contain content of Via Header from the request",
response.getContentAsString(),
Matchers.equalTo("1.1 " + viaHost));
assertThat(response.getContentAsString(), equalTo("1.1 " + viaHost));
}
@ParameterizedTest
@MethodSource("implsWithProtocols")
public void testProxyViaHeaderForVariousProtocols(Class<? extends ProxyServlet> proxyServletClass, String protocol) throws Exception
@MethodSource("impls")
public void testProxyViaHeaderValueIsAppended(Class<? extends ProxyServlet> proxyServletClass) throws Exception
{
AbstractProxyServlet proxyServlet = proxyServletClass.getDeclaredConstructor().newInstance();
String host = InetAddress.getLocalHost().getHostName();
HttpServletRequest clientRequest = mockClientRequest(protocol);
HttpRequest proxyRequest = mockProxyRequest();
startServer(new EmptyHttpServlet()
{
@Override
protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException
{
// Make sure the proxy coalesced the Via headers into just one.
org.eclipse.jetty.server.Request jettyRequest = (org.eclipse.jetty.server.Request)request;
assertEquals(1, jettyRequest.getHttpFields().getFields(HttpHeader.VIA).size());
PrintWriter writer = response.getWriter();
List<String> viaValues = Collections.list(request.getHeaders("Via"));
writer.write(String.join(", ", viaValues));
}
});
String viaHost = "beatrix";
startProxy(proxyServletClass, Collections.singletonMap("viaHost", viaHost));
startClient();
proxyServlet.addViaHeader(clientRequest, proxyRequest);
String existingViaHeader = "1.0 charon";
ContentResponse response = client.newRequest("http://localhost:" + serverConnector.getLocalPort())
.header(HttpHeader.VIA, existingViaHeader)
.send();
String expected = String.join(", ", existingViaHeader, "1.1 " + viaHost);
assertThat(response.getContentAsString(), equalTo(expected));
}
String expectedProtocol = protocol.startsWith("HTTP") ? protocol.split("/", 2)[1] : protocol;
String expectedVia = expectedProtocol + " " + host;
String expectedViaWithLocalhost = expectedProtocol + " localhost";
@ParameterizedTest
@ValueSource(strings = {"HTTP/2.0", "FCGI/1.0"})
public void testViaHeaderProtocols(String protocol) throws Exception
{
startServer(new EmptyHttpServlet()
{
@Override
protected void service(HttpServletRequest request, HttpServletResponse response) throws IOException
{
PrintWriter writer = response.getWriter();
List<String> viaValues = Collections.list(request.getHeaders("Via"));
writer.write(String.join(", ", viaValues));
}
});
String viaHost = "proxy";
startProxy(new ProxyServlet()
{
@Override
protected void addViaHeader(HttpServletRequest clientRequest, Request proxyRequest)
{
HttpServletRequest wrapped = new HttpServletRequestWrapper(clientRequest)
{
@Override
public String getProtocol()
{
return protocol;
}
};
super.addViaHeader(wrapped, proxyRequest);
}
}, Collections.singletonMap("viaHost", viaHost));
startClient();
assertThat("Response expected to contain a Via header with the right protocol version and host",
proxyRequest.getHeaders().getField("Via").getValue(),
Matchers.anyOf(Matchers.equalTo(expectedVia), Matchers.equalTo(expectedViaWithLocalhost)));
ContentResponse response = client.GET("http://localhost:" + serverConnector.getLocalPort());
String expectedProtocol = protocol.startsWith("HTTP/") ? protocol.substring("HTTP/".length()) : protocol;
String expected = expectedProtocol + " " + viaHost;
assertThat(response.getContentAsString(), equalTo(expected));
}
@ParameterizedTest
@ -1076,7 +1039,7 @@ public class ProxyServletTest
ContentResponse response = client.newRequest("localhost", serverConnector.getLocalPort())
.timeout(5, TimeUnit.SECONDS)
.send();
assertThat(response.getStatus(), Matchers.greaterThanOrEqualTo(500));
assertThat(response.getStatus(), greaterThanOrEqualTo(500));
}
catch (ExecutionException e)
{
@ -1209,7 +1172,7 @@ public class ProxyServletTest
// Make sure there is error page content, as the proxy-to-client response has been reset.
InputStream input = listener.getInputStream();
String body = IO.toString(input);
assertThat(body, Matchers.containsString("HTTP ERROR 504"));
assertThat(body, containsString("HTTP ERROR 504"));
chunk1Latch.countDown();
// Result succeeds because a 504 is a valid HTTP response.

View File

@ -662,15 +662,13 @@ public class GzipHandler extends HandlerWrapper implements GzipFactory
baseRequest.getHttpInput().addInterceptor(new GzipHttpInputInterceptor(baseRequest.getHttpChannel().getByteBufferPool(), _inflateBufferSize));
for (ListIterator<HttpField> i = baseRequest.getHttpFields().listIterator(); i.hasNext(); )
baseRequest.getHttpFields().computeField(HttpHeader.CONTENT_LENGTH.asString(), (name, fields) ->
{
HttpField field = i.next();
if (field.getHeader() == HttpHeader.CONTENT_LENGTH)
{
i.set(new HttpField("X-Content-Length", field.getValue()));
break;
}
}
if (fields == null)
return null;
String length = fields.stream().map(HttpField::getValue).findAny().orElse("0");
return new HttpField("X-Content-Length", length);
});
}
}