Add RestrictNetworkInterfaces= #18385

mauriciovasquezbernal · 2021-01-26T17:01:30Z

Add RestrictNetworkInterfaces= option to limit the network interfaces a program in a unit can use.

This PR uses the libbpf support introduced in #17655. The feature is implemented with BPF_PROG_TYPE_CGROUP_SKB programs attached to the BPF_CGROUP_INET_{INGRESS|EGRESS} hooks.

There are still some TODOs but I wanted to open the PR to start the discussion now.

TODO:

Investigate compatibility with bpf-firewall as both of them use the same kind and attach type for the BPF programs. In order to use options provided by the bp-firewall together with RestrictNetworkIntferfaces both programs should use BPF_F_ALLOW_MULTI but it's not used by bpf-firewall: acf7f25. Related bpf: pid1: Pin reference to BPF programs for post-coldplug #17495.
Rebase once Introduce SocketBind{Allow|Deny}= properties powered by source compiled BPF #17655 (or other PR introducing libbpf) is merged.
How to remove elements from a set of strings?
When to detach the eBPF programs https://github.com/systemd/systemd/pull/17655/files#r561982214?
Create & configure veth pairs from C in the tests?

/cc @iaguis

lgtm-com · 2021-02-19T22:02:21Z

This pull request introduces 1 alert when merging a9c79a8 into fa98c99 - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2021-02-22T15:23:05Z

This pull request introduces 1 alert when merging c2d7499 into 5735ab0 - view on LGTM.com

new alerts:

1 for Unused import

poettering

Looks pretty good! A few comments

src/core/bpf/restrict_ifaces/restrict-ifaces.bpf.c

src/core/restrict-ifaces.c

poettering · 2021-02-24T14:26:23Z

src/core/restrict-ifaces.c

+        dummy = 0;
+
+        SET_FOREACH(iface, restrict_network_interfaces) {
+                ifindex = if_nametoindex(iface);


Please use resolve_interface() from socket-netlink.c: it also supports resolving new-style "altnames" for interfaces

Do you mean resolve_ifname()? resolve_interface() tries to parse the interface name to an integer before using netlink, I think it's not correct for this use case as an interface with an integer name can have a if_index different to the name.

well, in networkd and elsewhere we generally explicitly check that people cannot name ifaces numerically, so that no ambiguity can happen, and so that ifindex and ifname can be used unambigiously interchangable.

For transient services it might make sense to pinpoint ifaces by ifindex, since they are generated programmatically on the fly. hence i see no eason why not to accept ifindexes here, if people want to specify them

src/core/load-fragment.c

src/core/dbus-cgroup.c

src/test/test-restrict-ifaces.c

lgtm-com · 2021-02-25T01:06:58Z

This pull request introduces 1 alert when merging eee13b7 into fa02711 - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2021-02-25T20:58:28Z

This pull request introduces 1 alert when merging 56e90a5 into 584e9ba - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2021-02-26T01:57:00Z

This pull request introduces 1 alert when merging ca463a4 into 8b2620e - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2021-02-26T12:51:49Z

This pull request introduces 1 alert when merging c146e5b into 499c851 - view on LGTM.com

new alerts:

1 for Unused import

poettering · 2021-05-10T15:51:09Z

Any chance you can rebase now that #17655 has been merged? Should be an easy merge then

mauriciovasquezbernal

I handled the comments.

mauriciovasquezbernal · 2021-08-17T18:32:53Z

src/core/restrict-ifaces.c

+        _cleanup_(sd_netlink_unrefp) sd_netlink *rtnl = NULL;
+        char *iface;
+        uint8_t dummy = 0;
+        int ifindex;


I moved dummy and ifindex, others are needed outside.

mauriciovasquezbernal · 2021-08-17T18:39:24Z

src/core/load-fragment.c

+                        log_syntax(unit, LOG_WARNING, filename, line, r,
+                                   "Trailing garbage in %s, ignoring: %s", lvalue, rvalue);
+                        break;
+                }


Ah, got it. Thanks for the clarification.

poettering

looks really good, we are close!

poettering · 2021-08-18T14:33:38Z

src/core/bpf/restrict_ifaces/restrict-ifaces.bpf.c

+}
+
+SEC("cgroup_skb/egress")
+int sd_restrictif_e(struct __sk_buff *sk) {


just wondering, souldn't the param be const?

poettering · 2021-08-18T14:44:10Z

src/core/dbus-cgroup.c

+
+                        if (!c->restrict_network_interfaces) {
+                                c->restrict_network_interfaces_is_allow_list = is_allow_list;
+                        }


no {} around single line if blocks

poettering · 2021-08-18T14:45:43Z

src/core/dbus-cgroup.c

+                                return 1;
+                        }
+
+                        if (!c->restrict_network_interfaces) {


i'd probably check for emptiness rathre than non-NULL here. i.e. if (set_isempty(c->restrict_network_interfaces). note that a NULL set is considered empty, hence such a check is mostly equivalent, but kinda underlines that the set is allocated lazily here

src/core/dbus-cgroup.c

poettering · 2021-08-18T14:48:32Z

src/core/bpf/restrict_ifaces/restrict-ifaces.bpf.c

+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+
+const volatile __u8 is_allow_list = 0;


how does this actually work? i.e. how does the consuming code get access to this, and in particular write access?

The generated skeleton includes a structure with those variables, they are filled by the user space part (obj->rodata->is_allow_list = is_allow_list; in restrict-ifaces.c) before loading the BPF object. When the object is loaded, libbpf creates an eBPF map that backes up those variables, libbpf copies the value of those to the map and freezes it to avoid further modifications in the case the variable is const.

src/core/dbus-cgroup.c

src/core/load-fragment.c

poettering · 2021-08-18T14:52:25Z

the ifname validity check is still missing...

The code is composed by two BPF_PROG_TYPE_CGROUP_SKB programs that are loaded in the cgroup inet ingress and egress hooks (BPF_CGROUP_INET_{INGRESS|EGRESS}). The decision to let a packet pass or not is based on a map that contains the indexes of the interfaces. Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

This commit introduces all the logic to load and attach the BPF programs to restrict network interfaces when a unit specifying it is loaded. Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

mauriciovasquezbernal · 2021-08-18T20:57:15Z

the ifname validity check is still missing...

🙈 I added it to both, load-fragment.c and dbus-cgroup.c.

test/units/testsuite-62.sh

poettering · 2021-08-19T08:40:37Z

Excellent! Found one indenting nit, but doesn#t matter. Let's get this baby landed.

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

jameshilliard · 2021-08-23T01:16:19Z

FYI I refactored the meson build for this in #20429 to try and fix cross compilation, probably good to try and test everything still works there.

github-actions bot added the systemctl label Jan 26, 2021

mauriciovasquezbernal mentioned this pull request Jan 26, 2021

bpf: extend bpf cgroup program support #13496

Merged

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch 5 times, most recently from a01e2c4 to 3a13693 Compare February 2, 2021 18:52

iaguis mentioned this pull request Feb 11, 2021

Add RestrictFileSystems= property using LSM BPF #18145

Merged

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch from 3a13693 to a9c79a8 Compare February 19, 2021 21:20

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch from a9c79a8 to c2d7499 Compare February 22, 2021 14:41

poettering requested changes Feb 24, 2021

View reviewed changes

poettering added pid1 reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Feb 24, 2021

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch from c2d7499 to eee13b7 Compare February 25, 2021 00:29

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch from eee13b7 to 56e90a5 Compare February 25, 2021 20:20

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch from 56e90a5 to ca463a4 Compare February 26, 2021 01:13

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch from ca463a4 to c146e5b Compare February 26, 2021 12:09

jkartseva mentioned this pull request Apr 21, 2021

Introduce SocketBind{Allow|Deny}= properties powered by source compiled BPF #17655

Merged

poettering added the needs-rebase label May 10, 2021

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch 4 times, most recently from bb1568c to 05d4ac0 Compare May 19, 2021 19:35

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Aug 17, 2021

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch from 90d7a5f to 89028c4 Compare August 17, 2021 18:57

mauriciovasquezbernal commented Aug 17, 2021

View reviewed changes

poettering requested changes Aug 18, 2021

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch from 89028c4 to b65230a Compare August 18, 2021 15:58

mauriciovasquezbernal added 7 commits August 18, 2021 15:55

core: implement RestrictNetworkInterfaces=

6f50d4f

This commit introduces all the logic to load and attach the BPF programs to restrict network interfaces when a unit specifying it is loaded. Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

core: add load fragment implementation for RestrictNetworkInterfaces=

4f0c25c

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

core: add D-bus properties for RestrictNetworkInterfaces=

a59cb62

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

Document RestrictNetworkInterfaces dbus properties

57585d5

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

man: add RestrictNetworkInterfaces= documentation

795ccb0

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

src/test: add restrict network interfaces to test-cgroup-mask

2ce150f

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch from b65230a to 294f3ef Compare August 18, 2021 20:56

poettering reviewed Aug 19, 2021

View reviewed changes

test/units/testsuite-62.sh Show resolved Hide resolved

mauriciovasquezbernal added 3 commits August 19, 2021 07:25

tests: add integration test for RestrictNetworkInterfaces=

00d6fce

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

systemctl: show RestrictNetworkInterfaces= in systemctl show

7e959a7

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

README: add requirements for RestrictNetworkInterfaces=

4368984

Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>

mauriciovasquezbernal force-pushed the mauricio/restrict-network-interfaces branch from 294f3ef to 4368984 Compare August 19, 2021 12:25

poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Aug 19, 2021

poettering merged commit dc13195 into systemd:main Aug 20, 2021

networkException mentioned this pull request Dec 15, 2023

core: allow interface altnames in RestrictNetworkInterfaces= #30489

Merged

Uh oh!

Add RestrictNetworkInterfaces= #18385

Add RestrictNetworkInterfaces= #18385

Uh oh!

Conversation

mauriciovasquezbernal commented Jan 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented Feb 19, 2021

Uh oh!

lgtm-com bot commented Feb 22, 2021

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lgtm-com bot commented Feb 25, 2021

Uh oh!

lgtm-com bot commented Feb 25, 2021

Uh oh!

lgtm-com bot commented Feb 26, 2021

Uh oh!

lgtm-com bot commented Feb 26, 2021

Uh oh!

poettering commented May 10, 2021

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

poettering commented Aug 18, 2021

Uh oh!

mauriciovasquezbernal commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

poettering commented Aug 19, 2021

Uh oh!

jameshilliard commented Aug 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

mauriciovasquezbernal commented Jan 26, 2021 •

edited

Loading

mauriciovasquezbernal commented Aug 18, 2021 •

edited

Loading