Merge pull request #5788 from freedomofpress/5317-check-for-updates

Check for updates before most securedrop-admin commands

admin/securedrop_admin/__init__.py

@@ -24,6 +24,7 @@
 """
 
 import argparse
+import functools
 import ipaddress
 import logging
 import os
@@ -45,6 +46,9 @@
 from pkg_resources import parse_version
 from cryptography.hazmat.primitives import serialization
 from cryptography.hazmat.primitives.asymmetric import x25519
+
+from typing import cast
+
 from typing import List
 
 from typing import Set
@@ -80,6 +84,11 @@ class JournalistAlertEmailException(Exception):
 
 # The type of each entry within SiteConfig.desc
 _T = TypeVar('_T', bound=Union[int, str, bool])
+
+# The function type used for the @update_check_required decorator; see
+# https://mypy.readthedocs.io/en/stable/generics.html#declaring-decorators
+_FuncT = TypeVar('_FuncT', bound=Callable[..., Any])
+
 # (var, default, type, prompt, validator, transform, condition)
 _DescEntryType = Tuple[str, _T, Type[_T], str, Optional[Validator], Optional[Callable], Callable]
 
@@ -516,7 +525,7 @@ def user_prompt_config(self) -> Dict[str, Any]:
                 self._config_in_progress[var] = ''
                 continue
             self._config_in_progress[var] = self.user_prompt_config_one(desc,
-                                                            self.config.get(var))  # noqa: E501
+                                                                        self.config.get(var))  # noqa: E501
         return self._config_in_progress
 
     def user_prompt_config_one(
@@ -690,6 +699,57 @@ def setup_logger(verbose: bool = False) -> None:
     sdlog.addHandler(stdout)
 
 
+def update_check_required(cmd_name: str) -> Callable[[_FuncT], _FuncT]:
+    """
+    This decorator can be added to any subcommand that is part of securedrop-admin
+    via `@update_check_required("name_of_subcommand")`. It forces a check for
+    updates, and aborts if the locally installed code is out of date. It should
+    be generally added to all subcommands that make modifications on the
+    server or on the Admin Workstation.
+
+    The user can override this check by specifying the --force argument before
+    any subcommand.
+    """
+    def decorator_update_check(func: _FuncT) -> _FuncT:
+        @functools.wraps(func)
+        def wrapper(*args: Any, **kwargs: Any) -> Any:
+            cli_args = args[0]
+            if cli_args.force:
+                sdlog.info("Skipping update check because --force argument was provided.")
+                return func(*args, **kwargs)
+
+            update_status, latest_tag = check_for_updates(cli_args)
+            if update_status is True:
+
+                # Useful for troubleshooting
+                branch_status = get_git_branch(cli_args)
+
+                sdlog.error("You are not running the most recent signed SecureDrop release "
+                            "on this workstation.")
+                sdlog.error("Latest available version: {}".format(latest_tag))
+
+                if branch_status is not None:
+                    sdlog.error("Current branch status: {}".format(branch_status))
+                else:
+                    sdlog.error("Problem determining current branch status.")
+
+                sdlog.error("Running outdated or mismatched code can cause significant "
+                            "technical issues.")
+                sdlog.error("To display more information about your repository state, run:\n\n\t"
+                            "git status\n")
+                sdlog.error("If you are certain you want to proceed, run:\n\n\t"
+                            "./securedrop-admin --force {}\n".format(cmd_name))
+                sdlog.error("To apply the latest updates, run:\n\n\t"
+                            "./securedrop-admin update\n")
+                sdlog.error("If this fails, see the latest upgrade guide on "
+                            "https://docs.securedrop.org/ for instructions.")
+                sys.exit(1)
+            return func(*args, **kwargs)
+        return cast(_FuncT, wrapper)
+    return decorator_update_check
+
+
+@update_check_required("sdconfig")
 def sdconfig(args: argparse.Namespace) -> int:
     """Configure SD site settings"""
     SiteConfig(args).load_and_update_config(validate=False)
@@ -752,8 +812,10 @@ def find_or_generate_new_torv3_keys(args: argparse.Namespace) -> int:
     return 0
 
 
+@update_check_required("install")
 def install_securedrop(args: argparse.Namespace) -> int:
     """Install/Update SecureDrop"""
+
     SiteConfig(args).load_and_update_config(prompt=False)
 
     sdlog.info("Now installing SecureDrop on remote servers.")
@@ -767,6 +829,7 @@ def install_securedrop(args: argparse.Namespace) -> int:
     )
 
 
+@update_check_required("verify")
 def verify_install(args: argparse.Namespace) -> int:
     """Run configuration tests against SecureDrop servers"""
 
@@ -776,6 +839,7 @@ def verify_install(args: argparse.Namespace) -> int:
                                  cwd=os.getcwd())
 
 
+@update_check_required("backup")
 def backup_securedrop(args: argparse.Namespace) -> int:
     """Perform backup of the SecureDrop Application Server.
     Creates a tarball of submissions and server config, and fetches
@@ -789,6 +853,7 @@ def backup_securedrop(args: argparse.Namespace) -> int:
     return subprocess.check_call(ansible_cmd, cwd=args.ansible_path)
 
 
+@update_check_required("restore")
 def restore_securedrop(args: argparse.Namespace) -> int:
     """Perform restore of the SecureDrop Application Server.
     Requires a tarball of submissions and server config, created via
@@ -825,6 +890,7 @@ def restore_securedrop(args: argparse.Namespace) -> int:
     return subprocess.check_call(ansible_cmd, cwd=args.ansible_path)
 
 
+@update_check_required("tailsconfig")
 def run_tails_config(args: argparse.Namespace) -> int:
     """Configure Tails environment post SD install"""
     sdlog.info("Configuring Tails workstation environment")
@@ -851,7 +917,10 @@ def check_for_updates(args: argparse.Namespace) -> Tuple[bool, str]:
     """Check for SecureDrop updates"""
     sdlog.info("Checking for SecureDrop updates...")
 
-    # Determine what branch we are on
+    # Determine what tag we are likely to be on. Caveat: git describe
+    # may produce very surprising results, because it will locate the most recent
+    # _reachable_ tag. However, in our current branching model, it can be
+    # relied on to determine if we're on the latest tag or not.
     current_tag = subprocess.check_output(['git', 'describe'],
                                           cwd=args.root).decode('utf-8').rstrip('\n')  # noqa: E501
 
@@ -879,6 +948,19 @@ def check_for_updates(args: argparse.Namespace) -> Tuple[bool, str]:
     return False, latest_tag
 
 
+def get_git_branch(args: argparse.Namespace) -> Optional[str]:
+    """
+    Returns the starred line of `git branch` output.
+    """
+    git_branch_raw = subprocess.check_output(['git', 'branch'],
+                                             cwd=args.root).decode('utf-8')
+    match = re.search(r"\* (.*)\n", git_branch_raw)
+    if match is not None and len(match.groups()) > 0:
+        return match.group(1)
+    else:
+        return None
+
+
 def get_release_key_from_keyserver(
     args: argparse.Namespace, keyserver: Optional[str] = None, timeout: int = 45
 ) -> None:
@@ -972,6 +1054,7 @@ def update(args: argparse.Namespace) -> int:
     return 0
 
 
+@update_check_required("logs")
 def get_logs(args: argparse.Namespace) -> int:
     """Get logs for forensics and debugging purposes"""
     sdlog.info("Gathering logs for forensics and debugging")
@@ -998,6 +1081,7 @@ def set_default_paths(args: argparse.Namespace) -> argparse.Namespace:
     return args
 
 
+@update_check_required("reset_admin_access")
 def reset_admin_access(args: argparse.Namespace) -> int:
     """Resets SSH access to the SecureDrop servers, locking it to
     this Admin Workstation."""
@@ -1021,6 +1105,8 @@ class ArgParseFormatterCombo(argparse.ArgumentDefaultsHelpFormatter,
                         help="Increase verbosity on output")
     parser.add_argument('-d', action='store_true', default=False,
                         help="Developer mode. Not to be used in production.")
+    parser.add_argument('--force', action='store_true', required=False,
+                        help="force command execution without update check")
     parser.add_argument('--root', required=True,
                         help="path to the root of the SecureDrop repository")
     parser.add_argument('--site-config',

admin/tests/test_integration.py

@@ -359,7 +359,7 @@ def verify_install_has_valid_config():
     Checks that securedrop-admin install validates the configuration.
     """
     cmd = os.path.join(os.path.dirname(CURRENT_DIR), 'securedrop_admin/__init__.py')
-    child = pexpect.spawn('python {0} --root {1} install'.format(cmd, SD_DIR))
+    child = pexpect.spawn('python {0} --force --root {1} install'.format(cmd, SD_DIR))
     child.expect(b"SUDO password:", timeout=5)
     child.close()
 
@@ -369,7 +369,7 @@ def test_install_with_no_config():
     Checks that securedrop-admin install complains about a missing config file.
     """
     cmd = os.path.join(os.path.dirname(CURRENT_DIR), 'securedrop_admin/__init__.py')
-    child = pexpect.spawn('python {0} --root {1} install'.format(cmd, SD_DIR))
+    child = pexpect.spawn('python {0} --force --root {1} install'.format(cmd, SD_DIR))
     child.expect(b'ERROR: Please run "securedrop-admin sdconfig" first.', timeout=5)
     child.expect(pexpect.EOF, timeout=5)
     child.close()
@@ -380,7 +380,7 @@ def test_install_with_no_config():
 def test_sdconfig_on_first_run():
     cmd = os.path.join(os.path.dirname(CURRENT_DIR),
                        'securedrop_admin/__init__.py')
-    child = pexpect.spawn('python {0} --root {1} sdconfig'.format(cmd, SD_DIR))
+    child = pexpect.spawn('python {0} --force --root {1} sdconfig'.format(cmd, SD_DIR))
     verify_username_prompt(child)
     child.sendline('')
     verify_reboot_prompt(child)
@@ -444,7 +444,7 @@ def test_sdconfig_on_first_run():
 def test_sdconfig_both_v2_v3_true():
     cmd = os.path.join(os.path.dirname(CURRENT_DIR),
                        'securedrop_admin/__init__.py')
-    child = pexpect.spawn('python {0} --root {1} sdconfig'.format(cmd, SD_DIR))
+    child = pexpect.spawn('python {0} --force --root {1} sdconfig'.format(cmd, SD_DIR))
     verify_username_prompt(child)
     child.sendline('')
     verify_reboot_prompt(child)
@@ -508,7 +508,7 @@ def test_sdconfig_both_v2_v3_true():
 def test_sdconfig_only_v2_true():
     cmd = os.path.join(os.path.dirname(CURRENT_DIR),
                        'securedrop_admin/__init__.py')
-    child = pexpect.spawn('python {0} --root {1} sdconfig'.format(cmd, SD_DIR))
+    child = pexpect.spawn('python {0} --force --root {1} sdconfig'.format(cmd, SD_DIR))
     verify_username_prompt(child)
     child.sendline('')
     verify_reboot_prompt(child)
@@ -572,7 +572,7 @@ def test_sdconfig_only_v2_true():
 def test_sdconfig_enable_journalist_alerts():
     cmd = os.path.join(os.path.dirname(CURRENT_DIR),
                        'securedrop_admin/__init__.py')
-    child = pexpect.spawn('python {0} --root {1} sdconfig'.format(cmd, SD_DIR))
+    child = pexpect.spawn('python {0} --force --root {1} sdconfig'.format(cmd, SD_DIR))
     verify_username_prompt(child)
     child.sendline('')
     verify_reboot_prompt(child)
@@ -641,7 +641,7 @@ def test_sdconfig_enable_journalist_alerts():
 def test_sdconfig_enable_https_on_source_interface():
     cmd = os.path.join(os.path.dirname(CURRENT_DIR),
                        'securedrop_admin/__init__.py')
-    child = pexpect.spawn('python {0} --root {1} sdconfig'.format(cmd, SD_DIR))
+    child = pexpect.spawn('python {0} --force --root {1} sdconfig'.format(cmd, SD_DIR))
     verify_username_prompt(child)
     child.sendline('')
     verify_reboot_prompt(child)

admin/tests/test_securedrop-admin.py

@@ -55,6 +55,80 @@ def test_not_verbose(self, capsys):
         assert 'HIDDEN' not in out
         assert 'VISIBLE' in out
 
+    def test_update_check_decorator_when_no_update_needed(self, caplog):
+        """
+        When a function decorated with `@update_check_required` is run
+          And the `--force` argument was not given
+          And no update is required
+        Then the update check should run to completion
+          And no errors should be displayed
+          And the program should not exit
+          And the decorated function should be run
+        """
+        with mock.patch(
+            "securedrop_admin.check_for_updates", side_effect=[[False, "1.5.0"]]
+        ) as mocked_check, mock.patch(
+            "securedrop_admin.get_git_branch", side_effect=["develop"]
+        ), mock.patch("sys.exit") as mocked_exit:
+            # The decorator itself interprets --force
+            args = argparse.Namespace(force=False)
+            rv = securedrop_admin.update_check_required("update_check_test")(
+                lambda _: 100
+            )(args)
+            assert mocked_check.called
+            assert not mocked_exit.called
+            assert rv == 100
+            assert caplog.text == ''
+
+    def test_update_check_decorator_when_update_needed(self, caplog):
+        """
+        When a function decorated with `@update_check_required` is run
+          And the `--force` argument was not given
+          And an update is required
+        Then the update check should run to completion
+          And an error referencing the command should be displayed
+          And the current branch state should be included in the output
+          And the program should exit
+        """
+        with mock.patch(
+            "securedrop_admin.check_for_updates", side_effect=[[True, "1.5.0"]]
+        ) as mocked_check, mock.patch(
+            "securedrop_admin.get_git_branch", side_effect=["bad_branch"]
+        ), mock.patch("sys.exit") as mocked_exit:
+            # The decorator itself interprets --force
+            args = argparse.Namespace(force=False)
+            securedrop_admin.update_check_required("update_check_test")(
+                lambda _: _
+            )(args)
+            assert mocked_check.called
+            assert mocked_exit.called
+            assert "update_check_test" in caplog.text
+            assert "bad_branch" in caplog.text
+
+    def test_update_check_decorator_when_skipped(self, caplog):
+        """
+        When a function decorated with `@update_check_required` is run
+          And the `--force` argument was given
+        Then the update check should not run
+          And a message should be displayed acknowledging this
+          And the program should not exit
+          And the decorated function should be run
+        """
+        with mock.patch(
+            "securedrop_admin.check_for_updates", side_effect=[[True, "1.5.0"]]
+        ) as mocked_check, mock.patch(
+            "securedrop_admin.get_git_branch", side_effect=["develop"]
+        ), mock.patch("sys.exit") as mocked_exit:
+            # The decorator itself interprets --force
+            args = argparse.Namespace(force=True)
+            rv = securedrop_admin.update_check_required("update_check_test")(
+                lambda _: 100
+            )(args)
+            assert not mocked_check.called
+            assert not mocked_exit.called
+            assert "--force" in caplog.text
+            assert rv == 100
+
     def test_check_for_updates_update_needed(self, tmpdir, caplog):
         git_repo_path = str(tmpdir)
         args = argparse.Namespace(root=git_repo_path)
@@ -128,6 +202,40 @@ def test_check_for_updates_if_most_recent_tag_is_rc(self, tmpdir, caplog):
                 assert update_status is False
                 assert tag == '0.6.1'
 
+    @pytest.mark.parametrize(
+        "git_output, expected_rv",
+        [
+            (b'* develop\n',
+             'develop'),
+            (b' develop\n'
+             b'* release/1.7.0\n',
+             'release/1.7.0'),
+            (b'* (HEAD detached at 1.7.0)\n'
+             b'  develop\n'
+             b'  release/1.7.0\n',
+             '(HEAD detached at 1.7.0)'),
+            (b'  main\n'
+             b'* valid_+!@#$%&_branch_name\n',
+             'valid_+!@#$%&_branch_name'),
+            (b'Unrecognized output.',
+             None)
+        ]
+    )
+    def test_get_git_branch(self, git_output, expected_rv):
+        """
+        When `git branch` completes with exit code 0
+          And the output conforms to the expected format
+          Then `get_git_branch` should return a description of the current HEAD
+
+        When `git branch` completes with exit code 0
+          And the output does not conform to the expected format
+          Then `get_git_branch` should return `None`
+        """
+        args = argparse.Namespace(root=None)
+        with mock.patch('subprocess.check_output', side_effect=[git_output]):
+            rv = securedrop_admin.get_git_branch(args)
+            assert rv == expected_rv
+
     def test_update_exits_if_not_needed(self, tmpdir, caplog):
         git_repo_path = str(tmpdir)
         args = argparse.Namespace(root=git_repo_path)