launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:gpg-log-inject into launchpad:master.

Commit message:
Log injection of GPG keys into signing service

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

It's otherwise not very obvious that injection has actually happened.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:gpg-log-inject into launchpad:master.

diff --git a/lib/lp/archivepublisher/archivegpgsigningkey.py b/lib/lp/archivepublisher/archivegpgsigningkey.py
index 7439a32..685ba04 100644
--- a/lib/lp/archivepublisher/archivegpgsigningkey.py
+++ b/lib/lp/archivepublisher/archivegpgsigningkey.py
@@ -188,7 +188,7 @@ class ArchiveGPGSigningKey(SignableArchive):
         with open(export_path, 'w') as export_file:
             export_file.write(key.export())
 
-    def generateSigningKey(self):
+    def generateSigningKey(self, log=None):
         """See `IArchiveGPGSigningKey`."""
         assert self.archive.signing_key is None, (
             "Cannot override signing_keys.")
@@ -208,7 +208,8 @@ class ArchiveGPGSigningKey(SignableArchive):
 
         key_displayname = (
             "Launchpad PPA for %s" % self.archive.owner.displayname)
-        secret_key = getUtility(IGPGHandler).generateKey(key_displayname)
+        secret_key = getUtility(IGPGHandler).generateKey(
+            key_displayname, logger=log)
         self._setupSigningKey(secret_key)
 
     def setSigningKey(self, key_path, async_keyserver=False):
diff --git a/lib/lp/archivepublisher/interfaces/archivegpgsigningkey.py b/lib/lp/archivepublisher/interfaces/archivegpgsigningkey.py
index 85d3bfb..2efffb6 100644
--- a/lib/lp/archivepublisher/interfaces/archivegpgsigningkey.py
+++ b/lib/lp/archivepublisher/interfaces/archivegpgsigningkey.py
@@ -98,7 +98,7 @@ class IArchiveGPGSigningKey(ISignableArchive):
         :raises AssertionError: if the given key is public.
         """
 
-    def generateSigningKey():
+    def generateSigningKey(log=None):
         """Generate a new GPG secret/public key pair.
 
         For named-ppas, the existing signing-key for the default PPA
@@ -112,6 +112,7 @@ class IArchiveGPGSigningKey(ISignableArchive):
          * Store a reference for the public key in GPGKey table, which
            is set as the context archive 'signing_key'.
 
+        :param log: an optional logger.
         :raises AssertionError: if the context archive already has a
             `signing_key`.
         :raises GPGUploadFailure: if the just-generated key could not be
diff --git a/lib/lp/archivepublisher/tests/archive-signing.txt b/lib/lp/archivepublisher/tests/archive-signing.txt
index f2dfca9..a487a3b 100644
--- a/lib/lp/archivepublisher/tests/archive-signing.txt
+++ b/lib/lp/archivepublisher/tests/archive-signing.txt
@@ -333,7 +333,7 @@ Then modify the GPGHandler utility to return a sampledata key instead
 of generating a new one, mainly for running the test faster and for
 printing the context the key is generated.
 
-    >>> def mock_key_generator(name):
+    >>> def mock_key_generator(name, logger=None):
     ...     print 'Generating:', name
     ...     key_path = os.path.join(gpgkeysdir, 'sign.only@xxxxxxxxxxxxxxxxx')
     ...     return gpghandler.importSecretKey(open(key_path).read())
diff --git a/lib/lp/services/gpg/handler.py b/lib/lp/services/gpg/handler.py
index 965282a..e90538c 100644
--- a/lib/lp/services/gpg/handler.py
+++ b/lib/lp/services/gpg/handler.py
@@ -334,7 +334,7 @@ class GPGHandler:
             SigningKeyType.OPENPGP, secret_key, public_key, key.uids[0].name,
             now)
 
-    def generateKey(self, name):
+    def generateKey(self, name, logger=None):
         """See `IGPGHandler`."""
         context = get_gpgme_context()
 
@@ -368,6 +368,10 @@ class GPGHandler:
             'The key does not seem to exist in the local keyring.')
 
         if getFeatureFlag(GPG_INJECT):
+            if logger is not None:
+                logger.info(
+                    "Injecting key_type %s '%s' into signing service",
+                    SigningKeyType.OPENPGP, name)
             try:
                 self._injectKeyPair(key)
             except Exception:
diff --git a/lib/lp/services/gpg/tests/test_gpghandler.py b/lib/lp/services/gpg/tests/test_gpghandler.py
index 162f39a..5d8651e 100644
--- a/lib/lp/services/gpg/tests/test_gpghandler.py
+++ b/lib/lp/services/gpg/tests/test_gpghandler.py
@@ -362,7 +362,7 @@ class TestGPGHandler(TestCase):
             "op=index&search=0x%s" % fingerprint,
             self.gpg_handler.getURLForKeyInServer(fingerprint, public=True))
 
-    def assertGeneratesKey(self):
+    def assertGeneratesKey(self, logger=None):
         # We don't test real key generation because it depends on machine
         # entropy that we may not have in buildbot, and in general may be
         # slow.  The sample key on disk was generated by running the code
@@ -378,7 +378,8 @@ class TestGPGHandler(TestCase):
         #     export_file.write(new_key.export())
         self.useFixture(FakeGenerateKey("ppa-sample@xxxxxxxxxxxxxxxxx"))
         new_key = self.gpg_handler.generateKey(
-            u"Launchpad PPA for Celso \xe1\xe9\xed\xf3\xfa Providelo")
+            u"Launchpad PPA for Celso \xe1\xe9\xed\xf3\xfa Providelo",
+            logger=logger)
         # generateKey currently only generates passwordless sign-only keys,
         # i.e. they can sign content but cannot encrypt.  The generated key
         # contains a single UID and only its "name" term is set.
@@ -437,8 +438,13 @@ class TestGPGHandler(TestCase):
         mock_datetime = self.useFixture(MockPatch(
             "lp.services.gpg.handler.datetime")).mock
         mock_datetime.now = lambda: now
-        new_key = self.assertGeneratesKey()
+        logger = BufferLogger()
+        new_key = self.assertGeneratesKey(logger=logger)
         new_public_key = self.gpg_handler.retrieveKey(new_key.fingerprint)
+        self.assertEqual(
+            u"INFO Injecting key_type OpenPGP 'Launchpad PPA for Celso "
+            u"\xe1\xe9\xed\xf3\xfa Providelo' into signing service\n",
+            logger.getLogBuffer())
         self.assertEqual(1, signing_service_client.inject.call_count)
         self.assertThat(
             signing_service_client.inject.call_args[0],
diff --git a/lib/lp/soyuz/scripts/ppakeygenerator.py b/lib/lp/soyuz/scripts/ppakeygenerator.py
index c1aa3f6..190b4a0 100644
--- a/lib/lp/soyuz/scripts/ppakeygenerator.py
+++ b/lib/lp/soyuz/scripts/ppakeygenerator.py
@@ -33,7 +33,7 @@ class PPAKeyGenerator(LaunchpadCronScript):
             "Generating signing key for %s (%s)" %
             (archive.reference, archive.displayname))
         archive_signing_key = IArchiveGPGSigningKey(archive)
-        archive_signing_key.generateSigningKey()
+        archive_signing_key.generateSigningKey(log=self.logger)
         self.logger.info("Key %s" % archive.signing_key.fingerprint)
 
     def main(self):