Merge lp:~cjwatson/launchpad/librarian-accept-macaroon into lp:launchpad

Proposed by Colin Watson on 2018-05-04
Status: Needs review
Proposed branch: lp:~cjwatson/launchpad/librarian-accept-macaroon
Merge into: lp:launchpad
Diff against target: 983 lines (+427/-111)
11 files modified
configs/testrunner-appserver/launchpad-lazr.conf (+1/-1)
configs/testrunner/launchpad-lazr.conf (+1/-0)
database/schema/security.cfg (+4/-1)
lib/lp/code/model/codeimportjob.py (+2/-0)
lib/lp/services/config/schema-lazr.conf (+3/-0)
lib/lp/services/librarianserver/db.py (+24/-10)
lib/lp/services/librarianserver/tests/test_web.py (+142/-95)
lib/lp/services/librarianserver/web.py (+8/-1)
lib/lp/soyuz/configure.zcml (+10/-1)
lib/lp/soyuz/model/binarypackagebuild.py (+89/-2)
lib/lp/soyuz/tests/test_binarypackagebuild.py (+143/-0)
To merge this branch: bzr merge lp:~cjwatson/launchpad/librarian-accept-macaroon
Reviewer Review Type Date Requested Status
William Grant 2018-05-04 Needs Information on 2018-05-09
Review via email: mp+345079@code.launchpad.net

Commit message

Allow macaroon authentication to the librarian for BPBs' source files.

Description of the change

This will later allow us to dispatch private builds immediately, rather than having to wait for the source to be published first.

In this case we accept the macaroon as the password part of basic authentication with an empty username, similar to the git authentication method used by code import jobs. This is a bit weird, but it avoids needing to change launchpad-buildd, and we could always switch to a more conventional "Authorization: Macaroon ..." scheme later. (Alternatively, I suppose we could put the macaroon in a query parameter instead?)

The only things I can see that we'll need to do deployment-wise are to set up *.restricted.dogfood.launchpadlibrarian.net (both DNS and listening to it on a different IP address from the public librarian) and to configure librarian.macaroon_secret_key.

To post a comment you must log in.
William Grant (wgrant) wrote :

This seems like a massive layering violation. I really think we should just issue macaroons with LFA.id caveats directly, even though that loses us the BuildStatus.BUILDING restriction.

review: Needs Information
Colin Watson (cjwatson) wrote :

I don't understand the principle here. Can you explain why it's any more of a layering violation for a BPB issuer to grant permission to access its associated LFAs than it is for a CodeImportJob issuer to grant permission to access its associated GitRepositories?

In general, my intent in designing the IMacaroonIssuer interface was that various subsystems could grant the specific permissions they needed, and the verifying code could then use the getUtility(IMacaroonIssuer, token.identifier) idiom to get the appropriate verifier without having to have specific knowledge of what the verifier in question does. Apart from the security.cfg changes, nothing about the librarian changes here is at all specific to BPBs, and it would be perfectly possible to write a different verifier that grants access based on some other criteria; furthermore, if (hypothetically) a BPB needed access to some other private resource, the same macaroon could be used for both by generalising the verifier.

The only thing here that feels at all like a layering violation to me is that BPBMacaroonIssuer.verifyMacaroon takes an LFA ID rather than a build. But, firstly, that's something that it's free to generalise later without reference to the librarian (although I suppose that would be easier if it took an actual LFA; that would require a little bit of extra work in the librarian to avoid an extra query), and, secondly, it's OK for BPBs to know about LFAs, just not vice versa.

William Grant (wgrant) wrote :

On 09/05/18 23:24, Colin Watson wrote:
> I don't understand the principle here. Can you explain why it's any
> more of a layering violation for a BPB issuer to grant permission to
> access its associated LFAs than it is for a CodeImportJob issuer to
> grant permission to access its associated GitRepositories?

It was less obviously wrong there because the GitRepository
authorisation code already touched CodeImport and they were part of the
same Launchpad application. But in this case we have the librarian
touching complex bits of the Soyuz schema, which is ugly from a code
structure perspective and also means we have to be very careful around
synchronising schema changes and librarian deployments.

librarian today only examines non-librarian tables when GC introspects
the schema to work out which FKs there are to LFA.id, and that doesn't
seem like an architectural separation that we should abandon lightly.

> In general, my intent in designing the IMacaroonIssuer interface was
> that various subsystems could grant the specific permissions they
> needed, and the verifying code could then use the
> getUtility(IMacaroonIssuer, token.identifier) idiom to get the
> appropriate verifier without having to have specific knowledge of
> what the verifier in question does. Apart from the security.cfg
> changes, nothing about the librarian changes here is at all specific
> to BPBs, and it would be perfectly possible to write a different
> verifier that grants access based on some other criteria;
> furthermore, if (hypothetically) a BPB needed access to some other
> private resource, the same macaroon could be used for both by
> generalising the verifier.

I agree that sharing macaroons would be ideal, but having the librarian
itself validate macaroons that may require arbitrarily broad and complex
database logic (particularly given the librarian is a Twisted app which
really shouldn't be doing synchronous DB calls at all) doesn't seem right.

Colin Watson (cjwatson) wrote :

Would it be better if the librarian talked to the authserver for this?
I'd avoided that approach because it seemed like unnecessary complexity,
but it could easily be done asynchronously and it would allow preserving
the fine-grained permissions, which I'd really like to find a way to do.
We could do it only for the macaroon auth case, which I expect to
continue being comparatively rare.

William Grant (wgrant) wrote :

I think that sounds fine. It's a bit of a change from where we are today, but not a huge way off and not clearly in the wrong direction.

Unmerged revisions

18631. By Colin Watson on 2018-05-04

Allow macaroon authentication to the librarian for BPBs' source files.

This will later allow us to dispatch private builds immediately, rather than
having to wait for the source to be published first.

In this case we accept the macaroon as the password part of basic
authentication with an empty username, similar to the git authentication
method used by code import jobs. This is a bit weird, but it avoids needing
to change launchpad-buildd, and we could always switch to a more
conventional "Authorization: Macaroon ..." scheme later.

18630. By Colin Watson on 2018-05-04

Make CodeImportJobMacaroonIssuer.verifyMacaroon check that the context is the right type, just in case.

18629. By Colin Watson on 2018-05-04

Convert lp.services.librarianserver.tests.test_web to requests.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'configs/testrunner-appserver/launchpad-lazr.conf'
2--- configs/testrunner-appserver/launchpad-lazr.conf 2018-03-16 21:04:45 +0000
3+++ configs/testrunner-appserver/launchpad-lazr.conf 2018-05-04 10:59:37 +0000
4@@ -9,7 +9,7 @@
5 launch: False
6
7 [codeimport]
8-macaroon_secret_key: dev-macaroon-secret
9+macaroon_secret_key: codeimport-dev-macaroon-secret
10
11 [google_test_service]
12 launch: False
13
14=== modified file 'configs/testrunner/launchpad-lazr.conf'
15--- configs/testrunner/launchpad-lazr.conf 2018-03-16 21:04:45 +0000
16+++ configs/testrunner/launchpad-lazr.conf 2018-05-04 10:59:37 +0000
17@@ -129,6 +129,7 @@
18 restricted_download_url: http://localhost:58005/
19 restricted_upload_port: 59095
20 restricted_download_port: 58005
21+macaroon_secret_key: librarian-dev-macaroon-secret
22
23 [librarian_server]
24 root: /var/tmp/fatsam.test
25
26=== modified file 'database/schema/security.cfg'
27--- database/schema/security.cfg 2018-04-10 13:07:25 +0000
28+++ database/schema/security.cfg 2018-05-04 10:59:37 +0000
29@@ -1,4 +1,4 @@
30-# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
31+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
32 # GNU Affero General Public License version 3 (see the file LICENSE).
33 #
34 # Possible permissions: SELECT, INSERT, UPDATE, EXECUTE
35@@ -392,8 +392,11 @@
36 type=user
37
38 [librarian]
39+public.binarypackagebuild = SELECT
40 public.libraryfilealias = SELECT, INSERT, UPDATE
41 public.libraryfilecontent = SELECT, INSERT
42+public.sourcepackagerelease = SELECT
43+public.sourcepackagereleasefile = SELECT
44 type=user
45
46 [librarianfeedswift]
47
48=== modified file 'lib/lp/code/model/codeimportjob.py'
49--- lib/lp/code/model/codeimportjob.py 2018-03-15 20:44:04 +0000
50+++ lib/lp/code/model/codeimportjob.py 2018-05-04 10:59:37 +0000
51@@ -442,6 +442,8 @@
52
53 def verifyMacaroon(self, macaroon, context):
54 """See `IMacaroonIssuer`."""
55+ if not ICodeImportJob.providedBy(context):
56+ return False
57 if not self.checkMacaroonIssuer(macaroon):
58 return False
59 try:
60
61=== modified file 'lib/lp/services/config/schema-lazr.conf'
62--- lib/lp/services/config/schema-lazr.conf 2018-04-12 19:59:21 +0000
63+++ lib/lp/services/config/schema-lazr.conf 2018-05-04 10:59:37 +0000
64@@ -1194,6 +1194,9 @@
65
66 use_https = True
67
68+# Secret key for macaroons used to grant library file download permissions.
69+macaroon_secret_key: none
70+
71
72 [librarian_gc]
73 # The database user which will be used by this process.
74
75=== modified file 'lib/lp/services/librarianserver/db.py'
76--- lib/lp/services/librarianserver/db.py 2016-01-10 22:37:40 +0000
77+++ lib/lp/services/librarianserver/db.py 2018-05-04 10:59:37 +0000
78@@ -1,4 +1,4 @@
79-# Copyright 2009 Canonical Ltd. This software is licensed under the
80+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
81 # GNU Affero General Public License version 3 (see the file LICENSE).
82
83 """Database access layer for the Librarian."""
84@@ -11,10 +11,13 @@
85 import hashlib
86 import urllib
87
88+from pymacaroons import Macaroon
89 from storm.expr import (
90 And,
91 SQL,
92 )
93+from zope.component import getUtility
94+from zope.security.proxy import removeSecurityProxy
95
96 from lp.services.database.interfaces import IStore
97 from lp.services.database.sqlbase import session_store
98@@ -23,6 +26,7 @@
99 LibraryFileContent,
100 TimeLimitedToken,
101 )
102+from lp.services.macaroons.interfaces import IMacaroonIssuer
103
104
105 class Library:
106@@ -69,16 +73,26 @@
107 #
108 # This needs to match url_path_quote.
109 normalised_path = urllib.quote(urllib.unquote(path), safe='/~+')
110- store = session_store()
111- token_found = store.find(TimeLimitedToken,
112- SQL("age(created) < interval '1 day'"),
113- TimeLimitedToken.token == hashlib.sha256(token).hexdigest(),
114- TimeLimitedToken.path == normalised_path).is_empty()
115- store.reset()
116- if token_found:
117+ if isinstance(token, Macaroon):
118+ # We have no Zope interaction, so must remove the proxy.
119+ issuer = removeSecurityProxy(
120+ getUtility(IMacaroonIssuer, token.identifier))
121+ # Macaroons have enough other constraints that they don't
122+ # need to be path-specific; it's simpler and faster to just
123+ # check the alias ID.
124+ token_ok = issuer.verifyMacaroon(token, aliasid)
125+ else:
126+ store = session_store()
127+ token_ok = not store.find(TimeLimitedToken,
128+ SQL("age(created) < interval '1 day'"),
129+ TimeLimitedToken.token ==
130+ hashlib.sha256(token).hexdigest(),
131+ TimeLimitedToken.path == normalised_path).is_empty()
132+ store.reset()
133+ if token_ok:
134+ restricted = True
135+ else:
136 raise LookupError("Token stale/pruned/path mismatch")
137- else:
138- restricted = True
139 alias = LibraryFileAlias.selectOne(And(
140 LibraryFileAlias.id == aliasid,
141 LibraryFileAlias.contentID == LibraryFileContent.q.id,
142
143=== modified file 'lib/lp/services/librarianserver/tests/test_web.py'
144--- lib/lp/services/librarianserver/tests/test_web.py 2018-01-02 10:54:31 +0000
145+++ lib/lp/services/librarianserver/tests/test_web.py 2018-05-04 10:59:37 +0000
146@@ -1,26 +1,25 @@
147-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
148+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
149 # GNU Affero General Public License version 3 (see the file LICENSE).
150
151-from cStringIO import StringIO
152 from datetime import datetime
153+from gzip import GzipFile
154 import hashlib
155 import httplib
156+from io import BytesIO
157 import os
158 import unittest
159-from urllib2 import (
160- HTTPError,
161- urlopen,
162- )
163 from urlparse import urlparse
164
165 from lazr.uri import URI
166 import pytz
167+import requests
168 from storm.expr import SQL
169-import testtools
170 from testtools.matchers import EndsWith
171 import transaction
172 from zope.component import getUtility
173+from zope.security.proxy import removeSecurityProxy
174
175+from lp.buildmaster.enums import BuildStatus
176 from lp.services.config import config
177 from lp.services.database.interfaces import IMasterStore
178 from lp.services.database.sqlbase import (
179@@ -39,7 +38,12 @@
180 TimeLimitedToken,
181 )
182 from lp.services.librarianserver.storage import LibrarianStorage
183-from lp.testing.dbuser import switch_dbuser
184+from lp.services.macaroons.interfaces import IMacaroonIssuer
185+from lp.testing import TestCaseWithFactory
186+from lp.testing.dbuser import (
187+ dbuser,
188+ switch_dbuser,
189+ )
190 from lp.testing.layers import (
191 LaunchpadFunctionalLayer,
192 LaunchpadZopelessLayer,
193@@ -52,10 +56,9 @@
194 return str(parsed.replace(path=parsed.path.replace(old, new)))
195
196
197-class LibrarianWebTestCase(testtools.TestCase):
198+class LibrarianWebTestCase(TestCaseWithFactory):
199 """Test the librarian's web interface."""
200 layer = LaunchpadFunctionalLayer
201- dbuser = 'librarian'
202
203 # Add stuff to a librarian via the upload port, then check that it's
204 # immediately visible on the web interface. (in an attempt to test ddaa's
205@@ -75,9 +78,9 @@
206 for count in range(10):
207 # Upload a file. This should work without any exceptions being
208 # thrown.
209- sampleData = 'x' + ('blah' * (count % 5))
210+ sampleData = b'x' + (b'blah' * (count % 5))
211 fileAlias = client.addFile('sample', len(sampleData),
212- StringIO(sampleData),
213+ BytesIO(sampleData),
214 contentType='text/plain')
215
216 # Make sure we can get its URL
217@@ -98,9 +101,9 @@
218 fileObj.close()
219
220 # And make sure the URL works too
221- fileObj = urlopen(url)
222- self.assertEqual(sampleData, fileObj.read())
223- fileObj.close()
224+ response = requests.get(url)
225+ response.raise_for_status()
226+ self.assertEqual(sampleData, response.content)
227
228 def test_checkGzipEncoding(self):
229 # Files that end in ".txt.gz" are treated special and are returned
230@@ -108,29 +111,34 @@
231 # displaying Ubuntu build logs in the browser. The mimetype should be
232 # "text/plain" for these files.
233 client = LibrarianClient()
234- contents = 'Build log...'
235- build_log = StringIO(contents)
236+ contents = b'Build log...'
237+ build_log = BytesIO()
238+ with GzipFile(mode='wb', fileobj=build_log) as f:
239+ f.write(contents)
240+ build_log.seek(0)
241 alias_id = client.addFile(name="build_log.txt.gz",
242- size=len(contents),
243+ size=len(build_log.getvalue()),
244 file=build_log,
245 contentType="text/plain")
246
247 self.commit()
248
249 url = client.getURLForAlias(alias_id)
250- fileObj = urlopen(url)
251- mimetype = fileObj.headers['content-type']
252- encoding = fileObj.headers['content-encoding']
253+ response = requests.get(url)
254+ response.raise_for_status()
255+ mimetype = response.headers['content-type']
256+ encoding = response.headers['content-encoding']
257 self.assertTrue(mimetype == "text/plain; charset=utf-8",
258 "Wrong mimetype. %s != 'text/plain'." % mimetype)
259 self.assertTrue(encoding == "gzip",
260 "Wrong encoding. %s != 'gzip'." % encoding)
261+ self.assertEqual(contents, response.content)
262
263 def test_checkNoEncoding(self):
264 # Other files should have no encoding.
265 client = LibrarianClient()
266- contents = 'Build log...'
267- build_log = StringIO(contents)
268+ contents = b'Build log...'
269+ build_log = BytesIO(contents)
270 alias_id = client.addFile(name="build_log.tgz",
271 size=len(contents),
272 file=build_log,
273@@ -139,10 +147,10 @@
274 self.commit()
275
276 url = client.getURLForAlias(alias_id)
277- fileObj = urlopen(url)
278- mimetype = fileObj.headers['content-type']
279- self.assertRaises(KeyError, fileObj.headers.__getitem__,
280- 'content-encoding')
281+ response = requests.get(url)
282+ response.raise_for_status()
283+ mimetype = response.headers['content-type']
284+ self.assertNotIn('content-encoding', response.headers)
285 self.assertTrue(
286 mimetype == "application/x-tar",
287 "Wrong mimetype. %s != 'application/x-tar'." % mimetype)
288@@ -157,13 +165,17 @@
289 # ignored
290 client = LibrarianClient()
291 filename = 'sample.txt'
292- aid = client.addFile(filename, 6, StringIO('sample'), 'text/plain')
293+ aid = client.addFile(filename, 6, BytesIO(b'sample'), 'text/plain')
294 self.commit()
295 url = client.getURLForAlias(aid)
296- self.assertEqual(urlopen(url).read(), 'sample')
297+ response = requests.get(url)
298+ response.raise_for_status()
299+ self.assertEqual(response.content, b'sample')
300
301 old_url = uri_path_replace(url, str(aid), '42/%d' % aid)
302- self.assertEqual(urlopen(old_url).read(), 'sample')
303+ response = requests.get(url)
304+ response.raise_for_status()
305+ self.assertEqual(response.content, b'sample')
306
307 # If the content and alias IDs are not integers, a 404 is raised
308 old_url = uri_path_replace(url, str(aid), 'foo/%d' % aid)
309@@ -174,10 +186,12 @@
310 def test_404(self):
311 client = LibrarianClient()
312 filename = 'sample.txt'
313- aid = client.addFile(filename, 6, StringIO('sample'), 'text/plain')
314+ aid = client.addFile(filename, 6, BytesIO(b'sample'), 'text/plain')
315 self.commit()
316 url = client.getURLForAlias(aid)
317- self.assertEqual(urlopen(url).read(), 'sample')
318+ response = requests.get(url)
319+ response.raise_for_status()
320+ self.assertEqual(response.content, b'sample')
321
322 # Change the aliasid and assert we get a 404
323 self.assertIn(str(aid), url)
324@@ -192,29 +206,30 @@
325 def test_duplicateuploads(self):
326 client = LibrarianClient()
327 filename = 'sample.txt'
328- id1 = client.addFile(filename, 6, StringIO('sample'), 'text/plain')
329- id2 = client.addFile(filename, 6, StringIO('sample'), 'text/plain')
330+ id1 = client.addFile(filename, 6, BytesIO(b'sample'), 'text/plain')
331+ id2 = client.addFile(filename, 6, BytesIO(b'sample'), 'text/plain')
332
333 self.assertNotEqual(id1, id2, 'Got allocated the same id!')
334
335 self.commit()
336
337- self.assertEqual(client.getFileByAlias(id1).read(), 'sample')
338- self.assertEqual(client.getFileByAlias(id2).read(), 'sample')
339+ self.assertEqual(client.getFileByAlias(id1).read(), b'sample')
340+ self.assertEqual(client.getFileByAlias(id2).read(), b'sample')
341
342 def test_robotsTxt(self):
343 url = 'http://%s:%d/robots.txt' % (
344 config.librarian.download_host, config.librarian.download_port)
345- f = urlopen(url)
346- self.assertIn('Disallow: /', f.read())
347+ response = requests.get(url)
348+ response.raise_for_status()
349+ self.assertIn('Disallow: /', response.text)
350
351 def test_headers(self):
352 client = LibrarianClient()
353
354 # Upload a file so we can retrieve it.
355- sample_data = 'blah'
356+ sample_data = b'blah'
357 file_alias_id = client.addFile(
358- 'sample', len(sample_data), StringIO(sample_data),
359+ 'sample', len(sample_data), BytesIO(sample_data),
360 contentType='text/plain')
361 url = client.getURLForAlias(file_alias_id)
362
363@@ -229,9 +244,10 @@
364 self.commit()
365
366 # Fetch the file via HTTP, recording the interesting headers
367- result = urlopen(url)
368- last_modified_header = result.info()['Last-Modified']
369- cache_control_header = result.info()['Cache-Control']
370+ response = requests.get(url)
371+ response.raise_for_status()
372+ last_modified_header = response.headers['Last-Modified']
373+ cache_control_header = response.headers['Cache-Control']
374
375 # URLs point to the same content for ever, so we have a hardcoded
376 # 1 year max-age cache policy.
377@@ -247,9 +263,9 @@
378 client = LibrarianClient()
379
380 # Upload a file so we can retrieve it.
381- sample_data = 'blah'
382+ sample_data = b'blah'
383 file_alias_id = client.addFile(
384- 'sample', len(sample_data), StringIO(sample_data),
385+ 'sample', len(sample_data), BytesIO(sample_data),
386 contentType='text/plain')
387 url = client.getURLForAlias(file_alias_id)
388
389@@ -262,18 +278,19 @@
390 self.commit()
391
392 # Fetch the file via HTTP.
393- urlopen(url)
394+ response = requests.get(url)
395+ response.raise_for_status()
396
397 # Delete the on-disk file.
398 storage = LibrarianStorage(config.librarian_server.root, None)
399 os.remove(storage._fileLocation(file_alias.contentID))
400
401 # The URL now 500s, since the DB says it should exist.
402- exception = self.assertRaises(HTTPError, urlopen, url)
403- self.assertEqual(500, exception.code)
404- self.assertIn('Server', exception.info())
405- self.assertNotIn('Last-Modified', exception.info())
406- self.assertNotIn('Cache-Control', exception.info())
407+ response = requests.get(url)
408+ self.assertEqual(500, response.status_code)
409+ self.assertIn('Server', response.headers)
410+ self.assertNotIn('Last-Modified', response.headers)
411+ self.assertNotIn('Cache-Control', response.headers)
412
413 def get_restricted_file_and_public_url(self, filename='sample'):
414 # Use a regular LibrarianClient to ensure we speak to the
415@@ -281,10 +298,10 @@
416 # restricted files are served from.
417 client = LibrarianClient()
418 fileAlias = client.addFile(
419- filename, 12, StringIO('a' * 12), contentType='text/plain')
420+ filename, 12, BytesIO(b'a' * 12), contentType='text/plain')
421 # Note: We're deliberately using the wrong url here: we should be
422 # passing secure=True to getURLForAlias, but to use the returned URL
423- # we would need a wildcard DNS facility patched into urlopen; instead
424+ # we would need a wildcard DNS facility patched into requests; instead
425 # we use the *deliberate* choice of having the path of secure and
426 # insecure urls be the same, so that we can test it: the server code
427 # doesn't need to know about the fancy wildcard domains.
428@@ -301,9 +318,9 @@
429 # IFF there is a .restricted. in the host, then the library file alias
430 # in the subdomain must match that in the path.
431 client = LibrarianClient()
432- fileAlias = client.addFile('sample', 12, StringIO('a' * 12),
433+ fileAlias = client.addFile('sample', 12, BytesIO(b'a' * 12),
434 contentType='text/plain')
435- fileAlias2 = client.addFile('sample', 12, StringIO('b' * 12),
436+ fileAlias2 = client.addFile('sample', 12, BytesIO(b'b' * 12),
437 contentType='text/plain')
438 self.commit()
439 url = client.getURLForAlias(fileAlias)
440@@ -313,7 +330,8 @@
441 template_host = 'i%%d.restricted.%s' % download_host
442 path = get_libraryfilealias_download_path(fileAlias, 'sample')
443 # The basic URL must work.
444- urlopen(url)
445+ response = requests.get(url)
446+ response.raise_for_status()
447 # Use the network level protocol because DNS resolution won't work
448 # here (no wildcard support)
449 connection = httplib.HTTPConnection(
450@@ -356,20 +374,17 @@
451 fileAlias, url = self.get_restricted_file_and_public_url()
452 # The file should not be able to be opened - the token supplied
453 # is not one we issued.
454- self.require404(url + '?token=haxx0r')
455+ self.require404(url, params={"token": "haxx0r"})
456
457 def test_restricted_with_token(self):
458 fileAlias, url = self.get_restricted_file_and_public_url()
459 # We have the base url for a restricted file; grant access to it
460 # for a short time.
461 token = TimeLimitedToken.allocate(url)
462- url = url + "?token=%s" % token
463 # Now we should be able to access the file.
464- fileObj = urlopen(url)
465- try:
466- self.assertEqual("a" * 12, fileObj.read())
467- finally:
468- fileObj.close()
469+ response = requests.get(url, params={"token": token})
470+ response.raise_for_status()
471+ self.assertEqual(b"a" * 12, response.content)
472
473 def test_restricted_with_token_encoding(self):
474 fileAlias, url = self.get_restricted_file_and_public_url('foo~%')
475@@ -380,20 +395,16 @@
476 token = TimeLimitedToken.allocate(url)
477
478 # Now we should be able to access the file.
479- fileObj = urlopen(url + "?token=%s" % token)
480- try:
481- self.assertEqual("a" * 12, fileObj.read())
482- finally:
483- fileObj.close()
484+ response = requests.get(url, params={"token": token})
485+ response.raise_for_status()
486+ self.assertEqual(b"a" * 12, response.content)
487
488 # The token is valid even if the filename is encoded differently.
489 mangled_url = url.replace('~', '%7E')
490 self.assertNotEqual(mangled_url, url)
491- fileObj = urlopen(mangled_url + "?token=%s" % token)
492- try:
493- self.assertEqual("a" * 12, fileObj.read())
494- finally:
495- fileObj.close()
496+ response = requests.get(url, params={"token": token})
497+ response.raise_for_status()
498+ self.assertEqual(b"a" * 12, response.content)
499
500 def test_restricted_with_expired_token(self):
501 fileAlias, url = self.get_restricted_file_and_public_url()
502@@ -407,14 +418,55 @@
503 TimeLimitedToken.token == hashlib.sha256(token).hexdigest())
504 tokens.set(
505 TimeLimitedToken.created == SQL("created - interval '1 week'"))
506- url = url + "?token=%s" % token
507 # Now, as per test_restricted_no_token we should get a 404.
508- self.require404(url)
509+ self.require404(url, params={"token": token})
510+
511+ def test_restricted_with_macaroon(self):
512+ fileAlias, url = self.get_restricted_file_and_public_url()
513+ lfa = IMasterStore(LibraryFileAlias).get(LibraryFileAlias, fileAlias)
514+ with dbuser('testadmin'):
515+ build = self.factory.makeBinaryPackageBuild(
516+ archive=self.factory.makeArchive(private=True))
517+ naked_build = removeSecurityProxy(build)
518+ self.factory.makeSourcePackageReleaseFile(
519+ sourcepackagerelease=naked_build.source_package_release,
520+ library_file=lfa)
521+ naked_build.updateStatus(BuildStatus.BUILDING)
522+ issuer = getUtility(IMacaroonIssuer, "binary-package-build")
523+ macaroon = removeSecurityProxy(issuer).issueMacaroon(build)
524+ self.commit()
525+ response = requests.get(url, auth=("", macaroon.serialize()))
526+ response.raise_for_status()
527+ self.assertEqual(b"a" * 12, response.content)
528+
529+ def test_restricted_with_invalid_macaroon(self):
530+ fileAlias, url = self.get_restricted_file_and_public_url()
531+ lfa = IMasterStore(LibraryFileAlias).get(LibraryFileAlias, fileAlias)
532+ with dbuser('testadmin'):
533+ build = self.factory.makeBinaryPackageBuild(
534+ archive=self.factory.makeArchive(private=True))
535+ naked_build = removeSecurityProxy(build)
536+ self.factory.makeSourcePackageReleaseFile(
537+ sourcepackagerelease=naked_build.source_package_release,
538+ library_file=lfa)
539+ naked_build.updateStatus(BuildStatus.BUILDING)
540+ self.commit()
541+ self.require404(url, auth=("", "not-a-macaroon"))
542+
543+ def test_restricted_with_unverifiable_macaroon(self):
544+ fileAlias, url = self.get_restricted_file_and_public_url()
545+ with dbuser('testadmin'):
546+ build = self.factory.makeBinaryPackageBuild(
547+ archive=self.factory.makeArchive(private=True))
548+ removeSecurityProxy(build).updateStatus(BuildStatus.BUILDING)
549+ issuer = getUtility(IMacaroonIssuer, "binary-package-build")
550+ macaroon = removeSecurityProxy(issuer).issueMacaroon(build)
551+ self.commit()
552+ self.require404(url, auth=("", macaroon.serialize()))
553
554 def test_restricted_file_headers(self):
555 fileAlias, url = self.get_restricted_file_and_public_url()
556 token = TimeLimitedToken.allocate(url)
557- url = url + "?token=%s" % token
558 # Change the date_created to a known value for testing.
559 file_alias = IMasterStore(LibraryFileAlias).get(
560 LibraryFileAlias, fileAlias)
561@@ -423,9 +475,9 @@
562 # Commit the update.
563 self.commit()
564 # Fetch the file via HTTP, recording the interesting headers
565- result = urlopen(url)
566- last_modified_header = result.info()['Last-Modified']
567- cache_control_header = result.info()['Cache-Control']
568+ response = requests.get(url, params={"token": token})
569+ last_modified_header = response.headers['Last-Modified']
570+ cache_control_header = response.headers['Cache-Control']
571 # No caching for restricted files.
572 self.assertEqual(cache_control_header, 'max-age=0, private')
573 # And we should have a correct Last-Modified header too.
574@@ -433,13 +485,10 @@
575 last_modified_header, 'Tue, 30 Jan 2001 13:45:59 GMT')
576 # Perhaps we should also set Expires to the Last-Modified.
577
578- def require404(self, url):
579+ def require404(self, url, **kwargs):
580 """Assert that opening `url` raises a 404."""
581- try:
582- urlopen(url)
583- self.fail('404 not raised')
584- except HTTPError as e:
585- self.assertEqual(e.code, 404)
586+ response = requests.get(url, **kwargs)
587+ self.assertEqual(404, response.status_code)
588
589
590 class LibrarianZopelessWebTestCase(LibrarianWebTestCase):
591@@ -455,9 +504,9 @@
592 def test_getURLForAliasObject(self):
593 # getURLForAliasObject returns the same URL as getURLForAlias.
594 client = LibrarianClient()
595- content = "Test content"
596+ content = b"Test content"
597 alias_id = client.addFile(
598- 'test.txt', len(content), StringIO(content),
599+ 'test.txt', len(content), BytesIO(content),
600 contentType='text/plain')
601 self.commit()
602
603@@ -481,7 +530,7 @@
604 switch_dbuser('testadmin')
605
606 alias = getUtility(ILibraryFileAliasSet).create(
607- 'whatever', 8, StringIO('xxx\nxxx\n'), 'text/plain')
608+ 'whatever', 8, BytesIO(b'xxx\nxxx\n'), 'text/plain')
609 alias_id = alias.id
610 transaction.commit()
611
612@@ -493,8 +542,9 @@
613
614 # And it can be retrieved via the web
615 url = alias.http_url
616- retrieved_content = urlopen(url).read()
617- self.assertEqual(retrieved_content, 'xxx\nxxx\n')
618+ response = requests.get(url)
619+ response.raise_for_status()
620+ self.assertEqual(response.content, b'xxx\nxxx\n')
621
622 # But when we flag the content as deleted
623 cur = cursor()
624@@ -508,8 +558,5 @@
625 self.assertRaises(DownloadFailed, alias.open)
626
627 # And people see a 404 page
628- try:
629- urlopen(url)
630- self.fail('404 not raised')
631- except HTTPError as x:
632- self.assertEqual(x.code, 404)
633+ response = requests.get(url)
634+ self.assertEqual(404, response.status_code)
635
636=== modified file 'lib/lp/services/librarianserver/web.py'
637--- lib/lp/services/librarianserver/web.py 2018-01-01 17:40:03 +0000
638+++ lib/lp/services/librarianserver/web.py 2018-05-04 10:59:37 +0000
639@@ -1,4 +1,4 @@
640-# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
641+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
642 # GNU Affero General Public License version 3 (see the file LICENSE).
643
644 __metaclass__ = type
645@@ -7,6 +7,7 @@
646 import time
647 from urlparse import urlparse
648
649+from pymacaroons import Macaroon
650 from storm.exceptions import DisconnectionError
651 from twisted.internet import defer
652 from twisted.internet.threads import deferToThread
653@@ -116,6 +117,12 @@
654 return fourOhFour
655
656 token = request.args.get('token', [None])[0]
657+ if token is None:
658+ if not request.getUser() and request.getPassword():
659+ try:
660+ token = Macaroon.deserialize(request.getPassword())
661+ except Exception:
662+ pass
663 path = request.path
664 deferred = deferToThread(
665 self._getFileAlias, self.aliasID, token, path)
666
667=== modified file 'lib/lp/soyuz/configure.zcml'
668--- lib/lp/soyuz/configure.zcml 2017-07-18 16:22:03 +0000
669+++ lib/lp/soyuz/configure.zcml 2018-05-04 10:59:37 +0000
670@@ -1,4 +1,4 @@
671-<!-- Copyright 2009-2017 Canonical Ltd. This software is licensed under the
672+<!-- Copyright 2009-2018 Canonical Ltd. This software is licensed under the
673 GNU Affero General Public License version 3 (see the file LICENSE).
674 -->
675
676@@ -480,6 +480,15 @@
677 <allow interface="lp.buildmaster.interfaces.buildfarmjob.ISpecificBuildFarmJobSource"/>
678 </securedutility>
679
680+ <!-- BinaryPackageBuildMacaroonIssuer -->
681+
682+ <securedutility
683+ class="lp.soyuz.model.binarypackagebuild.BinaryPackageBuildMacaroonIssuer"
684+ provides="lp.services.macaroons.interfaces.IMacaroonIssuer"
685+ name="binary-package-build">
686+ <allow interface="lp.services.macaroons.interfaces.IMacaroonIssuerPublic"/>
687+ </securedutility>
688+
689 <!-- DistroArchSeriesBinaryPackage -->
690
691 <class
692
693=== modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
694--- lib/lp/soyuz/model/binarypackagebuild.py 2017-03-29 09:28:09 +0000
695+++ lib/lp/soyuz/model/binarypackagebuild.py 2018-05-04 10:59:37 +0000
696@@ -1,4 +1,4 @@
697-# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
698+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
699 # GNU Affero General Public License version 3 (see the file LICENSE).
700
701 __metaclass__ = type
702@@ -21,6 +21,10 @@
703
704 import apt_pkg
705 from debian.deb822 import PkgRelation
706+from pymacaroons import (
707+ Macaroon,
708+ Verifier,
709+ )
710 import pytz
711 from sqlobject import SQLObjectNotFound
712 from storm.expr import (
713@@ -81,6 +85,7 @@
714 LibraryFileAlias,
715 LibraryFileContent,
716 )
717+from lp.services.macaroons.interfaces import IMacaroonIssuer
718 from lp.soyuz.adapters.buildarch import determine_architectures_to_build
719 from lp.soyuz.enums import (
720 ArchivePurpose,
721@@ -102,7 +107,10 @@
722 from lp.soyuz.mail.binarypackagebuild import BinaryPackageBuildMailer
723 from lp.soyuz.model.binarypackagename import BinaryPackageName
724 from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
725-from lp.soyuz.model.files import BinaryPackageFile
726+from lp.soyuz.model.files import (
727+ BinaryPackageFile,
728+ SourcePackageReleaseFile,
729+ )
730 from lp.soyuz.model.packageset import Packageset
731 from lp.soyuz.model.queue import (
732 PackageUpload,
733@@ -1362,3 +1370,82 @@
734 % (build.title, build.id, build.archive.displayname,
735 build_queue.lastscore))
736 return new_builds
737+
738+
739+@implementer(IMacaroonIssuer)
740+class BinaryPackageBuildMacaroonIssuer:
741+
742+ @property
743+ def _root_secret(self):
744+ secret = config.librarian.macaroon_secret_key
745+ if not secret:
746+ raise RuntimeError("librarian.macaroon_secret_key not configured.")
747+ return secret
748+
749+ def issueMacaroon(self, context):
750+ """See `IMacaroonIssuer`.
751+
752+ For issuing, the context is an `IBinaryPackageBuild`.
753+ """
754+ if not removeSecurityProxy(context).archive.private:
755+ raise AssertionError(
756+ "Refusing to issue macaroon for public build.")
757+ macaroon = Macaroon(
758+ location=config.vhost.mainsite.hostname,
759+ identifier="binary-package-build", key=self._root_secret)
760+ macaroon.add_first_party_caveat(
761+ "lp.binary-package-build %s" % removeSecurityProxy(context).id)
762+ return macaroon
763+
764+ def checkMacaroonIssuer(self, macaroon):
765+ """See `IMacaroonIssuer`."""
766+ if macaroon.location != config.vhost.mainsite.hostname:
767+ return False
768+ try:
769+ verifier = Verifier()
770+ verifier.satisfy_general(
771+ lambda caveat: caveat.startswith("lp.binary-package-build "))
772+ return verifier.verify(macaroon, self._root_secret)
773+ except Exception:
774+ return False
775+
776+ def verifyMacaroon(self, macaroon, context):
777+ """See `IMacaroonIssuer`.
778+
779+ For verification, the context is a `LibraryFileAlias` ID. We check
780+ that the file is one of those required to build the
781+ `IBinaryPackageBuild` that is the context of the macaroon, and that
782+ the context build is currently building.
783+ """
784+ # Circular import.
785+ from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
786+
787+ if not isinstance(context, int):
788+ return False
789+ if not self.checkMacaroonIssuer(macaroon):
790+ return False
791+
792+ def verify_build(caveat):
793+ prefix = "lp.binary-package-build "
794+ if not caveat.startswith(prefix):
795+ return False
796+ try:
797+ build_id = int(caveat[len(prefix):])
798+ except ValueError:
799+ return False
800+ return not IStore(BinaryPackageBuild).find(
801+ BinaryPackageBuild,
802+ BinaryPackageBuild.id == build_id,
803+ BinaryPackageBuild.source_package_release_id ==
804+ SourcePackageRelease.id,
805+ SourcePackageReleaseFile.sourcepackagereleaseID ==
806+ SourcePackageRelease.id,
807+ SourcePackageReleaseFile.libraryfileID == context,
808+ BinaryPackageBuild.status == BuildStatus.BUILDING).is_empty()
809+
810+ try:
811+ verifier = Verifier()
812+ verifier.satisfy_general(verify_build)
813+ return verifier.verify(macaroon, self._root_secret)
814+ except Exception:
815+ return False
816
817=== modified file 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
818--- lib/lp/soyuz/tests/test_binarypackagebuild.py 2018-02-09 14:56:43 +0000
819+++ lib/lp/soyuz/tests/test_binarypackagebuild.py 2018-05-04 10:59:37 +0000
820@@ -10,8 +10,13 @@
821 timedelta,
822 )
823
824+from pymacaroons import Macaroon
825 import pytz
826 from simplejson import dumps
827+from testtools.matchers import (
828+ MatchesListwise,
829+ MatchesStructure,
830+ )
831 from zope.component import getUtility
832 from zope.security.proxy import removeSecurityProxy
833
834@@ -22,7 +27,9 @@
835 from lp.registry.interfaces.pocket import PackagePublishingPocket
836 from lp.registry.interfaces.series import SeriesStatus
837 from lp.registry.interfaces.sourcepackage import SourcePackageUrgency
838+from lp.services.config import config
839 from lp.services.log.logger import DevNullLogger
840+from lp.services.macaroons.interfaces import IMacaroonIssuer
841 from lp.services.webapp.interaction import ANONYMOUS
842 from lp.services.webapp.interfaces import OAuthPermission
843 from lp.soyuz.enums import (
844@@ -897,3 +904,139 @@
845 with anonymous_logged_in():
846 self.assertScoreWriteableByTeam(
847 archive, getUtility(ILaunchpadCelebrities).ppa_admin)
848+
849+
850+class TestBinaryPackageBuildMacaroonIssuer(TestCaseWithFactory):
851+ """Test BinaryPackageBuild macaroon issuing and verification.
852+
853+ The librarian server verifies these macaroons, and has no Zope
854+ interaction when doing so, so we take care to test that verification
855+ works without an interaction.
856+ """
857+
858+ layer = LaunchpadZopelessLayer
859+
860+ def setUp(self):
861+ super(TestBinaryPackageBuildMacaroonIssuer, self).setUp()
862+ self.pushConfig("librarian", macaroon_secret_key="some-secret")
863+
864+ def test_issueMacaroon_refuses_public_archive(self):
865+ build = self.factory.makeBinaryPackageBuild()
866+ issuer = getUtility(IMacaroonIssuer, "binary-package-build")
867+ self.assertRaises(
868+ AssertionError, removeSecurityProxy(issuer).issueMacaroon, build)
869+
870+ def test_issueMacaroon_good(self):
871+ build = self.factory.makeBinaryPackageBuild(
872+ archive=self.factory.makeArchive(private=True))
873+ issuer = getUtility(IMacaroonIssuer, "binary-package-build")
874+ macaroon = removeSecurityProxy(issuer).issueMacaroon(build)
875+ self.assertEqual("launchpad.dev", macaroon.location)
876+ self.assertEqual("binary-package-build", macaroon.identifier)
877+ self.assertThat(macaroon.caveats, MatchesListwise([
878+ MatchesStructure.byEquality(
879+ caveat_id="lp.binary-package-build %s" % build.id),
880+ ]))
881+
882+ def test_checkMacaroonIssuer_good(self):
883+ build = self.factory.makeBinaryPackageBuild(
884+ archive=self.factory.makeArchive(private=True))
885+ issuer = getUtility(IMacaroonIssuer, "binary-package-build")
886+ macaroon = removeSecurityProxy(issuer).issueMacaroon(build)
887+ self.assertTrue(issuer.checkMacaroonIssuer(macaroon))
888+
889+ def test_checkMacaroonIssuer_wrong_location(self):
890+ issuer = getUtility(IMacaroonIssuer, "binary-package-build")
891+ macaroon = Macaroon(
892+ location="another-location",
893+ key=removeSecurityProxy(issuer)._root_secret)
894+ self.assertFalse(issuer.checkMacaroonIssuer(macaroon))
895+
896+ def test_checkMacaroonIssuer_wrong_key(self):
897+ issuer = getUtility(IMacaroonIssuer, "binary-package-build")
898+ macaroon = Macaroon(
899+ location=config.vhost.mainsite.hostname, key="another-secret")
900+ self.assertFalse(issuer.checkMacaroonIssuer(macaroon))
901+
902+ def test_verifyMacaroon_good(self):
903+ build = self.factory.makeBinaryPackageBuild(
904+ archive=self.factory.makeArchive(private=True))
905+ sprf = self.factory.makeSourcePackageReleaseFile(
906+ sourcepackagerelease=build.source_package_release)
907+ lfa_id = sprf.libraryfile.id
908+ build.updateStatus(BuildStatus.BUILDING)
909+ issuer = removeSecurityProxy(
910+ getUtility(IMacaroonIssuer, "binary-package-build"))
911+ macaroon = issuer.issueMacaroon(build)
912+ logout()
913+ self.assertTrue(issuer.verifyMacaroon(macaroon, lfa_id))
914+
915+ def test_verifyMacaroon_wrong_location(self):
916+ build = self.factory.makeBinaryPackageBuild(
917+ archive=self.factory.makeArchive(private=True))
918+ sprf = self.factory.makeSourcePackageReleaseFile(
919+ sourcepackagerelease=build.source_package_release)
920+ lfa_id = sprf.libraryfile.id
921+ build.updateStatus(BuildStatus.BUILDING)
922+ issuer = removeSecurityProxy(
923+ getUtility(IMacaroonIssuer, "binary-package-build"))
924+ macaroon = Macaroon(
925+ location="another-location", key=issuer._root_secret)
926+ logout()
927+ self.assertFalse(issuer.verifyMacaroon(macaroon, lfa_id))
928+
929+ def test_verifyMacaroon_wrong_key(self):
930+ build = self.factory.makeBinaryPackageBuild(
931+ archive=self.factory.makeArchive(private=True))
932+ sprf = self.factory.makeSourcePackageReleaseFile(
933+ sourcepackagerelease=build.source_package_release)
934+ lfa_id = sprf.libraryfile.id
935+ build.updateStatus(BuildStatus.BUILDING)
936+ issuer = removeSecurityProxy(
937+ getUtility(IMacaroonIssuer, "binary-package-build"))
938+ macaroon = Macaroon(
939+ location=config.vhost.mainsite.hostname, key="another-secret")
940+ logout()
941+ self.assertFalse(issuer.verifyMacaroon(macaroon, lfa_id))
942+
943+ def test_verifyMacaroon_not_building(self):
944+ build = self.factory.makeBinaryPackageBuild(
945+ archive=self.factory.makeArchive(private=True))
946+ sprf = self.factory.makeSourcePackageReleaseFile(
947+ sourcepackagerelease=build.source_package_release)
948+ lfa_id = sprf.libraryfile.id
949+ issuer = removeSecurityProxy(
950+ getUtility(IMacaroonIssuer, "binary-package-build"))
951+ macaroon = issuer.issueMacaroon(build)
952+ logout()
953+ self.assertFalse(issuer.verifyMacaroon(macaroon, lfa_id))
954+
955+ def test_verifyMacaroon_wrong_build(self):
956+ build = self.factory.makeBinaryPackageBuild(
957+ archive=self.factory.makeArchive(private=True))
958+ sprf = self.factory.makeSourcePackageReleaseFile(
959+ sourcepackagerelease=build.source_package_release)
960+ lfa_id = sprf.libraryfile.id
961+ build.updateStatus(BuildStatus.BUILDING)
962+ other_build = self.factory.makeBinaryPackageBuild(
963+ archive=self.factory.makeArchive(private=True))
964+ other_build.updateStatus(BuildStatus.BUILDING)
965+ issuer = removeSecurityProxy(
966+ getUtility(IMacaroonIssuer, "binary-package-build"))
967+ macaroon = issuer.issueMacaroon(other_build)
968+ logout()
969+ self.assertFalse(issuer.verifyMacaroon(macaroon, lfa_id))
970+
971+ def test_verifyMacaroon_wrong_file(self):
972+ build = self.factory.makeBinaryPackageBuild(
973+ archive=self.factory.makeArchive(private=True))
974+ self.factory.makeSourcePackageReleaseFile(
975+ sourcepackagerelease=build.source_package_release)
976+ lfa = self.factory.makeLibraryFileAlias()
977+ lfa_id = lfa.id
978+ build.updateStatus(BuildStatus.BUILDING)
979+ issuer = removeSecurityProxy(
980+ getUtility(IMacaroonIssuer, "binary-package-build"))
981+ macaroon = issuer.issueMacaroon(build)
982+ logout()
983+ self.assertFalse(issuer.verifyMacaroon(macaroon, lfa_id))