Axum HTTP tracker: compact announce response in public mode
#190
Conversation
josecelano
changed the title
announce response in public mode
|
hi @WarmBeer I have some comments/questions regarding the compact Questions1.- Potential bug in the current implementation. As I mentioned here, I think we are adding an extra "e" byte in the current response between the "peer" and "peer6" dictionary keys. 2.- I'm going to change the implementation to use the bip_bencode crate. I've seen that @Power2All uses the bip_bencode crate here. I think that implementation is much cleaner. 3.- All keys in a Bencoded dictionary must be byte strings and must appear in lexicographical order. In the current responses, keys are not ordered. I've already implemented the solution using the bip_bencode crate and the response is different. I'm using this Bencode specification: https://en.wikipedia.org/wiki/Bencode 4.- Include one of both "peer" or "peer6" keys. In the torrust-axum, only one of the keys appears, depending on whether the client IP is a IPV4 or IPV6 IP. Reading the BEP, I assume we are doing the right thing by adding the extra key. In case a compact response is requested, the tracker MAY add another key to the response; peers6. This key has the same layout as peers in compact mode, but instead of using 6 bytes per endpoint, 18 bytes are used. peers6 contains address-port pairs where the addresses are all IPv6. 5.- Include only IPV4 ips in "peer" key and IPV6 ips in "peer6" key. We are always including both keys "peer" and "peer6", but we only add IPV4 ips in the "peer" key and IPV6 ips in the "peer6" keys. In this case, I think we are doing it wrong because the BEP 07 says: In case the tracker does not support the compact response as described in BEP-23, no change is necessary. Since the original peers response returns peer endpoints in their expanded string form, IPv6 addresses can be passed back this way. So the normal announce response ("peer" key) includes both IPV4 and IPV6 peer ips. ConclusionIn summary, I think I would:
@Power2All I think you are mixing IPV4 (6 bytes) and IPV6 (18 bytes) ips in the "peer6" key. How do you know where an IP finishes if not all the IPS have the same length? cc @da2ce7 |
josecelano
force-pushed
the
issue-187-axum-http-tracker-compact-announce-req-public-mode
branch
from
de3d90f to
b7193b2
Compare
|
Hi @josecelano
Yes I think you are right :').
👍
👍
We already filter the peers for a certain ip version: https://github.com/torrust/torrust-tracker/blob/develop/src/tracker/torrent.rs#L58 . So I suppose we can also drop the empty
We already pass all peers under one key |
|
Hi @da2ce7 @WarmBeer, this is ready to review. I have fixed two bugs:
I have not changed any other behaviour, but please read my comment above because we might need to introduce some changes. If we need to fix the current behaviour we can open a new issue. |
hi @WarmBeer , The Axum Torrust fork adds all IPs (V4 and V6) in both "peer" and "peer6". I think it's wrong for the "peer" key (IPV4) because IPs would have different sizes. But for the "peer6" key, we could convert IPV4 ips into IPV6. My question is whether we should do that or not. Additionally, The Axum Torrust fork filters IP depending on the client's IP. I mean, if the remote client is using an IPV4 IP, the response will include only the "peer" key (and not the "peer6" key) with both IPV4 and IPV6 peers. I would prefer always to return both of them as we do. I'm always talking only about the compact response. |
|
hey @WarmBeer I see what you told me: #[must_use]
pub fn get_peers(&self, optional_excluded_address: Option<&SocketAddr>) -> Vec<&peer::Peer> {
self.peers
.values()
.filter(|peer| match optional_excluded_address {
// Don't filter on ip_version
None => true,
// Filter out different ip_version from remote_addr
Some(excluded_address) => {
// Skip ip address of client
if peer.peer_addr.ip() == excluded_address.ip() {
return false;
}
match peer.peer_addr.ip() {
IpAddr::V4(_) => excluded_address.is_ipv4(),
IpAddr::V6(_) => excluded_address.is_ipv6(),
}
}
})
.take(MAX_SCRAPE_TORRENTS as usize)
.collect()
}If the client has an IPV4 IP, we remove all peers with IPV6 addresses even before building the compact response. I think we should rename that method because it filters out the client peer and all peers that do not use the same IP type/version as the client peer. Are you sure that's the correct behavior? I'm going to take a look at the fork's implementations. Maybe they are also doing the same before building the response. |
|
Hey @WarmBeer, sorry, one more question. The function: #[must_use]
pub fn get_peers(&self, optional_excluded_address: Option<&SocketAddr>) -> Vec<&peer::Peer> {
self.peers
.values()
.filter(|peer| match optional_excluded_address {
// Don't filter on ip_version
None => true,
// Filter out different ip_version from remote_addr
Some(excluded_address) => {
// Skip ip address of client
if peer.peer_addr.ip() == excluded_address.ip() {
return false;
}
match peer.peer_addr.ip() {
IpAddr::V4(_) => excluded_address.is_ipv4(),
IpAddr::V6(_) => excluded_address.is_ipv6(),
}
}
})
.take(MAX_SCRAPE_TORRENTS as usize)
.collect()
}filters out the peers that have the same IP as the peer that is being announced and also other peers which do not have the same IP type (V4 or V6) as the announced peer, right? But shouldn't two peers with the same IP but different ports be considered two different peers? |
I've just converted it into a draft again because I want to refactor some functions, especially function names to make it more explicit the way we calculate the list of peers for a given peer client. |
josecelano
force-pushed
the
issue-187-axum-http-tracker-compact-announce-req-public-mode
branch
from
d595151 to
eed4348
Compare
@WarmBeer I've changed the function to: pub fn get_peers_for_peer(&self, client: &Peer) -> Vec<&peer::Peer> {
self.peers
.values()
// Take peers which are not the client peer
.filter(|peer| peer.peer_addr != client.peer_addr)
// Take only peers with the same IP version as the client peer
.filter(|peer| peer.ip_version() == client.ip_version())
// Limit the number of peers on the result
.take(MAX_SCRAPE_TORRENTS as usize)
.collect()
}Now, two peers with the same IP but different ports are considered different peers, and they will be included in the The PR is ready to review again. |
It will be used to build the bencoded compact annouce response in the HTTP tracker. We are currently writting directly bytes into a byte buffer, but Bencode specification imposes some restrictions like: The keys in a dictionary must me alphabetically orderded. We are not doing that in the current implementation.
- Remove extra byte "e" between "peer" and "peer6" dictionary keys. - Alphabetically order dictionary keys.
Those test should have been enabled when the implementation was done.
…ut a different port Other refactores were made to improve funtions names. BREAKING CHANGE: before this a peer with the same IP as the client that is making the announce request was removed from the announce response regardless whether they have the same IP or not.
josecelano
force-pushed
the
issue-187-axum-http-tracker-compact-announce-req-public-mode
branch
from
eed4348 to
5020722
Compare
|
hi @WarmBeer, in the normal (non-compact) Peer IR = "-qB00000000000000001" = "2d71423030303030303030303030303030303031" = "2d 71 42 30 30 30 30 30 30 30 30 30 30 30 30 30 30 30 30 31" but I'm not sure that's the correct way. As the bencode specification allows any byte string I think we should just return the raw 20-byte array. Currently the response body is a "plain/text" but I think it should be an "application/octet-stream". [Bencoded files contained binary data](Because bencoded files contain binary data). What do you think? cc @Power2All |
|
Hey @josecelano ,
I tried Googling whether this is an ok practice, but I could not find a concise answer. So I think it is best to not map ipv4 for now, until we can test that it is useful and does not introduce any problems. |
Yes, you are right. I was thinking that it might be even more correct to filter on However, maybe the |
|
Hi @josecelano , @Power2All
I have tested a few random public trackers to see what they respond and it seems that 99% sends the |
Yes, I was also considering that, but I guess the client only care about how many peers they can get the torrent from, regardless their ID. |
OK, @WarmBeer that means:
Right? |
@josecelano That's right :) |
OK thank you very much @WarmBeer ! I'm going to switch this PR again to draft to change it, even if the goal for this PR is just to migrate to Axum, I think It makes sense to fix it now in the Axum implementation. Regarding what other trackers are doing with the MIME type, maybe it's not that wrong because the MIME type plan/text does not imply an specific encoding: https://www.rfc-editor.org/rfc/rfc5147.html#page-3 A byte string (a sequence of bytes, not necessarily characters) is encoded as :. The length is encoded in base 10, like integers, but must be non-negative (zero is allowed); the contents are just the bytes that make up the string. The string "spam" would be encoded as 4:spam. The specification does not deal with encoding of characters outside the ASCII set; to mitigate this, some BitTorrent applications explicitly communicate the encoding (most commonly UTF-8) in various non-standard ways. This is identical to how netstrings work, except that netstrings additionally append a comma suffix after the byte sequence. |
|
Hi @WarmBeer, the current non-compact response only returns peers using the same IP version as the client (the peer making the request). What I understand from BEP 7 is we should return all peers regardless of what IP version they are using: https://www.bittorrent.org/beps/bep_0007.html "In case the tracker does not support the compact response as described in BEP-23, no change is necessary. Since the original peers response returns peer endpoints in their expanded string form, IPv6 addresses can be passed back this way." "In case a compact response is requested, the tracker MAY add another key to the response; peers6. This key has the same layout as peers in compact mode, but instead of using 6 bytes per endpoint, 18 bytes are used. peers6 contains address-port pairs where the addresses are all IPv6." The current behavior:Non-compact response: pub struct NonCompact {
pub interval: u32,
pub interval_min: u32,
pub complete: u32,
pub incomplete: u32,
pub peers: Vec<Peer>, // <- Only IPV4 or IPV6 depending of what the client is using
// <- no "peers6" key
}Compact response: pub struct Compact {
pub interval: u32,
pub interval_min: u32,
pub complete: u32,
pub incomplete: u32,
pub peers: Vec<CompactPeer>, // <- Only IPV4 peers
pub peers6: Vec<CompactPeer>, // <- Only IPV6 peers
}The behaviour I think we should implement:Non-compact response: pub struct NonCompact {
pub interval: u32,
pub interval_min: u32,
pub complete: u32,
pub incomplete: u32,
pub peers: Vec<Peer>, // <- IPV4 and IPV6 peers
// <- no "peers6" key
}Compact response: pub struct Compact {
pub interval: u32,
pub interval_min: u32,
pub complete: u32,
pub incomplete: u32,
pub peers: Vec<CompactPeer>, // <- Only IPV4 peers
pub peers6: Vec<CompactPeer>, // <- Only IPV6 peers
} |
|
Hey @josecelano , that makes sense. I thought it would be useless to include incompatible ip's in the response. |
OK, @WarmBeer then I will also fix that in this PR. I also thought it would be useless, but I guess a peer could use different IPs to connect to other peers. And it also seems that if you bind a socket to the wildcard address (on the server) the client could make a request from an IPV4 address. |
|
I would have say, check out Torrust-Axum project on my account. |
|
Hi @WarmBeer, the "peer id" key in the non-compact response is also wrong. It should be "peer id" with a whitespace, not "peer_id". For example: |
Fixed some errors in the normal (non-compact) announce response in the HTTP tracker. Only fo rthe new Aux, implementation: - The peer id should be a 20-byte string not the hex value representation as an string. - Response body (bencode) should be binary (bytes). - The "peer id" key in the peer dictionary should have a white space "peer id" intead of "peer_id" (with underscore).
josecelano
force-pushed
the
issue-187-axum-http-tracker-compact-announce-req-public-mode
branch
from
ee0c2f1 to
7b9131e
Compare
…e response The normal (non-compact) announce response should included all peers except the peer making the request. We were excluding peers that are nos using the same IP version as the peer making the request. Peers are included in the bencoded response in the "peers" key of the main bencoded dictionary.
|
Hi @WarmBeer, I've fixed the bugs. I've also checked the response from another tracker that I've installed locally: As you can see, the "peer id" key has a whitespace, and the peer id is only 20 bytes ( The response does not have any MIME-type header.
Finally, I've left the default Axum header for a body containing bytes. It is supposed to be
I think the default browser behaviour is to show the body if all bytes have a char representation and to download the contents if it has binary data. But not sure about this,. |
|
Content-Type can be plain text or octet, it doesn't matter. |
No description provided.