| Message ID | 4AtymQ5ic7YPCQjgRG3Dj73aZuO_Rx7GX8YSKBPeoVoOG_Z8LjXXbqvvfaq-ap0fgLADcsE8zibqDwkO7kazYXa0eMA3EeEaiU_6wGQ0yI8=@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#56797] gnu: services: fprintd: Add PAM configuration. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/comparison | success | View comparision |
| cbaines/git-branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
On 27-07-2022 17:57, Maya via Guix-patches via wrote: > + (let ((fprintd-module > + #~(string-append #$(fprintd-configuration-fprintd config) "/lib/security/pam_fprintd.so"))) This can be simplified to (let ((fprintd-module (file-append (fprintd-configuration-fprintd config) "/lib/security/pam_fprintd.so"))) Greetings, Maxime.
On 27-07-2022 17:57, Maya via Guix-patches via wrote:
> + #:login-uid? #t))
What's this line for? I'm not finding 'login-uid?' anywhere in the
manual, a comment would be in order.
Greetings,
Maxie.
On 27-07-2022 17:57, Maya via Guix-patches via wrote: > Added a feature to fprintd-service-type to allow unlocking PAM modules (ie. gdm login, gnome polkit etc.) by fingerprint. > > --- Documentation is missing (in the manual), so as-is, this new feature is hard to find. Also, the manual required giving every top-level procedure a docstring IIRC, > gnu/services/authentication.scm | 49 +++++++++++++++++++++++++++++++-- > 1 file changed, 46 insertions(+), 3 deletions(-) > > diff --git a/gnu/services/authentication.scm b/gnu/services/authentication.scm > index f7becdfafb..5737c15f4c 100644 > --- a/gnu/services/authentication.scm > +++ b/gnu/services/authentication.scm > @@ -44,9 +44,50 @@ (define-module (gnu services authentication) > nslcd-configuration? > nslcd-service-type)) > > -(define-configuration fprintd-configuration > +(define-configuration/no-serialization fprintd-configuration > (fprintd (file-like fprintd) > - "The fprintd package")) > + "The fprintd package") > + (unlock-gdm? > + (boolean #t) > + "Generate PAM configuration that unlocks gdm with fprintd.") > + (unlock-other > + (list '("polkit-1" "sddm")) ;; polkit-1 is the name of a PAM module for GNOME polkit > + "List of other PAM modules that can be unlocked with fprintd. > + > +This depends on your desktop configuration. If you for example want GNOME prompts to be unlocked by fingerprint, you add @code{polkit-1} to this list. (This is enabled by default.) > +")) This documentation is unclear -- does this field need to be set to the _name_ of the module, or to the _file name_ of the _shared library_ (as a file-like, not a direct file name, because of staging), or ...? Also, the 'list' check can be more precise, IIRC there was some method for not just using list? but doing things like list-of-strings?. Anyway, I don't really know PAM, but I've written some comments on the patch, hopefullt they are useful.
>This can be simplified to > > (let ((fprintd-module (file-append (fprintd-configuration-fprintd >config) "/lib/security/pam_fprintd.so"))) Yes, thank you, I am not yet that great with my guix-fu. > > + #:login-uid? #t)) > What's this line for? I'm not finding 'login-uid?' anywhere in the > manual, a comment would be in order. I've got this from the unix-pam-service and from gdm-service-type. The code this refers to in gnu/system/pam.scm: ,@(if login-uid? (list (pam-entry ;to fill in /proc/self/loginuid (control "required") (module "pam_loginuid.so"))) '()) gdm-service-type uses it in all 3 of it's pam modules. So I figured it ought to be there. I can investigate further, but it seems like I should not touch it. > Documentation is missing (in the manual), so as-is, this new feature is > hard to find. Oh? I didn't know that. Doesn't define-configuration generate documentation automatically? If it does not, I will hapilly add it, but I have never written any, so it will be a learning process. > Also, the manual required giving every top-level procedure a docstring > IIRC, There is that requirement, yes. But there weren't any around this method so I thought the configuration sufficed, but if it is a requirement, I will do that. > > gnu/services/authentication.scm | 49 +++++++++++++++++++++++++++++++-- > > 1 file changed, 46 insertions(+), 3 deletions(-) > > > > diff --git a/gnu/services/authentication.scm b/gnu/services/authentication.scm > > index f7becdfafb..5737c15f4c 100644 > > --- a/gnu/services/authentication.scm > > +++ b/gnu/services/authentication.scm > > @@ -44,9 +44,50 @@ (define-module (gnu services authentication) > > nslcd-configuration? > > nslcd-service-type)) > > > > -(define-configuration fprintd-configuration > > +(define-configuration/no-serialization fprintd-configuration > > (fprintd (file-like fprintd) > > - "The fprintd package")) > > + "The fprintd package") > > + (unlock-gdm? > > + (boolean #t) > > + "Generate PAM configuration that unlocks gdm with fprintd.") > > + (unlock-other > > + (list '("polkit-1" "sddm")) ;; polkit-1 is the name of a PAM module for GNOME polkit > > + "List of other PAM modules that can be unlocked with fprintd. > > + > > +This depends on your desktop configuration. If you for example want GNOME prompts to be unlocked by fingerprint, you add @code{polkit-1} to this list. (This is enabled by default.) > +")) > This documentation is unclear -- does this field need to be set to the > _name_ of the module, or to the _file name_ of the _shared library_ (as > a file-like, not a direct file name, because of staging), or ...? Also, > the 'list' check can be more precise, IIRC there was some method for not > just using list? but doing things like list-of-strings?. The name of the pam module, not a shared library. So the file in /etc/pam.d. It is a direct name, since it is not inside the store, pam modules have static path. As for the configuration options, it's my first time using them and I didn't really understand the define-syntax definition, so I really just skimmed through the guix repository for some uses. > Anyway, I don't really know PAM, but I've written some comments on the > patch, hopefully they are useful. They are a lot! Thank you very much. I hope those comments will be less needed in the future, as I become better as a contributor. With all the best for tomorrow and all the days to come, Maya.
On 27-07-2022 22:26, Maya wrote: >> Documentation is missing (in the manual), so as-is, this new feature is >> hard to find. > Oh? I didn't know that. Doesn't define-configuration generate documentation automatically? If it does not, I will hapilly add it, but I have never written any, so it will be a learning process. > There is some procedure that takes a record type and generates some documentation, but it is not automatically copied into the manual, you will have to do that yourself (and maybe tweak the result a little: what's a good docstring in code doesn't always fit very well in a manual). >> Also, the manual required giving every top-level procedure a docstring >> IIRC, > There is that requirement, yes. But there weren't any around this method so I thought the configuration sufficed, but if it is a requirement, I will do that. > I don't know if the requirement is overly strictly formulated or if the surrounding code is wrong. >> This documentation is unclear -- does this field need to be set to the >> _name_ of the module, or to the_file name_ of the_shared library_ (as >> a file-like, not a direct file name, because of staging), or ...? Also, >> the 'list' check can be more precise, IIRC there was some method for not >> just using list? but doing things like list-of-strings?. > The name of the pam module, not a shared library. So the file in /etc/pam.d. It is a direct name, since it is not inside the store, pam modules have static path. To be clear, it is clear if you look at the default value, but I think it's best to be explicit in the documentation. > As for the configuration options, it's my first time using them and I didn't really understand the define-syntax definition, so I really just skimmed through the guix repository for some uses. IIRC, there are some procedures you can use to define list-of-x? procedures but I don't recall the details. Greetings, Maxime.
Hi Maya, Could you send an updated patch taking Maxime’s suggestions into account? Let us know here or on IRC if you need guidance. Thanks for your work! Ludo’.
diff --git a/gnu/services/authentication.scm b/gnu/services/authentication.scm index f7becdfafb..5737c15f4c 100644 --- a/gnu/services/authentication.scm +++ b/gnu/services/authentication.scm @@ -44,9 +44,50 @@ (define-module (gnu services authentication) nslcd-configuration? nslcd-service-type)) -(define-configuration fprintd-configuration +(define-configuration/no-serialization fprintd-configuration (fprintd (file-like fprintd) - "The fprintd package")) + "The fprintd package") + (unlock-gdm? + (boolean #t) + "Generate PAM configuration that unlocks gdm with fprintd.") + (unlock-other + (list '("polkit-1" "sddm")) ;; polkit-1 is the name of a PAM module for GNOME polkit + "List of other PAM modules that can be unlocked with fprintd. + +This depends on your desktop configuration. If you for example want GNOME prompts to be unlocked by fingerprint, you add @code{polkit-1} to this list. (This is enabled by default.) +")) + +(define (fprintd-pam-other-services config fprintd-module) + (lambda (pam) + (if (member (pam-service-name pam) + (fprintd-configuration-unlock-other config)) + (let ((sufficient + (pam-entry + (control "sufficient") + (module fprintd-module)))) + (pam-service + (inherit pam) + (auth (cons sufficient (pam-service-auth pam))))) + pam))) + +(define (fprintd-pam-gdm-services fprintd-module) + (list + (pam-service + (inherit (unix-pam-service "gdm-fingerprint" + #:login-uid? #t)) + (auth (list + (pam-entry + (control "required") + (module fprintd-module))))))) + +(define (fprintd-pam-services config) + (let ((fprintd-module + #~(string-append #$(fprintd-configuration-fprintd config) "/lib/security/pam_fprintd.so"))) + (cons + (fprintd-pam-other-services config fprintd-module) + (if fprintd-configuration-unlock-gdm? + (fprintd-pam-gdm-services fprintd-module) + '())))) (define (fprintd-dbus-service config) (list (fprintd-configuration-fprintd config))) @@ -57,7 +98,9 @@ (define fprintd-service-type (list (service-extension dbus-root-service-type fprintd-dbus-service) (service-extension polkit-service-type - fprintd-dbus-service))) + fprintd-dbus-service) + (service-extension pam-root-service-type + fprintd-pam-services))) (default-value (fprintd-configuration)) (description "Run fprintd, a fingerprint management daemon.")))