zeek
zeek copied to clipboard
zeek
Include in Jan's AF_PACKET plugin as builtin plugin
This has come up a few times and the motivation is mainly better "first timer" experience with Zeek. Concretely, if one wants to run a Zeek cluster with multiple workers and reasonable load balancing on Linux, AF_PACKET is a decent start. Without AF_PACKET support being built into Zeek, however, a new user's next experience is that of setting up a development environment in order to compile an external plugin (think compiler, kernel headers, zkg, ...). Only to get what could be termed basic functionality.
This is using the ZEEK_INCLUDE_PLUGINS infrastructure. I've used the all upper case spelling of AF_PACKET in the help output because it seems everyone else references/writes it like that. I think we should also write it like that in the docs.
I had chatted with @rsmmr about this and he could also see shipping the plugin's code within the Zeek source. I'm leaning towards preferring that too, but building it in as a submodule was so easy. I might open a separate PR / branch that pulls in the source code, just to have that as a contrast.
The main reason why including the source code with Zeek is that it would provide an example of a more advanced packet source within the Zeek tree. Examples of other advanced packet sources need special hardware (e.g., myricom, napatech) and more difficult to experiment with.
As of experimentation, having the AF_PACKET source code within the Zeek tree may make iterating on changes to the PktSrc interface easier (thinking #2033).
The main argument I'm aware of for going the submodule route is that resulting improvements to the plugin become available also to users who pull in the plugin via a zkg package. I'm not sure if that's decisive in the end. We could at first use the submodule, and later (say in the 6.x cycle) fully pull it in when people should no longer be using a non-AF_PACKET-enabled Zeek — but that might be overthinking it.
If we stick with the submodule, we should "graduate" the plugin from Jan's space into Zeek proper (assuming Jan you're okay with that!).
I like the submodule approach! Given that there are some todos, likely to be addressed in this process (e.g., https://github.com/J-Gras/zeek-af_packet-plugin/issues/29), and it might take some time for people to update to newer Zeek versions, the submodule approach seems to be a good choice. I am also happy to migrate the repo to the Zeek namespace on Github. This should make experimentation with changes to the Packet Source interface seamless.
Given that there are some todos, likely to be addressed in this process (e.g., https://github.com/J-Gras/zeek-af_packet-plugin/issues/29),
See my last commit - I think using just the latest is fine for now, but adding kernel headers to all the CI things is annoying and also made me wonder whether that dependency is "real":
- Suricata provides AF_PACKET support without requiring kernel headers (and they support it as first-class citizen)
- I've commented out the kernel headers related things in zeek-af_packet-plugin and de-installed the kernel-headers package locally and everything is happy. The linux related headers used by zeek-af_packet-plugin on my system are coming from
linux-libc-dev:$ sudo dpkg -S /usr/include/linux/if_packet.h linux-libc-dev:amd64: /usr/include/linux/if_packet.h
I'll check tomorrow, but I think the linux-headers dependency can just go unless there's a good reason. I believe it's needed for building out-of-tree modules that interact with the kernel API, but AF_PACKET is on a different level and just user-space headers should be fine. I might be missing some historic background, but maybe that can be history?
This PR is currently pointing at a branch in Jan's repo where the kernel-headers dependency has been removed and seems that all distros we have in ci/ are happy with this:
https://github.com/J-Gras/zeek-af_packet-plugin/pull/36
The btests got some more normalization tweaks to ignore Zeek_AF_Packet related things. The bigger effort will likely be docs :-)
I guess you are right: The dependency isn't needed anymore. My understanding is that kernel headers are also required by user space programs that interact with the OS, but Linux distributions are expected to provide kernel headers (https://kernelnewbies.org/KernelHeaders). However, these headers might not be up to date/match the running kernel. If I remember correctly, the plugin had to support CentOS 6 (2.6 kernels), when it was created. So I think the dependency was necessary back then. Nowadays, AF_Packet with tpacket v3 should be widely available, so that the defaults are just fine to use. The dependency was just never challenged. So, thanks a lot!
The main argument I'm aware of for going the submodule route is that resulting improvements to the plugin become available also to users who pull in the plugin via a zkg package.
It's maybe not the scenario you had in mind, but installing a plugin via zkg that's already builtin appears a fun ride (e.g. #2403). Further, after adding few CI pieces to zeek-af_packet-plugin, I'm suspecting that these will become non-functional once the zeekurity/zeek images and Debian OBS binary packages start to have AF_PACKET included by default.
Above can maybe be fixed, but could also be a motivating point for "external-plugins-builtin-by-default" should be part of the Zeek codebase, and --include-plugins a mechanism for downstream distributors rather than Zeek base itself.
The main argument I'm aware of for going the submodule route is that resulting improvements to the plugin become available also to users who pull in the plugin via a zkg package.
It's maybe not the scenario you had in mind, but installing a plugin via zkg that's already builtin appears a fun ride (e.g. #2403). Further, after adding few CI pieces to zeek-af_packet-plugin, I'm suspecting that these will become non-functional once the zeekurity/zeek images and Debian OBS binary packages start to have AF_PACKET included by default.
Above can maybe be fixed, but could also be a motivating point for "external-plugins-builtin-by-default" should be part of the Zeek codebase, and --include-plugins a mechanism for downstream distributors rather than Zeek base itself.
Sorry for the delay here, folks. The PR looks good to me. My only suggestion is that we migrate the package prior to merging. I just forked the plugin repo into the Zeek project before I realized that there's a proper way to transfer ownership, in the "Danger" settings of the original repo. If that sounds okay to you @J-Gras, please feel free to transfer any time and we'll look out for it over here. (I've deleted the fork.)
Fwiw, I feel pretty good about the submodule route. We keep it functional and maintained for folks running older Zeeks, I like the symmetry with the Spicy plugin, and we can always change it down the road. Also, wouldn't problems like #2403 also come up if it's included directly, not via a submodule? We'd still be building a plugin.
Also, wouldn't problems like #2403 also come up if it's included directly, not via a submodule? We'd still be building a plugin.
Yes. Technically the problem will continue to exist.
On the non-technical side, though, if AF_Packet is part of the Zeek source tree proper, building it as an external plugin effort and can be stamped unsupported or "you know what you're doing". With keeping the submodule, we kind of imply keeping the external repo and supporting installing it as an external plugin.
Edit: That said - I'm okay with it either way.
Fyi the migration of Jan's package is now complete: https://github.com/zeek/zeek-af_packet-plugin
Fyi the migration of Jan's package is now complete: https://github.com/zeek/zeek-af_packet-plugin
Rebased, updated, and moved the NEWS entry from the 5.1 section up into 5.2 now.
CI failed due to centos-7 being old enough to not have the TP_STATUS_CSUM_VALID.
Proposed fix to the plugin here: https://github.com/zeek/zeek-af_packet-plugin/pull/43
@ckreibich - is there anything you're missing here before this could go in?
Sorry for the delay Arne — it all looks good to me. If there's nothing more from you and Jan, this'll go in tomorrow. Thank you both!
Fyi I merged this with a submodule tweak to point it at Jan's new 4.0.0 release commit, i.e. the latest in its master.