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.

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

ForwardedRequestCustomizer seems to have no effect on port number, all URLs generated by Jetty like via Host/getRequestURL() contain connector port number #10304

Closed
uschindler opened this issue Aug 12, 2023 · 8 comments
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@uschindler
Copy link

uschindler commented Aug 12, 2023

Jetty version(s)
Jetty 12.0.0

Jetty Environment
core, ee8

Java version/vendor (use: java -version)
openjdk version "17.0.8" 2023-07-18
OpenJDK Runtime Environment (build 17.0.8+7-Ubuntu-122.04)
OpenJDK 64-Bit Server VM (build 17.0.8+7-Ubuntu-122.04, mixed mode, sharing)

OS type/version
does not matter

Description
I have seen this after upgrading my project to Jetty 12 from Jetty 10. The problem is that all RequestCustomizers are not able to correctly set the port number.

This is a blocker issue, because it makes it impossible to implement this common setup:

  • NGINX as user facing web server with HTTPS enabled
  • NGINX forwarding the requests with plain unencrypted HTTP/1.1 to Jetty, listening only on localhost with some arbitrary port number (in my case 8081). NGINX sets the following headers: X-Forwarded-For, X-Forwarded-Proto; the original Host header is forwarded as sent by client (no rewriting).
  • Jetty configured with: http_config.addCustomizer(new ForwardedRequestCustomizer());
  • Jetty 10 works fine: it reads the client IP address and all other information from X-Forwarded-For, the scheme is read from X-Forwarded-Proto, and host header is coming from Host header. It also extracts the port number from the host by using the default for the scheme, as client did not send it (443).
  • Jetty 12 is setup in same way, it successfully extracts the client's IP address and returns secure=true and uses "https://" for javax.servlet.HttServletRequest#getRequestURL(). But it always adds its own private port number. I also tried to use customizer.setForcedHost("xyz:443") to make sure it sees a port number also in host header. But it still constructs all URLs with port number 8081 where it listens on.

From my analysis, the customize() method in the RequestCustomizer does everything right and constructs the correct HostAndPort instance with hostname from Host header and port 443, but the javax.servlet API seems to still use the port number used by the connector's channel when constructing URLs (tried with several 3rd party servlets).

Summary: This bug is serious! With current Jetty 12 a common proxy setup using ForwardedRequestCustomize does not work as it tries to always inject its own hidden/private port number instead of the port as negotiated by client/proxy with the Host header. Theres no way to change it, so servlets are happy.

@uschindler uschindler added the Bug For general bugs on Jetty side label Aug 12, 2023
@gregw gregw self-assigned this Aug 12, 2023
@uschindler
Copy link
Author

uschindler commented Aug 14, 2023

Hi,
I analyzed the problem and compared Jetty 10 and Jetty 12. Basically I used the following simple application:

import java.io.IOException;
import java.nio.charset.StandardCharsets;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import org.eclipse.jetty.ee8.servlet.ServletContextHandler;
import org.eclipse.jetty.server.ForwardedRequestCustomizer;
import org.eclipse.jetty.server.HttpConfiguration;
import org.eclipse.jetty.server.HttpConnectionFactory;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;

public class JettyTest {

  public static void main(String[] args) throws Exception {
    // init webserver
    final var server=new Server();
    server.setStopAtShutdown(true);
    server.setStopTimeout(1000);
    
    // setup connector
    final var http_config = new HttpConfiguration();
    http_config.addCustomizer(new ForwardedRequestCustomizer());
    http_config.setSendServerVersion(false);
    http_config.setSendXPoweredBy(true);
    final var connector = new ServerConnector(server, new HttpConnectionFactory(http_config));
    connector.setHost(System.getProperty("server.host", "127.0.0.1"));
    connector.setPort(Integer.getInteger("server.port", 8081));
    server.addConnector(connector);
    
    // add context:
    final var ctx = new ServletContextHandler(ServletContextHandler.NO_SECURITY | ServletContextHandler.NO_SESSIONS);
    ctx.setContextPath("/");
    ctx.addServlet(DemoServlet.class, "/*");
    server.setHandler(ctx);
    
    // startup:
    server.start();
    server.join();
  }
  
  public static class DemoServlet extends HttpServlet {

    @Override
    protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
      String url = req.getRequestURL().toString();
      resp.setContentType("text/plain; charset=" + StandardCharsets.UTF_8.name());
      resp.getWriter().println(url);
    }
    
  }
}

I then did some tests:

> curl -H'Host: www.test.de' http://localhost:8081/
http://www.test.de:8081/
> curl -H'Host: www.test.de:1234' http://localhost:8081/
http://www.test.de:1234/
> curl -H'Host: www.test.de:443' http://localhost:8081/
http://www.test.de:443/
> curl -H'Host: www.test.de:80' http://localhost:8081/
http://www.test.de:8081/
> curl -H'Host: www.test.de:80' -H'X-Forwarded-Proto: https' http://localhost:8081/
https://www.test.de:8081/
> curl -H'Host: www.test.de' -H'X-Forwarded-Proto: https' http://localhost:8081/
https://www.test.de:8081/
> curl -H'Host: www.test.de:1234' -H'X-Forwarded-Proto: https' http://localhost:8081/
https://www.test.de:1234/
> curl -H'Host: www.test.de:443' -H'X-Forwarded-Proto: https' http://localhost:8081/
https://www.test.de/
> curl -H'Host: www.test.de' -H'X-Forwarded-Proto: https' -H'X-Forwarded-Port: 443' http://localhost:8081/
https://www.test.de:8081/
 > curl -H'Host: www.test.de' -H'X-Forwarded-Proto: https' -H'X-Forwarded-Port: 1234' http://localhost:8081/
https://www.test.de:1234/

You now see the problem appears when the "Host" header has no port number at all, or when the port number equals the standard port number. The same happens if you pass the new Jetty 12 header "X-Forwarded-Port", as soon as it is the default port, the connector's port number is injected.

So this looks like a bug in the ForwardedRequestCustomizer. The Java 12 version now has support added for "X-Forwarded-Port". Due to adding support for this it looks like the behaviour changed a bit. After parsing the "Host" header, the port number keeps undefined (if it is not given or the default). At a later stange the customizer then adds the connector port, although it should in reality add the default port to the request.

I think the handling should be fixed to explicitly and always set the port number on the wrapper for the connection metadata, although it is the default.

When you use 'setForcedHostHeader()' you can sometimes override the host, but often it also does not work with default ports (not sure why).

When I run the exact same code with Jetty 10 (just with the import statement of the Servlet API without ".ee8", the following output is seen:

> curl -H'Host: www.test.de' -H'X-Forwarded-Proto: https' http://localhost:8081/
https://www.test.de/
> curl -H'Host: www.test.de' -H'X-Forwarded-Proto: http' http://localhost:8081/
http://www.test.de/
> curl -H'Host: www.test.de:443' -H'X-Forwarded-Proto: https' http://localhost:8081/
https://www.test.de/
> curl -H'Host: www.test.de:1234' -H'X-Forwarded-Proto: https' http://localhost:8081/
https://www.test.de:1234/

This is the correct behaviour.

I am working on a fix.... If you have a good idea how to fix this, tell me! Basically: if a host header is passed or a port is passed it must be applied.

@uschindler
Copy link
Author

You can also reproduce the problem with the modern "Forwarded" header. I think the problem happens here:

            String host = forwarded._authority._host;
            int port = forwarded._authority._port;

            // Fall back to request metadata if needed.
            if (host == null)
            {
                host = builder.getHost();
            }

            if (port == MutableHostPort.UNSET) // is unset by headers
            {
                port = builder.getPort();
            }

            // Don't change port if port == IMPLIED.
            if (request.getHttpURI().getPort() == 0 && port > 0 && port == HttpScheme.CACHE.get(httpConfig.getSecureScheme()).getDefaultPort())
                port = 0;

Especially the places where it is compared to default ports seems broken.

@uschindler
Copy link
Author

I think I figured out:

  • When the proxy just passes the "Host" header through, the request customizer does not handle it (I misread the code). The handling of default "Host" header is done by the Jetty code in the Http parser.
  • When I pass "X-Forwarded-Host" in NGINX I can fix the problem.

So I think this is a behaviour change and as always, the "Forwarded" header should be used.

Nevertheless, the default behaviour of most reverse proxies is to pass the "Host" header unmodified and this leads to different behaviour in Jetty 9/10/11 vs Jetty 12. BTW, also Apache2's behaviour is to keep the "Host" header, it has no support for "X-Forwarded-Host".

@uschindler
Copy link
Author

uschindler commented Aug 14, 2023

One more info: I was able to fix this by telling NGINX to pass "Host" as "X-Forwarded-Host", but at same time I have to remove the original "Host" header. If both are given with same contents, the customizer code ignores the header.

So in my opinion, the default "Host" header parsing should be fixed to not automatically add the "port" number.

So at end it looks like the same bug about the host header which gets the port number injected: #10306

@joakime
Copy link
Contributor

joakime commented Aug 14, 2023

Look at the test cases at https://github.com/eclipse/jetty.project/blob/jetty-12.0.x/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java

There are many scenarios that are covered there.
Including many of your test scenarios (seem similar to the testNonStandardPortBehavior() tests)
Note how the scenarios change depending on your connector configuration.

Note also that X-Forwarded-Host can have a port, but there's no way to determine the isSecure or scheme from that port.
BTW, both Jetty and nginx recommend using the RFC Forwarded header, not the never-have-been-a-standard X-Forwarded-* headers.

@uschindler
Copy link
Author

BTW, both Jetty and nginx recommend using the RFC Forwarded header, not the never-have-been-a-standard X-Forwarded-* headers.

I know this from Jetty, but NGINX has by default no support for the header by default, you have to construct it on your own.

@uschindler
Copy link
Author

One addition: With the "Forwarded" header the same observations can be made (I just tried with curl). The problem is if the forwarding proxy does not remove the "original" Host header. If it is not rmroved then the default port is added implicit and then the forwarding customizer does not change the result anymore, because it thinks "authority is same".

Actually I think this bug is exactly the same like #10306, so this bug can be closed as duplicate.

@gregw
Copy link
Contributor

gregw commented Aug 14, 2023

Closed as duplicate of #10306

@gregw gregw closed this as completed Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

No branches or pull requests

3 participants