| Message ID | cc3ad4b4-fd8b-f3c5-31c4-e27a52c694c4@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#54309] services: auditd: use exclusive log directory for auditd | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/applying patch | fail | View Laminar job |
| cbaines/issue | success | View issue |
fesoj000 schreef op wo 09-03-2022 om 20:21 [+0100]: > Currently auditd writes logs to /var/log/audit.log. This is a problem because > auditd changes the permissions of the directory audit.log lives in to > 700. Why is auditd doing this? Can this behaviour be patched out? Is there an upstream report? > /var/log usually has 755, this is assumed by some services. postgresql > for example, fails when used together with auditd. Why does postgresql care about the group and other bits? Could postgresql be modified not to care? What are the reasons for changing the group and other bits? Perhaps that should be done by default by Guix when creating /var/log (POLA)? In any case, I would recommend adding to auditd.scm to make clear why the default log location is unacceptable. Greetings, Maxime.
Hi, On 3/9/22 8:36 PM, Maxime Devos wrote: > fesoj000 schreef op wo 09-03-2022 om 20:21 [+0100]: >> Currently auditd writes logs to /var/log/audit.log. This is a problem because >> auditd changes the permissions of the directory audit.log lives in to >> 700. > > Why is auditd doing this? Can this behaviour be patched out? Is there > an upstream reportThis is the default behavior. auditd will always change the permissions, but some attributes for this permission change can be configured in the config file. This behavior could be patched, but i don't think this is a good idea. Even the manpages assume /var/log/audit as the default log directory in some paragraphs. The auditd logfile contains events which can be usefull for debugging but usually this information is used in the aftermath of an cyberattack to learn more about what happend. It is even recommended to use a separate partition for /var/log/audit. auditd measures disk space and having /var/log/audit on a separate partition would deny unrelated processes from filling up the disk, effectively disabling audit logging. I think having /var/log/audit as the default log directory for auditd would not hurt. This would be more in line with other distros and further would allow to use a different partition. >> /var/log usually has 755, this is assumed by some services. postgresql >> for example, fails when used together with auditd. > > Why does postgresql care about the group and other bits? > Could postgresql be modified not to care? Maybe postgresql could be changed to gracefully handle this, but i am not sure what the benefit would be in this context. In my mind this is obviously a problem of how auditd is handled currently by auditd-service-type. Postgresql might be not the only service behaving this way. I did use postgresql as an example because this was the case i run into. > What are the reasons for changing the group and other bits? > Perhaps that should be done by default by Guix when creating > /var/log (POLA)? guix creates /var/log as 755, auditd changes its log directory to prevent access from unprivileged processes. Maybe auditd is paranoid in this case, but it is fine as long as it gets its own directory. > In any case, I would recommend adding to auditd.scm to make clear > why the default log location is unacceptable. The log location is configured by the configuration file. This configuration file is generated by auditd-service-type. The upstream [0] default configuration uses /var/log/audit as log directory. I think that documenting upstream default behavior does not add much value here. In fact, i think we can remove the log_file statement all together, because the built in default config uses /var/log/audit/audit.log [1]. I will prepare and test a new diff which removes the log_file statement. [0] https://github.com/linux-audit/audit-userspace/blob/master/init.d/auditd.conf [1] https://github.com/linux-audit/audit-userspace/blob/master/src/auditd-config.c#L314 BR
Hi there, i don't think this is the right spot to ask this, but i am not sure where else i could ask this. What is the patch acceptance process? I am wondering, what are steps from patch to commit? I have quite some packages, services and other patches laying around in varying quality. I recently started cleaning them up. I plan to get them integrated into master at some point. So, i assume that there has to be interest and time from a guix developer to review, maybe test and then integrate the changes/packages into one of the branches. Is there something i can do to help the process along? Maybe we could use this very patch as an example. Currently i am uncertain if this patch is appropriate for master, because of the risk that changing the auditd default log directory might break some setups. This could be 'fixed' by writing a news snippet. But i am not sure. Or is there still something else missing? BR
Hi, Am Freitag, dem 18.03.2022 um 20:17 +0100 schrieb fesoj000: > Hi there, > > i don't think this is the right spot to ask this, but i am not sure > where else i could ask this. > > What is the patch acceptance process? I am wondering, what are steps > from patch to commit? I have quite some packages, services and other > patches laying around in varying quality. I recently started > cleaning them up. I plan to get them integrated into master at some > point. In general it's send a patch, wait for review (this might take some time), argue with the reviewer, reluctantly send v2, v3, ... until your patch passes the quality criteria, gets stamped and arrives upstream via avian carrier. > So, i assume that there has to be interest and time from a guix > developer to review, maybe test and then integrate the > changes/packages into one of the branches. Note that there have already been two people reviewing; you currently owe me a v2 addressing the TOCTOU "race" of creating the audit directory without 700 permissions. > Is there something i can do to help the process along? Maybe we > could use this very patch as an example. Currently i am uncertain if > this patch is appropriate for master, because of the risk that > changing the auditd default log directory might break some setups. > This could be 'fixed' by writing a news snippet. But i > am not sure. Or is there still something else missing? In general, you should read the reviews carefully, reflect on them, implement suggested changes (or come up with better alternative solutions) and resend the patches with said changes; rinse and repeat until you hear a "LGTM". If the reviewer in question is a committer, you might hear a "Thanks, pushed" instead. As a rule of thumb, others will rarely actively fix up your code and if they do it's mostly minor things. Any work more than that takes them away from other review or their own submissions. Cheers
diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm index abde811f51..8478581999 100644 --- a/gnu/services/auditd.scm +++ b/gnu/services/auditd.scm @@ -31,7 +31,7 @@ (define-module (gnu services auditd) %default-auditd-configuration-directory)) (define auditd.conf - (plain-file "auditd.conf" "log_file = /var/log/audit.log\nlog_format = \ + (plain-file "auditd.conf" "log_file = /var/log/audit/audit.log\nlog_format = \ ENRICHED\nfreq = 1\nspace_left = 5%\nspace_left_action = \ syslog\nadmin_space_left_action = ignore\ndisk_full_action = \ ignore\ndisk_error_action = syslog\n")) @@ -50,6 +50,12 @@ (define-record-type* <auditd-configuration> (default audit)) (configuration-directory auditd-configuration-configuration-directory)) ; file-like +(define (auditd-activation config) + (with-imported-modules '((guix build utils)) + #~(begin + (use-modules (guix build utils)) + (mkdir-p "/var/log/audit")))) + (define (auditd-shepherd-service config) (let* ((audit (auditd-configuration-audit config)) (configuration-directory (auditd-configuration-configuration-directory config))) @@ -67,7 +73,9 @@ (define auditd-service-type (extensions (list (service-extension shepherd-root-service-type - auditd-shepherd-service))) + auditd-shepherd-service) + (service-extension activation-service-type + auditd-activation))) (default-value (auditd-configuration (configuration-directory %default-auditd-configuration-directory)))))