Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(yggctl): prefer system bus when run as root #143

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

subpop
Copy link
Collaborator

@subpop subpop commented Jul 19, 2023

yggctl will attempt to connect to the session bus only if
DBUS_SESSION_BUS_ADDRESS is non-empty and the effective UID of the
process is greater than zero. This results in root invocations of yggctl
preferring the system bus over the session bus.

Fixes: #142

@subpop subpop requested a review from jirihnidek July 19, 2023 17:04
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that refactoring is quite good. I would like to know why the environment variable DBUS_SESSION_BUS_ADDRESS was not empty in your case. I think that it is not normal situation. I was not able to reproduce this. See my comments in #142 Thus I'm not sure if os.Geteuid() > 0 is necessary.

cmd/yggctl/actions.go Show resolved Hide resolved
@subpop subpop force-pushed the improve-dbus-detection branch from 8a0a71e to 9db8d2e Compare July 20, 2023 14:40
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some request and suggestion.


conn, err = connect()
if err != nil {
if os.Getenv("DBUS_SESSION_BUS_ADDRESS") != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be changed to:

if os.Getenv("DBUS_SESSION_BUS_ADDRESS") != "" && os.Geteuid() > 0 {

otherwise this function will return incorrect error messages for the case you are trying to solve.

Copy link
Contributor

@jirihnidek jirihnidek Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I have better proposal to avoid checking conditions twice. What about this:

func connectBus() (*dbus.Conn, error) {
	var connect func(...dbus.ConnOption) (*dbus.Conn, error)
	var conn *dbus.Conn
	var err error
	var errMsg string
	if os.Getenv("DBUS_SESSION_BUS_ADDRESS") != "" && os.Geteuid() > 0 {
		connect = dbus.ConnectSessionBus
		errMsg = "cannot connect to session bus (" + os.Getenv("DBUS_SESSION_BUS_ADDRESS") + "): %w"
	} else {
		connect = dbus.ConnectSystemBus
		errMsg = "cannot connect to system bus: %w"
	}
	conn, err = connect()
	if err != nil {
		return nil, fmt.Errorf(errMsg, err)
	}
	return conn, nil
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Yea. This should work. And I completely forgot my Geteuid check when I rewrote the function. 🤦

yggctl will attempt to connect to the session bus only if
DBUS_SESSION_BUS_ADDRESS is non-empty and the effective UID of the
process is greater than zero. This results in root invocations of yggctl
preferring the system bus over the session bus.

Signed-off-by: Link Dupont <link@sub-pop.net>
@subpop subpop force-pushed the improve-dbus-detection branch from 9db8d2e to 4e62969 Compare July 21, 2023 15:01
@subpop subpop requested a review from jirihnidek July 21, 2023 15:01
Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks for updates 👍

@jirihnidek jirihnidek merged commit 95d8e02 into main Jul 21, 2023
@jirihnidek jirihnidek deleted the improve-dbus-detection branch July 21, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

yggctl incorrectly connects to session D-Bus socket
2 participants