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
=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py 2019-02-07 10:30:03 +0000
+++ lib/lp/snappy/interfaces/snap.py 2019-03-28 15:01:45 +0000
@@ -30,6 +30,7 @@
30 'SnapBuildAlreadyPending',30 'SnapBuildAlreadyPending',
31 'SnapBuildArchiveOwnerMismatch',31 'SnapBuildArchiveOwnerMismatch',
32 'SnapBuildDisallowedArchitecture',32 'SnapBuildDisallowedArchitecture',
33 'SnapBuildGitRepositoryUnreadable',
33 'SnapBuildRequestStatus',34 'SnapBuildRequestStatus',
34 'SnapNotOwner',35 'SnapNotOwner',
35 'SnapPrivacyMismatch',36 'SnapPrivacyMismatch',
@@ -139,8 +140,12 @@
139 that a malicious snap package build can't steal any secrets that its140 that a malicious snap package build can't steal any secrets that its
140 owner didn't already have access to. Furthermore, we want to make sure141 owner didn't already have access to. Furthermore, we want to make sure
141 that future changes to the team owning the snap package don't grant it142 that future changes to the team owning the snap package don't grant it
142 retrospective access to information about a private archive. For now,143 retrospective access to information about a private archive.
143 the simplest way to do this is to require that the owners match exactly.144
145 For now, the simplest way to do this is to require that the owners match
146 exactly. We can change this in future when builders authenticate to
147 archives using a token limited to the lifetime of the build rather than
148 using a static secret.
144 """149 """
145150
146 def __init__(self):151 def __init__(self):
@@ -149,6 +154,27 @@
149 "if the snap package owner and the archive owner are equal.")154 "if the snap package owner and the archive owner are equal.")
150155
151156
157@error_status(httplib.FORBIDDEN)
158class SnapBuildGitRepositoryUnreadable(Forbidden):
159 """Builds against private Git repositories require appropriate read access.
160
161 The snap package owner must have a read access grant to the repository.
162 It isn't sufficient for just the build requester to have access, because
163 the build results will be visible to the whole team, and in any case
164 automatic builds don't have a distinct requester.
165
166 Since the token used by the builder to access the repository is only
167 valid while the build is running, we don't need to worry about snap
168 builds exposing tokens that might be usable after later ownership
169 changes.
170 """
171
172 def __init__(self):
173 super(SnapBuildGitRepositoryUnreadable, self).__init__(
174 "Snap package builds against private Git repositories are only "
175 "allowed if the repository is shared with the snap package owner.")
176
177
152@error_status(httplib.BAD_REQUEST)178@error_status(httplib.BAD_REQUEST)
153class SnapBuildDisallowedArchitecture(Exception):179class SnapBuildDisallowedArchitecture(Exception):
154 """A build was requested for a disallowed architecture."""180 """A build was requested for a disallowed architecture."""
@@ -373,6 +399,9 @@
373 "Whether everything is set up to allow uploading builds of this "399 "Whether everything is set up to allow uploading builds of this "
374 "snap package to the store.")))400 "snap package to the store.")))
375401
402 def checkRequestBuild(requester, archive):
403 """May `requester` request builds of this snap from `archive`?"""
404
376 @call_with(requester=REQUEST_USER)405 @call_with(requester=REQUEST_USER)
377 @operation_parameters(406 @operation_parameters(
378 archive=Reference(schema=IArchive),407 archive=Reference(schema=IArchive),
379408
=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py 2019-02-26 06:47:45 +0000
+++ lib/lp/snappy/model/snap.py 2019-03-28 15:01:45 +0000
@@ -151,6 +151,7 @@
151 SnapBuildAlreadyPending,151 SnapBuildAlreadyPending,
152 SnapBuildArchiveOwnerMismatch,152 SnapBuildArchiveOwnerMismatch,
153 SnapBuildDisallowedArchitecture,153 SnapBuildDisallowedArchitecture,
154 SnapBuildGitRepositoryUnreadable,
154 SnapBuildRequestStatus,155 SnapBuildRequestStatus,
155 SnapNotOwner,156 SnapNotOwner,
156 SnapPrivacyMismatch,157 SnapPrivacyMismatch,
@@ -573,8 +574,8 @@
573 return False574 return False
574 return True575 return True
575576
576 def _checkRequestBuild(self, requester, archive):577 def checkRequestBuild(self, requester, archive):
577 """May `requester` request builds of this snap from `archive`?"""578 """See `ISnap`."""
578 if not requester.inTeam(self.owner):579 if not requester.inTeam(self.owner):
579 raise SnapNotOwner(580 raise SnapNotOwner(
580 "%s cannot create snap package builds owned by %s." %581 "%s cannot create snap package builds owned by %s." %
@@ -584,11 +585,15 @@
584 if archive.private and self.owner != archive.owner:585 if archive.private and self.owner != archive.owner:
585 # See rationale in `SnapBuildArchiveOwnerMismatch` docstring.586 # See rationale in `SnapBuildArchiveOwnerMismatch` docstring.
586 raise SnapBuildArchiveOwnerMismatch()587 raise SnapBuildArchiveOwnerMismatch()
588 if (self.git_repository is not None and
589 not self.git_repository.visibleByUser(self.owner)):
590 # See rationale in `SnapBuildGitRepositoryUnreadable` docstring.
591 raise SnapBuildGitRepositoryUnreadable()
587592
588 def requestBuild(self, requester, archive, distro_arch_series, pocket,593 def requestBuild(self, requester, archive, distro_arch_series, pocket,
589 channels=None, build_request=None):594 channels=None, build_request=None):
590 """See `ISnap`."""595 """See `ISnap`."""
591 self._checkRequestBuild(requester, archive)596 self.checkRequestBuild(requester, archive)
592 if not self._isArchitectureAllowed(distro_arch_series, pocket):597 if not self._isArchitectureAllowed(distro_arch_series, pocket):
593 raise SnapBuildDisallowedArchitecture(distro_arch_series, pocket)598 raise SnapBuildDisallowedArchitecture(distro_arch_series, pocket)
594599
@@ -612,7 +617,7 @@
612617
613 def requestBuilds(self, requester, archive, pocket, channels=None):618 def requestBuilds(self, requester, archive, pocket, channels=None):
614 """See `ISnap`."""619 """See `ISnap`."""
615 self._checkRequestBuild(requester, archive)620 self.checkRequestBuild(requester, archive)
616 job = getUtility(ISnapRequestBuildsJobSource).create(621 job = getUtility(ISnapRequestBuildsJobSource).create(
617 self, requester, archive, pocket, channels)622 self, requester, archive, pocket, channels)
618 return self.getBuildRequest(job.job_id)623 return self.getBuildRequest(job.job_id)
619624
=== modified file 'lib/lp/snappy/model/snapbuildbehaviour.py'
--- lib/lp/snappy/model/snapbuildbehaviour.py 2019-03-28 15:01:44 +0000
+++ lib/lp/snappy/model/snapbuildbehaviour.py 2019-03-28 15:01:45 +0000
@@ -37,15 +37,11 @@
37from lp.services.features import getFeatureFlag37from lp.services.features import getFeatureFlag
38from lp.services.twistedsupport import cancel_on_timeout38from lp.services.twistedsupport import cancel_on_timeout
39from lp.services.twistedsupport.treq import check_status39from lp.services.twistedsupport.treq import check_status
40from lp.snappy.interfaces.snap import (40from lp.snappy.interfaces.snap import SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG
41 SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG,
42 SnapBuildArchiveOwnerMismatch,
43 )
44from lp.snappy.interfaces.snapbuild import ISnapBuild41from lp.snappy.interfaces.snapbuild import ISnapBuild
45from lp.soyuz.adapters.archivedependencies import (42from lp.soyuz.adapters.archivedependencies import (
46 get_sources_list_for_building,43 get_sources_list_for_building,
47 )44 )
48from lp.soyuz.interfaces.archive import ArchiveDisabled
4945
5046
51@adapter(ISnapBuild)47@adapter(ISnapBuild)
@@ -70,6 +66,7 @@
7066
71 The build request is checked:67 The build request is checked:
72 * Virtualized builds can't build on a non-virtual builder68 * Virtualized builds can't build on a non-virtual builder
69 * The requester must be the snap owner or a member of that team
73 * The source archive may not be disabled70 * The source archive may not be disabled
74 * If the source archive is private, the snap owner must match the71 * If the source archive is private, the snap owner must match the
75 archive owner (see `SnapBuildArchiveOwnerMismatch` docstring)72 archive owner (see `SnapBuildArchiveOwnerMismatch` docstring)
@@ -80,10 +77,7 @@
80 raise AssertionError(77 raise AssertionError(
81 "Attempt to build virtual item on a non-virtual builder.")78 "Attempt to build virtual item on a non-virtual builder.")
8279
83 if not build.archive.enabled:80 build.snap.checkRequestBuild(build.requester, build.archive)
84 raise ArchiveDisabled(build.archive.displayname)
85 if build.archive.private and build.snap.owner != build.archive.owner:
86 raise SnapBuildArchiveOwnerMismatch()
8781
88 chroot = build.distro_arch_series.getChroot(pocket=build.pocket)82 chroot = build.distro_arch_series.getChroot(pocket=build.pocket)
89 if chroot is None:83 if chroot is None:
9084
=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py 2019-02-26 06:47:45 +0000
+++ lib/lp/snappy/tests/test_snap.py 2019-03-28 15:01:45 +0000
@@ -49,6 +49,11 @@
49from lp.buildmaster.interfaces.processor import IProcessorSet49from lp.buildmaster.interfaces.processor import IProcessorSet
50from lp.buildmaster.model.buildfarmjob import BuildFarmJob50from lp.buildmaster.model.buildfarmjob import BuildFarmJob
51from lp.buildmaster.model.buildqueue import BuildQueue51from lp.buildmaster.model.buildqueue import BuildQueue
52from lp.code.enums import (
53 BranchSubscriptionDiffSize,
54 BranchSubscriptionNotificationLevel,
55 CodeReviewNotificationLevel,
56 )
52from lp.code.errors import (57from lp.code.errors import (
53 BranchFileNotFound,58 BranchFileNotFound,
54 BranchHostingFault,59 BranchHostingFault,
@@ -3102,6 +3107,97 @@
3102 "if the snap package owner and the archive owner are equal.",3107 "if the snap package owner and the archive owner are equal.",
3103 response.body)3108 response.body)
31043109
3110 def test_requestBuild_git_private_owners_match(self):
3111 # Build requests against a private Git repository are allowed if the
3112 # Snap and GitRepository owners match exactly.
3113 distroseries = self.factory.makeDistroSeries(
3114 distribution=getUtility(IDistributionSet)['ubuntu'],
3115 registrant=self.person)
3116 processor = self.factory.makeProcessor(supports_virtualized=True)
3117 distroarchseries = self.makeBuildableDistroArchSeries(
3118 distroseries=distroseries, processor=processor, owner=self.person)
3119 distroarchseries_url = api_url(distroarchseries)
3120 archive_url = api_url(distroseries.main_archive)
3121 [git_ref] = self.factory.makeGitRefs(owner=self.person)
3122 snap = self.makeSnap(
3123 distroseries=distroseries, processors=[processor], git_ref=git_ref)
3124 with person_logged_in(self.person):
3125 git_ref.repository.transitionToInformationType(
3126 InformationType.PRIVATESECURITY, self.person)
3127 transaction.commit()
3128 private_webservice = webservice_for_person(
3129 self.person, permission=OAuthPermission.WRITE_PRIVATE)
3130 private_webservice.default_api_version = "devel"
3131 response = private_webservice.named_post(
3132 snap["self_link"], "requestBuild", archive=archive_url,
3133 distro_arch_series=distroarchseries_url, pocket="Updates")
3134 self.assertEqual(201, response.status)
3135
3136 def test_requestBuild_git_private_shared(self):
3137 # Build requests against a private Git repository are allowed if the
3138 # repository is shared with the Snap owner.
3139 distroseries = self.factory.makeDistroSeries(
3140 distribution=getUtility(IDistributionSet)['ubuntu'],
3141 registrant=self.person)
3142 processor = self.factory.makeProcessor(supports_virtualized=True)
3143 distroarchseries = self.makeBuildableDistroArchSeries(
3144 distroseries=distroseries, processor=processor, owner=self.person)
3145 distroarchseries_url = api_url(distroarchseries)
3146 archive_url = api_url(distroseries.main_archive)
3147 [git_ref] = self.factory.makeGitRefs(owner=self.person)
3148 snap = self.makeSnap(
3149 distroseries=distroseries, processors=[processor], git_ref=git_ref)
3150 with admin_logged_in() as admin:
3151 git_ref.repository.setOwner(self.factory.makePerson(), admin)
3152 git_ref.repository.transitionToInformationType(
3153 InformationType.PRIVATESECURITY, admin)
3154 git_ref.repository.subscribe(
3155 self.person, BranchSubscriptionNotificationLevel.NOEMAIL,
3156 BranchSubscriptionDiffSize.NODIFF,
3157 CodeReviewNotificationLevel.NOEMAIL, admin)
3158 self.assertTrue(git_ref.repository.visibleByUser(self.person))
3159 transaction.commit()
3160 private_webservice = webservice_for_person(
3161 self.person, permission=OAuthPermission.WRITE_PRIVATE)
3162 private_webservice.default_api_version = "devel"
3163 response = private_webservice.named_post(
3164 snap["self_link"], "requestBuild", archive=archive_url,
3165 distro_arch_series=distroarchseries_url, pocket="Updates")
3166 self.assertEqual(201, response.status)
3167
3168 def test_requestBuild_git_private_not_shared(self):
3169 # Build requests against a private Git repository are rejected if
3170 # the repository is not shared with the Snap owner.
3171 distroseries = self.factory.makeDistroSeries(
3172 distribution=getUtility(IDistributionSet)['ubuntu'],
3173 registrant=self.person)
3174 processor = self.factory.makeProcessor(supports_virtualized=True)
3175 distroarchseries = self.makeBuildableDistroArchSeries(
3176 distroseries=distroseries, processor=processor, owner=self.person)
3177 distroarchseries_url = api_url(distroarchseries)
3178 archive_url = api_url(distroseries.main_archive)
3179 [git_ref] = self.factory.makeGitRefs(owner=self.person)
3180 snap = self.makeSnap(
3181 distroseries=distroseries, processors=[processor], git_ref=git_ref)
3182 with admin_logged_in() as admin:
3183 git_ref.repository.setOwner(self.factory.makePerson(), admin)
3184 git_ref.repository.transitionToInformationType(
3185 InformationType.PRIVATESECURITY, admin)
3186 git_ref.repository.unsubscribe(self.person, admin)
3187 self.assertFalse(git_ref.repository.visibleByUser(self.person))
3188 transaction.commit()
3189 private_webservice = webservice_for_person(
3190 self.person, permission=OAuthPermission.WRITE_PRIVATE)
3191 private_webservice.default_api_version = "devel"
3192 response = private_webservice.named_post(
3193 snap["self_link"], "requestBuild", archive=archive_url,
3194 distro_arch_series=distroarchseries_url, pocket="Updates")
3195 self.assertEqual(403, response.status)
3196 self.assertEqual(
3197 "Snap package builds against private Git repositories are only "
3198 "allowed if the repository is shared with the snap package owner.",
3199 response.body)
3200
3105 def test_requestBuilds(self):3201 def test_requestBuilds(self):
3106 # Requests for builds for all relevant architectures can be3202 # Requests for builds for all relevant architectures can be
3107 # performed over the webservice, and the returned entry indicates3203 # performed over the webservice, and the returned entry indicates
31083204
=== modified file 'lib/lp/snappy/tests/test_snapbuildbehaviour.py'
--- lib/lp/snappy/tests/test_snapbuildbehaviour.py 2019-03-28 15:01:44 +0000
+++ lib/lp/snappy/tests/test_snapbuildbehaviour.py 2019-03-28 15:01:45 +0000
@@ -278,7 +278,7 @@
278 def test_verifyBuildRequest_archive_private_owners_match(self):278 def test_verifyBuildRequest_archive_private_owners_match(self):
279 archive = self.factory.makeArchive(private=True)279 archive = self.factory.makeArchive(private=True)
280 job = self.makeJob(280 job = self.makeJob(
281 archive=archive, registrant=archive.owner, owner=archive.owner)281 archive=archive, requester=archive.owner, owner=archive.owner)
282 lfa = self.factory.makeLibraryFileAlias()282 lfa = self.factory.makeLibraryFileAlias()
283 transaction.commit()283 transaction.commit()
284 job.build.distro_arch_series.addOrUpdateChroot(lfa)284 job.build.distro_arch_series.addOrUpdateChroot(lfa)