Add remote ip field to Http::Request

[anatol]

It is useful to get access from HTTP::Handler to underlying socket properties. I am interested in the client ip address (aka 'remote_ip'). Currently Http::Request has fields that are parsed out of the request body but no client ip.

[jhass]

Should probably differentiate between the IP the TCP connection came from and things like the X-Forwarded-For.

[anatol]

http server implementation is surprisingly simple and it was easy to write own TCP socket loop

server = TCPServer.new("0.0.0.0", my_port)
begin
  sock = server.accept
  io = BufferedIO.new(sock)
  request = HTTP::Request.from_io(io)
  response = my_process_function(sock, request)
  response.to_io io
  io.flush
  sock.close
end while true

It works great.

[jhass]

I'm currently using the following method: https://github.com/jhass/carc.in/blob/master/src/web.cr#L96-L102 But X-Real-Ip seems to be another header that's used for this.

A proper solution should fallback to the sockets peer ip afterwards.

[anatol]

It was easy to implement my own version of the server as in the comment above. This issue might be closed now if devs think repome_ip is not needed.

[asterite]

We'll soon take a look at this with @waj, as it seems this is very needed :-)

[ozra]

To be picky, the remote_ip should be associated with the connection, not the request. Though, use-wise it would be both more convenient and logical on the request object.

[crisward]

request.remote_ip would be very useful. Any news on this recently?

[crisward]

If you're using crystals behind nginx, you should be able to get the remote ip with

remote_ip = context.request.headers["X-Forwarded-For"]?

I'm using dokku with the heroku buildpack and this seems to work fine.

[RomanHargrave]

Luckily since I run everything behind a load balancer, I am able to, by way of headers, obtain a remote IP. That being said, it would be nice to have fields, perhaps a client_ip and a remote_ip.

The distinction between the two being that remote_ip = client_ip unless X-Forwarded-For (or X-Real-IP - is this actually in use?) are present.

[rdp]

See also #1722

[straight-shoota]

Given that this functionality is requested quite often (it has come up a few times on Gitter in the past weeks) and there is a working implementation in #1722, I wonder how this could be pushed forward?
The existing PR is outdated, but it should not be too difficult to rebase and move the implementation over to HTTP::Context (which was introduced in between) as suggested by @asterite and @ozra.

[samueleaton]

I'm trying to build a proxy server in crystal and need to add my own x-forwarded-for header before forwarding the request but need this to be available to the server context. Since this issue is super old, should I open a PR? Its a pretty straight forward addition.

[straight-shoota]

@asterite in #1772

We still need this functionality, though, but it will probably come from the core team. Maybe we can copy what Go does, but I don't know (I don't know much about HTTP and proxies).

I'd really like to see this move on and I don't know why this couldn't be implemented by a non-core member. It's actually not that big of a deal I think.

One hold-up was the question where the remote IP should be stored but it seems that HTTP::Context is accepted as appropriate.
The actual implementation is not really difficult and there are many examples to look at, including Go. Some details might need some discussion but I wouldn't expect serious problems.

[straight-shoota]

@ysbaddaden in #7302 (review)

asking for the remote address should be on demand, not called on each and every connection.

I've been thinking about this. While I generally agree, there seems a flaw in this approach: It makes Request dependent on a Socket instance. The method can't provide any value if the socket has already been closed. Obviously, a closed socket usually means handling the request should be aborted because you can't send a response anyway. But it might still be relevant to collect metadata about the connection (like the remote address) for logging and debugging purposes. This won't be possible if the connection is closed premature, before any server handler could query the request for its remote address.

This makes me wonder if it wouldn't be better to retrieve the address at initialization and store it in an ivar. It adds a little performance penalty, but is more fault tolerant and reduces overall complexity.