launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #30402
[Merge] ~cjwatson/launchpad:publisherconfig-base-dir into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:publisherconfig-base-dir into launchpad:master.
Commit message:
Expand relative directories in PublisherConfig.root_dir
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/449222
The `PublisherConfig` table is designed quite strangely: it includes a `root_dir` column which stores the root file system path at which each distribution is published. This is a problem for charmed deployments of the primary Ubuntu and copy archive publishers, because our charms don't use the same base directory as our legacy deployments.
Fortunately, all the existing paths are absolute, so we have a reasonable migration path that doesn't require us to change the database at the same time as migrating to a new deployment. Interpret relative paths in `PublisherConfig.root_dir` as being relative to a new `config.archivepublisher.archives_dir` configuration entry; we can set this to appropriate values in both legacy and charmed deployments, and then change existing production database rows to use relative paths.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:publisherconfig-base-dir into launchpad:master.
diff --git a/lib/lp/archivepublisher/config.py b/lib/lp/archivepublisher/config.py
index acf5480..af3b900 100644
--- a/lib/lp/archivepublisher/config.py
+++ b/lib/lp/archivepublisher/config.py
@@ -42,7 +42,7 @@ def getPubConfig(archive):
return None
pubconf.temproot = os.path.join(
- db_pubconf.root_dir, "%s-temp" % archive.distribution.name
+ db_pubconf.absolute_root_dir, "%s-temp" % archive.distribution.name
)
if archive.publishing_method == ArchivePublishingMethod.ARTIFACTORY:
@@ -72,13 +72,13 @@ def getPubConfig(archive):
archive.distribution.name,
)
elif archive.is_main:
- pubconf.distroroot = db_pubconf.root_dir
+ pubconf.distroroot = db_pubconf.absolute_root_dir
pubconf.archiveroot = os.path.join(
pubconf.distroroot, archive.distribution.name
)
pubconf.archiveroot += archive_suffixes[archive.purpose]
elif archive.is_copy:
- pubconf.distroroot = db_pubconf.root_dir
+ pubconf.distroroot = db_pubconf.absolute_root_dir
pubconf.archiveroot = os.path.join(
pubconf.distroroot,
archive.distribution.name + "-" + archive.name,
diff --git a/lib/lp/archivepublisher/interfaces/publisherconfig.py b/lib/lp/archivepublisher/interfaces/publisherconfig.py
index 2bf04fa..6543e08 100644
--- a/lib/lp/archivepublisher/interfaces/publisherconfig.py
+++ b/lib/lp/archivepublisher/interfaces/publisherconfig.py
@@ -8,11 +8,14 @@ __all__ = [
"IPublisherConfigSet",
]
+import os.path
+
from lazr.restful.fields import Reference
from zope.interface import Interface
from zope.schema import Int, TextLine
from lp import _
+from lp.app.validators.path import path_does_not_escape
from lp.registry.interfaces.distribution import IDistribution
@@ -32,6 +35,24 @@ class IPublisherConfig(Interface):
title=_("Root Directory"),
required=True,
description=_("The root directory for published archives."),
+ # Only basic validation; setting this field is restricted to
+ # Launchpad administrators, and they can already set full absolute
+ # paths here, so we don't currently worry about symlink attacks or
+ # similar. We forbid a leading ../ just to avoid getting ourselves
+ # into confusing tangles.
+ constraint=(
+ lambda path: os.path.isabs(path) or path_does_not_escape(path)
+ ),
+ )
+
+ absolute_root_dir = TextLine(
+ title=_("Root directory as an absolute path"),
+ required=True,
+ readonly=True,
+ description=_(
+ "The root directory for published archives, expanded relative to "
+ "config.archivepublisher.archives_dir."
+ ),
)
base_url = TextLine(
diff --git a/lib/lp/archivepublisher/model/publisherconfig.py b/lib/lp/archivepublisher/model/publisherconfig.py
index 3842a88..1d94e45 100644
--- a/lib/lp/archivepublisher/model/publisherconfig.py
+++ b/lib/lp/archivepublisher/model/publisherconfig.py
@@ -8,6 +8,8 @@ __all__ = [
"PublisherConfigSet",
]
+import os.path
+
from storm.locals import Int, Reference, Unicode
from zope.interface import implementer
@@ -15,13 +17,14 @@ from lp.archivepublisher.interfaces.publisherconfig import (
IPublisherConfig,
IPublisherConfigSet,
)
+from lp.services.config import config
from lp.services.database.interfaces import IPrimaryStore, IStore
from lp.services.database.stormbase import StormBase
@implementer(IPublisherConfig)
class PublisherConfig(StormBase):
- """See `IArchiveAuthToken`."""
+ """See `IPublisherConfig`."""
__storm_table__ = "PublisherConfig"
@@ -36,6 +39,16 @@ class PublisherConfig(StormBase):
copy_base_url = Unicode(name="copy_base_url", allow_none=False)
+ @property
+ def absolute_root_dir(self):
+ """See `IPublisherConfig`."""
+ if os.path.isabs(self.root_dir):
+ return self.root_dir
+ else:
+ return os.path.join(
+ config.archivepublisher.archives_dir, self.root_dir
+ )
+
@implementer(IPublisherConfigSet)
class PublisherConfigSet:
diff --git a/lib/lp/archivepublisher/security.py b/lib/lp/archivepublisher/security.py
index 62a2d87..3326cf9 100644
--- a/lib/lp/archivepublisher/security.py
+++ b/lib/lp/archivepublisher/security.py
@@ -10,5 +10,7 @@ from lp.security import AdminByAdminsTeam
__all__ = [] # type: List[str]
+# If edit access to this is ever opened up beyond admins, then we need to
+# take more care with validating IPublisherConfig.root_dir.
class ViewPublisherConfig(AdminByAdminsTeam):
usedfor = IPublisherConfig
diff --git a/lib/lp/archivepublisher/tests/test_config.py b/lib/lp/archivepublisher/tests/test_config.py
index a25accd..b25aab9 100644
--- a/lib/lp/archivepublisher/tests/test_config.py
+++ b/lib/lp/archivepublisher/tests/test_config.py
@@ -13,6 +13,7 @@ from pathlib import Path
from zope.component import getUtility
from lp.archivepublisher.config import getPubConfig
+from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet
from lp.registry.interfaces.distribution import IDistributionSet
from lp.services.config import config
from lp.services.log.logger import BufferLogger
@@ -67,6 +68,19 @@ class TestGetPubConfig(TestCaseWithFactory):
primary_config = getPubConfig(self.ubuntutest.main_archive)
self.assertEqual(archiveroot + "-uefi", primary_config.signingroot)
+ def test_primary_config_relative_root_dir(self):
+ self.pushConfig(
+ "archivepublisher", archives_dir="/srv/launchpad/archives"
+ )
+ getUtility(IPublisherConfigSet).getByDistribution(
+ self.ubuntutest
+ ).root_dir = "relative-ubuntutest"
+ primary_config = getPubConfig(self.ubuntutest.main_archive)
+ root = "/srv/launchpad/archives/relative-ubuntutest"
+ self.assertEqual(root, primary_config.distroroot)
+ self.assertEqual(root + "/ubuntutest", primary_config.archiveroot)
+ self.assertEqual(root + "/ubuntutest-temp", primary_config.temproot)
+
def test_partner_config(self):
# Partner archive configuration is correct.
# The publisher config for PARTNER contains only 'partner' in its
@@ -93,6 +107,24 @@ class TestGetPubConfig(TestCaseWithFactory):
self.assertIs(None, partner_config.metaroot)
self.assertEqual(archiveroot + "-staging", partner_config.stagingroot)
+ def test_partner_config_relative_root_dir(self):
+ self.pushConfig(
+ "archivepublisher", archives_dir="/srv/launchpad/archives"
+ )
+ getUtility(IPublisherConfigSet).getByDistribution(
+ self.ubuntutest
+ ).root_dir = "relative-ubuntutest"
+ partner_archive = getUtility(IArchiveSet).getByDistroAndName(
+ self.ubuntutest, "partner"
+ )
+ partner_config = getPubConfig(partner_archive)
+ root = "/srv/launchpad/archives/relative-ubuntutest"
+ self.assertEqual(root, partner_config.distroroot)
+ self.assertEqual(
+ root + "/ubuntutest-partner", partner_config.archiveroot
+ )
+ self.assertEqual(root + "/ubuntutest-temp", partner_config.temproot)
+
def test_copy_config(self):
# In the case of copy archives (used for rebuild testing) the
# archiveroot is of the form
@@ -118,6 +150,31 @@ class TestGetPubConfig(TestCaseWithFactory):
self.assertIs(None, copy_config.metaroot)
self.assertIs(None, copy_config.stagingroot)
+ def test_copy_config_relative_root_dir(self):
+ self.pushConfig(
+ "archivepublisher", archives_dir="/srv/launchpad/archives"
+ )
+ getUtility(IPublisherConfigSet).getByDistribution(
+ self.ubuntutest
+ ).root_dir = "relative-ubuntutest"
+ copy_archive = getUtility(IArchiveSet).new(
+ purpose=ArchivePurpose.COPY,
+ owner=self.ubuntutest.owner,
+ distribution=self.ubuntutest,
+ name="rebuildtest99",
+ )
+ copy_config = getPubConfig(copy_archive)
+ root = "/srv/launchpad/archives/relative-ubuntutest"
+ self.assertEqual(root, copy_config.distroroot)
+ self.assertEqual(
+ root + "/ubuntutest-rebuildtest99/ubuntutest",
+ copy_config.archiveroot,
+ )
+ self.assertEqual(
+ root + "/ubuntutest-rebuildtest99/ubuntutest-temp",
+ copy_config.temproot,
+ )
+
def test_getDiskPool(self):
primary_config = getPubConfig(self.ubuntutest.main_archive)
disk_pool = primary_config.getDiskPool(BufferLogger())
diff --git a/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py b/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
index 770c667..6fd5732 100644
--- a/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
+++ b/lib/lp/archivepublisher/tests/test_publish_ftpmaster.py
@@ -68,7 +68,9 @@ def get_pub_config(distro):
def get_archive_root(pub_config):
"""Return the archive root for the given publishing config."""
- return os.path.join(pub_config.root_dir, pub_config.distribution.name)
+ return os.path.join(
+ pub_config.absolute_root_dir, pub_config.distribution.name
+ )
def get_dists_root(pub_config):
@@ -218,7 +220,9 @@ class TestPublishFTPMasterScript(
:param script_code: The code to go into the script.
"""
distro_config = get_pub_config(distro)
- parts_base = os.path.join(distro_config.root_dir, "distro-parts")
+ parts_base = os.path.join(
+ distro_config.absolute_root_dir, "distro-parts"
+ )
self.enableRunParts(parts_base)
script_dir = os.path.join(parts_base, distro.name, parts_dir)
os.makedirs(script_dir)
diff --git a/lib/lp/archivepublisher/tests/test_publishdistro.py b/lib/lp/archivepublisher/tests/test_publishdistro.py
index bf36a5e..ad591fa 100644
--- a/lib/lp/archivepublisher/tests/test_publishdistro.py
+++ b/lib/lp/archivepublisher/tests/test_publishdistro.py
@@ -627,7 +627,7 @@ class TestPublishDistro(TestNativePublishingBase):
root_dir = (
getUtility(IPublisherConfigSet)
.getByDistribution(ubuntutest)
- .root_dir
+ .absolute_root_dir
)
repo_path = os.path.join(
root_dir,
diff --git a/lib/lp/archivepublisher/tests/test_publisherconfig.py b/lib/lp/archivepublisher/tests/test_publisherconfig.py
index 7966d8b..e883dab 100644
--- a/lib/lp/archivepublisher/tests/test_publisherconfig.py
+++ b/lib/lp/archivepublisher/tests/test_publisherconfig.py
@@ -68,6 +68,26 @@ class TestPublisherConfig(TestCaseWithFactory):
)
self.assertEqual(self.distribution.name, pubconf.distribution.name)
+ def test_preserve_absolute_root_dir(self):
+ self.pushConfig(
+ "archivepublisher", archives_dir="/srv/launchpad/archives"
+ )
+ pubconf = self.factory.makePublisherConfig(
+ root_dir="/srv/launchpad.test/distro-name"
+ )
+ self.assertEqual(
+ "/srv/launchpad.test/distro-name", pubconf.absolute_root_dir
+ )
+
+ def test_expand_relative_root_dir(self):
+ self.pushConfig(
+ "archivepublisher", archives_dir="/srv/launchpad/archives"
+ )
+ pubconf = self.factory.makePublisherConfig(root_dir="distro-name")
+ self.assertEqual(
+ "/srv/launchpad/archives/distro-name", pubconf.absolute_root_dir
+ )
+
class TestPublisherConfigSecurity(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
diff --git a/lib/lp/registry/browser/tests/test_distribution_views.py b/lib/lp/registry/browser/tests/test_distribution_views.py
index c5e893a..178f5dd 100644
--- a/lib/lp/registry/browser/tests/test_distribution_views.py
+++ b/lib/lp/registry/browser/tests/test_distribution_views.py
@@ -4,7 +4,12 @@
import soupmatchers
import transaction
from fixtures import FakeLogger
-from testtools.matchers import MatchesSetwise, MatchesStructure
+from testtools.matchers import (
+ Equals,
+ MatchesListwise,
+ MatchesSetwise,
+ MatchesStructure,
+)
from zope.component import getUtility
from lp.app.enums import InformationType
@@ -109,6 +114,48 @@ class TestDistributionPublisherConfigView(TestCaseWithFactory):
)
self._change_and_test_config()
+ def test_validate_absolute_root_dir(self):
+ form = {
+ "field.actions.save": "save",
+ "field.root_dir": "/srv/launchpad.test/distro-name",
+ "field.base_url": self.BASE_URL,
+ "field.copy_base_url": self.COPY_BASE_URL,
+ }
+ view = create_initialized_view(self.distro, name="+pubconf", form=form)
+ self.assertEqual([], view.errors)
+
+ def test_validate_relative_root_dir(self):
+ form = {
+ "field.actions.save": "save",
+ "field.root_dir": "distro-name",
+ "field.base_url": self.BASE_URL,
+ "field.copy_base_url": self.COPY_BASE_URL,
+ }
+ view = create_initialized_view(self.distro, name="+pubconf", form=form)
+ self.assertEqual([], view.errors)
+
+ def test_validate_relative_root_dir_no_dotdot(self):
+ form = {
+ "field.actions.save": "save",
+ "field.root_dir": "../distro-name",
+ "field.base_url": self.BASE_URL,
+ "field.copy_base_url": self.COPY_BASE_URL,
+ }
+ view = create_initialized_view(self.distro, name="+pubconf", form=form)
+ self.assertThat(
+ view.errors,
+ MatchesListwise(
+ [
+ MatchesStructure(
+ field_name=Equals("root_dir"),
+ errors=MatchesStructure.byEquality(
+ args=("Path would escape target directory",)
+ ),
+ )
+ ]
+ ),
+ )
+
class TestDistroAddView(TestCaseWithFactory):
"""Test the +add page for a new distribution."""
diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
index 28ce035..c5efae3 100644
--- a/lib/lp/services/config/schema-lazr.conf
+++ b/lib/lp/services/config/schema-lazr.conf
@@ -22,6 +22,10 @@ email_domain: answers.launchpad.net
# datatype: string
dbuser: archivepublisher
+# Base directory under which archives are stored. Relative paths in
+# PublisherConfig.root_dir are interpreted relative to this directory.
+archives_dir: /var/tmp/archives
+
# Location where the run-parts directories for publish-ftpmaster
# customization are to be found. Absolute path, or "none" to skip
# execution of run-parts.
