launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/fix-release-file-signing into lp:launchpad.

Commit message:
Fix signing of Release files when distsroot is overridden.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1759422 in Launchpad itself: "r18584 broke Release file signing with overridden distsroot"
  https://bugs.launchpad.net/launchpad/+bug/1759422

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/fix-release-file-signing/+merge/342252
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/fix-release-file-signing into lp:launchpad.

=== modified file 'lib/lp/archivepublisher/archivesigningkey.py'
--- lib/lp/archivepublisher/archivesigningkey.py	2018-01-19 17:38:51 +0000
+++ lib/lp/archivepublisher/archivesigningkey.py	2018-03-27 23:03:09 +0000
@@ -62,6 +62,7 @@
 
     def __init__(self, archive):
         self.archive = archive
+        self.pubconf = getPubConfig(self.archive)
 
     @property
     def can_sign(self):
@@ -119,9 +120,11 @@
                     "No signing key available for %s" %
                     self.archive.displayname)
 
-    def signRepository(self, suite, suffix='', log=None):
+    def signRepository(self, suite, pubconf=None, suffix='', log=None):
         """See `ISignableArchive`."""
-        suite_path = os.path.join(self._archive_root_path, 'dists', suite)
+        if pubconf is None:
+            pubconf = self.pubconf
+        suite_path = os.path.join(pubconf.distsroot, suite)
         release_file_path = os.path.join(suite_path, 'Release' + suffix)
         if not os.path.exists(release_file_path):
             raise AssertionError(
@@ -140,12 +143,12 @@
     def signFile(self, suite, path, log=None):
         """See `ISignableArchive`."""
         # Allow the passed path to be relative to the archive root.
-        path = os.path.realpath(os.path.join(self._archive_root_path, path))
+        path = os.path.realpath(os.path.join(self.pubconf.archiveroot, path))
 
         # Ensure the resulting path is within the archive root after
         # normalisation.
         # NOTE: uses os.sep to prevent /var/tmp/../tmpFOO attacks.
-        archive_root = self._archive_root_path + os.sep
+        archive_root = self.pubconf.archiveroot + os.sep
         if not path.startswith(archive_root):
             raise AssertionError(
                 "Attempting to sign file (%s) outside archive_root for %s" % (
@@ -159,10 +162,6 @@
 class ArchiveSigningKey(SignableArchive):
     """`IArchive` adapter for manipulating its GPG key."""
 
-    @property
-    def _archive_root_path(self):
-        return getPubConfig(self.archive).archiveroot
-
     def getPathForSecretKey(self, key):
         """See `IArchiveSigningKey`."""
         return os.path.join(

=== modified file 'lib/lp/archivepublisher/interfaces/archivesigningkey.py'
--- lib/lp/archivepublisher/interfaces/archivesigningkey.py	2018-01-19 15:38:21 +0000
+++ lib/lp/archivepublisher/interfaces/archivesigningkey.py	2018-03-27 23:03:09 +0000
@@ -36,10 +36,15 @@
 
     can_sign = Attribute("True if this archive is set up for signing.")
 
-    def signRepository(suite, log=None):
+    def signRepository(suite, pubconf=None, suffix='', log=None):
         """Sign the corresponding repository.
 
         :param suite: suite name to be signed.
+        :param pubconf: an optional publisher configuration instance
+            indicating where files should be written (if not passed, uses
+            the archive's defaults).
+        :param suffix: an optional suffix for repository index files (e.g.
+            ".new" to help with publishing files atomically).
         :param log: an optional logger.
         :raises CannotSignArchive: if the context archive is not set up for
             signing.

=== modified file 'lib/lp/archivepublisher/publishing.py'
--- lib/lp/archivepublisher/publishing.py	2018-01-19 17:38:51 +0000
+++ lib/lp/archivepublisher/publishing.py	2018-03-27 23:03:09 +0000
@@ -1247,7 +1247,8 @@
         if signable_archive.can_sign:
             # Sign the repository.
             self.log.debug("Signing Release file for %s" % suite)
-            signable_archive.signRepository(suite, suffix=".new", log=self.log)
+            signable_archive.signRepository(
+                suite, pubconf=self._config, suffix=".new", log=self.log)
             core_files.add("Release.gpg")
             core_files.add("InRelease")
         else:

=== modified file 'lib/lp/archivepublisher/tests/test_archivesigningkey.py'
--- lib/lp/archivepublisher/tests/test_archivesigningkey.py	2018-03-21 11:52:42 +0000
+++ lib/lp/archivepublisher/tests/test_archivesigningkey.py	2018-03-27 23:03:09 +0000
@@ -62,8 +62,7 @@
         self.assertTrue(signer.can_sign)
         signer.signFile(self.suite, filename)
 
-        signature = filename + '.gpg'
-        self.assertTrue(os.path.exists(signature))
+        self.assertTrue(os.path.exists(filename + ".gpg"))
 
     def test_signFile_absolute_outside_archive(self):
         filename = os.path.join(self.temp_dir, "signme")
@@ -72,7 +71,7 @@
         signer = ISignableArchive(self.archive)
         self.assertTrue(signer.can_sign)
         self.assertRaises(
-            AssertionError, lambda: signer.signFile(self.suite, filename))
+            AssertionError, signer.signFile, self.suite, filename)
 
     def test_signFile_relative_within_archive(self):
         filename_relative = "signme"
@@ -83,8 +82,7 @@
         self.assertTrue(signer.can_sign)
         signer.signFile(self.suite, filename_relative)
 
-        signature = filename + '.gpg'
-        self.assertTrue(os.path.exists(signature))
+        self.assertTrue(os.path.exists(filename + ".gpg"))
 
     def test_signFile_relative_outside_archive(self):
         filename_relative = "../signme"
@@ -94,8 +92,7 @@
         signer = ISignableArchive(self.archive)
         self.assertTrue(signer.can_sign)
         self.assertRaises(
-            AssertionError,
-            lambda: signer.signFile(self.suite, filename_relative))
+            AssertionError, signer.signFile, self.suite, filename_relative)
 
 
 class TestSignableArchiveWithRunParts(RunPartsMixin, TestCaseWithFactory):
@@ -144,6 +141,29 @@
                 "clear signature of %s (%s/%s)\n" %
                 (release_path, self.distro.name, self.suite)))
 
+    def test_signRepository_honours_pubconf(self):
+        pubconf = getPubConfig(self.archive)
+        pubconf.distsroot = self.makeTemporaryDirectory()
+        suite_dir = os.path.join(pubconf.distsroot, self.suite)
+        release_path = os.path.join(suite_dir, "Release")
+        write_file(release_path, "Release contents")
+
+        signer = ISignableArchive(self.archive)
+        self.assertTrue(signer.can_sign)
+        self.assertRaises(AssertionError, signer.signRepository, self.suite)
+        signer.signRepository(self.suite, pubconf=pubconf)
+
+        self.assertThat(
+            os.path.join(suite_dir, "Release.gpg"),
+            FileContains(
+                "detached signature of %s (%s/%s)\n" %
+                (release_path, self.distro.name, self.suite)))
+        self.assertThat(
+            os.path.join(suite_dir, "InRelease"),
+            FileContains(
+                "clear signature of %s (%s/%s)\n" %
+                (release_path, self.distro.name, self.suite)))
+
     def test_signFile_runs_parts(self):
         filename = os.path.join(self.archive_root, "signme")
         write_file(filename, "sign this")

=== modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
--- lib/lp/archivepublisher/tests/test_publisher.py	2018-03-26 06:51:44 +0000
+++ lib/lp/archivepublisher/tests/test_publisher.py	2018-03-27 23:03:09 +0000
@@ -34,6 +34,10 @@
 except ImportError:
     from backports import lzma
 import pytz
+from testscenarios import (
+    load_tests_apply_scenarios,
+    WithScenarios,
+    )
 from testtools.matchers import (
     ContainsAll,
     DirContains,
@@ -2928,9 +2932,15 @@
             os.rename(temporary_dists, original_dists)
 
 
-class TestPublisherRepositorySignatures(RunPartsMixin, TestPublisherBase):
+class TestPublisherRepositorySignatures(
+        WithScenarios, RunPartsMixin, TestPublisherBase):
     """Testing `Publisher` signature behaviour."""
 
+    scenarios = [
+        ('default distsroot', {'override_distsroot': False}),
+        ('overridden distsroot', {'override_distsroot': True}),
+        ]
+
     run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=10)
 
     archive_publisher = None
@@ -2947,6 +2957,9 @@
             allowed_suites = []
             self.archive_publisher = getPublisher(
                 archive, allowed_suites, self.logger)
+            if self.override_distsroot:
+                self.archive_publisher._config.distsroot = (
+                    self.makeTemporaryDirectory())
 
     def _publishArchive(self, archive):
         """Publish a test source in the given archive.
@@ -3335,3 +3348,6 @@
             ),
         }
         self.assertThat(self.fetchSums(rootdir), MatchesDict(expected))
+
+
+load_tests = load_tests_apply_scenarios