From 7fd01d908bed4df82f0e0cc1f06e60d8e4f2d569 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Thu, 26 Oct 2023 15:49:13 -0400 Subject: [PATCH] Have JI validate journalist key is valid During the Sequoia migration, we need to export the journalist public key from the GPG keyring into a file on disk. We also needed to validate the key was usable by Sequoia (e.g. no SHA-1 binding signatures). Previously the plan was to validate it during the postinst and error out if it wasn't valid, but if validation fails for whatever reason, then we abort, which interrupts the postinst, so, e.g. database upgrades won't be applied. In retrospect having the validation fail at package install time is bad, because it requires even more manual work to get an instance in a working state since you need to manually apply the updates after doing a key rotation. Now we validate the journalist key during startup of the Journalist Interface, printing and logging an error if it doesn't validate and then exiting. This should bring attention to journalists and therefore the admin that the instance needs manual attention. We will also include information about this change in the pre-release and release announcements. Fixes #7030. --- .../debian/securedrop-app-code.postinst | 3 +- securedrop/journalist.py | 24 +++++++++++++- securedrop/scripts/validate-pgp-key | 32 ------------------- securedrop/tests/test_journalist.py | 20 ++++++++++++ 4 files changed, 44 insertions(+), 35 deletions(-) delete mode 100755 securedrop/scripts/validate-pgp-key diff --git a/securedrop/debian/securedrop-app-code.postinst b/securedrop/debian/securedrop-app-code.postinst index 77167a3319..90ff93a23d 100644 --- a/securedrop/debian/securedrop-app-code.postinst +++ b/securedrop/debian/securedrop-app-code.postinst @@ -186,8 +186,7 @@ export_journalist_public_key() { # Export the GPG public key # shellcheck disable=SC2024 sudo -u www-data gpg2 --homedir=/var/lib/securedrop/keys --export --armor "$fingerprint" > $journalist_pub - # Verify integrity of what we just exported - sudo -u www-data /var/www/securedrop/scripts/validate-pgp-key "$journalist_pub" "$fingerprint" + # We explicitly do not validate the exported key here, that is done during JI startup fi fi diff --git a/securedrop/journalist.py b/securedrop/journalist.py index edb43e535f..1374599d3f 100644 --- a/securedrop/journalist.py +++ b/securedrop/journalist.py @@ -1,9 +1,13 @@ +import sys + from encryption import EncryptionManager, GpgKeyNotFoundError from execution import asynchronous from journalist_app import create_app from models import Source from sdconfig import SecureDropConfig +import redwood + config = SecureDropConfig.get_current() # app is imported by journalist.wsgi app = create_app(config) @@ -21,10 +25,28 @@ def prime_keycache() -> None: pass -prime_keycache() +def validate_journalist_key() -> None: + """Verify the journalist PGP key is valid""" + encryption_mgr = EncryptionManager.get_default() + # First check that we can read it + try: + journalist_key = encryption_mgr.get_journalist_public_key() + except Exception as e: + print(f"ERROR: Unable to read journalist public key: {e}", file=sys.stderr) + app.logger.error(f"ERROR: Unable to read journalist public key: {e}") + sys.exit(1) + # And then what we read is valid + try: + redwood.is_valid_public_key(journalist_key) + except redwood.RedwoodError as e: + print(f"ERROR: Journalist public key is not valid: {e}", file=sys.stderr) + app.logger.error(f"ERROR: Journalist public key is not valid: {e}") + sys.exit(1) if __name__ == "__main__": # pragma: no cover + validate_journalist_key() + prime_keycache() debug = getattr(config, "env", "prod") != "prod" # nosemgrep: python.flask.security.audit.app-run-param-config.avoid_app_run_with_bad_host app.run(debug=debug, host="0.0.0.0", port=8081) diff --git a/securedrop/scripts/validate-pgp-key b/securedrop/scripts/validate-pgp-key deleted file mode 100755 index e916444ac8..0000000000 --- a/securedrop/scripts/validate-pgp-key +++ /dev/null @@ -1,32 +0,0 @@ -#!/opt/venvs/securedrop-app-code/bin/python -""" -Verify PGP key file contains: - -* a valid PGP key. -* one PGP key. -* a PGP key matching the specified fingerprint. - -It creates a temporary GPG keyring, imports they key and verifies -that only the specified fingerprint was imported -""" - -import argparse -import sys -from pathlib import Path - -import redwood - -parser = argparse.ArgumentParser(description="Verify PGP key file") -parser.add_argument("pubkey", type=Path, help="Public key file") -parser.add_argument("fingerprint", help="Fingerprint of key") -args = parser.parse_args() - -pubkey = args.pubkey.read_text() - -fingerprint = redwood.is_valid_public_key(pubkey) -if fingerprint == args.fingerprint: - print("Success! Specified fingerprint matches pubkey file.") - sys.exit(0) -else: - print(f"Failed! Key contains {fingerprint}, not {args.fingerprint}") - sys.exit(1) diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 02ddd9e91d..03be2530d2 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -17,6 +17,7 @@ from flaky import flaky from flask import escape, g, url_for from flask_babel import gettext, ngettext +from journalist import validate_journalist_key from journalist_app.sessions import session from journalist_app.utils import mark_seen from models import ( @@ -51,6 +52,8 @@ from tests.utils.instrument import InstrumentedApp from two_factor import TOTP +from redwood import RedwoodError + from .utils import create_legacy_gpg_key, login_journalist # Smugly seed the RNG for deterministic testing @@ -3772,3 +3775,20 @@ def test_journalist_deletion(journalist_app, app_storage): assert len(SeenReply.query.filter_by(journalist_id=deleted.id).all()) == 2 # And there are no login attempts assert JournalistLoginAttempt.query.all() == [] + + +def test_validate_journalist_key(config, capsys): + # The test key passes validation + validate_journalist_key() + # Reading the key file fails + with patch( + "encryption.EncryptionManager.get_journalist_public_key", side_effect=RuntimeError("err") + ): + with pytest.raises(SystemExit): + validate_journalist_key() + assert capsys.readouterr().err == "ERROR: Unable to read journalist public key: err\n" + # Key fails validation + with patch("redwood.is_valid_public_key", side_effect=RedwoodError("err")): + with pytest.raises(SystemExit): + validate_journalist_key() + assert capsys.readouterr().err == "ERROR: Journalist public key is not valid: err\n"