Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apparmor: try again to fix and improve #101071

Merged
merged 10 commits into from Apr 24, 2021
Merged

apparmor: try again to fix and improve #101071

merged 10 commits into from Apr 24, 2021

Conversation

@ju1m
Copy link
Contributor

@ju1m ju1m commented Oct 19, 2020

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

  1. causing hydra-eval-jobs to crash because of NixOS/hydra#825
  2. 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:

  1. To simplify the generation of AppArmor profiles, this PR introduces and uses pkgs.apparmorRulesFromClosure, which leverages IFD (import-from-derivation) via pkgs.closureInfo.
  2. 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.
  3. 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:

  1. Use another Hydra instance where IFD is enabled, and maybe write a nixos/release-ifd.nix aggregating only those important tests.
  2. 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.
  3. 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.
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@bqv
Copy link
Contributor

@bqv bqv commented Oct 24, 2020

Bump?

@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Oct 25, 2020

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

@ju1m ju1m mentioned this pull request Oct 27, 2020
11 tasks
@ajs124 ajs124 mentioned this pull request Nov 2, 2020
10 tasks
@bqv
Copy link
Contributor

@bqv bqv commented Nov 21, 2020

@ju1m new conflicts

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

@ju1m ju1m force-pushed the apparmor branch 2 times, most recently from adb7d15 to e82d9c7 Nov 26, 2020
@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Nov 26, 2020

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

@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Nov 29, 2020

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
Copy link
Contributor

@bqv bqv commented Nov 29, 2020

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

@ju1m ju1m force-pushed the apparmor branch 3 times, most recently from 41205f3 to 3fb7db4 Dec 1, 2020
@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Mar 5, 2021

Rebased after merge conflict in nixos/modules/virtualisation/lxd.nix due to changes in 4adcb00

@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Mar 26, 2021

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
Copy link
Member

@mkg20001 mkg20001 commented Apr 18, 2021

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.

Copy link
Contributor

@necessarily-equal necessarily-equal left a comment

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

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

security.apparmor.enable = mkDefault true;
security.apparmor.killUnconfinedConfinables = mkDefault true;
Copy link
Contributor

@necessarily-equal necessarily-equal Apr 20, 2021

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

Copy link
Contributor Author

@ju1m ju1m Apr 23, 2021

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 Outdated Show resolved Hide resolved
@ju1m
Copy link
Contributor Author

@ju1m ju1m commented Apr 23, 2021

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

@aaronjanse
Copy link
Member

@aaronjanse aaronjanse commented Apr 24, 2021

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

@7c6f434c
Copy link
Member

@7c6f434c 7c6f434c commented Apr 24, 2021

@ofborg test apparmor

@7c6f434c 7c6f434c merged commit d04f1c4 into NixOS:staging Apr 24, 2021
22 of 23 checks passed
@vojta001
Copy link
Contributor

@vojta001 vojta001 commented May 11, 2021

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
Copy link
Member

@Flakebi Flakebi commented May 11, 2021

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
Copy link
Contributor

@vojta001 vojta001 commented May 11, 2021

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

@vcunat
Copy link
Member

@vcunat vcunat commented May 11, 2021

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

@vcunat
Copy link
Member

@vcunat vcunat commented May 14, 2021

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
Copy link
Contributor Author

@ju1m ju1m commented May 14, 2021

@vcunat see #123005 for a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment