Improve efficiency of source key management

securedrop_client/api_jobs/sync.py

@@ -5,7 +5,7 @@
 from sqlalchemy.orm.session import Session
 
 from securedrop_client.api_jobs.base import ApiJob
-from securedrop_client.crypto import GpgHelper, CryptoError
+from securedrop_client.crypto import GpgHelper
 from securedrop_client.storage import get_remote_data, update_local_storage
 
 
@@ -40,28 +40,8 @@ def call_api(self, api_client: API, session: Session) -> Any:
         remote_sources, remote_submissions, remote_replies = get_remote_data(api_client)
 
         update_local_storage(session,
+                             self.gpg,
                              remote_sources,
                              remote_submissions,
                              remote_replies,
                              self.data_dir)
-
-        fingerprints = self.gpg.fingerprints()
-        for source in remote_sources:
-            if source.key and source.key.get('type', None) == 'PGP':
-                pub_key = source.key.get('public', None)
-                fingerprint = source.key.get('fingerprint', None)
-                if not pub_key or not fingerprint:
-                    # The below line needs to be excluded from the coverage computation
-                    # as it will show as uncovered due to a cpython compiler optimziation.
-                    # See: https://bugs.python.org/issue2506
-                    continue  # pragma: no cover
-
-                if fingerprint in fingerprints:
-                    logger.debug("Skipping import of key with fingerprint {}".format(fingerprint))
-                    continue
-
-                try:
-                    logger.debug("Importing key with fingerprint {}".format(fingerprint))
-                    self.gpg.import_key(source.uuid, pub_key, fingerprint)
-                except CryptoError:
-                    logger.warning('Failed to import key for source {}'.format(source.uuid))

securedrop_client/crypto.py

@@ -22,7 +22,6 @@
 import struct
 import subprocess
 import tempfile
-import typing
 
 from sqlalchemy.orm import scoped_session
 from uuid import UUID
@@ -146,35 +145,15 @@ def _gpg_cmd_base(self) -> list:
         cmd.extend(['--trust-model', 'always'])
         return cmd
 
-    def fingerprints(self) -> typing.Dict[str, bool]:
-        """
-        Returns a map of key fingerprints.
-
-        The result is a map wherein each key is the fingerprint of a
-        key on our keyring, mapped to True. It's intended to help us
-        avoid expensive import operations for keys we already have.
-        """
-        cmd = self._gpg_cmd_base()
-        cmd.extend(["--list-public-keys", "--fingerprint", "--with-colons",
-                    "--fixed-list-mode", "--list-options", "no-show-photos"])
-        output = subprocess.check_output(cmd, universal_newlines=True)
-
-        fingerprints = {}
-        for line in output.splitlines():
-            if line.startswith("fpr:"):
-                fields = line.split(":")
-                fingerprint = fields[9]
-                fingerprints[fingerprint] = True
-
-        return fingerprints
-
     def import_key(self, source_uuid: UUID, key_data: str, fingerprint: str) -> None:
         session = self.session_maker()
         local_source = session.query(Source).filter_by(uuid=source_uuid).one()
 
+        logger.debug("Importing key with fingerprint %s", fingerprint)
         self._import(key_data)
 
         local_source.fingerprint = fingerprint
+        local_source.public_key = key_data
         session.add(local_source)
         session.commit()
 

securedrop_client/storage.py

@@ -30,6 +30,7 @@
 from sqlalchemy.orm.exc import NoResultFound
 from sqlalchemy.orm.session import Session
 
+from securedrop_client.crypto import CryptoError, GpgHelper
 from securedrop_client.db import (DraftReply, Source, Message, File, Reply, ReplySendStatus,
                                   ReplySendStatusCodes, User)
 from sdclientapi import API
@@ -100,6 +101,7 @@ def get_remote_data(api: API) -> Tuple[List[SDKSource], List[SDKSubmission], Lis
 
 
 def update_local_storage(session: Session,
+                         gpg: GpgHelper,
                          remote_sources: List[SDKSource],
                          remote_submissions: List[SDKSubmission],
                          remote_replies: List[SDKReply],
@@ -116,13 +118,43 @@ def update_local_storage(session: Session,
     # The following update_* functions may change the database state.
     # Because of that, each get_local_* function needs to be called just before
     # its respective update_* function.
-    update_sources(remote_sources, get_local_sources(session), session, data_dir)
+    update_sources(gpg, remote_sources, get_local_sources(session), session, data_dir)
     update_files(remote_files, get_local_files(session), session, data_dir)
     update_messages(remote_messages, get_local_messages(session), session, data_dir)
     update_replies(remote_replies, get_local_replies(session), session, data_dir)
 
 
-def update_sources(remote_sources: List[SDKSource],
+def update_source_key(gpg: GpgHelper, local_source: Source, remote_source: SDKSource) -> None:
+    """
+    Updates a source's GPG key.
+    """
+    if not remote_source.key.get("fingerprint"):
+        logger.error("New source data lacks key fingerprint")
+        return
+
+    if not remote_source.key.get("public"):
+        logger.error("New source data lacks public key")
+        return
+
+    if (
+        local_source.fingerprint == remote_source.key['fingerprint'] and
+        local_source.public_key == remote_source.key['public']
+    ):
+        logger.debug("Source key data is unchanged")
+        return
+
+    try:
+        # import_key updates the source's key and fingerprint, and commits
+        gpg.import_key(
+            remote_source.uuid,
+            remote_source.key['public'],
+            remote_source.key['fingerprint']
+        )
+    except CryptoError:
+        logger.error('Failed to update key information for source %s', remote_source.uuid)
+
+
+def update_sources(gpg: GpgHelper, remote_sources: List[SDKSource],
                    local_sources: List[Source], session: Session, data_dir: str) -> None:
     """
     Given collections of remote sources, the current local sources and a
@@ -134,40 +166,45 @@ def update_sources(remote_sources: List[SDKSource],
     * Local items not returned in the remote sources are deleted from the
       local database.
     """
-    local_uuids = {source.uuid for source in local_sources}
+    local_sources_by_uuid = {s.uuid: s for s in local_sources}
     for source in remote_sources:
-        if source.uuid in local_uuids:
+        if source.uuid in local_sources_by_uuid:
             # Update an existing record.
-            local_source = [s for s in local_sources
-                            if s.uuid == source.uuid][0]
+            local_source = local_sources_by_uuid[source.uuid]
             local_source.journalist_designation = source.journalist_designation
             local_source.is_flagged = source.is_flagged
-            local_source.public_key = source.key['public']
             local_source.interaction_count = source.interaction_count
             local_source.document_count = source.number_of_documents
             local_source.is_starred = source.is_starred
             local_source.last_updated = parse(source.last_updated)
+            session.commit()
 
-            # Removing the UUID from local_uuids ensures this record won't be
-            # deleted at the end of this function.
-            local_uuids.remove(source.uuid)
+            update_source_key(gpg, local_source, source)
+
+            # Removing the UUID from local_sources_by_uuid ensures
+            # this record won't be deleted at the end of this
+            # function.
+            del local_sources_by_uuid[source.uuid]
             logger.debug('Updated source {}'.format(source.uuid))
         else:
             # A new source to be added to the database.
             ns = Source(uuid=source.uuid,
                         journalist_designation=source.journalist_designation,
                         is_flagged=source.is_flagged,
-                        public_key=source.key['public'],
                         interaction_count=source.interaction_count,
                         is_starred=source.is_starred,
                         last_updated=parse(source.last_updated),
                         document_count=source.number_of_documents)
             session.add(ns)
+            session.commit()
+
+            update_source_key(gpg, ns, source)
+
             logger.debug('Added new source {}'.format(source.uuid))
 
     # The uuids remaining in local_uuids do not exist on the remote server, so
     # delete the related records.
-    for deleted_source in [s for s in local_sources if s.uuid in local_uuids]:
+    for deleted_source in local_sources_by_uuid.values():
         for document in deleted_source.collection:
             if isinstance(document, (Message, File, Reply)):
                 delete_single_submission_or_reply_on_disk(document, data_dir)

tests/api_jobs/test_sync.py

@@ -1,9 +1,11 @@
 import os
-from uuid import UUID
 
 from securedrop_client.api_jobs.sync import MetadataSyncJob
 from securedrop_client.crypto import GpgHelper, CryptoError
 
+from tests import factory
+
+
 with open(os.path.join(os.path.dirname(__file__), '..', 'files', 'test-key.gpg.pub.asc')) as f:
     PUB_KEY = f.read()
 
@@ -12,27 +14,23 @@ def test_MetadataSyncJob_success(mocker, homedir, session, session_maker):
     gpg = GpgHelper(homedir, session_maker, is_qubes=False)
     job = MetadataSyncJob(homedir, gpg)
 
-    mock_source = mocker.MagicMock()
-    mock_source.uuid = 'bar'
-    mock_source.key = {
-        'type': 'PGP',
-        'public': PUB_KEY,
-        'fingerprint': '123456ABC',
-    }
+    mock_source = factory.RemoteSource(
+        key={
+            'type': 'PGP',
+            'public': PUB_KEY,
+            'fingerprint': '123456ABC',
+        }
+    )
 
     mock_key_import = mocker.patch.object(job.gpg, 'import_key')
     mock_get_remote_data = mocker.patch(
         'securedrop_client.api_jobs.sync.get_remote_data',
-        return_value=([mock_source], 'submissions', 'replies'))
+        return_value=([mock_source], [], []))
 
     api_client = mocker.MagicMock()
     api_client.default_request_timeout = mocker.MagicMock()
     api_client.default_request_timeout = mocker.MagicMock()
 
-    mocker.patch(
-        'securedrop_client.api_jobs.sync.update_local_storage',
-        return_value=([mock_source], 'submissions', 'replies'))
-
     job.call_api(api_client, session)
 
     assert mock_key_import.call_args[0][0] == mock_source.uuid
@@ -48,27 +46,23 @@ def test_MetadataSyncJob_success_with_key_import_fail(mocker, homedir, session,
     gpg = GpgHelper(homedir, session_maker, is_qubes=False)
     job = MetadataSyncJob(homedir, gpg)
 
-    mock_source = mocker.MagicMock()
-    mock_source.uuid = 'bar'
-    mock_source.key = {
-        'type': 'PGP',
-        'public': PUB_KEY,
-        'fingerprint': '123456ABC',
-    }
+    mock_source = factory.RemoteSource(
+        key={
+            'type': 'PGP',
+            'public': PUB_KEY,
+            'fingerprint': '123456ABC',
+        }
+    )
 
     mock_key_import = mocker.patch.object(job.gpg, 'import_key',
                                           side_effect=CryptoError)
     mock_get_remote_data = mocker.patch(
         'securedrop_client.api_jobs.sync.get_remote_data',
-        return_value=([mock_source], 'submissions', 'replies'))
+        return_value=([mock_source], [], []))
 
     api_client = mocker.MagicMock()
     api_client.default_request_timeout = mocker.MagicMock()
 
-    mocker.patch(
-        'securedrop_client.api_jobs.sync.update_local_storage',
-        return_value=([mock_source], 'submissions', 'replies'))
-
     job.call_api(api_client, session)
 
     assert mock_key_import.call_args[0][0] == mock_source.uuid
@@ -84,26 +78,22 @@ def test_MetadataSyncJob_success_with_missing_key(mocker, homedir, session, sess
     gpg = GpgHelper(homedir, session_maker, is_qubes=False)
     job = MetadataSyncJob(homedir, gpg)
 
-    mock_source = mocker.MagicMock()
-    mock_source.uuid = 'bar'
-    mock_source.key = {
-        'type': 'PGP',
-        'pub_key': '',
-        'fingerprint': ''
-    }
+    mock_source = factory.RemoteSource(
+        key={
+            'type': 'PGP',
+            'public': '',
+            'fingerprint': '',
+        }
+    )
 
     mock_key_import = mocker.patch.object(job.gpg, 'import_key')
     mock_get_remote_data = mocker.patch(
         'securedrop_client.api_jobs.sync.get_remote_data',
-        return_value=([mock_source], 'submissions', 'replies'))
+        return_value=([mock_source], [], []))
 
     api_client = mocker.MagicMock()
     api_client.default_request_timeout = mocker.MagicMock()
 
-    mocker.patch(
-        'securedrop_client.api_jobs.sync.update_local_storage',
-        return_value=([mock_source], 'submissions', 'replies'))
-
     job.call_api(api_client, session)
 
     assert mock_key_import.call_count == 0
@@ -114,20 +104,16 @@ def test_MetadataSyncJob_only_import_new_source_keys(mocker, homedir, session, s
     """
     Verify that we only import source keys we don't already have.
     """
-    class LimitedImportGpgHelper(GpgHelper):
-        def import_key(self, source_uuid: UUID, key_data: str, fingerprint: str) -> None:
-            self._import(key_data)
-
-    gpg = LimitedImportGpgHelper(homedir, session_maker, is_qubes=False)
+    gpg = GpgHelper(homedir, session_maker, is_qubes=False)
     job = MetadataSyncJob(homedir, gpg)
 
-    mock_source = mocker.MagicMock()
-    mock_source.uuid = 'bar'
-    mock_source.key = {
-        'type': 'PGP',
-        'public': PUB_KEY,
-        'fingerprint': 'B2FF7FB28EED8CABEBC5FB6C6179D97BCFA52E5F',
-    }
+    mock_source = factory.RemoteSource(
+        key={
+            'type': 'PGP',
+            'public': PUB_KEY,
+            'fingerprint': '123456ABC',
+        }
+    )
 
     mock_get_remote_data = mocker.patch(
         'securedrop_client.api_jobs.sync.get_remote_data',
@@ -136,27 +122,19 @@ def import_key(self, source_uuid: UUID, key_data: str, fingerprint: str) -> None
     api_client = mocker.MagicMock()
     api_client.default_request_timeout = mocker.MagicMock()
 
-    mocker.patch(
-        'securedrop_client.api_jobs.sync.update_local_storage',
-        return_value=([mock_source], [], []))
-
-    mock_logger = mocker.patch('securedrop_client.api_jobs.sync.logger')
+    crypto_logger = mocker.patch('securedrop_client.crypto.logger')
+    storage_logger = mocker.patch('securedrop_client.storage.logger')
 
     job.call_api(api_client, session)
 
     assert mock_get_remote_data.call_count == 1
-    assert len(gpg.fingerprints()) == 2
 
-    log_msg = mock_logger.debug.call_args_list[0][0][0]
-    assert log_msg.startswith(
-        'Importing key with fingerprint {}'.format(mock_source.key['fingerprint'])
-    )
+    log_msg = crypto_logger.debug.call_args_list[0][0]
+    assert log_msg == ('Importing key with fingerprint %s', mock_source.key['fingerprint'])
 
     job.call_api(api_client, session)
 
     assert mock_get_remote_data.call_count == 2
 
-    log_msg = mock_logger.debug.call_args_list[1][0][0]
-    assert log_msg.startswith(
-        'Skipping import of key with fingerprint {}'.format(mock_source.key['fingerprint'])
-    )
+    log_msg = storage_logger.debug.call_args_list[1][0][0]
+    assert log_msg == 'Source key data is unchanged'