libct/cg/sd: simplify DetectUserDbusSessionBusAddress by kolyshkin · Pull Request #3356 · opencontainers/runc

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
thaJeztah merged 2 commits intoopencontainers:mainfrom
kolyshkin:user-dbus

kolyshkin commented Jan 28, 2022 •

edited

Loading

Uh oh!

Uh oh!

AkihiroSuda left a comment

Uh oh!

kolyshkin commented Jan 28, 2022

Uh oh!

kolyshkin commented Feb 1, 2022 •

edited

Loading

Uh oh!

kolyshkin commented Feb 1, 2022

Uh oh!

kolyshkin commented Feb 1, 2022

Uh oh!

kolyshkin commented Feb 16, 2022

Uh oh!

kolyshkin commented Mar 1, 2022

Uh oh!

kolyshkin commented Mar 9, 2022

Uh oh!

thaJeztah left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

kolyshkin commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Jan 28, 2022

Uh oh!

kolyshkin commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Feb 1, 2022

Uh oh!

kolyshkin commented Feb 1, 2022

Uh oh!

kolyshkin commented Feb 16, 2022

Uh oh!

kolyshkin commented Mar 1, 2022

Uh oh!

kolyshkin commented Mar 9, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kolyshkin commented Jan 28, 2022 •

edited

Loading

kolyshkin commented Feb 1, 2022 •

edited

Loading