gofiber / contrib

🧬 Repository for third party middlewares with dependencies
https://docs.gofiber.io/contrib/
MIT License
215 stars 115 forks source link

How to get client's real ip? #507

Open bangbaew opened 1 year ago

bangbaew commented 1 year ago

image

When make a remote request to my Gofiber endpoint, it gives http.client_ip = 10.8.11.189, which is container's local ip, but in Rust version of opentelemetry, used with Actix Web, it gives my real public ip out there, how can I make Gofiber's otel show public client ip?

gaby commented 1 year ago

@bangbaew Is the rust version also running inside Docker?

gaby commented 1 year ago

Found the issue. We are using the ClientIP from the context here: https://github.com/gofiber/contrib/blob/main/otelfiber/semconv.go#L59

We need to add support for X-Forwarded-For.

Related issue: https://github.com/open-telemetry/opentelemetry-go/issues/2282

I do think this should probably be fixed in Fiber instead of the middleware. Someone reported a similar issue when using c.IP() a few days ago on discord.

bangbaew commented 1 year ago

@bangbaew Is the rust version also running inside Docker?

It's running inside a container, same network as the Gofiber app. This is the Rust library i use: https://github.com/OutThereLabs/actix-web-opentelemetry

gaby commented 1 year ago

@bangbaew Is the rust version also running inside Docker?

It's running inside a container, same network as the Gofiber app. This is the Rust library i use: https://github.com/OutThereLabs/actix-web-opentelemetry

Yeah, this is a Fiber bug.

gaby commented 1 year ago

We can probably solve this by using this: https://docs.gofiber.io/api/ctx#ips

bangbaew commented 1 year ago

@bangbaew Is the rust version also running inside Docker?

It's running inside a container, same network as the Gofiber app. This is the Rust library i use: https://github.com/OutThereLabs/actix-web-opentelemetry

Yeah, this is a Fiber bug.

Yeah, the log IPs on the terminal as well, they all are local IPs, and I don't think they're any useful. image

gaby commented 1 year ago

@bangbaew Is the rust version also running inside Docker?

It's running inside a container, same network as the Gofiber app. This is the Rust library i use: https://github.com/OutThereLabs/actix-web-opentelemetry

Yeah, this is a Fiber bug.

Yeah, the log IPs on the terminal as well, they all are local IPs, and I don't think they're any useful. image

Those are expected since thats your IP inside the container. They only way to get the real IP in the logs is by parsing the Forwarded headers, it should be the first one in the List.

In one of your routes log ctx.IPs()

ReneWerner87 commented 1 year ago

https://github.com/gofiber/contrib/blob/bae3c8cc2db1c8cfcf747203af34f0ffa6a96bd9/otelfiber/semconv.go#L59-L62

https://github.com/gofiber/fiber/blob/634f163e3f6292e658e61d0dd9e3c475d87b5d54/ctx.go#L699-L701

https://docs.gofiber.io/next/api/fiber#config image

did you configure this header ? otherwise the fiber app can not determine the real ip

@gaby maybe we should extend the doc for these cases (ip method)

ReneWerner87 commented 1 year ago

https://github.com/gofiber/fiber/blob/master/ctx_test.go#L1265

gaby commented 1 year ago

https://github.com/gofiber/contrib/blob/bae3c8cc2db1c8cfcf747203af34f0ffa6a96bd9/otelfiber/semconv.go#L59-L62

https://github.com/gofiber/fiber/blob/634f163e3f6292e658e61d0dd9e3c475d87b5d54/ctx.go#L699-L701

https://docs.gofiber.io/next/api/fiber#config image

did you configure this header ? otherwise the fiber app can not determine the real ip

@gaby maybe we should extend the doc for these cases (ip method)

Agree, it's a bit confusing. From a otelfiber perspective using c.IPs() may be better since opentelemetry will auto-parse the list and only use the first IP which is the real client IP.

ReneWerner87 commented 1 year ago

@bangbaew have you ever tested what you get when you configure the header of the proxy (mostly forwarded-for ) in your fiber app ?

bangbaew commented 1 year ago

@bangbaew have you ever tested what you get when you configure the header of the proxy (mostly forwarded-for ) in your fiber app ?

If you mean have I tried logging from C.IPs() and c.GetReqHeaders(), I've tried them and the real IPs are shown in the fmt.Println, they both echo the X-Forwarded-For If I send a request over Kong Gateway endpoint, it will log this

"X-Forwarded-For": "{my real public ip}, 10.8.26.4",
"X-Real-Ip": "10.8.26.4"

The 10.8.26.4 is Kong instance's IP.

If I send a request directly, it will log this

"X-Forwarded-For": "{my real public ip}",
"X-Real-Ip": "{my real public ip}"

but both of them will log this in Jaeger UI

http.client_ip | 10.8.51.49 -- | -- You can see that the http.client_ip in Jaeger UI is the fiber instance's local ip, not even the forwarded IPs. But I don't know how to `configure the header of the proxy` in my fiber app.
ReneWerner87 commented 1 year ago

But I don't know how to configure the header of the proxy in my fiber app.

@bangbaew like this

app := fiber.New(fiber.Config{
    ProxyHeader: fiber.HeaderXForwardedFor,
})

https://docs.gofiber.io/next/api/fiber#config image

bangbaew commented 1 year ago

But I don't know how to configure the header of the proxy in my fiber app.

@bangbaew like this

app := fiber.New(fiber.Config{
  ProxyHeader: fiber.HeaderXForwardedFor,
})

https://docs.gofiber.io/next/api/fiber#config image

Thanks a lot! it shows the X-Forwarded-For IPs now, with both public IP and API Gateway's IP, can I make it record only the first value?

ReneWerner87 commented 1 year ago

do not think so, I would have to research

in any case, we should expand the documentation

@bangbaew you can do that, you know best where you searched for the solution of the problem

maybe in the examples and as a hint in the readme https://github.com/gofiber/contrib/tree/main/otelfiber#readme

ReneWerner87 commented 1 year ago

https://github.com/gofiber/fiber/commit/0dee42a57cd76d7922a753d437894fa214819a63

https://docs.gofiber.io/next/api/ctx#ip

gaby commented 1 year ago

@bangbaew opentelemetry says they only take the first value. Has that been the case for you after adding the header?

ReneWerner87 commented 1 year ago

maybe we can change the middleware and cut away the second value which comes back through the header