networkd: NetLabel integration by topimiettinen · Pull Request #23269 · systemd/systemd

topimiettinen · 2022-05-04T17:29:49Z

New directives IPv4NetLabelUnlabel= and IPv6NetLabelUnlabel= provide a
method for integrating dynamic network configuration into Linux NetLabel
subsystem rules, used by Linux security modules (LSMs) for network access
control. The options expect a whitespace separated list of NetLabel
labels. The labels must conform to lexical restrictions of LSM labels. When
an interface is configured with IP addresses, the addresses and subnetwork
masks will be appended to the NetLabel Fallback Peer Labeling rules. They will
be removed when the interface is deconfigured. Failures to manage the labels
will be ignored.

Example:

IPv4NetLabelUnlabel=system_u:object_r:localnet_peer_t:s0

With the above rules for interface eth0, when the interface is configured with
an IPv4 address of 10.0.0.0/8, systemd-networkd performs the equivalent of
netlabelctl operation

$ sudo netlabelctl unlbl add interface eth0 address:10.0.0.0/8 label:system_u:object_r:localnet_peer_t:s0

Result:

$ sudo netlabelctl -p unlbl list
...
 interface: eth0
   address: 10.0.0.0/8
    label: "system_u:object_r:localnet_peer_t:s0"
...

topimiettinen · 2022-05-04T17:55:43Z

Information for Fallback Peer Labeling for SELinux could be helpful to the reviewers.

lgtm-com · 2022-05-04T18:13:40Z

This pull request introduces 1 alert when merging 133cf88 into b0221bb - view on LGTM.com

new alerts:

1 for Unused local variable

topimiettinen · 2022-05-05T17:37:56Z

focal-amd failure most likely unrelated.

@yuwata PTAL.

yuwata

Instead of introducing IPv4NetLabelUnlabel=and IPv6NetLabelUnlabel= in [Network] section, please add NetLabel= or so in [Address], [DHCPv4], [DHCPv6], [IPv6AcceptRA], [DHCPPrefixDelegation] sections.
Then, we can assign different labels to each address.

Also, please move the netlabel related functions to networkd-netlabel.[ch] or so, as networkd-address.c and networkd-network.c are already too big.

src/libsystemd/sd-netlink/netlink-types-genl.c

src/network/networkd-address.c

src/network/networkd-network.c

src/network/networkd-address.c

src/basic/in-addr-util.c

src/network/networkd-netlabel.c

src/network/networkd-network-gperf.gperf

src/network/networkd-network.c

src/network/networkd-network.h

src/network/networkd-netlabel.c

src/network/networkd-network.c

topimiettinen · 2022-05-15T06:16:52Z

Hmm, still fails because of memory leaks.

src/network/networkd-netlabel.c

src/network/networkd-network.c

src/network/networkd-address.c

topimiettinen · 2022-05-16T19:20:26Z

Thanks for the review, updated.

topimiettinen · 2022-05-17T15:16:37Z

All green!

New directive `NetLabel=` provides a method for integrating dynamic network configuration into Linux NetLabel subsystem rules, used by Linux security modules (LSMs) for network access control. The option expects a whitespace separated list of NetLabel labels. The labels must conform to lexical restrictions of LSM labels. When an interface is configured with IP addresses, the addresses and subnetwork masks will be appended to the NetLabel Fallback Peer Labeling rules. They will be removed when the interface is deconfigured. Failures to manage the labels will be ignored. Example: ``` [DHCP] NetLabel=system_u:object_r:localnet_peer_t:s0 ``` With the above rules for interface `eth0`, when the interface is configured with an IPv4 address of 10.0.0.0/8, `systemd-networkd` performs the equivalent of `netlabelctl` operation ``` $ sudo netlabelctl unlbl add interface eth0 address:10.0.0.0/8 label:system_u:object_r:localnet_peer_t:s0 ``` Result: ``` $ sudo netlabelctl -p unlbl list ... interface: eth0 address: 10.0.0.0/8 label: "system_u:object_r:localnet_peer_t:s0" ... ```

keszybz · 2022-06-08T17:56:41Z

Why was this merged without review?

topimiettinen · 2022-06-08T18:39:23Z

But it was reviewed by @yuwata.

evverx · 2022-06-08T21:35:52Z

FWIW to avoid scenarios like this it should be possible to somewhat enforce approvals with https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule

Optionally, to require approvals before a pull request can be merged, select Require approvals, click the Required number of approvals before merging drop-down menu, then select the number of approving reviews you would like to require on the branch.

It's probably bureaucracy but on the other hand it appears a lot of PRs have been approved like that anyway so it seems it shouldn't be too annoying.

yuwata · 2022-06-09T03:17:07Z

But it was reviewed by @yuwata.

I reviewed several times, but I have neither said 'LGTM' nor set the green label...

Sorry for late final review, but this needs to be re-reviewed. Temporary setting 'merged-but-needs-fixing'.

keszybz · 2022-06-09T07:31:51Z

man/systemd.network.xml

+          <para>This setting provides a method for integrating dynamic network configuration into Linux
+          NetLabel subsystem rules, used by Linux security modules (LSMs) for network access control. The
+          option expects a whitespace separated list of NetLabel labels. The labels must conform to lexical
+          restrictions of LSM labels. When an interface is configured with IP addresses, the addresses and
+          subnetwork masks will be appended to the NetLabel Fallback Peer Labeling rules. They will be


It'd be good to add some hyperrefences here, e.g. to kernel docs for LSM and NetLabel.

keszybz · 2022-06-09T07:32:40Z

man/systemd.network.xml

+      <varlistentry>
+        <term><varname>NetLabel=</varname></term>
+        <listitem>
+          <para>As in [Address] section.</para>


"This setting accepts the same arguments as NetLabel= in the [Address] section."

I used the same short text 'As in ...' like is used elsewhere in this manual page (MUDURL=, UseDNS=, MACAddress= etc). Wouldn't it be more consistent to use similar style also here so that it can be sort of skipped by the reader more easily?

keszybz · 2022-06-09T07:32:53Z

man/systemd.network.xml

+      <varlistentry>
+        <term><varname>NetLabel=</varname></term>
+        <listitem>
+          <para>As in [Address] section.</para>


keszybz · 2022-06-09T07:32:58Z

man/systemd.network.xml

+      <varlistentry>
+        <term><varname>NetLabel=</varname></term>
+        <listitem>
+          <para>As in [Address] section.</para>


src/network/networkd-netlabel.c

keszybz · 2022-06-09T07:34:40Z

src/network/networkd-netlabel.c

+                return 1;
+        }
+
+        log_link_debug(link, "NetLabel operation successful");


Hmm, this message is not very informative. If I saw it without context, I would have no idea what was being done and why. Can you rephrase it?

I agree it would be nice to have more verbose message explaining what exactly was successful (or what failed). But there isn't any context related to the operation available in the callback, just Link (which may have several Address) and the raw message.

In the earlier version I used synchronous sending and then the errors messages were able to be more precise.

keszybz · 2022-06-09T07:36:15Z

src/network/networkd-netlabel.c

+                /* Label semantics depend on LSM but let's do basic checks */
+                if (!string_is_safe(w)) {
+                        log_syntax(unit, LOG_WARNING, filename, line, 0,
+                                   "Bad NetLabel label, ignoring: %s", w);


If w is not safe, it shouldn't be printed. Please cescape it before pritning.

keszybz · 2022-06-09T07:37:35Z

src/network/networkd-netlabel.c

+                r = netlabel_command(NLBL_UNLABEL_C_STATICADD, label, address);
+                if (r < 0)
+                        log_link_warning_errno(address->link, r, "Adding NetLabel %s for IP address %s failed, ignoring: %m",
+                                               label, strna(addr_str));


%m is missing.

keszybz · 2022-06-09T07:39:27Z

src/network/networkd-netlabel.c

+        int r;
+        Set **set = data;


Set **set = ASSERT_PTR(data); int r;

Add links to documentation to NetLabel, LSM, Netlabel Fallback Peer Labeling and NFT. Use a positive boolean function nft_identifier_good() to help grok the checks more easily than nft_identifier_bad(). Escape logged invalid items. Add an ASSERT_PTR() for 'data' in config_parse_netlabel(). Use a different debug message when NFT operation fails. Align nfproto_table strings. Remove NFT_SET_MSGS, it's only used once.

topimiettinen · 2022-06-11T15:39:18Z

Created #23712 for the fixes.

Addresses systemd#23269 (comment).

This reverts PR systemd#23269 and its follow-up commit. Especially, 2299b1c (partially), and 3cf6383. The PR was merged without final approval, and has several issues: - The NetLabel for static addresses are not assigned, as labels are stored in the Address objects managed by Network, instead of Link. - If NetLabel is specified for a static address, then the address section will be invalid and the address will not be configured, - It should be implemented with Request object, - There is no test about the feature.

topimiettinen added the network label May 4, 2022

topimiettinen mentioned this pull request May 4, 2022

RFE/RFD: lightweight integration of NFT sets for use in firewalls #22527

Closed

topimiettinen force-pushed the networkd-netlabel branch from 133cf88 to 2aa82e9 Compare May 4, 2022 20:38

topimiettinen added ci-failure-appears-unrelated please-review labels May 5, 2022

yuwata requested changes May 6, 2022

View reviewed changes

yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks new-feature and removed please-review labels May 6, 2022

poettering reviewed May 9, 2022

View reviewed changes

src/network/networkd-address.c Outdated Show resolved Hide resolved

poettering reviewed May 9, 2022

View reviewed changes

src/network/networkd-address.c Outdated Show resolved Hide resolved

poettering reviewed May 9, 2022

View reviewed changes

src/network/networkd-address.c Outdated Show resolved Hide resolved

poettering reviewed May 9, 2022

View reviewed changes

src/network/networkd-address.c Outdated Show resolved Hide resolved

poettering reviewed May 9, 2022

View reviewed changes

src/network/networkd-address.c Outdated Show resolved Hide resolved

topimiettinen force-pushed the networkd-netlabel branch 2 times, most recently from f1e10ff to 705af57 Compare May 9, 2022 19:11

topimiettinen requested a review from yuwata May 9, 2022 20:23

yuwata added please-review and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks ci-failure-appears-unrelated labels May 10, 2022

yuwata requested changes May 10, 2022

View reviewed changes

yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review labels May 10, 2022

topimiettinen force-pushed the networkd-netlabel branch from 705af57 to f1d4ddc Compare May 10, 2022 20:02

topimiettinen added please-review and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels May 10, 2022

topimiettinen force-pushed the networkd-netlabel branch from f1d4ddc to 504f349 Compare May 10, 2022 20:10

mrc0mmand reviewed May 13, 2022

View reviewed changes

src/network/networkd-netlabel.c Outdated Show resolved Hide resolved

topimiettinen force-pushed the networkd-netlabel branch from e69bee4 to d4cb7f7 Compare May 14, 2022 06:35

mrc0mmand reviewed May 14, 2022

View reviewed changes

src/network/networkd-network.c Show resolved Hide resolved

topimiettinen force-pushed the networkd-netlabel branch from d4cb7f7 to 75f288e Compare May 14, 2022 12:08

yuwata requested changes May 15, 2022

View reviewed changes

yuwata added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label May 15, 2022

topimiettinen force-pushed the networkd-netlabel branch from 75f288e to 7a4d4cf Compare May 16, 2022 19:16

topimiettinen removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label May 16, 2022

topimiettinen mentioned this pull request May 23, 2022

core: NFT sets #22587

Merged

topimiettinen force-pushed the networkd-netlabel branch from 7a4d4cf to ad07e69 Compare June 5, 2022 08:43

topimiettinen force-pushed the networkd-netlabel branch from ad07e69 to 1c659e0 Compare June 5, 2022 17:06

topimiettinen merged commit 3cf6383 into systemd:main Jun 6, 2022

topimiettinen deleted the networkd-netlabel branch June 7, 2022 17:23

yuwata added the merged-but-needs-fixing label Jun 9, 2022

yuwata mentioned this pull request Jun 9, 2022

re-review #22587 and #23269 #23676

Closed

keszybz reviewed Jun 9, 2022

View reviewed changes

yuwata pushed a commit to yuwata/systemd that referenced this pull request Jun 17, 2022

man: add links to NetLabel and LSM

7487fc0

Addresses systemd#23269 (comment).

yuwata pushed a commit to yuwata/systemd that referenced this pull request Jun 18, 2022

man: add links to NetLabel and LSM

023381c

Addresses systemd#23269 (comment).

yuwata mentioned this pull request Jun 24, 2022

Follow-ups to #22587 and #23269 #23712

Closed

Uh oh!

Conversation

topimiettinen commented May 4, 2022

Uh oh!

topimiettinen commented May 4, 2022

Uh oh!

lgtm-com bot commented May 4, 2022

Uh oh!

topimiettinen commented May 5, 2022

Uh oh!

yuwata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

topimiettinen commented May 15, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

topimiettinen commented May 16, 2022

Uh oh!

topimiettinen commented May 17, 2022

Uh oh!

keszybz commented Jun 8, 2022

Uh oh!

topimiettinen commented Jun 8, 2022

Uh oh!

evverx commented Jun 8, 2022

Uh oh!

yuwata commented Jun 9, 2022

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment