ConnectionInfo::realip_remote_addr() may or may not contain a port number which makes it difficult to handle robustly

[abonander]

Expected Behavior

The expected behavior is for ConnectionInfo::realip_remote_addr() to return a consistently formatted address, either with or without a port number. Preferably without as appending one when the X-Forwarded-For header is supplied would be incorrect (as the port number of the peer will not be the same port as the true peer if the request is being forwarded via a proxy, so copying the port number over is misleading).

Current Behavior

If there is no Forwarded or X-Forwarded-For header, .realip_remote_addr() returns the same value as .remote_addr() which is a socket address containing both an IP address and a port number.

However, if one of those headers is present then realip_remote_addr() will return just a bare IP address.

This makes it very difficult to parse and consume this value because IpAddr::from_str() will reject it in the former case and SocketAddr::from_str() will reject it in the latter case, so one is forced to attempt both.

Of course, since this value is set by the client and potentially untrustworthy (as the docs make very clear), then it may not be any kind of address at all. However, the issue here is that even when it's valid, it's still unnecessarily difficult to work with.

Possible Solution

Instead of directly forwarding to .remote_addr(), since the latter is set using SocketAddr::to_string() anyway, realip_remote_addr should default to socket_addr.ip().to_string().

This shouldn't break any existing usages that are correctly implemented as any attempt to parse this string as either a socket address or IP address without attempting both would have been error-prone to begin with.

This does have the downside of an extra, semi-redundant string allocation. As an optimization, the value stored internally could be an enum that's either a separately allocated String or a Range that can be used to slice the remote_addr() string to just the IP address segment. I'm not certain this would be necessary, though.

Steps to Reproduce (for bugs)

1. Write a request handler that attempts to parse the value of ConnectionInfo::realip_remote_addr() as a SocketAddr:

#[get("/")]
async fn log_realip_remote_addr(req: HttpRequest) -> HttpResponse {
    let realip_remote_addr: SocketAddr = req.connection_info().realip_remote_addr().unwrap().parse().unwrap();

    log::info!("Received connection from {}", realip_remote_addr);

    HttpResponse::Ok().finish()
}

2. Make a request to that handler and see that it completes normally.
3. Manually add X-Forwarded-For: <any IP address> to the request.
4. Observe the handler panic.

Context

When using the API of Circle, a payment provider, mutating requests that touch real money require passing a metadata object with information that can be used to find the user that originated the request in case of an audit: https://developers.circle.com/reference#payments-payments-create

This includes the user's IP address.

When the Actix-web application is deployed as part of a Kubernetes cluster in Google Cloud, and configured with an external load balancer, the .peer_addr() of the request will be that of the load balancer itself in Gcloud, so this cannot be used. Gcloud will append the IP address that originated to the request to the X-Forwarded-For header, creating it if it does not exist: https://cloud.google.com/load-balancing/docs/https#x-forwarded-for_header

Thus, the .realip_remote_addr() will be a socket address when testing the application locally, but a bare IP address when deployed. Ideally it would just always be an IP address for consistency and ease of handling.

Your Environment

• Rust Version (I.e, output of rustc -V): 1.56.0
• Actix Web Version: 4.0.0-beta.8 (looking at the code, the behavior is identical on -beta.15)

[robjtede]

First of all, thanks for the excellent write up 👍🏻

Feels to me that having the port is never useful anyway. The PR to fix this just removes it in the peer address case.