launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/publisher-missing-signed-files into lp:launchpad.

Commit message:
Cope with sign.d implementations that decline to produce some of the requested signatures.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/publisher-missing-signed-files/+merge/344996

This squashes a cowboy currently in place on pepo, but in a different way.  ubuntu-archive-publishing currently skips InRelease generation for precise and trusty, even (perhaps incorrectly) for partner, so if partner is updated for either of those series then the publisher would previously crash.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/publisher-missing-signed-files into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/archivesigningkey.py'
--- lib/lp/archivepublisher/archivesigningkey.py	2018-03-28 09:30:48 +0000
+++ lib/lp/archivepublisher/archivesigningkey.py	2018-05-03 00:40:46 +0000
@@ -41,6 +41,7 @@
 from lp.registry.interfaces.gpg import IGPGKeySet
 from lp.services.config import config
 from lp.services.gpg.interfaces import IGPGHandler
+from lp.services.osutils import remove_if_exists
 from lp.services.propertycache import get_property_cache
 
 
@@ -80,8 +81,10 @@
         key more than once.
 
         :param signatures: A sequence of (input path, output path,
-            `SigningMode`, suite) tuples.
+            `SigningMode`, suite) tuples.  Note that some backends may make
+            a policy decision not to produce all the requested output paths.
         :param log: An optional logger.
+        :return: A list of output paths that were produced.
         """
         if not self.can_sign:
             raise CannotSignArchive(
@@ -95,6 +98,7 @@
             gpghandler = getUtility(IGPGHandler)
             secret_key = gpghandler.importSecretKey(secret_key_export)
 
+        output_paths = []
         for input_path, output_path, mode, suite in signatures:
             if self.archive.signing_key is not None:
                 with open(input_path) as input_file:
@@ -103,8 +107,10 @@
                     input_content, secret_key, mode=self.gpgme_modes[mode])
                 with open(output_path, "w") as output_file:
                     output_file.write(signature)
+                output_paths.append(output_path)
             elif find_run_parts_dir(
                     self.archive.distribution.name, "sign.d") is not None:
+                remove_if_exists(output_path)
                 env = {
                     "ARCHIVEROOT": self.pubconf.archiveroot,
                     "INPUT_PATH": input_path,
@@ -116,10 +122,13 @@
                 run_parts(
                     self.archive.distribution.name, "sign.d",
                     log=log, env=env)
+                if os.path.exists(output_path):
+                    output_paths.append(output_path)
             else:
                 raise AssertionError(
                     "No signing key available for %s" %
                     self.archive.displayname)
+        return output_paths
 
     def signRepository(self, suite, pubconf=None, suffix='', log=None):
         """See `ISignableArchive`."""
@@ -132,14 +141,22 @@
                 "Release file doesn't exist in the repository: %s" %
                 release_file_path)
 
-        self._makeSignatures([
-            (release_file_path,
-             os.path.join(suite_path, 'Release.gpg' + suffix),
-             SigningMode.DETACHED, suite),
-            (release_file_path,
-             os.path.join(suite_path, 'InRelease' + suffix),
-             SigningMode.CLEAR, suite),
-            ], log=log)
+        output_names = []
+        for output_path in self._makeSignatures([
+                (release_file_path,
+                 os.path.join(suite_path, 'Release.gpg' + suffix),
+                 SigningMode.DETACHED, suite),
+                (release_file_path,
+                 os.path.join(suite_path, 'InRelease' + suffix),
+                 SigningMode.CLEAR, suite),
+                ], log=log):
+            output_name = os.path.basename(output_path)
+            if suffix:
+                output_name = output_name[:-len(suffix)]
+            assert (
+                os.path.join(suite_path, output_name + suffix) == output_path)
+            output_names.append(output_name)
+        return output_names
 
     def signFile(self, suite, path, log=None):
         """See `ISignableArchive`."""

=== modified file 'lib/lp/archivepublisher/interfaces/archivesigningkey.py'
--- lib/lp/archivepublisher/interfaces/archivesigningkey.py	2018-03-27 23:02:02 +0000
+++ lib/lp/archivepublisher/interfaces/archivesigningkey.py	2018-05-03 00:40:46 +0000
@@ -46,6 +46,8 @@
         :param suffix: an optional suffix for repository index files (e.g.
             ".new" to help with publishing files atomically).
         :param log: an optional logger.
+        :return: A sequence of output paths that were produced, relative to
+            the suite's path, with `suffix` removed.
         :raises CannotSignArchive: if the context archive is not set up for
             signing.
         :raises AssertionError: if there is no Release file in the given

=== modified file 'lib/lp/archivepublisher/publishing.py'
--- lib/lp/archivepublisher/publishing.py	2018-03-27 23:26:12 +0000
+++ lib/lp/archivepublisher/publishing.py	2018-05-03 00:40:46 +0000
@@ -1264,12 +1264,10 @@
         if signable_archive.can_sign:
             # Sign the repository.
             self.log.debug("Signing Release file for %s" % suite)
-            signable_archive.signRepository(
-                suite, pubconf=self._config, suffix=".new", log=self.log)
-            core_files.add("Release.gpg")
-            extra_by_hash_files["Release.gpg"] = "Release.gpg.new"
-            core_files.add("InRelease")
-            extra_by_hash_files["InRelease"] = "InRelease.new"
+            for signed_name in signable_archive.signRepository(
+                    suite, pubconf=self._config, suffix=".new", log=self.log):
+                core_files.add(signed_name)
+                extra_by_hash_files[signed_name] = signed_name + ".new"
         else:
             # Skip signature if the archive is not set up for signing.
             self.log.debug("No signing key available, skipping signature.")

=== modified file 'lib/lp/archivepublisher/tests/archive-signing.txt'
--- lib/lp/archivepublisher/tests/archive-signing.txt	2018-01-19 15:38:21 +0000
+++ lib/lp/archivepublisher/tests/archive-signing.txt	2018-05-03 00:40:46 +0000
@@ -401,7 +401,7 @@
     >>> release_file.write('This is a fake release file.')
     >>> release_file.close()
 
-    >>> archive_signing_key.signRepository(test_suite)
+    >>> _ = archive_signing_key.signRepository(test_suite)
 
     >>> print open(release_path + '.gpg').read()
     -----BEGIN PGP SIGNATURE-----

=== modified file 'lib/lp/archivepublisher/tests/test_archivesigningkey.py'
--- lib/lp/archivepublisher/tests/test_archivesigningkey.py	2018-03-28 09:30:48 +0000
+++ lib/lp/archivepublisher/tests/test_archivesigningkey.py	2018-05-03 00:40:46 +0000
@@ -129,7 +129,8 @@
 
         signer = ISignableArchive(self.archive)
         self.assertTrue(signer.can_sign)
-        signer.signRepository(self.suite)
+        self.assertContentEqual(
+            ["Release.gpg", "InRelease"], signer.signRepository(self.suite))
 
         self.assertThat(
             os.path.join(suite_dir, "Release.gpg"),
@@ -154,7 +155,9 @@
         signer = ISignableArchive(self.archive)
         self.assertTrue(signer.can_sign)
         self.assertRaises(AssertionError, signer.signRepository, self.suite)
-        signer.signRepository(self.suite, pubconf=pubconf)
+        self.assertContentEqual(
+            ["Release.gpg", "InRelease"],
+            signer.signRepository(self.suite, pubconf=pubconf))
 
         self.assertThat(
             os.path.join(suite_dir, "Release.gpg"),

=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
--- lib/lp/archivepublisher/tests/test_publisher.py	2018-04-05 11:32:50 +0000
+++ lib/lp/archivepublisher/tests/test_publisher.py	2018-05-03 00:40:46 +0000
@@ -3213,7 +3213,7 @@
 
         # Release.gpg and InRelease exist with suitable fake signatures.
         # Note that the signatures are made before Release.new is renamed to
-        # to Release.
+        # Release.
         self.assertThat(
             self.release_file_signature_path,
             FileContains(
@@ -3236,6 +3236,55 @@
         self.assertThat(
             sync_args[1], ContainsAll(['Release', 'Release.gpg', 'InRelease']))
 
+    def testRepositorySignatureWithSelectiveRunParts(self):
+        """Check publisher behaviour when partially signing repositories.
+
+        A 'sign.d' run-parts implementation may choose to produce only a
+        subset of signatures.
+        """
+        cprov = getUtility(IPersonSet).getByName('cprov')
+        self.assertIsNone(cprov.archive.signing_key)
+        self.enableRunParts(distribution_name=cprov.archive.distribution.name)
+        sign_directory = os.path.join(
+            self.parts_directory, cprov.archive.distribution.name, 'sign.d')
+        with open(os.path.join(sign_directory, '10-sign'), 'w') as sign_script:
+            sign_script.write(dedent("""\
+                #! /bin/sh
+                [ "$(basename "$OUTPUT_PATH" .new)" != InRelease ] || exit 0
+                echo "$MODE signature of $INPUT_PATH" \\
+                     "($ARCHIVEROOT, $DISTRIBUTION/$SUITE)" \\
+                    >"$OUTPUT_PATH"
+                """))
+            os.fchmod(sign_script.fileno(), 0o755)
+
+        self.setupPublisher(cprov.archive)
+        self.archive_publisher._syncTimestamps = FakeMethod()
+
+        self._publishArchive(cprov.archive)
+
+        # Release exists.
+        self.assertThat(self.release_file_path, PathExists())
+
+        # Release.gpg exists with a suitable fake signature.  Note that the
+        # signature is made before Release.new is renamed to Release.
+        self.assertThat(
+            self.release_file_signature_path,
+            FileContains(
+                "detached signature of %s.new (%s, %s/breezy-autotest)\n" %
+                (self.release_file_path,
+                 self.archive_publisher._config.archiveroot,
+                 cprov.archive.distribution.name)))
+
+        # InRelease does not exist.
+        self.assertThat(self.inline_release_file_path, Not(PathExists()))
+
+        # The publisher synchronises the various Release file timestamps.
+        self.assertEqual(1, self.archive_publisher._syncTimestamps.call_count)
+        sync_args = self.archive_publisher._syncTimestamps.extract_args()[0]
+        self.assertEqual(self.distroseries.name, sync_args[0])
+        self.assertThat(sync_args[1], ContainsAll(['Release', 'Release.gpg']))
+        self.assertNotIn('InRelease', sync_args[1])
+
 
 class TestPublisherLite(TestCaseWithFactory):
     """Lightweight unit tests for the publisher."""