Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have JI validate journalist key is valid #7035

Merged
merged 1 commit into from
Oct 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions securedrop/debian/securedrop-app-code.postinst
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
24 changes: 23 additions & 1 deletion securedrop/journalist.py
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -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}")
Comment on lines +35 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Why both print() and app.logger.error() here? Is that for capsys's benefit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to scream as loudly as possible that there's an issue. I started with print because I thought it might be to early to use a logger, but no, the logger seemed fine, so then I added it in as an extra thing but figured leaving in the print could help and wouldn't be harmful.

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)
32 changes: 0 additions & 32 deletions securedrop/scripts/validate-pgp-key

This file was deleted.

20 changes: 20 additions & 0 deletions securedrop/tests/test_journalist.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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"