Navigation Menu

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

Fixed apparmor module issue #26463 #82659

Closed
wants to merge 1 commit into from
Closed

Conversation

Qubasa
Copy link
Contributor

@Qubasa Qubasa commented Mar 15, 2020

Motivation for this change

Making apparmor useful in NixOS by allowing apparmor-utils to work properly.
Fixing bug #26463

Things done

The apparmor utils expect some configuration files to be present in /etc/apparmor and /etc/apparmor.d
Additionally all paths beginnging with /usr in severity.db and logprof.conf have benn replaced by
/nix/store/* to make them viable under NixOS.
Two more options have been added mainly:

  • extraParserConfig
    Append configuration lines to /etc/apparmor/parser.conf
  • extraLogConfig
    Append configuration lines to /etc/apparmor/logprof.conf
  • 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.


# Pretty much everything in here, is there to
# prevent apparmor utilities from crashing.
environment.etc."apparmor/logprof.conf".text =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does these configuration comes from? Could we directly take it from the package instead of copying it to here?

Copy link
Contributor Author

@Qubasa Qubasa Mar 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well the apparmor and apparmor-profiles package does not provide these files. I have them from here https://gitlab.com/apparmor/apparmor/-/tree/master/utils%2Ftest should we create a package only for these files? Seems a bit overkill to me.

"/usr"
]
[
"/nix/store/*"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not yet sure about the implications of this.
In NixOS not just root can install stuff to the /nix/store...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These configuration files do not affect the behaviour of apparmor itself and only affect the apparmor-utils. They use the [globs] in parser.conf to add asterisks where needed when they generate a rule for you through aa-logprof.
The severity.db will be used by aa-logprof to grade the severity of binaries and display them to the user.

${cfg.extraLogConfig}
'';

environment.etc."/apparmor/severity.db".text =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, can we take this from apparmor directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but no but yes, see above ^^

Comment on lines +603 to +612
# Enable logging to /var/log/messages so that
# aa-logprof can parse the text logfiles
services.journald.forwardToSyslog = true;
services.rsyslogd = {
enable = true;
extraConfig = ''
$ModLoad imklog
kern.* -/var/log/messages
'';
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be preferable to use the Audit framework?

And what if audit(d) is already enabled, and the user explicitly enables Apparmor? Won't this change break existing setups?

@bqv
Copy link
Contributor

bqv commented Aug 5, 2020

Superceded by #93457

@Qubasa Qubasa closed this Aug 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants