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

UriComponentsBuilder#fromHttpRequest should consider RFC-7239 Forwarded headers [SPR-11856] #16475

Closed
spring-projects-issues opened this issue Jun 7, 2014 · 11 comments
Assignees
Labels
in: web type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Jun 7, 2014

Oliver Drotbohm opened SPR-11856 and commented

See the spec for details. tl;dr - a standardized version of what previously has been X-Forwarded-….


Issue Links:

  • #20809 spring-web CORS requires X-Forwarded-Port

Referenced from: commits 4611d05

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jul 16, 2014

Rossen Stoyanchev commented

This is a good idea to revisit for 4.2 perhaps. The RFC is labeled "proposed standard" as of June 2014 so I doubt it's in use yet.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 19, 2015

Arjen Poutsma commented

It is still a proposed standard as of May 2015. Should I work on this?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 19, 2015

Rossen Stoyanchev commented

It looks like it. That seems to indicate its content is generally stable and already discussed but is considered immature still due to lack of implementations. It might be worth checking how it compares overall to our existing support in UriComponentsBuilder#fromHttpRequest. Considering the nature of the spec there probably isn't too much risk of change and it would be just a matter of adapting to the header names and syntax.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 20, 2015

Arjen Poutsma commented

One interesting thing to note: it does not seem like RFC-7239 has a replacement for the X-Forwarded-Port header. Or at least, I can't seem to find it.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 20, 2015

Oliver Drotbohm commented

That seems to be integrated into Forwarded By and Forwarded For.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 21, 2015

Arjen Poutsma commented

I am not convinced they are. Those parameters take the IP address of the client, so it would be logical if the port number is the client-side port; not the server-side port. Otherwise, you would have a client-side IP address with a server-side port, and that doesn't make a lot of sense to me.

Let's take a look at an example, adapted from the RFC: a request from a client with IP address 192.0.2.43 passes through a proxy with IP address 198.51.100.17, before reaching an origin server. The client-side port is 1234, the proxy server-side port is 80, the proxy client-side port is 5678, and the server server-side port is 8080. In other words, we the following connections:

192.0.2.43:1234->198.51.100.17:80
198.51.100.17:5678->server:8080

In my understanding, the RFC states the following:

  • The HTTP request between the client and the proxy has no "Forwarded" header field.
  • The HTTP request between the proxy and server has a "Forwarded: for=192.0.2.43:1234" header field. Or should the header be "Forwarded: for=192.0.2.43:80”? But that would be quite strange, since it would combine a client-side IP with a server-side port.

I am going to ask for feedback to the RFC authors on this, because this is confusing. At any rate, it looks this won't be in 4.2. RC1.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 21, 2015

Arjen Poutsma commented

According to the RFC authors, the second request should have a "Forwarded: for=192.0.2.43:1234" header field, which suggest that the port given in the "for" section is not a direct replacement for X-Forwarded-Port.

I've asked the subsequent question to the authors: "In RFC 7239, what is the replacement for the non-standard X-Forwarded-Port header, which in the example above should be 80? Section 7.4 of the RFC, covering the transition from these “old” headers, does not cover it. Or is there no replacement for this header?"

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 21, 2015

Arjen Poutsma commented

The authors responded:

There is no defined replacement for this header. Adding it would require a new RFC that has been IESG or WG sponsored documents. It was made this complicated as there were significant concerns about these headers from a security and privacy point of view.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 21, 2015

Arjen Poutsma commented

PR at #804, though I am sure this issue should have a bit more discussion before merging it.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented May 21, 2015

Rossen Stoyanchev commented

The PR looks good to me. As far as the forwarded port question, we can only support what the RFC defines. The only minor issue I see is that the host and proto values can be quoted it seems.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Jun 9, 2015

Arjen Poutsma commented

I've updated the PR to deal with quoted values. It can now be merged, IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants