Overhaul core Tracker: review whitelist functionality

[josecelano]

Parent issue: #1261

Context

Some months ago I wrote a comment on the function to handle an announce request in the tracker core.

    pub fn announce(
        &self,
        info_hash: &InfoHash,
        peer: &mut peer::Peer,
        remote_client_ip: &IpAddr,
        peers_wanted: &PeersWanted,
    ) -> AnnounceData {
        // code-review: in the `scrape` function we perform an authorization check.
        // We check if the torrent is whitelisted. Should we also check authorization here?
        // I think so because the `Tracker` has the responsibility for checking authentication and authorization.
        // The `Tracker` has delegated that responsibility to the handlers
        // (because we want to return a friendly error response) but that does not mean we should
        // double-check authorization at this domain level too.
        // I would propose to return a `Result<AnnounceData, Error>` here.
        // Besides, regarding authentication the `Tracker` is also responsible for authentication but
        // we are actually handling authentication at the handlers level. So I would extract that
        // responsibility into another authentication service.

        tracing::debug!("Before: {peer:?}");
        peer.change_ip(&assign_ip_address_to_peer(remote_client_ip, self.config.net.external_ip));
        tracing::debug!("After: {peer:?}");

        let stats = self.upsert_peer_and_get_stats(info_hash, peer);

        let peers = self
            .in_memory_torrent_repository
            .get_peers_for(info_hash, peer, peers_wanted.limit());

        AnnounceData {
            peers,
            stats,
            policy: self.config.announce_policy,
        }
    }

The tracker can work on listed mode, meaning it only accepts torrents whose info-hash have been included in the whitelist. There was an smell in the code because the whitelist authorization is performed at the delivery layer (controllers) for the announce request but in the scrape handler (domain) in the tracker core for the scrape request. It's strange that is not done at the same level.

In this issue I'm going to explain how the whitelist currently works and how I think it should work. I'm also going to explain how it's implemented (specially the design) and how I think it should be implemented.

Whitelist functionality

Current behavior

For the announce request, the request is rejected immediately (in the controller) and the client receives an error.

For the scrape request, there is no check at the controller level. All requests reach the domain layer. The scrape handler includes all the requested torrents in the response. However, torrents that have not been included in the whitelist return zeroed stats.

Proposal for a behavior change (not implemented in this issue)

For me, It's confusing to return always data for non-allowed torrents because there is no way to know if that torrent is not allow or there is no data for the torrent because there are no peers yet.

I think there are some alternatives:

1. Return zeroed data for the not allowed one (what we are doing right now).
2. Return info only for the allowed ones.
3. Return an error.

• With a generic message indicating that at least one of the requested torrents is not allowed.
• With a message indicating the list of non allowed infohashes.

I have asked ChatGTP (see the response at the end of the issue description) and It said the most common approach is option 2. Apparently (I have not confirm it) OpenTracker and XBT Tracker do that.

I also think option 2 is the best because:

• The client can know anyway if a torrent is included in the list or nor by making an announce request for that torrent.
• The client can stop scraping non allowed torrents because it would have a way to know that the torrent is not allowed, directly from the scrape response.
• If we confirmed that is the common behavior, we would follow the non-official but common practices.

NOTE: That would be a breaking change because clients always expect to receive one item for each torrent in the response. If we decide to change it it should be done in the next major version.

Whitelist design

I'm going to explain how the functionality is implemented now and the changes I want to make immediately. Those changes are not intended to implement the proposal for a behavior change described above. There are independent of that change in the functionality.

Current design

Proposal for a design change

There are some things I would like to refactor:

1. Introduce submodules for handlers in UDP: servers::udp::handlers::{announce, scrape}.
2. Create the missing services (app layer) in the UDP tracker. There is no intermediary level between handlers and the core tracker. It will moved to its own package udp-tracker-core
3. Move the service services::announce::invoke() in the HTTP tracker to the http-tracker-core package.
4. Move the service services::announce::invoke() in the UDP tracker to the udp-tracker-core package.
5. Move whitelist authorization from the handler (in the framework level) to the application service in the http-tracker-core and udp-tracker-core packages.
6. In the tracker-core announce handler return a Result<AnnounceData, AnnounceError> when the torrent is not included in the whitelist.
7. In the tracker-core scrape handler return a Result<ScrapeData, ScrapeError> so we are able to return errors in the future without breaking the public API.
8. Add version module also for the UDP tracker. I don't see any reason to use v1 in the http tracker but not in the UDP tracker.

cc @da2ce7

---

Part of the ChatGTP response about how scrape works for listed trackers: