search

crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.41k stars 1.62k forks source link

Expose connection metadata in HTTP::Server::Context #5784

Open straight-shoota opened 6 years ago

straight-shoota commented 6 years ago

With #5776 upcoming, HTTP::Server can listen on multiple networks sockets - and even without that, it would be useful for a handler to be able to access information about the network connection used to process.

My initial idea was to expose the socket as Context#socket but that would also allow to take over the IO which should be avoided (there's Response#upgrade for that).

There really just needs to be access to metadata about the socket, for example local_address and remote_address (note that remote_address is not particularly useful to identify the actual client's IP because of proxies and whatnot). I don't know if there would be any other useful data.

We could think about encapsulating the values in a struct like Socket::Metadata but I'm not sure it's worth adding another type for just two data points.

A question is whether to expose it: on Context or Response. I'd say the most logical place would be Context as it relates to both request and response. On the other hand, other network related features (such as the mentioned #upgrade as well as #close) are on Response.

Another issue is how to implement it. It could either be just delegates to the Socket instance. this would be easy in Reponse because the network socket is already available internally as @io. For Context it would either need access to Response's internal io or store a reference to the socket itself. A different solution would be to just copy the values to the Context (or Response) instance.

And the third question I'm not sure about, how this should behave when no network connection is involved (for example in specs that don't go through the network stack). Should it just return nil or a default value like the loopback address (but what would that be for Unix sockets?).

straight-shoota commented 6 years ago

I've looked at some other API's and I found a nice addition in Java's ServletRequest: It has methods named getServerName() and getServerPort() which expose the values from the Host header if available and default to the local address. Go's http.Request similarly has a Host property which is either the value of the Host header or the host included in the request URI. It may or may not include a port.

RX14 commented 6 years ago

It should conceptually be on Request instead of Response, Context should continue holding just a request and response.

straight-shoota commented 6 years ago

@RX14 Having it on Request would mean, these methods were also exposed in a client context, because this class is used as representing a request both in the client and server. A client request can't have any network connection data unless it is modified when it is being sent over a network connection. And it would still probably need different implementations for client and server context.

There are separate Response classes for client and server, which makes it far more easily to incorporate access to connection data into the server response. Conceptually, this is not great, I agree. But I don't think we can implement this with a sane API unless we had a separate server Request type.

RX14 commented 6 years ago

Which is why I said more than a year ago now we shouldn't use the same class for client and server requests. We either should have 2 or 4 classes: HTTP::Request and HTTP::Response or HTTP::{Client, Server}::{Request, Response}. This weird middle ground is hacky.

rdp commented 6 years ago

See also #1722 and #453

straight-shoota commented 5 years ago

7610 finally added HTTP::Request#remote_address to retrieve the remote (i.e. client) address of an HTTP request from a server context. So the most requested feature has arrived. But there is more.

With HTTP::Server being able to listen on multiple interfaces, it might be interesting to expose information on the local interface as well. There could be #local_address in the same manner. But I'm not sure whether that's worth it, storing the address in every request when it's only very rarely useful.

I don't think there is currently any means to discover whether a request was made over TLS or not. This has probably more relevant use cases. For example, when the server has interfaces for both HTTP and HTTPS, a handler might restrict some behaviour depending on whether its encrypted or not (for example form submissions, login cookies etc.). This would be similar to the X-Forwarded-Proto header.

elaine-jackson commented 5 years ago

7610 finally added HTTP::Request#remote_address to retrieve the remote (i.e. client) address of an HTTP request from a server context. So the most requested feature has arrived. But there is more.

With HTTP::Server being able to listen on multiple interfaces, it might be interesting to expose information on the local interface as well. There could be #local_address in the same manner. But I'm not sure whether that's worth it, storing the address in every request when it's only very rarely useful.

I don't think there is currently any means to discover whether a request was made over TLS or not. This has probably more relevant use cases. For example, when the server has interfaces for both HTTP and HTTPS, a handler might restrict some behaviour depending on whether its encrypted or not (for example form submissions, login cookies etc.). This would be similar to the X-Forwarded-Proto header.

Nginx can pass a header with the schema used to connect. Of course this isn't something Crystal's HTTP Server can do for you. But if you need this functionality now, it is available by means of a reverse proxy like Nginx (which given the amount of testing in production, among other factors https://stackoverflow.com/a/50522656 (answer discusses Ruby and Puma, same concepts apply with Crystal), you should be using in front of Crystal's application server)

Blacksmoke16 commented 3 years ago

I don't think there is currently any means to discover whether a request was made over TLS or not. This has probably more relevant use cases.

Bumping this. A use case I have is for generating URLs to a given route, which could be an absolute URL including the scheme/hostname. I think ideally I would use the scheme of the original request, but alas I'm not able to determine that.

Blacksmoke16 commented 2 months ago

As a follow up to my last comment, can someone confirm the way to determine if a request was HTTP or HTTPS is essentially to use X-Forwarded-Proto from a trusted proxy and if you're not using a proxy you're just kinda out of luck and should default to HTTP? I'm not missing something here? As from what I can tell that seems to be true.

EDIT:

I went and checked a few diff frameworks handle getting the scheme/if request is over https:

  1. Express will use trusted proxy if available, otherwise fallback on if the connection has TLS information exposed
  2. Django will use trusted proxy if available, otherwise fallback on http. The request type can be subclassed which changes the fallback to wsgi.url_scheme which is set based on if the config has ssl enabled. At least via gunicorn
  3. Symfony will use trusted proxy if available, otherwise fallback on the value of $_SERVER['HTTPS'], tho I have no idea how this works as I can't even find it in the src code :shrug:

I dont think Crystal exposes TLS connection info on the request. So in order to determine the scheme I guess I'd either have to assume https if the server was started with bind_tls or just keep it simple and require a trusted proxy and use http otherwise? Or allow the user to define the base url/scheme in configuration to make it more explicit...