[bug#54309] What is the process from here?

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.


 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(-)

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.
 
>   (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

On 3/18/22 11:36 PM, Liliana Marie Prikler wrote:
> 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.

>>    (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.

> 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.

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.  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.

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?

BR

Am Dienstag, dem 22.03.2022 um 17:50 +0100 schrieb fesoj000:
> 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