Review the process of selecting the client/peer IP when the tracker is behind a proxy

josecelano commented 1 year ago

A comment on this PR originated a discussion and finally this issue.

Introduction

The tracker can be installed behind a reverse proxy:

client          <-> HTTP proxy                       <-> tracker                   <-> Internet
ip:                 header:                              config:                       peer addr:
145.254.214.256     X-Forwarded-For = 145.254.214.256    on_reverse_proxy = true       145.254.214.256

Preconditions

The tracker service provider also sets up and handles the HTTP proxy.
The HTTP proxy adds a new X-Forwarded-For header with the client IP if the header does not exist yet. If the header exists, it adds the client IP at the enf (rigth) of the header value, containg the list of address.
The tracker is configured with the option on_reverse_proxy = true

Actual behavior

The application is taken the rigthmost IP address in the header. That means the IP of the direct client of the owned HTTP proxy. The reason is we do not trust other proxies. They could spoof that header.

https://github.com/torrust/torrust-tracker/blob/develop/src/http/filters.rs#L135-L158

/// Get `PeerAddress` from `RemoteAddress` or Forwarded
fn peer_addr((on_reverse_proxy, remote_addr, x_forwarded_for): (bool, Option<SocketAddr>, Option<String>)) -> WebResult<IpAddr> {
    if !on_reverse_proxy && remote_addr.is_none() {
        return Err(reject::custom(Error::AddressNotFound));
    }

    if on_reverse_proxy && x_forwarded_for.is_none() {
        return Err(reject::custom(Error::AddressNotFound));
    }

    if on_reverse_proxy {
        let mut x_forwarded_for_raw = x_forwarded_for.unwrap();
        // remove whitespace chars
        x_forwarded_for_raw.retain(|c| !c.is_whitespace());
        // get all forwarded ip's in a vec
        let x_forwarded_ips: Vec<&str> = x_forwarded_for_raw.split(',').collect();
        // set client ip to last forwarded ip
        let x_forwarded_ip = *x_forwarded_ips.last().unwrap();

        IpAddr::from_str(x_forwarded_ip).map_err(|_| reject::custom(Error::AddressNotFound))
    } else {
        Ok(remote_addr.unwrap().ip())
    }
}

Consecuence

Since we do not trust proxies, the tracker will have only the rigth client IP if there is exactly one proxy (owned proxy) in the middle.

If the client tries to reach the tracker and there are other proxies in the middle, the tracker will not have the rigth client public IP.

This was the intended behavior.

If we use the leftmost IP address (or the first public IP starting on the left) the tracker could risk to be spammed with false peers.

The UDP tracker uses a conenction ID to avoid it.

Conclusion

Should we keep the current behavior?
Is there al alternative way to avoid the spam/spoofing?

Links

https://security.stackexchange.com/questions/254199/how-to-prevent-spoofing-of-x-forwarded-for-header

Initial Proposals

Keep the current behavior.
Take the leftmost (public) IP address and do nothing about false peers. The tracker would contain false peers but peers can go offile or the can change thier IP often so that's should not be a big problem.
Try to connect to the peer to validate it.

coldwinds commented 9 months ago

Hello, @josecelano

Is the ip parameter could be supported as an option?

see here https://wiki.theory.org/BitTorrent_Tracker_Protocol

http://some.tracker.com:999/announce ?info_hash=12345678901234567890 &peer_id=ABCDEFGHIJKLMNOPQRST &ip=255.255.255.255 &port=6881 &downloaded=1234 &left=98765 &event=stopped

Which will give a chance to reverse proxy to pass peer's ip by rewrite peer's query and add/overwrite a ip parameter to it.

Opentracker's Makefile also have a build option for this:

https://erdgeist.org/arts/software/opentracker/

By default opentracker will only allow the connecting endpoint's IP address to be announced. Bittorrent standard allows clients to provide an IP address in its query string. You can make opentracker use this IP address by enabling -DWANT_IP_FROM_QUERY_STRING.

josecelano commented 9 months ago

Hello, @josecelano

Is the ip parameter could be supported as an option?

see here https://wiki.theory.org/BitTorrent_Tracker_Protocol

http://some.tracker.com:999/announce ?info_hash=12345678901234567890 &peer_id=ABCDEFGHIJKLMNOPQRST &ip=255.255.255.255 &port=6881 &downloaded=1234 &left=98765 &event=stopped

Which will give a chance to reverse proxy to pass peer's ip by rewrite peer's query and add/overwrite a ip parameter to it.

Opentracker's Makefile also have a build option for this:

https://erdgeist.org/arts/software/opentracker/

By default opentracker will only allow the connecting endpoint's IP address to be announced. Bittorrent standard allows clients to provide an IP address in its query string. You can make opentracker use this IP address by enabling -DWANT_IP_FROM_QUERY_STRING.

Hi @coldwinds I think that's a good-to-have feature and I think it should not be hard to implement. Maybe you can open a discussion for a feature request so that we can discuss how to implement it. We could add a new option in the configuration for the core Tracker service or maybe that could be enabled independently for any of the HTTP and/or UDP trackers. I prefer the second option because it's more flexible:

[[udp_trackers]]
bind_address = "0.0.0.0:6868"
enabled = true
want_ip_from_query_string = true
# ...

[[udp_trackers]]
bind_address = "0.0.0.0:6969"
enabled = true
want_ip_from_query_string = false

[[http_trackers]]
bind_address = "0.0.0.0:7070"
enabled = true
want_ip_from_query_string = false
# ...

[[http_trackers]]
bind_address = "0.0.0.0:7171"
enabled = true
want_ip_from_query_string = true

We're focussed now on releasing version v3.0.0 but I will add it to the roadmap.

Just for the record:

The BEP 03 - HTTP Trackers says:

An optional parameter giving the IP (or dns name) which this peer is at. Generally used for the origin if it's on the same machine as the tracker.

The BEP 15 - UDP Trackers says:

Do note that most trackers will only honor the IP address field under limited circumstances.

cc @da2ce7 @WarmBeer

coldwinds commented 9 months ago

Hi @josecelano I just post a feature request here https://github.com/torrust/torrust-tracker/discussions/532

And yes I think separately config on trackers is more flexible and looks good.

josecelano commented 9 months ago

Using the Forwarded header instead of X-Forwarded-For: https://www.nginx.com/resources/wiki/start/topics/examples/forwarded/

torrust / torrust-tracker