[PATCH] services: auditd: use exclusive log directory for auditd

OpenSubmitted by fesoj000.
Details
4 participants
  • fesoj000
  • Liliana Marie Prikler
  • Liliana Marie Prikler
  • Maxime Devos
Owner
unassigned
Severity
normal
F
F
fesoj000 wrote on 9 Mar 20:21 +0100
(address . guix-patches@gnu.org)
cc3ad4b4-fd8b-f3c5-31c4-e27a52c694c4@gmail.com
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. /var/log usually has 755, this is assumed by some services. postgresql
for example, fails when used together with auditd.

On redhat for example, auditd uses /var/log/audit/ as its log directory. This
change mirrors this behaviour.

* gnu/services/auditd.scm: add auditd-activation function and extend
activation-service-type.
---
gnu/services/auditd.scm | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

Toggle diff (39 lines)
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)))))
-- 
2.34.0
M
M
Maxime Devos wrote on 9 Mar 20:36 +0100
4ca12a3e0b1662addecb8bcca1f63ba5e223e8b8.camel@telenet.be
fesoj000 schreef op wo 09-03-2022 om 20:21 [+0100]:
Toggle quote (4 lines)
> 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?

Toggle quote (3 lines)
> /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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYikBqRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7u6NAPsEXmKP7fsQbcH0vymV1FoyouVQ
1zBRBm9lSWb1eLkC5AEAw3kSFrRC4HAyxEhGM2UzPIWwHBU5OKrZm0i+kaRXgwA=
=ODy0
-----END PGP SIGNATURE-----


F
F
fesoj000 wrote on 9 Mar 21:44 +0100
df590499-4508-249a-1083-ca596e1cf778@gmail.com
Hi,

On 3/9/22 8:36 PM, Maxime Devos wrote:
Toggle quote (7 lines)
> 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.

Toggle quote (5 lines)
>> /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.

Toggle quote (3 lines)
> 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.

Toggle quote (2 lines)
> 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.



BR
F
F
fesoj000 wrote on 9 Mar 22:00 +0100
[PATCH] services: auditd: use exclusive log directory for auditd
(address . 54309@debbugs.gnu.org)
b2d2cc9b-0c3f-b5a3-e564-76ab8f17459f@gmail.com
Use the upstream default log file for auditd.

* gnu/services/auditd.scm: add auditd-activation function and extend
activation-service-type.
---
gnu/services/auditd.scm | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)

Toggle diff (44 lines)
diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm
index abde811f51..c88e974adb 100644
--- a/gnu/services/auditd.scm
+++ b/gnu/services/auditd.scm
@@ -31,10 +31,9 @@ (define-module (gnu services auditd)
              %default-auditd-configuration-directory))
  
  (define auditd.conf
-  (plain-file "auditd.conf" "log_file = /var/log/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"))
+  (plain-file "auditd.conf" "log_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"))
  
  (define %default-auditd-configuration-directory
    (computed-file "auditd"
@@ -50,6 +49,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 +72,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)))))
-- 
2.34.0
L
L
Liliana Marie Prikler wrote on 10 Mar 08:12 +0100
ddafff6b38aba33f1e4a703114b243ac8273cb6c.camel@ist.tugraz.at
Hi,

Am Mittwoch, dem 09.03.2022 um 22:00 +0100 schrieb fesoj000:
Toggle quote (25 lines)
> Use the upstream default log file for auditd.
>
> * gnu/services/auditd.scm: add auditd-activation function and extend
> activation-service-type.
> ---
>   gnu/services/auditd.scm | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm
> index abde811f51..c88e974adb 100644
> --- a/gnu/services/auditd.scm
> +++ b/gnu/services/auditd.scm
> @@ -31,10 +31,9 @@ (define-module (gnu services auditd)
>               %default-auditd-configuration-directory))
>  
>   (define auditd.conf
> -  (plain-file "auditd.conf" "log_file =
> /var/log/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"))
> +  (plain-file "auditd.conf" "log_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"))
I'm not sure what the rationale behind writing auditd.conf this way is,
but note that can simply writethis as "\
log_format = ENRICHED
freq = 1
space_left = 5%
..."

Doing this, it would take up some more vertical real estate, but imho
it'd be easier to read. We might also want to make some of these
configurable later on, e.g. space_left, but that's not relevant to this
patch set.

Toggle quote (12 lines)
>   (define %default-auditd-configuration-directory
>     (computed-file "auditd"
> @@ -50,6 +49,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"))))
I think guix should already create this directory with the 700
permissions auditd demands, to prevent any TOCTOU-style tampering.


Cheers
F
F
fesoj000 wrote on 10 Mar 11:36 +0100
dc42a4cf-791a-1f0f-e8bc-66d498af63bd@gmail.com
Hi,

On 3/10/22 8:12 AM, Liliana Marie Prikler wrote:
Toggle quote (39 lines)
> Hi,
>
> Am Mittwoch, dem 09.03.2022 um 22:00 +0100 schrieb fesoj000:
>> Use the upstream default log file for auditd.
>>
>> * gnu/services/auditd.scm: add auditd-activation function and extend
>> activation-service-type.
>> ---
>>   gnu/services/auditd.scm | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm
>> index abde811f51..c88e974adb 100644
>> --- a/gnu/services/auditd.scm
>> +++ b/gnu/services/auditd.scm
>> @@ -31,10 +31,9 @@ (define-module (gnu services auditd)
>>               %default-auditd-configuration-directory))
>>
>>   (define auditd.conf
>> -  (plain-file "auditd.conf" "log_file =
>> /var/log/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"))
>> +  (plain-file "auditd.conf" "log_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"))
> I'm not sure what the rationale behind writing auditd.conf this way is,
> but note that can simply writethis as "\
> log_format = ENRICHED
> freq = 1
> space_left = 5%
> ..."
>
> Doing this, it would take up some more vertical real estate, but imho
> it'd be easier to read. We might also want to make some of these
> configurable later on, e.g. space_left, but that's not relevant to this
> patch set.
Sure, i will send a new patch later.

Toggle quote (14 lines)
>>   (define %default-auditd-configuration-directory
>>     (computed-file "auditd"
>> @@ -50,6 +49,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"))))
> I think guix should already create this directory with the 700
> permissions auditd demands, to prevent any TOCTOU-style tampering.
Good point.
F
F
fesoj000 wrote on 10 Mar 17:29 +0100
[PATCH] services: auditd: use exclusive log directory for auditd
(address . 54309@debbugs.gnu.org)
8b52d7e5-314d-406e-87b7-cdc087dc7826@gmail.com
Use /var/log/audit for auditd. This is the upstream default.

Further, rework the config file generated by auditd-service-type. Only
write values which diverge from the upstream default.

* gnu/services/auditd.scm: add auditd-activation function and extend
activation-service-type.
---
gnu/services/auditd.scm | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)

Toggle diff (47 lines)
diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm
index abde811f51..1eb0ed291b 100644
--- a/gnu/services/auditd.scm
+++ b/gnu/services/auditd.scm
@@ -31,10 +31,10 @@ (define-module (gnu services auditd)
              %default-auditd-configuration-directory))
  
  (define auditd.conf
-  (plain-file "auditd.conf" "log_file = /var/log/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"))
+  (plain-file "auditd.conf" "\
+space_left = 5%
+space_left_action = syslog
+"))
  
  (define %default-auditd-configuration-directory
    (computed-file "auditd"
@@ -50,6 +50,14 @@ (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))
+        (let ((var-log-audit "/var/log/audit"))
+          (mkdir-p var-log-audit)
+          (chmod var-log-audit #o700)))))
+
  (define (auditd-shepherd-service config)
    (let* ((audit (auditd-configuration-audit config))
           (configuration-directory (auditd-configuration-configuration-directory config)))
@@ -67,7 +75,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)))))
-- 
2.34.0
F
F
fesoj000 wrote on 18 Mar 20:17 +0100
What is the process from here?
(address . 54309@debbugs.gnu.org)
b70a1fd7-e432-3d6d-8dea-17d4231e407e@gmail.com
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
L
L
Liliana Marie Prikler wrote on 18 Mar 21:06 +0100
8640c4e7b1a46e76ea4df8c21a290d1aa72de0d8.camel@gmail.com
Hi,

Am Freitag, dem 18.03.2022 um 20:17 +0100 schrieb fesoj000:
Toggle quote (10 lines)
> 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.

Toggle quote (3 lines)
> 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.

Toggle quote (6 lines)
> 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
F
F
fesoj000 wrote on 18 Mar 22:48 +0100
fafb2565-5593-aabb-1852-2af4e7dd7478@gmail.com
On 3/18/22 9:06 PM, Liliana Marie Prikler wrote:
Toggle quote (6 lines)
>> 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.
Yes, that is true. But i addressed the rest, i think. New version inline.


From 0605a2b5cc8beb816e3ff557d7be060a050f91b7 Mon Sep 17 00:00:00 2001
From: fesoj000 <fesoj000@gmail.com>
Date: Wed, 9 Mar 2022 20:07:42 +0100
Subject: [PATCH] services: auditd: use exclusive log directory for auditd

Use /var/log/audit for auditd. This is the upstream default.

Further, rework the config file generated by auditd-service-type. Only
write values which diverge from the upstream default.

* gnu/services/auditd.scm: add auditd-activation function and extend
activation-service-type.
---
gnu/services/auditd.scm | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)

Toggle diff (47 lines)
diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm
index abde811f51..602a6c5a48 100644
--- a/gnu/services/auditd.scm
+++ b/gnu/services/auditd.scm
@@ -31,10 +31,10 @@ (define-module (gnu services auditd)
              %default-auditd-configuration-directory))
  
  (define auditd.conf
-  (plain-file "auditd.conf" "log_file = /var/log/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"))
+  (plain-file "auditd.conf" "\
+space_left = 5%
+space_left_action = syslog
+"))
  
  (define %default-auditd-configuration-directory
    (computed-file "auditd"
@@ -50,6 +50,14 @@ (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))
+        (let ((var-log-audit "/var/log/audit"))
+          (umask #o077)
+          (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 +75,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)))))
-- 
2.34.0
L
L
Liliana Marie Prikler wrote on 18 Mar 23:36 +0100
87f117ba35bb40fe063d5cd0bee61039a5f9801e.camel@gmail.com
Am Freitag, dem 18.03.2022 um 22:48 +0100 schrieb fesoj000:
Toggle quote (10 lines)
> On 3/18/22 9:06 PM, Liliana Marie Prikler wrote:
> > > 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.
> Yes, that is true. But i addressed the rest, i think. New version
> inline.
For the record, inline patches generate noise that's hard to separate
when applying, so you'd probably want to avoid them. If you don't have
git send-email set up regular attachments also work for some, though
they do become tedious as well with series.

Toggle quote (36 lines)
> From 0605a2b5cc8beb816e3ff557d7be060a050f91b7 Mon Sep 17 00:00:00
> 2001
> From: fesoj000 <fesoj000@gmail.com>
> Date: Wed, 9 Mar 2022 20:07:42 +0100
> Subject: [PATCH] services: auditd: use exclusive log directory for
> auditd
>
> Use /var/log/audit for auditd. This is the upstream default.
>
> Further, rework the config file generated by auditd-service-type.
> Only
> write values which diverge from the upstream default.
>
> * gnu/services/auditd.scm: add auditd-activation function and extend
> activation-service-type.
> ---
>   gnu/services/auditd.scm | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm
> index abde811f51..602a6c5a48 100644
> --- a/gnu/services/auditd.scm
> +++ b/gnu/services/auditd.scm
> @@ -31,10 +31,10 @@ (define-module (gnu services auditd)
>               %default-auditd-configuration-directory))
>  
>   (define auditd.conf
> -  (plain-file "auditd.conf" "log_file =
> /var/log/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"))
> +  (plain-file "auditd.conf" "\
> +space_left = 5%
> +space_left_action = syslog
> +"))
I can understand discarding the log_file entry because we now use
upstream default, but the rest should remain imo.
Toggle quote (15 lines)
>   (define %default-auditd-configuration-directory
>     (computed-file "auditd"
> @@ -50,6 +50,14 @@ (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))
> +        (let ((var-log-audit "/var/log/audit"))
> +          (umask #o077)
> +          (mkdir-p var-log-audit)))))
> +
This would also apply umask 077 to /var and /var/log if those don't
already exist. More importantly, code executed after that will also
inherit the umask, which I don't think is the intended consequence.


Cheers
F
F
fesoj000 wrote on 19 Mar 12:10 +0100
f877fe2f-1d94-fd39-7f4e-08dfd3810071@gmail.com
On 3/18/22 11:36 PM, Liliana Marie Prikler wrote:
Toggle quote (54 lines)
> Am Freitag, dem 18.03.2022 um 22:48 +0100 schrieb fesoj000:
>> On 3/18/22 9:06 PM, Liliana Marie Prikler wrote:
>>>> 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.
>> Yes, that is true. But i addressed the rest, i think. New version
>> inline.
> For the record, inline patches generate noise that's hard to separate
> when applying, so you'd probably want to avoid them. If you don't have
> git send-email set up regular attachments also work for some, though
> they do become tedious as well with series.
>
>> From 0605a2b5cc8beb816e3ff557d7be060a050f91b7 Mon Sep 17 00:00:00
>> 2001
>> From: fesoj000 <fesoj000@gmail.com>
>> Date: Wed, 9 Mar 2022 20:07:42 +0100
>> Subject: [PATCH] services: auditd: use exclusive log directory for
>> auditd
>>
>> Use /var/log/audit for auditd. This is the upstream default.
>>
>> Further, rework the config file generated by auditd-service-type.
>> Only
>> write values which diverge from the upstream default.
>>
>> * gnu/services/auditd.scm: add auditd-activation function and extend
>> activation-service-type.
>> ---
>>   gnu/services/auditd.scm | 20 +++++++++++++++-----
>>   1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm
>> index abde811f51..602a6c5a48 100644
>> --- a/gnu/services/auditd.scm
>> +++ b/gnu/services/auditd.scm
>> @@ -31,10 +31,10 @@ (define-module (gnu services auditd)
>>               %default-auditd-configuration-directory))
>>
>>   (define auditd.conf
>> -  (plain-file "auditd.conf" "log_file =
>> /var/log/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"))
>> +  (plain-file "auditd.conf" "\
>> +space_left = 5%
>> +space_left_action = syslog
>> +"))
> I can understand discarding the log_file entry because we now use
> upstream default, but the rest should remain imo.
Alright. Lets first keep all options. At another point in time we can
rethink the default options. Maybe when implementing configuration for
auditd.

Toggle quote (17 lines)
>>   (define %default-auditd-configuration-directory
>>     (computed-file "auditd"
>> @@ -50,6 +50,14 @@ (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))
>> +        (let ((var-log-audit "/var/log/audit"))
>> +          (umask #o077)
>> +          (mkdir-p var-log-audit)))))
>> +
> This would also apply umask 077 to /var and /var/log if those don't
> already exist.
Hm, it seems that 'gnu/services.scm: (activation-script)' ensures the
existence of /var/log before the auditd activation gexp is running. So,
the reasoning behind your remark is that we can not guarantee the
existence of /var/log in every case? What cases might that be? I will
take care of it anyway for the sake of robustness, but i am curious.

Toggle quote (2 lines)
> More importantly, code executed after that will also
> inherit the umask, which I don't think is the intended consequence.
I was under the impression that every activation script is run it its
own process. But that is not the case. This changes things, more care
is needed.

Patch will follow later.
F
F
fesoj000 wrote on 19 Mar 12:34 +0100
[PATCH] services: auditd: use exclusive log directory for auditd
(address . 54309@debbugs.gnu.org)
a545ae09-1801-3d71-ef4c-a490dcc39cdf@gmail.com
Use /var/log/audit for auditd. This is the upstream default.

Further, make the inline config file more readable.

* gnu/services/auditd.scm: add auditd-activation function and extend
activation-service-type.
---
gnu/services/auditd.scm | 27 ++++++++++++++++++++++-----
1 file changed, 22 insertions(+), 5 deletions(-)

Toggle diff (54 lines)
diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm
index abde811f51..e9e19c61be 100644
--- a/gnu/services/auditd.scm
+++ b/gnu/services/auditd.scm
@@ -31,10 +31,15 @@ (define-module (gnu services auditd)
              %default-auditd-configuration-directory))
  
  (define auditd.conf
-  (plain-file "auditd.conf" "log_file = /var/log/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"))
+  (plain-file "auditd.conf" "\
+log_format = ENRICHED
+freq = 1
+space_left = 5%
+space_left_action = syslog
+admin_space_left_action = ignore
+disk_full_action = ignore
+disk_error_action = syslog
+"))
  
  (define %default-auditd-configuration-directory
    (computed-file "auditd"
@@ -50,6 +55,16 @@ (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))
+        ;; make sure /var/log exists before changing umask
+        (mkdir-p "/var/log")
+        (let* ((previous-umask (umask #o077)))
+          (mkdir-p "/var/log/audit")
+          (umask previous-umask)))))
+
  (define (auditd-shepherd-service config)
    (let* ((audit (auditd-configuration-audit config))
           (configuration-directory (auditd-configuration-configuration-directory config)))
@@ -67,7 +82,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)))))
-- 
2.34.0
M
M
Maxime Devos wrote on 20 Mar 00:09 +0100
Re: [bug#54309] What is the process from here?
b311a2b72fd271fd3e7b78a8aca11cd4896fe49c.camel@telenet.be
Liliana Marie Prikler schreef op vr 18-03-2022 om 23:36 [+0100]:
Toggle quote (12 lines)
> > +(define (auditd-activation config)
> > +  (with-imported-modules '((guix build utils))
> > +    #~(begin
> > +        (use-modules (guix build utils))
> > +        (let ((var-log-audit "/var/log/audit"))
> > +          (umask #o077)
> > +          (mkdir-p var-log-audit)))))
> > +
> This would also apply umask 077 to /var and /var/log if those don't
> already exist.  More importantly, code executed after that will also
> inherit the umask, which I don't think is the intended consequence.

More concretely, the procedure 'mkdir-p/perms' would address the umask
issue, but not the potential ‘oops too restrictive permissions for /var
and /var/log' issue. Additionally, as var-log-audit is only used in a
single place, you could simplify to

#~(begin
(use-modules ...)
(mkdir-p/perms "/var/log/audit"))

here.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYjZiwBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7k/wAQDRUa/PJU/p21gJrWCwCH6z4246
mkZ6BrAZUGXGBD/l7gD+NEdmv7dn7d82v7++/g3aTi1dZWlAdZFZr8Eg4cRDYg0=
=d/f5
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 20 Mar 00:13 +0100
Re: [bug#54309] [PATCH] services: auditd: use exclusive log directory for auditd
80cc8802439c49138638dee6d4fb9015723f9727.camel@telenet.be
fesoj000 schreef op za 19-03-2022 om 12:34 [+0100]:
Toggle quote (4 lines)
> +        (let* ((previous-umask (umask #o077)))
> +          (mkdir-p "/var/log/audit")
> +          (umask previous-umask)))))

I cannot recommend this, what if 'mkdir-p' throws an exception?
That might cause problems. Or maybe not, but it would require
some analysis that can be avoided with 'mkdir-p/perms'.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYjZjhBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7q6aAP48JIAfJ1LLtn22p4Ps43AiU7Er
ncPY6sMxUH5KIZXefgD/du52/IebjsxywuoL1p1MP1vo0/Z90xVd8zcXI8vUBAQ=
=hgd/
-----END PGP SIGNATURE-----


F
F
fesoj000 wrote on 20 Mar 21:22 +0100
dce285c1-3d60-31ab-8f5a-1389c42080c2@gmail.com
On 3/20/22 12:13 AM, Maxime Devos wrote:
Toggle quote (8 lines)
> fesoj000 schreef op za 19-03-2022 om 12:34 [+0100]:
>> +        (let* ((previous-umask (umask #o077)))
>> +          (mkdir-p "/var/log/audit")
>> +          (umask previous-umask)))))
>
> I cannot recommend this, what if 'mkdir-p' throws an exception?
> That might cause problems. Or maybe not, but it would require
> some analysis that can be avoided with 'mkdir-p/perms'.
Hm, but i still have to set umask to prevent TOCTOU, the
implementation of 'mkdir-p/perms' does not take care of that.

BR
M
M
Maxime Devos wrote on 20 Mar 21:30 +0100
05b76afd93703efe8fca7fd11513c07343123dd3.camel@telenet.be
fesoj000 schreef op zo 20-03-2022 om 21:22 [+0100]:
Toggle quote (6 lines)
> > I cannot recommend this, what if 'mkdir-p' throws an exception?
> > That might cause problems.  Or maybe not, but it would require
> > some analysis that can be avoided with 'mkdir-p/perms'.
> Hm, but i still have to set umask to prevent TOCTOU, the
> implementation of 'mkdir-p/perms' does not take care of that.

mkdir-p/perms could be modified to take care of that.
If that is done, then other users of mkdir-p/perms would benefit as
well.

To implement this, I recommend using the prodecures from
-- that patch was written to remove the TOCTOU!

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYjeO7BccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7uaQAP9x64C9LxQ9a7zwaAtTpMjlRs0A
CAbEvZkjjIi/cREyIQEAn1JIs/z1wT5juNCj1DdhWeQvW4wFpaASFS0VEs6AyAk=
=vQlN
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 20 Mar 21:35 +0100
164b19e525895c50d73e0a84658c1d30c6ab452f.camel@telenet.be
Maxime Devos schreef op zo 20-03-2022 om 21:30 [+0100]:
Toggle quote (16 lines)
> fesoj000 schreef op zo 20-03-2022 om 21:22 [+0100]:
> > > I cannot recommend this, what if 'mkdir-p' throws an exception?
> > > That might cause problems.  Or maybe not, but it would require
> > > some analysis that can be avoided with 'mkdir-p/perms'.
> > Hm, but i still have to set umask to prevent TOCTOU, the
> > implementation of 'mkdir-p/perms' does not take care of that.
>
> mkdir-p/perms could be modified to take care of that.
> If that is done, then other users of mkdir-p/perms would benefit as
> well.
>
> To implement this, I recommend using the prodecures from
> <https://lists.gnu.org/archive/html/guile-devel/2021-11/msg00005.html
> >
> -- that patch was written to remove the TOCTOU!

It seems to be ignored so far upstream, so I'll look into defining
a Guile package variant that can be used by the activation code.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYjeQARccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7jndAP4vJegPum4a6DSNEHRtSGLywGcu
PzAW+e2xmG/tOnODxwEAwcnBgKCCJpsrqWIvBwDu0BIslZazdAiliGULghFJuAQ=
=Vvn8
-----END PGP SIGNATURE-----


F
F
fesoj000 wrote on 22 Mar 17:50 +0100
Re: [bug#54309] What is the process from here?
9f1cfbac-652f-ea32-d3fc-b730534ff666@gmail.com
On 3/20/22 12:09 AM, Maxime Devos wrote:
Toggle quote (16 lines)
> Liliana Marie Prikler schreef op vr 18-03-2022 om 23:36 [+0100]:
>>> +(define (auditd-activation config)
>>> +  (with-imported-modules '((guix build utils))
>>> +    #~(begin
>>> +        (use-modules (guix build utils))
>>> +        (let ((var-log-audit "/var/log/audit"))
>>> +          (umask #o077)
>>> +          (mkdir-p var-log-audit)))))
>>> +
>> This would also apply umask 077 to /var and /var/log if those don't
>> already exist.  More importantly, code executed after that will also
>> inherit the umask, which I don't think is the intended consequence.
>
> More concretely, the procedure 'mkdir-p/perms' would address the umask
> issue, but not the potential ‘oops too restrictive permissions for /var
> and /var/log' issue.
Ok, i can assume that a future version of 'mkdir-p/perms' will handle the
umask.

Should the activation now handle potential permission problems from past
activations and auditd starts? Can you try to explain in more detail
please?

BR
L
L
Liliana Marie Prikler wrote on 22 Mar 21:06 +0100
f771c36e4d8a7c580ea05b89dc47cf19ce0e07ba.camel@gmail.com
Am Dienstag, dem 22.03.2022 um 17:50 +0100 schrieb fesoj000:
Toggle quote (24 lines)
> On 3/20/22 12:09 AM, Maxime Devos wrote:
> > Liliana Marie Prikler schreef op vr 18-03-2022 om 23:36 [+0100]:
> > > > +(define (auditd-activation config)
> > > > +  (with-imported-modules '((guix build utils))
> > > > +    #~(begin
> > > > +        (use-modules (guix build utils))
> > > > +        (let ((var-log-audit "/var/log/audit"))
> > > > +          (umask #o077)
> > > > +          (mkdir-p var-log-audit)))))
> > > > +
> > > This would also apply umask 077 to /var and /var/log if those
> > > don't already exist.  More importantly, code executed after that
> > > will also inherit the umask, which I don't think is the intended
> > > consequence.
> >
> > More concretely, the procedure 'mkdir-p/perms' would address the
> > umask issue, but not the potential ‘oops too restrictive
> > permissions for /var and /var/log' issue.
> Ok, i can assume that a future version of 'mkdir-p/perms' will handle
> the umask.
>
> Should the activation now handle potential permission problems from
> past activations and auditd starts? Can you try to explain in more
> detail please?
My personal solution would be to use (mkdir-p "/var/log") followed by
(mkdir "/var/log/audit" #o700).

Cheers
F
F
fesoj000 wrote on 23 Mar 21:22 +0100
[PATCHv2] services: auditd: use exclusive log directory for auditd
(address . 54309@debbugs.gnu.org)
e93a3c6e-0b66-de42-908c-6f521e8f4460@gmail.com
Use /var/log/audit for auditd. This is the upstream default.

* gnu/services/auditd.scm: add auditd-activation function and extend
activation-service-type.
---
gnu/services/auditd.scm | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)

Toggle diff (52 lines)
diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm
index abde811f51..8e3d5fd2bc 100644
--- a/gnu/services/auditd.scm
+++ b/gnu/services/auditd.scm
@@ -31,10 +31,15 @@ (define-module (gnu services auditd)
              %default-auditd-configuration-directory))
  
  (define auditd.conf
-  (plain-file "auditd.conf" "log_file = /var/log/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"))
+  (plain-file "auditd.conf" "\
+log_format = ENRICHED
+freq = 1
+space_left = 5%
+space_left_action = syslog
+admin_space_left_action = ignore
+disk_full_action = ignore
+disk_error_action = syslog
+"))
  
  (define %default-auditd-configuration-directory
    (computed-file "auditd"
@@ -50,6 +55,14 @@ (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))
+        ;; ensure /var/log exists with default permissions
+        (mkdir-p "/var/log")
+        (mkdir-p/perm "/var/log/audit" (getpwnam "root") #o700))))
+
  (define (auditd-shepherd-service config)
    (let* ((audit (auditd-configuration-audit config))
           (configuration-directory (auditd-configuration-configuration-directory config)))
@@ -67,7 +80,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)))))
-- 
2.34.0
F
F
fesoj000 wrote on 23 Mar 21:39 +0100
[PATCHv3] services: auditd: use exclusive log directory for auditd
(address . 54309@debbugs.gnu.org)
10e679cf-b574-5ae5-76f7-d59828ed0dae@gmail.com
Use /var/log/audit for auditd. This is the upstream default.

* gnu/services/auditd.scm: add auditd-activation function and extend
activation-service-type.
---
gnu/services/auditd.scm | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)

Toggle diff (60 lines)
diff --git a/gnu/services/auditd.scm b/gnu/services/auditd.scm
index abde811f51..33849155a1 100644
--- a/gnu/services/auditd.scm
+++ b/gnu/services/auditd.scm
@@ -26,15 +26,21 @@ (define-module (gnu services auditd)
    #:use-module (guix records)
    #:use-module (guix gexp)
    #:use-module (guix packages)
+  #:use-module (guix modules)
    #:export (auditd-configuration
              auditd-service-type
              %default-auditd-configuration-directory))
  
  (define auditd.conf
-  (plain-file "auditd.conf" "log_file = /var/log/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"))
+  (plain-file "auditd.conf" "\
+log_format = ENRICHED
+freq = 1
+space_left = 5%
+space_left_action = syslog
+admin_space_left_action = ignore
+disk_full_action = ignore
+disk_error_action = syslog
+"))
  
  (define %default-auditd-configuration-directory
    (computed-file "auditd"
@@ -50,6 +56,16 @@ (define-record-type* <auditd-configuration>
                             (default audit))
    (configuration-directory auditd-configuration-configuration-directory))      ; file-like
  
+(define (auditd-activation config)
+  (with-imported-modules (source-module-closure '((gnu build activation)
+                                                  (guix build utils)))
+    #~(begin
+        (use-modules (gnu build activation)
+                     (guix build utils))
+        ;; ensure /var/log exists with default permissions
+        (mkdir-p "/var/log")
+        (mkdir-p/perms "/var/log/audit" (getpwnam "root") #o700))))
+
  (define (auditd-shepherd-service config)
    (let* ((audit (auditd-configuration-audit config))
           (configuration-directory (auditd-configuration-configuration-directory config)))
@@ -67,7 +83,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)))))
-- 
2.34.0
?