Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#5104 fix protocol version in Via header to work with H2 and other protocols #5107

Closed
wants to merge 1 commit into from
Closed

#5104 fix protocol version in Via header to work with H2 and other protocols #5107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 52 additions & 2 deletions jetty-proxy/src/main/java/org/eclipse/jetty/proxy/AbstractProxyServlet.java
View file
Open in desktop
Expand Up @@ -109,6 +109,7 @@ public abstract class AbstractProxyServlet extends HttpServlet
private String _viaHost;
private HttpClient _client;
private long _timeout;
private boolean oldAddViaHeaderCalled;

@Override
public void init() throws ServletException
Expand Down Expand Up @@ -167,6 +168,9 @@ public String getHostHeader()

public String getViaHost()
{
if (_viaHost == null)
_viaHost = viaHost();

return _viaHost;
}

Expand Down Expand Up @@ -509,13 +513,59 @@ protected Set<String> findConnectionHeaders(HttpServletRequest clientRequest)

protected void addProxyHeaders(HttpServletRequest clientRequest, Request proxyRequest)
{
addViaHeader(proxyRequest);
addViaHeader(clientRequest, proxyRequest);
addXForwardedHeaders(clientRequest, proxyRequest);
}

/**
* Adds the HTTP Via header to the proxied request.
*
* @deprecated Use {@link #addViaHeader(HttpServletRequest, Request)} instead.
* @param proxyRequest the request being proxied
*/
@Deprecated
protected void addViaHeader(Request proxyRequest)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
proxyRequest.header(HttpHeader.VIA, "http/1.1 " + getViaHost());
oldAddViaHeaderCalled = true;
gregw marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* 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
* 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.
*
* @param clientRequest the client request
* @param proxyRequest the request being proxied
* @see <a href="https://tools.ietf.org/html/rfc7230#section-5.7.1">RFC 7230 section 5.7.1</a>
*/
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this test, pl;us I don't think it works if super is called. I think you need to use reflection in init() to see if the method has been over ridden. Then I think you should still do your checking for non http/1 proxy Request and only call the old method IF over ridden AND HTTP/1, otherwise always do it the new way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works in this test at least:

@Test
public void testInheritance() throws Exception
{
    ProxyServlet derivedProxyServlet = new ProxyServlet()
    {
        @Override
        protected void addViaHeader(Request proxyRequest)
        {
            System.err.println("addViaHeader called: " + proxyRequest);
            super.addViaHeader(proxyRequest);
        }
    };

    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());

    assertThat("Response expected to contain content of Via Header from the request",
               response.getContentAsString(),
               Matchers.equalTo("1.1 " + viaHost));
}

This assumes we want to fix the other issues I mentioned that http as the protocol name was wrong and that we should not include protocol name nor the / when HTTP is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this test an another that does not call super. Can you recheck, @gregw ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it is the other way around. If somebody implements addViaHeader(Request proxyRequest) that doesn't call super, then you assume they have taken care of it. OK.... I guess that works, but it is rather ugly.... hmm but so is reflection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that works, but it is rather ugly

It is certainly ugly. I think it wouldn't stick out as much if I would have done a check for the present of the Via header at this point, but that seemed to have a running time of O(n). That's why I added this flag. It's a micro-optimization, but I'll take it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can be fixed in general.
A subclass calling super and expecting the header to be there will break (e.g. NPE).
Broken for broken, I'd say to implement the method empty, don't bother with the boolean flag and add the new logic.

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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method must call the old deprecated one.

Solution1:

  • get Via header value
  • call deprecated addViaHeader() method (now empty)
  • get again Via header value; if it's changed, do nothing, else your new logic.

Solution2:

  • call deprecated addViaHeader() method (now empty)
  • your new logic

Opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This is called from addProxyHeaders. For 1.1 clients, it's the same header value with one small change -- the protocol name for 1.1 is now uppercase. That's another bug though. The protocol is case sensitive and the registered name in IANA is exactly HTTP not http. This fixes clients for 2.0 and other protocols. I think this is good to go. If you still disagree, can you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you mean. They overrode addViaHeader and it's not called anymore by addProxyHeaders.

Hum...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. How's that?


protected void addXForwardedHeaders(HttpServletRequest clientRequest, Request proxyRequest)
Expand Down
140 changes: 138 additions & 2 deletions jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java
View file
Open in desktop
Expand Up @@ -26,6 +26,8 @@
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;
Expand Down Expand Up @@ -63,6 +65,7 @@
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;
Expand Down Expand Up @@ -116,6 +119,42 @@ public static Stream<Arguments> impls()
).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;
Expand Down Expand Up @@ -145,6 +184,12 @@ private void startProxy(Class<? extends ProxyServlet> proxyServletClass) throws
}

private void startProxy(Class<? extends ProxyServlet> proxyServletClass, Map<String, String> initParams) throws Exception
{
proxyServlet = proxyServletClass.getDeclaredConstructor().newInstance();
startProxy(proxyServlet, initParams);
}

private void startProxy(AbstractProxyServlet proxyServlet, Map<String, String> initParams) throws Exception
{
QueuedThreadPool proxyPool = new QueuedThreadPool();
proxyPool.setName("proxy");
Expand All @@ -159,8 +204,6 @@ private void startProxy(Class<? extends ProxyServlet> proxyServletClass, Map<Str
proxyConnector = new ServerConnector(proxy, new HttpConnectionFactory(configuration));
proxy.addConnector(proxyConnector);

proxyServlet = proxyServletClass.getDeclaredConstructor().newInstance();

proxyContext = new ServletContextHandler(proxy, "/", true, false);
ServletHolder proxyServletHolder = new ServletHolder(proxyServlet);
proxyServletHolder.setInitParameters(initParams);
Expand All @@ -185,6 +228,26 @@ private HttpClient prepareClient() throws Exception
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"))
{

};
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline these 2 methods, they are only used once.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing so obscures the point of the test, and I'd really rather not. Though currently only used once, they make the test more readable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. The test is less readable and using mocks obscures the test. Remove them.

@AfterEach
public void dispose() throws Exception
{
Expand Down Expand Up @@ -549,6 +612,79 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws Se
Matchers.equalTo("localhost:" + serverConnector.getLocalPort()));
}

@ParameterizedTest
@MethodSource("subclassesWithProtocols")
public void testInheritance(ProxyServlet derivedProxyServlet, String protocol) throws Exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please no.
It's impossible to read this test and understand what's doing without jumping in 3 other different places in the code.
Don't use mock anywhere, not needed and too brittle.
Don't use @ParameterizedTest, not needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alt-Space in Intellij and I can see the implementation of all 3 of those other methods without leaving the test. Having code in various places isn't a problem IHMO, especially when that code does only 1 thing. I want tests that are easy to read and understand the intent. Cruft obscures that and makes the test harder to understand. This is your guys party, and I will change this if you insist.

If this test isn't parameterized, then compatibility with all existing subclasses isn't tested. In which case, the test can be deleted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh the irony of somebody using some intellij key sequence in discussion with @sbordet, who is normally the one to say that ctrl-rightalt-elbow-headbutt will solve your problems! @travisspencer you just made my day :)

But seriously, I too am not a fan of mock in test harness and in this case I find the method names confusing as it looks like they will return some third party mock library generated test classes. But they don't, they just return simple instantiations of the real classes. So in this case I too think it would be simpler to inline them and avoid the confusion of a "mock" name in a method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll shift the code around as @sbordet suggested. My consolation is that I made your day, @gregw .

{
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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just inline the ProxyServlet subclass here.

startClient();

HttpRequest proxyRequest = mockProxyRequest();
derivedProxyServlet.addViaHeader(proxyRequest);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 lines are useless?


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));
}

@ParameterizedTest
@MethodSource("impls")
public void testProxyViaHeaderIsPresent(Class<? extends ProxyServlet> proxyServletClass) 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(proxyServletClass, Collections.singletonMap("viaHost", viaHost));
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));
}

@ParameterizedTest
@MethodSource("implsWithProtocols")
public void testProxyViaHeaderForVariousProtocols(Class<? extends ProxyServlet> proxyServletClass, String protocol) throws Exception
{
AbstractProxyServlet proxyServlet = proxyServletClass.getDeclaredConstructor().newInstance();
String host = InetAddress.getLocalHost().getHostName();
HttpServletRequest clientRequest = mockClientRequest(protocol);
HttpRequest proxyRequest = mockProxyRequest();

proxyServlet.addViaHeader(clientRequest, proxyRequest);

String expectedProtocol = protocol.startsWith("HTTP") ? protocol.split("/", 2)[1] : protocol;
String expectedVia = expectedProtocol + " " + host;
String expectedViaWithLocalhost = expectedProtocol + " localhost";

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)));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please no.
This is not testing ProxyServlet, it is just testing that this test calls addViaHeader(), which well... it does.

If by mistake the real call to addViaHeader() is removed from ProxyServlet (e.g. a bad merge or some mistake), this test will pass, but ProxyServlet won't work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it testing that addViaHeader actually does add the Via header correctly for all the protocols?
I think this test to check the method in isolation is fine. OK it is also tested by the previous test that checks the whole servlet, but testing the method directly is OK as well... so long as it doesn't need a third party mock library to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, that's what it's doing @gregw . I agree that it's helpful in that sense.


@ParameterizedTest
@MethodSource("impls")
public void testProxyWhiteList(Class<? extends ProxyServlet> proxyServletClass) throws Exception
Expand Down