-
-
Notifications
You must be signed in to change notification settings - Fork 17.8k
nixos/auditd: support plugins #420043
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/auditd: support plugins #420043
Conversation
|
This would be augmented by #420001, but not strictly necessary. I just believe it makes sense to adopt both, and bring both to modern standards. This is a good opportunity. |
ea6d40c to
a4bcc35
Compare
|
The upstream PR just landed |
2bd5220 to
c7b37e7
Compare
|
Ran the VM test by rebasing to master, worked perfectly fine |
|
linux-audit/audit-userspace#469 (comment)
if this isn't merged by then, i'll bump to 4.0.6 (assuming that is the next release tag) instead of fetching the patch. It might also make sense to test service restarting in the VM test, though i did so by adding that on my local branch and it "just worked", so chances are we are unaffected by that. |
nixos/modules/security/auditd.nix
Outdated
| ]) | ||
| ); | ||
| options = { | ||
| # space_left needs to be larger than admin_space_left, yet they default to be the same if left open. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe. Problem is, this does not have to be a number, it can also be e.g. "10%", and the other can be a number, making assertions extremely awkward (and fully impossible if one is % and the other is absolute).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrote an assert. It will not check the case where one is % and the other absolute, but it does check %/% and abs/abs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a gut feeling that this is one of those too complex assertions that will become a maintancen burden but since it's already written I won't ask you to undo it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe, and then i can still remove it. But the regex to match for definitions is quite strict, so there shouldn't be anything unexpected happening here.
nixos/modules/security/auditd.nix
Outdated
| security.auditd.plugins = { | ||
| af_unix = { | ||
| path = lib.getExe' pkgs.audit "audisp-af_unix"; | ||
| args = [ | ||
| "0640" | ||
| "/var/run/audispd_events" | ||
| "string" | ||
| ]; | ||
| format = "binary"; | ||
| }; | ||
| remote = { | ||
| path = lib.getExe' pkgs.audit "audisp-remote"; | ||
| config = { }; | ||
| }; | ||
| filter = { | ||
| path = lib.getExe' pkgs.audit "audisp-filter"; | ||
| args = [ | ||
| "allowlist" | ||
| "/etc/audit/audisp-filter.conf" | ||
| (lib.getExe' pkgs.audit "audisp-syslog") | ||
| "LOG_USER" | ||
| "LOG_INFO" | ||
| "interpret" | ||
| ]; | ||
| config = { }; | ||
| }; | ||
| syslog = { | ||
| path = lib.getExe' pkgs.audit "audisp-syslog"; | ||
| args = [ "LOG_INFO" ]; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This configuration is not introspectable from the options search. Reconfiguring the module from the config block is not ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but i am not super sure how to do this better. I still want to allow for arbitrary plugins to be configurable, and these default plugins need to be configured somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no good answer for how to do this any other way, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if there isn't a better way to do this, then this should be fine. Yes its not really introspectable, but without a solution its not actionable either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, i added the definition to defaultText, that should be acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good enough solution for now and definitely a significant improvement over the status quo.
|
Thanks for the detailed review! I copied most of the descriptions from the man pages, but it is entirely fair to adjust them to our style of documentation that is properly searchable and readable. I can try the assertion (though the logic will get awkward). Making the default plugins introspectable is more complex, not sure how to approach that yet. |
99e24cf to
57ddbb7
Compare
|
Seeing as i have some work to do here still, it is probably better to get #421698 merged (so we can start the staging cycle) and fix up the nix support to be able to configure this stuff later. That way we don't need to hurry as much, which is just prone to unnecessary mistakes. |
57ddbb7 to
01acf10
Compare
01acf10 to
18cf4bb
Compare
18cf4bb to
9eacded
Compare
9eacded to
cd9f500
Compare
|
Update: Today i went to build and run the audit testsuite (https://github.com/linux-audit/audit-testsuite), with plans of adding that to a VM test. Result: Hilariously broken, mostly because we don't even configure audit at all currently (until this PR gets a merge). This is probably not great, working audit (with a green test suite) would be nice. |
cd9f500 to
0646f66
Compare
0646f66 to
d34cdd8
Compare
nikstur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! There's some things to clean up but overall it's a great improvement and thus merge-worthy.
nixos/modules/security/auditd.nix
Outdated
| security.auditd.plugins = { | ||
| af_unix = { | ||
| path = lib.getExe' pkgs.audit "audisp-af_unix"; | ||
| args = [ | ||
| "0640" | ||
| "/var/run/audispd_events" | ||
| "string" | ||
| ]; | ||
| format = "binary"; | ||
| }; | ||
| remote = { | ||
| path = lib.getExe' pkgs.audit "audisp-remote"; | ||
| config = { }; | ||
| }; | ||
| filter = { | ||
| path = lib.getExe' pkgs.audit "audisp-filter"; | ||
| args = [ | ||
| "allowlist" | ||
| "/etc/audit/audisp-filter.conf" | ||
| (lib.getExe' pkgs.audit "audisp-syslog") | ||
| "LOG_USER" | ||
| "LOG_INFO" | ||
| "interpret" | ||
| ]; | ||
| config = { }; | ||
| }; | ||
| syslog = { | ||
| path = lib.getExe' pkgs.audit "audisp-syslog"; | ||
| args = [ "LOG_INFO" ]; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good enough solution for now and definitely a significant improvement over the status quo.
nixos/modules/security/auditd.nix
Outdated
| } | ||
| ) (lib.filterAttrs (_: v: v.config != null) cfg.plugins)); | ||
|
|
||
| security.auditd.plugins = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be aware of overriding parts of this config. The way config is merged based on it's priority is no t immediately obvious. See #285114
There is no action you can take right now but when this module is used, this might come up in the future.
nixos/modules/security/auditd.nix
Outdated
| ]) | ||
| ); | ||
| options = { | ||
| # space_left needs to be larger than admin_space_left, yet they default to be the same if left open. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a gut feeling that this is one of those too complex assertions that will become a maintancen burden but since it's already written I won't ask you to undo it.
d34cdd8 to
cf70c17
Compare
|
Thank you for the review, and happy to have this finally moving forward a bit :) |
Upstream PR merged: linux-audit/audit-userspace#467
Needed to fix #419093
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.