Skip to content

Commit

Permalink
Issue #5247 - Improve testing of ForwardedRequestCustomizer
Browse files Browse the repository at this point in the history 
+ 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 <joakim.erdfelt@gmail.com>
  • Loading branch information
@joakime
joakime committed Sep 11, 2020
1 parent 4280303 commit ac42cbd
Show file tree
Hide file tree
Showing 3 changed files with 229 additions and 264 deletions.
186 changes: 123 additions & 63 deletions jetty-server/src/main/java/org/eclipse/jetty/server/ForwardedRequestCustomizer.java
View file
Expand Up @@ -83,17 +83,17 @@
* </tr>
* <tr>
* <td>2</td>
* <td><code>X-Forwarded-Host</code> Header</td>
* <td><code>X-Forwarded-Port</code> Header</td>
* <td>n/a</td>
* <td>Required</td>
* <td>Optional</td>
* <td>left-most value</td>
* <td>left-most value (only if {@link #getForwardedPortAsAuthority()} is true)</td>
* </tr>
* <tr>
* <td>3</td>
* <td><code>X-Forwarded-Port</code> Header</td>
* <td>n/a</td>
* <td><code>X-Forwarded-Host</code> Header</td>
* <td>Required</td>
* <td>left-most value (only if {@link #getForwardedPortAsAuthority()} is true)</td>
* <td>Implied</td>
* <td>left-most value</td>
* </tr>
* <tr>
* <td>4</td>
Expand Down Expand Up @@ -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))

This comment has been minimized.

Sign in to view

Copy link
@bentmann

bentmann Oct 10, 2020

Trying to update from jetty 9.4.31 to 9.4.32, I observe that HttpServletRequest.getRequestURL() returns the wrong scheme for HTTPS requests in the new version, this code change seemingly involved in the issue:

In my particular setup, jetty is brought up with an SSL connector on a custom port, say 12345. We further have both ForwardedRequestCustomizer and SecureRequestCustomizer configured, in that order. Now when a vanilla (i.e. no forwarded headers) GET https://localhost:12345 occurs, line 498 above sets port from the request URI, then here finds it matching the secure port and goes on to call request.setScheme(proto). proto however is still "http". Now when SecureRequestCustomer runs next, it won't fix the scheme per https://github.com/eclipse/jetty.project/blob/jetty-9.4.32.v20200930/jetty-server/src/main/java/org/eclipse/jetty/server/SecureRequestCustomizer.java#L212.

Does that make sense?

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Oct 11, 2020

Author Contributor

Please file an issue, and provide the entire set of Request headers so that we can evaluate the behavior of the ForwardedRequestCustomizer.

Also, having BOTH ForwardedRequestCustomizer and SecureRequestCustomizer is very unusual, as they overlap in functionality for "HttpServletRequest.isSecure".
Also, that SecureRequestCustomizer is incapable of filing out the forwarded security details in the HttpServletRequest attributes as they don't exist at the point in time that the SecureRequestCustomizer executes (as it's behind a proxy that already answered the SSL/TLS layer).

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Oct 11, 2020

Author Contributor

Also, be aware of issue #5417 - as it's likely you have a badly configured HttpConfiguration.securePort on your server side.

That port should be your logical secure port, the one seen from the public side of your Proxy, not the actual one used by your connectors.

This comment has been minimized.

Sign in to view

Copy link
@gregw

gregw Oct 12, 2020

Contributor

@joakime I don't think it is unusual to have both ForwardedRequestCustomizer and SecureRequestCustomizer as they serve different purposes... it just so happens that both may end up setting isSecure, but for different reasons.

This comment has been minimized.

Sign in to view

Copy link
@gregw

gregw Oct 12, 2020

Contributor

@joakime is the issue here that we never consider the scheme from a URI in the request line?

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Oct 12, 2020

Author Contributor

So, if there are no Forwarding headers, we still see a change occurring in the ForwardingRequestCustomizer?
That's not good. Will file an issue.

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Oct 12, 2020

Author Contributor

Issue #5437 opened.

This comment has been minimized.

Sign in to view

Copy link
@bentmann

bentmann Oct 12, 2020

provide the entire set of Request headers

The issue you filled is accurate and reflects what I meant to express with "vanilla (i.e. no forwarded headers)". FTR, the request headers were

Host: localhost:12345
Connection: keep-alive
User-Agent: Test/1.0

it's likely you have a badly configured HttpConfiguration.securePort on your server side.

I observed the issue with automated tests where Jetty's SSL connector is directly hit by the client, not going through a proxy. In such a setup, the port of the SSL connector is the secure port of the test server.

if there are no Forwarding headers, we still see a change occurring in the ForwardingRequestCustomizer?

Correct, because the customizer reacts to requestURI.getPort() == getSecurePort(config)

This comment has been minimized.

Sign in to view

Copy link
@joakime

joakime Oct 13, 2020

Author Contributor

The changes from PR #5419 addressed this, and are now merged into jetty-9.4.x
{
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);
Expand Down Expand Up @@ -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)
Expand All @@ -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;
Expand All @@ -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;
}
}

Expand All @@ -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
{
Expand All @@ -653,6 +712,7 @@ private class Forwarded extends QuotedCSVParser
MutableHostPort _for;
String _proto;
int _protoPriority = -1;
Boolean _secure;

public Forwarded(Request request, HttpConfiguration config)
{
Expand All @@ -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()
Expand Down Expand Up @@ -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;
}
}

Expand All @@ -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;
}
}

Expand All @@ -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")
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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);
Expand All @@ -840,6 +895,11 @@ private void updateProto(String proto, int priority)
{
_proto = proto;
_protoPriority = priority;

if (_proto.equalsIgnoreCase(_config.getSecureScheme()))
{
_secure = true;
}
}
}
}
Expand Down

0 comments on commit ac42cbd

Please sign in to comment.