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

travisspencer · 2020-08-01T09:33:40Z

This PR fixes the issue discussed on the user mailing list this week where the Via header was hard coded to use http/1.1.
#5104

travisspencer · 2020-08-01T09:46:42Z

I don't understand why the IP check failed. I've committed before and the only thing I've changed is to add a PGP key. Is that the issue? I didn't see a place to configure that in my Eclipse account. If you note the commit in this PR, the author and sign off are the same as 1d5ceee, for instance. Any clues from those who understand the nuances more than me?

travisspencer · 2020-08-01T09:50:25Z

FYI: I checked and my ECA doesn't expire till 2022.

sbordet · 2020-08-01T15:07:42Z

@travisspencer can you please check your ECA is valid for the current ECA version?
E.g. that you filed it for ECA 1.0 but did not update to ECA 2.0?

I searched but could not find an ECA for your email, so we are at a hard stop here.

@WalkerWatch anything we can do to solve this?

sbordet · 2020-08-01T15:17:30Z

jetty-proxy/src/main/java/org/eclipse/jetty/proxy/AbstractProxyServlet.java

@@ -167,7 +167,7 @@ public String getHostHeader()

    public String getViaHost()
    {
-        return _viaHost;
+        return _viaHost == null ? viaHost() : _viaHost;


Don't really want to compute viaHost() all the times I call this getter.
I would leave it as it was originally.

we should assign the result to the field to avoid this. As discussed on the mailing list, we're not using this class as a Servlet, so the _viaHost value was null. I wasn't gonna mess with that, but it also came up in the tests where the host was null. I didn't want to expect HTTP/1.1 null, so I did this. I'll fix it to save the results in the field though, so it's only called once.

jetty-proxy/src/main/java/org/eclipse/jetty/proxy/AbstractProxyServlet.java

sbordet · 2020-08-01T15:19:42Z

jetty-proxy/src/main/java/org/eclipse/jetty/proxy/AbstractProxyServlet.java

+     * 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.
+     *
+     * @since 9.4.32


Remove @since, we don't track these things.

Thought not. Removed.

sbordet · 2020-08-01T15:25:32Z

jetty-proxy/src/main/java/org/eclipse/jetty/proxy/AbstractProxyServlet.java

+        viaHeaderValue += protocolName + " " + getViaHost();
+
+        proxyRequest.header(HttpHeader.VIA, viaHeaderValue);
+    }


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?

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?

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

Hum...

Updated. How's that?

sbordet · 2020-08-01T15:27:27Z

jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java

+
+        };
+    }
+


Inline these 2 methods, they are only used once.

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.

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

travisspencer · 2020-08-01T19:06:49Z

@travisspencer can you please check your ECA is valid for the current ECA version?
E.g. that you filed it for ECA 1.0 but did not update to ECA 2.0?

I've signed Version 3.0.0 October 22, 2018. When I search, I can't find newer. I wonder if it's something else.

I searched but could not find an ECA for your email, so we are at a hard stop here.

My Eclipse username is tspencer5c1 if that helps find things.

travisspencer · 2020-08-01T19:59:34Z

On the ECA issues you can see from my Eclipse account that my email in the author field of the commit matches (what I can see at least) in my profile:

joakime · 2020-08-01T20:37:11Z

On the ECA issues you can see from my Eclipse account that my email in the author field of the commit matches (what I can see at least) in my profile:

What is the correct username for your eclipse account.

As the one you mentioned earlier results in a 404 - https://accounts.eclipse.org/users/tspencer5c1/eca

Here's mine, for example - https://accounts.eclipse.org/users/jerdfelt/eca

travisspencer · 2020-08-02T08:13:23Z

That URL to my account opens fine, @joakime, after I login, and yours is a 403 (for me). Here it is:

My email in my Eclipse account (see previous comment with screenshot) matches the one in the commit. It's a little hard to see in GitHub, but it's this:

commit 93c777eb8c8fd6e85c4e659a8a6c6b5725b2de51 (HEAD -> fix/jetty-9.4.x/5104-incorrect-protocol-version-in-Via-header, origin/fix/jetty-9.4.x/5104-incorrect-protocol-version-in-Via-header)
gpg: Signature made Sat Aug  1 21:49:29 2020 CEST
gpg:                using RSA key 52B8B12E4022D5748175BB29E4EB8B447442E467
gpg: Good signature from "Travis Spencer <travis@curity.io>" [ultimate]
gpg:                 aka "Travis Spencer <travis.spencer@curity.io>" [ultimate]
gpg:                 aka "[jpeg image of size 10978]" [ultimate]
Author: Travis Spencer <travis@curity.io>
Date:   Sat Aug 1 11:25:48 2020 +0200

    Issue #5104 - Fix protocol version in Via header to work with H2 and other protocols

    Bug: https://github.com/eclipse/jetty.project/issues/5104
    Signed-off-by: Travis Spencer <travis@curity.io>

This also matches what's in my GitHub profile:

I have passed these checks in multiple, previous PRs, like Pull Request #4129. I am starting to think that this doesn't have anything to do with my account but the ECA check. Could that be the case?

joakime · 2020-08-02T13:13:18Z

This is super strange.

Leave this PR open!

I'm going to file an issue with the Eclipse side about the ECA validation on this PR.

See https://bugs.eclipse.org/bugs/show_bug.cgi?id=565761

jetty-proxy/src/main/java/org/eclipse/jetty/proxy/AbstractProxyServlet.java

gregw · 2020-08-02T15:49:47Z

jetty-proxy/src/main/java/org/eclipse/jetty/proxy/AbstractProxyServlet.java

+        oldAddViaHeaderCalled = false;
+        addViaHeader(proxyRequest);
+
+        if (!oldAddViaHeaderCalled)


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.

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.

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

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.

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.

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.

… and other protocols Bug: jetty#5104 Signed-off-by: Travis Spencer <travis@curity.io>

joakime · 2020-08-04T11:36:48Z

Still waiting on Eclipse to address the ECA validation concerns here.

travisspencer · 2020-08-05T07:01:13Z

While we wait on the ECA issues, @sbordet , is there anything else you think should be done to the code?

gregw · 2020-08-11T09:43:21Z

@sbordet can you re-review.... I know ECA still has problems, but I can see enough evidence here that it has been signed, so I think we could merge OK

joakime · 2020-08-11T13:19:49Z

I know ECA still has problems, but I can see enough evidence here that it has been signed, so I think we could merge OK

Go ahead and comment on the open issue about this ECA validation wonkiness - https://bugs.eclipse.org/bugs/show_bug.cgi?id=565761

sbordet · 2020-08-11T19:22:07Z

jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java

@@ -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


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.

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.

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.

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

sbordet · 2020-08-11T19:22:26Z

jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java

+            }
+        });
+        String viaHost = "my-good-via-host.example.org";
+        startProxy(derivedProxyServlet, Collections.singletonMap("viaHost", viaHost));


Just inline the ProxyServlet subclass here.

sbordet · 2020-08-11T19:23:23Z

jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java

+        startClient();
+
+        HttpRequest proxyRequest = mockProxyRequest();
+        derivedProxyServlet.addViaHeader(proxyRequest);


These 2 lines are useless?

sbordet · 2020-08-11T19:28:11Z

jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java

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


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.

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.

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

sbordet · 2020-08-11T19:38:42Z

jetty-proxy/src/main/java/org/eclipse/jetty/proxy/AbstractProxyServlet.java

+        oldAddViaHeaderCalled = false;
+        addViaHeader(proxyRequest);
+
+        if (!oldAddViaHeaderCalled)


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.

sbordet · 2020-08-11T19:40:35Z

jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ProxyServletTest.java

+
+        };
+    }
+


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

sbordet · 2020-08-11T20:42:17Z

@travisspencer let's do this:

in AbstractProxyServlet.addProxyHeaders(), just before calling addViaHeader(proxyRequest), set the clientRequest as an attribute in proxyRequest; use key AbstractProxyServlet.CLIENT_REQUEST_ATTRIBUTE since it's already there.
in addViaHeader(proxyRequest), remove the attribute so now you have the clientRequest, and then call your new method with 2 parameters.

This will solve the override issue. We don't need anymore tests for the override behavior, etc. so remove those too.

Implement the new logic in your new method (well, you've done it already) and let's verify it works with an integration test (not a unit test).
For HTTP/2 there is a test class in http2-client called ProxyTest - you should be able to add a test in there for the Via header functionality.

Maybe close this PR and open a new one, in the hope that it will get your ECA correctly. Thanks!

joakime · 2020-08-11T21:01:07Z

Maybe close this PR and open a new one, in the hope that it will get your ECA correctly. Thanks!

The ECA issue will still exist, as even the manual verification steps against his userid and email address still fail.

sbordet · 2020-08-12T13:06:09Z

@travisspencer I apologize for being too harsh in my review of your PR - bad word choice on my part.

Have you seen my proposal in the comment above?
I think it'll solve most of the issues and be much cleaner.

In addition to the suggestion above, I think we can always add clientRequest as an attribute, not only when there is an Expect: 100-continue, which means removing what subclasses do now, and add clientRequest as an attribute in AbstractProxyServlet.

You keen on doing that, perhaps in a different PR?

travisspencer · 2020-08-12T13:52:15Z

I apologize for being too harsh in my review of your PR

No apology needed, @sbordet . I wasn't bothered by anything you said or the way you said it.

Have you seen my proposal in the comment above?

Yes, but I'm preoccupied with other tasks today, so I haven't had time to internalize it enough to reply yet.

Please feel free to jump in here and make a PR on top of mine if you have more bandwidth then me. My ETA is probably end of week.

sbordet · 2020-08-12T15:11:01Z

Please feel free to jump in here and make a PR on top of mine

Will do today.

joakime · 2020-08-12T21:14:06Z

@travisspencer can you validate your email address entries as outlined in comment 4 in the ECA issue -> https://bugs.eclipse.org/bugs/show_bug.cgi?id=565761#c4

sbordet · 2020-08-12T22:11:46Z

Closed in favor of #5144.

travisspencer · 2020-08-13T07:29:30Z

Fixed the ECA issues @joakime -- see comment here: #5144 (comment)

sbordet requested changes Aug 1, 2020

View reviewed changes

travisspencer force-pushed the fix/jetty-9.4.x/5104-incorrect-protocol-version-in-Via-header branch 4 times, most recently from b6ec5ff to 93c777e Compare August 1, 2020 19:49

gregw requested changes Aug 2, 2020

View reviewed changes

Issue jetty#5104 - Fix protocol version in Via header to work with H2…

f071325

… and other protocols Bug: jetty#5104 Signed-off-by: Travis Spencer <travis@curity.io>

travisspencer force-pushed the fix/jetty-9.4.x/5104-incorrect-protocol-version-in-Via-header branch from 93c777e to f071325 Compare August 3, 2020 06:15

gregw linked an issue Aug 4, 2020 that may be closed by this pull request

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

Closed

gregw approved these changes Aug 4, 2020

View reviewed changes

joakime added Do Not Merge Missing ECA labels Aug 11, 2020

sbordet self-requested a review August 11, 2020 19:08

sbordet requested changes Aug 11, 2020

View reviewed changes

sbordet closed this Aug 12, 2020

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

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

Conversation

travisspencer commented Aug 1, 2020 • edited by gregw

travisspencer commented Aug 1, 2020

travisspencer commented Aug 1, 2020

sbordet commented Aug 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisspencer commented Aug 1, 2020

travisspencer commented Aug 1, 2020

joakime commented Aug 1, 2020

travisspencer commented Aug 2, 2020

joakime commented Aug 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakime commented Aug 4, 2020

travisspencer commented Aug 5, 2020

gregw commented Aug 11, 2020

joakime commented Aug 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

travisspencer Aug 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbordet commented Aug 11, 2020

joakime commented Aug 11, 2020

sbordet commented Aug 12, 2020 • edited

travisspencer commented Aug 12, 2020

sbordet commented Aug 12, 2020

joakime commented Aug 12, 2020

sbordet commented Aug 12, 2020

travisspencer commented Aug 13, 2020

travisspencer commented Aug 1, 2020 •

edited by gregw

travisspencer Aug 12, 2020 •

edited

sbordet commented Aug 12, 2020 •

edited