apparmor: try again to fix and improve #101071

ju1m · 2020-10-19T13:19:06Z

This PR is ready to be merged. It re-introduces work done in #93457, hopefully without the import-from-derivation which was:

causing hydra-eval-jobs to crash because of Fix unhelpful error messages in aggregate jobs. hydra#825
and requiring the option allow-import-from-derivation to be set.

Old and wrong analysis believing apparmorRulesFromClosure is an IFD

Please, do not merge this PR before NixOS/hydra#825 has been merged. It maintains a mergeable version of PR #93457, previously reverted due to a ~~(so far not understood)~~ problem raised by the hydra evaluation.
~~It may or may not be caused by allow-import-from-derivation = false. More investigation is needed.~~
This problem is due to the following:

To simplify the generation of AppArmor profiles, this PR introduces and uses pkgs.apparmorRulesFromClosure, which leverages IFD (import-from-derivation) via pkgs.closureInfo.
However, requiring IFD means that hydra-eval-jobs now needs to perform builds simply to do its task of listing Hydra jobs in a big JSON object for hydra-eval-jobset. For that reason or another, @vcunat informed us that: "import-from-derivation is better avoided in the official nixpkgs repo (not sure if there's some rule on that) and it's intentionally disabled on Hydra.nixos.org", causing all NixOS tests depending on packages or services using pkgs.apparmorRulesFromClosure to fail on hydra.nixos.org.
hydra-eval-jobs is used on hydra.nixos.org to eval nixos/release-combined.nix where an aggregate optimization is used, but if one of the constituents of this aggregation fails then the overall jobset generation (in JSON) fails with:

$ NIX_PATH= hydra-eval-jobs -I . nixos/release-combined.nix --option allow-import-from-derivation false --verbose
[…]
error: [json.exception.type_error.302] type must be string, but is null

The good news is that this bug is being fixed by @samueldr in NixOS/hydra#825, such that hydra-eval-jobs now ignores failing aggregated jobs instead of crashing:

$ NIX_PATH= hydra-eval-jobs -I . nixos/release-combined.nix --option allow-import-from-derivation false --verbose
[…]
  "nixos.tests.transmission.x86_64-linux": {                                                                                                                                                                                                                   
    "error": "error: --- EvalError --- hydra-eval-jobs\nattempted to realize '/nix/store/f46p5hlwks57rmxdpm0jkixlq1mf5dw4-apparmor-utils-2.13.5.drv' during evaluation but 'allow-import-from-derivation' is false"                                            
  },
[…]

In that example I've added nixos.tests.transmission amongst aggregated jobs in nixos/release-combined.nix, commenting all others in nixos/release-combined.nix to get a quick run (because running on all jobs takes… ~5 hours on my computer). By lack of time and computer power, I was unable to bisect which aggregated tests were failing, but I was able to reproduce that crashing with all tests enabled, and no more crashing with NixOS/hydra#825.

The bad news is that adding AppArmor profiles using pkgs.apparmorRulesFromClosure will potentially disable tests on hydra.nixos.org down the road, maybe a lot of them eventually.
If those cases are considered important enough to not be disabled, one could:

Use another Hydra instance where IFD is enabled, and maybe write a nixos/release-ifd.nix aggregating only those important tests.
Not use pkgs.apparmorRulesFromClosure on the concerned services/packages, by listing manually all the dependencies, though this could be very hard to maintain for many services/packages.
Write less secure AppArmor rules globbing over the whole /nix/store, instead of limiting them to the packages of a closure.

Motivation for this change

See PR #93457.

Things done

Revert commit 420f89c
Fix logprof.conf generation by removing the import-from-derivation (IFD) caused by builtins.readFile on the apparmor-utils derivation.
Update to latest major AppArmor release : 3.0.1.
Add an optional name attribute to apparmorRulesFromClosure to make listing /nix/store/ more human readable, so that there is some context rather than hundreds of paths designated "apparmor-closure-rules".
Fix aa-unconfined not finding netstat nor ss at runtime because it overwrites PATH.
Move bin.transmission-daemon profile to Nixpkgs, and customize it in NixOS with local/bin.transmission-daemon.
Add nixos/tests/apparmor.nix, testing AppArmor is enabled in the kernel and apparmorRulesFromClosure works.
Disable security.apparmor.killUnconfinedConfinables by default, but enable it in nixos/modules/profiles/hardened.nix.

bqv · 2020-10-24T16:24:09Z

Bump?

ju1m · 2020-10-25T04:00:10Z

Thanks @bqv, I've rebased to merge with iputils changes in 0a55c5d

bqv · 2020-11-21T07:00:34Z

@ju1m new conflicts

Also, is there any chance you'll be able to resolve the hydra issue soon?

ju1m · 2020-11-26T04:09:20Z

Rebased after {utillinux => util-linux} rename in #104776 .

ju1m · 2020-11-29T07:11:25Z

Also, is there any chance you'll be able to resolve the hydra issue soon?

Good news everyone! Turns out @samueldr has been tackling this hydra-eval-jobs issue in NixOS/hydra#825. I've updated the intro with all the details.

bqv · 2020-11-29T10:51:21Z

I imagine some semblance of option 1 is the most reasonable and realistic

pkgs/os-specific/linux/apparmor/default.nix

pkgs/os-specific/linux/iputils/default.nix

pkgs/os-specific/linux/apparmor/default.nix

ju1m · 2021-03-26T06:14:06Z

Well, is there anything remaining to be discussed, done or tested to get this PR merged? Please tell me if I've missed something.
@andir, have I addressed your concerns well enough?

mkg20001 · 2021-04-18T22:37:18Z

I'm not sure, can we just merge this already? I'd be in favor of just merging this once conflicts are resolved, as I don't see any objections. I'd do that once conflicts are fixed.

necessarily-equal

Fancy! Note I didn't take time to test this yet

necessarily-equal · 2021-04-20T12:51:55Z

nixos/modules/profiles/hardened.nix

@@ -36,6 +36,7 @@ with lib;
  security.virtualisation.flushL1DataCache = mkDefault "always";

  security.apparmor.enable = mkDefault true;
+  security.apparmor.killUnconfinedConfinables = mkDefault true;


Doesn't the doc you added say this default to false?

Well, it does not appear to contradict the doc to me, though I've tried to clarify a bit, but tell me if I've missed somthing or if that's not clear enough. See also #101071 (comment) where I explain why I proposed to still set killUnconfinedConfinables in the hardened.nix profile.

nixos/modules/security/apparmor.nix

This reverts commit 420f89c.

First because IFD (import-from-derivation) is not allowed on hydra.nixos.org, and second because without NixOS/hydra#825 hydra-eval-jobs crashes instead of skipping aggregated jobs which fail (here because they required an IFD).

ju1m · 2021-04-23T05:30:09Z

I've rebased against latest staging branch which only had minor conflicts.

aaronjanse · 2021-04-24T09:35:24Z

Thank you @ju1m! I'm very excited for this to be merged

7c6f434c · 2021-04-24T10:30:49Z

@ofborg test apparmor

vojta001 · 2021-05-11T07:58:16Z

What is the state of this PR? I see it is merged into staging, but not yet to master. Will it make it into 21-05 release?

Flakebi · 2021-05-11T09:02:25Z

It is part of the current staging-next pr #122068, which should be merged before the 21.05 branch-off. So, unless anything unexpected happens, it will be part of 21.05. Release schedule

vojta001 · 2021-05-11T09:16:27Z

Ahhhh, great! Thanks for clarification. I am still learning the nixpkgs release process.

vcunat · 2021-05-11T12:40:48Z

We're all just learning it, as this approach to release process is new and being used for the first time :-)

vcunat · 2021-05-14T13:45:46Z

This PR uses install with /dev/stdin on a few places, and apparently that will break build on darwin:

install: skipping file '/dev/stdin', as it was replaced while being copied

BTW, there's no point in setting permissions there (except for the executable bit), as all nix store contents will be normalized anyway.

ju1m · 2021-05-14T15:46:24Z

@vcunat see #123005 for a fix.

ofborg bot added 6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) labels Oct 19, 2020

ofborg bot requested review from matthewbauer, Phreedom, thoughtpolice, joachifm, primeos and lheckemann October 19, 2020 13:43

ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ 10.rebuild-linux: 501-1000 labels Oct 19, 2020

ju1m force-pushed the apparmor branch from 4ffc1e7 to 30a414d Compare October 25, 2020 03:22

ju1m mentioned this pull request Oct 27, 2020

lib: add mkEnabledOption #94983

Closed

11 tasks

ajs124 mentioned this pull request Nov 2, 2020

dbus: Add AppArmor support #102537

Merged

10 tasks

ju1m force-pushed the apparmor branch 2 times, most recently from adb7d15 to e82d9c7 Compare November 26, 2020 04:01

bqv reviewed Dec 1, 2020

View reviewed changes

pkgs/os-specific/linux/apparmor/default.nix Outdated Show resolved Hide resolved

bqv reviewed Dec 1, 2020

View reviewed changes

pkgs/os-specific/linux/iputils/default.nix Outdated Show resolved Hide resolved

bqv reviewed Dec 1, 2020

View reviewed changes

pkgs/os-specific/linux/apparmor/default.nix Show resolved Hide resolved

ju1m force-pushed the apparmor branch 3 times, most recently from 41205f3 to 3fb7db4 Compare December 1, 2020 16:36

ofborg bot removed the 2.status: merge conflict label Mar 5, 2021

SuperSandro2000 added the 2.status: merge conflict label Apr 19, 2021

necessarily-equal suggested changes Apr 20, 2021

View reviewed changes

ju1m force-pushed the apparmor branch from 63dfb37 to d061a07 Compare April 23, 2021 05:12

ju1m and others added 10 commits April 23, 2021 07:17

Revert "Revert "apparmor: fix and improve the service""

05d334c

This reverts commit 420f89c.

nixos/apparmor: remove an IFD

fca06b1

First because IFD (import-from-derivation) is not allowed on hydra.nixos.org, and second because without NixOS/hydra#825 hydra-eval-jobs crashes instead of skipping aggregated jobs which fail (here because they required an IFD).

nixos/apparmor: fix logprof.conf generation

27032f4

apparmor: 2.13.5 -> 3.0.0

8f9b29d

apparmor: 3.0.0 -> 3.0.1

7996dfb

nixos/apparmor: move release note to 21.05

03b2156

transmission: move apparmor profile to Nixpkgs

b280e64

nixos/apparmor: improve code readability

45e5d72

nixos/apparmor: add test for apparmorRulesFromClosure

76887d7

nixos/apparmor: disable killUnconfinedConfinables by default

b42a0e2

ju1m force-pushed the apparmor branch from d061a07 to b42a0e2 Compare April 23, 2021 05:20

ofborg bot removed the 2.status: merge conflict label Apr 23, 2021

7c6f434c merged commit d04f1c4 into NixOS:staging Apr 24, 2021

ju1m mentioned this pull request May 14, 2021

apparmor: fix profile building on darwin #123005

Merged

11 tasks

ju1m mentioned this pull request Feb 28, 2022

logrotate: move mail dependency from package to service #162225

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apparmor: try again to fix and improve #101071

apparmor: try again to fix and improve #101071

ju1m commented Oct 19, 2020 •

edited

bqv commented Oct 24, 2020

ju1m commented Oct 25, 2020

bqv commented Nov 21, 2020

ju1m commented Nov 26, 2020

ju1m commented Nov 29, 2020

bqv commented Nov 29, 2020

ju1m commented Mar 26, 2021

mkg20001 commented Apr 18, 2021

necessarily-equal left a comment

necessarily-equal Apr 20, 2021

ju1m Apr 23, 2021

ju1m commented Apr 23, 2021

aaronjanse commented Apr 24, 2021

7c6f434c commented Apr 24, 2021

vojta001 commented May 11, 2021

Flakebi commented May 11, 2021

vojta001 commented May 11, 2021

vcunat commented May 11, 2021

vcunat commented May 14, 2021

ju1m commented May 14, 2021

apparmor: try again to fix and improve #101071

apparmor: try again to fix and improve #101071

Conversation

ju1m commented Oct 19, 2020 • edited

Motivation for this change

Things done

bqv commented Oct 24, 2020

ju1m commented Oct 25, 2020

bqv commented Nov 21, 2020

ju1m commented Nov 26, 2020

ju1m commented Nov 29, 2020

bqv commented Nov 29, 2020

ju1m commented Mar 26, 2021

mkg20001 commented Apr 18, 2021

necessarily-equal left a comment

Choose a reason for hiding this comment

necessarily-equal Apr 20, 2021

Choose a reason for hiding this comment

ju1m Apr 23, 2021

Choose a reason for hiding this comment

ju1m commented Apr 23, 2021

aaronjanse commented Apr 24, 2021

7c6f434c commented Apr 24, 2021

vojta001 commented May 11, 2021

Flakebi commented May 11, 2021

vojta001 commented May 11, 2021

vcunat commented May 11, 2021

vcunat commented May 14, 2021

ju1m commented May 14, 2021

ju1m commented Oct 19, 2020 •

edited