diff mbox series

[bug#56690] gnu: seatd-service-type: Should use seat group.

Message ID 87h72n24ra.fsf@muradm.net
State Accepted
Headers show
Series [bug#56690] gnu: seatd-service-type: Should use seat group. | expand

Checks

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

Commit Message

muradm Aug. 7, 2022, 8:05 p.m. UTC
here is updated patch:
- group is now correctly configurable
- dropped user field as it is mostlikely pointless
- group is created if necessary
- documentation updated adding mentioning of seatd.sock 
  permissions
- adding test case for seatd.sock ownership

thanks in advance,
muradm
muradm <mail@muradm.net> writes:

> [[PGP Signed Part:Undecided]]
>
> Hi,
>
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Hi,
>>
>> muradm <mail@muradm.net> skribis:
>>
>>> * gnu/services/desktop.scm (seatd-service-type): Uses "seat" 
>>> group.
>>> [extensions]: Added account-service-type with %seatd-accounts.
>>> (%seatd-accounts): List with "seat" group.
>>> (<seatd-configuration>): [group] Change default value to 
>>> "seat".
>>> * doc/guix.texi: Mention that users may need to become members 
>>> of
>>> "seat" group and update default value for group field.
>>
>> I guess I’m missing some context: is this fixing a bug 
>> currently
>> present?  (Apologies if this has been discussed elsewhere!)
>>
>
> Not really a bug, but misconfiguration i suppose. Started here 
> with
> commit about month or two ago:
>
> https://lists.gnu.org/archive/html/guix-devel/2022-08/msg00021.html
>
> Basically, with original configuration, greeter was in the wheel 
> group
> which allowed it to communicate with seatd over /run/seatd.sock.
>
>>> +Users which are going to interact with @code{seatd} daemon 
>>> while
>>> logged in
>>
>> s/which/who/
>>
>
> With above fix, wheel and other groups were removed. While it 
> was not
> affecting default greeter agretty, some people including me, use
> graphical greeter gtkgreet or others based on sway. Then sway 
> with
> greeter started by greetd needs to communicate with seatd. Due 
> to
> the fact of missing permission, greeter just dies with blank 
> screen.
>
> So "users which are going to interact" basically users who want
> to run sway, or anything else requiring libseat based seat 
> management
> present.
>
>>> +should be added to @code{seat} group. For instance:
>>> +
>>> +@lisp
>>> +(user-account
>>> +  (name "alice")
>>> +  (group "users")
>>> +  (supplementary-groups '("wheel"   ;allow use of sudo, etc.
>>> +                          "seat"    ;interact with seatd
>>> +                          "audio"   ;sound card
>>> +                          "video"   ;video devices such as 
>>> webcams
>>> +                          "cdrom")) ;the good ol' CD-ROM
>>> +  (comment "Bob's sister"))
>>
>> The problem I see with this extra doc is that even I wouldn’t 
>> know
>> how
>> to tell whether I’m going to “interact with seatd”. 
>> Fundamentally
>> it’s
>> not something I really care about.  :-)
>>
>> How could we improve on this?  Like, if this is important, 
>> should it
>> be
>> the default?
>>
>
> Two options, a) users who want greetd/seatd setup normally 
> advanced
> users wishing to get away from systemd/logind/dbus world, so 
> they
> probably was to be aware of what is going on; b) copy a piece of
> documentation from seatd, explaining seatd.sock maybe. Other 
> than
> that I could ask the same question about video, audio etc. 
> groups :)
>
>> Thanks,
>> Ludo’.
>
> [[End of PGP Signed Part]]

Comments

Liliana Marie Prikler Aug. 8, 2022, 6:08 a.m. UTC | #1
Am Sonntag, dem 07.08.2022 um 23:05 +0300 schrieb muradm:

> * gnu/services/desktop.scm (seatd-service-type): Uses "seat" group.
> [extensions]: Added account-service-type with seatd-accounts.
> (seatd-accounts): Conditionally produces list with "seat" group.
> (<seatd-configuration>):
> [user] Drop user field, since it is not going to be used.
Removed field.
> [group] Change default value to "seat".
> [existing-group?] Add field which controls if group should be
> created or not.
Would be Added field, but see below.
> * doc/guix.texi: Mention that users may need to become members of
> "seat" group and update default value for group field. Add
> explanation on seatd.sock file. Remove dropped user field.

> +When seat mamanagement is provided by @code{seatd}, users that
> acquire
management.
> +resources provided by @code{seatd} should have permissions to access
> +its UNIX domain socket. By default, @code{seatd-service-type}
> provides
> +``seat'' group. And user should become its member.
Which user?  Closely related, who acquires resources provided by
@code{seatd}?  Just the greeter?  A regular user logging in?
What access level is needed/provided?  Read access?  Write access?


> +  (group seatd-group (default "seat"))
> +  (existing-group? seatd-existing-group? (default #f))
AFAIK this is not necessary.  accounts-service-type can handle multiple
eq? groups, so as long as you're careful with what you put into group,
you shouldn't get an error.

Cheers
diff mbox series

Patch

From edf954714a71ea3c1b8a872df40ed3735dff10f8 Mon Sep 17 00:00:00 2001
From: muradm <mail@muradm.net>
Date: Fri, 22 Jul 2022 07:09:54 +0300
Subject: [PATCH v2] gnu: seatd-service-type: Should use seat group.
To: 56690@debbugs.gnu.org

* gnu/services/desktop.scm (seatd-service-type): Uses "seat" group.
[extensions]: Added account-service-type with seatd-accounts.
(seatd-accounts): Conditionally produces list with "seat" group.
(<seatd-configuration>):
[user] Drop user field, since it is not going to be used.
[group] Change default value to "seat".
[existing-group?] Add field which controls if group should be
created or not.
* doc/guix.texi: Mention that users may need to become members of
"seat" group and update default value for group field. Add
explanation on seatd.sock file. Remove dropped user field.
---
 doc/guix.texi            | 32 ++++++++++++++++++++++++++++----
 gnu/services/desktop.scm | 15 +++++++++++----
 gnu/tests/desktop.scm    |  9 +++++++++
 3 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index 21cee4e369..cb896fedb4 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -23139,6 +23139,29 @@  input), without requiring the applications needing access to be root.
   %base-services)
 
 @end lisp
+
+@code{seatd} operates over a UNIX domain socket, with @code{libseat}
+providing the client-side of the protocol. Then applications dealing
+with seat management (e.g. @code{sway}) connects to @code{seatd} via
+mentioned socket.
+
+When seat mamanagement is provided by @code{seatd}, users that acquire
+resources provided by @code{seatd} should have permissions to access
+its UNIX domain socket. By default, @code{seatd-service-type} provides
+``seat'' group. And user should become its member.
+
+@lisp
+(user-account
+  (name "alice")
+  (group "users")
+  (supplementary-groups '("wheel"   ;allow use of sudo, etc.
+                          "seat"    ;interact with seatd
+                          "audio"   ;sound card
+                          "video"   ;video devices such as webcams
+                          "cdrom")) ;the good ol' CD-ROM
+  (comment "Bob's sister"))
+@end lisp
+
 @end defvr
 
 @deftp {Data Type} seatd-configuration
@@ -23148,12 +23171,13 @@  Configuration record for the seatd daemon service.
 @item @code{seatd} (default: @code{seatd})
 The seatd package to use.
 
-@item @code{user} (default: @samp{"root"})
-User to own the seatd socket.
-
-@item @code{group} (default: @samp{"users"})
+@item @code{group} (default: @samp{"seat"})
 Group to own the seatd socket.
 
+@item @code{existing-group?} (default: @samp{#f})
+If group specified in @code{group} field is pre-existing,
+or should be created by @code{seatd-service-type}.
+
 @item @code{socket} (default: @samp{"/run/seatd.sock"})
 Where to create the seatd socket.
 
diff --git a/gnu/services/desktop.scm b/gnu/services/desktop.scm
index 29a3722f1b..9a36927b9f 100644
--- a/gnu/services/desktop.scm
+++ b/gnu/services/desktop.scm
@@ -13,7 +13,7 @@ 
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me@tobias.gr>
 ;;; Copyright © 2020 Reza Alizadeh Majd <r.majd@pantherx.org>
 ;;; Copyright © 2021 Brice Waegeneire <brice@waegenei.re>
-;;; Copyright © 2021 muradm <mail@muradm.net>
+;;; Copyright © 2021, 2022 muradm <mail@muradm.net>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -1645,8 +1645,8 @@  (define-record-type* <seatd-configuration> seatd-configuration
   make-seatd-configuration
   seatd-configuration?
   (seatd seatd-package (default seatd))
-  (user seatd-user (default "root"))
-  (group seatd-group (default "users"))
+  (group seatd-group (default "seat"))
+  (existing-group? seatd-existing-group? (default #f))
   (socket seatd-socket (default "/run/seatd.sock"))
   (logfile seatd-logfile (default "/var/log/seatd.log"))
   (loglevel seatd-loglevel (default "info")))
@@ -1660,7 +1660,6 @@  (define (seatd-shepherd-service config)
          (provision '(seatd elogind))
          (start #~(make-forkexec-constructor
                    (list #$(file-append (seatd-package config) "/bin/seatd")
-                         "-u" #$(seatd-user config)
                          "-g" #$(seatd-group config))
                    #:environment-variables
                    (list (string-append "SEATD_LOGLEVEL="
@@ -1670,6 +1669,13 @@  (define (seatd-shepherd-service config)
                    #:log-file #$(seatd-logfile config)))
          (stop #~(make-kill-destructor)))))
 
+(define seatd-accounts
+  (match-lambda
+    (($ <seatd-configuration> _ group existing-group?)
+     `(,@(if existing-group?  '() (list (user-group
+                                         (name group)
+                                         (system? #t))))))))
+
 (define seatd-environment
   (match-lambda
     (($ <seatd-configuration> _ _ _ socket)
@@ -1683,6 +1689,7 @@  (define seatd-service-type
 applications needing access to be root.")
    (extensions
     (list
+     (service-extension account-service-type seatd-accounts)
      (service-extension session-environment-service-type seatd-environment)
      ;; TODO: once cgroups is separate dependency we should not mount it here
      ;; for now it is mounted here, because elogind mounts it
diff --git a/gnu/tests/desktop.scm b/gnu/tests/desktop.scm
index 25971f9225..6fe6ec21be 100644
--- a/gnu/tests/desktop.scm
+++ b/gnu/tests/desktop.scm
@@ -255,6 +255,15 @@  (define (sock-var-sock var)
                    (socks (map wait-for-unix-socket-m socks)))
                 (and (= 2 (length socks)) (every identity socks)))))
 
+          (test-equal "seatd.sock ownership"
+            '("root" "seat")
+            `(,(marionette-eval
+                '(passwd:name (getpwuid (stat:uid (stat "/run/seatd.sock"))))
+                marionette)
+              ,(marionette-eval
+                '(group:name (getgrgid (stat:gid (stat "/run/seatd.sock"))))
+                marionette)))
+
           (test-assert "greetd is ready"
             (begin
               (marionette-type "ps -C greetd -o pid,args --no-headers > ps-greetd\n"
-- 
2.37.1