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

nixos: Disable audit by default #17916

Closed
wants to merge 4 commits into from
Closed

nixos: Disable audit by default #17916

wants to merge 4 commits into from

Conversation

dezgeg
Copy link
Contributor

@dezgeg dezgeg commented Aug 22, 2016

Because in its default enabled state it it causes a global performance
hit on all system calls (https://fedorahosted.org/fesco/ticket/1311) and
unwanted spam in dmesg, in particular when using Chromium (#13710).

To actually make audit fully disabled, use the big hammer by putting audit=0 to the kernel parameters.
That is actually needed because otherwise journald starts it anyway. For some discussion on the usefulness of that:
- https://fedorahosted.org/fesco/ticket/1311
- systemd/systemd#959
- openSUSE/systemd@64f83d3

Finally, split audit to multiple outputs - that prevents the library part of audit (used by systemd for instance) from depending on openldap, which currently brings in 180+ MB of crud
into the closure due to it depending on glibc headers and other stuff. (That one could
really be fixed by NixOS/patchelf#98, FWIW).

cc @edolstra @copumpkin

@mention-bot
Copy link

@dezgeg, thanks for your PR! By analyzing the annotation information on this pull request, we identified @copumpkin, @edolstra and @domenkozar to be potential reviewers

@edolstra
Copy link
Member

Seems fine with me, but maybe it's nicer to use the -a task,never audit rule suggested by https://fedorahosted.org/fesco/ticket/1311? That simplifies dealing with the case where we switch from a configuration where auditing is disabled to one where it's enabled, which might not be the case if audit=0.

@copumpkin
Copy link
Member

Agree with @edolstra. Also wanted to point out that although I added a module for audit, it was already enabled by default before I added it, based on kernel settings.

@dezgeg
Copy link
Contributor Author

dezgeg commented Aug 25, 2016

Yes, I think that should work, but then we depend on the whole audit package again, and it feels daft to have some package installed to fix some other's braindamage.

Hilariously enough, they actually recommend auditing from kernel completely when using containers: https://github.com/systemd/systemd/blob/dadd6ecfa5eaf842763dca545b4c04f33831789e/README#L104 :D. I wonder if that's actually relevant there and not bitrotten...

Because in its default enabled state it it causes a global performance
hit on all system calls (https://fedorahosted.org/fesco/ticket/1311) and
unwanted spam in dmesg, in particular when using Chromium
(NixOS#13710).
@dezgeg
Copy link
Contributor Author

dezgeg commented Aug 28, 2016

Ok, now we call auditctl -e 0 -a task,never if the service is not enabled.

@dezgeg dezgeg added this to the 16.09 milestone Aug 28, 2016
@FRidh
Copy link
Member

FRidh commented Aug 30, 2016

Can this be considered before the branch off? I'm not familiar with the details but shutting up chromium does make me happy.

@dezgeg
Copy link
Contributor Author

dezgeg commented Aug 31, 2016

Unless someone objects, I plan to take this in today along with #18132 to avoid extra building (as both are systemd dependencies).

@dezgeg dezgeg self-assigned this Aug 31, 2016
@dezgeg
Copy link
Contributor Author

dezgeg commented Sep 1, 2016

In staging now (to fix one conflict) which is almost build, merging to master soon as trunk-combined first evaluates successfully (there was eval error which I hopefully fixed this morning) to get a reference point.

dezgeg added a commit that referenced this pull request Sep 1, 2016
Brings in:
    - changed output order for multiple outputs:
      #14766
    - audit disabled by default
      #17916

 Conflicts:
	pkgs/development/libraries/openldap/default.nix
@dezgeg
Copy link
Contributor Author

dezgeg commented Sep 1, 2016

Merged.

@domenkozar
Copy link
Member

@dezgeg could you add a changelog entry for this?

@dezgeg
Copy link
Contributor Author

dezgeg commented Sep 30, 2016

@domenkozar it already has one: 5ad122b

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.

6 participants