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

Make DBUS session check more resilient #684

Merged
merged 1 commit into from
Apr 7, 2021
Merged

Make DBUS session check more resilient #684

merged 1 commit into from
Apr 7, 2021

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented Apr 2, 2021

We should always ensure that the DBUS session is set to the logged in user, who should not be root.

Towards #668

Status

Ready for review

Test plan

  1. Configure your environment to staging
  2. Check out this branch in dom0 via make clone
  3. Build a staging env via make staging
    • Observe that update-xfce-settings runs successfully
  4. Attempt to run the update-xfce-settings script as root
    • Observe that you can't

We should always ensure that the DBUS session is set to the
logged in user, who should not be root.
Copy link
Contributor

@conorsch conorsch left a comment

Choose a reason for hiding this comment

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

Works as advertised. Still can't explain how uid 0 was running those tasks, but the error report was clear that it was happening.

Spent some time reading up on how dbus works, which has changed a bit in versions of systemd, so we may want to revisit this code during the Qubes 4.1 transition (when systemd should go from v231 to v245). I'll post a patch that I tested locally, but am not appending to the PR as part of the merge, since the changes presented work well as they are.

@eloquence
Copy link
Member Author

eloquence commented Apr 7, 2021

but the error report was clear that it was happening.

Just noting that, if you look at @emkll's reported output, you'll notice that $USER was correctly set to m (his Qubes username). My hypothesis is that runas was working just fine, but for some reason (use of sudo or something else) there was also a DBUS session for root, which the script as previously written did not override.

@conorsch
Copy link
Contributor

conorsch commented Apr 7, 2021

Tried this locally:

diff --git a/dom0/update-xfce-settings b/dom0/update-xfce-settings
index f1adecf..4d64730 100755
--- a/dom0/update-xfce-settings
+++ b/dom0/update-xfce-settings
@@ -23,12 +23,15 @@ ICONSIZE=64
 
 if ! [ -x "$(command -v xfconf-query)" ]; then
   echo "Error: xfconf-query is not installed." >&2
-  exit 1
+  exit 2
 fi
 
-# This script requires a valid DBUS session to work. When run non-interactively,
-# we assume that a sesssion is running for the current user.
-export DBUS_SESSION_BUS_ADDRESS="unix:path=/run/user/$(id -u $USER)/bus"
+# This script requires a valid DBUS session to work. When run interactively,
+# DBUS_SESSION_BUS_ADDRESS should already be populated. Otherwise, create
+# a session so that the xfconf-query commands succeed.
+if [[ -z "${DBUS_SESSION_BUS_ADDRESS:-}" ]]; then
+    eval "$(dbus-launch)"
+fi

and it handled the edge case of DBUS_SESSION_BUS_ADDRESS not being defined. Which notably it isn't during a make staging run: if you test -n $DBUS_SESSION_BUS_ADDRESS in that script, it will fail. Whether we hardcode the path or politely request a session doesn't seem to make much of a difference. Once downside of politely requesting a fresh session via dbus-launch is that the session appears to hang around forever. In the case of the Workstation, we expect shutdowns almost daily, but I'm still not comfortable forking the dbus process on every update without identifying a cleanup mechanism.

@conorsch conorsch merged commit d94d774 into main Apr 7, 2021
@legoktm legoktm deleted the dbus-factor branch May 28, 2024 15:25
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.

2 participants