Merge lp:~cjwatson/launchpad/git-honour-access-tokens into lp:launchpad
- git-honour-access-tokens
- Merge into devel
Proposed by
Colin Watson
Status: | Merged |
---|---|
Merged at revision: | 19049 |
Proposed branch: | lp:~cjwatson/launchpad/git-honour-access-tokens |
Merge into: | lp:launchpad |
Prerequisite: | lp:~cjwatson/launchpad/git-issue-access-tokens |
Diff against target: |
249 lines (+145/-14) 3 files modified
lib/lp/code/model/codeimportjob.py (+4/-0) lib/lp/code/xmlrpc/git.py (+25/-14) lib/lp/code/xmlrpc/tests/test_git.py (+116/-0) |
To merge this branch: | bzr merge lp:~cjwatson/launchpad/git-honour-access-tokens |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Grant | code | Approve | |
Review via email: mp+366053@code.launchpad.net |
Commit message
Make the Git XML-RPC API honour user macaroons.
Description of the change
To post a comment you must log in.
Revision history for this message
William Grant (wgrant) : | # |
Revision history for this message
Colin Watson (cjwatson) : | # |
Revision history for this message
Colin Watson (cjwatson) wrote : | # |
Revision history for this message
William Grant (wgrant) : | # |
review:
Needs Fixing
(code)
Revision history for this message
Colin Watson (cjwatson) : | # |
Revision history for this message
Colin Watson (cjwatson) : | # |
Revision history for this message
William Grant (wgrant) wrote : | # |
In _verifyAuthParams's "User authentication with no macaroon. We do no additional checks here." case it's very unclear that some other kind of authentication has already taken place, and verifyAuthParams isn't in fact the thing that's verifying the auth. Otherwise I think this is all safe enough -- thanks.
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/codeimportjob.py' |
2 | --- lib/lp/code/model/codeimportjob.py 2019-05-01 15:47:37 +0000 |
3 | +++ lib/lp/code/model/codeimportjob.py 2019-05-10 16:51:16 +0000 |
4 | @@ -463,6 +463,10 @@ |
5 | |
6 | def verifyPrimaryCaveat(self, caveat_value, context, **kwargs): |
7 | """See `MacaroonIssuerBase`.""" |
8 | + # Code import jobs only support free-floating macaroons for Git |
9 | + # authentication, not ones bound to a user. |
10 | + if kwargs.get("user"): |
11 | + return False |
12 | if context is None: |
13 | # We're only verifying that the macaroon could be valid for some |
14 | # context. |
15 | |
16 | === modified file 'lib/lp/code/xmlrpc/git.py' |
17 | --- lib/lp/code/xmlrpc/git.py 2019-05-09 15:44:59 +0000 |
18 | +++ lib/lp/code/xmlrpc/git.py 2019-05-10 16:51:16 +0000 |
19 | @@ -60,7 +60,10 @@ |
20 | InvalidName, |
21 | NoSuchSourcePackageName, |
22 | ) |
23 | -from lp.registry.interfaces.person import NoSuchPerson |
24 | +from lp.registry.interfaces.person import ( |
25 | + IPersonSet, |
26 | + NoSuchPerson, |
27 | + ) |
28 | from lp.registry.interfaces.product import ( |
29 | InvalidProductName, |
30 | NoSuchProduct, |
31 | @@ -125,7 +128,7 @@ |
32 | super(GitAPI, self).__init__(*args, **kwargs) |
33 | self.repository_set = getUtility(IGitRepositorySet) |
34 | |
35 | - def _verifyMacaroon(self, macaroon_raw, repository=None): |
36 | + def _verifyMacaroon(self, macaroon_raw, repository=None, user=None): |
37 | try: |
38 | macaroon = Macaroon.deserialize(macaroon_raw) |
39 | # XXX cjwatson 2019-04-23: Restrict exceptions once |
40 | @@ -137,7 +140,7 @@ |
41 | except ComponentLookupError: |
42 | return False |
43 | return issuer.verifyMacaroon( |
44 | - macaroon, repository, require_context=False) |
45 | + macaroon, repository, require_context=False, user=user) |
46 | |
47 | def _performLookup(self, requester, path, auth_params): |
48 | repository, extra_path = getUtility(IGitLookup).getByPath(path) |
49 | @@ -149,7 +152,10 @@ |
50 | writable = None |
51 | |
52 | if macaroon_raw is not None: |
53 | - verified = self._verifyMacaroon(macaroon_raw, naked_repository) |
54 | + verify_user = ( |
55 | + None if requester == LAUNCHPAD_SERVICES else requester) |
56 | + verified = self._verifyMacaroon( |
57 | + macaroon_raw, naked_repository, user=verify_user) |
58 | if not verified: |
59 | # Macaroon authentication failed. Don't fall back to the |
60 | # requester's permissions, since macaroons typically have |
61 | @@ -201,6 +207,7 @@ |
62 | if not grants.is_empty(): |
63 | writable = True |
64 | private = repository.private |
65 | + |
66 | return { |
67 | "path": hosting_path, |
68 | "writable": writable, |
69 | @@ -390,17 +397,18 @@ |
70 | getUtility(IGitRefScanJobSource).create( |
71 | removeSecurityProxy(repository)) |
72 | |
73 | + @return_fault |
74 | def authenticateWithPassword(self, username, password): |
75 | """See `IGitAPI`.""" |
76 | - # XXX cjwatson 2016-10-06: We only support free-floating macaroons |
77 | - # at the moment, not ones bound to a user. |
78 | - if not username: |
79 | - verified = self._verifyMacaroon(password) |
80 | - if verified: |
81 | - auth_params = {"macaroon": password} |
82 | - if _is_issuer_internal(verified): |
83 | - auth_params["user"] = LAUNCHPAD_SERVICES |
84 | - return auth_params |
85 | + user = getUtility(IPersonSet).getByName(username) if username else None |
86 | + verified = self._verifyMacaroon(password, user=user) |
87 | + if verified: |
88 | + auth_params = {"macaroon": password} |
89 | + if user is not None: |
90 | + auth_params["uid"] = user.id |
91 | + elif _is_issuer_internal(verified): |
92 | + auth_params["user"] = LAUNCHPAD_SERVICES |
93 | + return auth_params |
94 | # Only macaroons are supported for password authentication. |
95 | return faults.Unauthorized() |
96 | |
97 | @@ -428,7 +436,10 @@ |
98 | try: |
99 | macaroon_raw = auth_params.get("macaroon") |
100 | if macaroon_raw is not None: |
101 | - verified = self._verifyMacaroon(macaroon_raw, repository) |
102 | + verify_user = ( |
103 | + None if requester == LAUNCHPAD_SERVICES else requester) |
104 | + verified = self._verifyMacaroon( |
105 | + macaroon_raw, repository, user=verify_user) |
106 | if not verified: |
107 | # Macaroon authentication failed. Don't fall back to |
108 | # the requester's permissions, since macaroons typically |
109 | |
110 | === modified file 'lib/lp/code/xmlrpc/tests/test_git.py' |
111 | --- lib/lp/code/xmlrpc/tests/test_git.py 2019-05-09 15:44:59 +0000 |
112 | +++ lib/lp/code/xmlrpc/tests/test_git.py 2019-05-10 16:51:16 +0000 |
113 | @@ -1081,6 +1081,46 @@ |
114 | repository.registrant, path, permission="read", |
115 | macaroon_raw=macaroons[0].serialize()) |
116 | |
117 | + def test_translatePath_user_macaroon(self): |
118 | + # A user with a suitable macaroon can write to the corresponding |
119 | + # repository, but not others, even if they own them. |
120 | + self.pushConfig("codehosting", git_macaroon_secret_key="some-secret") |
121 | + requester = self.factory.makePerson() |
122 | + repositories = [ |
123 | + self.factory.makeGitRepository(owner=requester) for _ in range(2)] |
124 | + repositories.append(self.factory.makeGitRepository( |
125 | + owner=requester, information_type=InformationType.PRIVATESECURITY)) |
126 | + issuer = getUtility(IMacaroonIssuer, "git-repository") |
127 | + with person_logged_in(requester): |
128 | + macaroons = [ |
129 | + removeSecurityProxy(issuer).issueMacaroon( |
130 | + repository, user=requester) |
131 | + for repository in repositories] |
132 | + paths = [ |
133 | + u"/%s" % repository.unique_name for repository in repositories] |
134 | + for i, repository in enumerate(repositories): |
135 | + for j, macaroon in enumerate(macaroons): |
136 | + login(ANONYMOUS) |
137 | + if i == j: |
138 | + self.assertTranslates( |
139 | + requester, paths[i], repository, True, |
140 | + permission="write", macaroon_raw=macaroon.serialize(), |
141 | + private=(i == 2)) |
142 | + else: |
143 | + self.assertUnauthorized( |
144 | + requester, paths[i], permission="write", |
145 | + macaroon_raw=macaroon.serialize()) |
146 | + login(ANONYMOUS) |
147 | + self.assertUnauthorized( |
148 | + requester, paths[i], permission="write", |
149 | + macaroon_raw=Macaroon( |
150 | + location=config.vhost.mainsite.hostname, |
151 | + identifier="another", key="another-secret").serialize()) |
152 | + login(ANONYMOUS) |
153 | + self.assertUnauthorized( |
154 | + requester, paths[i], permission="write", |
155 | + macaroon_raw="nonsense") |
156 | + |
157 | def test_notify(self): |
158 | # The notify call creates a GitRefScanJob. |
159 | repository = self.factory.makeGitRepository() |
160 | @@ -1157,6 +1197,35 @@ |
161 | self.git_api.authenticateWithPassword("", "nonsense"), |
162 | faults.Unauthorized) |
163 | |
164 | + def test_authenticateWithPassword_user_macaroon(self): |
165 | + # A user with a suitable macaroon can authenticate using it, in |
166 | + # which case we return both the macaroon and the uid for use by |
167 | + # later calls. |
168 | + self.pushConfig("codehosting", git_macaroon_secret_key="some-secret") |
169 | + requester = self.factory.makePerson() |
170 | + issuer = getUtility(IMacaroonIssuer, "git-repository") |
171 | + macaroon = removeSecurityProxy(issuer).issueMacaroon( |
172 | + self.factory.makeGitRepository(owner=requester), user=requester) |
173 | + self.assertEqual( |
174 | + {"macaroon": macaroon.serialize(), "uid": requester.id}, |
175 | + self.git_api.authenticateWithPassword( |
176 | + requester.name, macaroon.serialize())) |
177 | + self.assertIsInstance( |
178 | + self.git_api.authenticateWithPassword("", macaroon.serialize()), |
179 | + faults.Unauthorized) |
180 | + self.assertIsInstance( |
181 | + self.git_api.authenticateWithPassword( |
182 | + "nonexistent", macaroon.serialize()), |
183 | + faults.Unauthorized) |
184 | + other_macaroon = Macaroon(identifier="another", key="another-secret") |
185 | + self.assertIsInstance( |
186 | + self.git_api.authenticateWithPassword( |
187 | + requester.name, other_macaroon.serialize()), |
188 | + faults.Unauthorized) |
189 | + self.assertIsInstance( |
190 | + self.git_api.authenticateWithPassword(requester.name, "nonsense"), |
191 | + faults.Unauthorized) |
192 | + |
193 | def test_checkRefPermissions_code_import(self): |
194 | # A code import worker with a suitable macaroon has repository owner |
195 | # privileges on a repository associated with a running code import |
196 | @@ -1267,6 +1336,53 @@ |
197 | LAUNCHPAD_SERVICES, ref.repository, [path], {path: []}, |
198 | macaroon_raw=macaroon.serialize()) |
199 | |
200 | + def test_checkRefPermissions_user_macaroon(self): |
201 | + # A user with a suitable macaroon has their ordinary privileges on |
202 | + # the corresponding repository, but not others, even if they own |
203 | + # them. |
204 | + self.pushConfig("codehosting", git_macaroon_secret_key="some-secret") |
205 | + requester = self.factory.makePerson() |
206 | + repositories = [ |
207 | + self.factory.makeGitRepository(owner=requester) for _ in range(2)] |
208 | + repositories.append(self.factory.makeGitRepository( |
209 | + owner=requester, information_type=InformationType.PRIVATESECURITY)) |
210 | + ref_path = b"refs/heads/master" |
211 | + issuer = getUtility(IMacaroonIssuer, "git-repository") |
212 | + with person_logged_in(requester): |
213 | + macaroons = [ |
214 | + removeSecurityProxy(issuer).issueMacaroon( |
215 | + repository, user=requester) |
216 | + for repository in repositories] |
217 | + for i, repository in enumerate(repositories): |
218 | + for j, macaroon in enumerate(macaroons): |
219 | + login(ANONYMOUS) |
220 | + if i == j: |
221 | + expected_permissions = ["create", "push", "force_push"] |
222 | + else: |
223 | + expected_permissions = [] |
224 | + self.assertHasRefPermissions( |
225 | + requester, repository, [ref_path], |
226 | + {ref_path: expected_permissions}, |
227 | + macaroon_raw=macaroon.serialize()) |
228 | + login(ANONYMOUS) |
229 | + self.assertHasRefPermissions( |
230 | + None, repository, [ref_path], {ref_path: []}, |
231 | + macaroon_raw=macaroon.serialize()) |
232 | + login(ANONYMOUS) |
233 | + self.assertHasRefPermissions( |
234 | + self.factory.makePerson(), repository, [ref_path], |
235 | + {ref_path: []}, macaroon_raw=macaroon.serialize()) |
236 | + login(ANONYMOUS) |
237 | + self.assertHasRefPermissions( |
238 | + requester, repository, [ref_path], {ref_path: []}, |
239 | + macaroon_raw=Macaroon( |
240 | + location=config.vhost.mainsite.hostname, |
241 | + identifier="another", key="another-secret").serialize()) |
242 | + login(ANONYMOUS) |
243 | + self.assertHasRefPermissions( |
244 | + requester, repository, [ref_path], {ref_path: []}, |
245 | + macaroon_raw="nonsense") |
246 | + |
247 | |
248 | class TestGitAPISecurity(TestGitAPIMixin, TestCaseWithFactory): |
249 | """Slow tests for `IGitAPI`. |
This should all be much clearer now, so please take another look.