Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug that non-connected Udp sockets aren't displayed #82

Merged
merged 6 commits into from
Jan 7, 2020

Conversation

zhangxp1998
Copy link
Collaborator

This is in response to #81

With this change bandwhich will correctly display UDP traffic sent from sockets that are not connected to any remote port.

Xnip2020-01-06_18-00-07

@zhangxp1998 zhangxp1998 requested a review from ebroto January 6, 2020 23:01
@zhangxp1998 zhangxp1998 force-pushed the udp branch 8 times, most recently from 2f7807b to fd5bacf Compare January 7, 2020 01:43
@imsnif imsnif self-requested a review January 7, 2020 07:23
@imsnif
Copy link
Owner

imsnif commented Jan 7, 2020

Wow, thanks for all the quick work on this!

This is a big change in mostly untested code. I would like to go over this before it's merged. If @ebroto wants to as well (and has the time) that would of course be great :)

@ebroto
Copy link
Collaborator

ebroto commented Jan 7, 2020

Will do this evening if that's OK :)

@imsnif
Copy link
Owner

imsnif commented Jan 7, 2020

Will do this evening if that's OK :)

That's my plan as well after taking care of #51 one way or the other.

Copy link
Owner

@imsnif imsnif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, this looks great. I think you've done good work on both parts of this (the lsof issue and the data structure change).

I like the new approach of keeping track of process names. I think it's much more robust and don't feel it's prone to misidentification. iirc from my networking days, I think a bound local port is unique for an interface - as you mentioned in the other thread.

I also tested this out on my machine and it behaves well. I see some UNKNOWNs here and there, but I think we can squash that behaviour later if it gets in the way (or solve the bugs if we find we're mislabeling them).

I left some comments, otherwise LGTM

if let Some(process_name) = connections_to_procs.get(local_socket) {
Some(process_name)
} else if let Some(process_name) = connections_to_procs.get(&LocalSocket {
ip: IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, we always have the info of whether a LocalSocket is v4 or v6, so can we make an "IpVersion" enum in LocalSocket, then we won't need these conditionals?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, we don't need the extra enum. Because IpAddr itself knows whether it is v4 or v6. We can figure out whether a local socket is v4 or v6, but still, we must conditionally create a v4 wildcard(0.0.0.0) or a v6 wildcard(::0).

src/main.rs Outdated Show resolved Hide resolved
pub enum Protocol {
Tcp,
Udp,
Icmp,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thumbs up

@@ -2,14 +2,14 @@ use crate::network::{Connection, Direction, Segment};

use ::std::collections::HashMap;

#[derive(Clone)]
#[derive(Clone, Debug)]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think cloned is used in clone_and_reset + I would say Debug never hurts

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was referring to the debug... hmm - doesn't it add needless stuff? (I honestly don't know - asking :) )

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It adds an impl Debug for X which is handy to use with dbg! among other use cases. The thing is that if you don't impl it for a struct member, you can naturally not have it automatically impl'd for the struct. Maybe personal taste though.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha - and so it doesn't add anything to the binary unless there's any use of it (with the macro of {:?} for example)? So... why don't we derive it for everything?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it would get optimized out TBH.

Well, there's some influential people that thinks we should, see e.g. Mr. BurntSushi

// "(LISTEN)" or "(ESTABLISHED)", this column may or may not be present
// let connection_state = columns[9];
// If this socket is in a "connected" state
if let Some(caps) = CONNECTION_REGEX.captures(connection_str) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments above are great. Can we add another one here with a sample row matching this regex?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

}
}

pub fn get_protocol(&self) -> Protocol {
return Protocol::from_str(&self.protocol).unwrap();
}

pub fn get_ip_address(&self) -> IpAddr {
return IpAddr::V4(self.ip.parse().unwrap());
pub fn get_remote_ip(&self) -> IpAddr {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these a performance optimization?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, they are just an attempt to parse both Ipv4 and ipv6 addresses.

src/os/macos.rs Outdated Show resolved Hide resolved
@@ -105,7 +105,7 @@ fn multiple_packets_of_traffic_from_different_connections() {
"2.2.2.2",
"10.0.0.2",
54321,
443,
4434,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the changes in the tests - could you explain them a little?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the old test cases, multiple processes are owning local port 443, but each process is connected to a different remote socket. This simulated scenario does not go well with the new logic of identifying processes, since we now expect a local port to be owned by a single process. So I changed the test cases so that each process is on a different port.

Copy link
Collaborator

@ebroto ebroto Jan 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we now expect a local port to be owned by a single process.

I'm thinking out loud, correct me if I'm wrong, but can't you assign multiple IPs to a single network interface? In that case, can't two processes use the same local port for the different IPs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but can't you assign multiple IPs to a single network interface

To the best of my knowledge, you can't. Different interfaces(WiFi/ethernet) can have different IPs. Which means 1 process can use port 443 of WiFi, and another one can use port 443 of ethernet. When identifying processes, local_ip is used along with port number and protocol, so we should be able to handle this situation correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ip command allows you to add multiple IPs to the same interface, but digging a bit into it I think it would create an "alias", so maybe the interface name would be different for our purposes, see here for example.

In any case, I think that's a niche case, not going to nitpick :) But I will try to setup this just to test how it works if I have time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm can you make 1 interface have more than 1 external IPs?(how does routing/ISP work then?)

I also know that this is possible (eg. on routers - they cannot be limited by their physical interfaces, otherwise it would make things very difficult) - but I have a feeling @Alcaro is more in on the nitty-gritty details here than I am.

But indeed, very niche for our use-case.

Routers? IIRC NAT boxes themselves typically have 1 IP? But they modify the packet's destination/source when forwarding packets. Not sure why non-NAT box routers need to deal with multiple IP on same interface though

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm can you make 1 interface have more than 1 external IPs?

Does IPv4 plus IPv6 count?

If no, still possible, but a lot less common.

sudo ip addr add 192.168.3.1/28 dev enp1s0f0
sudo ip addr add 192.168.4.1/28 dev enp1s0f0

Pingable and bindable, and most likely externally reachable if I had a suitably configurable router. Can't get IPv6 working that way, though - they show up in ifconfig, but I get EADDRNOTAVAIL if I try to bind them. No idea why, networking is tricky.

(how does routing/ISP work then?)

Routing/forwarding mostly stays in the kernel, it doesn't show up in lsof. Copying the packets to userspace would be a performance sinkhole. (Maintaining the NAT table may be partially in userspace, not sure.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm can you make 1 interface have more than 1 external IPs?

Does IPv4 plus IPv6 count?

If no, still possible, but a lot less common.

sudo ip addr add 192.168.3.1/28 dev enp1s0f0
sudo ip addr add 192.168.4.1/28 dev enp1s0f0

Pingable and bindable, and most likely externally reachable if I had a suitably configurable router. Can't get IPv6 working that way, though - they show up in ifconfig, but I get EADDRNOTAVAIL if I try to bind them. No idea why, networking is tricky.

(how does routing/ISP work then?)

Routing/forwarding mostly stays in the kernel, it doesn't show up in lsof. Copying the packets to userspace would be a performance sinkhole. (Maintaining the NAT table may be partially in userspace, not sure.)

sudo ip addr add 192.168.3.1/28 dev enp1s0f0
sudo ip addr add 192.168.4.1/28 dev enp1s0f0

I suppose this only adds an additional IP behind the NAT box(AKA private IP)?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course it's not globally accessible. If it was, I'd add 8.8.8.8 to my machine and see how much of the internet breaks.

I'm not even sure if it'd work in the LAN. I think it'd work on most routers, but probably not all - it depends on how they handle ARP requests. (The chosen addresses must be within the router's netmask, otherwise it'll send the packets in wrong direction.)

I can't test locally - the extra addresses I add don't stick. Either something (NetworkManager?) is instantly undoing my config, or ip addr silently fails. (My previous tests were done on an unplugged ethernet port.)

Of course such a configuration is extremely rare outside experimental conditions, even without unrelated processes sharing a port number. If supporting it would be troublesome, there's no need to.

Copy link
Collaborator

@ebroto ebroto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Overall LGTM. It seems to work properly on Linux, for TCP and UDP. If I'm not mistaken there is no support in procfs::net for ICMP.

I left some nits :)

src/os/lsof_utils.rs Outdated Show resolved Hide resolved
src/os/lsof_utils.rs Outdated Show resolved Hide resolved
src/os/lsof_utils.rs Outdated Show resolved Hide resolved
src/network/connection.rs Outdated Show resolved Hide resolved
src/os/lsof_utils.rs Show resolved Hide resolved
src/display/ui_state.rs Show resolved Hide resolved
src/display/ui_state.rs Show resolved Hide resolved
@zhangxp1998 zhangxp1998 force-pushed the udp branch 3 times, most recently from 9303480 to a445b0e Compare January 7, 2020 20:29
@zhangxp1998 zhangxp1998 force-pushed the udp branch 6 times, most recently from 78150a6 to 26ad0c0 Compare January 7, 2020 21:08
@ebroto ebroto self-requested a review January 7, 2020 21:20
Copy link
Owner

@imsnif imsnif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you feel this is ready to be merged - go for it. I'm satisfied once the CI passes.

@imsnif
Copy link
Owner

imsnif commented Jan 7, 2020

Great work, btw! Be sure to add it to the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants