[WIP] Add remote_address to HTTP::Server::Context

[omarroth]

Would appear to close #453 and #5784.

This PR adds support for remote_address by pulling it from the underlying IO.

I'm currently unsure if adding this as a separate field to Context is appropriate. It may make more sense to provide a Context::Info or Context::Metadata, which could contain remote_address and local_address, or other useful information that would be relevant in some cases, with functionality similar to #1722.

Example use:

# A very basic HTTP server
require "http/server"

server = HTTP::Server.new do |context|
  context.response.content_type = "text/plain"
  context.response.print "Hello to #{context.remote_address}!"
end

address = server.bind_tcp 8080
puts "Listening on http://#{address}"
server.listen

$ curl http://127.0.0.1:8080
Hello to 127.0.0.1:37024!

This PR also patches OpenSSL::SSL::Socket so the IPAddress can be determined for servers with SSL enabled.

It might be good to change this line to a warning or safer default.

Still needs tests.

[sam0x17]

This is so needed. looks like one potential issue on linux though based on the CI output

[straight-shoota]

IMHO this PR is premature. We should continue the discussion about how to expose and implement this in #5784.

[omarroth]

I agree it's premature, which is why it's marked as [WIP].

This comment in #453 is mostly the reason for this PR. What is implemented here is minimal functionality with the expectation that it will be expanded to include support for e.g. X-Forwarded-For similar to #1722.

My hope for this PR is to further discussion. There doesn't appear to be much consensus reached in #5784, although I would be happy to move this functionality into HTTP::Server::Response if that is more appropriate.

[omarroth]

A couple thoughts:

In #1722 there's an option to use headers to determine the IP, namely X-Forwarded-For, Client-Ip, and several others were mentioned. Would it make sense to leave this to the user to implement? There appear to be a pretty varied list of headers in use that would likely need to be supported, while the user would have a better understanding of what they need.

This example exposes client_ip, which is determined from headers, so it might make sense to implement that here. So for example, if a client is connecting to a server via proxy, client_ip would return the result of X-Forwarded-For, and remote_ip would return the proxy.

It makes sense to me that this should belong in HTTP::{Server,Client}::Response, since there are already several other methods for handling the socket itself, namely close, flush, and write. I would consider this PR a way of providing information about the socket. It is also keeps HTTP::Server::Context clean. I'd be interested in other thoughts.

Currently this PR supports HTTP servers, with and without SSL. Are there any other protocols/sockets that need to be supported?

[ysbaddaden]

My problem is always the same: asking for the remote address should be on demand, not called on each and every connection.

It should also be located on Request. It should also be a nilable, or a raise on nil since the IO is expected to be a socket, but that's not always the case.

[straight-shoota]

@omarroth

In #1722 there's an option to use headers to determine the IP, namely X-Forwarded-For, Client-Ip, and several others were mentioned. Would it make sense to leave this to the user to implement?

As you state in the next paragraph, these are two different concepts: a) The remote address of the immediate network connection. b) The client address of a HTTP connection which may be equal to the remote address, but that could also be some kind of proxy or other intermediary.

I think both should be made available in stdlib's HTTP implementation, but this PR focuses on a) which needs to come first. b) is the next step.

Currently this PR supports HTTP servers, with and without SSL. Are there any other protocols/sockets that need to be supported?

The server is specific to HTTP, so no other application servers need to be supported. But obviously all kinds of HTTP transport streams, i.e. TCP and Unix sockets with and without SSL.

[omarroth]

Thanks for taking a look at this everyone.

I moved remote_address into HTTP::Server:Response, and it now will raise an exception on unsupported socket types.

Currently it's handling the socket twice. Once to unwrap it if necessary and then again to determine the underlying type. It may be necessary to delegate it to SSLSocket if it's possible for a socket to be wrapped multiple times, but otherwise it seems cleaner to handle that in one place.

[ysbaddaden]

Why was the method moved the response object? It doesn't make sense.

HTTP::Server::Response musn't require OpenSSL, please implement #remote_address on SSLSocket, so the method can be reduced as a mere delegate to io, and we can keep building the compiler or a HTTP server without OpenSSL. That is:

def remote_address
  if (io = @io).responds_to?(:remote_address)
    io.remote_address
  else
    raise "#{@io} doesn't have a remote address"
  end
end

def remote_address?
  if (io = @io).responds_to?(:remote_address)
    io.remote_address
  end
end

[omarroth]

Where would you suggest exposing this?

[ysbaddaden]

The request object. Later we can implement #client_ip on the request, that will use the remote address and the x-forwarded request headers to return a client ip.

[straight-shoota]

That was essentially the consensus in #5784 and why previous attempts like #5870 failed. It just didn't work out yet because HTTP::Request is used in both the client and server and we need to agree whether we want to split it into two separate classes (HTTP::{Client,Server}}::Request) like it is already the case for Response.

[omarroth]

Apologies for not giving this PR much attention. I moved remote_address into HTTP::Request and made a minor change to HTTP::Request to allow for pulling it from the underlying IO.

I'll add specs when I have a free moment.

[samueleaton]

IO | Nil can be changed to IO? to match Headers?

[omarroth]

Closing in favor of #7610.