Feature, hide dns queries by default #161
Conversation
olehs0
changed the title
There was a problem hiding this comment.
Hey @olehs0 - great work! Thank you for keeping at it and giving your time for this feature. :)
I left just a few comments in the code. Other than that, can we add a test for this? Something like "pass the flag, send some DNS queries, see that they do not appear"? You can see examples in the other tests.
src/network/sniffer.rs
Outdated
| return None; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it would be more accurate to do this after creating the connection... something like:
if !show_dns {
if connection.remote_socket.port == 53 {
return None;
}
}There was a problem hiding this comment.
Hi @imsnif! Thank you, yeah, sure, i just was going to add a tests for this. This PR should be work in progress yet.
There was a problem hiding this comment.
@imsnif Good afternoon, could you please check my pr?
src/main.rs
Outdated
| @@ -115,6 +118,7 @@ where | |||
| { | |||
| let running = Arc::new(AtomicBool::new(true)); | |||
| let paused = Arc::new(AtomicBool::new(false)); | |||
| let dns_shown = Arc::new(AtomicBool::new(opts.render_opts.show_dns)); | |||
There was a problem hiding this comment.
Why does this need to be an Arc<AtomicBool> - do you think we can make it a normal boolean and then pass it around without cloning or anything? Seems easier to me.
There was a problem hiding this comment.
I just thought that we could switch between show_dns modes via some key. But, yeah, i will replace it, or maybe should I add switching functionality to it?
olehs0
changed the title
olehs0
changed the title
| fn handle_v4( | ||
| ip_packet: Ipv4Packet, | ||
| network_interface: &NetworkInterface, | ||
| show_dns: bool, |
There was a problem hiding this comment.
Hmm... can't we use self.dns_shown inside this function instead of passing show_dns to it?
There was a problem hiding this comment.
You're right. I'm not sure why we implemented it like that shrugs nevermind. :)
There was a problem hiding this comment.
@imsnif ok, so, we keep at as it is, or should i try to modify it and passing self parameter ?
| 54321, | ||
| 53, | ||
| b"I am a fake DNS query packet", | ||
| )), |
There was a problem hiding this comment.
Nice! This way all the other tests test this feature as well, because they should not show this packet.
|
Thank you very much for this @olehs0. Solid work. I hope to see more contributions from you in the future. |
Fixes #153 .
Provide ability to switch shown DNS queries by default, additionally with CLI flag to manage it. Now you can track only needed incoming requests.