Overall very interesting feature. The filtering is at pretty basic level as only addresses can be used, not ports, packet flags, SELinux labels, connection states, interfaces etc. but perhaps this can be extended in the future (in moderation).

For example, systemd-timesyncd should be able to send UDP packets to NTP servers' ports but not connect with TCP to IRC botnets. The IP addresses of the servers may not be known in advance (and the current filter syntax does not allow name resolution to be used), so it's not possible to specify an address based filter either.

Looks good to me, but perhaps this could be committed separately. Is this needed by other patches?
edit: this applies to 8e0e6ec, sorry.

yes, test-bpf.c needs that to work

Also this could be useful regardless of the rest of patches.
edit: this applies to df1888c.

Well, sure, but it also is necessary to make test-bpf.c

That said, even if it wasn't: we don't really insist on having strictly one separate PR for each topic, we permit some room to include not super-strictly related stuff in other PRs...)

Overall very interesting feature. The filtering is at pretty basic level as only addresses can be used, not ports, packet flags, SELinux labels, connection states, interfaces etc. but perhaps this can be extended in the future (in moderation).

For example, systemd-timesyncd should be able to send UDP packets to NTP servers' ports but not connect with TCP to IRC botnets. The IP addresses of the servers may not be known in advance (and the current filter syntax does not allow name resolution to be used), so it's not possible to specify an address based filter either.

I really don't want to create a second iptables here, hence we should be really careful with adding too many finegrained controls. One of the major benefits of doing this kind of service-level firewalling is that we know more about the context of the traffic than iptables ever knows: we know that nginx.service is about HTTP and ntpd.service about NTP, hence we don't have to derive application context from the port number necessarily. This is particular great for protocols such as bittorrent where the port number is effectively random.

But yeah, it certainly is in the cards to extend this a bit later, but we should keep a focus on keeping this simple, and not make it a fully generic iptables-reeimplementation.

I am very much against doing name lookups for IP ACL configuration. DNS is relatively dynamic, and requires networking to work (except in exceptional cases), and doing networking in order to restrict networking is kinda contradictory...

@poettering I don't think replicating iptables makes sense either. But IMHO the current level of functionality (address filters) is bit too simple to make much difference, I'd add protocol/port support to make it more useful.

Extending the functionality could take a different approach compared to iptables. For example, instead of adding more and more detailed filtering options, how about higher level switches like boolean IPDropUnusualPackets for filtering packets with exotic options?

On licensing:

1. It is OK to use the (GPLv2-only) headers from the kernel in our code (LGPL). Ref.: http://lkml.iu.edu/hypermail/linux/kernel/0301.1/0362.html,

using structure definitions, typedefs, enumeration constants, macros with simple bodies, etc., is NOT enough to make a derivative work. It would take a substantial amount of code (coming from inline functions or macros with substantial bodies) to do that. [RMS]

2. Copying the full header file into our repo is also fine, as long as we keep the copyright information and the original license. In effect, that file is under it's original license, and because of 1., the code that uses it can be separately licensed. This patch series does that with linux/bpf.h. Of course we have to be careful not to move any part of that file elsewhere in the repository or create derived work through other means.

3. Copying parts of the header file and removing the original copyright information and license is OK only if the parts being copied are completely trivial. Using the definitions from samples/bpf/libbpf.h is OK, because of 1., since those are simple structure definitions without any code. But at the same time, the definition of those macros is not completely trivial, and (while it could be argued both ways) I think it's much better to stay on the safe side, and not strip it of the original copyright and license.

Please update the patch series to copy samples/bpf/libbpf.h in its entirety, similarly to bpf.h.

@poettering I don't think replicating iptables makes sense either. But IMHO the current level of functionality (address filters) is bit too simple to make much difference, I'd add protocol/port support to make it more useful.

Well, this PR actually adds useful functionality to programs that were previously not available: IP ACLs for everything. Some programs support something like this, but certainly not all, and with this we add to everything in the service, unconditionally. This is is very different from filtering per IP port, or per IP protocol, or by "unusual packets". All that kind of filtering is essentially unnecessary and doesn't bring much new to the table: if your kernel chokes on "unusual packets", then something is wrong anyway, and if your app does TCP on port 4711 but is not supposed to do that, then your service is borked. That's not reason not to add additional restrictions on what port numbers and which protocols services can bind to, but it's in a very different territory: the IPAddressAllow=/IPAddressDeny= stuff adds new concepts where previously there was nothing there at all, while filtering by IP port/protocol just adds a second line of protection on something that should regularly not happen anyway...

Hence, I think yes, the IP ACLs this PR adds does make a major difference. It adds something very new, and very useful.

I have to say that, for embedded-system, being able to restrict easily all services to a unique IP address is nice...

We are working on a bpf program that firewall at the cgroup level and it would be nice if it could work well with systemd units. At the moment, we have the following race: our bpf program can only be installed after systemd has created the cgroup directory for the unit and if it is not fast enough, the application might already have sent/received network packets. The bpf filter needs to be installed between the fork and the exec.

So for my use-case, IPAccounting, IPAddressAllow and IPAddressDeny is not enough. I would like instead (or in addition) to have an option to specify an existing program:

IPIngressFilterBPF=/sys/fs/bpf/foo/myapp/my-filter
IPEgressFilterBPF=-/sys/fs/bpf/foo/myapp/my-filter2

So an external program could prepare a bpf program pinned on the bpf filesystem before the unit is started.

I would like to avoid Delegate= for the app because except the IP filter at the cgroup level, the app will not do other cgroup mangling.

Also, I would like to avoid using a global helper with ExecStartPre= because it would not work well with RootDirectory=, since the helper would be outside. And also, IPIngressFilterBPF would be more decoupled: if I replace my_firewall_cgroup application by my_other_firewall_cgroup I would not have to edit the ExecStartPre of every single unit file on the system as long as I keep the BPF programs pinned at the same location on the bpf filesystem.

With my solution, I don't need to have much BPF code in systemd: merely a couple of syscalls to retrieve the bpf program from the bpf filesystem and to install it on the cgroup. I would not need systemd to have any BPF assembly or to build a BPF program itself (but I understand it could be useful for other people's use cases).

/cc @iaguis @dongsupark @nhlfr

IPIngressFilterBPF=/sys/fs/bpf/foo/myapp/my-filter IPEgressFilterBPF=-/sys/fs/bpf/foo/myapp/my-filter2

makes sense to me. That said I think the common "/sys/fs/bpf" prefix should not be mentioned: we should use a relative path here, in order not to create the illusion any path outside of /sys/fs/bpf could be used here... And should we decide one day, that we want to reuse this setting for ELF filters, than that still be compatible conceptually, because for that we could then use absolute paths still.

Hmm, thinking more about that, maybe we should actually not permit configuration of any path at all actually, but just make these new options booleans and simply derive the path automatically from the unit name. i.e. if you want to install your own filter, then just name it properly in bpffs and set the bool and systemd would pick it up?

I would like to avoid Delegate= for the app because except the IP filter at the cgroup level, the app will not do other cgroup mangling.

We really should add a property that permits per-unit selection of what to delegate btw: "DelegateConcept=io bpf cpu" or something like that that permits listeing cgroup controllers but also bpf...

(anyway, the ability to enforce arbitrary foreign ebpf networking filters should probably be discussed in a separate issue and/or PR, let's leave this one for the IP accounting/filtering the suggested way)

All that kind of filtering is essentially unnecessary and doesn't bring much new to the table: if your kernel chokes on "unusual packets", then something is wrong anyway, and if your app does TCP on port 4711 but is not supposed to do that, then your service is borked.

I was thinking of the special kind of borked, where the service (e.g. systemd-timesyncd) suddenly starts trying to contact its new masters in botnet control network. I don't think that this could be blocked with system wide rules (or maybe SELinux could do it), if connecting to HTTPS ports is legitimate at system level. An application level firewall can be made much tighter than system wide rules can.