Added remote_ip to request object

[jwoertink]

Purpose

Fixes #1643

Description

If you need access to the client's IP address, using remote_address can be troublesome since it casts to Socket::Address, and not all Socket::Address subclasses contain an address method. So you usually end up with some code like this:

context.request.remote_address.as?(Socket::IPAddress).try(&.address) || "127.0.0.1"

But now you can just do

context.request.remote_ip || "127.0.0.1"

In addition, I've also added a new habitat config option ip_header_names option. This allows you to configure the header names of where the client IP may come from. e.g. X-Real-Ip, Forwarded-For, etc...

Checklist

• - An issue already exists detailing the issue/or feature request that this PR fixes
• - All specs are formatted with crystal tool format spec src
• - Inline documentation has been added and/or updated
• - Lucky builds on docker with ./script/setup
• - All builds and specs pass on docker with ./script/test

[jwoertink]

Not completely sold on this name, but naming is hard....

[robacarp]

In addition to my note about performance, there are some security implications to this magic. Remote IP detection isn't "guaranteed" or "safe" by any means, but that doesn't stop tons of project from using it to safeguard their admin dashboards, etc.

A safe load balancer implementation of the forwarded-for paradigm ensures that the header is wiped for all traffic, and then sets it correctly if the remote ip address is usable. Nobody should be able to set the header besides the load balancer.

• If Lucky always (or even by default) searches for these headers, then it becomes trivial to spoof a request address by simply injecting a header to all requests.
• If Lucky checks multiple headers, then correctly configuring a load balancer becomes more difficult. All possible headers need to be wiped, then the one "safe" header can be set.
• If the application isn't actually deployed behind a load balancer, Lucky should not be checking these headers at all.

[jwoertink]

Instead of being an array, should it just be the single string value for the header? It would make sense that once you figure out which header is being passed back, it'll always be that same one. I've ran in to an issue where the LB config has changed, but I guess in that case, you'd just update the app to use a different header and do a new deploy.

[robacarp]

@jwoertink I think that makes sense. Should this be an opt-in feature as well?

[jwoertink]

@robacarp which part are you thinking for the opt-in?

The remote_address has been in Lucky for a few versions now, and it's always used the X_FORWARED_FOR header. So that would stay as the default, but just give you the option to change it. The only new bit would be the remote_ip which would just be an extra property on request.

[robacarp]

My preference would be to have the whole thing be opt-in, but I understand why that might not be a desirable change to make. There are a bunch of ways an incorrectly configured x-forwarded-for system can be abused, and having a web server which reads the header without the proxy server in front of it is one of them.

I spent some time poking at what Rack does for this. A notable difference is that it doesn't return just one value, but an array of values. They take the added precaution of filtering out a default list of IP ranges, too. There's a lot more code in this rack module than I expected.

[jwoertink]

Ah, ok. I see what you're saying. In that case, we'd need to remove this

https://github.com/luckyframework/lucky_cli/blob/5649c9e78c9c359710347ef0e91fa87c04072ea6/src/web_app_skeleton/src/app_server.cr.ecr#L6

Then we could add docs on adding it in if you need. I would eventually like to make it more robust like they do with the Rack stuff. IPSpoofing checks and all that...

[jwoertink]

Got sent this article: https://adam-p.ca/blog/2022/03/x-forwarded-for/

One interesting thing is we currently have this line:

lucky/src/lucky/remote_ip_handler.cr

Line 21 in 4cb2108

if x_forwarded = request.headers["X_FORWARDED_FOR"]?.try(&.split(',').first?).presence

which uses the first (left most) IP, but according to this article, we should be using the last (right most)

When deriving the “real client IP address” from the X-Forwarded-For header, use the rightmost IP in the list.

It also mentions to use the last instances of the header, which I think Crystal is doing..

make sure to use the last instance of that header.

I think maybe we can include a link to this article in the comments. We make the default X-Forwarded-For, with the option to change it, but it's really up to you to figure out how to setup your reverse proxy and/or load balancer to pass back what you need. Then this will just handle the 80% use case.