Launchpad itself

Merge ~cjwatson/launchpad:snap-check-request-private-git into launchpad:master

  1. Git
  2. lp:~cjwatson/launchpad
  3. snap-check-request-private-git
  4. Merge into master
Proposed by Colin Watson
Status: Needs review
Proposed branch: ~cjwatson/launchpad:snap-check-request-private-git
Merge into: launchpad:master
Diff against target: 282 lines (+140/-16)
5 files modified
lib/lp/snappy/interfaces/snap.py (+31/-2)
lib/lp/snappy/model/snap.py (+9/-4)
lib/lp/snappy/model/snapbuildbehaviour.py (+3/-9)
lib/lp/snappy/tests/test_snap.py (+96/-0)
lib/lp/snappy/tests/test_snapbuildbehaviour.py (+1/-1)
Reviewer Review Type Date Requested Status
Launchpad code reviewers Pending
Review via email: mp+373761@code.launchpad.net

Commit message

Check that the snap owner has suitable read access if building a snap from a private Git repository

Description of the change

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.

To post a comment you must log in.

Unmerged commits

95a703d... by Colin Watson

Check that the snap owner has read access to the repository, rather than that owners match.

32b29d9... by Colin Watson

Refine description of SnapBuildArchiveOwnerMismatch.

b33164d... by Colin Watson

Refactor duplication between Snap.requestBuild(s) and SnapBuildBehaviour.

It makes sense for these to perform the same checks. One thing becomes
slightly stricter as a result: if the requester was a member of the snap's
owner team when the build was requested, but was removed from that team
before the build was dispatched, then the dispatched build will now fail.
That seems arguably more correct.

5e6f64a... by Colin Watson

Ensure that owners match exactly if building a snap from a private Git repository.

LP: #1639975

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
2index 70626c9..d7af538 100644
3--- a/lib/lp/snappy/interfaces/snap.py
4+++ b/lib/lp/snappy/interfaces/snap.py
5@@ -32,6 +32,7 @@ __all__ = [
6 'SnapBuildAlreadyPending',
7 'SnapBuildArchiveOwnerMismatch',
8 'SnapBuildDisallowedArchitecture',
9+ 'SnapBuildGitRepositoryUnreadable',
10 'SnapBuildRequestStatus',
11 'SnapNotOwner',
12 'SnapPrivacyMismatch',
13@@ -142,8 +143,12 @@ class SnapBuildArchiveOwnerMismatch(Forbidden):
14 that a malicious snap package build can't steal any secrets that its
15 owner didn't already have access to. Furthermore, we want to make sure
16 that future changes to the team owning the snap package don't grant it
17- retrospective access to information about a private archive. For now,
18- the simplest way to do this is to require that the owners match exactly.
19+ retrospective access to information about a private archive.
20+
21+ For now, the simplest way to do this is to require that the owners match
22+ exactly. We can change this in future when builders authenticate to
23+ archives using a token limited to the lifetime of the build rather than
24+ using a static secret.
25 """
26
27 def __init__(self):
28@@ -152,6 +157,27 @@ class SnapBuildArchiveOwnerMismatch(Forbidden):
29 "if the snap package owner and the archive owner are equal.")
30
31
32+@error_status(httplib.FORBIDDEN)
33+class SnapBuildGitRepositoryUnreadable(Forbidden):
34+ """Builds against private Git repositories require appropriate read access.
35+
36+ The snap package owner must have a read access grant to the repository.
37+ It isn't sufficient for just the build requester to have access, because
38+ the build results will be visible to the whole team, and in any case
39+ automatic builds don't have a distinct requester.
40+
41+ Since the token used by the builder to access the repository is only
42+ valid while the build is running, we don't need to worry about snap
43+ builds exposing tokens that might be usable after later ownership
44+ changes.
45+ """
46+
47+ def __init__(self):
48+ super(SnapBuildGitRepositoryUnreadable, self).__init__(
49+ "Snap package builds against private Git repositories are only "
50+ "allowed if the repository is shared with the snap package owner.")
51+
52+
53 @error_status(httplib.BAD_REQUEST)
54 class SnapBuildDisallowedArchitecture(Exception):
55 """A build was requested for a disallowed architecture."""
56@@ -385,6 +411,9 @@ class ISnapView(Interface):
57 "Whether everything is set up to allow uploading builds of this "
58 "snap package to the store.")))
59
60+ def checkRequestBuild(requester, archive):
61+ """May `requester` request builds of this snap from `archive`?"""
62+
63 @call_with(requester=REQUEST_USER)
64 @operation_parameters(
65 archive=Reference(schema=IArchive),
66diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
67index 18eee6e..0903770 100644
68--- a/lib/lp/snappy/model/snap.py
69+++ b/lib/lp/snappy/model/snap.py
70@@ -159,6 +159,7 @@ from lp.snappy.interfaces.snap import (
71 SnapBuildAlreadyPending,
72 SnapBuildArchiveOwnerMismatch,
73 SnapBuildDisallowedArchitecture,
74+ SnapBuildGitRepositoryUnreadable,
75 SnapBuildRequestStatus,
76 SnapNotOwner,
77 SnapPrivacyMismatch,
78@@ -636,8 +637,8 @@ class Snap(Storm, WebhookTargetMixin):
79 return False
80 return True
81
82- def _checkRequestBuild(self, requester, archive):
83- """May `requester` request builds of this snap from `archive`?"""
84+ def checkRequestBuild(self, requester, archive):
85+ """See `ISnap`."""
86 if not requester.inTeam(self.owner):
87 raise SnapNotOwner(
88 "%s cannot create snap package builds owned by %s." %
89@@ -647,11 +648,15 @@ class Snap(Storm, WebhookTargetMixin):
90 if archive.private and self.owner != archive.owner:
91 # See rationale in `SnapBuildArchiveOwnerMismatch` docstring.
92 raise SnapBuildArchiveOwnerMismatch()
93+ if (self.git_repository is not None and
94+ not self.git_repository.visibleByUser(self.owner)):
95+ # See rationale in `SnapBuildGitRepositoryUnreadable` docstring.
96+ raise SnapBuildGitRepositoryUnreadable()
97
98 def requestBuild(self, requester, archive, distro_arch_series, pocket,
99 channels=None, build_request=None):
100 """See `ISnap`."""
101- self._checkRequestBuild(requester, archive)
102+ self.checkRequestBuild(requester, archive)
103 if not self._isArchitectureAllowed(distro_arch_series, pocket):
104 raise SnapBuildDisallowedArchitecture(distro_arch_series, pocket)
105
106@@ -681,7 +686,7 @@ class Snap(Storm, WebhookTargetMixin):
107 def requestBuilds(self, requester, archive, pocket, channels=None,
108 architectures=None):
109 """See `ISnap`."""
110- self._checkRequestBuild(requester, archive)
111+ self.checkRequestBuild(requester, archive)
112 job = getUtility(ISnapRequestBuildsJobSource).create(
113 self, requester, archive, pocket, channels,
114 architectures=architectures)
115diff --git a/lib/lp/snappy/model/snapbuildbehaviour.py b/lib/lp/snappy/model/snapbuildbehaviour.py
116index cda3cff..8694fc1 100644
117--- a/lib/lp/snappy/model/snapbuildbehaviour.py
118+++ b/lib/lp/snappy/model/snapbuildbehaviour.py
119@@ -39,15 +39,11 @@ from lp.services.config import config
120 from lp.services.features import getFeatureFlag
121 from lp.services.twistedsupport import cancel_on_timeout
122 from lp.services.twistedsupport.treq import check_status
123-from lp.snappy.interfaces.snap import (
124- SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG,
125- SnapBuildArchiveOwnerMismatch,
126- )
127+from lp.snappy.interfaces.snap import SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG
128 from lp.snappy.interfaces.snapbuild import ISnapBuild
129 from lp.soyuz.adapters.archivedependencies import (
130 get_sources_list_for_building,
131 )
132-from lp.soyuz.interfaces.archive import ArchiveDisabled
133
134
135 def format_as_rfc3339(timestamp):
136@@ -83,6 +79,7 @@ class SnapBuildBehaviour(BuildFarmJobBehaviourBase):
137
138 The build request is checked:
139 * Virtualized builds can't build on a non-virtual builder
140+ * The requester must be the snap owner or a member of that team
141 * The source archive may not be disabled
142 * If the source archive is private, the snap owner must match the
143 archive owner (see `SnapBuildArchiveOwnerMismatch` docstring)
144@@ -93,10 +90,7 @@ class SnapBuildBehaviour(BuildFarmJobBehaviourBase):
145 raise AssertionError(
146 "Attempt to build virtual item on a non-virtual builder.")
147
148- if not build.archive.enabled:
149- raise ArchiveDisabled(build.archive.displayname)
150- if build.archive.private and build.snap.owner != build.archive.owner:
151- raise SnapBuildArchiveOwnerMismatch()
152+ build.snap.checkRequestBuild(build.requester, build.archive)
153
154 chroot = build.distro_arch_series.getChroot(pocket=build.pocket)
155 if chroot is None:
156diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
157index b7d2d8a..d73375d 100644
158--- a/lib/lp/snappy/tests/test_snap.py
159+++ b/lib/lp/snappy/tests/test_snap.py
160@@ -54,6 +54,11 @@ from lp.buildmaster.interfaces.buildqueue import IBuildQueue
161 from lp.buildmaster.interfaces.processor import IProcessorSet
162 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
163 from lp.buildmaster.model.buildqueue import BuildQueue
164+from lp.code.enums import (
165+ BranchSubscriptionDiffSize,
166+ BranchSubscriptionNotificationLevel,
167+ CodeReviewNotificationLevel,
168+ )
169 from lp.code.errors import (
170 BranchFileNotFound,
171 BranchHostingFault,
172@@ -3519,6 +3524,97 @@ class TestSnapWebservice(TestCaseWithFactory):
173 "if the snap package owner and the archive owner are equal.",
174 response.body)
175
176+ def test_requestBuild_git_private_owners_match(self):
177+ # Build requests against a private Git repository are allowed if the
178+ # Snap and GitRepository owners match exactly.
179+ distroseries = self.factory.makeDistroSeries(
180+ distribution=getUtility(IDistributionSet)['ubuntu'],
181+ registrant=self.person)
182+ processor = self.factory.makeProcessor(supports_virtualized=True)
183+ distroarchseries = self.makeBuildableDistroArchSeries(
184+ distroseries=distroseries, processor=processor, owner=self.person)
185+ distroarchseries_url = api_url(distroarchseries)
186+ archive_url = api_url(distroseries.main_archive)
187+ [git_ref] = self.factory.makeGitRefs(owner=self.person)
188+ snap = self.makeSnap(
189+ distroseries=distroseries, processors=[processor], git_ref=git_ref)
190+ with person_logged_in(self.person):
191+ git_ref.repository.transitionToInformationType(
192+ InformationType.PRIVATESECURITY, self.person)
193+ transaction.commit()
194+ private_webservice = webservice_for_person(
195+ self.person, permission=OAuthPermission.WRITE_PRIVATE)
196+ private_webservice.default_api_version = "devel"
197+ response = private_webservice.named_post(
198+ snap["self_link"], "requestBuild", archive=archive_url,
199+ distro_arch_series=distroarchseries_url, pocket="Updates")
200+ self.assertEqual(201, response.status)
201+
202+ def test_requestBuild_git_private_shared(self):
203+ # Build requests against a private Git repository are allowed if the
204+ # repository is shared with the Snap owner.
205+ distroseries = self.factory.makeDistroSeries(
206+ distribution=getUtility(IDistributionSet)['ubuntu'],
207+ registrant=self.person)
208+ processor = self.factory.makeProcessor(supports_virtualized=True)
209+ distroarchseries = self.makeBuildableDistroArchSeries(
210+ distroseries=distroseries, processor=processor, owner=self.person)
211+ distroarchseries_url = api_url(distroarchseries)
212+ archive_url = api_url(distroseries.main_archive)
213+ [git_ref] = self.factory.makeGitRefs(owner=self.person)
214+ snap = self.makeSnap(
215+ distroseries=distroseries, processors=[processor], git_ref=git_ref)
216+ with admin_logged_in() as admin:
217+ git_ref.repository.setOwner(self.factory.makePerson(), admin)
218+ git_ref.repository.transitionToInformationType(
219+ InformationType.PRIVATESECURITY, admin)
220+ git_ref.repository.subscribe(
221+ self.person, BranchSubscriptionNotificationLevel.NOEMAIL,
222+ BranchSubscriptionDiffSize.NODIFF,
223+ CodeReviewNotificationLevel.NOEMAIL, admin)
224+ self.assertTrue(git_ref.repository.visibleByUser(self.person))
225+ transaction.commit()
226+ private_webservice = webservice_for_person(
227+ self.person, permission=OAuthPermission.WRITE_PRIVATE)
228+ private_webservice.default_api_version = "devel"
229+ response = private_webservice.named_post(
230+ snap["self_link"], "requestBuild", archive=archive_url,
231+ distro_arch_series=distroarchseries_url, pocket="Updates")
232+ self.assertEqual(201, response.status)
233+
234+ def test_requestBuild_git_private_not_shared(self):
235+ # Build requests against a private Git repository are rejected if
236+ # the repository is not shared with the Snap owner.
237+ distroseries = self.factory.makeDistroSeries(
238+ distribution=getUtility(IDistributionSet)['ubuntu'],
239+ registrant=self.person)
240+ processor = self.factory.makeProcessor(supports_virtualized=True)
241+ distroarchseries = self.makeBuildableDistroArchSeries(
242+ distroseries=distroseries, processor=processor, owner=self.person)
243+ distroarchseries_url = api_url(distroarchseries)
244+ archive_url = api_url(distroseries.main_archive)
245+ [git_ref] = self.factory.makeGitRefs(owner=self.person)
246+ snap = self.makeSnap(
247+ distroseries=distroseries, processors=[processor], git_ref=git_ref)
248+ with admin_logged_in() as admin:
249+ git_ref.repository.setOwner(self.factory.makePerson(), admin)
250+ git_ref.repository.transitionToInformationType(
251+ InformationType.PRIVATESECURITY, admin)
252+ git_ref.repository.unsubscribe(self.person, admin)
253+ self.assertFalse(git_ref.repository.visibleByUser(self.person))
254+ transaction.commit()
255+ private_webservice = webservice_for_person(
256+ self.person, permission=OAuthPermission.WRITE_PRIVATE)
257+ private_webservice.default_api_version = "devel"
258+ response = private_webservice.named_post(
259+ snap["self_link"], "requestBuild", archive=archive_url,
260+ distro_arch_series=distroarchseries_url, pocket="Updates")
261+ self.assertEqual(403, response.status)
262+ self.assertEqual(
263+ "Snap package builds against private Git repositories are only "
264+ "allowed if the repository is shared with the snap package owner.",
265+ response.body)
266+
267 def test_requestBuilds(self):
268 # Requests for builds for all relevant architectures can be
269 # performed over the webservice, and the returned entry indicates
270diff --git a/lib/lp/snappy/tests/test_snapbuildbehaviour.py b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
271index 3a506c5..ad6a3bc 100644
272--- a/lib/lp/snappy/tests/test_snapbuildbehaviour.py
273+++ b/lib/lp/snappy/tests/test_snapbuildbehaviour.py
274@@ -304,7 +304,7 @@ class TestSnapBuildBehaviour(TestSnapBuildBehaviourBase):
275 def test_verifyBuildRequest_archive_private_owners_match(self):
276 archive = self.factory.makeArchive(private=True)
277 job = self.makeJob(
278- archive=archive, registrant=archive.owner, owner=archive.owner)
279+ archive=archive, requester=archive.owner, owner=archive.owner)
280 lfa = self.factory.makeLibraryFileAlias()
281 transaction.commit()
282 job.build.distro_arch_series.addOrUpdateChroot(lfa)

Subscribers

People subscribed via source and target branches

to status/vote changes: