nixos/incus: add AppArmor rules #386060
nixos/incus: add AppArmor rules #386060
Conversation
github-actions
bot
added
6.topic: nixos
github-actions
bot
added
10.rebuild-darwin: 0
|
Right now this ruleset is hand picked, since I err on the safe side to avoid broad rules, though there are some very broad rules defined in |
|
I'd love to get this support added, but we'll want a nixos test to validate it. Both for this PR and to ensure it continues to work. Are you willing to look into adding one? This test suite is a not trivial, but hopefully the patterns are clear enough. If not, feel free to ask here or on matrix #lxc:nixos.org |
MakiseKurisu
requested review from
adamcstephens
and removed request for
adamcstephens
|
|
When running nixpkgs-review against this pr and building nixosTests.incus this error happens: (please rebase against master and then fix the error) |
- security = {
- apparmor.enable = appArmor;
- dbus.apparmor = lib.optionalString appArmor "enabled";
- };
+ security.apparmor.enable = appArmor;
+
+ services.dbus.apparmor = lib.optionalString appArmor "enabled";
But that's not enough to get things working. The tests fail on dnsmasq getting blocked. |
| appArmor = incusTest { | ||
| inherit lts pkgs system; | ||
| appArmor = true; | ||
| instanceContainer = true; |
There was a problem hiding this comment.
Let's reduce the scope a bit more and disable legacy init for now. initLegacy = false; will also make the tests quicker by reducing the number of nixos instances.
nixos/tests/incus/incus-tests.nix
Outdated
| @@ -4,6 +4,7 @@ import ../make-test-python.nix ( | |||
| lib, | |||
|
|
|||
| lts ? true, | |||
| appArmor ? false, | |||
There was a problem hiding this comment.
Since you'll be in here anyway, please put this with the other test variants in alphabetical order. Thankd
There was a problem hiding this comment.
The idea is that AA is more like a general setting like LTS, and not an individual test itself. So we might want to check more combinations later on.
I'll still change this.
There was a problem hiding this comment.
Once we iron out the rules for all the incus things, I hope to turn apparmor on by default for the tests.
This is because I'm only trying to fix instance creation initially, which just need to handle
|
|
Yeah, I assumed you were just bridging. That's my typical use as well, but using the built in DHCP server is really the default setup and what the tests are using. I tried to include the dnsmasq profile but that didn't seem to work. I'm admittedly pretty inexperienced with apparmor though. I agree we should keep this maintainable. In the end, what's the real risk of letting processes read shared libraries out of the store? |
|
I think it's more about removing the potential attack vector and the principle of minimal permission. Nix store is world readable which is why you should not put clear secret there, but well misconfiguration happens, and that's when a strict rule can make a difference. If you use AA you probably care about those things. Though a correct implementation will probably need to parse the selected Incus derivation, find the underlying binaries (tar, xz, dnsmasq, etc.), and then generate a list of used so's. That feels way too complex, so I'm doing some broad rules now. However, the "container CPU limits can be hotplug changed" failed, and I don't see any obvious AA denies ( |
MakiseKurisu
changed the title
|
you may want to take a look at closure-info.nix: https://github.com/NixOS/nixpkgs/blob/9ba655c111d44efe1f127f7aabd12148842988f5/pkgs/build-support/closure-info.nix this may help with getting the binaries incus uses |
If you run the normal container tests does this pass? |
|
Seems so: |
|
Well I ran AA test again and it passed that specific sub test, but now failed on a different one: |
|
I think this is probably something on my computer though. When I reran normal container test, it failed at the same sub test. |
|
Just did another run when I booted up the computer, and it clears for all tests (there is So I can only think something on my computer is causing the issue. I am using a virtual machine as my primary PC so might be some nested virtualization qwirks. Probably worth for reviewing again? |
|
I wanna test this pull request. How would I add the changes this PR makes into my NixOS configuration? I'm using the 25.05 channel. No flakes. Is this correct? nixpkgs.overlays = [
(self: super: {
incus = super.incus.overrideAttrs (old: {
patches = (old.patches or []) ++ [
(super.fetchpatch {
url = "https://github.com/NixOS/nixpkgs/pull/386060.patch";
sha256 = "xxxx";
})
];
});
})
]; |
No, that would apply this PR, a nixpkgs patch, to the incus package -- and incus and nixpkgs are two entirely different code bases. I think there was a proposal for a NixOS module that would apply nixpkgs patches onto itself, but I cannot find anything now. Anyway, I think for one-off testing the easiest is to clone nixpkgs and checkout this PR, then build your NixOS with |
|
Channel can serve a similar purpose as flake input, but it is 100% easier to use flake, and I usually mention how I dogfood my PR in my own nixos-config in the PR message (check my past PRs), but this was quite trivial so I didn't do it this time. The easiest for you though is to just copy the extra AA rules in If you actually want to add PR as an input without flake, it should be something like this:
disabledModules = [ "virtualisation/incus.nix" ];
imports = [
<incusd/nixos/modules/virtualisation/incus.nix>
]; |
|
@MakiseKurisu are you interested in completing the apparmor support? I was curious so ran this against the full test suite, but the VM support isn't currently working either. If not, we can get this merged, but it would be great to work out the remaining blockers so we can enable apparmor by default in the tests. |
|
I can check this tomorrow to see how to handle this. Just reinstalled my main system today. |
Currently limit the test scope to instanceContainer since there is a known issue. allTests might cause too many false positives. initLegacy is also disabled to reduce test size.
|
Update rule now clears |
Add the necessary AppArmor rules when
incusdtries to extract an image to create a new instance.Fix #350012
Details can be found here.
CC @NixOS/lxc @umbra @broizter since I can't select them as reviewers.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.