Improved get_connection_id function to conform with the requirements listed in BEP-15#67
Conversation
josecelano
left a comment
josecelano
left a comment
There was a problem hiding this comment.
I think this is a more robust implementation in terms of avoiding spoofing. But I'm not sure if generating a hash every 2 minutes could be too much CPU use. And you also have to generate it for verification.
Maybe we could fix the test for one call and compare the performance between both implementations. Maybe the other implementation is reasonable in terms of security.
| use aquatic_udp_protocol::ConnectionId; | ||
|
|
||
| // todo: SALT should be randomly generated on startup | ||
| const SALT: &str = "SALT"; |
There was a problem hiding this comment.
hi @WarmBeer should we do it in the constructor (TorrentTracker::new)? or maybe in the main:
// Initialize Torrust tracker
let salt = "SALT";
let tracker = match TorrentTracker::new(config.clone()) {
Ok(tracker) => Arc::new(tracker, salt),
Err(error) => {
panic!("{}", error)
}
};I would also inject it get_connection_id so we can test it.
There was a problem hiding this comment.
Hey @josecelano ,
I think the generation of the salt could be moved here:
pub async fn start(&self) {
loop {
let mut data = [0; MAX_PACKET_SIZE];
let socket = self.socket.clone();
let tracker = self.tracker.clone();
tokio::select! {
_ = tokio::signal::ctrl_c() => {
info!("Stopping UDP server: {}..", socket.local_addr().unwrap());
break;
}
Ok((valid_bytes, remote_addr)) = socket.recv_from(&mut data) => {
let payload = data[..valid_bytes].to_vec();
debug!("Received {} bytes from {}", payload.len(), remote_addr);
debug!("{:?}", payload);
let response = handle_packet(remote_addr, payload, tracker).await;
UdpServer::send_response(socket, remote_addr, response).await;
}
}
}
}And since all connection id functions are only relevant for UDP, they can also be moved to the UDP module like you already said.
josecelano
force-pushed
the
protocol-tests
branch
from
9b3ec0a to
bfa803b
Compare
|
Hey @josecelano , I've added two benchmarks (using Criterion) to compare the speed of the two implementations for generating a connection id. The implementation that we currently have is of course very close to instant, so the benchmarks don't even show a time. And this is the result I got for generating a connection id using Blake3 hashes:
This means it would take 0.13 seconds to generate 1,000,000 connection id's using the new implementation. I think this is performant enough for our use case. Else we could potentially optimize the new implementation by skipping the Blake3 hashing and doing some light bit shifting on the same input. |
|
You can run the benchmarks yourself by running |
OK, so I guess we can merge it. We can add that explanation to the documentation and keep the other implementation in the docs. Maybe in the future, it could be a config option depending on whether you want to increase speed or security. I rebased the base branch with your latest changes so you have to rebase this one too. |
mickvandijke
force-pushed
the
protocol-tests
branch
from
2043b15 to
6e1689e
Compare
2 participants
No description provided.