Improve `security.audit{,d}` #429553

nikstur · 2025-07-30T08:30:00Z

The audit module behaved very weirdly. Instead of being guarded behind an mkIf it would start a "disable" script if the service wasn't enabled. This would thus always pull in the service making pkgs.audit (the bin output) a mandatory part of every NixOS system closure (the lib output has always been because of systemd).
@arianvp also reported that this sometimes led to race conditions with journald which also tries to communicate with the audit subsystem. This PR fixes this behaviour and along the way gets rid of unnecessary (mandatory) bash script.

The audit and auditd module are aligned with upstream and some weird options are removed. For example ConditionSecurity = [ "audit" ]; which doesn't make sense to set because audit-rules is supposed to enable the subsystem in the first place. The only reason this has ever worked in the past is because of some unfortunate misbehaviour of journald (which would enable the audit subsystem itself).

To summarize: security.audit is now responsible for loading the audit rules and enabling the audit subsystem. security.auditd is only responsible for starting auditd.

Closes #11864

This isssue can now be closed from my perspective. Shipping default rules might be possible but can be tracked separately.

Things done

Add a 👍 reaction to pull requests you find important.

LordGrimmauld · 2025-07-30T08:49:03Z

fyi: i do have some changes to the auditd module in #420043. That said, those changes are not directly touching the service, so while a merge conflict will happen, the changes themselves seem orthogonal.

LordGrimmauld · 2025-07-30T08:50:42Z

Also: Audit is already part of every nixos closure, simply from being a dependency of systemd. These module changes don't actually change that fact. Its good to try and do some cleanup, but some of the motivation seems invalid to me.

nikstur · 2025-07-30T09:26:45Z

Also: Audit is already part of every nixos closure, simply from being a dependency of systemd. These module changes don't actually change that fact. Its good to try and do some cleanup, but some of the motivation seems invalid to me.

The lib output is but not the bin output. These modules pull in the bin output.

LordGrimmauld · 2025-07-30T11:49:58Z

I have now picked this onto my auditd testsuite branch at https://github.com/LordGrimmauld/nixpkgs/commits/audit-testing-full/ and will be building the nixos test. Compiling the packages will take a bit, but the upstream testsuite is quite exhaustive. I just recently adopted audit and am still in the process of cleaning it all up. Your help is appreciated, but this also means a lot of my changes are not yet merged (like #420043 and #429438) or even submitted for review (such as the audit-testsuite things in my branch).

LordGrimmauld · 2025-07-30T14:20:38Z

So uh, something is very wrong.
audit-test.log

The relevant errors (as far as i can see):

vm-test-run-auditd> machine # [    3.917831] auditd[389]: Unable to set pidfile (No such file or directory)
vm-test-run-auditd> machine # [    3.940292] audisp-af_unix[398]: Couldn't open /var/run (No such file or directory)
vm-test-run-auditd> machine # [    3.948696] auditd[389]: plugin /nix/store/iv9g3v9fih8f02m3hiys1alw9jk9llva-audit-4.1.0-bin/bin/audisp-af_unix terminated unexpectedly
...
vm-test-run-auditd> machine # [    4.369361] auditd[389]: plugin /nix/store/iv9g3v9fih8f02m3hiys1alw9jk9llva-audit-4.1.0-bin/bin/audisp-af_unix terminated unexpectedly
vm-test-run-auditd> machine # [    4.371648] auditd[389]: plugin /nix/store/iv9g3v9fih8f02m3hiys1alw9jk9llva-audit-4.1.0-bin/bin/audisp-af_unix has exceeded max_restarts

So it seems auditd starts too early, and this is indeed a regression. Please fix.

nixos/modules/security/auditd.nix

nikstur · 2025-07-30T14:32:31Z

I just recently adopted audit

Very cool! Glad that someone is taking care of it.

I mostly care about being able to disable this to not rely on bash (i.e. this commit: 19d79d2b918969c4832276a5969464e01fb2e105) as part of the Bashless Activation initiative.

Although I think all the changes make a lot of sense and are easy wins, I'd be willing to reduce the scope of this PR to only 19d79d2b918969c4832276a5969464e01fb2e105 if that helps you.

LordGrimmauld · 2025-07-30T14:34:44Z

Thanks! With the most recent push to https://github.com/LordGrimmauld/nixpkgs/commits/audit-testing-full/, the testsuite now passes. And fair enough, you couldn't have known about the additional testsuite work i was doing.

The test suite complains about vm-test-run-auditd> machine # [ 36.458156] auditd[541]: Error receiving audit netlink packet (No buffer space available), but i'll have to cross-reference that with the old behavior. And seeing as the testsuite passes, that might well be an expected fail.

I'll take a look at the overall nix changes, now that the CI and unofficial tests pass.

LordGrimmauld

Couple thoughts, mostly nits and a request for more testing. Overall looks good though, and the cleanup is long overdue! The last PR touching audit module significantly was back in 2016...

nixos/tests/audit.nix

nixos/modules/security/audit.nix

LordGrimmauld · 2025-07-30T14:53:41Z

Although I think all the changes make a lot of sense and are easy wins, I'd be willing to reduce the scope of this PR to only 19d79d2 if that helps you.

Please don't, your changes are good! This is for the better, just needs some care to not unknowingly break things, now that someone (me) actually cares.

Align with upstream and also remove unnecessary dependency on bash along the way.

Remove config that doesn't make senes at all or on NixOS specifically.

LordGrimmauld

I am almost happy with this, this is definitely moving in the right direction!

nixos/tests/audit.nix

LordGrimmauld

Thank you for this work, no complaints from me left

nikstur · 2025-07-30T16:20:18Z

Thank you for this work, no complaints from me left

Thank you for the review. Learned a little more about audit on the way!

arianvp · 2025-07-30T16:41:16Z

One more thing that we didn't do in this PR due to mass rebuild but we do require to solve more race conditions :

We should move the service to use Type=forking. This requires a patch to the audit package as currently it's trying to create its PIDFile in $out/var/run/audit.pid which obviously doesn't work.

Reason why we need Type=forking is that audit uses fork() to notify when the daemon is ready for traffic. And things that order After=auditd.service will only get started once auditd is ready to process things. with Type=simple things with After=auditd.service will start too soon.

Otherwise things like https://github.com/systemd/systemd/blob/922885e0a5b729a3faaba35676b37013f323d309/units/systemd-update-utmp.service.in#L16 are racey.

LordGrimmauld · 2025-07-30T16:46:58Z

I imagine that too would fall under respecting $RUNTIME_DIRECTORY, also see linux-audit/audit-userspace#490

nikstur · 2025-07-30T17:42:21Z

We should probably set --runstatedir=/run in the configureFlags of the audit package. This is what archlinux does:
https://gitlab.archlinux.org/archlinux/packaging/packages/audit/-/blob/main/PKGBUILD?ref_type=heads#L73

LordGrimmauld · 2025-07-30T17:45:18Z

I don't see how that'd fix all the hardcoded /var/run paths though

arianvp · 2025-07-31T09:14:32Z

nixos/modules/security/auditd.nix

  options.security.auditd.enable = lib.mkEnableOption "the Linux Audit daemon";

  config = lib.mkIf config.security.auditd.enable {
-    boot.kernelParams = [ "audit=1" ];


This might need a release note? Not sure. I am a little bit out of the loop what the kernel param does. I assume it causes the audit subsystem to start before userspace even starts up. Does it buffer the events?

from the auditd docs they do say:

A boot param of audit=1 should be added to ensure that all processes that run before the audit daemon starts is marked as auditable by the kernel. Not doing that will make a few processes impossible to properly audit.

i'll try to look into this soon. Ideally there'd be a VM test to actually test this stuff and get immediate results. My approach to looking into this will be trying to break it (i.e. try to audit pid 1), and if that breaks see whether adding that kernel parameter fixes it.

We should just add the kernelParam back. But to the audit module and not the auditd module!.

huh? No that makes little sense. auditd is there for the userspace auditing, our audit module is only there for rules. And you can enable audit(d) without adding any rules.

iirc you can attach audit to existing processes using auditctl later, but only if those processes were started either after auditd or with the kernel flag. Which would mean that is entirely independent of our rule-loading module.
I might be mistaken, i have some reading to do to get this right.

our audit module is only there for rules. And you can enable audit(d) without adding any rules.

No, our audit module enables the audit subsystem in the kernel right now (via the -e rule). This is what our test shows. Enabling just the audit daemon does not enable the audit subsystem (because of the -s nochange, see https://man.archlinux.org/man/auditd.8.en#s=).

IMO the audit module is there to (a) enable the audit subsystem and (b) load the rules. You can then use whatever audit daemon you like (go-audit, auditd, etc.). That's why I also think this module should set the kernelParam.

The auditd module needs the audit module (to enable the audit subsystem) to receive any audit logs from the kernel.

We shouldn't couple enabling the audit subsystem direclty to the daemon used.

Anyways, let's finish this discussion here and take it elsewhere.

LordGrimmauld · 2025-08-01T07:15:26Z

Update: since linux-audit/audit-userspace@4ade146, upstream now officially tries to respect --runstatedir, so we should probably start using that. We could fetch that patch, or just wait ~a month for the next patch release

arianvp · 2025-08-01T07:56:26Z

Great stuff!

nikstur requested review from LordGrimmauld and arianvp July 30, 2025 08:30

nikstur changed the title ~~Fix audit~~ Improve security.audit{,d} Jul 30, 2025

nikstur mentioned this pull request Jul 28, 2025

Bashless Activation - Tracking Issue #428908

Closed

nikstur force-pushed the fix-audit branch from f2dc1b6 to 3c76a58 Compare July 30, 2025 11:23

LordGrimmauld reviewed Jul 30, 2025

View reviewed changes

nixos/modules/security/auditd.nix Outdated Show resolved Hide resolved

nikstur force-pushed the fix-audit branch from 3c76a58 to 86e967d Compare July 30, 2025 14:26

LordGrimmauld requested changes Jul 30, 2025

View reviewed changes

nikstur added 2 commits July 30, 2025 17:26

nixos/audit: add proper enable flag

774f6ed

Align with upstream and also remove unnecessary dependency on bash along the way.

nixos/auditd: align with upstream

50243c5

Remove config that doesn't make senes at all or on NixOS specifically.

LordGrimmauld mentioned this pull request Jul 30, 2025

can't set /run directory: neither $RUNTIME_DIRECTORY or --runstatedir are respected linux-audit/audit-userspace#490

Closed

nikstur force-pushed the fix-audit branch from 86e967d to 5b869c8 Compare July 30, 2025 15:44

nikstur requested a review from LordGrimmauld July 30, 2025 15:55

LordGrimmauld requested changes Jul 30, 2025

View reviewed changes

nixos/tests/audit.nix Outdated Show resolved Hide resolved

nixos/tests/audit.nix Outdated Show resolved Hide resolved

nikstur force-pushed the fix-audit branch 2 times, most recently from 2b786b8 to 8429939 Compare July 30, 2025 16:13

nixos/tests/audit: init

ddefab0

nikstur force-pushed the fix-audit branch from 8429939 to ddefab0 Compare July 30, 2025 16:15

LordGrimmauld approved these changes Jul 30, 2025

View reviewed changes

nikstur enabled auto-merge July 30, 2025 16:20

nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jul 30, 2025

nikstur disabled auto-merge July 30, 2025 17:39

nikstur merged commit 796b80f into NixOS:master Jul 30, 2025
24 of 27 checks passed

arianvp reviewed Jul 31, 2025

View reviewed changes

nikstur mentioned this pull request Aug 9, 2025

nixos/audit: fix journald test #432238

Merged

13 tasks

Uh oh!

Improve security.audit{,d} #429553

Improve security.audit{,d} #429553

Conversation

nikstur commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Things done

Uh oh!

LordGrimmauld commented Jul 30, 2025

Uh oh!

LordGrimmauld commented Jul 30, 2025

Uh oh!

nikstur commented Jul 30, 2025

Uh oh!

LordGrimmauld commented Jul 30, 2025

Uh oh!

LordGrimmauld commented Jul 30, 2025

Uh oh!

Uh oh!

nikstur commented Jul 30, 2025

Uh oh!

LordGrimmauld commented Jul 30, 2025

Uh oh!

LordGrimmauld left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LordGrimmauld commented Jul 30, 2025

Uh oh!

LordGrimmauld left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

LordGrimmauld left a comment

Choose a reason for hiding this comment

Uh oh!

nikstur commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arianvp commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LordGrimmauld commented Jul 30, 2025

Uh oh!

Uh oh!

nikstur commented Jul 30, 2025

Uh oh!

LordGrimmauld commented Jul 30, 2025

Uh oh!

arianvp Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

arianvp Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LordGrimmauld Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

nikstur Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

LordGrimmauld Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LordGrimmauld Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikstur Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

nikstur Aug 6, 2025

Choose a reason for hiding this comment

Improve `security.audit{,d}` #429553

Improve `security.audit{,d}` #429553

nikstur commented Jul 30, 2025 •

edited

Loading

nikstur commented Jul 30, 2025 •

edited

Loading

arianvp commented Jul 30, 2025 •

edited

Loading

arianvp Aug 4, 2025 •

edited

Loading

LordGrimmauld Aug 6, 2025 •

edited

Loading

LordGrimmauld Aug 6, 2025 •

edited

Loading