launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:archive-gpg-signing-key-fix-non-default into launchpad:master.

Commit message:
Fix crash when generating non-default PPA keys using signing service

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/391427

When generating a key for a user's non-default (i.e. non-first) PPA, we first ensure that there's a key for the same user's default PPA and then propagate it.  However, this mechanism was still using interfaces built for local GPG key generation, and failed when using the signing service.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:archive-gpg-signing-key-fix-non-default into launchpad:master.

diff --git a/lib/lp/archivepublisher/archivegpgsigningkey.py b/lib/lp/archivepublisher/archivegpgsigningkey.py
index 20a4d79..bdb2d16 100644
--- a/lib/lp/archivepublisher/archivegpgsigningkey.py
+++ b/lib/lp/archivepublisher/archivegpgsigningkey.py
@@ -14,6 +14,7 @@ __all__ = [
 import os
 
 import gpgme
+from twisted.internet import defer
 from twisted.internet.threads import deferToThread
 from zope.component import getUtility
 from zope.interface import implementer
@@ -244,23 +245,25 @@ class ArchiveGPGSigningKey(SignableArchive):
         with open(export_path, 'wb') as export_file:
             export_file.write(key.export())
 
+    @defer.inlineCallbacks
     def generateSigningKey(self, log=None, async_keyserver=False):
         """See `IArchiveGPGSigningKey`."""
         assert self.archive.signing_key_fingerprint is None, (
             "Cannot override signing_keys.")
 
         # Always generate signing keys for the default PPA, even if it
-        # was not expecifically requested. The default PPA signing key
+        # was not specifically requested. The default PPA signing key
         # is then propagated to the context named-ppa.
         default_ppa = self.archive.owner.archive
         if self.archive != default_ppa:
-            if default_ppa.signing_key is None:
-                IArchiveGPGSigningKey(default_ppa).generateSigningKey()
-            key = default_ppa.signing_key
-            self.archive.signing_key_owner = key.owner
-            self.archive.signing_key_fingerprint = key.fingerprint
+            if default_ppa.signing_key_fingerprint is None:
+                yield IArchiveGPGSigningKey(default_ppa).generateSigningKey(
+                    log=log, async_keyserver=async_keyserver)
+            self.archive.signing_key_owner = default_ppa.signing_key_owner
+            self.archive.signing_key_fingerprint = (
+                default_ppa.signing_key_fingerprint)
             del get_property_cache(self.archive).signing_key
-            return
+            defer.returnValue(None)
 
         key_displayname = (
             "Launchpad PPA for %s" % self.archive.owner.displayname)
@@ -278,7 +281,7 @@ class ArchiveGPGSigningKey(SignableArchive):
         else:
             signing_key = getUtility(IGPGHandler).generateKey(
                 key_displayname, logger=log)
-        return self._setupSigningKey(
+        yield self._setupSigningKey(
             signing_key, async_keyserver=async_keyserver)
 
     def setSigningKey(self, key_path, async_keyserver=False):
diff --git a/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py b/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
index 8b35b74..b521e85 100644
--- a/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
+++ b/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
@@ -12,7 +12,11 @@ from textwrap import dedent
 
 import six
 from testtools.matchers import (
+    Equals,
     FileContains,
+    Is,
+    MatchesStructure,
+    Not,
     StartsWith,
     )
 from testtools.twistedsupport import (
@@ -339,6 +343,35 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
         self.assertIn(b"-----BEGIN PGP PUBLIC KEY BLOCK-----\n", content)
 
     @defer.inlineCallbacks
+    def test_generateSigningKey_local_non_default_ppa(self):
+        # Generating a signing key locally using GPGHandler for a
+        # non-default PPA generates one for the user's default PPA first and
+        # then propagates it.
+        self.useFixture(FakeGenerateKey("ppa-sample@xxxxxxxxxxxxxxxxx"))
+        logger = BufferLogger()
+        # Use a display name that matches the pregenerated sample key.
+        owner = self.factory.makePerson(
+            displayname="Celso \xe1\xe9\xed\xf3\xfa Providelo")
+        default_ppa = self.factory.makeArchive(owner=owner)
+        another_ppa = self.factory.makeArchive(owner=owner)
+        yield IArchiveGPGSigningKey(another_ppa).generateSigningKey(
+            log=logger, async_keyserver=True)
+        self.assertThat(default_ppa, MatchesStructure(
+            signing_key=Not(Is(None)),
+            signing_key_owner=Not(Is(None)),
+            signing_key_fingerprint=Not(Is(None))))
+        self.assertIsNotNone(
+            getUtility(IGPGKeySet).getByFingerprint(
+                default_ppa.signing_key_fingerprint))
+        self.assertIsNone(
+            getUtility(ISigningKeySet).get(
+                SigningKeyType.OPENPGP, default_ppa.signing_key_fingerprint))
+        self.assertThat(another_ppa, MatchesStructure.byEquality(
+            signing_key=default_ppa.signing_key,
+            signing_key_owner=default_ppa.signing_key_owner,
+            signing_key_fingerprint=default_ppa.signing_key_fingerprint))
+
+    @defer.inlineCallbacks
     def test_generateSigningKey_signing_service(self):
         # Generating a signing key on the signing service stores it in the
         # database and pushes it to the keyserver.
@@ -374,3 +407,39 @@ class TestArchiveGPGSigningKey(TestCaseWithFactory):
         yield check_status(response)
         content = yield treq.content(response)
         self.assertIn(test_key, content)
+
+    @defer.inlineCallbacks
+    def test_generateSigningKey_signing_service_non_default_ppa(self):
+        # Generating a signing key on the signing service for a non-default
+        # PPA generates one for the user's default PPA first and then
+        # propagates it.
+        self.useFixture(
+            FeatureFixture({PUBLISHER_GPG_USES_SIGNING_SERVICE: "on"}))
+        signing_service_client = self.useFixture(
+            SigningServiceClientFixture(self.factory))
+        signing_service_client.generate.side_effect = None
+        test_key = test_pubkey_from_email("ftpmaster@xxxxxxxxxxxxx")
+        signing_service_client.generate.return_value = {
+            "fingerprint": "33C0A61893A5DC5EB325B29E415A12CAC2F30234",
+            "public-key": test_key,
+            }
+        logger = BufferLogger()
+        default_ppa = self.factory.makeArchive()
+        another_ppa = self.factory.makeArchive(owner=default_ppa.owner)
+        yield IArchiveGPGSigningKey(another_ppa).generateSigningKey(
+            log=logger, async_keyserver=True)
+        self.assertThat(default_ppa, MatchesStructure(
+            signing_key=Is(None),
+            signing_key_owner=Not(Is(None)),
+            signing_key_fingerprint=Not(Is(None))))
+        self.assertIsNone(
+            getUtility(IGPGKeySet).getByFingerprint(
+                default_ppa.signing_key_fingerprint))
+        signing_key = getUtility(ISigningKeySet).get(
+            SigningKeyType.OPENPGP, default_ppa.signing_key_fingerprint)
+        self.assertEqual(test_key, signing_key.public_key)
+        self.assertThat(another_ppa, MatchesStructure(
+            signing_key=Is(None),
+            signing_key_owner=Equals(default_ppa.signing_key_owner),
+            signing_key_fingerprint=Equals(
+                default_ppa.signing_key_fingerprint)))