Merge ~cjwatson/launchpad:archive-gpg-generate-using-signing-service into launchpad:master

[Colin Watson]

Generate archive signing keys using the signing service

This is guarded by the feature rule 'archivepublisher.gpg.signing_service.enabled'.

[Ioana Lasc (ilasc)]

LGTM

review: Approve

diff --git a/lib/lp/archivepublisher/archivegpgsigningkey.py b/lib/lp/archivepublisher/archivegpgsigningkey.py
index a342332..20a4d79 100644
--- a/lib/lp/archivepublisher/archivegpgsigningkey.py
+++ b/lib/lp/archivepublisher/archivegpgsigningkey.py
@@ -37,17 +37,24 @@ from lp.archivepublisher.run_parts import (
 from lp.registry.interfaces.gpg import IGPGKeySet
 from lp.services.config import config
 from lp.services.features import getFeatureFlag
-from lp.services.gpg.interfaces import IGPGHandler
+from lp.services.gpg.interfaces import (
+    IGPGHandler,
+    IPymeKey,
+    )
 from lp.services.osutils import remove_if_exists
 from lp.services.propertycache import (
     cachedproperty,
     get_property_cache,
     )
 from lp.services.signing.enums import (
+    OpenPGPKeyAlgorithm,
     SigningKeyType,
     SigningMode,
     )
-from lp.services.signing.interfaces.signingkey import ISigningKeySet
+from lp.services.signing.interfaces.signingkey import (
+    ISigningKey,
+    ISigningKeySet,
+    )
 
 
 @implementer(ISignableArchive)
@@ -72,7 +79,7 @@ class SignableArchive:
     def can_sign(self):
         """See `ISignableArchive`."""
         return (
-            self.archive.signing_key is not None or
+            self.archive.signing_key_fingerprint is not None or
             self._run_parts_dir is not None)
 
     @cachedproperty
@@ -237,9 +244,9 @@ class ArchiveGPGSigningKey(SignableArchive):
         with open(export_path, 'wb') as export_file:
             export_file.write(key.export())
 
-    def generateSigningKey(self, log=None):
+    def generateSigningKey(self, log=None, async_keyserver=False):
         """See `IArchiveGPGSigningKey`."""
-        assert self.archive.signing_key is None, (
+        assert self.archive.signing_key_fingerprint is None, (
             "Cannot override signing_keys.")
 
         # Always generate signing keys for the default PPA, even if it
@@ -257,13 +264,26 @@ class ArchiveGPGSigningKey(SignableArchive):
 
         key_displayname = (
             "Launchpad PPA for %s" % self.archive.owner.displayname)
-        secret_key = getUtility(IGPGHandler).generateKey(
-            key_displayname, logger=log)
-        self._setupSigningKey(secret_key)
+        if getFeatureFlag(PUBLISHER_GPG_USES_SIGNING_SERVICE):
+            try:
+                signing_key = getUtility(ISigningKeySet).generate(
+                    SigningKeyType.OPENPGP, key_displayname,
+                    openpgp_key_algorithm=OpenPGPKeyAlgorithm.RSA, length=4096)
+            except Exception as e:
+                if log is not None:
+                    log.exception(
+                        "Error generating signing key for %s: %s %s" %
+                        (self.archive.reference, e.__class__.__name__, e))
+                raise
+        else:
+            signing_key = getUtility(IGPGHandler).generateKey(
+                key_displayname, logger=log)
+        return self._setupSigningKey(
+            signing_key, async_keyserver=async_keyserver)
 
     def setSigningKey(self, key_path, async_keyserver=False):
         """See `IArchiveGPGSigningKey`."""
-        assert self.archive.signing_key is None, (
+        assert self.archive.signing_key_fingerprint is None, (
             "Cannot override signing_keys.")
         assert os.path.exists(key_path), (
             "%s does not exist" % key_path)
@@ -274,34 +294,46 @@ class ArchiveGPGSigningKey(SignableArchive):
         return self._setupSigningKey(
             secret_key, async_keyserver=async_keyserver)
 
-    def _uploadPublicSigningKey(self, secret_key):
+    def _uploadPublicSigningKey(self, signing_key):
         """Upload the public half of a signing key to the keyserver."""
         # The handler's security proxying doesn't protect anything useful
         # here, and when we're running in a thread we don't have an
         # interaction.
         gpghandler = removeSecurityProxy(getUtility(IGPGHandler))
-        pub_key = gpghandler.retrieveKey(secret_key.fingerprint)
-        gpghandler.uploadPublicKey(pub_key.fingerprint)
-        return pub_key
+        if IPymeKey.providedBy(signing_key):
+            pub_key = gpghandler.retrieveKey(signing_key.fingerprint)
+            gpghandler.uploadPublicKey(pub_key.fingerprint)
+            return pub_key
+        else:
+            assert ISigningKey.providedBy(signing_key)
+            gpghandler.submitKey(removeSecurityProxy(signing_key).public_key)
+            return signing_key
 
     def _storeSigningKey(self, pub_key):
         """Store signing key reference in the database."""
         key_owner = getUtility(ILaunchpadCelebrities).ppa_key_guard
-        key, _ = getUtility(IGPGKeySet).activate(
-            key_owner, pub_key, pub_key.can_encrypt)
-        self.archive.signing_key_owner = key.owner
+        if IPymeKey.providedBy(pub_key):
+            key, _ = getUtility(IGPGKeySet).activate(
+                key_owner, pub_key, pub_key.can_encrypt)
+        else:
+            assert ISigningKey.providedBy(pub_key)
+            key = pub_key
+        self.archive.signing_key_owner = key_owner
         self.archive.signing_key_fingerprint = key.fingerprint
         del get_property_cache(self.archive).signing_key
 
-    def _setupSigningKey(self, secret_key, async_keyserver=False):
+    def _setupSigningKey(self, signing_key, async_keyserver=False):
         """Mandatory setup for signing keys.
 
-        * Export the secret key into the protected disk location.
+        * Export the secret key into the protected disk location (for
+          locally-generated keys).
         * Upload public key to the keyserver.
-        * Store the public GPGKey reference in the database and update
-          the context archive.signing_key.
+        * Store the public GPGKey reference in the database (for
+          locally-generated keys) and update the context
+          archive.signing_key.
         """
-        self.exportSecretKey(secret_key)
+        if IPymeKey.providedBy(signing_key):
+            self.exportSecretKey(signing_key)
         if async_keyserver:
             # If we have an asynchronous keyserver running in the current
             # thread using Twisted, then we need some contortions to ensure
@@ -310,10 +342,10 @@ class ArchiveGPGSigningKey(SignableArchive):
             # Since that thread won't have a Zope interaction, we need to
             # unwrap the security proxy for it.
             d = deferToThread(
-                self._uploadPublicSigningKey, removeSecurityProxy(secret_key))
+                self._uploadPublicSigningKey, removeSecurityProxy(signing_key))
             d.addCallback(ProxyFactory)
             d.addCallback(self._storeSigningKey)
             return d
         else:
-            pub_key = self._uploadPublicSigningKey(secret_key)
+            pub_key = self._uploadPublicSigningKey(signing_key)
             self._storeSigningKey(pub_key)
diff --git a/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py b/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
index 87359e8..67366bd 100644
--- a/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
+++ b/lib/lp/archivepublisher/tests/test_archivegpgsigningkey.py
@@ -14,8 +14,15 @@ from testtools.matchers import (
     FileContains,
     StartsWith,
     )
-from testtools.twistedsupport import AsynchronousDeferredRunTest
-from twisted.internet import defer
+from testtools.twistedsupport import (
+    AsynchronousDeferredRunTest,
+    AsynchronousDeferredRunTestForBrokenTwisted,
+    )
+import treq
+from twisted.internet import (
+    defer,
+    reactor,
+    )
 from zope.component import getUtility
 
 from lp.archivepublisher.config import getPubConfig
@@ -26,18 +33,27 @@ from lp.archivepublisher.interfaces.archivegpgsigningkey import (
     )
 from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet
 from lp.archivepublisher.tests.test_run_parts import RunPartsMixin
+from lp.registry.interfaces.gpg import IGPGKeySet
 from lp.services.compat import mock
 from lp.services.features.testing import FeatureFixture
+from lp.services.gpg.interfaces import IGPGHandler
+from lp.services.gpg.tests.test_gpghandler import FakeGenerateKey
 from lp.services.log.logger import BufferLogger
 from lp.services.osutils import write_file
 from lp.services.signing.enums import (
     SigningKeyType,
     SigningMode,
     )
+from lp.services.signing.interfaces.signingkey import ISigningKeySet
 from lp.services.signing.tests.helpers import SigningServiceClientFixture
+from lp.services.twistedsupport.testing import TReqFixture
+from lp.services.twistedsupport.treq import check_status
 from lp.soyuz.enums import ArchivePurpose
 from lp.testing import TestCaseWithFactory
-from lp.testing.gpgkeys import gpgkeysdir
+from lp.testing.gpgkeys import (
+    gpgkeysdir,
+    test_pubkey_from_email,
+    )
 from lp.testing.keyserver import InProcessKeyServerFixture
 from lp.testing.layers import ZopelessDatabaseLayer
 
@@ -271,3 +287,89 @@ class TestSignableArchiveWithRunParts(RunPartsMixin, TestCaseWithFactory):
             FileContains(
                 "detached signature of %s (%s, %s/%s)\n" %
                 (filename, self.archive_root, self.distro.name, self.suite)))
+
+
+class TestArchiveGPGSigningKey(TestCaseWithFactory):
+
+    layer = ZopelessDatabaseLayer
+    # treq.content doesn't close the connection before yielding control back
+    # to the test, so we need to spin the reactor at the end to finish
+    # things off.
+    run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
+        timeout=10000)
+
+    @defer.inlineCallbacks
+    def setUp(self):
+        super(TestArchiveGPGSigningKey, self).setUp()
+        self.temp_dir = self.makeTemporaryDirectory()
+        self.pushConfig("personalpackagearchive", root=self.temp_dir)
+        self.keyserver = self.useFixture(InProcessKeyServerFixture())
+        yield self.keyserver.start()
+
+    @defer.inlineCallbacks
+    def test_generateSigningKey_local(self):
+        # Generating a signing key locally using GPGHandler stores it in the
+        # database and pushes it to the keyserver.
+        self.useFixture(FakeGenerateKey("[e-mail]"))
+        logger = BufferLogger()
+        # Use a display name that matches the pregenerated sample key.
+        owner = self.factory.makePerson(
+            displayname="Celso \xe1\xe9\xed\xf3\xfa Providelo")
+        archive = self.factory.makeArchive(owner=owner)
+        yield IArchiveGPGSigningKey(archive).generateSigningKey(
+            log=logger, async_keyserver=True)
+        # The key is stored in the database.
+        self.assertIsNotNone(archive.signing_key_owner)
+        self.assertIsNotNone(archive.signing_key_fingerprint)
+        # The key is stored as a GPGKey, not a SigningKey.
+        self.assertIsNotNone(
+            getUtility(IGPGKeySet).getByFingerprint(
+                archive.signing_key_fingerprint))
+        self.assertIsNone(
+            getUtility(ISigningKeySet).get(
+                SigningKeyType.OPENPGP, archive.signing_key_fingerprint))
+        # The key is uploaded to the keyserver.
+        client = self.useFixture(TReqFixture(reactor)).client
+        response = yield client.get(
+            getUtility(IGPGHandler).getURLForKeyInServer(
+                archive.signing_key_fingerprint, "get"))
+        yield check_status(response)
+        content = yield treq.content(response)
+        self.assertIn(b"-----BEGIN PGP PUBLIC KEY BLOCK-----\n", content)
+
+    @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.
+        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("[e-mail]")
+        signing_service_client.generate.return_value = {
+            "fingerprint": "33C0A61893A5DC5EB325B29E415A12CAC2F30234",
+            "public-key": test_key,
+            }
+        logger = BufferLogger()
+        archive = self.factory.makeArchive()
+        yield IArchiveGPGSigningKey(archive).generateSigningKey(
+            log=logger, async_keyserver=True)
+        # The key is stored in the database.
+        self.assertIsNotNone(archive.signing_key_owner)
+        self.assertIsNotNone(archive.signing_key_fingerprint)
+        # The key is stored as a SigningKey, not a GPGKey.
+        self.assertIsNone(
+            getUtility(IGPGKeySet).getByFingerprint(
+                archive.signing_key_fingerprint))
+        signing_key = getUtility(ISigningKeySet).get(
+            SigningKeyType.OPENPGP, archive.signing_key_fingerprint)
+        self.assertEqual(test_key, signing_key.public_key)
+        # The key is uploaded to the keyserver.
+        client = self.useFixture(TReqFixture(reactor)).client
+        response = yield client.get(
+            getUtility(IGPGHandler).getURLForKeyInServer(
+                archive.signing_key_fingerprint, "get"))
+        yield check_status(response)
+        content = yield treq.content(response)
+        self.assertIn(test_key, content)
diff --git a/lib/lp/services/gpg/handler.py b/lib/lp/services/gpg/handler.py
index b2bcbad..e3eba45 100644
--- a/lib/lp/services/gpg/handler.py
+++ b/lib/lp/services/gpg/handler.py
@@ -489,12 +489,8 @@ class GPGHandler:
             raise GPGKeyExpired(key)
         return key
 
-    def _submitKey(self, content):
-        """Submit an ASCII-armored public key export to the keyserver.
-
-        It issues a POST at /pks/add on the keyserver specified in the
-        configuration.
-        """
+    def submitKey(self, content):
+        """See `IGPGHandler`."""
         keyserver_http_url = '%s:%s' % (
             config.gpghandler.host, config.gpghandler.port)
 
@@ -527,7 +523,7 @@ class GPGHandler:
             return
 
         pub_key = self.retrieveKey(fingerprint)
-        self._submitKey(pub_key.export())
+        self.submitKey(pub_key.export())
 
     def getURLForKeyInServer(self, fingerprint, action='index', public=False):
         """See IGPGHandler"""
diff --git a/lib/lp/services/gpg/interfaces.py b/lib/lp/services/gpg/interfaces.py
index 78b44c8..d6f0f73 100644
--- a/lib/lp/services/gpg/interfaces.py
+++ b/lib/lp/services/gpg/interfaces.py
@@ -357,6 +357,17 @@ class IGPGHandler(Interface):
         :return: a `PymeKey`object containing the key information.
         """
 
+    def submitKey(content):
+        """Submit an ASCII-armored public key export to the keyserver.
+
+        It issues a POST at /pks/add on the keyserver specified in the
+        configuration.
+
+        :param content: The exported public key, as a byte string.
+        :raise GPGUploadFailure: if the keyserver could not be reached.
+        :raise AssertionError: if the POST request failed.
+        """
+
     def uploadPublicKey(fingerprint):
         """Upload the specified public key to a keyserver.
 
@@ -365,8 +376,8 @@ class IGPGHandler(Interface):
 
         :param fingerprint: The key fingerprint, which must be an hexadecimal
             string.
-        :raise GPGUploadFailure: if the keyserver could not be reaches.
-        :raise AssertionError: if the POST request doesn't succeed.
+        :raise GPGUploadFailure: if the keyserver could not be reached.
+        :raise AssertionError: if the POST request failed.
         """
 
     def localKeys(filter=None, secret=False):
diff --git a/lib/lp/services/signing/tests/helpers.py b/lib/lp/services/signing/tests/helpers.py
index f819745..6831edb 100644
--- a/lib/lp/services/signing/tests/helpers.py
+++ b/lib/lp/services/signing/tests/helpers.py
@@ -49,7 +49,7 @@ class SigningServiceClientFixture(fixtures.Fixture):
                   openpgp_key_algorithm=None, length=None):
         key = bytes(PrivateKey.generate().public_key)
         data = {
-            "fingerprint": self.factory.getUniqueHexString(40),
+            "fingerprint": self.factory.getUniqueHexString(40).upper(),
             "public-key": key,
             }
         self.generate_returns.append((key_type, data))
@@ -69,7 +69,7 @@ class SigningServiceClientFixture(fixtures.Fixture):
 
     def _inject(self, key_type, private_key, public_key, description,
                 created_at):
-        data = {'fingerprint': self.factory.getUniqueHexString(40)}
+        data = {'fingerprint': self.factory.getUniqueHexString(40).upper()}
         self.inject_returns.append(data)
         return data
 
diff --git a/lib/lp/soyuz/configure.zcml b/lib/lp/soyuz/configure.zcml
index 2e97958..643a85b 100644
--- a/lib/lp/soyuz/configure.zcml
+++ b/lib/lp/soyuz/configure.zcml
@@ -373,7 +373,6 @@
             set_schema="lp.soyuz.interfaces.archive.IArchiveRestricted"/>
         <require
             permission="launchpad.InternalScriptsOnly"
-            attributes="signing_key_owner"
             set_attributes="dirty_suites distribution signing_key_owner
                             signing_key_fingerprint"/>
     </class>
diff --git a/lib/lp/soyuz/interfaces/archive.py b/lib/lp/soyuz/interfaces/archive.py
index 407f953..b6cc663 100644
--- a/lib/lp/soyuz/interfaces/archive.py
+++ b/lib/lp/soyuz/interfaces/archive.py
@@ -461,6 +461,8 @@ class IArchiveSubscriberView(Interface):
             "explicit publish flag and any other constraints."))
     series_with_sources = Attribute(
         "DistroSeries to which this archive has published sources")
+    signing_key_owner = Reference(
+        title=_("Archive signing key owner"), required=False, schema=IPerson)
     signing_key_fingerprint = exported(
         Text(
             title=_("Archive signing key fingerprint"), required=False,
diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py
index a37b737..0d58e56 100644
--- a/lib/lp/soyuz/model/archive.py
+++ b/lib/lp/soyuz/model/archive.py
@@ -466,7 +466,7 @@ class Archive(SQLBase):
         return (
             not config.personalpackagearchive.require_signing_keys or
             not self.is_ppa or
-            self.signing_key is not None)
+            self.signing_key_fingerprint is not None)
 
     @property
     def reference(self):
@@ -2717,10 +2717,12 @@ class ArchiveSet:
                     (owner.name, distribution.name, name))
 
         # Signing-key for the default PPA is reused when it's already present.
-        signing_key = None
+        signing_key_owner = None
+        signing_key_fingerprint = None
         if purpose == ArchivePurpose.PPA:
             if owner.archive is not None:
-                signing_key = owner.archive.signing_key
+                signing_key_owner = owner.archive.signing_key_owner
+                signing_key_fingerprint = owner.archive.signing_key_fingerprint
             else:
                 # owner.archive is a cached property and we've just cached it.
                 del get_property_cache(owner).archive
@@ -2729,9 +2731,8 @@ class ArchiveSet:
             owner=owner, distribution=distribution, name=name,
             displayname=displayname, description=description,
             purpose=purpose, publish=publish,
-            signing_key_owner=signing_key.owner if signing_key else None,
-            signing_key_fingerprint=(
-                signing_key.fingerprint if signing_key else None),
+            signing_key_owner=signing_key_owner,
+            signing_key_fingerprint=signing_key_fingerprint,
             require_virtualized=require_virtualized)
 
         # Upon creation archives are enabled by default.
diff --git a/lib/lp/soyuz/scripts/ppakeygenerator.py b/lib/lp/soyuz/scripts/ppakeygenerator.py
index 190b4a0..88e1d84 100644
--- a/lib/lp/soyuz/scripts/ppakeygenerator.py
+++ b/lib/lp/soyuz/scripts/ppakeygenerator.py
@@ -34,7 +34,7 @@ class PPAKeyGenerator(LaunchpadCronScript):
             (archive.reference, archive.displayname))
         archive_signing_key = IArchiveGPGSigningKey(archive)
         archive_signing_key.generateSigningKey(log=self.logger)
-        self.logger.info("Key %s" % archive.signing_key.fingerprint)
+        self.logger.info("Key %s" % archive.signing_key_fingerprint)
 
     def main(self):
         """Generate signing keys for the selected PPAs."""
@@ -45,11 +45,11 @@ class PPAKeyGenerator(LaunchpadCronScript):
                 raise LaunchpadScriptFailure(
                     "No archive named '%s' could be found."
                     % self.options.archive)
-            if archive.signing_key is not None:
+            if archive.signing_key_fingerprint is not None:
                 raise LaunchpadScriptFailure(
                     "%s (%s) already has a signing_key (%s)"
                     % (archive.reference, archive.displayname,
-                       archive.signing_key.fingerprint))
+                       archive.signing_key_fingerprint))
             archives = [archive]
         else:
             archive_set = getUtility(IArchiveSet)
diff --git a/lib/lp/soyuz/scripts/tests/test_ppakeygenerator.py b/lib/lp/soyuz/scripts/tests/test_ppakeygenerator.py
index 56e8710..a5d3caf 100644
--- a/lib/lp/soyuz/scripts/tests/test_ppakeygenerator.py
+++ b/lib/lp/soyuz/scripts/tests/test_ppakeygenerator.py
@@ -83,7 +83,7 @@ class TestPPAKeyGenerator(TestCase):
             LaunchpadScriptFailure,
             ("~cprov/ubuntu/ppa (PPA for Celso Providelo) already has a "
              "signing_key (%s)" %
-             cprov.archive.signing_key.fingerprint),
+             cprov.archive.signing_key_fingerprint),
             key_generator.main)
 
     def testGenerateKeyForASinglePPA(self):
@@ -95,14 +95,14 @@ class TestPPAKeyGenerator(TestCase):
         cprov = getUtility(IPersonSet).getByName('cprov')
         self._fixArchiveForKeyGeneration(cprov.archive)
 
-        self.assertTrue(cprov.archive.signing_key is None)
+        self.assertIsNone(cprov.archive.signing_key_fingerprint)
 
         txn = FakeTransaction()
         key_generator = self._getKeyGenerator(
             archive_reference='~cprov/ubuntutest/ppa', txn=txn)
         key_generator.main()
 
-        self.assertTrue(cprov.archive.signing_key is not None)
+        self.assertIsNotNone(cprov.archive.signing_key_fingerprint)
         self.assertEqual(txn.commit_count, 1)
 
     def testGenerateKeyForAllPPA(self):
@@ -115,13 +115,13 @@ class TestPPAKeyGenerator(TestCase):
 
         for archive in archives:
             self._fixArchiveForKeyGeneration(archive)
-            self.assertTrue(archive.signing_key is None)
+            self.assertIsNone(archive.signing_key_fingerprint)
 
         txn = FakeTransaction()
         key_generator = self._getKeyGenerator(txn=txn)
         key_generator.main()
 
         for archive in archives:
-            self.assertTrue(archive.signing_key is not None)
+            self.assertIsNotNone(archive.signing_key_fingerprint)
 
         self.assertEqual(txn.commit_count, len(archives))
diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py
index 91acf53..5c3f3b6 100644
--- a/lib/lp/soyuz/tests/test_archive.py
+++ b/lib/lp/soyuz/tests/test_archive.py
@@ -4046,7 +4046,7 @@ class TestSigningKeyPropagation(TestCaseWithFactory):
 
     def test_ppa_created_with_no_signing_key(self):
         ppa = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
-        self.assertIsNone(ppa.signing_key)
+        self.assertIsNone(ppa.signing_key_fingerprint)
 
     def test_default_signing_key_propagated_to_new_ppa(self):
         person = self.factory.makePerson()