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

Jump to bottom

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

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

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

WesleyJeanette opened this issue Mar 27, 2019 · 4 comments
Labels
FeatureRequest help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@WesleyJeanette
Copy link

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).

go/src/net/http/httputil/reverseproxy.go

Lines 249 to 257 in 3089d18

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 bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. FeatureRequest labels Mar 28, 2019
@bradfitz
Copy link
Contributor

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
Copy link

solodynamo commented Mar 28, 2019 •
edited

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
Copy link

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
Copy link
Contributor

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.

@jbarlowbignell jbarlowbignell mentioned this issue Apr 9, 2021
Open
@neild neild mentioned this issue Jun 8, 2022
Closed
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
FeatureRequest help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@bradfitz @WesleyJeanette @luka-zitnik @AndrewBurian @seankhliao @solodynamo