launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #24030
[Merge] ~cjwatson/launchpad:snap-check-request-private-git into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:snap-check-request-private-git into launchpad:master.
Commit message:
Check that the snap owner has suitable read access if building a snap from a private Git repository
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1639975 in Launchpad itself: "support for building private snaps"
https://bugs.launchpad.net/launchpad/+bug/1639975
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/373761
This is essentially the same as https://code.launchpad.net/~cjwatson/launchpad/snap-check-request-private-git/+merge/365071, converted to git and rebased on master.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:snap-check-request-private-git into launchpad:master.
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index 70626c9..d7af538 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -32,6 +32,7 @@ __all__ = [
'SnapBuildAlreadyPending',
'SnapBuildArchiveOwnerMismatch',
'SnapBuildDisallowedArchitecture',
+ 'SnapBuildGitRepositoryUnreadable',
'SnapBuildRequestStatus',
'SnapNotOwner',
'SnapPrivacyMismatch',
@@ -142,8 +143,12 @@ class SnapBuildArchiveOwnerMismatch(Forbidden):
that a malicious snap package build can't steal any secrets that its
owner didn't already have access to. Furthermore, we want to make sure
that future changes to the team owning the snap package don't grant it
- retrospective access to information about a private archive. For now,
- the simplest way to do this is to require that the owners match exactly.
+ retrospective access to information about a private archive.
+
+ For now, the simplest way to do this is to require that the owners match
+ exactly. We can change this in future when builders authenticate to
+ archives using a token limited to the lifetime of the build rather than
+ using a static secret.
"""
def __init__(self):
@@ -152,6 +157,27 @@ class SnapBuildArchiveOwnerMismatch(Forbidden):
"if the snap package owner and the archive owner are equal.")
+@error_status(httplib.FORBIDDEN)
+class SnapBuildGitRepositoryUnreadable(Forbidden):
+ """Builds against private Git repositories require appropriate read access.
+
+ The snap package owner must have a read access grant to the repository.
+ It isn't sufficient for just the build requester to have access, because
+ the build results will be visible to the whole team, and in any case
+ automatic builds don't have a distinct requester.
+
+ Since the token used by the builder to access the repository is only
+ valid while the build is running, we don't need to worry about snap
+ builds exposing tokens that might be usable after later ownership
+ changes.
+ """
+
+ def __init__(self):
+ super(SnapBuildGitRepositoryUnreadable, self).__init__(
+ "Snap package builds against private Git repositories are only "
+ "allowed if the repository is shared with the snap package owner.")
+
+
@error_status(httplib.BAD_REQUEST)
class SnapBuildDisallowedArchitecture(Exception):
"""A build was requested for a disallowed architecture."""
@@ -385,6 +411,9 @@ class ISnapView(Interface):
"Whether everything is set up to allow uploading builds of this "
"snap package to the store.")))
+ def checkRequestBuild(requester, archive):
+ """May `requester` request builds of this snap from `archive`?"""
+
@call_with(requester=REQUEST_USER)
@operation_parameters(
archive=Reference(schema=IArchive),
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 18eee6e..0903770 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -159,6 +159,7 @@ from lp.snappy.interfaces.snap import (
SnapBuildAlreadyPending,
SnapBuildArchiveOwnerMismatch,
SnapBuildDisallowedArchitecture,
+ SnapBuildGitRepositoryUnreadable,
SnapBuildRequestStatus,
SnapNotOwner,
SnapPrivacyMismatch,
@@ -636,8 +637,8 @@ class Snap(Storm, WebhookTargetMixin):
return False
return True
- def _checkRequestBuild(self, requester, archive):
- """May `requester` request builds of this snap from `archive`?"""
+ def checkRequestBuild(self, requester, archive):
+ """See `ISnap`."""
if not requester.inTeam(self.owner):
raise SnapNotOwner(
"%s cannot create snap package builds owned by %s." %
@@ -647,11 +648,15 @@ class Snap(Storm, WebhookTargetMixin):
if archive.private and self.owner != archive.owner:
# See rationale in `SnapBuildArchiveOwnerMismatch` docstring.
raise SnapBuildArchiveOwnerMismatch()
+ if (self.git_repository is not None and
+ not self.git_repository.visibleByUser(self.owner)):
+ # See rationale in `SnapBuildGitRepositoryUnreadable` docstring.
+ raise SnapBuildGitRepositoryUnreadable()
def requestBuild(self, requester, archive, distro_arch_series, pocket,
channels=None, build_request=None):
"""See `ISnap`."""
- self._checkRequestBuild(requester, archive)
+ self.checkRequestBuild(requester, archive)
if not self._isArchitectureAllowed(distro_arch_series, pocket):
raise SnapBuildDisallowedArchitecture(distro_arch_series, pocket)
@@ -681,7 +686,7 @@ class Snap(Storm, WebhookTargetMixin):
def requestBuilds(self, requester, archive, pocket, channels=None,
architectures=None):
"""See `ISnap`."""
- self._checkRequestBuild(requester, archive)
+ self.checkRequestBuild(requester, archive)
job = getUtility(ISnapRequestBuildsJobSource).create(
self, requester, archive, pocket, channels,
architectures=architectures)
diff --git a/lib/lp/snappy/model/snapbuildbehaviour.py b/lib/lp/snappy/model/snapbuildbehaviour.py
index cda3cff..8694fc1 100644
--- a/lib/lp/snappy/model/snapbuildbehaviour.py
+++ b/lib/lp/snappy/model/snapbuildbehaviour.py
@@ -39,15 +39,11 @@ from lp.services.config import config
from lp.services.features import getFeatureFlag
from lp.services.twistedsupport import cancel_on_timeout
from lp.services.twistedsupport.treq import check_status
-from lp.snappy.interfaces.snap import (
- SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG,
- SnapBuildArchiveOwnerMismatch,
- )
+from lp.snappy.interfaces.snap import SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG
from lp.snappy.interfaces.snapbuild import ISnapBuild
from lp.soyuz.adapters.archivedependencies import (
get_sources_list_for_building,
)
-from lp.soyuz.interfaces.archive import ArchiveDisabled
def format_as_rfc3339(timestamp):
@@ -83,6 +79,7 @@ class SnapBuildBehaviour(BuildFarmJobBehaviourBase):
The build request is checked:
* Virtualized builds can't build on a non-virtual builder
+ * The requester must be the snap owner or a member of that team
* The source archive may not be disabled
* If the source archive is private, the snap owner must match the
archive owner (see `SnapBuildArchiveOwnerMismatch` docstring)
@@ -93,10 +90,7 @@ class SnapBuildBehaviour(BuildFarmJobBehaviourBase):
raise AssertionError(
"Attempt to build virtual item on a non-virtual builder.")
- if not build.archive.enabled:
- raise ArchiveDisabled(build.archive.displayname)
- if build.archive.private and build.snap.owner != build.archive.owner:
- raise SnapBuildArchiveOwnerMismatch()
+ build.snap.checkRequestBuild(build.requester, build.archive)
chroot = build.distro_arch_series.getChroot(pocket=build.pocket)
if chroot is None:
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index b7d2d8a..d73375d 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -54,6 +54,11 @@ from lp.buildmaster.interfaces.buildqueue import IBuildQueue
from lp.buildmaster.interfaces.processor import IProcessorSet
from lp.buildmaster.model.buildfarmjob import BuildFarmJob
from lp.buildmaster.model.buildqueue import BuildQueue
+from lp.code.enums import (
+ BranchSubscriptionDiffSize,
+ BranchSubscriptionNotificationLevel,
+ CodeReviewNotificationLevel,
+ )
from lp.code.errors import (
BranchFileNotFound,
BranchHostingFault,
@@ -3519,6 +3524,97 @@ class TestSnapWebservice(TestCaseWithFactory):
"if the snap package owner and the archive owner are equal.",
response.body)
+ def test_requestBuild_git_private_owners_match(self):
+ # Build requests against a private Git repository are allowed if the
+ # Snap and GitRepository owners match exactly.
+ distroseries = self.factory.makeDistroSeries(
+ distribution=getUtility(IDistributionSet)['ubuntu'],
+ registrant=self.person)
+ processor = self.factory.makeProcessor(supports_virtualized=True)
+ distroarchseries = self.makeBuildableDistroArchSeries(
+ distroseries=distroseries, processor=processor, owner=self.person)
+ distroarchseries_url = api_url(distroarchseries)
+ archive_url = api_url(distroseries.main_archive)
+ [git_ref] = self.factory.makeGitRefs(owner=self.person)
+ snap = self.makeSnap(
+ distroseries=distroseries, processors=[processor], git_ref=git_ref)
+ with person_logged_in(self.person):
+ git_ref.repository.transitionToInformationType(
+ InformationType.PRIVATESECURITY, self.person)
+ transaction.commit()
+ private_webservice = webservice_for_person(
+ self.person, permission=OAuthPermission.WRITE_PRIVATE)
+ private_webservice.default_api_version = "devel"
+ response = private_webservice.named_post(
+ snap["self_link"], "requestBuild", archive=archive_url,
+ distro_arch_series=distroarchseries_url, pocket="Updates")
+ self.assertEqual(201, response.status)
+
+ def test_requestBuild_git_private_shared(self):
+ # Build requests against a private Git repository are allowed if the
+ # repository is shared with the Snap owner.
+ distroseries = self.factory.makeDistroSeries(
+ distribution=getUtility(IDistributionSet)['ubuntu'],
+ registrant=self.person)
+ processor = self.factory.makeProcessor(supports_virtualized=True)
+ distroarchseries = self.makeBuildableDistroArchSeries(
+ distroseries=distroseries, processor=processor, owner=self.person)
+ distroarchseries_url = api_url(distroarchseries)
+ archive_url = api_url(distroseries.main_archive)
+ [git_ref] = self.factory.makeGitRefs(owner=self.person)
+ snap = self.makeSnap(
+ distroseries=distroseries, processors=[processor], git_ref=git_ref)
+ with admin_logged_in() as admin:
+ git_ref.repository.setOwner(self.factory.makePerson(), admin)
+ git_ref.repository.transitionToInformationType(
+ InformationType.PRIVATESECURITY, admin)
+ git_ref.repository.subscribe(
+ self.person, BranchSubscriptionNotificationLevel.NOEMAIL,
+ BranchSubscriptionDiffSize.NODIFF,
+ CodeReviewNotificationLevel.NOEMAIL, admin)
+ self.assertTrue(git_ref.repository.visibleByUser(self.person))
+ transaction.commit()
+ private_webservice = webservice_for_person(
+ self.person, permission=OAuthPermission.WRITE_PRIVATE)
+ private_webservice.default_api_version = "devel"
+ response = private_webservice.named_post(
+ snap["self_link"], "requestBuild", archive=archive_url,
+ distro_arch_series=distroarchseries_url, pocket="Updates")
+ self.assertEqual(201, response.status)
+
+ def test_requestBuild_git_private_not_shared(self):
+ # Build requests against a private Git repository are rejected if
+ # the repository is not shared with the Snap owner.
+ distroseries = self.factory.makeDistroSeries(
+ distribution=getUtility(IDistributionSet)['ubuntu'],
+ registrant=self.person)
+ processor = self.factory.makeProcessor(supports_virtualized=True)
+ distroarchseries = self.makeBuildableDistroArchSeries(
+ distroseries=distroseries, processor=processor, owner=self.person)
+ distroarchseries_url = api_url(distroarchseries)
+ archive_url = api_url(distroseries.main_archive)
+ [git_ref] = self.factory.makeGitRefs(owner=self.person)
+ snap = self.makeSnap(
+ distroseries=distroseries, processors=[processor], git_ref=git_ref)
+ with admin_logged_in() as admin:
+ git_ref.repository.setOwner(self.factory.makePerson(), admin)
+ git_ref.repository.transitionToInformationType(
+ InformationType.PRIVATESECURITY, admin)
+ git_ref.repository.unsubscribe(self.person, admin)
+ self.assertFalse(git_ref.repository.visibleByUser(self.person))
+ transaction.commit()
+ private_webservice = webservice_for_person(
+ self.person, permission=OAuthPermission.WRITE_PRIVATE)
+ private_webservice.default_api_version = "devel"
+ response = private_webservice.named_post(
+ snap["self_link"], "requestBuild", archive=archive_url,
+ distro_arch_series=distroarchseries_url, pocket="Updates")
+ self.assertEqual(403, response.status)
+ self.assertEqual(
+ "Snap package builds against private Git repositories are only "
+ "allowed if the repository is shared with the snap package owner.",
+ response.body)
+
def test_requestBuilds(self):
# Requests for builds for all relevant architectures can be
# performed over the webservice, and the returned entry indicates
diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
index 3a506c5..ad6a3bc 100644
--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
@@ -304,7 +304,7 @@ class TestSnapBuildBehaviour(TestSnapBuildBehaviourBase):
def test_verifyBuildRequest_archive_private_owners_match(self):
archive = self.factory.makeArchive(private=True)
job = self.makeJob(
- archive=archive, registrant=archive.owner, owner=archive.owner)
+ archive=archive, requester=archive.owner, owner=archive.owner)
lfa = self.factory.makeLibraryFileAlias()
transaction.commit()
job.build.distro_arch_series.addOrUpdateChroot(lfa)
