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

feat: Enrich addr info with remote addr info #684

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

Asphaltt
Copy link
Member

@Asphaltt Asphaltt commented Dec 8, 2024

It is able to get full 5-tuple info when hooking connect and accept syscalls with format saddr:sport-daddr-dport always.

$ sudo ./bin/ecapture tls -d
...
2024-12-08T16:44:24Z DBG AddConn success fd=5 pid=113767 tuple=192.168.241.133:37736-172.217.194.113:443
2024-12-10T05:08:04Z DBG AddConn success fd=4 pid=121587 tuple=192.168.241.1:54587-192.168.241.133:8080
...

See #682 to get more context.

See eBPF Talk: 在 bpf 里将 fd 和 sock 关联起来 for design ideas and technical implementation.

@cfc4n cfc4n added the enhancement New feature or request label Dec 9, 2024
@chilli13
Copy link

chilli13 commented Dec 9, 2024

	rport, lport := binary.BigEndian.Uint16(ce.Ports[0:2]), binary.LittleEndian.Uint16(ce.Ports[2:4])
	raddr, laddr := net.IP(ce.Addrs[0:4]), net.IP(ce.Addrs[4:8])
	ce.Tuple = fmt.Sprintf("%s:%d-%s:%d", laddr, lport, raddr, rport)

Compared with remote/local ip/port, the expression of src/dest ip/port is more in line with the usage scenario requirements. Can the information output be designed according to the src_ip:src_port-dest_ip:dest_port format?

@Asphaltt
Copy link
Member Author

Asphaltt commented Dec 9, 2024

Can the information output be designed according to the src_ip:src_port-dest_ip:dest_port format?

Can. But unnecessary, because of performance consideration.

As struct connect_event_t is already compact enough, it must swap positions of local&remote addr info in bpf when it's passive connection, in order to avoid adding more field to struct connect_event_t.

@cfc4n WDYT?

@cfc4n
Copy link
Member

cfc4n commented Dec 9, 2024

Can the information output be designed according to the src_ip:src_port-dest_ip:dest_port format?

as log 2024-12-08T16:44:24Z DBG AddConn success fd=5 pid=113767 tuple=192.168.241.133:37736-172.217.194.113:443 ,Isn't this the format you expected?

@chilli13
Copy link

Yes, it is the expected format, but compared with local-remote, the source -destination format is more suitable for most business scenarios. The current format can also meet the requirements, we will consider the business layer and make the difference between source and destination. Thanks very much for the contribution.

@chilli13
Copy link

Can the information output be designed according to the src_ip:src_port-dest_ip:dest_port format?

Can. But unnecessary, because of performance consideration.

As struct connect_event_t is already compact enough, it must swap positions of local&remote addr info in bpf when it's passive connection, in order to avoid adding more field to struct connect_event_t.

@cfc4n WDYT?

Add a direction field to indicate whether the session is initiated by the local . For example, 0: accept, indicating passive acceptance of the connection, and 1: connect, indicating that the host initiates the request. Whether this solution can be accepted?

AddConn success fd=5 pid=113767 direction=accept/connect tuple=192.168.241.133:37736-172.217.194.113:443

@Asphaltt
Copy link
Member Author

AddConn success fd=5 pid=113767 direction=accept/connect tuple=192.168.241.133:37736-172.217.194.113:443

It's unacceptable for me. However, it's easier to provide saddr:sport-daddr:dport than direction.

It is able to get full 5-tuple info when hooking `connect` and `accept`
syscalls with format `saddr:sport-daddr-dport` always.

```bash
$ sudo ./bin/ecapture tls -d
...
2024-12-08T16:44:24Z DBG AddConn success fd=5 pid=113767 tuple=192.168.241.133:37736-172.217.194.113:443
2024-12-10T05:08:04Z DBG AddConn success fd=4 pid=121587 tuple=192.168.241.1:54587-192.168.241.133:8080
...
```

Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
Copy link
Member

@cfc4n cfc4n left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks.

@cfc4n cfc4n merged commit 4ada2d5 into gojue:master Dec 10, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants