Merge pull request freedomofpress#7272 from timini/bigfix-7200

Have securedrop-admin emit distinct error messages

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

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()

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