From ac42cbd1436f6a161ef03e1ca7f9c4a2bf686933 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Fri, 11 Sep 2020 11:11:09 -0500 Subject: [PATCH] Issue #5247 - Improve testing of ForwardedRequestCustomizer + Merge ProxyPass tests from CheckReverseProxyHeadersTest into ForwardedRequestCustomizerTest + Deleted CheckReverseProxyHeadersTest.java + Add more tests for ForcedHost configuration + Updated ForwardedRequestCustomizer to conform to expectations Signed-off-by: Joakim Erdfelt --- .../server/ForwardedRequestCustomizer.java | 186 ++++++++++------ .../server/CheckReverseProxyHeadersTest.java | 200 ------------------ .../ForwardedRequestCustomizerTest.java | 107 +++++++++- 3 files changed, 229 insertions(+), 264 deletions(-) delete mode 100644 jetty-server/src/test/java/org/eclipse/jetty/server/CheckReverseProxyHeadersTest.java diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java index 31c825e26982..5e3f8e275d44 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java @@ -83,17 +83,17 @@ * * * 2 - * X-Forwarded-Host Header + * X-Forwarded-Port Header + * n/a * Required - * Optional - * left-most value + * left-most value (only if {@link #getForwardedPortAsAuthority()} is true) * * * 3 - * X-Forwarded-Port Header - * n/a + * X-Forwarded-Host Header * Required - * left-most value (only if {@link #getForwardedPortAsAuthority()} is true) + * Implied + * left-most value * * * 4 @@ -460,26 +460,80 @@ public void customize(Connector connector, HttpConfiguration config, Request req } } + String proto = "http"; + + // Is secure status configured from headers? + if (forwarded.isSecure()) + { + // set default to https + proto = config.getSecureScheme(); + } + + // Set Scheme from configured protocol if (forwarded._proto != null) { - request.setScheme(forwarded._proto); - if (forwarded._proto.equalsIgnoreCase(config.getSecureScheme())) - request.setSecure(true); + proto = forwarded._proto; + request.setScheme(proto); + } + + // Set authority + String host = null; + int port = -1; + + // Use authority from headers, if configured. + if (forwarded._authority != null) + { + host = forwarded._authority._host; + port = forwarded._authority._port; + } + + // Fall back to request metadata if needed. + HttpURI requestURI = request.getMetaData().getURI(); + if (host == null) + { + host = requestURI.getHost(); + } + if (port == MutableHostPort.UNSET) // is unset by headers + { + port = requestURI.getPort(); + } + if (port == MutableHostPort.IMPLIED) // is implied + { + // get Implied port (from protocol / scheme) and HttpConfiguration + int defaultPort = 80; + port = proto.equalsIgnoreCase(config.getSecureScheme()) ? getSecurePort(config) : defaultPort; + } + + // Update authority if different from metadata + if (!host.equalsIgnoreCase(requestURI.getHost()) || + port != requestURI.getPort()) + { + httpFields.put(new HostPortHttpField(host, port)); + request.setAuthority(host, port); } - if (forwarded.hasAuthority()) + // Set secure status + if (forwarded.isSecure() || + proto.equalsIgnoreCase(config.getSecureScheme()) || + port == getSecurePort(config)) { - httpFields.put(new HostPortHttpField(forwarded._authority._host, forwarded._authority._port)); - request.setAuthority(forwarded._authority._host, forwarded._authority._port); + request.setSecure(true); + request.setScheme(proto); } + // Set Remote Address if (forwarded.hasFor()) { - int port = forwarded._for._port > 0 ? forwarded._for._port : request.getRemotePort(); - request.setRemoteAddr(InetSocketAddress.createUnresolved(forwarded._for._host, port)); + int forPort = forwarded._for._port > 0 ? forwarded._for._port : request.getRemotePort(); + request.setRemoteAddr(InetSocketAddress.createUnresolved(forwarded._for._host, forPort)); } } + protected static int getSecurePort(HttpConfiguration config) + { + return config.getSecurePort() > 0 ? config.getSecurePort() : 443; + } + protected void onError(HttpField field, Throwable t) { throw new BadMessageException("Bad header value for " + field.getName(), t); @@ -577,9 +631,12 @@ private boolean updateForwardedHandle(MethodHandles.Lookup lookup, String header private static class MutableHostPort { + public static final int UNSET = -1; + public static final int IMPLIED = 0; + String _host; int _hostPriority = -1; - int _port = -1; + int _port = UNSET; int _portPriority = -1; public void setHost(String host, int priority) @@ -593,7 +650,7 @@ public void setHost(String host, int priority) public void setPort(int port, int priority) { - if (port > 0 && priority > _portPriority) + if (priority > _portPriority) { _port = port; _portPriority = priority; @@ -602,22 +659,26 @@ public void setPort(int port, int priority) public void setHostPort(HostPort hostPort, int priority) { - if (_host == null || priority > _hostPriority) + if (priority > _hostPriority) { _host = hostPort.getHost(); _hostPriority = priority; + } - // Is this an authoritative port? - if (priority == FORWARDED_PRIORITY) - { - // Trust port (even if 0/unset) - _port = hostPort.getPort(); - _portPriority = priority; - } - else - { - setPort(hostPort.getPort(), priority); - } + int port = hostPort.getPort(); + // Is port supplied? + if (port > 0 && priority > _portPriority) + { + _port = hostPort.getPort(); + _portPriority = priority; + } + // Since we are Host:Port pair, the port could be unspecified + // Meaning it's implied. + // Make sure that we switch the tracked port from unset to implied + else if (_port == UNSET) + { + // set port to implied (with no priority) + _port = IMPLIED; } } @@ -633,16 +694,14 @@ public String toString() } } - private static final int MAX_PRIORITY = 999; + private static final int FORCED_PRIORITY = 999; private static final int FORWARDED_PRIORITY = 8; private static final int XFORWARDED_HOST_PRIORITY = 7; private static final int XFORWARDED_FOR_PRIORITY = 6; private static final int XFORWARDED_PORT_PRIORITY = 5; private static final int XFORWARDED_SERVER_PRIORITY = 4; - // HostPort seen in Request metadata - private static final int REQUEST_PRIORITY = 3; - private static final int XFORWARDED_PROTO_PRIORITY = 2; - private static final int XPROXIED_HTTPS_PRIORITY = 1; + private static final int XFORWARDED_PROTO_PRIORITY = 3; + private static final int XPROXIED_HTTPS_PRIORITY = 2; private class Forwarded extends QuotedCSVParser { @@ -653,6 +712,7 @@ private class Forwarded extends QuotedCSVParser MutableHostPort _for; String _proto; int _protoPriority = -1; + Boolean _secure; public Forwarded(Request request, HttpConfiguration config) { @@ -661,21 +721,14 @@ public Forwarded(Request request, HttpConfiguration config) _config = config; if (_forcedHost != null) { - getAuthority().setHostPort(_forcedHost.getHostPort(), MAX_PRIORITY); - } - else - { - HttpURI requestURI = request.getMetaData().getURI(); - if (requestURI.getHost() != null) - { - getAuthority().setHostPort(new HostPort(requestURI.getHost(), requestURI.getPort()), REQUEST_PRIORITY); - } + getAuthority().setHost(_forcedHost.getHostPort().getHost(), FORCED_PRIORITY); + getAuthority().setPort(_forcedHost.getHostPort().getPort(), FORCED_PRIORITY); } } - public boolean hasAuthority() + public boolean isSecure() { - return _authority != null && _authority._host != null; + return (_secure != null && _secure); } public boolean hasFor() @@ -707,8 +760,7 @@ public void handleCipherSuite(HttpField field) _request.setAttribute("javax.servlet.request.cipher_suite", field.getValue()); if (isSslIsSecure()) { - _request.setSecure(true); - _request.setScheme(_config.getSecureScheme()); + _secure = true; } } @@ -718,8 +770,7 @@ public void handleSslSessionId(HttpField field) _request.setAttribute("javax.servlet.request.ssl_session_id", field.getValue()); if (isSslIsSecure()) { - _request.setSecure(true); - _request.setScheme(_config.getSecureScheme()); + _secure = true; } } @@ -732,7 +783,8 @@ public void handleForwardedHost(HttpField field) @SuppressWarnings("unused") public void handleForwardedFor(HttpField field) { - getFor().setHostPort(new HostPort(getLeftMost(field.getValue())), XFORWARDED_FOR_PRIORITY); + HostPort hostField = new HostPort(getLeftMost(field.getValue())); + getFor().setHostPort(hostField, XFORWARDED_FOR_PRIORITY); } @SuppressWarnings("unused") @@ -762,8 +814,9 @@ public void handleHttps(HttpField field) { if ("on".equalsIgnoreCase(field.getValue()) || "true".equalsIgnoreCase(field.getValue())) { + _secure = true; updateProto(HttpScheme.HTTPS.asString(), XPROXIED_HTTPS_PRIORITY); - updatePort(443, XPROXIED_HTTPS_PRIORITY); + updatePort(getSecurePort(_config), XPROXIED_HTTPS_PRIORITY); } } @@ -783,39 +836,41 @@ protected void parsedParam(StringBuffer buffer, int valueLength, int paramName, switch (name) { case "by": + { if (!getProxyAsAuthority()) break; if (value.startsWith("_") || "unknown".equals(value)) break; - getAuthority().setHostPort(new HostPort(value), FORWARDED_PRIORITY); + HostPort hostField = new HostPort(value); + getAuthority().setHost(hostField.getHost(), FORWARDED_PRIORITY); + getAuthority().setPort(hostField.getPort(), FORWARDED_PRIORITY); break; + } case "for": + { if (value.startsWith("_") || "unknown".equals(value)) break; - getFor().setHostPort(new HostPort(value), FORWARDED_PRIORITY); + HostPort hostField = new HostPort(value); + getFor().setHost(hostField.getHost(), FORWARDED_PRIORITY); + getFor().setPort(hostField.getPort(), FORWARDED_PRIORITY); break; + } case "host": + { if (value.startsWith("_") || "unknown".equals(value)) break; - getAuthority().setHostPort(new HostPort(value), FORWARDED_PRIORITY); + HostPort hostField = new HostPort(value); + getAuthority().setHost(hostField.getHost(), FORWARDED_PRIORITY); + getAuthority().setPort(hostField.getPort(), FORWARDED_PRIORITY); break; + } case "proto": updateProto(value, FORWARDED_PRIORITY); - getAuthority().setPort(getPortForProto(value), FORWARDED_PRIORITY); break; } } } - private int getPortForProto(String proto) - { - if ("http".equalsIgnoreCase(proto)) - return 80; - if ("https".equalsIgnoreCase(proto)) - return 443; - return -1; - } - private void updateAuthority(String value, int priority) { HostPort hostField = new HostPort(value); @@ -840,6 +895,11 @@ private void updateProto(String proto, int priority) { _proto = proto; _protoPriority = priority; + + if (_proto.equalsIgnoreCase(_config.getSecureScheme())) + { + _secure = true; + } } } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/CheckReverseProxyHeadersTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/CheckReverseProxyHeadersTest.java deleted file mode 100644 index 87dd5ca50b11..000000000000 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/CheckReverseProxyHeadersTest.java +++ /dev/null @@ -1,200 +0,0 @@ -// -// ======================================================================== -// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. -// ------------------------------------------------------------------------ -// All rights reserved. This program and the accompanying materials -// are made available under the terms of the Eclipse Public License v1.0 -// and Apache License v2.0 which accompanies this distribution. -// -// The Eclipse Public License is available at -// http://www.eclipse.org/legal/epl-v10.html -// -// The Apache License v2.0 is available at -// http://www.opensource.org/licenses/apache2.0.php -// -// You may elect to redistribute this code under either of these licenses. -// ======================================================================== -// - -package org.eclipse.jetty.server; - -import java.io.IOException; -import javax.servlet.ServletException; -import javax.servlet.http.HttpServletRequest; -import javax.servlet.http.HttpServletResponse; - -import org.eclipse.jetty.server.handler.AbstractHandler; -import org.junit.jupiter.api.Test; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; - -/** - * - */ -public class CheckReverseProxyHeadersTest -{ - @Test - public void testCheckReverseProxyHeaders() throws Exception - { - // Classic ProxyPass from example.com:80 to localhost:8080 - testRequest("Host: localhost:8080\n" + - "X-Forwarded-For: 10.20.30.40\n" + - "X-Forwarded-Host: example.com", request -> - { - assertEquals("example.com", request.getServerName()); - assertEquals(80, request.getServerPort()); - assertEquals("10.20.30.40", request.getRemoteAddr()); - assertEquals("10.20.30.40", request.getRemoteHost()); - assertEquals("example.com", request.getHeader("Host")); - assertEquals("http", request.getScheme()); - assertFalse(request.isSecure()); - }); - - // IPv6 ProxyPass from example.com:80 to localhost:8080 - testRequest("Host: localhost:8080\n" + - "X-Forwarded-For: 10.20.30.40\n" + - "X-Forwarded-Host: [::1]", request -> - { - assertEquals("[::1]", request.getServerName()); - assertEquals(80, request.getServerPort()); - assertEquals("10.20.30.40", request.getRemoteAddr()); - assertEquals("10.20.30.40", request.getRemoteHost()); - assertEquals("[::1]", request.getHeader("Host")); - assertEquals("http", request.getScheme()); - assertFalse(request.isSecure()); - }); - - // IPv6 ProxyPass from example.com:80 to localhost:8080 - testRequest("Host: localhost:8080\n" + - "X-Forwarded-For: 10.20.30.40\n" + - "X-Forwarded-Host: [::1]:8888", request -> - { - assertEquals("[::1]", request.getServerName()); - assertEquals(8888, request.getServerPort()); - assertEquals("10.20.30.40", request.getRemoteAddr()); - assertEquals("10.20.30.40", request.getRemoteHost()); - assertEquals("[::1]:8888", request.getHeader("Host")); - assertEquals("http", request.getScheme()); - assertFalse(request.isSecure()); - }); - - // ProxyPass from example.com:81 to localhost:8080 - testRequest("Host: localhost:8080\n" + - "X-Forwarded-For: 10.20.30.40\n" + - "X-Forwarded-Host: example.com:81\n" + - "X-Forwarded-Server: example.com\n" + - "X-Forwarded-Proto: https", request -> - { - assertEquals("example.com", request.getServerName()); - assertEquals(81, request.getServerPort()); - assertEquals("10.20.30.40", request.getRemoteAddr()); - assertEquals("10.20.30.40", request.getRemoteHost()); - assertEquals("example.com:81", request.getHeader("Host")); - assertEquals("https", request.getScheme()); - assertTrue(request.isSecure()); - - }); - - // Multiple ProxyPass from example.com:80 to rp.example.com:82 to localhost:8080 - testRequest("Host: localhost:8080\n" + - "X-Forwarded-For: 10.20.30.40, 10.0.0.1\n" + - "X-Forwarded-Host: example.com, rp.example.com:82\n" + - "X-Forwarded-Server: example.com, rp.example.com\n" + - "X-Forwarded-Proto: https, http", request -> - { - assertEquals("example.com", request.getServerName()); - assertEquals(443, request.getServerPort()); - assertEquals("10.20.30.40", request.getRemoteAddr()); - assertEquals("10.20.30.40", request.getRemoteHost()); - assertEquals("example.com", request.getHeader("Host")); - assertEquals("https", request.getScheme()); - assertTrue(request.isSecure()); - }); - } - - private void testRequest(String headers, RequestValidator requestValidator) throws Exception - { - Server server = new Server(); - // Activate reverse proxy headers checking - HttpConnectionFactory http = new HttpConnectionFactory(); - http.getHttpConfiguration().addCustomizer(new ForwardedRequestCustomizer()); - - LocalConnector connector = new LocalConnector(server, http); - - server.setConnectors(new Connector[]{connector}); - ValidationHandler validationHandler = new ValidationHandler(requestValidator); - server.setHandler(validationHandler); - - try - { - server.start(); - connector.getResponse("GET / HTTP/1.1\r\n" + "Connection: close\r\n" + headers + "\r\n\r\n"); - Error error = validationHandler.getError(); - - if (error != null) - { - throw error; - } - } - finally - { - server.stop(); - } - } - - /** - * Interface for validate a wrapped request. - */ - private static interface RequestValidator - { - /** - * Validate the current request. - * - * @param request the request. - */ - void validate(HttpServletRequest request); - } - - /** - * Handler for validation. - */ - private static class ValidationHandler extends AbstractHandler - { - private final RequestValidator _requestValidator; - private Error _error; - - private ValidationHandler(RequestValidator requestValidator) - { - _requestValidator = requestValidator; - } - - /** - * Retrieve the validation error. - * - * @return the validation error or null if there was no error. - */ - public Error getError() - { - return _error; - } - - @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException - { - try - { - _requestValidator.validate(request); - } - catch (Error e) - { - _error = e; - } - catch (Throwable e) - { - _error = new Error(e); - } - } - } -} diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java index 63c0d42ead55..54d8e3b20ed7 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java @@ -76,6 +76,7 @@ public void init() throws Exception http.getHttpConfiguration().setRequestHeaderSize(512); http.getHttpConfiguration().setResponseHeaderSize(512); http.getHttpConfiguration().setOutputBufferSize(2048); + http.getHttpConfiguration().setSecurePort(443); customizer = new ForwardedRequestCustomizer(); http.getHttpConfiguration().addCustomizer(customizer); connector = new LocalConnector(server, http); @@ -277,6 +278,83 @@ public static Stream cases() .requestURL("https://myhost/") ), + // ================================================================= + // ProxyPass usages + Arguments.of(new Request("ProxyPass (example.com:80 to localhost:8080)") + .headers( + "GET / HTTP/1.1", + "Host: localhost:8080", + "X-Forwarded-For: 10.20.30.40", + "X-Forwarded-Host: example.com" + ), + new Expectations() + .scheme("http").serverName("example.com").serverPort(80) + .remoteAddr("10.20.30.40") + .requestURL("http://example.com/") + ), + Arguments.of(new Request("ProxyPass (example.com:81 to localhost:8080)") + .headers( + "GET / HTTP/1.1", + "Host: localhost:8080", + "X-Forwarded-For: 10.20.30.40", + "X-Forwarded-Host: example.com:81", + "X-Forwarded-Server: example.com", + "X-Forwarded-Proto: https" + ), + new Expectations() + .scheme("https").serverName("example.com").serverPort(81) + .remoteAddr("10.20.30.40") + .requestURL("https://example.com:81/") + ), + Arguments.of(new Request("ProxyPass (example.com:443 to localhost:8443)") + .headers( + "GET / HTTP/1.1", + "Host: localhost:8443", + "X-Forwarded-Host: example.com", + "X-Forwarded-Proto: https" + ), + new Expectations() + .scheme("https").serverName("example.com").serverPort(443) + .requestURL("https://example.com/") + ), + Arguments.of(new Request("ProxyPass (IPv6 from [::1]:80 to localhost:8080)") + .headers( + "GET / HTTP/1.1", + "Host: localhost:8080", + "X-Forwarded-For: 10.20.30.40", + "X-Forwarded-Host: [::1]" + ), + new Expectations() + .scheme("http").serverName("[::1]").serverPort(80) + .remoteAddr("10.20.30.40") + .requestURL("http://[::1]/") + ), + Arguments.of(new Request("ProxyPass (IPv6 from [::1]:8888 to localhost:8080)") + .headers( + "GET / HTTP/1.1", + "Host: localhost:8080", + "X-Forwarded-For: 10.20.30.40", + "X-Forwarded-Host: [::1]:8888" + ), + new Expectations() + .scheme("http").serverName("[::1]").serverPort(8888) + .remoteAddr("10.20.30.40") + .requestURL("http://[::1]:8888/") + ), + Arguments.of(new Request("Multiple ProxyPass (example.com:80 to rp.example.com:82 to localhost:8080)") + .headers( + "GET / HTTP/1.1", + "Host: localhost:8080", + "X-Forwarded-For: 10.20.30.40, 10.0.0.1", + "X-Forwarded-Host: example.com, rp.example.com:82", + "X-Forwarded-Server: example.com, rp.example.com", + "X-Forwarded-Proto: https, http" + ), + new Expectations() + .scheme("https").serverName("example.com").serverPort(443) + .remoteAddr("10.20.30.40") + .requestURL("https://example.com/") + ), // ================================================================= // X-Forwarded-* usages Arguments.of(new Request("X-Forwarded-Proto (old syntax)") @@ -574,7 +652,34 @@ public static Stream cases() .requestURL("http://example.com/") .remoteAddr("192.0.2.43").remotePort(0) ), - + // ================================================================= + // Forced Behavior + Arguments.of(new Request("Forced Host (no port)") + .configureCustomizer((customizer) -> customizer.setForcedHost("always.example.com")) + .headers( + "GET / HTTP/1.1", + "Host: myhost", + "X-Forwarded-For: 11.9.8.7:1111", + "X-Forwarded-Host: example.com:2222" + ), + new Expectations() + .scheme("http").serverName("always.example.com").serverPort(80) + .requestURL("http://always.example.com/") + .remoteAddr("11.9.8.7").remotePort(1111) + ), + Arguments.of(new Request("Forced Host with port") + .configureCustomizer((customizer) -> customizer.setForcedHost("always.example.com:9090")) + .headers( + "GET / HTTP/1.1", + "Host: myhost", + "X-Forwarded-For: 11.9.8.7:1111", + "X-Forwarded-Host: example.com:2222" + ), + new Expectations() + .scheme("http").serverName("always.example.com").serverPort(9090) + .requestURL("http://always.example.com:9090/") + .remoteAddr("11.9.8.7").remotePort(1111) + ), // ================================================================= // Legacy Headers Arguments.of(new Request("X-Proxied-Https")