From 0b646ee6b78c5e4e1dff6ddbf641b839360d8a87 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 13 Oct 2020 05:10:42 -0500 Subject: [PATCH 1/7] Issue #5443 - Forwarding Headers are optional Signed-off-by: Joakim Erdfelt --- .../server/ForwardedRequestCustomizer.java | 113 ++++++++++-------- .../ForwardedRequestCustomizerTest.java | 28 ++++- 2 files changed, 84 insertions(+), 57 deletions(-) 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 3e20cece188a..0dd3b608bad1 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 @@ -461,13 +461,17 @@ public void customize(Connector connector, HttpConfiguration config, Request req // Do a single pass through the header fields as it is a more efficient single iteration. Forwarded forwarded = new Forwarded(request, config); + boolean match = false; for (HttpField field : httpFields) { try { MethodHandle handle = _handles.get(field.getName()); if (handle != null) + { + match = true; handle.invoke(forwarded, field); + } } catch (Throwable t) { @@ -475,67 +479,70 @@ public void customize(Connector connector, HttpConfiguration config, Request req } } - String proto = "http"; - - // Is secure status configured from headers? - if (forwarded.isSecure()) + if (match) { - // set default to https - proto = config.getSecureScheme(); - } + String proto = "http"; - // Set Scheme from configured protocol - if (forwarded._proto != null) - { - proto = forwarded._proto; - request.setScheme(proto); - } + // Is secure status configured from headers? + if (forwarded.isSecure()) + { + // set default to https + proto = config.getSecureScheme(); + } - // Set authority - String host = null; - int port = -1; + // Set Scheme from configured protocol + if (forwarded._proto != null) + { + proto = forwarded._proto; + request.setScheme(proto); + } - // Use authority from headers, if configured. - if (forwarded._authority != null) - { - host = forwarded._authority._host; - port = forwarded._authority._port; - } + // Set authority + String host = null; + int port = -1; - // 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(); - } - // Don't change port if port == IMPLIED. + // Use authority from headers, if configured. + if (forwarded._authority != null) + { + host = forwarded._authority._host; + port = forwarded._authority._port; + } - // Update authority if different from metadata - if (!host.equalsIgnoreCase(requestURI.getHost()) || - port != requestURI.getPort()) - { - httpFields.put(new HostPortHttpField(host, port)); - request.setAuthority(host, 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(); + } + // Don't change port if port == IMPLIED. - // Set secure status - if (forwarded.isSecure() || - proto.equalsIgnoreCase(config.getSecureScheme()) || - port == getSecurePort(config)) - { - request.setSecure(true); - request.setScheme(proto); - } + // Update authority if different from metadata + if (!host.equalsIgnoreCase(requestURI.getHost()) || + port != requestURI.getPort()) + { + httpFields.put(new HostPortHttpField(host, port)); + request.setAuthority(host, port); + } - // Set Remote Address - if (forwarded.hasFor()) - { - int forPort = forwarded._for._port > 0 ? forwarded._for._port : request.getRemotePort(); - request.setRemoteAddr(InetSocketAddress.createUnresolved(forwarded._for._host, forPort)); + // Set secure status + if (forwarded.isSecure() || + proto.equalsIgnoreCase(config.getSecureScheme()) || + port == getSecurePort(config)) + { + request.setSecure(true); + request.setScheme(proto); + } + + // Set Remote Address + if (forwarded.hasFor()) + { + int forPort = forwarded._for._port > 0 ? forwarded._for._port : request.getRemotePort(); + request.setRemoteAddr(InetSocketAddress.createUnresolved(forwarded._for._host, forPort)); + } } } 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 d425e894c16f..b49539c3ca33 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 @@ -135,6 +135,26 @@ public void destroy() throws Exception public static Stream cases() { return Stream.of( + // HTTP 1.0 + Arguments.of( + new Request("HTTP/1.0 - no Host header") + .headers( + "GET /example HTTP/1.0" + ), + new Expectations() + .scheme("http").serverName("0.0.0.0").serverPort(80) + .requestURL("http://0.0.0.0/example") + ), + Arguments.of( + new Request("HTTP/1.0 - Empty Host header") + .headers( + "GET /example HTTP/1.0", + "Host:" + ), + new Expectations() + .scheme("http").serverName("0.0.0.0").serverPort(80) + .requestURL("http://0.0.0.0/example") + ), // Host IPv4 Arguments.of( new Request("IPv4 Host Only") @@ -157,12 +177,12 @@ public static Stream cases() ), Arguments.of(new Request("IPv4 in Request Line") .headers( - "GET http://1.2.3.4:2222/ HTTP/1.1", + "GET https://1.2.3.4:2222/ HTTP/1.1", "Host: wrong" ), new Expectations() - .scheme("http").serverName("1.2.3.4").serverPort(2222) - .requestURL("http://1.2.3.4:2222/") + .scheme("https").serverName("1.2.3.4").serverPort(2222) + .requestURL("https://1.2.3.4:2222/") ), Arguments.of(new Request("IPv6 in Request Line") .headers( @@ -931,7 +951,7 @@ private static class Expectations implements Consumer public void accept(Actual actual) { assertThat("scheme", actual.scheme.get(), is(expectedScheme)); - if (actual.scheme.get().equals("https")) + if (expectedScheme.equals("https")) { assertTrue(actual.wasSecure.get(), "wasSecure"); } From f0681b33ebfaefa7082de5afe615ca01ef49955b Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 13 Oct 2020 07:03:37 -0500 Subject: [PATCH 2/7] Issue #5443 - Forwarding Headers are optional + Simplify isSecure handling in customize. + Simplify handling of `Proxy-Ssl-Id` header. + Simplify handling of `Proxy-auth-cert` header. Signed-off-by: Joakim Erdfelt --- .../jetty/server/ForwardedRequestCustomizer.java | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) 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 0dd3b608bad1..a059eb777c58 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 @@ -481,13 +481,12 @@ public void customize(Connector connector, HttpConfiguration config, Request req if (match) { - String proto = "http"; + String proto; // Is secure status configured from headers? if (forwarded.isSecure()) { - // set default to https - proto = config.getSecureScheme(); + request.setSecure(true); } // Set Scheme from configured protocol @@ -528,15 +527,6 @@ public void customize(Connector connector, HttpConfiguration config, Request req request.setAuthority(host, port); } - // Set secure status - if (forwarded.isSecure() || - proto.equalsIgnoreCase(config.getSecureScheme()) || - port == getSecurePort(config)) - { - request.setSecure(true); - request.setScheme(proto); - } - // Set Remote Address if (forwarded.hasFor()) { @@ -801,6 +791,7 @@ public void handleCipherSuite(HttpField field) if (isSslIsSecure()) { _secure = true; + _proto = "https"; } } @@ -811,6 +802,7 @@ public void handleSslSessionId(HttpField field) if (isSslIsSecure()) { _secure = true; + _proto = "https"; } } From abdada05b1d08ade47c1e8e82774fb5a2e98bb26 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 13 Oct 2020 07:15:38 -0500 Subject: [PATCH 3/7] Issue #5443 - Forwarding Headers are optional + Improve / document implied secure scheme behaviors for both `Proxy-Ssl-Id` or `Proxy-auth-cert` Signed-off-by: Joakim Erdfelt --- .../server/ForwardedRequestCustomizer.java | 29 ++++++++++++++----- 1 file changed, 21 insertions(+), 8 deletions(-) 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 a059eb777c58..0ebd13a19166 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 @@ -481,8 +481,6 @@ public void customize(Connector connector, HttpConfiguration config, Request req if (match) { - String proto; - // Is secure status configured from headers? if (forwarded.isSecure()) { @@ -492,8 +490,12 @@ public void customize(Connector connector, HttpConfiguration config, Request req // Set Scheme from configured protocol if (forwarded._proto != null) { - proto = forwarded._proto; - request.setScheme(proto); + request.setScheme(forwarded._proto); + } + // Set scheme if header implies secure scheme is to be used (see #isSslIsSecure()) + else if (forwarded._secureScheme) + { + request.setScheme(config.getSecureScheme()); } // Set authority @@ -741,6 +743,7 @@ private class Forwarded extends QuotedCSVParser String _proto; Source _protoSource = Source.UNSET; Boolean _secure; + boolean _secureScheme = false; public Forwarded(Request request, HttpConfiguration config) { @@ -784,25 +787,35 @@ private MutableHostPort getFor() return _for; } - @SuppressWarnings("unused") + /** + * Called if header is Proxy-auth-cert + */ public void handleCipherSuite(HttpField field) { _request.setAttribute("javax.servlet.request.cipher_suite", field.getValue()); + + // Is ForwardingRequestCustomizer configured to trigger isSecure and scheme change on this header? if (isSslIsSecure()) { _secure = true; - _proto = "https"; + // track desire for secure scheme, actual protocol will be resolved later. + _secureScheme = true; } } - @SuppressWarnings("unused") + /** + * Called if header is Proxy-Ssl-Id + */ public void handleSslSessionId(HttpField field) { _request.setAttribute("javax.servlet.request.ssl_session_id", field.getValue()); + + // Is ForwardingRequestCustomizer configured to trigger isSecure and scheme change on this header? if (isSslIsSecure()) { _secure = true; - _proto = "https"; + // track desire for secure scheme, actual protocol will be resolved later. + _secureScheme = true; } } From ea1103077cb6938d96ddc1c978c7fad7a5cb39e0 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 13 Oct 2020 08:20:27 -0500 Subject: [PATCH 4/7] Issue #5443 - Forwarding Headers are optional + Additional tests for HTTP/1.0 + Overly complex negative test cases for `X-Forwarded-Proto: http` and `X-Proxied-Https: off` + Failure testcase for `X-Proxied-Https: foo` Signed-off-by: Joakim Erdfelt --- .../server/ForwardedRequestCustomizer.java | 38 ++- .../ForwardedRequestCustomizerTest.java | 247 +++++++++++++++++- 2 files changed, 267 insertions(+), 18 deletions(-) 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 0ebd13a19166..11932f26cc40 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 @@ -819,20 +819,26 @@ public void handleSslSessionId(HttpField field) } } - @SuppressWarnings("unused") + /** + * Called if header is X-Forwarded-Host + */ public void handleForwardedHost(HttpField field) { updateAuthority(getLeftMost(field.getValue()), Source.XFORWARDED_HOST); } - @SuppressWarnings("unused") + /** + * Called if header is X-Forwarded-For + */ public void handleForwardedFor(HttpField field) { HostPort hostField = new HostPort(getLeftMost(field.getValue())); getFor().setHostPort(hostField, Source.XFORWARDED_FOR); } - @SuppressWarnings("unused") + /** + * Called if header is X-Forwarded-Server + */ public void handleForwardedServer(HttpField field) { if (getProxyAsAuthority()) @@ -840,7 +846,9 @@ public void handleForwardedServer(HttpField field) updateAuthority(getLeftMost(field.getValue()), Source.XFORWARDED_SERVER); } - @SuppressWarnings("unused") + /** + * Called if header is X-Forwarded-Port + */ public void handleForwardedPort(HttpField field) { int port = HostPort.parsePort(getLeftMost(field.getValue())); @@ -848,13 +856,17 @@ public void handleForwardedPort(HttpField field) updatePort(port, Source.XFORWARDED_PORT); } - @SuppressWarnings("unused") + /** + * Called if header is X-Forwarded-Proto + */ public void handleProto(HttpField field) { updateProto(getLeftMost(field.getValue()), Source.XFORWARDED_PROTO); } - @SuppressWarnings("unused") + /** + * Called if header is X-Proxied-Https + */ public void handleHttps(HttpField field) { if ("on".equalsIgnoreCase(field.getValue()) || "true".equalsIgnoreCase(field.getValue())) @@ -863,9 +875,21 @@ public void handleHttps(HttpField field) updateProto(HttpScheme.HTTPS.asString(), Source.XPROXIED_HTTPS); updatePort(getSecurePort(_config), Source.XPROXIED_HTTPS); } + else if ("off".equalsIgnoreCase(field.getValue()) || "false".equalsIgnoreCase(field.getValue())) + { + _secure = false; + updateProto(HttpScheme.HTTP.asString(), Source.XPROXIED_HTTPS); + updatePort(80, Source.XPROXIED_HTTPS); + } + else + { + throw new BadMessageException("Invalid value for " + field.getName()); + } } - @SuppressWarnings("unused") + /** + * Called if header is Forwarded + */ public void handleRFC7239(HttpField field) { addValue(field.getValue()); 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 b49539c3ca33..b5e90fad16ec 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 @@ -32,7 +32,6 @@ import org.eclipse.jetty.server.handler.AbstractHandler; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -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; @@ -143,6 +142,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("0.0.0.0").serverPort(80) + .secure(false) .requestURL("http://0.0.0.0/example") ), Arguments.of( @@ -153,8 +153,32 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("0.0.0.0").serverPort(80) + .secure(false) .requestURL("http://0.0.0.0/example") ), + Arguments.of( + new Request("HTTP/1.0 - No Host header, with X-Forwarded-Host") + .headers( + "GET /example HTTP/1.0", + "X-Forwarded-Host: alt.example.net:7070" + ), + new Expectations() + .scheme("http").serverName("alt.example.net").serverPort(7070) + .secure(false) + .requestURL("http://alt.example.net:7070/example") + ), + Arguments.of( + new Request("HTTP/1.0 - Empty Host header, with X-Forwarded-Host") + .headers( + "GET /example HTTP/1.0", + "Host:", + "X-Forwarded-Host: alt.example.net:7070" + ), + new Expectations() + .scheme("http").serverName("alt.example.net").serverPort(7070) + .secure(false) + .requestURL("http://alt.example.net:7070/example") + ), // Host IPv4 Arguments.of( new Request("IPv4 Host Only") @@ -164,6 +188,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("1.2.3.4").serverPort(2222) + .secure(false) .requestURL("http://1.2.3.4:2222/") ), Arguments.of(new Request("IPv6 Host Only") @@ -173,6 +198,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("[::1]").serverPort(2222) + .secure(false) .requestURL("http://[::1]:2222/") ), Arguments.of(new Request("IPv4 in Request Line") @@ -182,6 +208,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("1.2.3.4").serverPort(2222) + .secure(true) .requestURL("https://1.2.3.4:2222/") ), Arguments.of(new Request("IPv6 in Request Line") @@ -191,6 +218,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("[::1]").serverPort(2222) + .secure(false) .requestURL("http://[::1]:2222/") ), @@ -207,6 +235,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("[2001:db8:cafe::17]").remotePort(4711) ), @@ -220,6 +249,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("192.0.2.43").remotePort(0) ), @@ -233,6 +263,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("192.0.2.43").remotePort(0) ), @@ -247,6 +278,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("192.0.2.43").remotePort(0) ), @@ -260,6 +292,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("192.0.2.43").remotePort(0) ), @@ -271,6 +304,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("192.0.2.43").remotePort(0) ), @@ -284,6 +318,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("example.com").serverPort(80) + .secure(false) .requestURL("http://example.com/") .remoteAddr("192.0.2.43").remotePort(0) ), @@ -297,6 +332,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("myhost").serverPort(443) + .secure(true) .requestURL("https://myhost/") ), @@ -311,6 +347,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("example.com").serverPort(80) + .secure(false) .remoteAddr("10.20.30.40") .requestURL("http://example.com/") ), @@ -325,6 +362,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("example.com").serverPort(81) + .secure(true) .remoteAddr("10.20.30.40") .requestURL("https://example.com:81/") ), @@ -337,6 +375,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("example.com").serverPort(443) + .secure(true) .requestURL("https://example.com/") ), Arguments.of(new Request("ProxyPass (IPv6 from [::1]:80 to localhost:8080)") @@ -348,6 +387,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("[::1]").serverPort(80) + .secure(false) .remoteAddr("10.20.30.40") .requestURL("http://[::1]/") ), @@ -360,6 +400,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("[::1]").serverPort(8888) + .secure(false) .remoteAddr("10.20.30.40") .requestURL("http://[::1]:8888/") ), @@ -374,6 +415,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("example.com").serverPort(443) + .secure(true) .remoteAddr("10.20.30.40") .requestURL("https://example.com/") ), @@ -387,6 +429,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("myhost").serverPort(443) + .secure(true) .requestURL("https://myhost/") ), Arguments.of(new Request("X-Forwarded-For (multiple headers)") @@ -398,6 +441,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("10.9.8.7").remotePort(0) ), @@ -409,6 +453,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("10.9.8.7").remotePort(1111) ), @@ -420,6 +465,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("[2001:db8:cafe::17]").remotePort(1111) ), @@ -432,6 +478,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(2222) + .secure(false) .requestURL("http://myhost:2222/") .remoteAddr("[1:2:3:4:5:6:7:8]").remotePort(0) ), @@ -446,6 +493,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(2222) + .secure(false) .requestURL("http://myhost:2222/") .remoteAddr("[1:2:3:4:5:6:7:8]").remotePort(0) ), @@ -458,6 +506,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(2222) + .secure(false) .requestURL("http://myhost:2222/") .remoteAddr("[1:2:3:4:5:6:7:8]").remotePort(0) ), @@ -470,6 +519,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(4444) + .secure(false) .requestURL("http://myhost:4444/") .remoteAddr("192.168.1.200").remotePort(0) ), @@ -483,6 +533,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("192.168.1.200").remotePort(4444) ), @@ -495,6 +546,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(4444) + .secure(false) .requestURL("http://myhost:4444/") .remoteAddr("192.168.1.200").remotePort(0) ), @@ -509,6 +561,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("www.example.com").serverPort(4333) + .secure(true) .requestURL("https://www.example.com:4333/") .remoteAddr("8.5.4.3").remotePort(2222) ), @@ -523,6 +576,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("www.example.com").serverPort(4333) + .secure(true) .requestURL("https://www.example.com:4333/") .remoteAddr("8.5.4.3").remotePort(2222) ), @@ -538,6 +592,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("www.example.com").serverPort(4333) + .secure(true) .requestURL("https://www.example.com:4333/") .remoteAddr("8.5.4.3").remotePort(2222) ), @@ -553,6 +608,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("www.example.com").serverPort(4333) + .secure(true) .requestURL("https://www.example.com:4333/") .remoteAddr("8.5.4.3").remotePort(2222) ), @@ -568,6 +624,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("www.example.com").serverPort(4333) + .secure(true) .requestURL("https://www.example.com:4333/") .remoteAddr("8.5.4.3").remotePort(2222) ), @@ -581,6 +638,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("fw.example.com").serverPort(4333) + .secure(false) .requestURL("http://fw.example.com:4333/") .remoteAddr("8.5.4.3").remotePort(2222) ), @@ -594,6 +652,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("fw.example.com").serverPort(4333) + .secure(false) .requestURL("http://fw.example.com:4333/") .remoteAddr("8.5.4.3").remotePort(2222) ), @@ -609,6 +668,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("sub1.example.com").serverPort(10003) + .secure(true) .requestURL("https://sub1.example.com:10003/") .remoteAddr("127.0.0.1").remotePort(8888) ), @@ -624,6 +684,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("sub1.example.com").serverPort(10003) + .secure(true) .requestURL("https://sub1.example.com:10003/") .remoteAddr("127.0.0.1").remotePort(8888) ), @@ -640,6 +701,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("sub1.example.com").serverPort(10003) + .secure(true) .requestURL("https://sub1.example.com:10003/") .remoteAddr("127.0.0.1").remotePort(8888) ), @@ -655,6 +717,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("sub1.example.com").serverPort(10003) + .secure(true) .requestURL("https://sub1.example.com:10003/") .remoteAddr("127.0.0.1").remotePort(8888) ), @@ -671,9 +734,35 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("example.com").serverPort(80) + .secure(false) .requestURL("http://example.com/") .remoteAddr("192.0.2.43").remotePort(0) ), + Arguments.of( + new Request("RFC7239 - mixed with HTTP/1.0 - No Host header") + .headers( + "GET /example HTTP/1.0", + "Forwarded: for=1.1.1.1:6060,proto=http;host=alt.example.net:7070" + ), + new Expectations() + .scheme("http").serverName("alt.example.net").serverPort(7070) + .secure(false) + .requestURL("http://alt.example.net:7070/example") + .remoteAddr("1.1.1.1").remotePort(6060) + ), + Arguments.of( + new Request("RFC7239 - mixed with HTTP/1.0 - Empty Host header") + .headers( + "GET /example HTTP/1.0", + "Host:", + "Forwarded: for=1.1.1.1:6060,proto=http;host=alt.example.net:7070" + ), + new Expectations() + .scheme("http").serverName("alt.example.net").serverPort(7070) + .secure(false) + .requestURL("http://alt.example.net:7070/example") + .remoteAddr("1.1.1.1").remotePort(6060) + ), // ================================================================= // Forced Behavior Arguments.of(new Request("Forced Host (no port)") @@ -686,6 +775,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("always.example.com").serverPort(80) + .secure(false) .requestURL("http://always.example.com/") .remoteAddr("11.9.8.7").remotePort(1111) ), @@ -699,6 +789,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("always.example.com").serverPort(9090) + .secure(false) .requestURL("http://always.example.com:9090/") .remoteAddr("11.9.8.7").remotePort(1111) ), @@ -712,6 +803,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("myhost").serverPort(443) + .secure(true) .requestURL("https://myhost/") .remoteAddr("0.0.0.0").remotePort(0) ), @@ -724,6 +816,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("0.0.0.0").remotePort(0) .sslSession("Wibble") @@ -737,6 +830,7 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("myhost").serverPort(443) + .secure(true) .requestURL("https://myhost/") .remoteAddr("0.0.0.0").remotePort(0) .sslSession("0123456789abcdef") @@ -750,6 +844,7 @@ public static Stream cases() ), new Expectations() .scheme("http").serverName("myhost").serverPort(80) + .secure(false) .requestURL("http://myhost/") .remoteAddr("0.0.0.0").remotePort(0) .sslCertificate("Wibble") @@ -763,9 +858,121 @@ public static Stream cases() ), new Expectations() .scheme("https").serverName("myhost").serverPort(443) + .secure(true) .requestURL("https://myhost/") .remoteAddr("0.0.0.0").remotePort(0) .sslCertificate("0123456789abcdef") + ), + // ================================================================= + // Complicated scenarios + Arguments.of(new Request("No initial authority, X-Forwarded-Proto on http, Proxy-Ssl-Id exists (setSslIsSecure==true)") + .configureCustomizer((customizer) -> customizer.setSslIsSecure(true)) + .headers( + "GET /foo HTTP/1.1", + "Host: myhost", + "X-Forwarded-Proto: http", + "Proxy-Ssl-Id: Wibble" + ), + new Expectations() + .scheme("http").serverName("myhost").serverPort(80) + .secure(true) + .requestURL("http://myhost/foo") + .remoteAddr("0.0.0.0").remotePort(0) + .sslSession("Wibble") + ), + Arguments.of(new Request("https initial authority, X-Forwarded-Proto on http, Proxy-Ssl-Id exists (setSslIsSecure==false)") + .configureCustomizer((customizer) -> customizer.setSslIsSecure(false)) + .headers( + "GET https://alt.example.net/foo HTTP/1.1", + "Host: myhost", + "X-Forwarded-Proto: http", + "Proxy-Ssl-Id: Wibble" + ), + new Expectations() + .scheme("http").serverName("alt.example.net").serverPort(80) + .secure(false) + .requestURL("http://alt.example.net/foo") + .remoteAddr("0.0.0.0").remotePort(0) + .sslSession("Wibble") + ), + Arguments.of(new Request("No initial authority, X-Proxied-Https off, Proxy-Ssl-Id exists (setSslIsSecure==true)") + .configureCustomizer((customizer) -> customizer.setSslIsSecure(true)) + .headers( + "GET /foo HTTP/1.1", + "Host: myhost", + "X-Proxied-Https: off", // this wins for scheme and secure + "Proxy-Ssl-Id: Wibble" + ), + new Expectations() + .scheme("http").serverName("myhost").serverPort(80) + .secure(false) + .requestURL("http://myhost/foo") + .remoteAddr("0.0.0.0").remotePort(0) + .sslSession("Wibble") + ), + Arguments.of(new Request("Https initial authority, X-Proxied-Https off, Proxy-Ssl-Id exists (setSslIsSecure==true)") + .configureCustomizer((customizer) -> customizer.setSslIsSecure(true)) + .headers( + "GET https://alt.example.net/foo HTTP/1.1", + "Host: myhost", + "X-Proxied-Https: off", // this wins for scheme and secure + "Proxy-Ssl-Id: Wibble" + ), + new Expectations() + .scheme("http").serverName("alt.example.net").serverPort(80) + .secure(false) + .requestURL("http://alt.example.net/foo") + .remoteAddr("0.0.0.0").remotePort(0) + .sslSession("Wibble") + ), + Arguments.of(new Request("Https initial authority, X-Proxied-Https off, Proxy-Ssl-Id exists (setSslIsSecure==true) (alt order)") + .configureCustomizer((customizer) -> customizer.setSslIsSecure(true)) + .headers( + "GET https://alt.example.net/foo HTTP/1.1", + "Host: myhost", + "Proxy-Ssl-Id: Wibble", + "X-Proxied-Https: off" // this wins for scheme and secure + ), + new Expectations() + .scheme("http").serverName("alt.example.net").serverPort(80) + .secure(false) + .requestURL("http://alt.example.net/foo") + .remoteAddr("0.0.0.0").remotePort(0) + .sslSession("Wibble") + ), + Arguments.of(new Request("Http initial authority, X-Proxied-Https off, Proxy-Ssl-Id exists (setSslIsSecure==false)") + .configureCustomizer((customizer) -> customizer.setSslIsSecure(false)) + .headers( + "GET https://alt.example.net/foo HTTP/1.1", + "Host: myhost", + "X-Proxied-Https: off", + "Proxy-Ssl-Id: Wibble", + "Proxy-auth-cert: 0123456789abcdef" + ), + new Expectations() + .scheme("http").serverName("alt.example.net").serverPort(80) + .secure(false) + .requestURL("http://alt.example.net/foo") + .remoteAddr("0.0.0.0").remotePort(0) + .sslSession("Wibble") + .sslCertificate("0123456789abcdef") + ), + Arguments.of(new Request("Http initial authority, X-Proxied-Https off, Proxy-Ssl-Id exists (setSslIsSecure==false) (alt)") + .configureCustomizer((customizer) -> customizer.setSslIsSecure(false)) + .headers( + "GET https://alt.example.net/foo HTTP/1.1", + "Host: myhost", + "Proxy-Ssl-Id: Wibble", + "Proxy-auth-cert: 0123456789abcdef", + "X-Proxied-Https: off" + ), + new Expectations() + .scheme("http").serverName("alt.example.net").serverPort(80) + .secure(false) + .requestURL("http://alt.example.net/foo") + .remoteAddr("0.0.0.0").remotePort(0) + .sslSession("Wibble") + .sslCertificate("0123456789abcdef") ) ); } @@ -868,20 +1075,31 @@ public void testNonStandardPortBehavior(Request request, Expectations expectatio expectations.accept(actual); } - @Test - public void testBadInput() throws Exception + public static Stream badRequestCases() { - Request request = new Request("Bad port value") - .headers( - "GET / HTTP/1.1", - "Host: myhost", - "X-Forwarded-Port: " - ); + return Stream.of( + new Request("Bad port value") + .headers( + "GET / HTTP/1.1", + "Host: myhost", + "X-Forwarded-Port: " + ), + new Request("Invalid X-Proxied-Https value") + .headers( + "GET / HTTP/1.1", + "Host: myhost", + "X-Proxied-Https: foo" + ) + ); + } + @ParameterizedTest + @MethodSource("badRequestCases") + public void testBadInput(Request request) throws Exception + { request.configure(customizer); String rawRequest = request.getRawRequest((header) -> header); - // System.out.println(rawRequest); HttpTester.Response response = HttpTester.parseResponse(connector.getResponse(rawRequest)); assertThat("status", response.getStatus(), is(400)); @@ -946,12 +1164,13 @@ private static class Expectations implements Consumer int expectedRemotePort = 0; String expectedSslSession; String expectedSslCertificate; + Boolean secure; @Override public void accept(Actual actual) { assertThat("scheme", actual.scheme.get(), is(expectedScheme)); - if (expectedScheme.equals("https")) + if (secure != null && secure) { assertTrue(actual.wasSecure.get(), "wasSecure"); } @@ -973,6 +1192,12 @@ public void accept(Actual actual) } } + public Expectations secure(boolean flag) + { + this.secure = flag; + return this; + } + public Expectations scheme(String scheme) { this.expectedScheme = scheme; From 457025bc16c5cd6b4a0ecf547fc281df9839af81 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 13 Oct 2020 08:24:49 -0500 Subject: [PATCH 5/7] Issue #5443 - Forwarding Headers are optional Additional NPE safety checks. Signed-off-by: Joakim Erdfelt --- .../org/eclipse/jetty/server/ForwardedRequestCustomizer.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 11932f26cc40..dac5c33f0c4d 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 @@ -522,8 +522,9 @@ else if (forwarded._secureScheme) // Don't change port if port == IMPLIED. // Update authority if different from metadata - if (!host.equalsIgnoreCase(requestURI.getHost()) || - port != requestURI.getPort()) + if (requestURI != null && host != null && + (!host.equalsIgnoreCase(requestURI.getHost()) || + port != requestURI.getPort())) { httpFields.put(new HostPortHttpField(host, port)); request.setAuthority(host, port); From 0721178007a884d2e4ef4b6ecf238266218e900b Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 13 Oct 2020 08:31:32 -0500 Subject: [PATCH 6/7] Issue #5443 - Forwarding Headers are optional The `X-Proxied-Https: off` case should have an implied port not a hardcoded port. Signed-off-by: Joakim Erdfelt --- .../org/eclipse/jetty/server/ForwardedRequestCustomizer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 dac5c33f0c4d..9f34c4066645 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 @@ -880,7 +880,7 @@ else if ("off".equalsIgnoreCase(field.getValue()) || "false".equalsIgnoreCase(fi { _secure = false; updateProto(HttpScheme.HTTP.asString(), Source.XPROXIED_HTTPS); - updatePort(80, Source.XPROXIED_HTTPS); + updatePort(MutableHostPort.IMPLIED, Source.XPROXIED_HTTPS); } else { From 89dc16ae0999955699c16b26fbef80d64504c172 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 13 Oct 2020 10:27:01 -0500 Subject: [PATCH 7/7] Issue #5443 - Forwarding Headers are optional Cleanup handling of forwarded.authority Signed-off-by: Joakim Erdfelt --- .../server/ForwardedRequestCustomizer.java | 51 ++++++++++--------- 1 file changed, 26 insertions(+), 25 deletions(-) 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 9f34c4066645..f43ddfecfaba 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 @@ -498,36 +498,37 @@ else if (forwarded._secureScheme) request.setScheme(config.getSecureScheme()); } - // 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; - } + String host = forwarded._authority._host; + int 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(); - } - // Don't change port if port == IMPLIED. + HttpURI requestURI = request.getMetaData().getURI(); - // Update authority if different from metadata - if (requestURI != null && host != null && - (!host.equalsIgnoreCase(requestURI.getHost()) || - port != requestURI.getPort())) - { - httpFields.put(new HostPortHttpField(host, port)); - request.setAuthority(host, port); + if (requestURI != null) + { + // Fall back to request metadata if needed. + if (host == null) + { + host = requestURI.getHost(); + } + + if (port == MutableHostPort.UNSET) // is unset by headers + { + port = requestURI.getPort(); + } + + // Don't change port if port == IMPLIED. + + // Update authority if different from metadata + if (!host.equalsIgnoreCase(requestURI.getHost()) || + port != requestURI.getPort()) + { + httpFields.put(new HostPortHttpField(host, port)); + request.setAuthority(host, port); + } + } } // Set Remote Address