hostsfile: Copy the SELinux context to the temp file before overwrite

[refi64]

On SELinux-enabled systems, /etc/hosts has a different type net_conf_t than the other files in /etc, so the temporary file that overwrites it ends up with the wrong context, resulting in many system services becoming unable to access the file. To fix this, manually look up the context /etc/hosts has and copy it to the temporary file before the rename.

In order to avoid depending on libselinux on systems that don't use it, this support is gated behind the new "selinux" feature. It is installed and enabled in the Dockerfile, however, in order to ensure that it still builds.

[refi64]

hmm the CI failures seem unrelated?

[#] docker start 2bc1b0ae0db1e203e180da06a86cd383b8b76472feeead435e2cc35f216f8023
2bc1b0ae0db1e203e180da06a86cd3[83](https://github.com/tonarino/innernet/actions/runs/5361835266/jobs/9728189885?pr=273#step:4:84)b8b76472feeead435e2cc35f216f8023
 DEBUG wireguard_control::backends::kernel > get_by_name: got 1 response message(s) from netlink request
 DEBUG wireguard_control::backends::kernel > get_by_name: parsed wireguard device evilcorp with 1 peer(s)
- Creating a new CIDR from first peer.
[#] docker exec 2bc1b0ae0db1e203e180da06a[86](https://github.com/tonarino/innernet/actions/runs/5361835266/jobs/9728189885?pr=273#step:4:87)cd383b8b76472feeead435e2cc35f216f8023 innernet add-cidr evilcorp --name robots --cidr 10.66.2.0/24 --parent evilcorp --yes
Error: No such container: 2bc1b0ae0db1e203e180da06a86cd383b8b76472feeead435e2cc35f216f8023

but I don't think I have perms to retry.

[strohel]

but I don't think I have perms to retry.

Just retried them, let's see.

[mcginty]

Unfortunately I'm not an SELinux user so I can't really confirm the correctness of the logic, but the code looks clean enough.

Do you think it's worth the hassle to do this context copying compared to, say, only allowing overwriting /etc/hosts in-place if SELinux is detected?

[refi64]

Do you think it's worth the hassle to do this context copying compared to, say, only allowing overwriting /etc/hosts in-place if SELinux is detected?

IMO yes: SELinux-enabled is the default on a lot of systems, namely the RHEL variants & Fedora. Losing atomic updates on those systems in favor of potentially wiping the hosts file (learned the importance of atomic file updates the hard way before 🙃) would be pretty unfortunate 😅 .

[mcginty]

This seems reasonable to me - @strohel @bschwind any objections?

[strohel]

This looks good to me, I have just 2 considerations:

• @refi64 can you please rebase on current main? That should fix the Docker CI as a side-effect
• I guess selinux is specific to Linux, i.e. what happens if one tries to build innernet on a Mac with selinux feature enabled? Maybe we should make it target-specific, but I'm not sure how cargo features and target-specific dependencies interact. (ideally we don't have the feature at all outside linux, but I fear that's not possible)

[bschwind]

Looks good, just a small suggestion.

I also agree with @strohel we need a rebase on main which should fix CI and the existing merge conflict.

[strohel]

Adding some more notes on logging messages.

[refi64]

whoops looks like this fell off my radar, all comments should be fixed now.

[refi64]

Clippy failure seems unrelated:

error: accessing first element with `responses.get(0)`
Error:   --> netlink-request/src/lib.rs:64:19
   |
64 |             match responses.get(0) {
   |                   ^^^^^^^^^^^^^^^^ help: try: `responses.first()`

[bschwind]

@refi64 I'll take care of the clippy issues in main and then we can rebase this on top of those changes.

[bschwind]

@refi64 done, if you rebase on main the clippy step should now pass.

[refi64]

@bschwind just rebased 🤞

[bschwind]

LGTM++

I'll wait for a quick review from @strohel before merging.

[strohel]

This is looking good, but one late request @refi64: could you please document the new feature (flag) in README.md? Also stating that it is currently disabled by default. We'll then make sure we mention it in the next relase notes, pinging package maintainers to decide whether they want to activate the flag or not.

[jebotz]

This is looking good, but one late request @refi64: could you please document the new feature (flag) in README.md? Also stating that it is currently disabled by default.

Since @refi64 hasn't responded, and I'd really like to see this merged (I use the code, works fine for me) I've added the documentation; I'm including it here as a patch...

From 831d19d907e004e89dde797d9191b07700c78d77 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?J=C3=BCrgen=20Botz?= <[email protected]>
Date: Sun, 28 Apr 2024 17:02:13 +0200
Subject: [PATCH] Add info about selinux feature to README.md

---
 README.md | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/README.md b/README.md
index b6ee618..592afc1 100644
--- a/README.md
+++ b/README.md
@@ -232,6 +232,14 @@ Note that you'll be responsible for updating manually.
 - `libclang` (see more info at [https://crates.io/crates/clang-sys](https://crates.io/crates/clang-sys))
 - `libsqlite3`
 
+### Cargo build feature for SELinux
+
+If your target system uses SELinux, you will want to enable the
+'selinux' feature when building the innernet binary. This will ensure
+that innernet maintains the correct selinux context on the /etc/hosts
+file when adding hosts.  To do so add ```--features selinux``` to the
+```cargo build``` options.
+
 Build:
 
 ```sh
-- 
2.44.0

[bschwind]

Thanks, @jebotz! Thankfully @refi64 has allowed (perhaps by default) project maintainers to push to fork branches which back a PR on the main repo, so I was able to update it and incorporate your change. I'll try to get this merged once I fix the CI errors.

[refi64]

oops sorry it looks like I completely missed the last comment 😅 thanks for pushing that update!

[strohel]

LGTM++, added some REAME nits which would be nice to address, but very minor and approving already.

[jebotz]

So, how about someone merge this?