[bug#63314] [PATCH 0/2] Add PAM shepherd requirements

["Ludovic Courtès" <ludo@gnu.org>]

Hello!

Josselin Poiret <dev@jpoiret.xyz> skribis:

> From: Josselin Poiret <dev@jpoiret.xyz>
>
> * gnu/system/pam.scm (<pam-extender>): New record type.
> (pam-shepherd-service): Add Shepherd synchronization point.
>
> * gnu/services/mail.scm (dovecot-shepherd-service)
> * gnu/services/lightdm.scm (lightdm-shepherd-service)
> * gnu/services/mail.scm (opensmtpd-shepherd-service)
> * gnu/services/sddm.scm (sddm-shepherd-service)
> * gnu/services/ssh.scm (lsh-shepherd-service, openssh-shepherd-service)
> * gnu/services/xorg.scm (slim-shepherd-service, gdm-shepherd-service)
> * gnu/services/base.scm (greetd-shepherd-services): Add PAM requirement.
>
> * gnu/system/pam.scm (/etc-entry, extend-configuration,
> pam-root-service-type, pam-root-service)
> * gnu/services/authentication.scm (pam-ldap-pam-service)
> * gnu/services/base.scm (pam-limits-service-type)
> (greetd-pam-service)
> * gnu/services/desktop.scm (pam-gnome-keyring)
> * gnu/services/kerberos.scm (pam-krb5-pam-service)
> * gnu/services/pam-mount.scm (pam-mount-pam-service): Adapt to pam-extenders.

The approach looks reasonable to me, well done!

> +;; A PAM transformer consists of a procedure acting on each PAM entry, with an
> +;; additional list of shepherd-requirements that the meta PAM sheherd service
> +;; will rely on.
> +(define-record-type* <pam-extender>
> +  pam-extender make-pam-extender pam-extender?
> +  (transformer pam-extender-transformer)
> +  (shepherd-requirements pam-extender-shepherd-requirements
> +                         (default '())))

I would call it <pam-extension> (similar to <home-bash-extension>).
There’s a typo in the comment (“sheherd”); s/rely on/depend on/.

>  ;; Overall PAM configuration: a list of services, plus a procedure that takes
>  ;; one <pam-service> and returns a <pam-service>.  The procedure is used to
>  ;; implement cross-cutting concerns such as the use of the 'elogind.so'
>  ;; session module that keeps track of logged-in users.
>  (define-record-type* <pam-configuration>
> -  pam-configuration make-pam-configuration? pam-configuration?
> +  pam-configuration make-pam-configuration pam-configuration?
>    (services  pam-configuration-services)          ;list of <pam-service>
> -  (transform pam-configuration-transform))        ;procedure
> +  (extenders pam-configuration-extenders))        ;list of <pam-extender>

Instead of storing extensions, we should keep the full configuration
here (similar to <home-bash-configuration>).  That is, remove
‘extenders’ and instead add ‘shepherd-requirements’.

> +(define (pam-shepherd-service config)
> +  (define requirements
> +    (match config
> +      (($ <pam-configuration> services extenders)
> +       (concatenate (map pam-extender-shepherd-requirements extenders)))))

Rather: (append-map …)

Also please add a docstring.

>  (define (extend-configuration initial extensions)
>    "Extend INITIAL with NEW."
> -  (let-values (((services procs)
> +  (let-values (((services extenders)
>                  (partition pam-service? extensions)))
>      (pam-configuration
>       (services (append (pam-configuration-services initial)
>                         services))
> -     (transform (apply compose
> -                       (pam-configuration-transform initial)
> -                       procs)))))
> +     (extenders (append (pam-configuration-extenders initial)
> +                        extenders)))))

This would need to be adjusted accordingly.

Also, we need to preserve backward compatibility, so we should first do
something like:

  (let ((extensions (map (lambda (extension)
                           (if (pam-extension? extension)
                               extension
                               (begin
                                 (warn-about-deprecation …)
                                 (pam-extension (transformer extension)))))
                         extensions)))
   …)                         

Ludo’.