[bug#73925] [PATCH] add access control to daemon socket in shepherd service

[Ludovic Courtès]

Hi,

Reepca Russelstein <reepca@russelstein.xyz> skribis:

> From b5163889efb544cfe83cd2bcb3ebd3a957c95a18 Mon Sep 17 00:00:00 2001
> Message-ID: 
> <b5163889efb544cfe83cd2bcb3ebd3a957c95a18.1729465936.git.reepca@russelstein.xyz>
> From: Reepca Russelstein <reepca@russelstein.xyz>
> Date: Sat, 19 Oct 2024 22:43:27 -0500
> Subject: [PATCH] services: guix-configuration: add access control to daemon
>  socket.
>
> * gnu/services/base.scm
>   (guix-configuration-socket-directory-{perms,group,user}): new fields.
>   (guix-shepherd-service): use them.
> * doc/guix.texi: document them.
>
> Change-Id: Ic228377b25a83692b0c637dafbd03c4609e332fc

That’s a welcome addition.

> +@item @code{socket-directory-perms} (default: @code{#o755})

s/perms/permissions/

> +Permissions to set for the directory @file{/var/guix/daemon-socket}.
> +This, together with @code{socket-directory-group} and
> +@code{socket-directory-user}, determines who can connect to the guix
> +daemon via its unix socket.  TCP socket operation is unaffected by
> +these.

s/guix daemon/build daemon/ and s/unix/Unix/

> +@item @code{socket-directory-group} (default: @code{#f})
> +Group to set for the directory @file{/var/guix/daemon-socket}, or
> +@code{#f} to keep its group as root.
> +
> +@item @code{socket-directory-user} (default: @code{#f})
> +User to set for the directory @file{/var/guix/daemon-socket}, or
> +@code{#f} to keep its user as root.

Maybe group them together:

  @item @code{socket-directory-user} (default: @code{#f})
  @itemx @code{socket-directory-group} (default: @code{#f})
  User and group owning the @file{/var/guix/daemon-socket} directory.
  …

> -    (guix build-group build-accounts authorize-key? authorized-keys
> -          use-substitutes? substitute-urls max-silent-time timeout
> -          log-compression discover? extra-options log-file
> -          http-proxy tmpdir chroot-directories environment)
> +                (guix build-group build-accounts authorize-key? 
> authorized-keys

Please avoid reindenting.

> +                  ;; Ensure that a fresh directory is used, in case the old
> +                  ;; one was more permissive and processes have a file
> +                  ;; descriptor referencing it hanging around, ready to use
> +                  ;; with openat.
> +                  (false-if-exception
> +                   (delete-file-recursively "/var/guix/daemon-socket"))
> +                  (let ((perms #$(logand socket-directory-perms
> +                                         (lognot #o022))))
> +                    (mkdir "/var/guix/daemon-socket" perms)
> +                    ;; Override umask
> +                    (chmod "/var/guix/daemon-socket" perms))

Speaking of ‘openat’, maybe use ‘mkdir-p/perms’ instead of doing it in
two steps?

Apart from that it LGTM.

Could you send an updated patch?

Thanks,
Ludo’.