Subject: [bug#78188] [PATCH v2] services: kwallet: New service.

From: Maxim Cournoyer <[email-masked]>
Date: Fri, 02 May 2025 08:59:13 +0900

Hi,

Sergio Pastor Pérez <[email-masked]> writes:

> Change-Id: I1330ce5e1648a8ddf6ddd507255a73335d6baa51
> ---
>  doc/guix.texi            | 37 ++++++++++++++++++++++++
>  gnu/services/desktop.scm | 61 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 98 insertions(+)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 7b418a4089..c6861b3182 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -27131,6 +27131,43 @@ Desktop Services
>  @end table
>  @end deftp
>  
> +@defvar kwallet-service-type
> +This is the type of the service that adds the
> +@uref{https://invent.kde.org/plasma/kwallet-pam, KWallet keyring}.  Its
> +value is a @code{kwallet-configuration} object (see below).  Note that,
> +contrary to @code{gnome-desktop-service-type},
> +@code{plasma-desktop-service-type} does not include this service.

Does gnome-desktop-service-type include the kwallet-service-type?  I
wouldn't think so.  You probably meant to say it "doesn't include a
wallet service in its default configuration." ?  Is this the normal
expectation for the KDE desktop?  I'd assume it comes with kwallet
pre-configured, if using Fedora for example.  If it does, we should
probably do so to avoid breaking users expectations.  I've recently made
the adjustment in GNOME to have the GNOME keyring unlocked by default
for the GNOME desktop, as that's what users expect.

> +This service adds the @code{kwallet-pam} package to the system profile
> +and extends PAM with entries using @code{pam_kwallet5.so},
> +unlocking a user's login keyring when they log in or setting its
> +password with passwd.

s/passwd/@command{passwd}/

> +@end defvar
> +
> +@deftp {Data Type} kwallet-configuration
> +Configuration record for the KWallet Keyring service.
> +
> +@table @asis
> +@item @code{keyring} (default: @code{kwallet-pam})
> +The KWallet keyring package to use.
> +
> +@item @code{pam-services}
> +A list of @code{(@var{service} . @var{kind})} pairs denoting PAM
> +services to extend, where @var{service} is the name of an existing
> +service to extend and @var{kind} is one of @code{login} or
> +@code{passwd}.

Perhaps mention "is one of the @code{login} or @{passwd} symbols.".  I
gues the quote is on the alist so individual values should not be
quoted, but just to avoid any ambiguity.

> +
> +If @code{login} is given, it adds an optional
> +@code{pam_kwallet5.so} to the auth block without arguments and to
> +the session block with @code{auto_start}.  If @code{passwd} is given, it
> +adds an optional @code{pam_kwallet5.so} to the password block
> +without arguments.
> +
> +By default, this field contains ``sddm'' with the value @code{login}
> +and ``passwd'' is with the value @code{passwd}.

Does KDE not have its own graphical desktop manager?  Perhaps it's not
yet ready in Guix?

[...]

> +;;;
> +;;; kwallet-service-type
> +;;;

nitpick: We conventionally add a '.' after these section names, as in:

;;;
;;; kwallet-service-type.
;;;

> +
> +(define-record-type* <kwallet-configuration> kwallet-configuration
> +  make-kwallet-configuration
> +  kwallet-configuration?
> +  (wallet kwallet-package (default kwallet-pam))
> +  (pam-services kwallet-pam-services (default '(("sddm" . login)
> +                                                ("passwd" . passwd)))))
> +
> +(define (pam-kwallet config)

Add add a brief docstring here saying this returns a PAM extension for
KWallet.

> +  (match config
> +    (#f '())                          ;explicitly disabled by user
> +    (_
> +     (define (%pam-keyring-entry . arguments)
> +       (pam-entry
> +        (control "optional")
> +        (module (file-append (kwallet-package config)
> +                             "/lib/security/pam_kwallet5.so"))
> +        (arguments arguments)))
> +
> +     (list
> +      (pam-extension
> +       (transformer
> +        (lambda (service)
> +          (case (assoc-ref (kwallet-pam-services config)
> +                           (pam-service-name service))
> +            ((login)
> +             (pam-service
> +              (inherit service)
> +              (auth (append (pam-service-auth service)
> +                            (list (%pam-keyring-entry))))
> +              (session (append (pam-service-session service)
> +                               (list (%pam-keyring-entry "auto_start"))))))
> +            ((passwd)
> +             (pam-service
> +              (inherit service)
> +              (password (append (pam-service-password service)
> +                                (list (%pam-keyring-entry))))))
> +            (else service)))))))))
> +
> +;; TODO: consider integrating service in `<plasma-desktop-configuration>' as
> +;; done in `<gnome-desktop-configuration>'. This requires rewritting the
> +;; `<plasma-desktop-service-type>' as done for `<gnome-desktop-service-type>'.

Ah, I see my comment above is acknowledged here as a TODO.  I'd
encourage you to pursue that next!

> +(define kwallet-service-type
> +  (service-type
> +   (name 'kwallet)
> +   (extensions (list
> +                (service-extension pam-root-service-type pam-kwallet)))
> +   (default-value (kwallet-configuration))
> +   (description "Return a service, that extends PAM with entries using

I'd drop the first comma.

> +@code{pam_kwallet5.so}, unlocking a user's login keyring when they
> log in or

s/a user's/the user's/

> +setting its password with passwd.")))

s/passwd/@command{passwd}/

Otherwise it LGTM.  Could you please send a v2?

-- 
Thanks,
Maxim