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.

Sign up for GitHub

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

Add forwarded-host-header and forwarded-prefix-header configuration #9809

Closed
wants to merge 1 commit into from
Closed

Add forwarded-host-header and forwarded-prefix-header configuration #9809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions docs/src/main/asciidoc/vertx.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,6 +703,23 @@ java.lang.IllegalStateException: Failed to create cache dir

Assuming `/tmp/` is writeable this can be fixed by setting the `vertx.cacheDirBase` property to point to a directory in `/tmp/` for instance in OpenShift by creating an environment variable `JAVA_OPTS` with the value `-Dvertx.cacheDirBase=/tmp/vertx`.

== Proxy handling

In case quarkus is called through a proxy and it shall behave like it is running on that proxy host, please set the properties

----
quarkus.http.proxy-address-forwarding=true <1>
quarkus.http.forwarded-host-header=X-Forwarded-Host <2>
quarkus.http.forwarded-prefix-header=X-Forwarded-Prefix <3>
----

<1> This settings allows the replacement of absolute URI using X-Forwarded-* headers.<2>
<2> Defines the request header quarkus shall replace its internal host with (Disabled by default).
<3> Defines the request header quarkus shall prefix the request's URI (Disabled by default).

An example use case is the authentication handling with the quarkus OIDC extension behind a proxy.
Without these settings, quarkus sends the wrong redirect to the authentication server.

== Going further

There are many other facets of Quarkus using Vert.x underneath:
Expand Down
41 changes: 41 additions & 0 deletions extensions/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/EnvoyHeaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package io.quarkus.vertx.http;

import org.hamcrest.Matchers;
import org.jboss.shrinkwrap.api.ShrinkWrap;
import org.jboss.shrinkwrap.api.asset.StringAsset;
import org.jboss.shrinkwrap.api.spec.JavaArchive;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;
import io.restassured.RestAssured;

public class EnvoyHeaderTest {

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class)
.addClasses(ForwardedHandlerInitializer.class)
.addAsResource(new StringAsset("quarkus.http.proxy-address-forwarding=true\n"
+ "quarkus.http.forwarded-prefix-header=X-Envoy-Original-Path\n"),
"application.properties"));

@Test
public void test() {
RestAssured.given()
.header("X-Envoy-Original-Path", "prefix")
.get("/uri")
.then()
.body(Matchers.equalTo("/prefix/uri"));
}

@Test
public void testWithEmptyPrefix() {
RestAssured.given()
.header("X-Envoy-Original-Path", "")
.get("/uri")
.then()
.body(Matchers.equalTo("/uri"));
}

}
2 changes: 1 addition & 1 deletion ...ons/vertx-http/deployment/src/test/java/io/quarkus/vertx/http/ForwardedForHeaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public void test() {
.header("X-Forwarded-Host", "somehost")
.get("/forward")
.then()
.body(Matchers.equalTo("https|somehost|backend:4444"));
.body(Matchers.equalTo("https|localhost|backend:4444"));
}

}
41 changes: 41 additions & 0 deletions ...rtx-http/deployment/src/test/java/io/quarkus/vertx/http/ForwardedForPrefixHeaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package io.quarkus.vertx.http;

import org.hamcrest.Matchers;
import org.jboss.shrinkwrap.api.ShrinkWrap;
import org.jboss.shrinkwrap.api.asset.StringAsset;
import org.jboss.shrinkwrap.api.spec.JavaArchive;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;
import io.restassured.RestAssured;

public class ForwardedForPrefixHeaderTest {

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class)
.addClasses(ForwardedHandlerInitializer.class)
.addAsResource(new StringAsset("quarkus.http.proxy-address-forwarding=true\n"
+ "quarkus.http.forwarded-prefix-header=X-Forwarded-Prefix\n"),
"application.properties"));

@Test
public void testWithPrefix() {
RestAssured.given()
.header("X-Forwarded-Prefix", "prefix")
.get("/uri")
.then()
.body(Matchers.equalTo("/prefix/uri"));
}

@Test
public void testWithEmptyPrefix() {
RestAssured.given()
.header("X-Forwarded-Prefix", "")
.get("/uri")
.then()
.body(Matchers.equalTo("/uri"));
}

}
38 changes: 38 additions & 0 deletions ...rtx-http/deployment/src/test/java/io/quarkus/vertx/http/ForwardedForServerHeaderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package io.quarkus.vertx.http;

import static org.assertj.core.api.Assertions.assertThat;

import org.hamcrest.Matchers;
import org.jboss.shrinkwrap.api.ShrinkWrap;
import org.jboss.shrinkwrap.api.asset.StringAsset;
import org.jboss.shrinkwrap.api.spec.JavaArchive;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;
import io.restassured.RestAssured;

public class ForwardedForServerHeaderTest {

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class)
.addClasses(ForwardedHandlerInitializer.class)
.addAsResource(new StringAsset("quarkus.http.proxy-address-forwarding=true\n"
+ "quarkus.http.forwarded-host-header=X-Forwarded-Server\n"),
"application.properties"));

@Test
public void test() {
assertThat(RestAssured.get("/forward").asString()).startsWith("http|");

RestAssured.given()
.header("X-Forwarded-Proto", "https")
.header("X-Forwarded-For", "backend:4444")
.header("X-Forwarded-Server", "somehost")
.get("/forward")
.then()
.body(Matchers.equalTo("https|somehost|backend:4444"));
}

}
4 changes: 4 additions & 0 deletions ...ertx-http/deployment/src/test/java/io/quarkus/vertx/http/ForwardedHandlerInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ public void register(@Observes Router router) {
router.route("/forward").handler(rc -> rc.response()
.end(rc.request().scheme() + "|" + rc.request().getHeader(HttpHeaders.HOST) + "|"
+ rc.request().remoteAddress().toString()));

router.route("/uri").handler(rc -> rc.response()
.end(rc.request().uri()));

}

}
46 changes: 46 additions & 0 deletions ...oyment/src/test/java/io/quarkus/vertx/http/ProxyAddressForwardingDefaultSettingsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package io.quarkus.vertx.http;

import static org.assertj.core.api.Assertions.assertThat;

import org.hamcrest.Matchers;
import org.jboss.shrinkwrap.api.ShrinkWrap;
import org.jboss.shrinkwrap.api.asset.StringAsset;
import org.jboss.shrinkwrap.api.spec.JavaArchive;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;
import io.restassured.RestAssured;

public class ProxyAddressForwardingDefaultSettingsTest {

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class)
.addClasses(ForwardedHandlerInitializer.class)
.addAsResource(new StringAsset("quarkus.http.proxy-address-forwarding=true\n"),
"application.properties"));

@Test
public void testWithHostHeader() {
assertThat(RestAssured.get("/forward").asString()).startsWith("http|");

RestAssured.given()
.header("X-Forwarded-Proto", "https")
.header("X-Forwarded-For", "backend:4444")
.header("X-Forwarded-Host", "somehost")
.header("X-Forwarded-Prefix", "prefix")
.get("/forward")
.then()
.body(Matchers.equalTo("https|localhost|backend:4444"));
}

@Test
public void testWithPrefixHeader() {
RestAssured.given()
.header("X-Forwarded-Prefix", "prefix")
.get("/uri")
.then()
.body(Matchers.equalTo("/uri"));
}
}
63 changes: 57 additions & 6 deletions ...sions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/ForwardedParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package io.quarkus.vertx.http.runtime;

import java.util.Optional;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

Expand All @@ -38,7 +39,6 @@ class ForwardedParser {
private static final AsciiString FORWARDED = AsciiString.cached("Forwarded");
private static final AsciiString X_FORWARDED_SSL = AsciiString.cached("X-Forwarded-Ssl");
private static final AsciiString X_FORWARDED_PROTO = AsciiString.cached("X-Forwarded-Proto");
private static final AsciiString X_FORWARDED_HOST = AsciiString.cached("X-Forwarded-Host");
private static final AsciiString X_FORWARDED_PORT = AsciiString.cached("X-Forwarded-Port");
private static final AsciiString X_FORWARDED_FOR = AsciiString.cached("X-Forwarded-For");

Expand All @@ -48,17 +48,25 @@ class ForwardedParser {

private final HttpServerRequest delegate;
private final boolean allowForward;
private final Optional<String> forwardedHostHeader;
private final Optional<String> forwardedPrefixHeader;

private boolean calculated;
private String host;
private int port = -1;
private String scheme;
private String uri;
private String absoluteURI;
private SocketAddress remoteAddress;

ForwardedParser(HttpServerRequest delegate, boolean allowForward) {
ForwardedParser(HttpServerRequest delegate,
boolean allowForward,
Optional<String> forwardedHostHeader,
Optional<String> forwardedPrefixHeader) {
this.delegate = delegate;
this.allowForward = allowForward;
this.forwardedHostHeader = forwardedHostHeader;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be optional, both of these need to have a specific configuration option to turn them on.

To give you an idea of why this can cause problems say I have an environment that knows nothing about X-Forwarded-Prefix, but security is done by the front end proxy.

An attacker could send a request to / but with X-Forwarded-Prefix set to /secure. The front end proxy will just see /, and allow it, but then Quarkus will serve up content from /secure.

If you are going to enable these options they can only be turned on in an environment where you know these headers will only be set by a trusted source.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! :-)

For clarification, when you mentioned "both of these", do you mean x-forwarded-host/x-forwarded-server as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the "quarkus.http.proxy-address-forwarding" config property be enough to prevent those scenarios?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I need to do some more research on this. The question is do frontend proxies that send one or more of these always send all of them (or make sure to disallow forwarding them)?

If all the main implementations do this then as this is not really a standard we are probably fine to just use a single switch. If not then maybe even our current configuration is too coarse grained.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ejba Looks like a switch per header can be the most secure solution based on what you've discussed with @stuartwdouglas and @kraeftbraeu, please follow up with this update.

May be it can make sense to split the PR in 2 parts, the 1st part will only deal with X-Forwarded-Host and X-Forwarded-Server to accelerate it, X-Forwarded-Prefix related updated can be discussed further as needed in another (draft) PR...
Sounds good ?
Cheers, Sergey

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sberyozkin, just two questions:

  • Should I keep this quarkus.http.proxy-address-forwarding property?
  • Should I default every x-forward-* header as disabled, despite the backward compatibility?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the approach with two separate PRs, since it helps separate needs. To be honest I still dont get the security issues with the prefix header. Isnt it set by the reverse proxy? Or is it possible that the reverse proxy just forwards these headers?

So, for the host/server selection two separate switches shall be used with a startup exception when both are set to true?
Or can we stay with the current solution but we extend the documentation (and maybe the startup log) with a hint to this security issue when using this option?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The property quarkus.http.proxy-address-forwarding says if the absoluteUri shall be recalculated from forwarded-headers in principle. I'd say that should be kept.

When we start the additional properties

  • quarkus.http.allow-x-forwarded-server
  • quarkus.http.allow-x-forwarded-host
  • quarkus.http.allow-x-forwarded-prefix
    and the second one is no more enabled by default, backward compatibility is broken.

I'm new to the quarkus project but IMHO broken backward compatibility reduces trust to a software project and should definitely be avoided if possible.

Of course security issues are still important. But in the end the key is the awareness by the quarkus users. And I would prefer to write a good documentation rather than making the configuration a little more complicated to push the user to think about security issues.

@stuartwdouglas what do you think?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, #10138 is the new PR.

this.forwardedPrefixHeader = forwardedPrefixHeader;
}

public String scheme() {
Expand All @@ -80,6 +88,13 @@ boolean isSSL() {
return scheme.equals(HTTPS_SCHEME);
}

String uri() {
if (!calculated)
calculate();

return uri;
}

String absoluteURI() {
if (!calculated)
calculate();
Expand All @@ -98,6 +113,7 @@ private void calculate() {
calculated = true;
remoteAddress = delegate.remoteAddress();
scheme = delegate.scheme();
uri = delegate.uri();
setHostAndPort(delegate.host(), port);

String forwardedSsl = delegate.getHeader(X_FORWARDED_SSL);
Expand Down Expand Up @@ -134,9 +150,11 @@ private void calculate() {
port = -1;
}

String hostHeader = delegate.getHeader(X_FORWARDED_HOST);
if (hostHeader != null) {
setHostAndPort(hostHeader.split(",")[0], port);
if (forwardedHostHeader.isPresent()) {
String hostHeader = delegate.getHeader(forwardedHostHeader.get());
if (hostHeader != null) {
setHostAndPort(hostHeader.split(",")[0], port);
}
}

String portHeader = delegate.getHeader(X_FORWARDED_PORT);
Expand All @@ -148,6 +166,13 @@ private void calculate() {
if (forHeader != null) {
remoteAddress = parseFor(forHeader.split(",")[0], remoteAddress.port());
}

if (forwardedPrefixHeader.isPresent()) {
String prefixHeader = delegate.getHeader(AsciiString.cached(forwardedPrefixHeader.get()));
if (prefixHeader != null) {
uri = appendPrefixToUri(prefixHeader, uri);
}
}
}

if (((scheme.equals(HTTP_SCHEME) && port == 80) || (scheme.equals(HTTPS_SCHEME) && port == 443))) {
Expand All @@ -156,7 +181,8 @@ private void calculate() {

host = host + (port >= 0 ? ":" + port : "");
delegate.headers().set(HttpHeaders.HOST, host);
absoluteURI = scheme + "://" + host + delegate.uri();
absoluteURI = scheme + "://" + host + uri;
ejba marked this conversation as resolved.
Show resolved Hide resolved
log.debugf("Recalculated absoluteURI to %s", absoluteURI);
}

private void setHostAndPort(String hostToParse, int defaultPort) {
Expand Down Expand Up @@ -191,4 +217,29 @@ private int parsePort(String portToParse, int defaultPort) {
return defaultPort;
}
}

private String appendPrefixToUri(String prefix, String uri) {
String parsed = stripSlash(prefix);
return parsed.isEmpty() ? uri : '/' + parsed + uri;
}

private String stripSlash(String uri) {
String result;
if (!uri.isEmpty()) {
int beginIndex = 0;
if (uri.startsWith("/")) {
beginIndex = 1;
}

int endIndex = uri.length();
if (uri.endsWith("/")) {
endIndex = uri.length() - 1;
}
result = uri.substring(beginIndex, endIndex);
} else {
result = uri;
}

return result;
}
}
14 changes: 11 additions & 3 deletions ...tp/runtime/src/main/java/io/quarkus/vertx/http/runtime/ForwardedServerRequestWrapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package io.quarkus.vertx.http.runtime;

import java.util.Map;
import java.util.Optional;

import javax.net.ssl.SSLPeerUnverifiedException;
import javax.net.ssl.SSLSession;
Expand Down Expand Up @@ -35,9 +36,16 @@ class ForwardedServerRequestWrapper implements HttpServerRequest {
private String uri;
private String absoluteURI;

ForwardedServerRequestWrapper(HttpServerRequest request, boolean allowForwarded) {
ForwardedServerRequestWrapper(HttpServerRequest request,
boolean allowForwarded,
Optional<String> forwardedHostHeader,
Optional<String> forwardedPrefixHeader) {
delegate = request;
forwardedParser = new ForwardedParser(delegate, allowForwarded);

forwardedParser = new ForwardedParser(delegate,
allowForwarded,
forwardedHostHeader,
forwardedPrefixHeader);
}

void changeTo(HttpMethod method, String uri) {
Expand Down Expand Up @@ -137,7 +145,7 @@ public String rawMethod() {
@Override
public String uri() {
if (!modified) {
return delegate.uri();
return forwardedParser.uri();
}
return uri;
}
Expand Down
Loading