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

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

[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: https://github.com/luckyframework/lucky/blob/4cb2108f3e05acaadc6c948a07b2838154e8871a/src/lucky/remote_ip_handler.cr#L21

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.