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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature(event): create net_flow_tcp_begin event #3750

Merged
merged 5 commits into from Dec 18, 2023

Conversation

rafaeldtinoco
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco commented Dec 10, 2023

馃憞 PR Comment BEGIN


8b88ac2 feature(network): add network flow end base for events
34b6682 chore(derive/net_packet): refactor derivation functions
fd0b8da chore(review): no need to check amount of ret variables
12a144d feature(event): create net_flow_tcp_begin event
e2eaabc docs(network): add net_tcp_connect and net_flow_tcp_begin docs


8b88ac2 feature(network): add network flow end base for events

- add the base for keeping flow states (for future stats)
- flow states allow knowing end flows for tcp and udp

12a144d feature(event): create net_flow_tcp_begin event

- create net_flow_tcp_begin event derived from a net_packet_tcp_flow_base. The
  base event is only submitted to userland whenever TCP has SYN and ACK flags.
  It will also submit packets with FIN flags for the future `net_flow_tcp_end`
  derived event (but it needs some state keeping so not done now).

> Differently than ACK+SYN combination flags, which allows observer to know the
> direction of the connection (egress with ACK+SYN the connection is incoming
> to host, ingress with ACK+SYN the connection is outgoing from host), the FIN
> flag might come in either direction, with or without an ACK, so existing
> connections state must be kept in order to know when the flow has ceased to
> exist.

馃憜 PR Comment END

Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz left a comment

Choose a reason for hiding this comment

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

I have a question on approach:
You've chosen to basically create a "fake" flow tcp base packet, and do the standard derive from base to actual.
But you could've just as well appended the flow flag to the regular base tcp packet and added another derive function there, without adding an additional "fake" base packet. I think it would've been more readable, at the cost of more derivation entries (though maybe we could've done double derivation here base->tcp->flow).
So just wanted to know why you chose this approach.

Take note of my first comment on trace.PakcetMetadata too please.

Overall I've also added suggestions below, and I think the logic by its own is sound.

pkg/events/core.go Outdated Show resolved Hide resolved
pkg/events/derive/net_flow_tcp.go Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Outdated Show resolved Hide resolved
pkg/ebpf/c/common/network.h Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Outdated Show resolved Hide resolved
@rafaeldtinoco
Copy link
Contributor Author

Im doing some refactoring now to address suggestions and improve the base network code.

@rafaeldtinoco rafaeldtinoco force-pushed the flow-events branch 4 times, most recently from 8b88ac2 to 7fa222f Compare December 18, 2023 12:22
pkg/ebpf/c/common/network.h Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
pkg/events/derive/net_flow.go Show resolved Hide resolved
pkg/events/derive/net_flow_tcp.go Outdated Show resolved Hide resolved
pkg/ebpf/c/tracee.bpf.c Show resolved Hide resolved
- create net_flow_tcp_begin event derived from a net_packet_tcp_flow_base. The
  base event is only submitted to userland whenever TCP has SYN and ACK flags.
  It will also submit packets with FIN flags for the future `net_flow_tcp_end`
  derived event (but it needs some state keeping so not done now).

> Differently than ACK+SYN combination flags, which allows observer to know the
> direction of the connection (egress with ACK+SYN the connection is incoming
> to host, ingress with ACK+SYN the connection is outgoing from host), the FIN
> flag might come in either direction, with or without an ACK, so existing
> connections state must be kept in order to know when the flow has ceased to
> exist.
- add the base for keeping flow states (for future stats)
- flow states allow knowing end flows for tcp and udp
Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz left a comment

Choose a reason for hiding this comment

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

LGTM

@rafaeldtinoco rafaeldtinoco merged commit 8c89999 into aquasecurity:main Dec 18, 2023
30 checks passed
@rafaeldtinoco rafaeldtinoco deleted the flow-events branch December 18, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

network connection event
2 participants