Launchpad itself

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

  1. snap-check-request-private-git
  2. Merge into devel
Proposed by Colin Watson
Status: Rejected
Rejected by: Colin Watson
Proposed branch: lp:~cjwatson/launchpad/snap-check-request-private-git
Merge into: lp:launchpad
Prerequisite: lp:~cjwatson/launchpad/snap-build-behaviour-macaroon
Diff against target: 281 lines (+140/-16) (has conflicts)
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)
Text conflict in lib/lp/services/config/schema-lazr.conf
To merge this branch: bzr merge lp:~cjwatson/launchpad/snap-check-request-private-git
Reviewer Review Type Date Requested Status
Launchpad code reviewers Pending
Review via email: mp+365071@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
William Grant (wgrant) wrote :

The owner of a repository can't necessarily read it. Ownership grants edit if read is granted, but read may have been revoked from +sharing.

lp:~cjwatson/launchpad/snap-check-request-private-git updated
18646. 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.

18647. By Colin Watson

Refine description of SnapBuildArchiveOwnerMismatch.

18648. By Colin Watson

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

Revision history for this message
Colin Watson (cjwatson) wrote :

OK, I've rearranged this as we discussed on IRC today.

lp:~cjwatson/launchpad/snap-check-request-private-git updated
18649. By Colin Watson

Merge devel.

Revision history for this message
Colin Watson (cjwatson) wrote :

Superseded by https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/373761.

Unmerged revisions

18649. By Colin Watson

Merge devel.

18648. By Colin Watson

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

18647. By Colin Watson

Refine description of SnapBuildArchiveOwnerMismatch.

18646. 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.

18645. By Colin Watson

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

Preview Diff

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