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

Remove legacy updater #430

Merged
merged 5 commits into from
Feb 19, 2020
Merged

Remove legacy updater #430

merged 5 commits into from
Feb 19, 2020

Conversation

eloquence
Copy link
Member

@eloquence eloquence commented Jan 23, 2020

Status

Ready for review

Description

Resolves #412

The preflight updater is now at feature parity with the cron job that fires every 24 hours to update VMs:

Therefore, it is safe to remove the cron job at this time. This PR preserves the securedrop-login script, which enforces updates to the sd-viewer template on login, until #415 is resolved.

NOTE: Because it changes the contents of the RPM package, this PR bumps the RPM version.

Test plan

Preparatory steps

  • Make sure you are using a workstation that has been previously provisioned from a different branch that includes the updater (e.g., master)
  • Check out this branch in your dev VM, make clone it into dom0

Test plan

  • Run make clean
  • Confirm the following files do not exist (make clean logic is still removing them)
    • /srv/salt/securedrop-update
    • /usr/bin/securedrop-update
    • /etc/cron.daily/securedrop-update-cron (symlink)
  • Run make all
  • Confirm the following files do not exist (make clean logic is still removing them)
    • /srv/salt/securedrop-update
    • /usr/bin/securedrop-update
    • /etc/cron.daily/securedrop-update-cron (symlink)

Checklist

  • make flake8 passes
  • make test passes in dom0
    • I am seeing mismatches in the RPC policy test, which appear to be unrelated to this PR, which does not touch RPC policies.
  • I have added/removed files, and have updated packaging logic in MANIFEST.in and rpm-build/SPECS/securedrop-workstation-dom0-config.spec

Attribution

This PR is based on #322, originally by @kushaldas.

@eloquence
Copy link
Member Author

I've tested this PR and can confirm that it works as intended; until we land it, patching from this branch should be a viable workaround if you want to provision a workstation without the legacy updater getting in the way (#378) or conflicting with the preflight updater (#396).

@eloquence eloquence force-pushed the remove_auto_updater branch 4 times, most recently from 25cbedd to ba42f32 Compare January 24, 2020 00:12
@eloquence
Copy link
Member Author

eloquence commented Jan 24, 2020

To cherry-pick this change without creating a new commit, as part of provisioning, you can use this command on the master branch:

git cherry-pick -n ..origin/remove_auto_updater

The change will be staged but not committed:

git diff --cached

Until we're ready to merge this, I'll keep it in sync with master.

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.

Minor comments, throughout. Haven't tested, but based on visual review, does what it says on the tin.

- /etc/pki/rpm-gpg/RPM-GPG-KEY-securedrop-workstation-test
- /etc/cron.daily/securedrop-update-cron
Copy link
Contributor

Choose a reason for hiding this comment

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

Because the clean action ensures state=absent, it actually makes sense to preserve the entries here. Note that /srv/* items may still need to be removed, either here or via clean-salt.

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.

dom0/sd-dom0-files.sls Show resolved Hide resolved
@eloquence eloquence force-pushed the remove_auto_updater branch from ba42f32 to e034bbb Compare January 24, 2020 01:02
@eloquence eloquence force-pushed the remove_auto_updater branch 2 times, most recently from ac2c430 to 90d0c25 Compare February 12, 2020 21:08
@eloquence
Copy link
Member Author

eloquence commented Feb 12, 2020

Since this changes the RPM, it should bump the package version -- I've not done that for now, but will before promoting to "ready", once other dependencies for this PR are met.

@@ -43,16 +43,14 @@ install -m 755 -d %{buildroot}/srv/salt/sd/sd-workstation
install -m 755 -d %{buildroot}/srv/salt/sd/sys-firewall
install -m 755 -d %{buildroot}/usr/share/%{name}/scripts
install -m 755 -d %{buildroot}/srv/salt/sd/usb-autoattach
install -m 755 -d %{buildroot}/%{_bindir}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary as the directory was previously created as part of the setup.py run.

Copy link
Member Author

@eloquence eloquence Feb 12, 2020

Choose a reason for hiding this comment

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

(As an aside, %{_bindir} has a leading slash, so those sorts of uses resolve to //. That's a noop, but can omit the unnecessary / - which is also used in the original code - if that's preferred.)

@eloquence
Copy link
Member Author

(Rebased to fix conflicts.)

@eloquence eloquence marked this pull request as ready for review February 19, 2020 01:23
Copy link
Contributor

@emkll emkll left a comment

Choose a reason for hiding this comment

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

After merging #458, the only thing that the cron job will is customization within templates themselves. Given the runtimes of the make all command and the ability for potential admins to manually apply those states, I think this is fine to merge now.

LGTM, thanks @eloquence and @kushaldas . Tested with provided test plan and additional steps (specifically staging scenario).

Development environment

  • Run make clean
  • Confirm the following files do not exist (make clean logic is still removing them)
    • /srv/salt/securedrop-update
    • /usr/bin/securedrop-update
    • /etc/cron.daily/securedrop-update-cron (symlink)
  • Run make all
  • make all completes without error
  • Confirm the following files do not exist (make clean logic is still removing them)
    • /srv/salt/securedrop-update
    • /usr/bin/securedrop-update
    • /etc/cron.daily/securedrop-update-cron (symlink)
  • make test: All tests pass

Staging Environment

  • Build rpm on this branch
  • Installs successfully in dom0
  • securedrop-admin --apply completes successfully
  • securedrop-update and /etc/cron.daily/securedrop-update-cron are absent

@emkll emkll merged commit 25ea8b9 into master Feb 19, 2020
@emkll emkll deleted the remove_auto_updater branch February 19, 2020 14:56
cfm pushed a commit that referenced this pull request Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove update cron job
4 participants