freedomofpress / securedrop-workstation

Qubes-based SecureDrop Journalist Workstation environment for submission handling
GNU Affero General Public License v3.0
141 stars 43 forks source link

Preflight updater does not preserve reboot requirement after partial failures #778

Open eloquence opened 2 years ago

eloquence commented 2 years ago

Steps to reproduce

Hypothetical steps based on @cfm's report in https://github.com/freedomofpress/securedrop-workstation/issues/776#issuecomment-1144060763, have not attempted to reproduce yet:

  1. Induce a system state where a dom0 update is required (e.g., by downgrading a system package, or the securedrop-workstation-dom0-config package).
  2. Run the preflight updater from the terminal and tail ~/.securedrop_launcher/logs/launcher.log until dom0 updates are completed
  3. Interrupt connectivity and wait for updater to fail
  4. Restore connectivity and re-run updater

Expected behavior

Previously installed dom0 updates should prompt a reboot after updates are completed.

Actual behavior

Updater completes on second run, but never triggers reboot requirement.

Analysis

The updater writes a status JSON object to disk caled sdw-update-status in the format

{ "last_status_update": "<timestamp>", "status": "<integer>"}

The valid states are defined as an enum here: https://github.com/freedomofpress/securedrop-workstation/blob/a96de4792f1b9f6cdeba92437ae45a58b4111f05/launcher/sdw_updater_gui/Updater.py#L526-L534

This reflects the result of the last run. Using this data structure, the updater cannot set both the "failed" and "reboot required" flag at the same time -- it has to choose one. That choice is made here:

https://github.com/freedomofpress/securedrop-workstation/blob/a96de4792f1b9f6cdeba92437ae45a58b4111f05/launcher/sdw_updater_gui/Updater.py#L360-L367

In the scenario where dom0 updates were detected in a run but other updates failed, the updater discards the information about the reboot requirement in order to enforce a re-run. However, that re-run will no longer detect available dom0 updates, so if it succeeds, it will simply conclude as UPDATES_OK.

In fact, the updater assumes that REBOOT_REQUIRED means the prior run was fully completed. See this check:

https://github.com/freedomofpress/securedrop-workstation/blob/a96de4792f1b9f6cdeba92437ae45a58b4111f05/launcher/sdw_updater_gui/Updater.py#L480-L484

So, a naive solution to prioritize the reboot status over the failure status will not work -- it would mean that we would ignore the failure after the next reboot, and proceed to launch the app without resolving the problem. I think to fully address this, we will need to treat the reboot flag as distinct from the (partial) success or failure of the entire update run. A revised status object could have this structure:

{ "last_status_update": "<timestamp>", "status": "<integer>", "reboot_required": <bool> }
deeplow commented 4 months ago

I can confirm this is still a problem as of 1.0.0 / client 0.12.0