Use --batch option for refreshing release key to resolve graphical updater issues
#4233
Conversation
KwadroNaut
requested review from
emkll,
heartsucker,
kushaldas,
msheiny and
redshiftzero
as code owners
eloquence
changed the title
--batch option for refreshing release key to resolve graphical updater issues
|
Confirmed, tested by checking out branch on Admin Workstation and bouncing the network connection. Updater appears and update to latest release completes as expected. |
|
I will merge this after chatting with @zenmonkeykstop once in the evening. |
admin/securedrop_admin/__init__.py
Outdated
| @@ -642,7 +642,7 @@ def check_for_updates(args): | |||
|
|
|||
|
|
|||
| def get_release_key_from_keyserver(args, keyserver=None, timeout=45): | |||
| gpg_recv = ['timeout', str(timeout), 'gpg', '--recv-key'] | |||
| gpg_recv = ['timeout', str(timeout), 'gpg', '--batch', '--recv-key'] | |||
There was a problem hiding this comment.
I think we should add --no-tty also to be on the safe side (so that gpg doesn't try to write output to the tty, which doesn't exist). From the gpg docs:
--no-tty
Make sure that the TTY (terminal) is never used for any output. This option is needed in some cases because
GnuPG sometimes prints warnings to the TTY if if --batch is used.
There was a problem hiding this comment.
Thanks once again to catch the specialties and tame GnuPG. Updated per your suggestion. However I pushed that change, and then ran into a broken test.
In admin/tests/test_integration.py test_update_fails_when_no_signature_present checks the cli contained output like "Signature verification failed" not sure on the best way forward, feel free to edit my changes.
There was a problem hiding this comment.
@KwadroNaut I think the test failure was due to keyserver flakiness. I've had the test suite pass several times. The only consistent problem is a linting problem; flake8 wants the second line of arguments to start after the opening bracket of the list: securedrop_admin/__init__.py:646:13: E128 continuation line under-indented for visual indent. If you have time to slide that over and push again, great, otherwise a maintainer will just do it later in the week. Thanks again!
KwadroNaut
force-pushed
the
recv
branch
2 times, most recently
from
5042837 to
95ac980
Compare
|
|
Tacked on a commit to fix the linting error that @kushaldas flagged. Assuming CI passes, please re-review, @kushaldas! |
|
Thanks @conorsch ! |
|
CI error in https://circleci.com/gh/freedomofpress/securedrop/24326 indicates that a rebase is required, will push one up shortly. We're aiming to include this change in 0.12.1. |
you don't have an interactive prompt in the gui-updater, so you
shouldn't use a tty and --batch or --no-tty is meant for such tings.
--no-tty
Make sure that the TTY (terminal) is never used for any output.
This option is needed in some cases because
GnuPG sometimes prints warnings to the TTY if if --batch is used.
--batch
Use batch mode. Never ask, do not allow interactive commands.
The test, which uses a subshell, is looking for errors on the cli, this
should be changed.
The commit from @KwadroNaut added a few options, and flake8 was characteristically opinionated about whitespace surrounding the change. Updated the whitespace, without changes to the logic as written by @KwadroNaut.
|
Rebased on top of 10a2eee (latest develop). |
Status
Ready for review
Description of Changes
Fixes #4100
Changes proposed in this pull request:
By using --batch for refreshing the release key, there's no tty, and the described error will not happen and you can update the key and workstation without manual intervention.
Testing
It's imperative to launch the gui-updater not from a terminal, since this is a known work-around. I've tested with a 0.11 workstation, tried to update with an up-to-date Tails by using the gui.
Deployment
Should improve updates and upgrades. Not aware of any effects to fresh installs.
Checklist
If you made changes to the server application code:
make ci-lint) and tests (make -C securedrop test) pass in the development containerIf you made changes to
securedrop-admin:make -C admin test) pass in the admin development containerI find the setup process burden a bit too high for a random passer-by, I don't have a admin development container, sorry.
Trivial code change:
I looked at admin/tests/ but it looks like they're all launched with a TTY available.