libct/cg/sd: simplify DetectUserDbusSessionBusAddress #3356

kolyshkin · 2022-01-28T02:29:26Z

Properly escape the value of dbus address spec.
Remove using systemctl --user --no-pager show-environment because:
- is does the same thing we do (get address from DBUS_SESSION_BUS_ADDRESS or try to construct it from XDG_RUNTIME_DIR);
- it fails when both those variables are unset:
$ echo $DBUS_SESSION_BUS_ADDRESS, $XDG_RUNTIME_DIR
unix:path=/run/user/1000/bus, /run/user/1000
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus
$ unset DBUS_SESSION_BUS_ADDRESS
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus
$ unset XDG_RUNTIME_DIR
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
Failed to connect to bus: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined (consider using [email protected] --user to connect to bus of other user)
Since it does not make sense to suggest systemctl --user start dbus
if the proper environment is not set, remove that suggestion from the error
message text. Since DBUS_SESSION_BUS_ADDRESS environment variable
is set by dbus-run-session (or dbus-launch, or something similar that is
supposed to be run during the login process), add a suggestion to
re-login.

libcontainer/cgroups/systemd/user.go

AkihiroSuda

systemctl --user show-environment is for covering $XDG_RUNTIME_DIR is set but $XDG_RUNTIME_DIR/bus does not exist case, not for covering $XDG_RUNTIME_DIR is unset case

kolyshkin · 2022-01-28T04:39:20Z

systemctl --user show-environment is for covering $XDG_RUNTIME_DIR is set but $XDG_RUNTIME_DIR/bus does not exist case, not for covering $XDG_RUNTIME_DIR is unset case

Right, I missed that path.

However, current version of systemd does the same thing as we do -- it tries "unix:path=%s/bus", where %s is the value of XDG_RUNTIME_DIR.

Here's the logic:
https://github.com/systemd/systemd/blob/be7148ebed5d73c4a76bc6089ebe2e82d8fa33e0/src/libsystemd/sd-bus/sd-bus.c#L1332-L1347

The only caveat is it uses bus_address_escape function:
https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318

Will take a look into it.

kolyshkin · 2022-02-01T02:55:14Z

The only caveat is it uses bus_address_escape function:
https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318

Will take a look into it.

It seems that if we are re-creating DBUS_SESSION_BUS_ADDRESS from XDG_RUNTIME_DIR (which we do), we need to escape some characters, same as what bus_address_escape function in systemd does:
https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318.

Escaping rules are described at https://dbus.freedesktop.org/doc/dbus-specification.html#addresses, "Value-escaping ..."). It looks like godbus/dbus should do that.

Changing to draft for now.

D-Bus specification requires that the values in server address need to be escaped in a special way, and other clients perform the needed escaping (e.g. systemd [2] does that). More to say, if dbus sees a character that should have been escaped, it returns an error [3]. Add EscapeBusAddressValue function (with some tests and a benchmark), and use it from tryDiscoverDbusSessionBusAddress(). The function is exported as it might be of use to other packages (in particular, runc [4]). NOTE that the function does not escape '*' symbol, because de-facto it is the list of optionally-escaped characters since 2007, but that is not yet documented in D-Bus spec (see [5] for more details). [1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses [2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318 [3] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330 [4] opencontainers/runc#3356 [5] https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/248 Signed-off-by: Kir Kolyshkin <[email protected]>

D-Bus specification [1] requires that the values in server address need to be escaped in a special way, and other clients perform the needed escaping (e.g. systemd [2] does that). More to say, if dbus sees a character that should have been escaped, it returns an error [3]. Add EscapeBusAddressValue function (with some tests and a benchmark), and use it from tryDiscoverDbusSessionBusAddress(). The function is exported as it might be of use to other packages (in particular, runc [4]). NOTE that the function does not escape '*' symbol, because de-facto it is the list of optionally-escaped characters since 2007, but that is not yet documented in D-Bus spec (see [5] for more details). [1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses [2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318 [3] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330 [4] opencontainers/runc#3356 [5] https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/248 Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2022-02-01T22:00:27Z

OK, I have added proper address value escaping, so now our fallback is the same as systemd's (see previous comments).

@AkihiroSuda PTAL

kolyshkin · 2022-02-01T22:09:51Z

systemctl --user show-environment is for covering $XDG_RUNTIME_DIR is set but $XDG_RUNTIME_DIR/bus does not exist case, not for covering $XDG_RUNTIME_DIR is unset case

Right, I missed that path.

However, current version of systemd does the same thing as we do -- it tries "unix:path=%s/bus", where %s is the value of XDG_RUNTIME_DIR.

In other words,

if XDG_RUNTIME_DIR is set, and $XDG_RUNTIME_DIR/bus exists, our code will use it and won't fall back to calling systemctl;
if XDG_RUNTIME_DIR is not set, or $XDG_RUNTIME_DIR/bus does not exist (ot can't be connected to), systemctl --user can not be used at all, as it needs a dbus connection.

Therefore,

a fallback of using systemctl is useless, as it will fail anyway.
suggesting to try systemctl --user start dbus in error message is useless, too, as it will fail anyway.

D-Bus specification [1] requires that the values in server address need to be escaped in a special way, and other clients perform the needed escaping (e.g. systemd [2] does that). More to say, if dbus sees a character that should have been escaped, it returns an error [3]. Add EscapeBusAddressValue function (with some tests and a benchmark), and use it from tryDiscoverDbusSessionBusAddress(). The function is exported as it might be of use to other packages (in particular, runc [4]). NOTE that the function does not escape '*' symbol, because de-facto it is the list of optionally-escaped characters since 2007, but that is not yet documented in D-Bus spec (see [5] for more details). [1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses [2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318 [3] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330 [4] opencontainers/runc#3356 [5] https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/248 Signed-off-by: Kir Kolyshkin <[email protected]>

D-Bus specification [1] requires that the values in server address need to be escaped in a special way, and other clients perform the needed escaping (e.g. systemd [2] does that). More to say, if dbus sees a character that should have been escaped, it returns an error [3]. Add EscapeBusAddressValue function (with some tests and a benchmark), and use it from tryDiscoverDbusSessionBusAddress(). Add UnescapeBusAddressValue, and use it from getKey(). The functions are exported as they might be of use to other packages (in particular, runc [4]). NOTE that the Escape... function does not escape '*' symbol, because de-facto it is the list of optionally-escaped characters since 2007, but that is not yet documented in D-Bus spec (see [5] for more details). [1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses [2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318 [3] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330 [4] opencontainers/runc#3356 [5] https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/248 Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2022-02-16T01:13:24Z

I have rebased this to benefit from godbus/dbus#302, so the patch is now much smaller. See #3356 (comment) as for why calling systemd --user show-environment does not make sense.

PTAL @AkihiroSuda 🙏🏻

outdated

D-Bus specification [1] requires that the values in server address need to be escaped in a special way, and other clients perform the needed escaping (e.g. systemd [2] does that). More to say, if dbus sees a character that should have been escaped, it returns an error [3]. Add EscapeBusAddressValue function (with some tests and a benchmark), and use it from tryDiscoverDbusSessionBusAddress(). Add UnescapeBusAddressValue, and use it from getKey(). The functions are exported as they might be of use to other packages (in particular, runc [4]). NOTE that the Escape... function does not escape '*' symbol, because de-facto it is the list of optionally-escaped characters since 2007, but that is not yet documented in D-Bus spec (see [5] for more details). [1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses [2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318 [3] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330 [4] opencontainers/runc#3356 [5] https://gitlab.freedesktop.org/dbus/dbus/-/merge_requests/248 Signed-off-by: Kir Kolyshkin <[email protected]>

D-Bus specification [1] requires that the values in server address need to be escaped in a special way, and other clients perform the needed escaping (e.g. systemd [2] does that, as well as recent godbus [3]). More to say, it is important to perform such escaping, since if dbus sees a character that should have been escaped but it's not, it returns an error [4]. Fix tryDiscoverDbusSessionBusAddress to use dbus.EscapeBusAddressValue function, recently added to godbus [3]. [1] https://dbus.freedesktop.org/doc/dbus-specification.html#addresses [2] https://github.com/systemd/systemd/blob/5efbd0bf897a990ebe43d7dc69141d87c404ac9a/src/libsystemd/sd-bus/bus-internal.c#L294-L318 [3] godbus/dbus#302 [4] https://gitlab.freedesktop.org/dbus/dbus/-/blob/37b76d13738e782fe2eb12abdd0179745c0b3f81/dbus/dbus-address.c#L330 Signed-off-by: Kir Kolyshkin <[email protected]>

Apparently, "systemctl --user --no-pager show-environment" is useless without DBUS_SESSION_BUS_ADDRESS or XDG_RUNTIME_DIR set: $ echo $DBUS_SESSION_BUS_ADDRESS, $XDG_RUNTIME_DIR unix:path=/run/user/1000/bus, /run/user/1000 $ systemctl --user --no-pager show-environment | grep DBUS_SESS DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus $ unset DBUS_SESSION_BUS_ADDRESS $ systemctl --user --no-pager show-environment | grep DBUS_SESS DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus $ unset XDG_RUNTIME_DIR $ systemctl --user --no-pager show-environment | grep DBUS_SESS Failed to connect to bus: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined (consider using --machine=<user>@.host --user to connect to bus of other user) So, it does not make sense to try it to get the address. Also, it does not make sense to suggest "systemctl --user start dbus" either, for the same reason, so remove that suggestion from the error message text. Since DBUS_SESSION_BUS_ADDRESS environment variable, on which the code relies, is et by dbus-run-session (or dbus-launch, or something similar that is supposed to be run during the login process), add a suggestion to re-login. Finally, fix the following linter warning: > error-strings: error strings should not be capitalized or end with punctuation or a newline (revive) Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2022-03-01T19:40:17Z

Removed the dbus bump commit as it is now obsoleted by #3398.

@AkihiroSuda @thaJeztah PTAL

kolyshkin · 2022-03-09T01:54:06Z

@thaJeztah @cyphar @mrunalp PTAL

thaJeztah

LGTM

kolyshkin requested a review from AkihiroSuda January 28, 2022 02:29

kolyshkin added the area/rootless label Jan 28, 2022

kolyshkin force-pushed the user-dbus branch 2 times, most recently from 162d3aa to 0e63e5e Compare January 28, 2022 03:07

AkihiroSuda added the area/cgroupv2 label Jan 28, 2022

kolyshkin mentioned this pull request Jan 28, 2022

libct/cg/sd: optimize and test findDeviceGroup #3357

Merged

AkihiroSuda reviewed Jan 28, 2022

View reviewed changes

libcontainer/cgroups/systemd/user.go Show resolved Hide resolved

AkihiroSuda previously requested changes Jan 28, 2022

View reviewed changes

kolyshkin marked this pull request as draft February 1, 2022 02:55

kolyshkin mentioned this pull request Feb 1, 2022

Introduce and use EscapeBusAddressValue godbus/dbus#302

Merged

kolyshkin force-pushed the user-dbus branch from 0e63e5e to 2e258b7 Compare February 1, 2022 21:52

kolyshkin marked this pull request as ready for review February 1, 2022 22:00

kolyshkin requested a review from AkihiroSuda February 1, 2022 22:09

kolyshkin force-pushed the user-dbus branch 2 times, most recently from 37a05b1 to d58070b Compare February 16, 2022 01:11

AkihiroSuda previously approved these changes Feb 16, 2022

View reviewed changes

kolyshkin requested a review from thaJeztah February 16, 2022 22:43

kolyshkin dismissed AkihiroSuda’s stale review via 1a93520 March 1, 2022 19:39

kolyshkin force-pushed the user-dbus branch from d58070b to 1a93520 Compare March 1, 2022 19:39

kolyshkin requested a review from AkihiroSuda March 1, 2022 19:40

AkihiroSuda approved these changes Mar 2, 2022

View reviewed changes

thaJeztah approved these changes Mar 9, 2022

View reviewed changes

thaJeztah merged commit 51e607f into opencontainers:main Mar 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libct/cg/sd: simplify DetectUserDbusSessionBusAddress #3356

libct/cg/sd: simplify DetectUserDbusSessionBusAddress #3356

kolyshkin commented Jan 28, 2022 •

edited

Loading

AkihiroSuda left a comment

kolyshkin commented Jan 28, 2022

kolyshkin commented Feb 1, 2022 •

edited

Loading

kolyshkin commented Feb 1, 2022

kolyshkin commented Feb 1, 2022

kolyshkin commented Feb 16, 2022

kolyshkin commented Mar 1, 2022

kolyshkin commented Mar 9, 2022

thaJeztah left a comment

libct/cg/sd: simplify DetectUserDbusSessionBusAddress #3356

libct/cg/sd: simplify DetectUserDbusSessionBusAddress #3356

Conversation

kolyshkin commented Jan 28, 2022 • edited Loading

AkihiroSuda left a comment

Choose a reason for hiding this comment

kolyshkin commented Jan 28, 2022

kolyshkin commented Feb 1, 2022 • edited Loading

kolyshkin commented Feb 1, 2022

kolyshkin commented Feb 1, 2022

kolyshkin commented Feb 16, 2022

kolyshkin commented Mar 1, 2022

kolyshkin commented Mar 9, 2022

thaJeztah left a comment

Choose a reason for hiding this comment

kolyshkin commented Jan 28, 2022 •

edited

Loading

kolyshkin commented Feb 1, 2022 •

edited

Loading