bug#56971: greeter user permissions are not enough to talk with seatd

bug#56971: greeter user permissions are not enough to talk with seatd

[muradm]

[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]


Hi,

As per discussion here:
https://lists.gnu.org/archive/html/guix-devel/2022-08/msg00020.html

Above change reduced permissions of greeter user.
While it is ok for greeters that do not talk to seatd,
greeters talking to seatd lost access to seatd socket.
As result, greeter (e.g. gtkgreet) requiring communication
with seatd is failing to start, causing "black screen"
behavior on active terminal (switching to the other non seatd
related terminal is possible, for manual permissions
adjustment as workaround).

To address this issue, we need more flexible control over
seatd user/group, which creates seatd.sock, and greeter user
which connects to seatd.sock.

Other distros (Arch for instance) introduced "seat" group.
So user which wants to login on system controlled by seatd
should be member of that group.

However, not all greeters require that, so I decided to make
more flexible. Propsed solutions consists of:

* 56690 - gnu: seatd-service-type: Should use seat group.
With this change, if seatd-service-type is present in the
system configuration, "seat" group will be added, and seatd
will run as root/seat. Group is configurable, but default is 
"seat".

* 56699 - gnu: greetd-service-type: Add greeter-extra-groups 
  config field.
With this change, if user wants to use seatd-service-type with
greeter requiring seatd.sock, he can add "seat" group to
greeter-extra-groups field.

Thanks in advance,
muradm


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

bug#56971: greeter user permissions are not enough to talk with seatd

[Liliana Marie Prikler]

block 56971 by 56690 56699
thanks

Hi muradm,

Am Donnerstag, dem 04.08.2022 um 12:45 +0300 schrieb muradm:
> [...] greeter (e.g. gtkgreet) requiring communication
> with seatd is failing to start, causing "black screen"
> behavior on active terminal (switching to the other non seatd
> related terminal is possible, for manual permissions
> adjustment as workaround).
> 
> To address this issue, we need more flexible control over
> seatd user/group, which creates seatd.sock, and greeter user
> which connects to seatd.sock.
Okay.

> However, not all greeters require that, so I decided to make
> more flexible.
Flexibility for its own sake is not always the right solution.  On the
other hand, looking at the two patches, it appears they are to be used
in combination?

>  Propsed solutions consists of:
> 
> * 56690 - gnu: seatd-service-type: Should use seat group.
> With this change, if seatd-service-type is present in the
> system configuration, "seat" group will be added, and seatd
> will run as root/seat. Group is configurable, but default is 
> "seat".
Why just the group and no user?  Is it not possible to launch seatd as
non-root?

> * 56699 - gnu: greetd-service-type: Add greeter-extra-groups 
>   config field.
> With this change, if user wants to use seatd-service-type with
> greeter requiring seatd.sock, he can add "seat" group to
> greeter-extra-groups field.
Note that you still have a TODO on that patch.

Cheers

bug#56971: greeter user permissions are not enough to talk with seatd

[muradm]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]


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

> block 56971 by 56690 56699
> thanks
>
> Hi muradm,
Hi Liliana,

> Am Donnerstag, dem 04.08.2022 um 12:45 +0300 schrieb muradm:
>> [...] greeter (e.g. gtkgreet) requiring communication
>> with seatd is failing to start, causing "black screen"
>> behavior on active terminal (switching to the other non seatd
>> related terminal is possible, for manual permissions
>> adjustment as workaround).
>>
>> To address this issue, we need more flexible control over
>> seatd user/group, which creates seatd.sock, and greeter user
>> which connects to seatd.sock.
> Okay.
>
>> However, not all greeters require that, so I decided to make
>> more flexible.
> Flexibility for its own sake is not always the right solution. 
> On the
> other hand, looking at the two patches, it appears they are to 
> be used
> in combination?
>
No, technically they are not strongly dependent on each other,
could be applied one after another in no particular order.
After both are applied, in cooperation they address this issue.

>>  Propsed solutions consists of:
>>
>> * 56690 - gnu: seatd-service-type: Should use seat group.
>> With this change, if seatd-service-type is present in the
>> system configuration, "seat" group will be added, and seatd
>> will run as root/seat. Group is configurable, but default is
>> "seat".
> Why just the group and no user?  Is it not possible to launch 
> seatd as
> non-root?
seatd provides a way for display servers to access input/output 
devices
without having to be root. So seatd it self has to run as root.
When seatd opening socket as root/seat, all members of seat would
be able to communicate with it. Also socket could be opened with
seat/seat for instance, but there is no specific point in doing 
so.
Will be one more unused system user around.
Arch seems to follow similar way, root/seat is ok for socket.
Also will signal that seatd is running as root.

>> * 56699 - gnu: greetd-service-type: Add greeter-extra-groups
>>   config field.
>> With this change, if user wants to use seatd-service-type with
>> greeter requiring seatd.sock, he can add "seat" group to
>> greeter-extra-groups field.
> Note that you still have a TODO on that patch.
That TODO is from the initial commit, it is about cgroup file
system mounting, and totally out of scope of this issue.

> Cheers
Thanks in advance


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

bug#56971: greeter user permissions are not enough to talk with seatd

[Liliana Marie Prikler]

Am Donnerstag, dem 04.08.2022 um 15:52 +0300 schrieb muradm:
> 
> Liliana Marie Prikler <liliana.prikler@ist.tugraz.at> writes:
> 
> > [...] [L]ooking at the two patches, it appears they are to 
> > be used in combination?
> > 
> No, technically they are not strongly dependent on each other,
> could be applied one after another in no particular order.
> After both are applied, in cooperation they address this issue.
This is what I'm saying, albeit in different words.  As far as I
understand, neither of these patches really accomplishes anything if
not put together.  Thus, you more or less opened three issues to
address one.

> > 
> seatd it self has to run as root.
Okay.

> That TODO is from the initial commit, it is about cgroup file
> system mounting, and totally out of scope of this issue.
I didn't mean your code, I meant a suggestion from a reviewer that you
haven't addressed yet (to my knowledge at least).

Cheers

bug#56971: greeter user permissions are not enough to talk with seatd

[muradm]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]


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

> Am Donnerstag, dem 04.08.2022 um 15:52 +0300 schrieb muradm:
>>
>> Liliana Marie Prikler <liliana.prikler@ist.tugraz.at> writes:
>>
>> > [...] [L]ooking at the two patches, it appears they are to
>> > be used in combination?
>> >
>> No, technically they are not strongly dependent on each other,
>> could be applied one after another in no particular order.
>> After both are applied, in cooperation they address this issue.
> This is what I'm saying, albeit in different words.  As far as I
> understand, neither of these patches really accomplishes 
> anything if
> not put together.  Thus, you more or less opened three issues to
> address one.
Really I don't know what to comment here else. My analysis showed
two independent issues, one is that seatd should have a declared
group so that users of it could join it. This issues is not 
specific
to greetd/greeter in any way. Any other greeting mechanism could
fall short on this. And second, greeter today required conditional
group to interact with seatd, or it could be any other group like
input, usb, modem or else depending on user setup.
Solutions are offered accordingly. Third issue, this bug I was
asked to open. I don't understand, is it a sin to have multiple
issues, or what is the problem here?

>
>> >
>> seatd it self has to run as root.
> Okay.
>
>> That TODO is from the initial commit, it is about cgroup file
>> system mounting, and totally out of scope of this issue.
> I didn't mean your code, I meant a suggestion from a reviewer 
> that you
> haven't addressed yet (to my knowledge at least).
done

>
> Cheers


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

bug#56971: greeter user permissions are not enough to talk with seatd

[Liliana Marie Prikler]

Am Freitag, dem 05.08.2022 um 09:48 +0300 schrieb muradm:
> 
> Liliana Marie Prikler <liliana.prikler@ist.tugraz.at> writes:
> 
> > Am Donnerstag, dem 04.08.2022 um 15:52 +0300 schrieb muradm:
> > > 
> > > Liliana Marie Prikler <liliana.prikler@ist.tugraz.at> writes:
> > > 
> > > > [...] [L]ooking at the two patches, it appears they are to
> > > > be used in combination?
> > > > 
> > > No, technically they are not strongly dependent on each other,
> > > could be applied one after another in no particular order.
> > > After both are applied, in cooperation they address this issue.
> > This is what I'm saying, albeit in different words.  As far as I
> > understand, neither of these patches really accomplishes 
> > anything if
> > not put together.  Thus, you more or less opened three issues to
> > address one.
> Really I don't know what to comment here else.  My analysis showed
> two independent issues, one is that seatd should have a declared
> group so that users of it could join it.  This issues is not 
> specific to greetd/greeter in any way.  Any other greeting mechanism
> could fall short on this.
But it is greetd that does, no?  If there are other greeting mechanisms
currently packaged in Guix falling short of this, please do tell.

> And second, greeter today required conditional group to interact with
> seatd, or it could be any other group like input, usb, modem or else
> depending on user setup.
> Solutions are offered accordingly. Third issue, this bug I was
> asked to open. I don't understand, is it a sin to have multiple
> issues, or what is the problem here?
It is not really a problem, but an observation.  I personally think a
single series that has both patches would have been more visible than
this thing split across three topics for two patches.  There is a small
overhead if you have to consider merges and blocks, even if debbugs
supports them.

Cheers

bug#56971: greeter user permissions are not enough to talk with seatd

[muradm]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]


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

> Am Freitag, dem 05.08.2022 um 09:48 +0300 schrieb muradm:
>>
>> Liliana Marie Prikler <liliana.prikler@ist.tugraz.at> writes:
>>
>> > Am Donnerstag, dem 04.08.2022 um 15:52 +0300 schrieb muradm:
>> > >
>> > > Liliana Marie Prikler <liliana.prikler@ist.tugraz.at> 
>> > > writes:
>> > >
>> > > > [...] [L]ooking at the two patches, it appears they are 
>> > > > to
>> > > > be used in combination?
>> > > >
>> > > No, technically they are not strongly dependent on each 
>> > > other,
>> > > could be applied one after another in no particular order.
>> > > After both are applied, in cooperation they address this 
>> > > issue.
>> > This is what I'm saying, albeit in different words.  As far 
>> > as I
>> > understand, neither of these patches really accomplishes
>> > anything if
>> > not put together.  Thus, you more or less opened three issues 
>> > to
>> > address one.
>> Really I don't know what to comment here else.  My analysis 
>> showed
>> two independent issues, one is that seatd should have a 
>> declared
>> group so that users of it could join it.  This issues is not
>> specific to greetd/greeter in any way.  Any other greeting 
>> mechanism
>> could fall short on this.
> But it is greetd that does, no?  If there are other greeting 
> mechanisms
> currently packaged in Guix falling short of this, please do 
> tell.
Point is not that "there are any/others affected", the point is, 
that
seatd is providing and interface, and currently it has a problem,
which is wrong permission.

>> And second, greeter today required conditional group to 
>> interact with
>> seatd, or it could be any other group like input, usb, modem or 
>> else
>> depending on user setup.
>> Solutions are offered accordingly. Third issue, this bug I was
>> asked to open. I don't understand, is it a sin to have multiple
>> issues, or what is the problem here?
> It is not really a problem, but an observation.  I personally 
> think a
> single series that has both patches would have been more visible 
> than
> this thing split across three topics for two patches.  There is 
> a small
> overhead if you have to consider merges and blocks, even if 
> debbugs
> supports them.
This is phylosophical topic, which I suppose is not in the scope.

>
> Cheers


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

bug#56971: greeter user permissions are not enough to talk with seatd

[Liliana Marie Prikler]

Am Sonntag, dem 07.08.2022 um 23:48 +0300 schrieb muradm:
> Point is not that "there are any/others affected", the point is, 
> that seatd is providing [an] interface, and currently it has a
> problem, which is wrong permission.
But there are two sides in this permission issue.  Note that the
subject line of the email is the inverse of what you just said.  Note
further how this issue doesn't require a single, but two patches to
solve, which are currently marked as blockers.  These are obviously
related.

Cheers

bug#56690: greeter user permissions are not enough to talk with seatd

[Liliana Marie Prikler]

Am Donnerstag, dem 04.08.2022 um 12:45 +0300 schrieb muradm:
> * 56690 - gnu: seatd-service-type: Should use seat group.
> With this change, if seatd-service-type is present in the
> system configuration, "seat" group will be added, and seatd
> will run as root/seat. Group is configurable, but default is 
> "seat".
I made it so that by default the sanitizer is used to turn the string
"seat" into a group and used (ice-9 match), reducing some needless
redundancy.  I also reworded the manual to the best of my ability
following our conversations and adapted the commit message.

> * 56699 - gnu: greetd-service-type: Add greeter-extra-groups 
>   config field.
> With this change, if user wants to use seatd-service-type with
> greeter requiring seatd.sock, he can add "seat" group to
> greeter-extra-groups field.
I fixed some minor issue in the manual and reindented the marionette-
type in the tests, also reworded the commit message.

I didn't get the chance to run the system tests – some timeout causes
the marionette build to fail on my machine – but I verified
independently that at least the seatd socket has the right permissions.
I hope this will be enough for you to get gtkgreet running.

Cheers