From 7d0f3fbc0ac2cc128ef3d8f4caf6393d35168514 Mon Sep 17 00:00:00 2001 From: John Hensley Date: Wed, 1 Apr 2020 10:27:07 -0400 Subject: [PATCH 1/9] Rename CryptoUtil.getkey It actually returns a fingerprint, not a key. --- securedrop/create-dev-data.py | 2 +- securedrop/crypto_util.py | 11 ++++++-- securedrop/journalist.py | 2 +- securedrop/journalist_app/col.py | 2 +- securedrop/journalist_app/main.py | 2 +- securedrop/models.py | 2 +- securedrop/source_app/main.py | 4 +-- .../tests/functional/functional_test.py | 2 +- securedrop/tests/test_crypto_util.py | 28 +++++++++---------- securedrop/tests/test_integration.py | 2 +- securedrop/tests/test_journalist.py | 4 +-- securedrop/tests/test_journalist_api.py | 2 +- securedrop/tests/utils/db_helper.py | 2 +- 13 files changed, 35 insertions(+), 30 deletions(-) diff --git a/securedrop/create-dev-data.py b/securedrop/create-dev-data.py index fa1609089e..f04bdb8d7f 100755 --- a/securedrop/create-dev-data.py +++ b/securedrop/create-dev-data.py @@ -124,7 +124,7 @@ def create_source_and_submissions(num_submissions=2, num_replies=2, source.journalist_filename) current_app.crypto_util.encrypt( next(replies), - [current_app.crypto_util.getkey(source.filesystem_id), + [current_app.crypto_util.get_fingerprint(source.filesystem_id), config.JOURNALIST_KEY], current_app.storage.path(source.filesystem_id, fname)) diff --git a/securedrop/crypto_util.py b/securedrop/crypto_util.py index bd595234dd..39d25503ed 100644 --- a/securedrop/crypto_util.py +++ b/securedrop/crypto_util.py @@ -243,7 +243,7 @@ def genkeypair(self, name, secret): return genkey_obj def delete_reply_keypair(self, source_filesystem_id): - key = self.getkey(source_filesystem_id) + key = self.get_fingerprint(source_filesystem_id) # If this source was never flagged for review, they won't have a reply # keypair if not key: @@ -256,7 +256,12 @@ def delete_reply_keypair(self, source_filesystem_id): temp_gpg.delete_keys(key, secret=True, subkeys=True) self.keycache.delete(source_filesystem_id) - def getkey(self, name): + def get_fingerprint(self, name): + """ + Returns the fingerprint of the GPG key for the given name. + + The supplied name is usually a source filesystem ID. + """ fingerprint = self.keycache.get(name) if fingerprint: # cache hit return fingerprint @@ -271,7 +276,7 @@ def getkey(self, name): return None def export_pubkey(self, name): - fingerprint = self.getkey(name) + fingerprint = self.get_fingerprint(name) if fingerprint: return self.gpg.export_keys(fingerprint) else: diff --git a/securedrop/journalist.py b/securedrop/journalist.py index 7a142fe6f2..8873b90c0c 100644 --- a/securedrop/journalist.py +++ b/securedrop/journalist.py @@ -16,7 +16,7 @@ def prime_keycache(): """ with app.app_context(): for source in Source.query.filter_by(pending=False).all(): - app.crypto_util.getkey(source.filesystem_id) + app.crypto_util.get_fingerprint(source.filesystem_id) prime_keycache() diff --git a/securedrop/journalist_app/col.py b/securedrop/journalist_app/col.py index 0703fb9f57..0f92b04e39 100644 --- a/securedrop/journalist_app/col.py +++ b/securedrop/journalist_app/col.py @@ -33,7 +33,7 @@ def remove_star(filesystem_id): def col(filesystem_id): form = ReplyForm() source = get_source(filesystem_id) - source.has_key = current_app.crypto_util.getkey(filesystem_id) + source.has_key = current_app.crypto_util.get_fingerprint(filesystem_id) return render_template("col.html", filesystem_id=filesystem_id, source=source, form=form) diff --git a/securedrop/journalist_app/main.py b/securedrop/journalist_app/main.py index 59c2689e07..a8658fd472 100644 --- a/securedrop/journalist_app/main.py +++ b/securedrop/journalist_app/main.py @@ -108,7 +108,7 @@ def reply(): g.source.journalist_filename) current_app.crypto_util.encrypt( form.message.data, - [current_app.crypto_util.getkey(g.filesystem_id), + [current_app.crypto_util.get_fingerprint(g.filesystem_id), config.JOURNALIST_KEY], output=current_app.storage.path(g.filesystem_id, filename), ) diff --git a/securedrop/models.py b/securedrop/models.py index b0b5e5ad05..ae94630d3f 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -118,7 +118,7 @@ def collection(self): @property def fingerprint(self): - return current_app.crypto_util.getkey(self.filesystem_id) + return current_app.crypto_util.get_fingerprint(self.filesystem_id) @fingerprint.setter def fingerprint(self, value): diff --git a/securedrop/source_app/main.py b/securedrop/source_app/main.py index 0921398597..c1748c0166 100644 --- a/securedrop/source_app/main.py +++ b/securedrop/source_app/main.py @@ -130,7 +130,7 @@ def lookup(): # Generate a keypair to encrypt replies from the journalist # Only do this if the journalist has flagged the source as one # that they would like to reply to. (Issue #140.) - if not current_app.crypto_util.getkey(g.filesystem_id) and \ + if not current_app.crypto_util.get_fingerprint(g.filesystem_id) and \ g.source.flagged: db_uri = current_app.config['SQLALCHEMY_DATABASE_URI'] async_genkey(current_app.crypto_util, @@ -145,7 +145,7 @@ def lookup(): replies=replies, flagged=g.source.flagged, new_user=session.get('new_user', None), - haskey=current_app.crypto_util.getkey( + haskey=current_app.crypto_util.get_fingerprint( g.filesystem_id)) @view.route('/submit', methods=('POST',)) diff --git a/securedrop/tests/functional/functional_test.py b/securedrop/tests/functional/functional_test.py index a5b8dcb5fc..640cd5164c 100644 --- a/securedrop/tests/functional/functional_test.py +++ b/securedrop/tests/functional/functional_test.py @@ -307,7 +307,7 @@ def wait_for_source_key(self, source_name): filesystem_id = self.source_app.crypto_util.hash_codename(source_name) def key_available(filesystem_id): - assert self.source_app.crypto_util.getkey(filesystem_id) + assert self.source_app.crypto_util.get_fingerprint(filesystem_id) self.wait_for(lambda: key_available(filesystem_id), timeout=60) diff --git a/securedrop/tests/test_crypto_util.py b/securedrop/tests/test_crypto_util.py index 154f767009..6105bc5d7d 100644 --- a/securedrop/tests/test_crypto_util.py +++ b/securedrop/tests/test_crypto_util.py @@ -40,7 +40,7 @@ def test_encrypt_success(source_app, config, test_source): with source_app.app_context(): ciphertext = source_app.crypto_util.encrypt( message, - [source_app.crypto_util.getkey(test_source['filesystem_id']), + [source_app.crypto_util.get_fingerprint(test_source['filesystem_id']), config.JOURNALIST_KEY], source_app.storage.path(test_source['filesystem_id'], 'somefile.gpg')) @@ -70,7 +70,7 @@ def test_encrypt_without_output(source_app, config, test_source): with source_app.app_context(): ciphertext = source_app.crypto_util.encrypt( message, - [source_app.crypto_util.getkey(test_source['filesystem_id']), + [source_app.crypto_util.get_fingerprint(test_source['filesystem_id']), config.JOURNALIST_KEY]) plaintext = source_app.crypto_util.decrypt( test_source['codename'], @@ -96,7 +96,7 @@ def test_encrypt_binary_stream(source_app, config, test_source): with io.open(os.path.realpath(__file__)) as fh: ciphertext = source_app.crypto_util.encrypt( fh, - [source_app.crypto_util.getkey(test_source['filesystem_id']), + [source_app.crypto_util.get_fingerprint(test_source['filesystem_id']), config.JOURNALIST_KEY], source_app.storage.path(test_source['filesystem_id'], 'somefile.gpg')) @@ -116,7 +116,7 @@ def test_encrypt_fingerprints_not_a_list_or_tuple(source_app, test_source): with source_app.app_context(): ciphertext = source_app.crypto_util.encrypt( message, - source_app.crypto_util.getkey(test_source['filesystem_id']), + source_app.crypto_util.get_fingerprint(test_source['filesystem_id']), source_app.storage.path(test_source['filesystem_id'], 'somefile.gpg')) plaintext = source_app.crypto_util.decrypt(test_source['codename'], @@ -133,7 +133,7 @@ def test_basic_encrypt_then_decrypt_multiple_recipients(source_app, with source_app.app_context(): ciphertext = source_app.crypto_util.encrypt( message, - [source_app.crypto_util.getkey(test_source['filesystem_id']), + [source_app.crypto_util.get_fingerprint(test_source['filesystem_id']), config.JOURNALIST_KEY], source_app.storage.path(test_source['filesystem_id'], 'somefile.gpg')) @@ -208,7 +208,7 @@ def test_genkeypair(source_app): db.session.commit() source_app.crypto_util.genkeypair(source.filesystem_id, codename) - assert source_app.crypto_util.getkey(filesystem_id) is not None + assert source_app.crypto_util.get_fingerprint(filesystem_id) is not None def parse_gpg_date_string(date_string): @@ -241,15 +241,15 @@ def test_reply_keypair_creation_and_expiration_dates(source_app): db.session.commit() source_app.crypto_util.genkeypair(source.filesystem_id, codename) - # crypto_util.getkey only returns the fingerprint of the key. We need + # crypto_util.get_fingerprint only returns the fingerprint of the key. We need # the full output of gpg.list_keys() to check the creation and # expire dates. # - # TODO: it might be generally useful to refactor crypto_util.getkey so + # TODO: it might be generally useful to refactor crypto_util.get_fingerprint so # it always returns the entire key dictionary instead of just the # fingerprint (which is always easily extracted from the entire key # dictionary). - new_key_fingerprint = source_app.crypto_util.getkey(filesystem_id) + new_key_fingerprint = source_app.crypto_util.get_fingerprint(filesystem_id) new_key = [key for key in source_app.crypto_util.gpg.list_keys() if new_key_fingerprint == key['fingerprint']][0] @@ -267,7 +267,7 @@ def test_reply_keypair_creation_and_expiration_dates(source_app): def test_delete_reply_keypair(source_app, test_source): fid = test_source['filesystem_id'] source_app.crypto_util.delete_reply_keypair(fid) - assert source_app.crypto_util.getkey(fid) is None + assert source_app.crypto_util.get_fingerprint(fid) is None def test_delete_reply_keypair_pinentry_status_is_handled(source_app, test_source, @@ -285,7 +285,7 @@ def test_delete_reply_keypair_pinentry_status_is_handled(source_app, test_source captured = capsys.readouterr() assert "ValueError: Unknown status message: 'PINENTRY_LAUNCHED'" not in captured.err - assert source_app.crypto_util.getkey(fid) is None + assert source_app.crypto_util.get_fingerprint(fid) is None def test_delete_reply_keypair_no_key(source_app): @@ -295,12 +295,12 @@ def test_delete_reply_keypair_no_key(source_app): source_app.crypto_util.delete_reply_keypair('Reality Winner') -def test_getkey(source_app, test_source): - assert (source_app.crypto_util.getkey(test_source['filesystem_id']) +def test_get_fingerprint(source_app, test_source): + assert (source_app.crypto_util.get_fingerprint(test_source['filesystem_id']) is not None) # check that a non-existent key returns None - assert source_app.crypto_util.getkey('x' * 50) is None + assert source_app.crypto_util.get_fingerprint('x' * 50) is None def test_export_pubkey(source_app, test_source): diff --git a/securedrop/tests/test_integration.py b/securedrop/tests/test_integration.py index 971a1ac4bf..345ad0ee10 100644 --- a/securedrop/tests/test_integration.py +++ b/securedrop/tests/test_integration.py @@ -304,7 +304,7 @@ def _helper_test_reply(journalist_app, source_app, config, test_journo, # Block up to 15s for the reply keypair, so we can test sending a reply def assertion(): - assert current_app.crypto_util.getkey(filesystem_id) is not None + assert current_app.crypto_util.get_fingerprint(filesystem_id) is not None utils.asynchronous.wait_for_assertion(assertion, 15) # Create 2 replies to test deleting on journalist and source interface diff --git a/securedrop/tests/test_journalist.py b/securedrop/tests/test_journalist.py index 559f5fe213..87c699d74e 100644 --- a/securedrop/tests/test_journalist.py +++ b/securedrop/tests/test_journalist.py @@ -1691,7 +1691,7 @@ def test_delete_source_deletes_source_key(journalist_app, utils.db_helper.reply(journo, source, 2) # Source key exists - source_key = current_app.crypto_util.getkey( + source_key = current_app.crypto_util.get_fingerprint( test_source['filesystem_id']) assert source_key is not None @@ -1699,7 +1699,7 @@ def test_delete_source_deletes_source_key(journalist_app, test_source['filesystem_id']) # Source key no longer exists - source_key = current_app.crypto_util.getkey( + source_key = current_app.crypto_util.get_fingerprint( test_source['filesystem_id']) assert source_key is None diff --git a/securedrop/tests/test_journalist_api.py b/securedrop/tests/test_journalist_api.py index 9a36126a11..e4cac45198 100644 --- a/securedrop/tests/test_journalist_api.py +++ b/securedrop/tests/test_journalist_api.py @@ -652,7 +652,7 @@ def test_authorized_user_can_add_reply(journalist_app, journalist_api_token, # First we must encrypt the reply, or it will get rejected # by the server. - source_key = current_app.crypto_util.getkey( + source_key = current_app.crypto_util.get_fingerprint( test_source['source'].filesystem_id) reply_content = current_app.crypto_util.gpg.encrypt( 'This is a plaintext reply', source_key).data diff --git a/securedrop/tests/utils/db_helper.py b/securedrop/tests/utils/db_helper.py index 53ef7afbeb..320be282e4 100644 --- a/securedrop/tests/utils/db_helper.py +++ b/securedrop/tests/utils/db_helper.py @@ -71,7 +71,7 @@ def reply(journalist, source, num_replies): source.journalist_filename) current_app.crypto_util.encrypt( str(os.urandom(1)), - [current_app.crypto_util.getkey(source.filesystem_id), + [current_app.crypto_util.get_fingerprint(source.filesystem_id), config.JOURNALIST_KEY], current_app.storage.path(source.filesystem_id, fname)) From e3df3ed027d2908d3997548fb6eb07754da5cf57 Mon Sep 17 00:00:00 2001 From: John Hensley Date: Wed, 1 Apr 2020 11:11:16 -0400 Subject: [PATCH 2/9] Add caching of source public keys Also rename the fingerprint cache and increase its maximum size. --- securedrop/crypto_util.py | 45 ++++++++++++++++++---------- securedrop/journalist.py | 2 +- securedrop/models.py | 2 +- securedrop/tests/test_crypto_util.py | 11 ++++--- 4 files changed, 37 insertions(+), 23 deletions(-) diff --git a/securedrop/crypto_util.py b/securedrop/crypto_util.py index 39d25503ed..b62700078f 100644 --- a/securedrop/crypto_util.py +++ b/securedrop/crypto_util.py @@ -78,7 +78,8 @@ def put(self, item, value): self.cache.popitem(last=False) def delete(self, item): - del self.cache[item] + if item in self.cache: + del self.cache[item] class CryptoException(Exception): @@ -99,9 +100,12 @@ class CryptoUtil: # to set an expiration date. DEFAULT_KEY_EXPIRATION_DATE = '0' - keycache_limit = 1000 + keycache_limit = 10000 keycache = FIFOCache(keycache_limit) + fprcache_limit = 10000 + fprcache = FIFOCache(fprcache_limit) + def __init__(self, scrypt_params, scrypt_id_pepper, @@ -114,7 +118,7 @@ def __init__(self, self.__securedrop_root = securedrop_root self.__word_list = word_list - if os.environ.get('SECUREDROP_ENV') == 'test': + if os.environ.get('SECUREDROP_ENV') in ('dev', 'test'): # Optimize crypto to speed up tests (at the expense of security # DO NOT use these settings in production) self.__gpg_key_length = 1024 @@ -254,7 +258,8 @@ def delete_reply_keypair(self, source_filesystem_id): temp_gpg = gnupg.GPG(binary='gpg2', homedir=self.gpg_key_dir) # The subkeys keyword argument deletes both secret and public keys. temp_gpg.delete_keys(key, secret=True, subkeys=True) - self.keycache.delete(source_filesystem_id) + self.fprcache.delete(source_filesystem_id) + self.keycache.delete(self.get_fingerprint(source_filesystem_id)) def get_fingerprint(self, name): """ @@ -262,26 +267,36 @@ def get_fingerprint(self, name): The supplied name is usually a source filesystem ID. """ - fingerprint = self.keycache.get(name) - if fingerprint: # cache hit + fingerprint = self.fprcache.get(name) + if fingerprint: return fingerprint - # cache miss + target_uid = 'Autogenerated Key <{}>'.format(name) for key in self.gpg.list_keys(): - for uid in key['uids']: - if name in uid: - self.keycache.put(name, key['fingerprint']) - return key['fingerprint'] + if target_uid in key['uids']: + self.fprcache.put(name, key['fingerprint']) + return key['fingerprint'] return None - def export_pubkey(self, name): + def get_pubkey(self, name): + """ + Returns the GPG public key for the given name. + + The supplied name is usually a source filesystem ID. + """ fingerprint = self.get_fingerprint(name) - if fingerprint: - return self.gpg.export_keys(fingerprint) - else: + if not fingerprint: return None + key = self.keycache.get(fingerprint) + if key: + return key + + key = self.gpg.export_keys(fingerprint) + self.keycache.put(fingerprint, key) + return key + def encrypt(self, plaintext, fingerprints, output=None): # Verify the output path if output: diff --git a/securedrop/journalist.py b/securedrop/journalist.py index 8873b90c0c..ea8ca9573d 100644 --- a/securedrop/journalist.py +++ b/securedrop/journalist.py @@ -16,7 +16,7 @@ def prime_keycache(): """ with app.app_context(): for source in Source.query.filter_by(pending=False).all(): - app.crypto_util.get_fingerprint(source.filesystem_id) + app.crypto_util.get_pubkey(source.filesystem_id) prime_keycache() diff --git a/securedrop/models.py b/securedrop/models.py index ae94630d3f..402c979ed1 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -131,7 +131,7 @@ def fingerprint(self): @property def public_key(self): # type: () -> str - return current_app.crypto_util.export_pubkey(self.filesystem_id) + return current_app.crypto_util.get_pubkey(self.filesystem_id) @public_key.setter def public_key(self, value): diff --git a/securedrop/tests/test_crypto_util.py b/securedrop/tests/test_crypto_util.py index 6105bc5d7d..ccc9c8c0ff 100644 --- a/securedrop/tests/test_crypto_util.py +++ b/securedrop/tests/test_crypto_util.py @@ -303,17 +303,16 @@ def test_get_fingerprint(source_app, test_source): assert source_app.crypto_util.get_fingerprint('x' * 50) is None -def test_export_pubkey(source_app, test_source): +def test_get_pubkey(source_app, test_source): begin_pgp = '-----BEGIN PGP PUBLIC KEY BLOCK----' # check that a filesystem_id exports the pubkey - exported = source_app.crypto_util.export_pubkey( - test_source['filesystem_id']) - assert exported.startswith(begin_pgp) + pubkey = source_app.crypto_util.get_pubkey(test_source['filesystem_id']) + assert pubkey.startswith(begin_pgp) # check that a non-existent identifer exports None - exported = source_app.crypto_util.export_pubkey('x' * 50) - assert exported is None + pubkey = source_app.crypto_util.get_pubkey('x' * 50) + assert pubkey is None @given( From 5769c230ae1de4616975f4d5d47f1dcb9773bd3c Mon Sep 17 00:00:00 2001 From: John Hensley Date: Wed, 1 Apr 2020 13:48:11 -0400 Subject: [PATCH 3/9] Add source detail to create-dev-data output As sources are being added, include the count in the output, to give a better idea of progress. --- securedrop/create-dev-data.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/securedrop/create-dev-data.py b/securedrop/create-dev-data.py index f04bdb8d7f..ab1bbd477d 100755 --- a/securedrop/create-dev-data.py +++ b/securedrop/create-dev-data.py @@ -58,12 +58,14 @@ def main(staging=False): # Add test sources and submissions num_sources = int(os.getenv('NUM_SOURCES', 2)) - for i in range(num_sources): + for i in range(1, num_sources + 1): if i == 0: # For the first source, the journalist who replied will be deleted - create_source_and_submissions(journalist_who_replied=journalist_tobe_deleted) + create_source_and_submissions( + i, num_sources, journalist_who_replied=journalist_tobe_deleted + ) continue - create_source_and_submissions() + create_source_and_submissions(i, num_sources) # Now let us delete one journalist db.session.delete(journalist_tobe_deleted) db.session.commit() @@ -89,8 +91,9 @@ def add_test_user(username, password, otp_secret, is_admin=False, db.session.rollback() -def create_source_and_submissions(num_submissions=2, num_replies=2, - journalist_who_replied=None): +def create_source_and_submissions( + source_index, source_count, num_submissions=2, num_replies=2, journalist_who_replied=None +): # Store source in database codename = current_app.crypto_util.genrandomid() filesystem_id = current_app.crypto_util.hash_codename(codename) @@ -137,9 +140,13 @@ def create_source_and_submissions(num_submissions=2, num_replies=2, db.session.commit() - print("Test source (codename: '{}', journalist designation '{}') " - "added with {} submissions and {} replies".format( - codename, journalist_designation, num_submissions, num_replies)) + print( + "Test source {}/{} (codename: '{}', journalist designation '{}') " + "added with {} submissions and {} replies".format( + source_index, source_count, codename, journalist_designation, + num_submissions, num_replies + ) + ) if __name__ == "__main__": # pragma: no cover From f40fca2e9bba6772f04ad88939d7dca063f7b5bf Mon Sep 17 00:00:00 2001 From: John Hensley Date: Fri, 3 Apr 2020 15:01:19 -0400 Subject: [PATCH 4/9] Fix: CryptoUtil cache cleaning, create-dev-data.py deleted journo Improve the order of operations when cleaning deleted keys out of the CryptoUtil caches. Fix index of deleted journalist in create-dev-data.py. --- securedrop/create-dev-data.py | 2 +- securedrop/crypto_util.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/securedrop/create-dev-data.py b/securedrop/create-dev-data.py index ab1bbd477d..346c34278e 100755 --- a/securedrop/create-dev-data.py +++ b/securedrop/create-dev-data.py @@ -59,7 +59,7 @@ def main(staging=False): # Add test sources and submissions num_sources = int(os.getenv('NUM_SOURCES', 2)) for i in range(1, num_sources + 1): - if i == 0: + if i == 1: # For the first source, the journalist who replied will be deleted create_source_and_submissions( i, num_sources, journalist_who_replied=journalist_tobe_deleted diff --git a/securedrop/crypto_util.py b/securedrop/crypto_util.py index b62700078f..89ad979c8a 100644 --- a/securedrop/crypto_util.py +++ b/securedrop/crypto_util.py @@ -258,8 +258,8 @@ def delete_reply_keypair(self, source_filesystem_id): temp_gpg = gnupg.GPG(binary='gpg2', homedir=self.gpg_key_dir) # The subkeys keyword argument deletes both secret and public keys. temp_gpg.delete_keys(key, secret=True, subkeys=True) - self.fprcache.delete(source_filesystem_id) self.keycache.delete(self.get_fingerprint(source_filesystem_id)) + self.fprcache.delete(source_filesystem_id) def get_fingerprint(self, name): """ From 4330421065e872e2269d0d5ca7515043c9cfd110 Mon Sep 17 00:00:00 2001 From: John Hensley Date: Fri, 3 Apr 2020 19:11:34 -0400 Subject: [PATCH 5/9] Ensure source key caches are primed at Apache startup By default, mod_wsgi waits for the first request to load the application. By adding the "process-group" and "application-group" options to the "WSGIScriptAlias" directive, we can tell it to load on process start instead, so the caches get populated then instead of forcing the first request to wait. See: https://modwsgi.readthedocs.io/en/latest/configuration-directives/WSGIScriptAlias.html --- .../roles/app/templates/sites-available/journalist.conf | 3 +-- .../staging/app/apache/test_apache_journalist_interface.py | 6 ++++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf b/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf index c7d2245747..ca7274eb83 100644 --- a/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf +++ b/install_files/ansible-base/roles/app/templates/sites-available/journalist.conf @@ -2,8 +2,7 @@ ServerName {{ securedrop_app_apache_listening_address }} WSGIDaemonProcess journalist processes=2 threads=30 display-name=%{GROUP} python-path=/var/www/securedrop -WSGIProcessGroup journalist -WSGIScriptAlias / /var/www/journalist.wsgi +WSGIScriptAlias / /var/www/journalist.wsgi process-group=journalist application-group=journalist WSGIPassAuthorization On # Tell the browser not to cache HTML responses in order to minimize the chance diff --git a/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py b/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py index 4a968f08af..11446b7bf1 100644 --- a/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py +++ b/molecule/testinfra/staging/app/apache/test_apache_journalist_interface.py @@ -92,8 +92,10 @@ def test_apache_headers_journalist_interface(host, header): securedrop_test_vars.apache_listening_address), "WSGIDaemonProcess journalist processes=2 threads=30 display-name=%{{GROUP}} python-path={}".format( # noqa securedrop_test_vars.securedrop_code), - 'WSGIProcessGroup journalist', - 'WSGIScriptAlias / /var/www/journalist.wsgi', + ( + 'WSGIScriptAlias / /var/www/journalist.wsgi ' + 'process-group=journalist application-group=journalist' + ), 'WSGIPassAuthorization On', 'Header set Cache-Control "no-store"', "Alias /static {}/static".format(securedrop_test_vars.securedrop_code), From 639ecf8b4d022a0c7b7d8abd3ed16fbd203c61f5 Mon Sep 17 00:00:00 2001 From: John Hensley Date: Mon, 6 Apr 2020 14:29:32 -0400 Subject: [PATCH 6/9] Use Redis for source key caching --- securedrop/crypto_util.py | 50 ++++++---------------------- securedrop/tests/test_crypto_util.py | 24 +------------ 2 files changed, 12 insertions(+), 62 deletions(-) diff --git a/securedrop/crypto_util.py b/securedrop/crypto_util.py index 89ad979c8a..d67153c724 100644 --- a/securedrop/crypto_util.py +++ b/securedrop/crypto_util.py @@ -1,6 +1,5 @@ # -*- coding: utf-8 -*- -import collections from distutils.version import StrictVersion import pretty_bad_protocol as gnupg import os @@ -12,6 +11,7 @@ from datetime import date from flask import current_app from pretty_bad_protocol._util import _is_stream, _make_binary_stream +from redis import Redis import rm @@ -55,33 +55,6 @@ def monkey_patch_delete_handle_status(self, key, value): gnupg._parsers.DeleteResult._handle_status = monkey_patch_delete_handle_status -class FIFOCache(): - """ - We implemented this simple cache instead of using functools.lru_cache - (this uses a different cache replacement policy (FIFO), but either - FIFO or LRU works for our key fingerprint cache) - due to the inability to remove an item from its cache. - - See: https://bugs.python.org/issue28178 - """ - def __init__(self, maxsize: int): - self.maxsize = maxsize - self.cache = collections.OrderedDict() # type: collections.OrderedDict - - def get(self, item): - if item in self.cache: - return self.cache[item] - - def put(self, item, value): - self.cache[item] = value - if len(self.cache) > self.maxsize: - self.cache.popitem(last=False) - - def delete(self, item): - if item in self.cache: - del self.cache[item] - - class CryptoException(Exception): pass @@ -100,11 +73,8 @@ class CryptoUtil: # to set an expiration date. DEFAULT_KEY_EXPIRATION_DATE = '0' - keycache_limit = 10000 - keycache = FIFOCache(keycache_limit) - - fprcache_limit = 10000 - fprcache = FIFOCache(fprcache_limit) + REDIS_SOURCE_FINGERPRINT_HASH = "sd/crypto-util/source-fingerprints" + REDIS_SOURCE_KEY_HASH = "sd/crypto-util/source-keys" def __init__(self, scrypt_params, @@ -152,6 +122,8 @@ def __init__(self, with io.open(adjectives_file) as f: self.adjectives = f.read().splitlines() + self.redis = Redis(decode_responses=True) + # Make sure these pass before the app can run def do_runtime_tests(self): if self.scrypt_id_pepper == self.scrypt_gpg_pepper: @@ -258,8 +230,8 @@ def delete_reply_keypair(self, source_filesystem_id): temp_gpg = gnupg.GPG(binary='gpg2', homedir=self.gpg_key_dir) # The subkeys keyword argument deletes both secret and public keys. temp_gpg.delete_keys(key, secret=True, subkeys=True) - self.keycache.delete(self.get_fingerprint(source_filesystem_id)) - self.fprcache.delete(source_filesystem_id) + self.redis.hdel(self.REDIS_SOURCE_KEY_HASH, self.get_fingerprint(source_filesystem_id)) + self.redis.hdel(self.REDIS_SOURCE_FINGERPRINT_HASH, source_filesystem_id) def get_fingerprint(self, name): """ @@ -267,14 +239,14 @@ def get_fingerprint(self, name): The supplied name is usually a source filesystem ID. """ - fingerprint = self.fprcache.get(name) + fingerprint = self.redis.hget(self.REDIS_SOURCE_FINGERPRINT_HASH, name) if fingerprint: return fingerprint target_uid = 'Autogenerated Key <{}>'.format(name) for key in self.gpg.list_keys(): if target_uid in key['uids']: - self.fprcache.put(name, key['fingerprint']) + self.redis.hset(self.REDIS_SOURCE_FINGERPRINT_HASH, name, key['fingerprint']) return key['fingerprint'] return None @@ -289,12 +261,12 @@ def get_pubkey(self, name): if not fingerprint: return None - key = self.keycache.get(fingerprint) + key = self.redis.hget(self.REDIS_SOURCE_KEY_HASH, fingerprint) if key: return key key = self.gpg.export_keys(fingerprint) - self.keycache.put(fingerprint, key) + self.redis.hset(self.REDIS_SOURCE_KEY_HASH, fingerprint, key) return key def encrypt(self, plaintext, fingerprints, output=None): diff --git a/securedrop/tests/test_crypto_util.py b/securedrop/tests/test_crypto_util.py index ccc9c8c0ff..2b77cc624a 100644 --- a/securedrop/tests/test_crypto_util.py +++ b/securedrop/tests/test_crypto_util.py @@ -11,7 +11,7 @@ import crypto_util import models -from crypto_util import CryptoUtil, CryptoException, FIFOCache +from crypto_util import CryptoUtil, CryptoException from db import db @@ -342,25 +342,3 @@ def test_encrypt_then_decrypt_gives_same_result( decrypted_text = crypto.decrypt(secret, ciphertext) assert decrypted_text == message - - -def test_fifo_cache(): - cache = FIFOCache(3) - - cache.put('item 1', 1) - cache.put('item 2', 2) - cache.put('item 3', 3) - - assert cache.get('item 1') == 1 - assert cache.get('item 2') == 2 - assert cache.get('item 3') == 3 - - cache.put('item 4', 4) - # Maxsize is 3, so adding item 4 should kick out item 1 - assert not cache.get('item 1') - assert cache.get('item 2') == 2 - assert cache.get('item 3') == 3 - assert cache.get('item 4') == 4 - - cache.delete('item 2') - assert not cache.get('item 2') From b9a77172df3842b3dd6622f3d43d294beae7b3f7 Mon Sep 17 00:00:00 2001 From: John Hensley Date: Tue, 7 Apr 2020 17:51:18 -0400 Subject: [PATCH 7/9] Fix WSGI app init in journalist Apache config --- .../securedrop-app-code/debian/postinst | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/install_files/securedrop-app-code/debian/postinst b/install_files/securedrop-app-code/debian/postinst index 3d156a8487..63541cd25f 100644 --- a/install_files/securedrop-app-code/debian/postinst +++ b/install_files/securedrop-app-code/debian/postinst @@ -65,18 +65,33 @@ database_migration() { fi } -# Supports passing authorization headers for the SecureDrop API. -# Only affects the Journalist Interface. Required for unattended upgrade -# to v0.9.0. -function permit_wsgi_authorization() { +function adjust_wsgi_configuration { journalist_conf="/etc/apache2/sites-available/journalist.conf" if test -f $journalist_conf; then - # First we check whether the line is poresent. + # Supports passing authorization headers for the SecureDrop API. + # Only affects the Journalist Interface. Required for unattended upgrade + # to v0.9.0. + # + # First we check whether the line is present. # Next we find a target line to anchor the insertion. # Then we insert the line, along with the target line that was matched. if ! grep -qP '^WSGIPassAuthorization' "$journalist_conf"; then perl -pi -e 's/^(WSGIScriptAlias .*)/$1\nWSGIPassAuthorization On/' "$journalist_conf" fi + + # Add process-group and application-group to WSGIScriptAlias + # to ensure the application is initialized at process start + # instead of waiting for the first request. (See + # https://modwsgi.readthedocs.io/en/latest/configuration-directives/WSGIScriptAlias.html) + if grep -qP '^WSGIScriptAlias / /var/www/journalist.wsgi$' "$journalist_conf"; then + perl -pi -e 's/^(WSGIScriptAlias .*)/$1 process-group=journalist application-group=journalist/' "$journalist_conf" + fi + + # Remove the WSGIProcessGroup directive; it's not needed if + # specified in WSGIScriptAlias. + if grep -qP '^WSGIProcessGroup journalist' "$journalist_conf"; then + perl -pi -e 's/^WSGIProcessGroup journalist.*\n//' "$journalist_conf" + fi fi } @@ -158,7 +173,7 @@ case "$1" in aa-enforce /etc/apparmor.d/usr.sbin.apache2 # Munge Apache config while service is stopped. - permit_wsgi_authorization + adjust_wsgi_configuration # Restart apache so it loads with the apparmor profiles in enforce mode. service apache2 restart From 07e47087aa82a47d0aadba6bb2d5398c9353206f Mon Sep 17 00:00:00 2001 From: John Hensley Date: Tue, 7 Apr 2020 19:00:31 -0400 Subject: [PATCH 8/9] Revert CryptoUtil.get_fingerprint UID matching --- securedrop/crypto_util.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/securedrop/crypto_util.py b/securedrop/crypto_util.py index d67153c724..65d03c81f8 100644 --- a/securedrop/crypto_util.py +++ b/securedrop/crypto_util.py @@ -243,11 +243,11 @@ def get_fingerprint(self, name): if fingerprint: return fingerprint - target_uid = 'Autogenerated Key <{}>'.format(name) for key in self.gpg.list_keys(): - if target_uid in key['uids']: - self.redis.hset(self.REDIS_SOURCE_FINGERPRINT_HASH, name, key['fingerprint']) - return key['fingerprint'] + for uid in key['uids']: + if name in uid: + self.redis.hset(self.REDIS_SOURCE_FINGERPRINT_HASH, name, key['fingerprint']) + return key['fingerprint'] return None From 5aca09278a3cfe60ea7865c1d61d1c912aeeef94 Mon Sep 17 00:00:00 2001 From: John Hensley Date: Wed, 8 Apr 2020 09:56:53 -0400 Subject: [PATCH 9/9] Rename CryptoUtil Redis keys. --- securedrop/crypto_util.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/securedrop/crypto_util.py b/securedrop/crypto_util.py index 65d03c81f8..392e22f272 100644 --- a/securedrop/crypto_util.py +++ b/securedrop/crypto_util.py @@ -73,8 +73,8 @@ class CryptoUtil: # to set an expiration date. DEFAULT_KEY_EXPIRATION_DATE = '0' - REDIS_SOURCE_FINGERPRINT_HASH = "sd/crypto-util/source-fingerprints" - REDIS_SOURCE_KEY_HASH = "sd/crypto-util/source-keys" + REDIS_FINGERPRINT_HASH = "sd/crypto-util/fingerprints" + REDIS_KEY_HASH = "sd/crypto-util/keys" def __init__(self, scrypt_params, @@ -230,8 +230,8 @@ def delete_reply_keypair(self, source_filesystem_id): temp_gpg = gnupg.GPG(binary='gpg2', homedir=self.gpg_key_dir) # The subkeys keyword argument deletes both secret and public keys. temp_gpg.delete_keys(key, secret=True, subkeys=True) - self.redis.hdel(self.REDIS_SOURCE_KEY_HASH, self.get_fingerprint(source_filesystem_id)) - self.redis.hdel(self.REDIS_SOURCE_FINGERPRINT_HASH, source_filesystem_id) + self.redis.hdel(self.REDIS_KEY_HASH, self.get_fingerprint(source_filesystem_id)) + self.redis.hdel(self.REDIS_FINGERPRINT_HASH, source_filesystem_id) def get_fingerprint(self, name): """ @@ -239,14 +239,14 @@ def get_fingerprint(self, name): The supplied name is usually a source filesystem ID. """ - fingerprint = self.redis.hget(self.REDIS_SOURCE_FINGERPRINT_HASH, name) + fingerprint = self.redis.hget(self.REDIS_FINGERPRINT_HASH, name) if fingerprint: return fingerprint for key in self.gpg.list_keys(): for uid in key['uids']: if name in uid: - self.redis.hset(self.REDIS_SOURCE_FINGERPRINT_HASH, name, key['fingerprint']) + self.redis.hset(self.REDIS_FINGERPRINT_HASH, name, key['fingerprint']) return key['fingerprint'] return None @@ -261,12 +261,12 @@ def get_pubkey(self, name): if not fingerprint: return None - key = self.redis.hget(self.REDIS_SOURCE_KEY_HASH, fingerprint) + key = self.redis.hget(self.REDIS_KEY_HASH, fingerprint) if key: return key key = self.gpg.export_keys(fingerprint) - self.redis.hset(self.REDIS_SOURCE_KEY_HASH, fingerprint, key) + self.redis.hset(self.REDIS_KEY_HASH, fingerprint, key) return key def encrypt(self, plaintext, fingerprints, output=None):