From b1f4b5c32eecf8a39d575f5938571188c4b85ecd Mon Sep 17 00:00:00 2001 From: Tim Richardson Date: Wed, 23 Oct 2024 14:36:00 +0100 Subject: [PATCH] Have securedrop-admin emit distinct error messages Don't emit "Signature verification failed" for multiple different error branches, which makes it far more difficult to diagnose exactly what step failed. Fixes #7200. --- admin/securedrop_admin/__init__.py | 21 +++++++-------------- admin/tests/test_integration.py | 4 ++-- admin/tests/test_securedrop-admin.py | 10 +++++----- 3 files changed, 14 insertions(+), 21 deletions(-) diff --git a/admin/securedrop_admin/__init__.py b/admin/securedrop_admin/__init__.py index ae69c6ba99..4b861c733a 100755 --- a/admin/securedrop_admin/__init__.py +++ b/admin/securedrop_admin/__init__.py @@ -1066,31 +1066,24 @@ def update(args: argparse.Namespace) -> int: and len(good_sig_matches) == 1 and bad_sig_text not in sig_result ): - # Finally, we check that there is no branch of the same name - # prior to reporting success. + # Check for duplicate branch name cmd = ["git", "show-ref", "--heads", "--verify", f"refs/heads/{latest_tag}"] try: - # We expect this to produce a non-zero exit code, which - # will produce a subprocess.CalledProcessError subprocess.check_output(cmd, stderr=subprocess.STDOUT, cwd=args.root) - sdlog.info("Signature verification failed.") + sdlog.error("Update failed: Branch name collision detected") return 1 except subprocess.CalledProcessError as e: if "not a valid ref" in e.output.decode("utf-8"): - # Then there is no duplicate branch. sdlog.info("Signature verification successful.") - else: # If any other exception occurs, we bail. - sdlog.info("Signature verification failed.") + else: + sdlog.error("Update failed: Git command error") return 1 - else: # If anything else happens, fail and exit 1 - sdlog.info("Signature verification failed.") + else: + sdlog.error("Update failed: Invalid signature format") return 1 except subprocess.CalledProcessError: - # If there is no signature, or if the signature does not verify, - # then git tag -v exits subprocess.check_output will exit 1 - # and subprocess.check_output will throw a CalledProcessError - sdlog.info("Signature verification failed.") + sdlog.error("Update failed: Missing or invalid signature") return 1 # Only if the proper signature verifies do we check out the latest diff --git a/admin/tests/test_integration.py b/admin/tests/test_integration.py index 9ccff54b44..90e27429dc 100644 --- a/admin/tests/test_integration.py +++ b/admin/tests/test_integration.py @@ -650,7 +650,7 @@ def test_update_fails_when_no_signature_present(securedrop_git_repo): child = pexpect.spawn(f"coverage run {cmd} --root {ansible_base} update") output = child.read() assert b"Updated to SecureDrop" not in output - assert b"Signature verification failed" in output + assert b"Update failed: Missing or invalid signature" in output child.expect(pexpect.EOF, timeout=10) # Wait for CLI to exit child.close() @@ -682,7 +682,7 @@ def test_update_with_duplicate_branch_and_tag(securedrop_git_repo): # Verify that we do not falsely check out a branch instead of a tag. assert b"Switched to branch" not in output assert b"Updated to SecureDrop" not in output - assert b"Signature verification failed" in output + assert b"Update failed: Branch name collision detected" in output child.expect(pexpect.EOF, timeout=10) # Wait for CLI to exit child.close() diff --git a/admin/tests/test_securedrop-admin.py b/admin/tests/test_securedrop-admin.py index acd65999f2..fe7a59fd8a 100644 --- a/admin/tests/test_securedrop-admin.py +++ b/admin/tests/test_securedrop-admin.py @@ -385,7 +385,7 @@ def test_update_signature_does_not_verify(self, tmpdir, caplog): with mock.patch("subprocess.check_output", return_value=git_output): ret_code = securedrop_admin.update(args) assert "Applying SecureDrop updates..." in caplog.text - assert "Signature verification failed." in caplog.text + assert "Update failed: Invalid signature format" in caplog.text assert "Updated to SecureDrop" not in caplog.text assert ret_code != 0 @@ -406,7 +406,7 @@ def test_update_malicious_key_named_fingerprint(self, tmpdir, caplog): with mock.patch("subprocess.check_output", return_value=git_output): ret_code = securedrop_admin.update(args) assert "Applying SecureDrop updates..." in caplog.text - assert "Signature verification failed." in caplog.text + assert "Update failed: Invalid signature format" in caplog.text assert "Updated to SecureDrop" not in caplog.text assert ret_code != 0 @@ -428,7 +428,7 @@ def test_update_malicious_key_named_good_sig(self, tmpdir, caplog): with mock.patch("subprocess.check_output", return_value=git_output): ret_code = securedrop_admin.update(args) assert "Applying SecureDrop updates..." in caplog.text - assert "Signature verification failed." in caplog.text + assert "Update failed: Invalid signature format" in caplog.text assert "Updated to SecureDrop" not in caplog.text assert ret_code != 0 @@ -451,7 +451,7 @@ def test_update_malicious_key_named_good_sig_fingerprint(self, tmpdir, caplog): with mock.patch("subprocess.check_output", return_value=git_output): ret_code = securedrop_admin.update(args) assert "Applying SecureDrop updates..." in caplog.text - assert "Signature verification failed." in caplog.text + assert "Update failed: Invalid signature format" in caplog.text assert "Updated to SecureDrop" not in caplog.text assert ret_code != 0 @@ -469,7 +469,7 @@ def test_no_signature_on_update(self, tmpdir, caplog): ): ret_code = securedrop_admin.update(args) assert "Applying SecureDrop updates..." in caplog.text - assert "Signature verification failed." in caplog.text + assert "Update failed: Missing or invalid signature" in caplog.text assert "Updated to SecureDrop" not in caplog.text assert ret_code != 0