GNU bug report logs - #65538
[PATCH v2] services: greetd: Add pam-gnupg support.

Previous Next

Full log

Message #8 received at 65538 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Carlos Durán Domínguez <wurt <at> wurtshell.com>
Cc: Tobias Geerinckx-Rice <me <at> tobias.gr>,
 Simon Tournier <zimon.toutoune <at> gmail.com>, paren <at> disroot.org,
 Christopher Baines <mail <at> cbaines.net>, Ricardo Wurmus <rekado <at> elephly.net>,
 Raghav Gururajan <rg <at> raghavgururajan.name>, jgart <jgart <at> dismail.de>,
 Mathieu Othacehe <othacehe <at> gnu.org>, 65538 <at> debbugs.gnu.org
Subject: Re: bug#65538: [PATCH v2] services: greetd: Add pam-gnupg support.
Date: Thu, 05 Oct 2023 14:57:09 +0200

Hello,

Carlos Durán Domínguez <wurt <at> wurtshell.com> skribis:

> I retry to implement the pam-gnupg module for the greetd system service. It is A PAM module that hands over your login password to gpg-agent. I added de documentation and the insert-before procedure (maybe it needs a better name), to ensure that the pam-gnupg module will be loaded at the end.
>
> * doc/guix.texi: documentation about #:gnupg? option on (greetd-configuration).
> * gnu/services.scm (insert-before): new procedure.
> * gnu/services/base.scm (greetd-configuration): new option #:gnupg?.
> * gnu/services/pam-mount.scm: ensure that pam mount module goes before pam gnupg module.
> * gnu/system/pam.scm (pam-gnupg-module?): new procedure and ensure that pam gnupg module is at the end of (unix-pam-service).

Nice work!

A minor point: the commit log should normally lists all
changed/added/removed entities.  You can use ‘git log’ to see examples,
but the committer will tweak it for you if needed (no big deal).

[...]

> +@item @code{gnupg?} (default: @code{#f})
> +If enabled, @code{pam-gnupg} will attempt to automatically unlock the
> +user's GPG keys with the login password via @code{gpg-agent}.  The
> +keygrips of all keys to be unlocked should be written to
> +@file{~/.pam-gnupg}, and can be queried with @code{gpg -K
> +--with-keygrip}.  Presetting passphrases must be enabled by adding
> +@code{allow-preset-passphrase} in @file{~/.gnupg/gpg-agent.conf}.

Perhaps you can add a cross-reference to the relevant part of the GnuPG
manual?  (With @pxref or similar.)

> +(define (insert-before pred lst1 lst2)
> +    "Return a list appending LST2 just before the first element on LST1 that
> + satisfy the predicate PRED."
> +    (cond
> +     ((null? lst1) lst2)
> +     ((pred (car lst1)) (append lst2 lst1))
> +     (else (cons (car lst1) (insert-before pred (cdr lst1) lst2)))))

I’d rather have it in (guix utils).  Also, please use ‘match’ and avoid
car/cdr as per
<https://guix.gnu.org/manual/devel/en/html_node/Data-Types-and-Pattern-Matching.html>.

>             (pam-service
>              (inherit pam)
> -            (auth (append (pam-service-auth pam)
> -                          (list optional-pam-mount)))
> -            (session (append (pam-service-session pam)
> -                             (list optional-pam-mount))))
> +            (auth (insert-before pam-gnupg-module?
> +                                 (pam-service-auth pam)
> +                                 (list optional-pam-mount)))
> +            (session (insert-before pam-gnupg-module?
> +                                   (pam-service-session pam)
> +                                   (list optional-pam-mount))))

Could you add a comment explaining why this ordering is important?

> +(define (pam-gnupg-module? name)
> +  "Return `#t' if NAME is the path to the pam-gnupg module, `#f' otherwise."
> + (equal? (pam-entry-module name)
> +         (file-append pam-gnupg "/lib/security/pam_gnupg.so")))

<package> records in general cannot be compared with ‘equal?’, so the
above procedure won’t work in the general case.  (It wouldn’t work with
custom variants of the ‘pam-gnupg’ package, too.)

Can you think of another way we could check whether a <pam-entry>
corresponds to ‘pam-gnupg’?

Thanks,
Ludo’.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.