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

extractClientIp directive ignores X-Forwarded-For header if it includes port #3790

Open
dimap-tr opened this issue Mar 21, 2021 · 8 comments

Comments

@dimap-tr
Copy link

It seems that when X-Forwarded-For has a port, the system fails to parse it as a X-Forwarded-For header, and instead parses it as a RawHeader. This causes the implementation of the extractClientIp directive to not take it into account.
When looking at the parsing itself, I see the following error : ErrorInfo(Invalid input ':', expected listSep or 'EOI' (line 1, column 14), (details omitted), )

To reproduce use a route with extractClientIp directive, send an http request with X-Forwarded-For: 10.0.0.1:99999, expected - extractClientIp provides 10.0.0.1, actual - extractClientIp provides the value of the remote address.

@dimap-tr
Copy link
Author

Looking at the parser code at akka.http.impl.model.parser.SimpleHeaders it clearly does not account for port:

  // de-facto standard as per http://en.wikipedia.org/wiki/X-Forwarded-For
  // It's not clear in which format IpV6 addresses are to be expected, the ones we've seen in the wild
  // were not quoted and that's also what the "Transition" section in the draft says:
  // http://tools.ietf.org/html/draft-ietf-appsawg-http-forwarded-10
  def `x-forwarded-for` = {
    def addr = rule { (`ip-v4-address` | `ip-v6-address`) ~> (RemoteAddress(_)) | "unknown" ~ push(RemoteAddress.Unknown) }
    rule { oneOrMore(addr).separatedBy(listSep) ~ EOI ~> (`X-Forwarded-For`(_)) }
  }
...
  def `ip-v4-address` = rule {
    `ip-number` ~ '.' ~ `ip-number` ~ '.' ~ `ip-number` ~ '.' ~ `ip-number` ~> (Array[Byte](_, _, _, _))
  }

@raboof
Copy link
Contributor

raboof commented Mar 22, 2021

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For doesn't mention allowing a port, but the standardized Forwarded header does allow one (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded).

It might make sense to support a port, but on the other hand, perhaps it's better to move to the standardized Forwarded header anyway?

@dimap-tr
Copy link
Author

dimap-tr commented Mar 22, 2021

We detected the issue when attempting to run our server in Azure environment - it appears that Azure GW by default sends the X-Forwarded-For with the port, rendering extractClientIp directive unusable. Also Azure does not send the Forwarded header at the moment.

@raboof
Copy link
Contributor

raboof commented Mar 22, 2021

That indeed matches what I read at https://docs.microsoft.com/en-us/azure/application-gateway/how-application-gateway-works - would be good to support this, would you be interested in preparing a PR?

@dimap-tr
Copy link
Author

@raboof Sure, will try to get it done in the next couple of days.

@jrudolph
Copy link
Contributor

There's a previous discussion in #2191

@ryanfaircloth
Copy link

I am also impacted by this.

@ryanfaircloth
Copy link

Also mentioning the IPv6 address with port is likely to have this problem as well
https://datatracker.ietf.org/doc/html/rfc5952#section-6

In my case over 24 hours this caused about 44 billion warning errors to be logged

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

No branches or pull requests

4 participants