Option to log X-Forwarded-For client IP

[hydrargyrum]

Is your feature request related to a problem? Please describe.
Gotify logs the client IP address by using only the remote IP address.
But if the requests comes from a reverse proxy, the only IP logged is the reverse proxy IP address.

Describe the solution you'd like
Reverse proxies typically add headers like X-Forwarded-For containing the real client IP address.
It should be optional though because if there's no reverse proxy, a client may forge a fake X-Forwarded-For header. Gotify can't determine by itself alone if there's a reverse proxy, so the configuration should be explicit.

Describe alternatives you've considered
Viewing the reverse proxy logs instead of gotify logs, but it's not as convenient.

Additional context
None

[jmattheis]

I'm open to accept PRs for this. This improvement would require another setting like trustHttpForwardedHeaders, otherwise it can be abused, when a user doesn't have a reverse proxy.

[tessus]

@jmattheis I was looking at the code, but I doubt that this is a good first issue. The only place where ClientIP is mentioned is in the logFormatter function. However, I cannot find where the param struct members are actually assigned.

I believe you have used so many external modules that someone would have to read up on 5 different modules to get the gist of it. For you this is all very logical and easy to understand, because you wrote the code in the first place. You can probably make the change in a few minutes.

Usually it goes like this:

1. Retrieve IP, headers from the request (communication with client)
2. use those values (if variable used as you suggested, then conditionally)

Can you please give some guidance on how (and more importantly where in the code) these values are retrieved and the param struct is built? e.g. where is the value param.ClientIP coming from?

[jmattheis]

ClientIP is defined in the gin project that is used for HTTP routing in gotify. https://github.com/gin-gonic/gin/blob/v1.8.1/logger.go#L66 the struct is also instantiated there.

I'm also not so sure about implementing this anymore. Only changing the logging ip should be fairly simple, but the X-Forwarded headers could be used in other places, f.ex. when checking the websocket origin.

Only adding this setting to fix the log, seems not worth the effort.

[tessus]

Thanks for the reply.

The problem is that the current logs are not very useful when you use a reverse proxy. You have to admit that seeing from where the traffic/request is coming is an important part of debugging. Seeing the IP of the reverse proxy does not make much sense.

This is the first project I have encountered that does not actually log the client's IP, but the reverse proxy's.

And when it comes to X-Forwarded headers. This is the responsibility of the system admin who sets up the reverse proxy. I am not sure why you see this as a security issue (your first reply). Especially in websocket connections the real client's IP is more important than the reverse proxy's, or am I wrong?

[jmattheis]

It looks like this actually works out of the box.

which isn't good, as it's possible to spoof the client ip like this. It seems the default behavior of gin-gonic is to trust all x-forwarded-for / x-real-ip headers.

[tessus]

Maybe it just trusts 127.0.0.1 by default. Have you tried from another machine?

as it's possible to spoof the client ip like this.

Can you please explain the attack vector? The adversary still needs the token to actually do something, or am I wrong?

[jmattheis]

The attack vector is that it's possible to spoof the client ip. Gotify only uses the client ip for inside the logs. So not that great of a problem. See here gin-gonic/gin#2473

Maybe it just trusts 127.0.0.1 by default. Have you tried from another machine?

yeah, this would at least be a better default than trust everything. Gin-gonic exposes a setTrusterProxies setter which acceps a list of CIDRs, maybe we create a setting like this inside the gotify config.