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

Check for updates before most securedrop-admin commands #5788

Merged
merged 2 commits into from
Feb 19, 2021

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented Feb 12, 2021

Status

Ready for review

Description of Changes

Fixes #5317

Provides a new decorator, @update_check_required, and adds it to securedrop-admin subcommands which make significant modifications on the workstation or the server. If the update check detects a need to update (using the existing logic in securedrop-admin), it will display a message explaining how to update. A --force option is provided in case the user wishes to intentionally run a subcommand with a playbook version that differs from the most recent signed release tag (e.g., during QA).

Testing

This change is best tested on a real or virtualized Admin Workstation in Tails.

  1. Create a backup copy of your ~/Persistent/securedrop as warranted.
  2. Fetch all updates with git fetch --all and check out git tag 1.7.1
  3. Apply the changes from this branch via git cherry-pick -n 75249c2ef9dc9ad383fa02e6c1fe9a3a4117b38d && git cherry-pick -n 26e608723fed7e7a0f166caed3d2e395583a02f1 . This will stage the changes for commit -- you can verify this via git diff --cached.
  4. Run securedrop-admin check_for_updates
  • Observe that the command reports "All updates applied"
  1. Run securedrop-admin logs
  • Observe that securedrop-admin checks for updates before running the playbook
  • Observe that securedrop-admin attempts to run the logs playbook
  1. Switch to an older tag (you may need to discard re-apply the changes)
  2. Run securedrop-admin logs
  • Observe that securedrop-admin checks for updates and reports that updates are available
  • Observe that securedrop-admin fails to complete due to a version mismatch
  • Observe that securedrop-admin reports the checked out tag as Current branch status
  1. Run securedrop-admin --force logs
  • Observe that securedrop-admin now skips the update check and runs the logs playbook
  1. Check out the develop branch and repeat steps 7 and 8 and the associated checks.
  2. Bounce the network connection in Tails.
  • Observe that the graphical updater runs to completion and puts you back on the 1.7.1 tag.
  • (Optional) Test and observe that all securedrop-admin subcommands annotated via the decorator (see diff) run the update check and produce the expected help output
  1. Discard the unstaged changes with git reset --hard.

Deployment

Note that this will only be available to users after the next workstation update.

Checklist

If you made changes to securedrop-admin:

  • Linting and tests (make -C admin test) pass in the admin development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

Docs

While this change is largely self-documenting and does not conflict with previous guidance, I intend to add a brief explanation to the documentation, in particular https://docs.securedrop.org/en/stable/admin.html#the-securedrop-admin-utility

@kushaldas kushaldas self-assigned this Feb 15, 2021
Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

Did multiple reboots and also ./securedrop-admin --force tailsconfig to make sure things are okay. Rest of the change looks okay to me. Worked as intended.

I will do one more round after this comes out of draft and formally approve it.

admin/securedrop_admin/__init__.py Show resolved Hide resolved
admin/securedrop_admin/__init__.py Show resolved Hide resolved
if update_status is True:
sdlog.error("You are not running the most recent signed SecureDrop release "
"on this workstation.")
sdlog.error("Latest available version: {}".format(latest_tag))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can also print the current branch/tag the repository is in.

Copy link
Member Author

@eloquence eloquence Feb 17, 2021

Choose a reason for hiding this comment

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

I played with that as well in an earlier version. The problem is that the current_tag variable is only useful for version comparisons, as it produces highly confusing output e.g., on develop, due to its reliance on git describe. For current develop, that results in 0.6-rc2-2646-g5eaad3b04 (!), as that's the most recent reachable tag. In addition to a branch or tag, the local repo may be in HEAD detached state, or in any of a number of different git states (dirty repo, rebase in progress, etc.).

The first line of git status is generally informative, as is the starred line of git branch. We could collect either line (we should probably do so in check_for_updates) and print it out. I think that would be helpful to ease troubleshooting.

I have a slight preference for git branch as its output tends to be a bit more terse. I would suggest doing this in a follow-up PR, to keep this one tightly scoped to the update check. Here's a one-liner for doing that in Python (without error handling):

re.search(r"\* (.*)\n", subprocess.check_output(['git', 'branch']).decode('utf-8')).group(1)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no strong feelings one way or the other if the goal is just to print out the current branch/tag - but! the output from git status will also let you know if there are code changes in the repo (e.g. unsanctioned poking at ansible defaults), giving you a few more options:

  • you're on x branch/tag, update to <latest>
  • you're on x branch/tag and there are changed files, resolve this then upgrade to latest

This is probably not going to be a one-liner though.

Copy link
Member Author

@eloquence eloquence Feb 18, 2021

Choose a reason for hiding this comment

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

For now pushed to a WIP branch: https://github.com/freedomofpress/securedrop/compare/5317-check-for-updates..5317-check-for-updates-debug-info

This tries to get us the best of both worlds:

  • use git branch for terse information that can be included in the output
  • recommends using git status for troubleshooting

Will still need its own test and may fail CI as-is. If the approach seems sound, happy to include in this PR, or in a follow-up PR, at the reviewer's discretion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since review is still pending, I'll just clean this change up a bit further and fold it into this PR now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, and test plan updated. I've refrained from any further mangling of the git branch output as it's possible to use Tails in languages other than English.

@eloquence eloquence added this to the 1.8.0 milestone Feb 17, 2021
@eloquence eloquence force-pushed the 5317-check-for-updates branch from 3e0531e to e63f6e9 Compare February 17, 2021 06:06
@eloquence
Copy link
Member Author

eloquence commented Feb 17, 2021

(Rebased on latest develop and squashed.)

@eloquence eloquence marked this pull request as ready for review February 17, 2021 06:45
@eloquence
Copy link
Member Author

(Added test plan and promoted to ready.)

@eloquence eloquence force-pushed the 5317-check-for-updates branch from e63f6e9 to 033a9a6 Compare February 18, 2021 23:50
@eloquence eloquence force-pushed the 5317-check-for-updates branch from 033a9a6 to 26e6087 Compare February 19, 2021 00:37
Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

Code change looks good.

  1. Create a backup copy of your ~/Persistent/securedrop as warranted.
  2. Fetch all updates with git fetch --all and check out git tag 1.7.1
  3. Apply the changes from this branch via git cherry-pick -n 75249c2ef9dc9ad383fa02e6c1fe9a3a4117b38d && git cherry-pick -n 26e608723fed7e7a0f166caed3d2e395583a02f1 . This will stage the changes for commit -- you can verify this via git diff --cached.
  4. Run securedrop-admin check_for_updates
  • Observe that the command reports "All updates applied"
  1. Run securedrop-admin logs
  • Observe that securedrop-admin checks for updates before running the playbook
  • Observe that securedrop-admin attempts to run the logs playbook
  1. Switch to an older tag (you may need to discard re-apply the changes)
  2. Run securedrop-admin logs
  • Observe that securedrop-admin checks for updates and reports that updates are available
  • Observe that securedrop-admin fails to complete due to a version mismatch
  • Observe that securedrop-admin reports the checked out tag as Current branch status
  1. Run securedrop-admin --force logs
  • Observe that securedrop-admin now skips the update check and runs the logs playbook
  1. Check out the develop branch and repeat steps 7 and 8 and the associated checks.
  2. Bounce the network connection in Tails.
  • Observe that the graphical updater runs to completion and puts you back on the 1.7.1 tag.
  • (Optional) Test and observe that all securedrop-admin subcommands annotated via the decorator (see diff) run the update check and produce the expected help output
  1. Discard the unstaged changes with git reset --hard.

@kushaldas kushaldas merged commit 8ab3e76 into develop Feb 19, 2021
@kushaldas kushaldas deleted the 5317-check-for-updates branch February 19, 2021 09:47
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.

Warn or fail when running outdated playbook
3 participants