diff mbox series

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

Message ID 20220722042745.26745-1-mail@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 July 22, 2022, 4:27 a.m. UTC
* 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.
---
 doc/guix.texi            | 18 +++++++++++++++++-
 gnu/services/desktop.scm |  8 ++++++--
 2 files changed, 23 insertions(+), 3 deletions(-)

Comments

\( July 24, 2022, 4:28 p.m. UTC | #1
Because patches with replies are more likely to be visible: LGTM :)

    -- (
Liliana Marie Prikler Aug. 5, 2022, 8:10 a.m. UTC | #2
Am Freitag, dem 22.07.2022 um 07:27 +0300 schrieb muradm:
> * 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.
Note, that your current patch adds a little asymmetry.  Even if you
configure seatd to use a group different from seat, a (now useless)
seat group will be created.

There are (at least) two possible fixes for this:
1. Disable configuration for the group altogether, marking the field as
deprecated.
2. Change the field into one that accepts a group.  Also sanitize the
field so that if a string such as "seat" is provided, it is turned into
a group.  Then make seatd-accounts return this group.

Cheers
Ludovic Courtès Aug. 6, 2022, 8:46 p.m. UTC | #3
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!)

> +Users which are going to interact with @code{seatd} daemon while logged in

s/which/who/

> +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?

Thanks,
Ludo’.
\( Aug. 6, 2022, 8:50 p.m. UTC | #4
On Sat Aug 6, 2022 at 9:46 PM BST, Ludovic Courtès wrote:
> I guess I’m missing some context: is this fixing a bug currently
> present?  (Apologies if this has been discussed elsewhere!)

This is one of two patches that fix a problem where any greetd greeter
more complex than agreety hangs on boot, basically rendering greetd
useless. I think the underlying cause is their being unable to connect
to seatd.sock?

At least, that's the symptom I know about. I'm not sure whether there
are others.

    -- (
muradm Aug. 7, 2022, 5:28 p.m. UTC | #5
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’.
muradm Aug. 7, 2022, 8:45 p.m. UTC | #6
Fixed in v2.

Liliana Marie Prikler <liliana.prikler@ist.tugraz.at> writes:

> Am Freitag, dem 22.07.2022 um 07:27 +0300 schrieb muradm:
>> * 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.
> Note, that your current patch adds a little asymmetry.  Even if 
> you
> configure seatd to use a group different from seat, a (now 
> useless)
> seat group will be created.
>
> There are (at least) two possible fixes for this:
> 1. Disable configuration for the group altogether, marking the 
> field as
> deprecated.
> 2. Change the field into one that accepts a group.  Also 
> sanitize the
> field so that if a string such as "seat" is provided, it is 
> turned into
> a group.  Then make seatd-accounts return this group.
>
> Cheers
Ludovic Courtès Aug. 8, 2022, 8:58 a.m. UTC | #7
Hi,

"(" <paren@disroot.org> skribis:

> On Sat Aug 6, 2022 at 9:46 PM BST, Ludovic Courtès wrote:
>> I guess I’m missing some context: is this fixing a bug currently
>> present?  (Apologies if this has been discussed elsewhere!)
>
> This is one of two patches that fix a problem where any greetd greeter
> more complex than agreety hangs on boot, basically rendering greetd
> useless. I think the underlying cause is their being unable to connect
> to seatd.sock?
>
> At least, that's the symptom I know about. I'm not sure whether there
> are others.

Is there a bug report, and do we have system tests for this
functionality?

I admit I know little about greetd and cases where it might be used.
Having system tests for that would help make sure the relevant
functionality works.

Thanks,
Ludo’.
\( Aug. 8, 2022, 9:12 a.m. UTC | #8
On Mon Aug 8, 2022 at 9:58 AM BST, Ludovic Courtès wrote:
> Is there a bug report, and do we have system tests for this
> functionality?

I don't believe there are system tests for greetd, no. There is
a bug report, though: <https://issues.guix.gnu.org/56971>.

> I admit I know little about greetd and cases where it might be used.

As I understand it, greetd is a daemon that handles the sensitive parts
of display managers, which it calls 'greeters'. It allows you to write a
login program without having to write those difficult and sensitive parts
by simply writing a GUI that sends JSON messages to the socket when it
gets input.

So the problem is some greeters try to talk to seatd, but since they
don't have the right permissions, they bail out.

    -- (
muradm Aug. 8, 2022, 6:55 p.m. UTC | #9
Ludovic Courtès <ludo@gnu.org> writes:

> Hi,
>
> "(" <paren@disroot.org> skribis:
>
>> On Sat Aug 6, 2022 at 9:46 PM BST, Ludovic Courtès wrote:
>>> I guess I’m missing some context: is this fixing a bug 
>>> currently
>>> present?  (Apologies if this has been discussed elsewhere!)
>>
>> This is one of two patches that fix a problem where any greetd 
>> greeter
>> more complex than agreety hangs on boot, basically rendering 
>> greetd
>> useless. I think the underlying cause is their being unable to 
>> connect
>> to seatd.sock?
>>
>> At least, that's the symptom I know about. I'm not sure whether 
>> there
>> are others.
>
> Is there a bug report, and do we have system tests for this
> functionality?
Problem started with conversation on guix-devel, and related 
commit.
Last message of thread:
https://lists.gnu.org/archive/html/guix-devel/2022-08/msg00021.html

I was travelling and missed that change, when I
"guix pull && guix system reconfigure"ed at home and realized the
problem, I submitted fixes in the form of two patches 56690 and 
56699.

Then I was asked to open a bug report in guix-devel list, which 
is:
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=56971

>
> I admit I know little about greetd and cases where it might be 
> used.
> Having system tests for that would help make sure the relevant
> functionality works.
There is:

  make check-system TESTS="minimal-desktop"

Patches in 56690 and 56699 now include the tests for this case as 
well.

>
> Thanks,
> Ludo’.
muradm Aug. 8, 2022, 7:44 p.m. UTC | #10
"(" <paren@disroot.org> writes:

> On Mon Aug 8, 2022 at 9:58 AM BST, Ludovic Courtès wrote:
>> Is there a bug report, and do we have system tests for this
>> functionality?
>
> I don't believe there are system tests for greetd, no. There is
> a bug report, though: <https://issues.guix.gnu.org/56971>.
>
>> I admit I know little about greetd and cases where it might be 
>> used.
>
> As I understand it, greetd is a daemon that handles the 
> sensitive parts
> of display managers, which it calls 'greeters'. It allows you to 
> write a
> login program without having to write those difficult and 
> sensitive parts
> by simply writing a GUI that sends JSON messages to the socket 
> when it
> gets input.
For greetd/greeter this is fine explanation.

> So the problem is some greeters try to talk to seatd, but since 
> they
> don't have the right permissions, they bail out.
To be more correct here, greeter that requires both talking to 
greetd
and talking to seatd via libseat. Suppose gtkgreet which is 
running
with sway. So greetd will start greeter which is
"sway -c config-which-starts-gtkgreet.conf". Now you have two 
processes
in the scope of greeter, one is sway which has to talk to swatd 
via
libseat and the other is gtkgreet which is going to talk with 
greetd.

The one who bails out is sway here due to lack of permissions for
seatd.sock for talking to seatd via libseat.

>
>     -- (
diff mbox series

Patch

diff --git a/doc/guix.texi b/doc/guix.texi
index 3c5864ec1a..750ed9b121 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -23151,6 +23151,22 @@  input), without requiring the applications needing access to be root.
   %base-services)
 
 @end lisp
+
+Users which are going to interact with @code{seatd} daemon while logged in
+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"))
+@end lisp
+
 @end defvr
 
 @deftp {Data Type} seatd-configuration
@@ -23163,7 +23179,7 @@  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{socket} (default: @samp{"/run/seatd.sock"})
diff --git a/gnu/services/desktop.scm b/gnu/services/desktop.scm
index 29a3722f1b..0d7cd71732 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.
 ;;;
@@ -1646,7 +1646,7 @@  (define-record-type* <seatd-configuration> seatd-configuration
   seatd-configuration?
   (seatd seatd-package (default seatd))
   (user seatd-user (default "root"))
-  (group seatd-group (default "users"))
+  (group seatd-group (default "seat"))
   (socket seatd-socket (default "/run/seatd.sock"))
   (logfile seatd-logfile (default "/var/log/seatd.log"))
   (loglevel seatd-loglevel (default "info")))
@@ -1670,6 +1670,9 @@  (define (seatd-shepherd-service config)
                    #:log-file #$(seatd-logfile config)))
          (stop #~(make-kill-destructor)))))
 
+(define %seatd-accounts
+  (list (user-group (name "seat") (system? #t))))
+
 (define seatd-environment
   (match-lambda
     (($ <seatd-configuration> _ _ _ socket)
@@ -1683,6 +1686,7 @@  (define seatd-service-type
 applications needing access to be root.")
    (extensions
     (list
+     (service-extension account-service-type (const %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