Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support X-Forwarded-Prefix #9622

Closed
wicksim opened this issue May 27, 2020 · 16 comments · Fixed by #10266
Closed

Support X-Forwarded-Prefix #9622

wicksim opened this issue May 27, 2020 · 16 comments · Fixed by #10266
Labels
kind/enhancement New feature or request
Milestone

Comments

@wicksim
Copy link

wicksim commented May 27, 2020

Description
My application runs behind a reverse proxy and I need to get the original path from UriInfo to return it as a part of the Location-header in a REST-resource. There already is support for several X-Forwarded--headers, but not for X-Forwarded-Prefix.

It would be nice to have support for this header. Maybe it is even possible to make the header-name configurable? Because X-Forwarded-Prefix is not a standard and I'm using envoy, which only provides x-envoy-original-path.

I've seen that ForwardedParser is inspired by the Spring-solution. Spring seems to have support for X-Forwarded-Prefix.

@wicksim wicksim added the kind/enhancement New feature or request label May 27, 2020
@ejba
Copy link
Contributor

ejba commented May 30, 2020

As far as I understood it, it's about to append X-Forwarded-Prefix to HttpServerRequest's URI and assign it to the absoluteURI in ForwardedParser. If that so, I think it's straightforward and I would like to contribute on this one.

@wicksim
Copy link
Author

wicksim commented Jun 2, 2020

I didn't spend much time in analysing till now. But it would be nice if you would contribute on this one, because I will not have time to do this in the next few days.

@sberyozkin
Copy link
Member

@ejba Hi, can you please check what is needed for X-Forwarded-Server, a user reports a proxy host is reported with this header, may be it can be checked if X_FORWARDED_HOST is not set, CC @stuartwdouglas

@kraeftbraeu
Copy link

kraeftbraeu commented Jun 4, 2020

Hi, the user is me :)
In my case if have both headers set:

  • X-Forwarded-Host with the host of my quarkus appserver and
  • X-Forwarded-Server with my apache reverse proxy host.

My case would work with a new property that defines which Header defines the host. Or a logik like
hostHeader = realHost.equals(hostHeaderFromXForwardedHost ? hostHeaderFromXForwardedServer : hostHeaderFromXForwardedHost;

@ejba
Copy link
Contributor

ejba commented Jun 4, 2020

@sberyozkin Yes, I can do it! So it's okay for you if I do a PR to solve both use cases?

@kraeftbraeu
Copy link

That would be great. How can I help?

@ejba
Copy link
Contributor

ejba commented Jun 5, 2020

@kraeftbraeu well, I don't understand why CI is failing. Could you help me with that? /cc @sberyozkin

@kraeftbraeu
Copy link

kraeftbraeu commented Jun 5, 2020

Unfortunately that github PR checks magic is new to me, but I'll try.
Maybe it's something in the last commit of the last two days?

@sberyozkin
Copy link
Member

@ejba Thanks for the PR

@ejba
Copy link
Contributor

ejba commented Jun 7, 2020

@sberyozkin Should X-Forwarded-Prefix be confirmed that is a valid URI? If so, what's the expected behavior here? Log a warning and ignore the header if it isn't valid?

@atoom
Copy link

atoom commented Jun 12, 2020

Hi,

Since there is no standard defined for X-Forwarded-Prefix I have been trying to look around and see how different products/vendors/frameworks are using it. So far the two most concrete examples are the Traefik StripPrefix middleware and the Spring ForwardedHeaderFilter.

My understanding is that Traefik expects the backend to ignore the header on incoming requests but use it when constructing urls in the response, for example 301/302 redirects. Spring on the other hand seems to use the header value to dynamically append it to the context root/path on the the incoming request and use it when mapping the request to the handler code.

I cannot understand why the Spring team have done their implementation in such a way and I have tried to find the original motivation but there are not that many details except for issue SPR-14270.

What is the use-case when you have and external facing url that gets stripped in a reverse proxy and forwarded as a header which then gets appended back on again by the framework ?

Are you sure you want to copy the Spring implementation ? :)

@ejba
Copy link
Contributor

ejba commented Jun 12, 2020

Hi @atoom,

What is the use-case when you have and external facing url that gets stripped in a reverse proxy and forwarded as a header which then gets appended back on again by the framework ?

You should have a look into this issue (spring-projects/spring-framework#4732), it gives you more details about the problem.

@atoom
Copy link

atoom commented Jun 12, 2020

Hi @ejba

It is the dynamic SERVER_SERVLET_CONTEXT_PATH part I don't get. How is it possible to setup the routing rules inside the application if the context path can vary per request ? Isn't it better to have a static context path within the application and let the reverse proxy handle the per environment/request dynamic configuration ? The reverse proxy could of course handle response rewrites of location header urls, cookie paths etc but since most application framework already honor various X-Forwarded-* headers it makes sense to also use X-Forwarded-Prefix in response urls etc.

Request:

https://api.example.com/weather/v1/today-> http://backend/v1/today
X-Forwarded-Proto: https
X-Forwarded-Port: 443
X-Forwarded-Host: api.example.com
X-Forwarded-Prefix: /weather

Pseudo application logic:

@GET("/v1/today")
handler(req, resp) {
  resp.sendRedirect("/v1/latest");
}

Response:

HTTP/1.1 302 Found
Location: https://api.example.com/weather/v1/latest

Maybe I have a naive/simple view of the world but to me it makes more sense to leave the incoming url as it is and only use X-Forwarded-Prefix when creating the response.

@stuartwdouglas
Copy link
Member

So this is kinda tricky. I think you would want to apply this after the mapping layer.

e.g. If you have a request for /bar with a prefix of /foo you would want to map it to the endpoint at /bar, but then modify the request so it looks like the actual URL is /foo/bar (and set the context path accordingly).

We can do this with Servlet easily enough, but I don't think we can make this a generic HTTP thing, as the modification has to happen after the request has been routed.

@sberyozkin
Copy link
Member

Hi @atoom, so there are 2 problems which are being addressed in @ejba's PR, one is to do with X-Forwarded-Prefix, one is about choosing between X-Forwarded-Host and X-Forwarded-Server. The latter problem is sorted out with the configurable options, can X-Forwarded-Prefix dilemma be resolved in a similar way, instead of the only way. I propose to get the PR going and follow up with the enhancement request to make the way X-Forwarded-Prefix is treated configurable.
Does it work ?
We really need the X-Forwarded-Host and X-Forwarded-Server solution for 1.6.0

@atoom
Copy link

atoom commented Jun 18, 2020

Hi @sberyozkin,

I did not mean to slow up your progress on this - we have an internal discussion about this issue in our team so I was looking around various open source projects to try to find a common best practice on how this has been implemented. The way Spring have chosen to do their implementation did not fit us but when I saw that you had chosen a similar approach I was curious on your reasoning for the decision :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
7 participants