Merge lp:~cjwatson/launchpad/restore-git-snap-build-macaroon into lp:launchpad
- restore-git-snap-build-macaroon
- Merge into devel
Proposed by
Colin Watson
Status: | Merged |
---|---|
Merged at revision: | 18964 |
Proposed branch: | lp:~cjwatson/launchpad/restore-git-snap-build-macaroon |
Merge into: | lp:launchpad |
Prerequisite: | lp:~cjwatson/launchpad/refactor-git-code-import-authz-2 |
Diff against target: |
274 lines (+145/-3) 5 files modified
lib/lp/code/model/tests/test_codeimportjob.py (+2/-0) lib/lp/code/xmlrpc/git.py (+16/-2) lib/lp/code/xmlrpc/tests/test_git.py (+89/-1) lib/lp/snappy/model/snapbuild.py (+5/-0) lib/lp/snappy/tests/test_snapbuild.py (+33/-0) |
To merge this branch: | bzr merge lp:~cjwatson/launchpad/restore-git-snap-build-macaroon |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Grant | code | Approve | |
Review via email: mp+367204@code.launchpad.net |
Commit message
Make GitAPI accept snap-build macaroons again.
Description of the change
I added support for snap-build macaroons recently, and then promptly forgot about it when refactoring code import authorisation, largely because I hadn't explicitly written any tests for it.
In the process of writing these tests, I noticed that I really should have forced snap-build macaroons to be read-only, so I've done that. The mechanism for doing this is pretty ad-hoc, but we can always improve it later.
To post a comment you must log in.
Revision history for this message
William Grant (wgrant) : | # |
review:
Approve
(code)
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'lib/lp/code/model/tests/test_codeimportjob.py' | |||
2 | --- lib/lp/code/model/tests/test_codeimportjob.py 2019-04-26 13:13:37 +0000 | |||
3 | +++ lib/lp/code/model/tests/test_codeimportjob.py 2019-05-09 16:04:44 +0000 | |||
4 | @@ -1336,6 +1336,8 @@ | |||
5 | 1336 | getUtility(ICodeImportJobWorkflow).startJob(job, machine) | 1336 | getUtility(ICodeImportJobWorkflow).startJob(job, machine) |
6 | 1337 | macaroon = removeSecurityProxy(issuer).issueMacaroon(job) | 1337 | macaroon = removeSecurityProxy(issuer).issueMacaroon(job) |
7 | 1338 | self.assertMacaroonVerifies( | 1338 | self.assertMacaroonVerifies( |
8 | 1339 | issuer, macaroon, None, require_context=False) | ||
9 | 1340 | self.assertMacaroonVerifies( | ||
10 | 1339 | issuer, macaroon, job, require_context=False) | 1341 | issuer, macaroon, job, require_context=False) |
11 | 1340 | self.assertMacaroonVerifies( | 1342 | self.assertMacaroonVerifies( |
12 | 1341 | issuer, macaroon, job.code_import.target, require_context=False) | 1343 | issuer, macaroon, job.code_import.target, require_context=False) |
13 | 1342 | 1344 | ||
14 | === modified file 'lib/lp/code/xmlrpc/git.py' | |||
15 | --- lib/lp/code/xmlrpc/git.py 2019-05-09 16:04:44 +0000 | |||
16 | +++ lib/lp/code/xmlrpc/git.py 2019-05-09 16:04:44 +0000 | |||
17 | @@ -103,6 +103,17 @@ | |||
18 | 103 | 103 | ||
19 | 104 | :param verified: An `IMacaroonVerificationResult`. | 104 | :param verified: An `IMacaroonVerificationResult`. |
20 | 105 | """ | 105 | """ |
21 | 106 | return verified.issuer_name in ("code-import-job", "snap-build") | ||
22 | 107 | |||
23 | 108 | |||
24 | 109 | def _can_internal_issuer_write(verified): | ||
25 | 110 | """Does this internal-only issuer have write access? | ||
26 | 111 | |||
27 | 112 | Some macaroons used by internal services are intended for writing to the | ||
28 | 113 | repository; others only allow read access. | ||
29 | 114 | |||
30 | 115 | :param verified: An `IMacaroonVerificationResult`. | ||
31 | 116 | """ | ||
32 | 106 | return verified.issuer_name == "code-import-job" | 117 | return verified.issuer_name == "code-import-job" |
33 | 107 | 118 | ||
34 | 108 | 119 | ||
35 | @@ -162,7 +173,7 @@ | |||
36 | 162 | # so we can bypass other checks. This is only permitted for | 173 | # so we can bypass other checks. This is only permitted for |
37 | 163 | # selected macaroon issuers used by internal services. | 174 | # selected macaroon issuers used by internal services. |
38 | 164 | hosting_path = naked_repository.getInternalPath() | 175 | hosting_path = naked_repository.getInternalPath() |
40 | 165 | writable = True | 176 | writable = _can_internal_issuer_write(verified) |
41 | 166 | private = naked_repository.private | 177 | private = naked_repository.private |
42 | 167 | 178 | ||
43 | 168 | # In any other case, the macaroon constrains the permissions of | 179 | # In any other case, the macaroon constrains the permissions of |
44 | @@ -387,7 +398,7 @@ | |||
45 | 387 | verified = self._verifyMacaroon(password) | 398 | verified = self._verifyMacaroon(password) |
46 | 388 | if verified: | 399 | if verified: |
47 | 389 | auth_params = {"macaroon": password} | 400 | auth_params = {"macaroon": password} |
49 | 390 | if verified.issuer_name == "code-import-job": | 401 | if _is_issuer_internal(verified): |
50 | 391 | auth_params["user"] = LAUNCHPAD_SERVICES | 402 | auth_params["user"] = LAUNCHPAD_SERVICES |
51 | 392 | return auth_params | 403 | return auth_params |
52 | 393 | # Only macaroons are supported for password authentication. | 404 | # Only macaroons are supported for password authentication. |
53 | @@ -432,6 +443,9 @@ | |||
54 | 432 | raise faults.Unauthorized() | 443 | raise faults.Unauthorized() |
55 | 433 | 444 | ||
56 | 434 | if internal: | 445 | if internal: |
57 | 446 | if not _can_internal_issuer_write(verified): | ||
58 | 447 | raise faults.Unauthorized() | ||
59 | 448 | |||
60 | 435 | # We know that the authentication parameters | 449 | # We know that the authentication parameters |
61 | 436 | # specifically grant access to this repository because | 450 | # specifically grant access to this repository because |
62 | 437 | # we were able to verify the macaroon using the | 451 | # we were able to verify the macaroon using the |
63 | 438 | 452 | ||
64 | === modified file 'lib/lp/code/xmlrpc/tests/test_git.py' | |||
65 | --- lib/lp/code/xmlrpc/tests/test_git.py 2019-05-09 16:04:44 +0000 | |||
66 | +++ lib/lp/code/xmlrpc/tests/test_git.py 2019-05-09 16:04:44 +0000 | |||
67 | @@ -20,6 +20,7 @@ | |||
68 | 20 | from zope.security.proxy import removeSecurityProxy | 20 | from zope.security.proxy import removeSecurityProxy |
69 | 21 | 21 | ||
70 | 22 | from lp.app.enums import InformationType | 22 | from lp.app.enums import InformationType |
71 | 23 | from lp.buildmaster.enums import BuildStatus | ||
72 | 23 | from lp.code.enums import ( | 24 | from lp.code.enums import ( |
73 | 24 | GitGranteeType, | 25 | GitGranteeType, |
74 | 25 | GitRepositoryType, | 26 | GitRepositoryType, |
75 | @@ -38,8 +39,10 @@ | |||
76 | 38 | from lp.code.xmlrpc.git import GitAPI | 39 | from lp.code.xmlrpc.git import GitAPI |
77 | 39 | from lp.registry.enums import TeamMembershipPolicy | 40 | from lp.registry.enums import TeamMembershipPolicy |
78 | 40 | from lp.services.config import config | 41 | from lp.services.config import config |
79 | 42 | from lp.services.features.testing import FeatureFixture | ||
80 | 41 | from lp.services.macaroons.interfaces import IMacaroonIssuer | 43 | from lp.services.macaroons.interfaces import IMacaroonIssuer |
81 | 42 | from lp.services.webapp.escaping import html_escape | 44 | from lp.services.webapp.escaping import html_escape |
82 | 45 | from lp.snappy.interfaces.snap import SNAP_TESTING_FLAGS | ||
83 | 43 | from lp.testing import ( | 46 | from lp.testing import ( |
84 | 44 | admin_logged_in, | 47 | admin_logged_in, |
85 | 45 | ANONYMOUS, | 48 | ANONYMOUS, |
86 | @@ -1035,6 +1038,49 @@ | |||
87 | 1035 | code_imports[0].registrant, path, permission="write", | 1038 | code_imports[0].registrant, path, permission="write", |
88 | 1036 | macaroon_raw=macaroons[0].serialize()) | 1039 | macaroon_raw=macaroons[0].serialize()) |
89 | 1037 | 1040 | ||
90 | 1041 | def test_translatePath_private_snap_build(self): | ||
91 | 1042 | # A builder with a suitable macaroon can read from a repository | ||
92 | 1043 | # associated with a running private snap build. | ||
93 | 1044 | self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS)) | ||
94 | 1045 | self.pushConfig( | ||
95 | 1046 | "launchpad", internal_macaroon_secret_key="some-secret") | ||
96 | 1047 | with person_logged_in(self.factory.makePerson()) as owner: | ||
97 | 1048 | refs = [ | ||
98 | 1049 | self.factory.makeGitRefs( | ||
99 | 1050 | owner=owner, information_type=InformationType.USERDATA)[0] | ||
100 | 1051 | for _ in range(2)] | ||
101 | 1052 | builds = [ | ||
102 | 1053 | self.factory.makeSnapBuild( | ||
103 | 1054 | requester=owner, owner=owner, git_ref=ref, private=True) | ||
104 | 1055 | for ref in refs] | ||
105 | 1056 | issuer = getUtility(IMacaroonIssuer, "snap-build") | ||
106 | 1057 | macaroons = [ | ||
107 | 1058 | removeSecurityProxy(issuer).issueMacaroon(build) | ||
108 | 1059 | for build in builds] | ||
109 | 1060 | repository = refs[0].repository | ||
110 | 1061 | path = u"/%s" % repository.unique_name | ||
111 | 1062 | self.assertUnauthorized( | ||
112 | 1063 | LAUNCHPAD_SERVICES, path, permission="write", | ||
113 | 1064 | macaroon_raw=macaroons[0].serialize()) | ||
114 | 1065 | removeSecurityProxy(builds[0]).updateStatus(BuildStatus.BUILDING) | ||
115 | 1066 | self.assertTranslates( | ||
116 | 1067 | LAUNCHPAD_SERVICES, path, repository, False, permission="read", | ||
117 | 1068 | macaroon_raw=macaroons[0].serialize(), private=True) | ||
118 | 1069 | self.assertUnauthorized( | ||
119 | 1070 | LAUNCHPAD_SERVICES, path, permission="read", | ||
120 | 1071 | macaroon_raw=macaroons[1].serialize()) | ||
121 | 1072 | self.assertUnauthorized( | ||
122 | 1073 | LAUNCHPAD_SERVICES, path, permission="read", | ||
123 | 1074 | macaroon_raw=Macaroon( | ||
124 | 1075 | location=config.vhost.mainsite.hostname, identifier="another", | ||
125 | 1076 | key="another-secret").serialize()) | ||
126 | 1077 | self.assertUnauthorized( | ||
127 | 1078 | LAUNCHPAD_SERVICES, path, permission="read", | ||
128 | 1079 | macaroon_raw="nonsense") | ||
129 | 1080 | self.assertUnauthorized( | ||
130 | 1081 | repository.registrant, path, permission="read", | ||
131 | 1082 | macaroon_raw=macaroons[0].serialize()) | ||
132 | 1083 | |||
133 | 1038 | def test_notify(self): | 1084 | def test_notify(self): |
134 | 1039 | # The notify call creates a GitRefScanJob. | 1085 | # The notify call creates a GitRefScanJob. |
135 | 1040 | repository = self.factory.makeGitRepository() | 1086 | repository = self.factory.makeGitRepository() |
136 | @@ -1088,6 +1134,29 @@ | |||
137 | 1088 | self.git_api.authenticateWithPassword("", "nonsense"), | 1134 | self.git_api.authenticateWithPassword("", "nonsense"), |
138 | 1089 | faults.Unauthorized) | 1135 | faults.Unauthorized) |
139 | 1090 | 1136 | ||
140 | 1137 | def test_authenticateWithPassword_private_snap_build(self): | ||
141 | 1138 | self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS)) | ||
142 | 1139 | self.pushConfig( | ||
143 | 1140 | "launchpad", internal_macaroon_secret_key="some-secret") | ||
144 | 1141 | with person_logged_in(self.factory.makePerson()) as owner: | ||
145 | 1142 | [ref] = self.factory.makeGitRefs( | ||
146 | 1143 | owner=owner, information_type=InformationType.USERDATA) | ||
147 | 1144 | build = self.factory.makeSnapBuild( | ||
148 | 1145 | requester=owner, owner=owner, git_ref=ref, private=True) | ||
149 | 1146 | issuer = getUtility(IMacaroonIssuer, "snap-build") | ||
150 | 1147 | macaroon = removeSecurityProxy(issuer).issueMacaroon(build) | ||
151 | 1148 | self.assertEqual( | ||
152 | 1149 | {"macaroon": macaroon.serialize(), "user": "+launchpad-services"}, | ||
153 | 1150 | self.git_api.authenticateWithPassword("", macaroon.serialize())) | ||
154 | 1151 | other_macaroon = Macaroon(identifier="another", key="another-secret") | ||
155 | 1152 | self.assertIsInstance( | ||
156 | 1153 | self.git_api.authenticateWithPassword( | ||
157 | 1154 | "", other_macaroon.serialize()), | ||
158 | 1155 | faults.Unauthorized) | ||
159 | 1156 | self.assertIsInstance( | ||
160 | 1157 | self.git_api.authenticateWithPassword("", "nonsense"), | ||
161 | 1158 | faults.Unauthorized) | ||
162 | 1159 | |||
163 | 1091 | def test_checkRefPermissions_code_import(self): | 1160 | def test_checkRefPermissions_code_import(self): |
164 | 1092 | # A code import worker with a suitable macaroon has repository owner | 1161 | # A code import worker with a suitable macaroon has repository owner |
165 | 1093 | # privileges on a repository associated with a running code import | 1162 | # privileges on a repository associated with a running code import |
166 | @@ -1157,7 +1226,7 @@ | |||
167 | 1157 | ref_path = "refs/heads/master" | 1226 | ref_path = "refs/heads/master" |
168 | 1158 | self.assertHasRefPermissions( | 1227 | self.assertHasRefPermissions( |
169 | 1159 | LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []}, | 1228 | LAUNCHPAD_SERVICES, repository, [ref_path], {ref_path: []}, |
171 | 1160 | macaroon_raw=macaroons[0]) | 1229 | macaroon_raw=macaroons[0].serialize()) |
172 | 1161 | with celebrity_logged_in("vcs_imports"): | 1230 | with celebrity_logged_in("vcs_imports"): |
173 | 1162 | getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine) | 1231 | getUtility(ICodeImportJobWorkflow).startJob(jobs[0], machine) |
174 | 1163 | self.assertHasRefPermissions( | 1232 | self.assertHasRefPermissions( |
175 | @@ -1179,6 +1248,25 @@ | |||
176 | 1179 | code_imports[0].registrant, repository, [ref_path], {ref_path: []}, | 1248 | code_imports[0].registrant, repository, [ref_path], {ref_path: []}, |
177 | 1180 | macaroon_raw=macaroons[0].serialize()) | 1249 | macaroon_raw=macaroons[0].serialize()) |
178 | 1181 | 1250 | ||
179 | 1251 | def test_checkRefPermissions_private_snap_build(self): | ||
180 | 1252 | # A builder with a suitable macaroon cannot write to a repository, | ||
181 | 1253 | # even if it is associated with a running private snap build. | ||
182 | 1254 | self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS)) | ||
183 | 1255 | self.pushConfig( | ||
184 | 1256 | "launchpad", internal_macaroon_secret_key="some-secret") | ||
185 | 1257 | with person_logged_in(self.factory.makePerson()) as owner: | ||
186 | 1258 | [ref] = self.factory.makeGitRefs( | ||
187 | 1259 | owner=owner, information_type=InformationType.USERDATA) | ||
188 | 1260 | build = self.factory.makeSnapBuild( | ||
189 | 1261 | requester=owner, owner=owner, git_ref=ref, private=True) | ||
190 | 1262 | issuer = getUtility(IMacaroonIssuer, "snap-build") | ||
191 | 1263 | macaroon = removeSecurityProxy(issuer).issueMacaroon(build) | ||
192 | 1264 | build.updateStatus(BuildStatus.BUILDING) | ||
193 | 1265 | path = ref.path.encode("UTF-8") | ||
194 | 1266 | self.assertHasRefPermissions( | ||
195 | 1267 | LAUNCHPAD_SERVICES, ref.repository, [path], {path: []}, | ||
196 | 1268 | macaroon_raw=macaroon.serialize()) | ||
197 | 1269 | |||
198 | 1182 | 1270 | ||
199 | 1183 | class TestGitAPISecurity(TestGitAPIMixin, TestCaseWithFactory): | 1271 | class TestGitAPISecurity(TestGitAPIMixin, TestCaseWithFactory): |
200 | 1184 | """Slow tests for `IGitAPI`. | 1272 | """Slow tests for `IGitAPI`. |
201 | 1185 | 1273 | ||
202 | === modified file 'lib/lp/snappy/model/snapbuild.py' | |||
203 | --- lib/lp/snappy/model/snapbuild.py 2019-05-01 15:47:37 +0000 | |||
204 | +++ lib/lp/snappy/model/snapbuild.py 2019-05-09 16:04:44 +0000 | |||
205 | @@ -628,6 +628,11 @@ | |||
206 | 628 | # Circular import. | 628 | # Circular import. |
207 | 629 | from lp.snappy.model.snap import Snap | 629 | from lp.snappy.model.snap import Snap |
208 | 630 | 630 | ||
209 | 631 | if context is None: | ||
210 | 632 | # We're only verifying that the macaroon could be valid for some | ||
211 | 633 | # context. | ||
212 | 634 | return True | ||
213 | 635 | |||
214 | 631 | try: | 636 | try: |
215 | 632 | build_id = int(caveat_value) | 637 | build_id = int(caveat_value) |
216 | 633 | except ValueError: | 638 | except ValueError: |
217 | 634 | 639 | ||
218 | === modified file 'lib/lp/snappy/tests/test_snapbuild.py' | |||
219 | --- lib/lp/snappy/tests/test_snapbuild.py 2019-04-25 21:57:30 +0000 | |||
220 | +++ lib/lp/snappy/tests/test_snapbuild.py 2019-05-09 16:04:44 +0000 | |||
221 | @@ -843,6 +843,33 @@ | |||
222 | 843 | macaroon = issuer.issueMacaroon(build) | 843 | macaroon = issuer.issueMacaroon(build) |
223 | 844 | self.assertMacaroonVerifies(issuer, macaroon, ref.repository) | 844 | self.assertMacaroonVerifies(issuer, macaroon, ref.repository) |
224 | 845 | 845 | ||
225 | 846 | def test_verifyMacaroon_good_no_context(self): | ||
226 | 847 | [ref] = self.factory.makeGitRefs( | ||
227 | 848 | information_type=InformationType.USERDATA) | ||
228 | 849 | build = self.factory.makeSnapBuild( | ||
229 | 850 | snap=self.factory.makeSnap(git_ref=ref, private=True)) | ||
230 | 851 | build.updateStatus(BuildStatus.BUILDING) | ||
231 | 852 | issuer = removeSecurityProxy( | ||
232 | 853 | getUtility(IMacaroonIssuer, "snap-build")) | ||
233 | 854 | macaroon = issuer.issueMacaroon(build) | ||
234 | 855 | self.assertMacaroonVerifies( | ||
235 | 856 | issuer, macaroon, None, require_context=False) | ||
236 | 857 | self.assertMacaroonVerifies( | ||
237 | 858 | issuer, macaroon, ref.repository, require_context=False) | ||
238 | 859 | |||
239 | 860 | def test_verifyMacaroon_no_context_but_require_context(self): | ||
240 | 861 | [ref] = self.factory.makeGitRefs( | ||
241 | 862 | information_type=InformationType.USERDATA) | ||
242 | 863 | build = self.factory.makeSnapBuild( | ||
243 | 864 | snap=self.factory.makeSnap(git_ref=ref, private=True)) | ||
244 | 865 | build.updateStatus(BuildStatus.BUILDING) | ||
245 | 866 | issuer = removeSecurityProxy( | ||
246 | 867 | getUtility(IMacaroonIssuer, "snap-build")) | ||
247 | 868 | macaroon = issuer.issueMacaroon(build) | ||
248 | 869 | self.assertMacaroonDoesNotVerify( | ||
249 | 870 | ["Expected macaroon verification context but got None."], | ||
250 | 871 | issuer, macaroon, None) | ||
251 | 872 | |||
252 | 846 | def test_verifyMacaroon_wrong_location(self): | 873 | def test_verifyMacaroon_wrong_location(self): |
253 | 847 | [ref] = self.factory.makeGitRefs( | 874 | [ref] = self.factory.makeGitRefs( |
254 | 848 | information_type=InformationType.USERDATA) | 875 | information_type=InformationType.USERDATA) |
255 | @@ -856,6 +883,9 @@ | |||
256 | 856 | self.assertMacaroonDoesNotVerify( | 883 | self.assertMacaroonDoesNotVerify( |
257 | 857 | ["Macaroon has unknown location 'another-location'."], | 884 | ["Macaroon has unknown location 'another-location'."], |
258 | 858 | issuer, macaroon, ref.repository) | 885 | issuer, macaroon, ref.repository) |
259 | 886 | self.assertMacaroonDoesNotVerify( | ||
260 | 887 | ["Macaroon has unknown location 'another-location'."], | ||
261 | 888 | issuer, macaroon, ref.repository, require_context=False) | ||
262 | 859 | 889 | ||
263 | 860 | def test_verifyMacaroon_wrong_key(self): | 890 | def test_verifyMacaroon_wrong_key(self): |
264 | 861 | [ref] = self.factory.makeGitRefs( | 891 | [ref] = self.factory.makeGitRefs( |
265 | @@ -869,6 +899,9 @@ | |||
266 | 869 | location=config.vhost.mainsite.hostname, key="another-secret") | 899 | location=config.vhost.mainsite.hostname, key="another-secret") |
267 | 870 | self.assertMacaroonDoesNotVerify( | 900 | self.assertMacaroonDoesNotVerify( |
268 | 871 | ["Signatures do not match"], issuer, macaroon, ref.repository) | 901 | ["Signatures do not match"], issuer, macaroon, ref.repository) |
269 | 902 | self.assertMacaroonDoesNotVerify( | ||
270 | 903 | ["Signatures do not match"], | ||
271 | 904 | issuer, macaroon, ref.repository, require_context=False) | ||
272 | 872 | 905 | ||
273 | 873 | def test_verifyMacaroon_refuses_branch(self): | 906 | def test_verifyMacaroon_refuses_branch(self): |
274 | 874 | branch = self.factory.makeAnyBranch( | 907 | branch = self.factory.makeAnyBranch( |