Re-implement connection ID generation for UDP tracker

[josecelano]

TODO:

• Add test for verify_connection_id
• Move get_connection_id to udp module.
• Move current_time to clock module.
• Add "pepper" to the implementation: Re-implement connection ID generation for UDP tracker #60 (comment)
• Write more documentation. Something like my comment here. I think I'm going to use the Rust /// inline documentation in this case.
• Reimplement connection ID generation function using encryption as proposed by @da2ce7 here.
• Use the function verify_connection_id to verify the UDP announce request.
• Refactor handlers to avoid creating the EncryptedConnectionIdIssuer multiple times.
• Generate SALT when the server starts. We call it SERVER_SCRET now.
• Generate a random secret. We are using a fixed one.
• Refactor code to include some improvements from @WarmBeer PR.
• Change the Blowfish crate. The one we are using is deprecated.
• Refactor code to include some improvements from @da2ce7 PR.

REMOVED FROM TODO:

• Change one of the tests where I use "sleep" instead of mocking time. We need to implement a new serializer where we can inject the time. It's out of the scope of this PR and it's only ten milliseconds the delay introduced in the test execution.

[josecelano]

pub struct TorrentPeer {
    pub peer_id: PeerId,
    pub peer_addr: SocketAddr,
    #[serde(serialize_with = "ser_instant")]
    pub updated: std::time::Instant,
    #[serde(with = "NumberOfBytesDef")]
    pub uploaded: NumberOfBytes,
    #[serde(with = "NumberOfBytesDef")]
    pub downloaded: NumberOfBytes,
    #[serde(with = "NumberOfBytesDef")]
    pub left: NumberOfBytes,
    #[serde(with = "AnnounceEventDef")]
    pub event: AnnounceEvent,
}

@WarmBeer , would not it be nice to rename the updated field like this:

#[serde(serialize_with = "ser_instant")]
#[serde(rename(deserialize = "updated_milliseconds_ago", serialize = "updated_milliseconds_ago"))]
pub updated: std::time::Instant,

To get a json like this:

{
  "info_hash": "4beb7001cb833968582c67f55cc59dcc6c8d3fe5",
  "seeders": 1,
  "completed": 0,
  "leechers": 0,
  "peers": [
    {
      "peer_id": {
        "id": "2d7142343431302d7358376d33786d2877674179",
        "client": "qBittorrent"
      },
      "peer_addr": "192.168.1.88:17548",
      "updated_milliseconds_ago": 385,
      "uploaded": 0,
      "downloaded": 0,
      "left": 0,
      "event": "None"
    }
  ]
}

I generally like to add the units in the attribute name.

We can use any other name like: not_updated_duration_in_msecs, not_updated_duration_in_msecs, ...

[mickvandijke]

I think renaming it to "updated_milliseconds_ago" is fine. Just “updated” is indeed too vague.

[josecelano]

I think renaming it to "updated_milliseconds_ago" is fine. Just “updated” is indeed too vague.

I'm not going to do it in this PR. Especially since it's a breaking change. Unless I find a way to add an extra field in the json without removing the "update" one.

[josecelano]

hi @WarmBeer, I do not understand the requirements for the UDP tracker connection ID.

pub fn get_connection_id(remote_address: &SocketAddr) -> ConnectionId {
    match std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH) {
        Ok(duration) => ConnectionId(((duration.as_secs() / 3600) | ((remote_address.port() as u64) << 36)) as i64),
        Err(_) => ConnectionId(0x7FFFFFFFFFFFFFFF),
    }
}

I have created three temporarily tests, but they may be wrong.

1. connection_id_in_udp_tracker_should_be_the_same_for_one_client_during_some_hours
2. connection_id_in_udp_tracker_should_be_different_for_each_tracker_client
3. connection_id_in_udp_tracker_should_be_different_for_the_same_tracker_client_at_different_times

Are those behaviours right?

1 is passing, but I do not know if that is true. If that is true, how many hours should the connection id last?
2 is failing. Generating two connection ids from different addresses gives the same connection id.
3 is failing. Generating two connection ids from the same address after a while gives the same connection id. Should not the connection id change after a while, even for the same client?

On the other hand, should not the connection id be "guessable by the client"? From https://www.bittorrent.org/beps/bep_0015.html. With that implementation, I guess the client knows it because it knows its address and the current time in Unix Epoch.

Finally, as you can see in the test implementations, "mocking time" in tests leads to fragile, slow, or not feasible tests. One solution is controlling the clock in tests. For example https://users.rust-lang.org/t/proper-way-to-mock-in-rust/60621/5?u=josecelano

For dynamic languages, I could override the SystemTime::now() function for tests. I've seen this answer to do something similar https://stackoverflow.com/a/59614532/3012842 in Rust, but it implies changing your production code. I suppose the best option is to create the trait and use a different implementation for testing. In the end, I have to change the prod code too :-) but injecting the dependency instead of writing the "switch" directly.

In general, I think it is worth it to be able to mock the clock; other tests may depend on the time inderectly. For example, the torrent endpoint returns the time elapsed since the client was updated.

[mickvandijke]

pub fn get_connection_id(remote_address: &SocketAddr) -> ConnectionId {
    match std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH) {
        Ok(duration) => ConnectionId(((duration.as_secs() / 3600) | ((remote_address.port() as u64) << 36)) as i64),
        Err(_) => ConnectionId(0x7FFFFFFFFFFFFFFF),
    }
}

This is a very poor implementation of generating a Connection ID. I see why it fails your tests :'). get_connection_id needs a complete refactor to fulfil the requirements mentioned in https://www.bittorrent.org/beps/bep_0015.html

[josecelano]

pub fn get_connection_id(remote_address: &SocketAddr) -> ConnectionId {
    match std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH) {
        Ok(duration) => ConnectionId(((duration.as_secs() / 3600) | ((remote_address.port() as u64) << 36)) as i64),
        Err(_) => ConnectionId(0x7FFFFFFFFFFFFFFF),
    }
}

This is a very poor implementation of generating a Connection ID. I see why it fails your tests :'). get_connection_id needs a complete refactor to fulfil the requirements mentioned in https://www.bittorrent.org/beps/bep_0015.html

OK, then I can:

• Re-implement it. I can take a look at other implementations. I do not think changing this implementation will produce many changes in the code.
• Change the current_time function to use an external clock. "System clock" for production.

[josecelano]

pub fn get_connection_id(remote_address: &SocketAddr) -> ConnectionId {
    match std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH) {
        Ok(duration) => ConnectionId(((duration.as_secs() / 3600) | ((remote_address.port() as u64) << 36)) as i64),
        Err(_) => ConnectionId(0x7FFFFFFFFFFFFFFF),
    }
}

This is a very poor implementation of generating a Connection ID. I see why it fails your tests :'). get_connection_id needs a complete refactor to fulfil the requirements mentioned in https://www.bittorrent.org/beps/bep_0015.html

I did not know there was another implementation of the UDP tracker, and that function was copied from there:

https://github.com/naim94a/udpt/blob/master/src/server.rs#L354-L359

Other implementations generate a random id, and they store it in the DB:

https://github.com/zaninime/btracker/blob/4c48d8a381befbfce7fea000249630eb6d97e936/cmd/tracker.go#L52

I think I'm going to keep this change out of the scope of this PR and create a new issue, so we can discuss what it's the best implementation for the connection ID.

[josecelano]

It seems the current implementation for the get_connection_id could be valid after all.

Long explanation: #62 (comment)

If that is the case I will continue adding tests to it and an explanation about why it works.

[josecelano]

@WarmBeer I think we could move the function get_connection_id from the src/protocol/utils.rs to the src/udp/handlers.rs module because the connection ID is only used by the UDP tracker, isn't it?

[josecelano]

hi @WarmBeer @da2ce7, I have fixed the tests and created new ones for the connection id generation:

PASS [   0.018s] torrust-tracker protocol::utils::tests::connection_id_in_udp_tracker_should_be_different_for_each_client_at_the_same_time_if_they_use_a_different_port
PASS [   0.012s] torrust-tracker protocol::utils::tests::connection_id_in_udp_tracker_should_be_the_same_for_one_client_during_one_hour
PASS [   0.016s] torrust-tracker protocol::utils::tests::connection_id_in_udp_tracker_should_change_for_the_same_client_and_port_after_one_hour
PASS [   0.016s] torrust-tracker protocol::utils::tests::connection_id_in_udp_tracker_should_expire_after_one_hour
PASS [   0.008s] torrust-tracker protocol::utils::tests::connection_id_is_generated_based_on_remote_client_port_an_hours_passed_since_unix_epoch

I assume the function is OK. We should find out why it sues one to expire instead of the two minutes mentioned in the protocol specification.

I've create a "clock" module, but for this test I was not necessary to mock the system clock. I've only mocked the current time (the "now").

Now you need to pass the "now":

pub fn get_connection_id(remote_address: &SocketAddr, current_timestamp: u64) -> ConnectionId {
    ConnectionId(((current_timestamp / 3600) | ((remote_address.port() as u64) << 36)) as i64)
}

And this is the way to can use it:

pub async fn handle_connect(remote_addr: SocketAddr, request: &ConnectRequest, tracker: Arc<TorrentTracker>) -> Result<Response, ServerError> {
    let connection_id = get_connection_id(&remote_addr, current_timestamp_from_system_clock());
...

I have slightly changed the behavior of the get_connection_id function. It was returning a hardcoded connection id when the system clock throws an error. Now I'm using "unwrap" and the method panics if it can't get the current time. @WarmBeer do you know why we can't not relay on the clock being there always. I assume that if we can't even get the time from the system something really bad has happened. :-). I can't revert that original behavior if you want.

[josecelano]

Rebased.

[josecelano]

hi @WarmBeer, I've been thinking again about your solution, and I do not know if adding a hash increases security. In the previous implementations, you have 10000 possible connections ids for a given two-minute window. In the new implementation for one IP, you still have the same amount of possible connections ids. If you are an attacker, you can call the tracker and generate those 10000 IDs. Becuase the SALT does not depend on the client. It's the same for all clients. This is like a challenge-response authentication where we use a finite number of responses for the challenge. Maybe that's why the protocol says the connection ID must last only 2 minutes. You can make 10000 requests in less than 2 minutes with all the possibilities, and one of them will be accepted as a legitimate request.

On the other hand, adding the IP to the hash does not improve security because it is supposed that one attacker will try to impersonate one IP. So it will try to generate the ID only for that IP.

Am I missing something?

cc @da2ce7

[josecelano]

hi @WarmBeer could you please look at the latest commit.

I've extracted the RequestHandler because we need to keep the cypher initialization state.

I did the most simple thing:

pub struct RequestHandler {
    encrypted_connection_id_issuer: EncryptedConnectionIdIssuer,
    // todo: inject also a crate::protocol::Clock in order to make it easier to test it.
}

but I suppose I should do something like:

pub struct RequestHandler {
    encrypted_connection_id_issuer: Arc<EncryptedConnectionIdIssuer>
}

I suppose I have to move the RequestHandler initialization up to use always the same one. In fact, until we reach the server. So we must also convert the handle_packet function into a struct, right?

I want a confirmation because I do not know that part of the code and it uses concurrency. Until now we were using pure functions.

[mickvandijke]

Hi @josecelano ,

I moved some code around so that the RequestHandler (renamed it to PacketHandler) gets created at UdpServer::start() instead of on every UDP packet: #78

[josecelano]

Now it generates the random Blowfish key here.

I've copied the code from @da2ce7's PR.

I've also asked in the crate repo what the safest way to generate a random key is.

[da2ce7]

@josecelano
You may have noticed that in my PR I decided to use a different implementation of blowfish:

Instead of: https://crates.io/crates/rust-crypto

I used: https://crates.io/crates/blowfish

The rust-crypto crate hasn't been updated in over 6 years, and is deprecated.

[josecelano]

@josecelano You may have noticed that in my PR I decided to use a different implementation of blowfish:

Instead of: https://crates.io/crates/rust-crypto

I used: https://crates.io/crates/blowfish

The rust-crypto crate hasn't been updated in over 6 years, and is deprecated.

@da2ce7 yes, I left you a comment in your PR. I did not see any deprecation message.

You are using https://crates.io/crates/blowfish:

• https://crates.io/crates/blowfish
• https://github.com/RustCrypto/block-ciphers
• 33 contributors
• 488 stars
• 102 forks
• Latest commit: 3 days ago
• It has a warning: "It's generally recommended not to use other cipher implementations in this repository besides the aes crate."

The base branch uses https://crates.io/crates/rust-crypto:

• https://crates.io/crates/rust-crypto
• https://github.com/DaGenix/rust-crypto/
• 50 contributors
• 1.2K stars
• 265 forks
• Latest commit: 6 years ago
• Used by 19.1K
• It has a warning: "Rust-Crypto has not been thoroughly audited for correctness, so any use where security is important is not recommended at this time."

I wonder why it was deprecated. It seems to be used by a lot of people.

But OK, then we should change it.

[josecelano]

@WarmBeer I think I have implemented 80% of the changes in your PR.

I was thinking again about what you said here, and I tried to change my implementation to reflect your idea. This is the commit:

90af290

Now I destroy the current object when it's not needed anymore:

• Here for the encryption.
• Here for the decryption.

I consume the unneeded value, but I do not reuse it. It's a solution in the middle. It uses less memory, but it's not as fast as yours because we build the new value from scratch. If this solution were slow, I would change it again to your proposal.

[mickvandijke]

@josecelano I think doing it like this is ok 👍

My only nitpicks are that preferably extract_client_id etc.. would be changed to to_client_id etc.. as this naming convention seems to be used most in Rust.

And:

fn extract_expiration_timestamp(&self) -> Timestamp32 {
        let timestamp_bytes = &self.0[4..];
        let timestamp = Timestamp32::from_le_bytes(timestamp_bytes);
        timestamp
}  

to:

fn extract_expiration_timestamp(&self) -> Timestamp32 {
        let timestamp_bytes = &self.0[4..];
        Timestamp32::from_le_bytes(timestamp_bytes)
}  

As this is preferred by Clippy: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return

[josecelano]

@josecelano I think doing it like this is ok +1

My only nitpicks are that preferably extract_client_id etc.. would be changed to to_client_id etc.. as this naming convention seems to be used most in Rust.

And:

fn extract_expiration_timestamp(&self) -> Timestamp32 {
        let timestamp_bytes = &self.0[4..];
        let timestamp = Timestamp32::from_le_bytes(timestamp_bytes);
        timestamp
}  

to:

fn extract_expiration_timestamp(&self) -> Timestamp32 {
        let timestamp_bytes = &self.0[4..];
        Timestamp32::from_le_bytes(timestamp_bytes)
}  

As this is preferred by Clippy: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return

OK.

Regarding conventions, I thought the to_client_id was used only when you consume the input value. In this case, It's just a getter. Or not even a getter, It's a kind of parser. Where I can read more about those conventions. I bought a book, but I have not had time to read it yet. Is there an official summary for the style? I've seen this.

Regarding Clippy, I'm using Visual Studio Code with the Rust Analyzer extension, and I thought Clippy was the default linter, but It's not. I've switched it to Clippy, and now I get that warning. By the way, what IDE are you using?

If you want to enforce Clippy rules, we can install Megalinter with Clippy. I did it here.

[josecelano]

@josecelano I think doing it like this is ok +1

My only nitpicks are that preferably extract_client_id etc.. would be changed to to_client_id etc.. as this naming convention seems to be used most in Rust.

And:

fn extract_expiration_timestamp(&self) -> Timestamp32 {
        let timestamp_bytes = &self.0[4..];
        let timestamp = Timestamp32::from_le_bytes(timestamp_bytes);
        timestamp
}  

to:

fn extract_expiration_timestamp(&self) -> Timestamp32 {
        let timestamp_bytes = &self.0[4..];
        Timestamp32::from_le_bytes(timestamp_bytes)
}  

As this is preferred by Clippy: https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return

Done.

[josecelano]

Hey, @WarmBeer In fact now I see a lot of warnings from Clippy. I will fix all of them in a new commit. Mine :-)

[mickvandijke]

Hey @josecelano ,

Regarding conventions, I thought the to_client_id was used only when you consume the input value. In this case, It's just a getter. Or not even a getter, It's a kind of parser. Where I can read more about those conventions. I bought a book, but I have not had time to read it yet. Is there an official summary for the style? I've seen this.

This describes the naming convention for using as, to and into. It comes down to using as when returning a shared reference. into when you are consuming self and returning an owned variable. And to when not consuming self, but still returning an owned variable.

Regarding Clippy, I'm using Visual Studio Code with the Rust Analyzer extension, and I thought Clippy was the default linter, but It's not. I've switched it to Clippy, and now I get that warning. By the way, what IDE are you using?

I am using Intellij IDEA with Rust plugins. I've only switched to Clippy a short time ago, so I wouldn't be surprised if you see warnings for my older code as well :|).

[mickvandijke]

@josecelano

If you want to enforce Clippy rules, we can install Megalinter with Clippy. I did it here.

This is a good idea 👍

[josecelano]

hi @WarmBeer @da2ce7, I think these other PRs make this one obsolete:

• Minimal Mockable Clock Implementation #81
• Format The Entire World #79

The only difference is the type of connection id. This PR uses the encrypted version, and the other uses the witness.

Anyway, I think it's faster to add the encrypted version to the other version. Maybe we should reconsider which version to use. The Witness version seems to be more common and speed could not be a problem given the slight difference.

On the other hand, maybe we could keep all of them just like "live documentation" making it clear only one is used in production at the moment.

My proposal is:

• Keep the current version in PR 81.
• Create a new repo where we put the three options and the benchmark. We could also write an article or better documentation for the three algorithms.