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

X-Forwarded-For header is not handled correctly #4320

Closed
haines opened this issue May 13, 2024 · 2 comments · Fixed by #4334
Closed

X-Forwarded-For header is not handled correctly #4320

haines opened this issue May 13, 2024 · 2 comments · Fixed by #4334

Comments

@haines
Copy link
Contributor

haines commented May 13, 2024

🐛 Bug Report

The remote IP is appended to the X-Forwarded-For header, but the original value is not overwritten, leading to duplication.

If multiple X-Forwarded-For headers are present, the remote IP is appended to the first one, when it should be appended to the last one.

To Reproduce

  1. Send a request to a gRPC-Gateway server with headers:
X-Forwarded-For: 1.2.3.4, 5.6.7.8
X-Forwarded-For: 9.10.11.12

Expected behavior

Either

  • the remote IP is appended to the value of the last X-Forwarded-For header pair using ", " as a delimiter, and that pair is replaced; or
  • an additional X-Forwarded-For pair is appended; or
  • the existing X-Forwarded-For pairs are merged by joining their values with the delimiter ", " and the remote IP is appended.

so that the resulting metadata contains either

"X-Forwarded-For": []string{"1.2.3.4, 5.6.7.8", "9.10.11.12, ${remoteIP}"},

or

"X-Forwarded-For": []string{"1.2.3.4, 5.6.7.8", "9.10.11.12", "${remoteIP}"},

or

"X-Forwarded-For": []string{"1.2.3.4, 5.6.7.8, 9.10.11.12, ${remoteIP}"},

Per https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For#parsing

There may be multiple X-Forwarded-For headers present in a request. The IP addresses in these headers must be treated as a single list, starting with the first IP address of the first header and continuing to the last IP address of the last header. There are two ways of making this single list:

  • join the X-Forwarded-For full header values with commas and then split by comma into a list, or
  • split each X-Forwarded-For header by comma into lists and then join the lists

It is insufficient to use only one of multiple X-Forwarded-For headers.

(Some reverse proxies will automatically join multiple X-Forwarded-For headers into one, but it is safest to not assume that this is the case.)

Actual Behavior

With v2.19.1, the resulting metadata has X-Forwarded-For: []string{"1.2.3.4, 5.6.7.8", "1.2.3.4, 5.6.7.8, ${remoteIP}"}

@haines haines changed the title X-Forwarded-For header is not overwritten with updated value X-Forwarded-For header is not handled correctly May 13, 2024
haines added a commit to haines/cerbos that referenced this issue May 14, 2024
haines added a commit to haines/cerbos that referenced this issue May 14, 2024
haines added a commit to haines/cerbos that referenced this issue May 14, 2024
haines added a commit to haines/cerbos that referenced this issue May 14, 2024
haines added a commit to cerbos/cerbos that referenced this issue May 14, 2024
This PR primarily aims to work around an issue with `X-Forwarded-For`
handling in the gRPC-Gateway
(grpc-ecosystem/grpc-gateway#4320).

Given a remote IP of 4.4.4.4 sending incoming headers of 

```
X-Forwarded-For: 1.1.1.1, 2.2.2.2
X-Forwarded-For: 3.3.3.3
```

the resulting headers forwarded by the gRPC-gateway are

```
X-Forwarded-For: 1.1.1.1, 2.2.2.2
X-Forwarded-For: 3.3.3.3
X-Forwarded-For: 1.1.1.1, 2.2.2.2, 4.4.4.4
```

The workaround I have added means that the resulting headers are

```
X-Forwarded-For: 1.1.1.1, 2.2.2.2
X-Forwarded-For: 3.3.3.3
X-Forwarded-For: 4.4.4.4
```

We now join the values with `", "` rather than `"|"` to preserve the
[de-facto standard
syntax](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For#syntax).

This PR also prevents spoofing the remote address in the audit logs by
setting the `x-cerbos-http-remote-addr` header; currently we take that
header into account even for requests not originating from the
gRPC-Gateway where it should be set. Even in the case of requests from
the gRPC-Gateway, our header is appended to the request headers, so we
need to make sure to use the last value for the header, not the first.

Signed-off-by: Andrew Haines <haines@cerbos.dev>
@johanbrandhorst
Copy link
Collaborator

Thanks for your bug report! This seems like a clear bug. Would you be willing to help fix this?

@haines
Copy link
Contributor Author

haines commented May 14, 2024

Yep, happy to, I'll submit a pull request tomorrow.

haines added a commit to cerbos/cerbos that referenced this issue May 17, 2024
…or` handling (#2157)

grpc-ecosystem/grpc-gateway#4320 is now fixed,
so we no longer need to rewrite the `X-Forwarded-For` header.

Signed-off-by: Andrew Haines <haines@cerbos.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants