[feat] self-repair the keyring if keys get duplicated

In some cases in the past keys got stored twice in different documents.
Hopefully this issue is solved now, this tries to self-repair the keyring
if encounters that. This is not really solving the problem, if it keeps
happening we need to investigate the source.

- Resolves: #7498

4 files changed, 204 insertions, 51 deletions

diff --git a/keymanager/changes/bug-7498_multiple_keys b/keymanager/changes/bug-7498_multiple_keys
new file mode 100644
index 0000000..90cf675
--- /dev/null
+++ b/keymanager/changes/bug-7498_multiple_keys
@@ -0,0 +1 @@
+- self-repair the keyring if keys get duplicated (Closes: #7485)
diff --git a/keymanager/src/leap/keymanager/openpgp.py b/keymanager/src/leap/keymanager/openpgp.py
index 069a78e..d648137 100644
--- a/keymanager/src/leap/keymanager/openpgp.py
+++ b/keymanager/src/leap/keymanager/openpgp.py
@@ -22,6 +22,7 @@ import os
 import re
 import shutil
 import tempfile
+import traceback
 import io
 
 
@@ -41,6 +42,8 @@ from leap.keymanager.keys import (
     TYPE_ADDRESS_PRIVATE_INDEX,
     KEY_ADDRESS_KEY,
     KEY_ID_KEY,
+    KEY_FINGERPRINT_KEY,
+    KEY_REFRESHED_AT_KEY,
     KEYMANAGER_ACTIVE_TYPE,
 )
 
@@ -422,41 +425,42 @@ class OpenPGPScheme(EncryptionScheme):
         :rtype: Deferred
         """
         def check_and_put(docs, key):
-            if len(docs) == 1:
-                doc = docs.pop()
-                oldkey = build_key_from_dict(OpenPGPKey, doc.content)
-                if key.fingerprint == oldkey.fingerprint:
-                    # in case of an update of the key merge them with gnupg
-                    with TempGPGWrapper(gpgbinary=self._gpgbinary) as gpg:
-                        gpg.import_keys(oldkey.key_data)
-                        gpg.import_keys(key.key_data)
-                        gpgkey = gpg.list_keys(secret=key.private).pop()
-                        mergedkey = self._build_key_from_gpg(
-                            gpgkey,
-                            gpg.export_keys(gpgkey['fingerprint'],
-                                            secret=key.private))
-                    mergedkey.validation = max(
-                        [key.validation, oldkey.validation])
-                    mergedkey.last_audited_at = oldkey.last_audited_at
-                    mergedkey.refreshed_at = key.refreshed_at
-                    mergedkey.encr_used = key.encr_used or oldkey.encr_used
-                    mergedkey.sign_used = key.sign_used or oldkey.sign_used
-                    doc.set_json(mergedkey.get_json())
-                    d = self._soledad.put_doc(doc)
-                else:
-                    logger.critical(
-                        "Can't put a key whith the same key_id and different "
-                        "fingerprint: %s, %s"
-                        % (key.fingerprint, oldkey.fingerprint))
-                    d = defer.fail(
-                        errors.KeyFingerprintMismatch(key.fingerprint))
+            deferred_repair = defer.succeed(None)
+            if len(docs) == 0:
+                return self._soledad.create_doc_from_json(key.get_json())
             elif len(docs) > 1:
+                deferred_repair = self._repair_key_docs(docs, key.key_id)
+
+            doc = docs[0]
+            oldkey = build_key_from_dict(OpenPGPKey, doc.content)
+            if key.fingerprint != oldkey.fingerprint:
                 logger.critical(
-                    "There is more than one key with the same key_id %s"
-                    % (key.key_id,))
-                d = defer.fail(errors.KeyAttributesDiffer(key.key_id))
-            else:
-                d = self._soledad.create_doc_from_json(key.get_json())
+                    "Can't put a key whith the same key_id and different "
+                    "fingerprint: %s, %s"
+                    % (key.fingerprint, oldkey.fingerprint))
+                return defer.fail(
+                    errors.KeyFingerprintMismatch(key.fingerprint))
+
+            # in case of an update of the key merge them with gnupg
+            with TempGPGWrapper(gpgbinary=self._gpgbinary) as gpg:
+                gpg.import_keys(oldkey.key_data)
+                gpg.import_keys(key.key_data)
+                gpgkey = gpg.list_keys(secret=key.private).pop()
+                mergedkey = self._build_key_from_gpg(
+                    gpgkey,
+                    gpg.export_keys(gpgkey['fingerprint'],
+                                    secret=key.private))
+            mergedkey.validation = max(
+                [key.validation, oldkey.validation])
+            mergedkey.last_audited_at = oldkey.last_audited_at
+            mergedkey.refreshed_at = key.refreshed_at
+            mergedkey.encr_used = key.encr_used or oldkey.encr_used
+            mergedkey.sign_used = key.sign_used or oldkey.sign_used
+            doc.set_json(mergedkey.get_json())
+            deferred_put = self._soledad.put_doc(doc)
+
+            d = defer.gatherResults([deferred_put, deferred_repair])
+            d.addCallback(lambda res: res[0])
             return d
 
         d = self._soledad.get_from_index(
@@ -533,14 +537,21 @@ class OpenPGPScheme(EncryptionScheme):
                 self.KEY_TYPE,
                 key_id,
                 '1' if private else '0')
-            d.addCallback(get_doc, key_id)
+            d.addCallback(get_doc, key_id, activedoc)
             return d
 
-        def get_doc(doclist, key_id):
-            leap_assert(
-                len(doclist) is 1,
-                'There is %d keys for id %s!' % (len(doclist), key_id))
-            return doclist.pop()
+        def get_doc(doclist, key_id, activedoc):
+            if len(doclist) == 0:
+                logger.warning('There is no key for id %s! Self-repairing it.'
+                               % (key_id))
+                d = self._soledad.delete_doc(activedoc)
+                d.addCallback(lambda _: None)
+                return d
+            elif len(doclist) > 1:
+                d = self._repair_key_docs(doclist, key_id)
+                d.addCallback(lambda _: doclist[0])
+                return d
+            return doclist[0]
 
         d = self._soledad.get_from_index(
             TYPE_ADDRESS_PRIVATE_INDEX,
@@ -598,18 +609,20 @@ class OpenPGPScheme(EncryptionScheme):
         def delete_key(docs):
             if len(docs) == 0:
                 raise errors.KeyNotFound(key)
-            if len(docs) > 1:
-                logger.critical("There is more than one key for key_id %s"
-                                % key.key_id)
-
-            doc = None
-            for d in docs:
-                if d.content['fingerprint'] == key.fingerprint:
-                    doc = d
-                    break
-            if doc is None:
+            elif len(docs) > 1:
+                logger.warning("There is more than one key for key_id %s"
+                               % key.key_id)
+
+            has_deleted = False
+            deferreds = []
+            for doc in docs:
+                if doc.content['fingerprint'] == key.fingerprint:
+                    d = self._soledad.delete_doc(doc)
+                    deferreds.append(d)
+                    has_deleted = True
+            if not has_deleted:
                 raise errors.KeyNotFound(key)
-            return self._soledad.delete_doc(doc)
+            return defer.gatherResults(deferreds)
 
         d = self._soledad.get_from_index(
             TYPE_ID_PRIVATE_INDEX,
@@ -621,6 +634,36 @@ class OpenPGPScheme(EncryptionScheme):
         d.addCallback(delete_key)
         return d
 
+    def _repair_key_docs(self, doclist, key_id):
+        """
+        If there is more than one key for a key id try to self-repair it
+
+        :return: a Deferred that will be fired once all the deletions are
+                 completed
+        :rtype: Deferred
+        """
+        logger.error("BUG ---------------------------------------------------")
+        logger.error("There is more than one key with the same key_id %s:"
+                     % (key_id,))
+
+        def log_key_doc(doc):
+            logger.error("\t%s: %s" % (doc.content[KEY_ADDRESS_KEY],
+                                       doc.content[KEY_FINGERPRINT_KEY]))
+
+        doclist.sort(key=lambda doc: doc.content[KEY_REFRESHED_AT_KEY],
+                     reverse=True)
+        log_key_doc(doclist[0])
+        deferreds = []
+        for doc in doclist[1:]:
+            log_key_doc(doc)
+            d = self._soledad.delete_doc(doc)
+            deferreds.append(d)
+
+        logger.error("")
+        logger.error(traceback.extract_stack())
+        logger.error("BUG (please report above info) ------------------------")
+        return defer.gatherResults(deferreds, consumeErrors=True)
+
     #
     # Data encryption, decryption, signing and verifying
     #
diff --git a/keymanager/src/leap/keymanager/tests/__init__.py b/keymanager/src/leap/keymanager/tests/__init__.py
index 9b95e1a..cd612c4 100644
--- a/keymanager/src/leap/keymanager/tests/__init__.py
+++ b/keymanager/src/leap/keymanager/tests/__init__.py
@@ -66,9 +66,15 @@ class KeyManagerWithSoledadTestCase(unittest.TestCase, BaseLeapTest):
             for private in [True, False]:
                 d = km.get_all_keys(private=private)
                 d.addCallback(delete_keys)
+                d.addCallback(check_deleted, private)
                 deferreds.append(d)
             return gatherResults(deferreds)
 
+        def check_deleted(_, private):
+            d = km.get_all_keys(private=private)
+            d.addCallback(lambda keys: self.assertEqual(keys, []))
+            return d
+
         # wait for the indexes to be ready for the tear down
         d = km._wrapper_map[OpenPGPKey].deferred_indexes
         d.addCallback(get_and_delete_keys)
@@ -91,6 +97,7 @@ class KeyManagerWithSoledadTestCase(unittest.TestCase, BaseLeapTest):
 
 
 # key 24D18DDF: public key "Leap Test Key <leap@leap.se>"
+KEY_ID = "2F455E2824D18DDF"
 KEY_FINGERPRINT = "E36E738D69173C13D709E44F2F455E2824D18DDF"
 PUBLIC_KEY = """
 -----BEGIN PGP PUBLIC KEY BLOCK-----
diff --git a/keymanager/src/leap/keymanager/tests/test_openpgp.py b/keymanager/src/leap/keymanager/tests/test_openpgp.py
index 5f85c74..bae83db 100644
--- a/keymanager/src/leap/keymanager/tests/test_openpgp.py
+++ b/keymanager/src/leap/keymanager/tests/test_openpgp.py
@@ -21,12 +21,15 @@ Tests for the OpenPGP support on Key Manager.
 """
 
 
-from twisted.internet.defer import inlineCallbacks
+from datetime import datetime
+from mock import Mock
+from twisted.internet.defer import inlineCallbacks, gatherResults, succeed
 
 from leap.keymanager import (
     KeyNotFound,
     openpgp,
 )
+from leap.keymanager.keys import TYPE_ID_PRIVATE_INDEX
 from leap.keymanager.openpgp import OpenPGPKey
 from leap.keymanager.tests import (
     KeyManagerWithSoledadTestCase,
@@ -34,6 +37,7 @@ from leap.keymanager.tests import (
     ADDRESS_2,
     KEY_FINGERPRINT,
     PUBLIC_KEY,
+    KEY_ID,
     PUBLIC_KEY_2,
     PRIVATE_KEY,
     PRIVATE_KEY_2,
@@ -247,6 +251,104 @@ class OpenPGPCryptoTestCase(KeyManagerWithSoledadTestCase):
         validsign = pgp.verify(data, pubkey, detached_sig=signature)
         self.assertTrue(validsign)
 
+    @inlineCallbacks
+    def test_self_repair_three_keys(self):
+        pgp = openpgp.OpenPGPScheme(
+            self._soledad, gpgbinary=self.gpg_binary_path)
+        yield pgp.put_ascii_key(PUBLIC_KEY, ADDRESS)
+
+        get_from_index = self._soledad.get_from_index
+        delete_doc = self._soledad.delete_doc
+
+        def my_get_from_index(*args):
+            if (args[0] == TYPE_ID_PRIVATE_INDEX and
+                    args[2] == KEY_ID):
+                k1 = OpenPGPKey(ADDRESS, key_id="1",
+                                refreshed_at=datetime(2005, 1, 1))
+                k2 = OpenPGPKey(ADDRESS, key_id="2",
+                                refreshed_at=datetime(2007, 1, 1))
+                k3 = OpenPGPKey(ADDRESS, key_id="3",
+                                refreshed_at=datetime(2001, 1, 1))
+                d1 = self._soledad.create_doc_from_json(k1.get_json())
+                d2 = self._soledad.create_doc_from_json(k2.get_json())
+                d3 = self._soledad.create_doc_from_json(k3.get_json())
+                return gatherResults([d1, d2, d3])
+            return get_from_index(*args)
+
+        self._soledad.get_from_index = my_get_from_index
+        self._soledad.delete_doc = Mock(return_value=succeed(None))
+
+        key = yield pgp.get_key(ADDRESS, private=False)
+
+        try:
+            self.assertEqual(key.key_id, "2")
+            self.assertEqual(self._soledad.delete_doc.call_count, 2)
+        finally:
+            self._soledad.get_from_index = get_from_index
+            self._soledad.delete_doc = delete_doc
+
+    @inlineCallbacks
+    def test_self_repair_no_keys(self):
+        pgp = openpgp.OpenPGPScheme(
+            self._soledad, gpgbinary=self.gpg_binary_path)
+        yield pgp.put_ascii_key(PUBLIC_KEY, ADDRESS)
+
+        get_from_index = self._soledad.get_from_index
+        delete_doc = self._soledad.delete_doc
+
+        def my_get_from_index(*args):
+            if (args[0] == TYPE_ID_PRIVATE_INDEX and
+                    args[2] == KEY_ID):
+                return succeed([])
+            return get_from_index(*args)
+
+        self._soledad.get_from_index = my_get_from_index
+        self._soledad.delete_doc = Mock(return_value=succeed(None))
+
+        try:
+            yield self.assertFailure(pgp.get_key(ADDRESS, private=False),
+                                     KeyNotFound)
+            self.assertEqual(self._soledad.delete_doc.call_count, 1)
+        finally:
+            self._soledad.get_from_index = get_from_index
+            self._soledad.delete_doc = delete_doc
+
+    @inlineCallbacks
+    def test_self_repair_put_keys(self):
+        pgp = openpgp.OpenPGPScheme(
+            self._soledad, gpgbinary=self.gpg_binary_path)
+
+        get_from_index = self._soledad.get_from_index
+        delete_doc = self._soledad.delete_doc
+
+        def my_get_from_index(*args):
+            if (args[0] == TYPE_ID_PRIVATE_INDEX and
+                    args[2] == KEY_ID):
+                k1 = OpenPGPKey(ADDRESS, key_id="1",
+                                fingerprint=KEY_FINGERPRINT,
+                                refreshed_at=datetime(2005, 1, 1))
+                k2 = OpenPGPKey(ADDRESS, key_id="2",
+                                fingerprint=KEY_FINGERPRINT,
+                                refreshed_at=datetime(2007, 1, 1))
+                k3 = OpenPGPKey(ADDRESS, key_id="3",
+                                fingerprint=KEY_FINGERPRINT,
+                                refreshed_at=datetime(2001, 1, 1))
+                d1 = self._soledad.create_doc_from_json(k1.get_json())
+                d2 = self._soledad.create_doc_from_json(k2.get_json())
+                d3 = self._soledad.create_doc_from_json(k3.get_json())
+                return gatherResults([d1, d2, d3])
+            return get_from_index(*args)
+
+        self._soledad.get_from_index = my_get_from_index
+        self._soledad.delete_doc = Mock(return_value=succeed(None))
+
+        try:
+            yield pgp.put_ascii_key(PUBLIC_KEY, ADDRESS)
+            self.assertEqual(self._soledad.delete_doc.call_count, 2)
+        finally:
+            self._soledad.get_from_index = get_from_index
+            self._soledad.delete_doc = delete_doc
+
     def _assert_key_not_found(self, pgp, address, private=False):
         d = pgp.get_key(address, private=private)
         return self.assertFailure(d, KeyNotFound)