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

net/http/httputil: Reverse Proxy X-Forwarded-For include Port #31095

Open
WesleyJeanette opened this issue Mar 27, 2019 · 4 comments

Comments

@WesleyJeanette
Copy link

commented Mar 27, 2019

In the reverse proxy the port is not being added to the header X-Forwarded-For. The port is not a requirement but optional. From RFC 7329

If the server receiving proxied requests requires some
   address-based functionality, this parameter MAY instead contain an IP
   address (and, potentially, a port number).

As the port currently isn't being included by default, and it is something I need, I am explicitly setting the header. The result is I end up with X-Forwarded-For: 123.34.567.89:9876, 123.34.567.89. It would be great to either add an option to include the ports or do a check that the value about to be added doesn't already exist in the string (duplicate without the port).

if clientIP, _, err := net.SplitHostPort(req.RemoteAddr); err == nil {
// If we aren't the first proxy retain prior
// X-Forwarded-For information as a comma+space
// separated list and fold multiple headers into one.
if prior, ok := outreq.Header["X-Forwarded-For"]; ok {
clientIP = strings.Join(prior, ", ") + ", " + clientIP
}
outreq.Header.Set("X-Forwarded-For", clientIP)
}

@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

Sure, we could add a bool on ReverseProxy for including the port. Or maybe make it an new enum type where zero value means current behavior but let people turn it off, on explicitly (despite it being the default), or on with port.

But the behavior you mention of dup-suppressing by matching on the IP with any port is also fine to do by default without knobs. Or maybe it wouldn't matter with the previous knob, actually.

@solodynamo

This comment has been minimized.

Copy link

commented Mar 28, 2019

Screen Shot 2019-03-28 at 10 19 51 AM

Acc to https://golang.org/pkg/net/http/#Request, server sets `RemoteAddr` to an "IP:port" so It should have been concatenated with the prior `X-Forwarded-For` in above snippet.
@AndrewBurian

This comment has been minimized.

Copy link

commented Mar 29, 2019

The functionality to include a port number is specified in RFC 7329, which defines the Forwarded header, not the non-standard X-Forwarded-For header.

I still stand by that a better option is to support the Forwarded header and add the port to it rather than debate the merits and update implementation for a header that has no rules.

@luka-zitnik

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

How does the support for Forwarded header relate to backward compatibility?

I think backward compatibility is missing in the linked patch. The X-Forwarded-For is still a de-facto standard.

After the backward compatibility is in place, even this issue could be easily solved, e.g. a boolean to forward client port could apply to either of the header fields.

ps I'm looking for a way to make first contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.