Hi,

the feature adds an easy way to integrate u2f user authentication to
rde setup and is inspired by rsauex personal config
(https://github.com/rsauex/dotfiles/blob/77e405cda4277e282725108528874b6d9ebee968/rsauex/services/pam-u2f.scm).
Although manual intervention is required to use it, in order to extract
the identifier from the security token. If the feature is approved, I can
add some documentation on how to use it in the manual, and also some
general notes on security token usage. I have made all arguments
optional for ease of use, this could also still be changed (don't now
what the general policy is therefore). Since the default control is
"sufficient", you won't be able to lock yourself out of your system,
even if no security-token is used.

On last thing. There is no license header in this file, would it be
suitable to add one?


---
 src/rde/features/security-token.scm | 58 +++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 8 deletions(-)

diff --git a/src/rde/features/security-token.scm b/src/rde/features/security-token.scm
index 94437733..b1993ed6 100644
--- a/src/rde/features/security-token.scm
+++ b/src/rde/features/security-token.scm
@@ -1,13 +1,16 @@
 (define-module (rde features security-token)
-  #:use-module (rde features)
-  #:use-module (gnu packages security-token)
-  #:use-module (gnu services)
-  #:use-module (gnu services base)
-  #:use-module (gnu services security-token)
-  #:use-module (rde system services accounts)
-  #:use-module (guix gexp)
+   #:use-module (gnu packages security-token)
+   #:use-module (gnu services base)
+   #:use-module (gnu services security-token)
+   #:use-module (gnu services)
+   #:use-module (gnu system pam)
+   #:use-module (guix gexp)
+   #:use-module (rde features predicates)
+   #:use-module (rde features)
+   #:use-module (rde system services accounts)
 
-  #:export (feature-security-token))
+   #:export (feature-security-token)
+   #:export (feature-pam-u2f-auth))
 
 (define (feature-security-token)
   "Add specific configuration to make security tokens work. It
@@ -30,3 +33,42 @@ includes the configuration to be able to use the token as a user
    (name 'security-token)
    (values `((security-token . #t)))
    (system-services-getter get-system-services)))
+
+(define* (feature-pam-u2f-auth
+          #:key
+          (control "sufficient") ;; for possible control flags: man pam.conf
+          (pam-modules '("login" "su" "sudo" "screen-locker" "swaylock" "greetd")) ;; ls /etc/pam.d/
+          (pam-entry-args '()))
+
+  "Configure pam modules to respect security token for authentication.
+   If not specified otherwise default location for u2fcfg is '~/.config/Yubico/u2f_keys'
+   and can be initialised with 'guix shell pam-u2f -- pamu2fcfg > ~/.config/Yubico/u2f_keys'."
+
+  (ensure-pred string? control)
+  (ensure-pred list-of-strings? pam-modules)
+  (ensure-pred list-of-strings? pam-entry-args)
+
+  (define f-name 'pam-u2f-auth)
+
+  (define (pam-u2f-auth-extension pam)
+    (if (member (pam-service-name pam) pam-modules)
+        (pam-service
+         (inherit pam)
+         (auth (cons* (pam-entry (control control)
+                                 (module (file-append (@ (gnu packages security-token) pam-u2f)
+                                                      "/lib/security/pam_u2f.so"))
+                                 (arguments pam-entry-args))
+                      (pam-service-auth pam))))
+        pam))
+
+  (define (get-system-services config)
+    (require-value 'security-token config)
+    (list
+     (simple-service 'pam-u2f pam-root-service-type
+                  (list (pam-extension
+                         (transformer pam-u2f-auth-extension))))))
+
+  (feature
+   (name f-name)
+   (values `((,f-name . #t)))
+   (system-services-getter get-system-services)))
-- 
2.41.0

-- 
Best regards,
Peter Kannewitz

On 2023-10-24 08:41, Peter Kannewitz wrote:

> Hi,

Hi Peter!

>
> the feature adds an easy way to integrate u2f user authentication to
> rde setup and is inspired by rsauex personal config
> (https://github.com/rsauex/dotfiles/blob/77e405cda4277e282725108528874b6d9ebee968/rsauex/services/pam-u2f.scm).
> Although manual intervention is required to use it, in order to extract
> the identifier from the security token. If the feature is approved, I can
> add some documentation on how to use it in the manual, and also some
> general notes on security token usage. 

It would be very useful!

> I have made all arguments optional for ease of use, this could also
> still be changed (don't now what the general policy is
> therefore). Since the default control is "sufficient", you won't be
> able to lock yourself out of your system, even if no security-token is
> used.
>
> On last thing. There is no license header in this file, would it be
> suitable to add one?

Yep.

>
>
> ---
>  src/rde/features/security-token.scm | 58 +++++++++++++++++++++++++----
>  1 file changed, 50 insertions(+), 8 deletions(-)
>
> diff --git a/src/rde/features/security-token.scm b/src/rde/features/security-token.scm
> index 94437733..b1993ed6 100644
> --- a/src/rde/features/security-token.scm
> +++ b/src/rde/features/security-token.scm
> @@ -1,13 +1,16 @@
>  (define-module (rde features security-token)
> -  #:use-module (rde features)
> -  #:use-module (gnu packages security-token)
> -  #:use-module (gnu services)
> -  #:use-module (gnu services base)
> -  #:use-module (gnu services security-token)
> -  #:use-module (rde system services accounts)
> -  #:use-module (guix gexp)
> +   #:use-module (gnu packages security-token)
> +   #:use-module (gnu services base)
> +   #:use-module (gnu services security-token)
> +   #:use-module (gnu services)
> +   #:use-module (gnu system pam)
> +   #:use-module (guix gexp)
> +   #:use-module (rde features predicates)
> +   #:use-module (rde features)
> +   #:use-module (rde system services accounts)
>  
> -  #:export (feature-security-token))
> +   #:export (feature-security-token)
> +   #:export (feature-pam-u2f-auth))
>  
>  (define (feature-security-token)
>    "Add specific configuration to make security tokens work. It
> @@ -30,3 +33,42 @@ includes the configuration to be able to use the token as a user
>     (name 'security-token)
>     (values `((security-token . #t)))
>     (system-services-getter get-system-services)))
> +
> +(define* (feature-pam-u2f-auth
> +          #:key
> +          (control "sufficient") ;; for possible control flags: man pam.conf
> +          (pam-modules '("login" "su" "sudo" "screen-locker" "swaylock" "greetd")) ;; ls /etc/pam.d/

It's better to keep lines under 80 characters.

> +          (pam-entry-args '()))
> +
> +  "Configure pam modules to respect security token for authentication.
> +   If not specified otherwise default location for u2fcfg is '~/.config/Yubico/u2f_keys'
> +   and can be initialised with 'guix shell pam-u2f -- pamu2fcfg > ~/.config/Yubico/u2f_keys'."

In Emacs you can use M-q for realigning docstrings.

> +
> +  (ensure-pred string? control)
> +  (ensure-pred list-of-strings? pam-modules)
> +  (ensure-pred list-of-strings? pam-entry-args)
> +
> +  (define f-name 'pam-u2f-auth)
> +
> +  (define (pam-u2f-auth-extension pam)
> +    (if (member (pam-service-name pam) pam-modules)
> +        (pam-service
> +         (inherit pam)
> +         (auth (cons* (pam-entry (control control)
> +                                 (module (file-append (@ (gnu packages security-token) pam-u2f)

There is no need to explicitly reference pam-u2f with full qualified
module path, because it's already imported in the header of the file.

> +                                                      "/lib/security/pam_u2f.so"))
> +                                 (arguments pam-entry-args))
> +                      (pam-service-auth pam))))
> +        pam))
> +
> +  (define (get-system-services config)
> +    (require-value 'security-token config)
> +    (list
> +     (simple-service 'pam-u2f pam-root-service-type
> +                  (list (pam-extension

It seems that the list is missaligned here.

> +                         (transformer pam-u2f-auth-extension))))))
> +
> +  (feature
> +   (name f-name)
> +   (values `((,f-name . #t)))
> +   (system-services-getter get-system-services)))
> -- 
> 2.41.0

The feature looks good, looking forward for the next revision!
Thank you for working on it.

-- 
Best regards,
Andrew Tropin

On 2023-10-24 18:34, Andrew Tropin wrote:

> On 2023-10-24 08:41, Peter Kannewitz wrote:
>> Hi,
>
> Hi Peter!
Hi Andrew :)
>> the feature adds an easy way to integrate u2f user authentication to
>> rde setup and is inspired by rsauex personal config
>> (https://github.com/rsauex/dotfiles/blob/77e405cda4277e282725108528874b6d9ebee968/rsauex/services/pam-u2f.scm).
>> Although manual intervention is required to use it, in order to extract
>> the identifier from the security token. If the feature is approved, I can
>> add some documentation on how to use it in the manual, and also some
>> general notes on security token usage. 
>
> It would be very useful!
Will do so in the next weeks. 
>> I have made all arguments optional for ease of use, this could also
>> still be changed (don't now what the general policy is
>> therefore). Since the default control is "sufficient", you won't be
>> able to lock yourself out of your system, even if no security-token is
>> used.
>>
>> On last thing. There is no license header in this file, would it be
>> suitable to add one?
>
> Yep.
I have copied one over. From commit history I assumed you contributed
the rest of the code.
>> ---
>>  src/rde/features/security-token.scm | 58 +++++++++++++++++++++++++----
>>  1 file changed, 50 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/rde/features/security-token.scm b/src/rde/features/security-token.scm
>> index 94437733..b1993ed6 100644
>> --- a/src/rde/features/security-token.scm
>> +++ b/src/rde/features/security-token.scm
>> @@ -1,13 +1,16 @@
>>  (define-module (rde features security-token)
>> -  #:use-module (rde features)
>> -  #:use-module (gnu packages security-token)
>> -  #:use-module (gnu services)
>> -  #:use-module (gnu services base)
>> -  #:use-module (gnu services security-token)
>> -  #:use-module (rde system services accounts)
>> -  #:use-module (guix gexp)
>> +   #:use-module (gnu packages security-token)
>> +   #:use-module (gnu services base)
>> +   #:use-module (gnu services security-token)
>> +   #:use-module (gnu services)
>> +   #:use-module (gnu system pam)
>> +   #:use-module (guix gexp)
>> +   #:use-module (rde features predicates)
>> +   #:use-module (rde features)
>> +   #:use-module (rde system services accounts)
>>  
>> -  #:export (feature-security-token))
>> +   #:export (feature-security-token)
>> +   #:export (feature-pam-u2f-auth))
>>  
>>  (define (feature-security-token)
>>    "Add specific configuration to make security tokens work. It
>> @@ -30,3 +33,42 @@ includes the configuration to be able to use the token as a user
>>     (name 'security-token)
>>     (values `((security-token . #t)))
>>     (system-services-getter get-system-services)))
>> +
>> +(define* (feature-pam-u2f-auth
>> +          #:key
>> +          (control "sufficient") ;; for possible control flags: man pam.conf
>> +          (pam-modules '("login" "su" "sudo" "screen-locker" "swaylock" "greetd")) ;; ls /etc/pam.d/
>
> It's better to keep lines under 80 characters.
Now I know what this indicator line is good for :)
>> +          (pam-entry-args '()))
>> +
>> +  "Configure pam modules to respect security token for authentication.
>> +   If not specified otherwise default location for u2fcfg is '~/.config/Yubico/u2f_keys'
>> +   and can be initialised with 'guix shell pam-u2f -- pamu2fcfg > ~/.config/Yubico/u2f_keys'."
>
> In Emacs you can use M-q for realigning docstrings.
Thanks didn't knew.
>> +
>> +  (ensure-pred string? control)
>> +  (ensure-pred list-of-strings? pam-modules)
>> +  (ensure-pred list-of-strings? pam-entry-args)
>> +
>> +  (define f-name 'pam-u2f-auth)
>> +
>> +  (define (pam-u2f-auth-extension pam)
>> +    (if (member (pam-service-name pam) pam-modules)
>> +        (pam-service
>> +         (inherit pam)
>> +         (auth (cons* (pam-entry (control control)
>> +                                 (module (file-append (@ (gnu packages security-token) pam-u2f)
>
> There is no need to explicitly reference pam-u2f with full qualified
> module path, because it's already imported in the header of the file.
Changed.
>> +                                                      "/lib/security/pam_u2f.so"))
>> +                                 (arguments pam-entry-args))
>> +                      (pam-service-auth pam))))
>> +        pam))
>> +
>> +  (define (get-system-services config)
>> +    (require-value 'security-token config)
>> +    (list
>> +     (simple-service 'pam-u2f pam-root-service-type
>> +                  (list (pam-extension
>
> It seems that the list is missaligned here.
Good catch, realigned.
>> +                         (transformer pam-u2f-auth-extension))))))
>> +
>> +  (feature
[ 5 more citation lines. Click/Enter to hide. ]
>> +   (name f-name)
>> +   (values `((,f-name . #t)))
>> +   (system-services-getter get-system-services)))
>> -- 
>> 2.41.0
>
> The feature looks good, looking forward for the next revision!
> Thank you for working on it.
It's a pleasure, here is the second try.

---
 src/rde/features/security-token.scm | 83 +++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 9 deletions(-)

diff --git a/src/rde/features/security-token.scm b/src/rde/features/security-token.scm
index 94437733..c0b8af09 100644
--- a/src/rde/features/security-token.scm
+++ b/src/rde/features/security-token.scm
@@ -1,13 +1,36 @@
-(define-module (rde features security-token)
-  #:use-module (rde features)
-  #:use-module (gnu packages security-token)
-  #:use-module (gnu services)
-  #:use-module (gnu services base)
-  #:use-module (gnu services security-token)
-  #:use-module (rde system services accounts)
-  #:use-module (guix gexp)
+;;; rde --- Reproducible development environment.
+;;;
+;;; Copyright © 2022 Andrew Tropin <andrew@trop.in>
+;;; Copyright © 2023 Peter Kannewitz <peter.kannewitz@posteo.net>
+;;;
+;;; This file is part of rde.
+;;;
+;;; rde is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; rde is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with rde.  If not, see <http://www.gnu.org/licenses/>.

-  #:export (feature-security-token))
+(define-module (rde features security-token)
+   #:use-module (gnu packages security-token)
+   #:use-module (gnu services base)
+   #:use-module (gnu services security-token)
+   #:use-module (gnu services)
+   #:use-module (gnu system pam)
+   #:use-module (guix gexp)
+   #:use-module (rde features predicates)
+   #:use-module (rde features)
+   #:use-module (rde system services accounts)
+
+   #:export (feature-security-token)
+   #:export (feature-pam-u2f-auth))

 (define (feature-security-token)
   "Add specific configuration to make security tokens work. It
@@ -30,3 +53,45 @@ includes the configuration to be able to use the token as a user
    (name 'security-token)
    (values `((security-token . #t)))
    (system-services-getter get-system-services)))
+
+(define* (feature-pam-u2f-auth
+          #:key
+          (control "sufficient") ;; for possible control flags: man pam.conf
+          (pam-modules
+           '("login" "su" "sudo" "screen-locker" "swaylock" "greetd"))
+          (pam-entry-args '()))
+
+  "Configure pam modules to respect security token for authentication.  If not
+specified otherwise default location for u2fcfg is '~/.config/Yubico/u2f-keys'
+and can be initialised with 'guix shell pam-u2f -- pamu2fcfg >
+~/.config/Yubico/u2f-keys'."
+
+  (ensure-pred string? control)
+  (ensure-pred list-of-strings? pam-modules)
+  (ensure-pred list-of-strings? pam-entry-args)
+
+  (define f-name 'pam-u2f-auth)
+
+  (define (pam-u2f-auth-extension pam)
+    (if (member (pam-service-name pam) pam-modules)
+        (pam-service
+         (inherit pam)
+         (auth
+          (cons*
+           (pam-entry (control control)
+                      (module (file-append pam-u2f "/lib/security/pam_u2f.so"))
+                      (arguments pam-entry-args))
+           (pam-service-auth pam))))
+        pam))
+
+  (define (get-system-services config)
+    (require-value 'security-token config)
+    (list
+     (simple-service 'pam-u2f pam-root-service-type
+                     (list (pam-extension
+                            (transformer pam-u2f-auth-extension))))))
+
+  (feature
+   (name f-name)
+   (values `((,f-name . #t)))
+   (system-services-getter get-system-services)))
-- 
2.41.0

-- 
Best regards,
Peter Kannewitz

On 2023-10-24 18:18, Peter Kannewitz wrote:

> On 2023-10-24 18:34, Andrew Tropin wrote:
>
>> On 2023-10-24 08:41, Peter Kannewitz wrote:
>>> Hi,
>>
>> Hi Peter!
> Hi Andrew :)
>>> the feature adds an easy way to integrate u2f user authentication to
>>> rde setup and is inspired by rsauex personal config
>>> (https://github.com/rsauex/dotfiles/blob/77e405cda4277e282725108528874b6d9ebee968/rsauex/services/pam-u2f.scm).
>>> Although manual intervention is required to use it, in order to extract
>>> the identifier from the security token. If the feature is approved, I can
>>> add some documentation on how to use it in the manual, and also some
>>> general notes on security token usage. 
>>
>> It would be very useful!
> Will do so in the next weeks. 
>>> I have made all arguments optional for ease of use, this could also
>>> still be changed (don't now what the general policy is
>>> therefore). Since the default control is "sufficient", you won't be
>>> able to lock yourself out of your system, even if no security-token is
>>> used.
>>>
>>> On last thing. There is no license header in this file, would it be
>>> suitable to add one?
>>
>> Yep.
> I have copied one over. From commit history I assumed you contributed
> the rest of the code.
>>> ---
>>>  src/rde/features/security-token.scm | 58 +++++++++++++++++++++++++----
>>>  1 file changed, 50 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/rde/features/security-token.scm b/src/rde/features/security-token.scm
>>> index 94437733..b1993ed6 100644
>>> --- a/src/rde/features/security-token.scm
>>> +++ b/src/rde/features/security-token.scm
>>> @@ -1,13 +1,16 @@
>>>  (define-module (rde features security-token)
>>> -  #:use-module (rde features)
>>> -  #:use-module (gnu packages security-token)
>>> -  #:use-module (gnu services)
>>> -  #:use-module (gnu services base)
>>> -  #:use-module (gnu services security-token)
>>> -  #:use-module (rde system services accounts)
>>> -  #:use-module (guix gexp)
>>> +   #:use-module (gnu packages security-token)
>>> +   #:use-module (gnu services base)
>>> +   #:use-module (gnu services security-token)
>>> +   #:use-module (gnu services)
>>> +   #:use-module (gnu system pam)
>>> +   #:use-module (guix gexp)
>>> +   #:use-module (rde features predicates)
>>> +   #:use-module (rde features)
>>> +   #:use-module (rde system services accounts)
>>>  
>>> -  #:export (feature-security-token))
>>> +   #:export (feature-security-token)
>>> +   #:export (feature-pam-u2f-auth))
>>>  
>>>  (define (feature-security-token)
>>>    "Add specific configuration to make security tokens work. It
>>> @@ -30,3 +33,42 @@ includes the configuration to be able to use the token as a user
>>>     (name 'security-token)
>>>     (values `((security-token . #t)))
>>>     (system-services-getter get-system-services)))
>>> +
>>> +(define* (feature-pam-u2f-auth
>>> +          #:key
>>> +          (control "sufficient") ;; for possible control flags: man pam.conf
>>> +          (pam-modules '("login" "su" "sudo" "screen-locker" "swaylock" "greetd")) ;; ls /etc/pam.d/
>>
>> It's better to keep lines under 80 characters.
> Now I know what this indicator line is good for :)
>>> +          (pam-entry-args '()))
>>> +
>>> +  "Configure pam modules to respect security token for authentication.
>>> +   If not specified otherwise default location for u2fcfg is '~/.config/Yubico/u2f_keys'
>>> +   and can be initialised with 'guix shell pam-u2f -- pamu2fcfg > ~/.config/Yubico/u2f_keys'."
>>
>> In Emacs you can use M-q for realigning docstrings.
> Thanks didn't knew.
>>> +
>>> +  (ensure-pred string? control)
>>> +  (ensure-pred list-of-strings? pam-modules)
>>> +  (ensure-pred list-of-strings? pam-entry-args)
>>> +
>>> +  (define f-name 'pam-u2f-auth)
>>> +
>>> +  (define (pam-u2f-auth-extension pam)
>>> +    (if (member (pam-service-name pam) pam-modules)
>>> +        (pam-service
>>> +         (inherit pam)
>>> +         (auth (cons* (pam-entry (control control)
>>> +                                 (module (file-append (@ (gnu packages security-token) pam-u2f)
>>
>> There is no need to explicitly reference pam-u2f with full qualified
>> module path, because it's already imported in the header of the file.
> Changed.
>>> +                                                      "/lib/security/pam_u2f.so"))
>>> +                                 (arguments pam-entry-args))
>>> +                      (pam-service-auth pam))))
>>> +        pam))
>>> +
>>> +  (define (get-system-services config)
>>> +    (require-value 'security-token config)
>>> +    (list
>>> +     (simple-service 'pam-u2f pam-root-service-type
>>> +                  (list (pam-extension
>>
>> It seems that the list is missaligned here.
> Good catch, realigned.
>>> +                         (transformer pam-u2f-auth-extension))))))
>>> +
>>> +  (feature
> [ 5 more citation lines. Click/Enter to hide. ]
>>> +   (name f-name)
>>> +   (values `((,f-name . #t)))
>>> +   (system-services-getter get-system-services)))
>>> -- 
>>> 2.41.0
>>
>> The feature looks good, looking forward for the next revision!
>> Thank you for working on it.
> It's a pleasure, here is the second try.
>
> ---
Everything above --- goes to commit message, everything just after ---
is ignored.

You can use it to add some comments on your patch, which are not
supposed to go to commit.

However, most of the time you just reply to review/comments with a plain
quoted email and send a new revision of the patch as a reply to the root
of thread.

Also, don't forget to leave empty lines around you inline comments
inside quoted text.  It's much easier to read it, when there is some
room.

>  src/rde/features/security-token.scm | 83 +++++++++++++++++++++++++----
>  1 file changed, 74 insertions(+), 9 deletions(-)
>
> diff --git a/src/rde/features/security-token.scm b/src/rde/features/security-token.scm
> index 94437733..c0b8af09 100644
> --- a/src/rde/features/security-token.scm
> +++ b/src/rde/features/security-token.scm
> @@ -1,13 +1,36 @@
> -(define-module (rde features security-token)
> -  #:use-module (rde features)
> -  #:use-module (gnu packages security-token)
> -  #:use-module (gnu services)
> -  #:use-module (gnu services base)
> -  #:use-module (gnu services security-token)
> -  #:use-module (rde system services accounts)
> -  #:use-module (guix gexp)
> +;;; rde --- Reproducible development environment.
> +;;;
> +;;; Copyright © 2022 Andrew Tropin <andrew@trop.in>
> +;;; Copyright © 2023 Peter Kannewitz <peter.kannewitz@posteo.net>
> +;;;
> +;;; This file is part of rde.
> +;;;
> +;;; rde is free software; you can redistribute it and/or modify it
> +;;; under the terms of the GNU General Public License as published by
> +;;; the Free Software Foundation; either version 3 of the License, or (at
> +;;; your option) any later version.
> +;;;
> +;;; rde is distributed in the hope that it will be useful, but
> +;;; WITHOUT ANY WARRANTY; without even the implied warranty of
> +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;;; GNU General Public License for more details.
> +;;;
> +;;; You should have received a copy of the GNU General Public License
> +;;; along with rde.  If not, see <http://www.gnu.org/licenses/>.
>
> -  #:export (feature-security-token))
> +(define-module (rde features security-token)
> +   #:use-module (gnu packages security-token)
> +   #:use-module (gnu services base)
> +   #:use-module (gnu services security-token)
> +   #:use-module (gnu services)
> +   #:use-module (gnu system pam)
> +   #:use-module (guix gexp)
> +   #:use-module (rde features predicates)
> +   #:use-module (rde features)
> +   #:use-module (rde system services accounts)
> +
> +   #:export (feature-security-token)
> +   #:export (feature-pam-u2f-auth))

There is no need for two exports, you can list all the exported items as
a part of one #:export.

>
>  (define (feature-security-token)
>    "Add specific configuration to make security tokens work. It
> @@ -30,3 +53,45 @@ includes the configuration to be able to use the token as a user
>     (name 'security-token)
>     (values `((security-token . #t)))
>     (system-services-getter get-system-services)))
> +
> +(define* (feature-pam-u2f-auth
> +          #:key
> +          (control "sufficient") ;; for possible control flags: man pam.conf
> +          (pam-modules
> +           '("login" "su" "sudo" "screen-locker" "swaylock" "greetd"))
> +          (pam-entry-args '()))
> +
> +  "Configure pam modules to respect security token for authentication.  If not
> +specified otherwise default location for u2fcfg is '~/.config/Yubico/u2f-keys'
> +and can be initialised with 'guix shell pam-u2f -- pamu2fcfg >
> +~/.config/Yubico/u2f-keys'."

It better to use texinfo quotes here: `'.

> +
> +  (ensure-pred string? control)
> +  (ensure-pred list-of-strings? pam-modules)
> +  (ensure-pred list-of-strings? pam-entry-args)
> +
> +  (define f-name 'pam-u2f-auth)
> +
> +  (define (pam-u2f-auth-extension pam)
> +    (if (member (pam-service-name pam) pam-modules)
> +        (pam-service
> +         (inherit pam)
> +         (auth
> +          (cons*
> +           (pam-entry (control control)
> +                      (module (file-append pam-u2f "/lib/security/pam_u2f.so"))
> +                      (arguments pam-entry-args))
> +           (pam-service-auth pam))))
> +        pam))
> +
> +  (define (get-system-services config)
> +    (require-value 'security-token config)
> +    (list
> +     (simple-service 'pam-u2f pam-root-service-type
> +                     (list (pam-extension
> +                            (transformer pam-u2f-auth-extension))))))
> +
> +  (feature
> +   (name f-name)
> +   (values `((,f-name . #t)))
> +   (system-services-getter get-system-services)))
> -- 
> 2.41.0

I can already merge it with minor tweaks, but it's better to send a v3
for the practice.

-- 
Best regards,
Andrew Tropin

On 2023-10-25 16:48, Andrew Tropin wrote:

> On 2023-10-24 18:18, Peter Kannewitz wrote:
>
>> On 2023-10-24 18:34, Andrew Tropin wrote:
>>
>>> On 2023-10-24 08:41, Peter Kannewitz wrote:
>>>> Hi,
>>>
>>> Hi Peter!
>> Hi Andrew :)
>>>> the feature adds an easy way to integrate u2f user authentication to
>>>> rde setup and is inspired by rsauex personal config
>>>> (https://github.com/rsauex/dotfiles/blob/77e405cda4277e282725108528874b6d9ebee968/rsauex/services/pam-u2f.scm).
>>>> Although manual intervention is required to use it, in order to extract
>>>> the identifier from the security token. If the feature is approved, I can
>>>> add some documentation on how to use it in the manual, and also some
>>>> general notes on security token usage. 
>>>
>>> It would be very useful!
>> Will do so in the next weeks. 
>>>> I have made all arguments optional for ease of use, this could also
>>>> still be changed (don't now what the general policy is
>>>> therefore). Since the default control is "sufficient", you won't be
>>>> able to lock yourself out of your system, even if no security-token is
>>>> used.
>>>>
>>>> On last thing. There is no license header in this file, would it be
>>>> suitable to add one?
>>>
>>> Yep.
>> I have copied one over. From commit history I assumed you contributed
>> the rest of the code.
>>>> ---
>>>>  src/rde/features/security-token.scm | 58 +++++++++++++++++++++++++----
>>>>  1 file changed, 50 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/src/rde/features/security-token.scm b/src/rde/features/security-token.scm
>>>> index 94437733..b1993ed6 100644
>>>> --- a/src/rde/features/security-token.scm
>>>> +++ b/src/rde/features/security-token.scm
>>>> @@ -1,13 +1,16 @@
>>>>  (define-module (rde features security-token)
>>>> -  #:use-module (rde features)
>>>> -  #:use-module (gnu packages security-token)
>>>> -  #:use-module (gnu services)
>>>> -  #:use-module (gnu services base)
>>>> -  #:use-module (gnu services security-token)
>>>> -  #:use-module (rde system services accounts)
>>>> -  #:use-module (guix gexp)
>>>> +   #:use-module (gnu packages security-token)
>>>> +   #:use-module (gnu services base)
>>>> +   #:use-module (gnu services security-token)
>>>> +   #:use-module (gnu services)
>>>> +   #:use-module (gnu system pam)
>>>> +   #:use-module (guix gexp)
>>>> +   #:use-module (rde features predicates)
>>>> +   #:use-module (rde features)
>>>> +   #:use-module (rde system services accounts)
>>>>  
>>>> -  #:export (feature-security-token))
>>>> +   #:export (feature-security-token)
>>>> +   #:export (feature-pam-u2f-auth))
>>>>  
>>>>  (define (feature-security-token)
>>>>    "Add specific configuration to make security tokens work. It
>>>> @@ -30,3 +33,42 @@ includes the configuration to be able to use the token as a user
>>>>     (name 'security-token)
>>>>     (values `((security-token . #t)))
>>>>     (system-services-getter get-system-services)))
>>>> +
>>>> +(define* (feature-pam-u2f-auth
>>>> +          #:key
>>>> +          (control "sufficient") ;; for possible control flags: man pam.conf
>>>> +          (pam-modules '("login" "su" "sudo" "screen-locker" "swaylock" "greetd")) ;; ls /etc/pam.d/
>>>
>>> It's better to keep lines under 80 characters.
>> Now I know what this indicator line is good for :)
>>>> +          (pam-entry-args '()))
>>>> +
>>>> +  "Configure pam modules to respect security token for authentication.
>>>> +   If not specified otherwise default location for u2fcfg is '~/.config/Yubico/u2f_keys'
>>>> +   and can be initialised with 'guix shell pam-u2f -- pamu2fcfg > ~/.config/Yubico/u2f_keys'."
>>>
>>> In Emacs you can use M-q for realigning docstrings.
>> Thanks didn't knew.
>>>> +
>>>> +  (ensure-pred string? control)
>>>> +  (ensure-pred list-of-strings? pam-modules)
>>>> +  (ensure-pred list-of-strings? pam-entry-args)
>>>> +
>>>> +  (define f-name 'pam-u2f-auth)
>>>> +
>>>> +  (define (pam-u2f-auth-extension pam)
>>>> +    (if (member (pam-service-name pam) pam-modules)
>>>> +        (pam-service
>>>> +         (inherit pam)
>>>> +         (auth (cons* (pam-entry (control control)
>>>> +                                 (module (file-append (@ (gnu packages security-token) pam-u2f)
>>>
>>> There is no need to explicitly reference pam-u2f with full qualified
>>> module path, because it's already imported in the header of the file.
>> Changed.
>>>> +                                                      "/lib/security/pam_u2f.so"))
>>>> +                                 (arguments pam-entry-args))
>>>> +                      (pam-service-auth pam))))
>>>> +        pam))
>>>> +
>>>> +  (define (get-system-services config)
>>>> +    (require-value 'security-token config)
>>>> +    (list
>>>> +     (simple-service 'pam-u2f pam-root-service-type
>>>> +                  (list (pam-extension
>>>
>>> It seems that the list is missaligned here.
>> Good catch, realigned.
>>>> +                         (transformer pam-u2f-auth-extension))))))
>>>> +
>>>> +  (feature
>> [ 5 more citation lines. Click/Enter to hide. ]
>>>> +   (name f-name)
>>>> +   (values `((,f-name . #t)))
>>>> +   (system-services-getter get-system-services)))
>>>> -- 
>>>> 2.41.0
>>>
>>> The feature looks good, looking forward for the next revision!
>>> Thank you for working on it.
>> It's a pleasure, here is the second try.
>>
>> ---
> Everything above --- goes to commit message, everything just after ---
> is ignored.
>
> You can use it to add some comments on your patch, which are not
> supposed to go to commit.
>
> However, most of the time you just reply to review/comments with a plain
> quoted email and send a new revision of the patch as a reply to the root
> of thread.
>
> Also, don't forget to leave empty lines around you inline comments
> inside quoted text.  It's much easier to read it, when there is some
> room.
>

Thank you for your welcoming help! 

>>  src/rde/features/security-token.scm | 83 +++++++++++++++++++++++++----
>>  1 file changed, 74 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/rde/features/security-token.scm b/src/rde/features/security-token.scm
>> index 94437733..c0b8af09 100644
>> --- a/src/rde/features/security-token.scm
>> +++ b/src/rde/features/security-token.scm
>> @@ -1,13 +1,36 @@
>> -(define-module (rde features security-token)
>> -  #:use-module (rde features)
>> -  #:use-module (gnu packages security-token)
>> -  #:use-module (gnu services)
>> -  #:use-module (gnu services base)
>> -  #:use-module (gnu services security-token)
>> -  #:use-module (rde system services accounts)
>> -  #:use-module (guix gexp)
>> +;;; rde --- Reproducible development environment.
>> +;;;
>> +;;; Copyright © 2022 Andrew Tropin <andrew@trop.in>
>> +;;; Copyright © 2023 Peter Kannewitz <peter.kannewitz@posteo.net>
>> +;;;
>> +;;; This file is part of rde.
>> +;;;
>> +;;; rde is free software; you can redistribute it and/or modify it
>> +;;; under the terms of the GNU General Public License as published by
>> +;;; the Free Software Foundation; either version 3 of the License, or (at
>> +;;; your option) any later version.
>> +;;;
>> +;;; rde is distributed in the hope that it will be useful, but
>> +;;; WITHOUT ANY WARRANTY; without even the implied warranty of
>> +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +;;; GNU General Public License for more details.
>> +;;;
>> +;;; You should have received a copy of the GNU General Public License
>> +;;; along with rde.  If not, see <http://www.gnu.org/licenses/>.
>>
>> -  #:export (feature-security-token))
>> +(define-module (rde features security-token)
>> +   #:use-module (gnu packages security-token)
>> +   #:use-module (gnu services base)
>> +   #:use-module (gnu services security-token)
>> +   #:use-module (gnu services)
>> +   #:use-module (gnu system pam)
>> +   #:use-module (guix gexp)
>> +   #:use-module (rde features predicates)
>> +   #:use-module (rde features)
>> +   #:use-module (rde system services accounts)
>> +
>> +   #:export (feature-security-token)
>> +   #:export (feature-pam-u2f-auth))
>
> There is no need for two exports, you can list all the exported items as
> a part of one #:export.
>
>>
>>  (define (feature-security-token)
>>    "Add specific configuration to make security tokens work. It
>> @@ -30,3 +53,45 @@ includes the configuration to be able to use the token as a user
>>     (name 'security-token)
>>     (values `((security-token . #t)))
>>     (system-services-getter get-system-services)))
>> +
>> +(define* (feature-pam-u2f-auth
>> +          #:key
>> +          (control "sufficient") ;; for possible control flags: man pam.conf
>> +          (pam-modules
>> +           '("login" "su" "sudo" "screen-locker" "swaylock" "greetd"))
>> +          (pam-entry-args '()))
>> +
>> +  "Configure pam modules to respect security token for authentication.  If not
>> +specified otherwise default location for u2fcfg is '~/.config/Yubico/u2f-keys'
>> +and can be initialised with 'guix shell pam-u2f -- pamu2fcfg >
>> +~/.config/Yubico/u2f-keys'."
>
> It better to use texinfo quotes here: `'.
>
>> +
>> +  (ensure-pred string? control)
>> +  (ensure-pred list-of-strings? pam-modules)
>> +  (ensure-pred list-of-strings? pam-entry-args)
>> +
>> +  (define f-name 'pam-u2f-auth)
>> +
>> +  (define (pam-u2f-auth-extension pam)
>> +    (if (member (pam-service-name pam) pam-modules)
>> +        (pam-service
>> +         (inherit pam)
>> +         (auth
>> +          (cons*
>> +           (pam-entry (control control)
>> +                      (module (file-append pam-u2f "/lib/security/pam_u2f.so"))
>> +                      (arguments pam-entry-args))
>> +           (pam-service-auth pam))))
>> +        pam))
>> +
>> +  (define (get-system-services config)
>> +    (require-value 'security-token config)
>> +    (list
>> +     (simple-service 'pam-u2f pam-root-service-type
>> +                     (list (pam-extension
>> +                            (transformer pam-u2f-auth-extension))))))
>> +
>> +  (feature
>> +   (name f-name)
>> +   (values `((,f-name . #t)))
>> +   (system-services-getter get-system-services)))
>> -- 
>> 2.41.0
>
> I can already merge it with minor tweaks, but it's better to send a v3
> for the practice.

-- 
Best regards,
Peter Kannewitz

---
 src/rde/features/security-token.scm | 83 +++++++++++++++++++++++++----
 1 file changed, 74 insertions(+), 9 deletions(-)

diff --git a/src/rde/features/security-token.scm b/src/rde/features/security-token.scm
index 94437733..ca69940a 100644
--- a/src/rde/features/security-token.scm
+++ b/src/rde/features/security-token.scm
@@ -1,13 +1,36 @@
-(define-module (rde features security-token)
-  #:use-module (rde features)
-  #:use-module (gnu packages security-token)
-  #:use-module (gnu services)
-  #:use-module (gnu services base)
-  #:use-module (gnu services security-token)
-  #:use-module (rde system services accounts)
-  #:use-module (guix gexp)
+;;; rde --- Reproducible development environment.
+;;;
+;;; Copyright © 2022 Andrew Tropin <andrew@trop.in>
+;;; Copyright © 2023 Peter Kannewitz <peter.kannewitz@posteo.net>
+;;;
+;;; This file is part of rde.
+;;;
+;;; rde is free software; you can redistribute it and/or modify it
+;;; under the terms of the GNU General Public License as published by
+;;; the Free Software Foundation; either version 3 of the License, or (at
+;;; your option) any later version.
+;;;
+;;; rde is distributed in the hope that it will be useful, but
+;;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;;; GNU General Public License for more details.
+;;;
+;;; You should have received a copy of the GNU General Public License
+;;; along with rde.  If not, see <http://www.gnu.org/licenses/>.
 
-  #:export (feature-security-token))
+(define-module (rde features security-token)
+   #:use-module (gnu packages security-token)
+   #:use-module (gnu services base)
+   #:use-module (gnu services security-token)
+   #:use-module (gnu services)
+   #:use-module (gnu system pam)
+   #:use-module (guix gexp)
+   #:use-module (rde features predicates)
+   #:use-module (rde features)
+   #:use-module (rde system services accounts)
+
+   #:export (feature-pam-u2f-auth
+             feature-security-token))
 
 (define (feature-security-token)
   "Add specific configuration to make security tokens work. It
@@ -30,3 +53,45 @@ includes the configuration to be able to use the token as a user
    (name 'security-token)
    (values `((security-token . #t)))
    (system-services-getter get-system-services)))
+
+(define* (feature-pam-u2f-auth
+          #:key
+          (control "sufficient") ;; for possible control flags: man pam.conf
+          (pam-modules
+           '("login" "su" "sudo" "screen-locker" "swaylock" "greetd"))
+          (pam-entry-args '()))
+
+  "Configure pam modules to respect security token for authentication.  If not
+specified otherwise default location for u2fcfg is `~/.config/Yubico/u2f-keys'
+and can be initialised with `guix shell pam-u2f -- pamu2fcfg >
+~/.config/Yubico/u2f-keys'."
+
+  (ensure-pred string? control)
+  (ensure-pred list-of-strings? pam-modules)
+  (ensure-pred list-of-strings? pam-entry-args)
+
+  (define f-name 'pam-u2f-auth)
+
+  (define (pam-u2f-auth-extension pam)
+    (if (member (pam-service-name pam) pam-modules)
+        (pam-service
+         (inherit pam)
+         (auth
+          (cons*
+           (pam-entry (control control)
+                      (module (file-append pam-u2f "/lib/security/pam_u2f.so"))
+                      (arguments pam-entry-args))
+           (pam-service-auth pam))))
+        pam))
+
+  (define (get-system-services config)
+    (require-value 'security-token config)
+    (list
+     (simple-service 'pam-u2f pam-root-service-type
+                     (list (pam-extension
+                            (transformer pam-u2f-auth-extension))))))
+
+  (feature
+   (name f-name)
+   (values `((,f-name . #t)))
+   (system-services-getter get-system-services)))
-- 
2.41.0
 

-- 
Best regards,
Peter Kannewitz

On 2023-10-25 16:37, Peter Kannewitz wrote:

> ---
>  src/rde/features/security-token.scm | 83 +++++++++++++++++++++++++----
>  1 file changed, 74 insertions(+), 9 deletions(-)
>
> diff --git a/src/rde/features/security-token.scm b/src/rde/features/security-token.scm
> index 94437733..ca69940a 100644
> --- a/src/rde/features/security-token.scm
> +++ b/src/rde/features/security-token.scm
> @@ -1,13 +1,36 @@
> -(define-module (rde features security-token)
> -  #:use-module (rde features)
> -  #:use-module (gnu packages security-token)
> -  #:use-module (gnu services)
> -  #:use-module (gnu services base)
> -  #:use-module (gnu services security-token)
> -  #:use-module (rde system services accounts)
> -  #:use-module (guix gexp)
> +;;; rde --- Reproducible development environment.
> +;;;
> +;;; Copyright © 2022 Andrew Tropin <andrew@trop.in>
> +;;; Copyright © 2023 Peter Kannewitz <peter.kannewitz@posteo.net>
> +;;;
> +;;; This file is part of rde.
> +;;;
> +;;; rde is free software; you can redistribute it and/or modify it
> +;;; under the terms of the GNU General Public License as published by
> +;;; the Free Software Foundation; either version 3 of the License, or (at
> +;;; your option) any later version.
> +;;;
> +;;; rde is distributed in the hope that it will be useful, but
> +;;; WITHOUT ANY WARRANTY; without even the implied warranty of
> +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +;;; GNU General Public License for more details.
> +;;;
> +;;; You should have received a copy of the GNU General Public License
> +;;; along with rde.  If not, see <http://www.gnu.org/licenses/>.
>  
> -  #:export (feature-security-token))
> +(define-module (rde features security-token)
> +   #:use-module (gnu packages security-token)
> +   #:use-module (gnu services base)
> +   #:use-module (gnu services security-token)
> +   #:use-module (gnu services)
> +   #:use-module (gnu system pam)
> +   #:use-module (guix gexp)
> +   #:use-module (rde features predicates)
> +   #:use-module (rde features)
> +   #:use-module (rde system services accounts)
> +
> +   #:export (feature-pam-u2f-auth
> +             feature-security-token))
>  
>  (define (feature-security-token)
>    "Add specific configuration to make security tokens work. It
> @@ -30,3 +53,45 @@ includes the configuration to be able to use the token as a user
>     (name 'security-token)
>     (values `((security-token . #t)))
>     (system-services-getter get-system-services)))
> +
> +(define* (feature-pam-u2f-auth
> +          #:key
> +          (control "sufficient") ;; for possible control flags: man pam.conf
> +          (pam-modules
> +           '("login" "su" "sudo" "screen-locker" "swaylock" "greetd"))
> +          (pam-entry-args '()))
> +
> +  "Configure pam modules to respect security token for authentication.  If not
> +specified otherwise default location for u2fcfg is `~/.config/Yubico/u2f-keys'
> +and can be initialised with `guix shell pam-u2f -- pamu2fcfg >
> +~/.config/Yubico/u2f-keys'."
> +
> +  (ensure-pred string? control)
> +  (ensure-pred list-of-strings? pam-modules)
> +  (ensure-pred list-of-strings? pam-entry-args)
> +
> +  (define f-name 'pam-u2f-auth)
> +
> +  (define (pam-u2f-auth-extension pam)
> +    (if (member (pam-service-name pam) pam-modules)
> +        (pam-service
> +         (inherit pam)
> +         (auth
> +          (cons*
> +           (pam-entry (control control)
> +                      (module (file-append pam-u2f "/lib/security/pam_u2f.so"))
> +                      (arguments pam-entry-args))
> +           (pam-service-auth pam))))
> +        pam))
> +
> +  (define (get-system-services config)
> +    (require-value 'security-token config)
> +    (list
> +     (simple-service 'pam-u2f pam-root-service-type
> +                     (list (pam-extension
> +                            (transformer pam-u2f-auth-extension))))))
> +
> +  (feature
> +   (name f-name)
> +   (values `((,f-name . #t)))
> +   (system-services-getter get-system-services)))
> -- 
> 2.41.0

LGTM, applied, pushed.  Congrats with your first contribution to RDE! :)

Thank you for your work!

-- 
Best regards,
Andrew Tropin