[WIP] Add security update notification script

[pierwill]

Couldn't help myself—gave addressing #236 a try. This approach uses a script in /etc/cron.hourly/ to display a notification on the hour if system uptime exceeds 5 days: "This SecureDrop Workstation has been operating for more than 5 days. For security reasons, we recommend restarting the system now."

The notify-send command does not support click actions, so to offer the user a button to shutdown the system (as @eloquence originally suggested), we might look into the approach used here: https://github.com/vlevit/notify-send.sh. However, users might be unlikely to be ready to shutdown at the time of notification, so the approach here might be good enough.

[redshiftzero]

hey @pierwill thanks for this! would you be interested in making this notify-send notification a pyqt4 dialog? This would make the notification very hard to miss: from the user's perspective a little window would pop up which they'd need to actively dismiss. Once the related functionality in #396 is merged we'll then also want to modify the conditions in which this alert pops up, see my comment in #236.

[pierwill]

would you be interested in making this notify-send notification a pyqt4 dialog?

I'll look into this!

[eloquence]

@pierwill If you have time to poke at this in the next few days, that'd be awesome; we'll need some version of this for the pilot, so we can work on this in the next sprint if you're busy. :)

[pierwill]

I had a tough time trying to understand how to use pyqt when I tried this week. I'm going to try again this weekend. I'll likely end up needing some help from the team, I think!

[eloquence]

Check out the graphical updater that's being worked on in #396 for some inspiration.

[pierwill]

Will do! Thanks― 👀

[eloquence]

Hi @pierwill, you had mentioned on Gitter that you were running into difficulties using PyQt4 and you don't mind us taking this one over. I'm happy to do so, but in case you want to take another stab at it, here's an example of how to do a simple alert in PyQt4 that will work on dom0:

https://gist.github.com/eloquence/940646c3e4e11a5b2d05bc418244bdb7

I was also able to test this outside Qubes on an Ubuntu 18.04 system by installing the pyqt4 system package from the Ubuntu repositories.

In terms of the check itself, I think we should check both uptime and the timestamp written to dom0 by the preflight updater (see changes introduced in #396). If both uptime and the time since the last update check are > 8 hours, we show the warning. Otherwise we do nothing.

That's not something we've discussed as a team yet but if you put in that logic, we can reason through it together.

Let me know if you want to poke at this again; otherwise I'll set aside some time tomorrow to work on it further.

[eloquence]

Hi @pierwill, this is coming together nicely. I added a commit that uses an exclusive lock to ensure we only show one dialog at a time, and renamed the file given that the intended use cases extend beyond an uptime check.

We spec'd the desired behavior out a bit more, here:
https://docs.google.com/document/d/1cCpcpLRiKC0tm_vsD4k4Q1Fr5_R34htCb8FNTV8mRCI/edit#heading=h.m6400jhq14zi

To cover the base case, we need to inspect the contents of os.path.join(DEFAULT_HOME, "./securedrop_launcher/sdw-last-updated"). If there's a file w/ a timestamp in the expected format here, and it's more than 5 days ago, we should show the notification.

The current uptime check should only run when this file doesn't exist at all.

There's some additional logic described in the GDoc, but if you have time to just work on the base case, that would already get us one big step closer. If not I'll pick this up again ASAP.

[eloquence]

Hi @pierwill, thanks for adding the additional timestamp. I cleaned things up a bit, added some logging, and fleshed out the business logic to utilize uptime and timestamp as intended.

The main case we have yet to cover is the one where the updater itself is already running (in which case we don't want to annoy the user with a separate warning). @emkll and I talked about using a simple "ps" output check for this, though I am tempted to add lockfile support to the main updater for this since we already have it here anyway.

If you have time to start poking at adding tests, that would be awesome; I'll be looking into the "is the updater running" check and inviting some feedback from a maintainer on the overall approach so far.

[eloquence]

Lockfile support added to preflight updater in 61a56a2. Not tested in Qubes yet.

Force-pushed to rebase to latest upstream master.

For the cron job to work, we need to obtain the user's Qubes username, as the script has to write to their home directory, not /root. Will add logic for that now, following the one we use in Salt to do the same thing.

Then I'll attempt to implement the lockfile logic to ensure that it's impossible to get the warning when the updater is running, but it's possible to run the updater while the warning is shown.

[eloquence]

The lockfile implementation is now done. In non-Qubes testing the warning now doesn't show if the updater is running, but the updater can still be run if the warning is shown. I've also cleaned up the code a fair bit.

I've also added code for getting the GUI user. I used getent, since it happily runs without root as well. Not tested in Qubes yet.

Next up:

• dom0 testing & fixes
• More code cleanup & tests

@pierwill, as always, please jump in if you have time to collaborate :). Even manual testing is appreciated.

[eloquence]

Changes I just pushed:

• Writing logs to the same directory used by the preflight updater. Note: the cron job runs as root by default (and therefore will create files with that ownership), we'll need to decide whether we want to use crontab to run it as the GUI user
• More code cleanup, exception handling, and documentation.

Launcher tests are currently failing because bandit doesn't like the hardcoded path that's used for the launcher lockfile. I'll have to read up on that a bit more, but just to recap, it writes its lockfile to /tmp because it doesn't have access to /run as the user.

As with previous revisions, will need to do another round of dom0 testing, but if things look good there, the next step is to add tests & resolve the above questions/issues. @pierwill, again, help with tests/testing would be very much appreciated if you have time over the weekend. :)

[eloquence]

I think the logical next steps here are:

• run the cron job as the GUI user via crontab. This ensures that both preflight updater and notification script run as the same user (they write logs to the same directory in the GUI user's HOME)
• create lock files in /run/user/<ID>, which is a more sensible location than /tmp and avoids the "hardcoded /tmp directory" security warning by bandit (not really applicable here, anyway)
• add tests

[eloquence]

Now can run as the GUI user, stores both lock files in /run/user, and logs all outcomes. Looks good in Qubes testing. Next up: modifying crontab & adding tests.

[eloquence]

Next up: modifying crontab

This is now done. The script is added via a new Salt state, sd-dom0-crontab, which adds it as an hourly cron job run as the user, using the blockreplace pragma we also use for modifying RPC policies. The cleanup script should cleanly remove those additions.

Functionally this all works as expected so far in my local testing, including lockfile interaction. Automated tests still TK.

[eloquence]

Made the following changes:

• Moved the notify script into the launcher directory, and installed it in /opt/securedrop alongside the launcher. This makes it easy for us to run the tests as part of the existing launcher tests; besides, we're likely to want to share code across both in future, and they're essentially part of the same system component.
• Separated the core business logic from the Qt bits, to make it more testable
• Added the first example test (which is currently failing in CI, go figure).

Now would be a good checkpoint to make sure I'm on the right track before fleshing out the tests. :)

[eloquence]

FWIW, to test the lock file behavior I'm using lsof, which is not available in our CircleCI container by default so I'm adding it. The CircleCI container (and Qubes, and most Linux systems) does have lslocks, which is a better fit anyway, but Macs don't have it out of the box, so this seems like the best compromise as far as I can tell to ensure the tests run cross-platform.

[eloquence]

Using importlib instead of imp here helps us to avoid the deprecation warning for imp.

[eloquence]

Further changes today:

• Added more lockfile tests. I'm using the multiprocessing module to cause a lock conflict, but I'm sure there's a more straightforward way to do so -- comments very much appreciated.

• More code reorganization, mostly for testability, but also to reduce side effects in functions and instead handle the continue vs. exit logic within the launcher/notifier entrypoint scripts.

• Rebased to latest upstream master and re-tested in Qubes.

The tests for the notification business logic are still pending, and relatively straightforward. Will aim to tackle those tomorrow.

[eloquence]

Testing manually is relatively straightforward:

1. Check out this branch from Pierwill's remote in Qubes
2. make clone && make prep-dom0
3. Test that preflight updater is still working as expected
4. Test that you can't run another preflight updater while it's running (new functionality)
5. Test that 4) creates a log entry in ~/.securedrop_launcher/launcher.log
6. Test that running /opt/securedrop/launcher/sdw-notify.py has the expected results for your current workstation state per the behavior design doc
7. Test that you can provoke the notification to appear when expected (easiest by manually modifying WARNING_THRESHOLD and UPTIME_GRACE_PERIOD in /opt/securedrop/launcher/sdw_notify/Notify.py and the timestamp written to ~/.securedrop_launcher/sdw-last-updated)
8. Test that all of the above creates logs in ~/.securedrop_launcher/logs/sdw-notify.log
9. Test that the cron job runs as expected (easiest by modifying the 0 to * in /etc/crontab to change it from "every hour" to "every minute")

Once automated tests are done and after some final refactoring, I'll structure this test plan a bit more carefully and do a final squash-rebase before submitting this formally for review, but all of the above should work for anyone wanting to give it a spin.

[eloquence]

I am closing this PR in favor of #445, which represents a squashed branch for easier review, with a PR body that represents the current state and test plan. However, I've kept the non-squashed branch in place here in case it is of historical interest.