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

Don't enable sdw-notify.timer unit via Salt. Use systemd-rpm-macros and user unit preset file. #1088

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

rocodes
Copy link
Contributor

@rocodes rocodes commented Jun 18, 2024

Status

Ready for review

Description of Changes

Towards #1052
Fixes #1006

  • Use systemd-rpm-macros in rpm .spec and enable user units according to a preset file.
  • Manage sdw-notify.timer (and all systemd user) units via systemd macros in rpm %post (and %preun) instead of via Salt, and stop using --global for enabling user units.

Testing

  • Visual review
  • CI
  • Manual testing (see below)
  • Start from an environment without a dom0 workstation config and without any securedrop systemd services configured in dom0. (This could be an --uninstall, or it could be sneakily removing the dom0 rpm and disabling then removing (deleting) the sd systemd services securedrop-user-xfce-icon-size, securedrop-user-xfce-settings, and sdw-notify.timer and sdw-notify.service. Reload the systemd user daemon (systemctl --user daemon-reload) and ensure those services are not listed.
  • Build an rpm from the tip of this branch (make build-rpm), and copy it into dom0, then install it. Reboot the laptop.
  • systemctl --user status for each of sdw-notify.timer, securedrop-user-xfce-icon-size, and securedrop-user-xfce-settings shows them as enabled, and systemctl status securedrop-logind-override-disable enabled (on dev systems the service will run; on staging and prod systems the expected behaviour is that the service will fail a ConditionPath check and be disabled.)
  • When the rpm is uninstalled, securedrop-related systemd units (user and system) are all removed.
  • Upgrade test plan: starting from a preinstalled securedrop-workstation-dom0-config and an existing workstation setup, confirm that desired services are loaded, as above.
  • Build rpm from the tip of this branch and install over top of your existing setup.
  • RPM installation completes successfully
  • Desired services are still in place, as above.

Deployment

Any special considerations for deployment? Consider both:

  1. Upgrading existing pilot instances
  2. New installs

Checklist

If you have made changes to the provisioning logic

  • All tests (make test) pass in dom0

If you have added or removed files

  • I have updated MANIFEST.in and rpm-build/SPECS/securedrop-workstation-dom0-config.spec

If documentation is required

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation

@rocodes rocodes added this to the 1.1.0 milestone Nov 26, 2024
@rocodes rocodes force-pushed the notify-timer-unsalted branch from 710318f to 67eb670 Compare November 28, 2024 19:07
@rocodes rocodes marked this pull request as ready for review November 28, 2024 19:35
@rocodes
Copy link
Contributor Author

rocodes commented Nov 28, 2024

ci failure unrelated to this change -
https://ws-ci-runner.securedrop.org/2024-11-28-190455591162-67eb6703adec3b9e60946c269ce0ae7d7c6b26a2-Qubes_4.2_A-update_20241128043845.log.txt
An error was encountered while installing package(s): E: Release file for tor+https://deb.debian.org/debian-security/dists/bookworm-security/InRelease is not valid yet (invalid for another 1min 10s).

@deeplow deeplow self-requested a review November 29, 2024 13:37
@deeplow deeplow self-assigned this Nov 29, 2024
Copy link
Contributor

@deeplow deeplow left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Always happy to see less salt.

I haven't waited the 2 hours to confirm this works, but I believe systemd is basically doing the same thing. So I don't expect it to be necessary.

However, I did notice that after sdw-admin --uninstall the timer is still there. There is still a symlink:

lrwxrwxrwx 1 root root 38 Nov 29 13:33 /etc/systemd/user/default.target.wants/sdw-notify.timer -> /usr/lib/systemd/user/sdw-notify.timer

I'm guessing there is a way remove in in an uninstall hook? Last time I checked debian handlers even had something do deal with systemd stuff automatically. However, it wasn't feature complete.

@deeplow
Copy link
Contributor

deeplow commented Nov 29, 2024

Interestingly this would "fix" some issue that's happening in the CI for your other PR

https://ws-ci-runner.securedrop.org/2024-11-28-211025306366-2f74007f228cc7866946973ee955e8962c1da807-Qubes_4.2_C-update_20241128050551.log.txt:

INFO:[2024-11-28-21:43:37:490899]           ID: enable-user-units
INFO:[2024-11-28-21:43:37:490945]     Function: cmd.run
INFO:[2024-11-28-21:43:37:490974]         Name: systemctl --user daemon-reload
INFO:[2024-11-28-21:43:37:491006] systemctl --user enable sdw-notify.timer
INFO:[2024-11-28-21:43:37:491034] 
INFO:[2024-11-28-21:43:37:491066]       Result: False
INFO:[2024-11-28-21:43:37:491093]      Comment: Command "systemctl --user daemon-reload
INFO:[2024-11-28-21:43:37:491126]               systemctl --user enable sdw-notify.timer
INFO:[2024-11-28-21:43:37:491151]               " run
INFO:[2024-11-28-21:43:37:491184]      Started: 21:42:53.059047
INFO:[2024-11-28-21:43:37:491210]     Duration: 189.213 ms
INFO:[2024-11-28-21:43:37:491243]      Changes:   
INFO:[2024-11-28-21:43:37:491269]               ----------
INFO:[2024-11-28-21:43:37:491302]               pid:
INFO:[2024-11-28-21:43:37:491328]                   13529
INFO:[2024-11-28-21:43:37:491360]               retcode:
INFO:[2024-11-28-21:43:37:491386]                   1
INFO:[2024-11-28-21:43:37:491418]               stderr:
INFO:[2024-11-28-21:43:37:491444]                   Failed to connect to bus: No such file or directory
INFO:[2024-11-28-21:43:37:491477]                   Failed to connect to bus: No such file or directory
INFO:[2024-11-28-21:43:37:491503]               stdout:

@rocodes rocodes marked this pull request as draft December 2, 2024 15:38
@rocodes
Copy link
Contributor Author

rocodes commented Dec 2, 2024

Hey, thanks for the review. Re: uninstall: I kept the removal logic in the salt run, so running the uninstall command (and rebooting or reloading systemd) should address the timer. But I flipped back to draft mode because I think I have a cleaner way of addressing this change and the comment I made about enabling for all users too. (See #1052)

@rocodes rocodes mentioned this pull request Dec 4, 2024
1 task
@rocodes rocodes force-pushed the notify-timer-unsalted branch from 67eb670 to 18da365 Compare December 6, 2024 16:41
@rocodes rocodes changed the title Enable sdw-notify.timer for all users in rpm %post instead of via Salt. Don't enable sdw-notify.timer unit via Salt. Use systemd-rpm-macros and user unit preset file. Dec 6, 2024
@rocodes rocodes force-pushed the notify-timer-unsalted branch from 18da365 to f593ee3 Compare December 6, 2024 17:18
@rocodes
Copy link
Contributor Author

rocodes commented Dec 6, 2024

I have updated this PR in order to address the CI failures (which presumably could also imply real-world provisioning failures) that @deeplow and others have noticed a few different places (#1006, #1205) in a more consistent way, by avoiding running systemd user commands via salt orchestration.

This PR does introduce an additional build dependency in our rpm .spec file (systemd-rpm-macros). These macros ship already with systemd.

@rocodes rocodes marked this pull request as ready for review December 6, 2024 17:26
@rocodes rocodes force-pushed the notify-timer-unsalted branch from f593ee3 to 9a0e5e5 Compare December 6, 2024 17:28
@rocodes rocodes requested review from deeplow and a team December 6, 2024 18:07
Add preset file for user systemd units. Use systemd rpm macro.
@rocodes rocodes force-pushed the notify-timer-unsalted branch from 9a0e5e5 to e00a878 Compare December 6, 2024 18:09
@rocodes
Copy link
Contributor Author

rocodes commented Dec 6, 2024

I've stepped through the manual test plan (get rid of dom0 rpm and securedrop systemd user units, install this rpm, verify units are loaded)

@rocodes
Copy link
Contributor Author

rocodes commented Dec 6, 2024

(as was rightly pointed out, now that this is coming in post 4.2 fresh install, I need to add an upgrade test plan!)

@cfm cfm assigned cfm and unassigned deeplow Dec 6, 2024
@rocodes
Copy link
Contributor Author

rocodes commented Dec 6, 2024

Notes re upgrade scenario:

The sdw-notify.service and .timer files were already managed by the workstation config rpm and installed at rpm install time, so there will be no conflict for folks upgrading. The difference is when/how the notify timer is enabled (rpm install time vs sdw-admin --apply time), and when/how it's disabled (rpm uninstall vs sdw-admin --uninstall).

You can confirm this by installing the 1.0.0 rpm (and running the dom0 highstate), then installing this rpm on top of it.

This means, if folks install the workstation-dom0-config rpm and then don't install SDW, they will have an edge case situation where they are notified to apply updates, but the SDW VMs haven't been provisioned yet. That is definitely an oddity, but it's consistent with other oddities about "configure at rpm install time" vs "configure during apply time" that we have already previously introduced (for example, shipping the .desktop file, changing icon size, provisioning files to the /srv/salt/securedrop_salt directory, although that last one is less obviously user-facing).

My opinion is that we should change/address that behaviour all together cause it's an edge case, but that it's acceptable right now. (For example, one way to address it would be to have the rpm post immediately pop up a GUI graphical installer ;) ). I'm documenting this so that no one is surprised, but noting that it's consistent with other changes we made as part of 4.2 compat.

Copy link
Member

@cfm cfm left a comment

Choose a reason for hiding this comment

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

I've tested the upgrade from from an up-to-date v1.0.0 and can confirm:

  • systemctl --user status sdw-notify.timer
  • systemctl status securedrop-user-xfce-icon-size
  • systemctl status securedrop-user-xfce-settings correctly on:
    ConditionPathExists=|!/var/lib/securedrop-workstation/dev
    ConditionPathExists=|/var/lib/securedrop-workstation/prod
    ConditionPathExists=|/var/lib/securedrop-workstation/staging

    —with /var/lib/securedrop-workstation/dev present.

@rocodes rocodes dismissed deeplow’s stale review December 6, 2024 21:39

changes addressed

@rocodes rocodes added this pull request to the merge queue Dec 6, 2024
Merged via the queue into main with commit 2943c4c Dec 6, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

DBus-related things are flaky in CI
3 participants