RedHatInsights / yggdrasil

GNU General Public License v3.0
21 stars 37 forks source link

yggctl incorrectly connects to session D-Bus socket #142

Closed subpop closed 1 year ago

subpop commented 1 year ago

Describe the bug When logged into a login shell as root, a session D-Bus is started by the seat manager. This results in the DBUS_SESSION_BUS_ADDRESS variable exported in the environment. The presence of that variable with a non-empty value causes yggctl to connect to the session D-Bus socket instead of the system socket.

To Reproduce Steps to reproduce the behavior:

  1. Log in as root
  2. Start yggd connecting to the system bus
  3. Run yggctl workers list as root

Expected behavior The list of workers.

Actual behavior An error occurs: cannot list workers: The name is not activatable

subpop commented 1 year ago

This is caused by a logical flaw in the bus connection heuristic. It assumes that if the value of DBUS_SESSION_BUS_ADDRESS is not empty, the session bus should be used.

    if os.Getenv("DBUS_SESSION_BUS_ADDRESS") != "" {
        conn, err = dbus.ConnectSessionBus()
    } else {
        conn, err = dbus.ConnectSystemBus()
    }
    if err != nil {
        return cli.Exit(fmt.Errorf("cannot connect to bus: %w", err), 1)
    }
jirihnidek commented 1 year ago

I wasn't able to reproduce this issue. I tried to reproduce it with yggdrasil installed from fedora 38 repository.

[root@localhost ~]# rpm -qa | grep yggdrasil
yggdrasil-0.3.1-2.fc38.x86_64
yggdrasil-worker-package-manager-0.2.0-1.fc38.x86_64

When I tried to reproduce this issue, then I it does not print any error:

[root@localhost ~]# systemctl start yggdrasil.service 

[root@localhost ~]# systemctl status yggdrasil.service 
● yggdrasil.service - yggdrasil system service
     Loaded: loaded (/usr/lib/systemd/system/yggdrasil.service; disabled; preset: disabled)
    Drop-In: /usr/lib/systemd/system/service.d
             └─10-timeout-abort.conf
     Active: active (running) since Thu 2023-07-20 10:21:26 CEST; 1s ago
       Docs: https://github.com/RedHatInsights/yggdrasil
   Main PID: 264502 (yggd)
      Tasks: 11 (limit: 38154)
     Memory: 7.7M
        CPU: 39ms
     CGroup: /system.slice/yggdrasil.service
             └─264502 /usr/bin/yggd

čec 20 10:21:26 thinkpad-p1 systemd[1]: Starting yggdrasil.service - yggdrasil system service...
čec 20 10:21:26 thinkpad-p1 systemd[1]: Started yggdrasil.service - yggdrasil system service.

[root@localhost ~]# yggctl workers list

[root@localhost ~]# systemctl status yggdrasil.service 
● yggdrasil.service - yggdrasil system service
     Loaded: loaded (/usr/lib/systemd/system/yggdrasil.service; disabled; preset: disabled)
    Drop-In: /usr/lib/systemd/system/service.d
             └─10-timeout-abort.conf
     Active: active (running) since Thu 2023-07-20 10:21:26 CEST; 13s ago
       Docs: https://github.com/RedHatInsights/yggdrasil
   Main PID: 264502 (yggd)
      Tasks: 11 (limit: 38154)
     Memory: 7.7M
        CPU: 40ms
     CGroup: /system.slice/yggdrasil.service
             └─264502 /usr/bin/yggd

čec 20 10:21:26 thinkpad-p1 systemd[1]: Starting yggdrasil.service - yggdrasil system service...
čec 20 10:21:26 thinkpad-p1 systemd[1]: Started yggdrasil.service - yggdrasil system service.

Is it necessary to use any specific version? Or where should I see the error?

You said that you "logged into a login shell as root". How exactly did you do that? When I login as a root, then this environment variable DBUS_SESSION_BUS_ADDRESS is not set for a root.

jirihnidek commented 1 year ago

When I try to get list of workers as non-root user, then I get the error:

[jiri@localhost yggdrasil]$ yggctl workers list
cannot list workers: The name is not activatable

but it is expected behavior, when there non-root user does run yggd.

The question is, why is environment variable DBUS_SESSION_BUS_ADDRESS non empty for root user, and what does this environment variable contain?

subpop commented 1 year ago

I see this on main (but this should be reproducible with 0.3.1 too), on a rhel-9.3 VM. When logging into a console session (directly on a TTY), DBUS_SESSION_BUS_ADDRESS gets set to the path to a D-Bus socket file; it appears the seat manager is creating a "user" D-Bus session for the root user. This isn't abnormal behavior exactly, but it can be unexpected; a user D-Bus session for the root user isn't commonly used.

subpop commented 1 year ago

I guess maybe this behavior should be considered generally. Given that yggd can run on either the system D-Bus or a session D-Bus socket, how should it determine which socket to connect to? The same logic as above is used in yggd:

    if os.Getenv("DBUS_SESSION_BUS_ADDRESS") != "" {
        conn, err = dbus.ConnectSessionBus()
    } else {
        conn, err = dbus.ConnectSystemBus()
    }

The same question applies to yggctl, but framed from a user experience. When running yggctl as a non-root user, which yggdrasil service is the user expecting to connect to: the system or the session bus? And similarly, when running yggctl as a root user, which yggdrasil service is the user expecting to connect to?

jirihnidek commented 1 year ago

BTW: is there any real use case, when yggd is run as non-root user and session bus is used? Or is it there only for us (developers) to make development/testing easier? If it is there only for us, then I vote for removing it, because it looks like it could be reason for various issues.

subpop commented 1 year ago

Yes. Satellite makes use of multiple instances of yggd running in parallel today; their current implementation is very restricted, so this multiple-bus support was added to allow them to run any number of isolated stacks of dbus-broker/yggd/workers.

https://github.com/RedHatInsights/yggdrasil/discussions/93#discussioncomment-3697552

subpop commented 1 year ago

I've also written a worker that demonstrates some value when running on the user session bus: https://github.com/subpop/yggdrasil-worker-desktop-notification

jirihnidek commented 1 year ago

OK, then shouldn't we add some command line options --system and --user to yggctl like busctl has? I think that it would be better solution than using some heuristics. What do you think?

(and the --address=ADDRESS would be useful too)

subpop commented 1 year ago

busctl is a D-Bus specific utility. It makes sense for it to have D-Bus specific notions like --system and --user. From yggdrasil's perspective, D-Bus is an implementation detail. I would like to avoid exposing D-Bus specific concepts like system bus and session bus from the UI if possible. --address makes sense; yggdrasil 0.2 does have a --socket-addr flag that names the gRPC socket. I don't know if having that flag fully answers the question though.

jirihnidek commented 1 year ago

OK