Launchpad itself

Merge lp:~wgrant/launchpad/make-zeca-useful into lp:launchpad

  1. make-zeca-useful
  2. Merge into devel
Proposed by William Grant
Status: Merged
Merged at revision: not available
Proposed branch: lp:~wgrant/launchpad/make-zeca-useful
Merge into: lp:launchpad
Diff against target: None lines
To merge this branch: bzr merge lp:~wgrant/launchpad/make-zeca-useful
Reviewer Review Type Date Requested Status
Celso Providelo (community) code Needs Fixing
Review via email: mp+10431@code.launchpad.net
To post a comment you must log in.
Revision history for this message
William Grant (wgrant) wrote :

= Summary =

canonical.zeca (the Launchpad testing keyserver) is very close to usable for a full local Launchpad instance. It allows key uploads, downloads and fingerprint searches, but doesn't support key ID searches (which gpg uses to retrieve unknown keys).

This branch makes it support key ID searches, and also moves the test zeca data directory to avoid clobbering development keys when running the test suite.

== Proposed fix ==

zeca should attempt to glob to match a key ID to a fingerprint.

== Pre-implementation notes ==

I discussed this a little with cprov, and he seemed to not think that this approach was entirely crackful.

== Implementation details ==

Nothing particularly special.

== Tests ==

I've extended the zeca tests to verify the new behaviour.

 $ bin/test -vv canonical.zeca

Revision history for this message
Celso Providelo (cprov) wrote :

Willian,

This change is definitely going in the right direction, thanks for proposing it!

On the current diff I can only point the fact that, when found by glob, the key file content is not escaped, which will result in a broken HTML page.

As we discussed on IRC, we could take the opportunity to extract the code used to locate key files and make this part of the application much nicer than it is, maybe plugging new key lookup mechanisms later.

I'd suggest locate_key(root, term), 'root' is static, but it might be easier to pass it in. Doing that we can also cover most of its aspects with unittests.

Looking forward to see the incremental patch.

review: Needs Fixing (code)
Revision history for this message
William Grant (wgrant) wrote :

On Wed, 2009-08-19 at 23:38 +0000, Celso Providelo wrote:
> Review: Needs Fixing code
> Willian,
>
> This change is definitely going in the right direction, thanks for proposing it!
>
> On the current diff I can only point the fact that, when found by glob, the key file content is not escaped, which will result in a broken HTML page.

Fixed. Oops.

> As we discussed on IRC, we could take the opportunity to extract the code used to locate key files and make this part of the application much nicer than it is, maybe plugging new key lookup mechanisms later.
>
> I'd suggest locate_key(root, term), 'root' is static, but it might be easier to pass it in. Doing that we can also cover most of its aspects with unittests.

I've extracted it into c.z.z.locate_key, with tests in
c.z.z.ftests.test_locate_key.

I considered removing some of the tests from c.z.z.ftests.harness, but
they're short and relevant enough that it seems good to leave them
there.

> Looking forward to see the incremental patch.

Thanks for the review! The diff is attached.

--
William Grant

take-two.diff
1=== added file 'lib/canonical/zeca/ftests/test_locate_key.py'
2--- lib/canonical/zeca/ftests/test_locate_key.py 1970-01-01 00:00:00 +0000
3+++ lib/canonical/zeca/ftests/test_locate_key.py 2009-08-20 01:01:54 +0000
4@@ -0,0 +1,43 @@
5+# Copyright 2009 Canonical Ltd. This software is licensed under the
6+# GNU Affero General Public License version 3 (see the file LICENSE).
7+
8+import unittest
9+import os.path
10+
11+from canonical.zeca.zeca import locate_key
12+
13+
14+class LocateKeyTestCase(unittest.TestCase):
15+ root = os.path.join(os.path.dirname(__file__), 'keys')
16+
17+ def assertKeyFile(self, suffix, filename):
18+ """Verify that a suffix maps to the given filename."""
19+
20+ if filename is not None:
21+ filename = os.path.join(self.root, filename)
22+ self.assertEqual(locate_key(self.root, suffix), filename)
23+
24+ def test_exact_fingerprint_match(self):
25+ self.assertKeyFile(
26+ '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get',
27+ '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get')
28+
29+ def test_keyid_glob_match(self):
30+ self.assertKeyFile(
31+ '0xDFD20543.get',
32+ '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get')
33+
34+ def test_keyid_without_prefix_glob_match(self):
35+ self.assertKeyFile(
36+ 'DFD20543.get',
37+ '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get')
38+
39+ def test_keyid_no_match(self):
40+ self.assertKeyFile('0xDEADBEEF.get', None)
41+
42+
43+def test_suite():
44+ suite = unittest.TestSuite()
45+ suite.addTest(unittest.makeSuite(LocateKeyTestCase))
46+ return suite
47+
48
49=== modified file 'lib/canonical/zeca/zeca.py'
50--- lib/canonical/zeca/zeca.py 2009-08-15 03:47:48 +0000
51+++ lib/canonical/zeca/zeca.py 2009-08-20 01:07:44 +0000
52@@ -9,12 +9,13 @@
53
54 - 'index' : returns key index information
55 - 'get': returns an ASCII armored public key
56-
57-It does not depend on GPG; it simply serves the information stored in
58-files at a given HOME (default to /home/keys/) with the following name
59-format:
60-
61-0x<keyid>.<operation>
62+ - 'add': adds a key to the collection (does not update the index)
63+
64+It only depends on GPG for key submission; for retrieval and searching
65+it just looks for files in the root (eg. /var/tmp/zeca). The files
66+are named like this:
67+
68+0x<keyid|fingerprint>.<operation>
69
70 Example:
71
72@@ -51,6 +52,33 @@
73 GREETING = 'Copyright 2004-2008 Canonical Ltd.\n'
74
75
76+def locate_key(root, suffix):
77+ """Find a key file in the root with the given suffix.
78+
79+ This does some globbing to possibly find a fingerprint-named key
80+ file when given a key ID.
81+
82+ :param root: The root directory in which to look.
83+ :param suffix: The key ID or fingerprint, of the form
84+ 0x<FINGERPRINT|KEYID>.<METHOD>
85+ :returns: An absolute path to the key file.
86+ """
87+ path = os.path.join(root, suffix)
88+
89+ if not os.path.exists(path):
90+ # GPG might request a key ID from us, but we name the keys by
91+ # fingerprint. Let's glob.
92+ if suffix.startswith('0x'):
93+ suffix = suffix[2:]
94+ keys = glob.glob(os.path.join(root, '*'+suffix))
95+ if len(keys) == 1:
96+ path = keys[0]
97+ else:
98+ return None
99+
100+ return path
101+
102+
103 class Zeca(Resource):
104 def getChild(self, name, request):
105 if name == '':
106@@ -96,22 +124,11 @@
107
108 filename = '%s.%s' % (keyid, action)
109
110- try:
111- path = os.path.join(self.root, filename)
112- fp = open(path)
113- except IOError:
114- # GPG might request a key ID from us, but we name the keys by
115- # fingerprint. Let's glob.
116- if filename.startswith('0x'):
117- filename = filename[2:]
118- keys = glob.glob(os.path.join(self.root, '*'+filename))
119- if len(keys) == 1:
120- content = open(keys[0]).read()
121- else:
122- content = 'Key Not Found'
123+ path = locate_key(self.root, filename)
124+ if path is not None:
125+ content = cgi.escape(open(path).read())
126 else:
127- content = cgi.escape(fp.read())
128- fp.close()
129+ content = 'Key Not Found'
130
131 page += '<pre>\n%s\n</pre>\n</html>' % content
132
Revision history for this message
Celso Providelo (cprov) wrote :
Download full text (7.7 KiB)

Hi Willian,

Thanks for addressing my comments, there are other minor suggestions
inline, but nothing special.

Review approved.

> === modified file 'configs/testrunner/launchpad-lazr.conf'
> --- configs/testrunner/launchpad-lazr.conf 2009-07-29 18:53:51 +0000
> +++ configs/testrunner/launchpad-lazr.conf 2009-08-20 13:09:20 +0000
> @@ -195,3 +195,6 @@
>
> [vhosts]
> use_https: False
> +
> +[zeca]
> +root: /var/tmp/zeca.test
>

Good change, I forgot to mention it before. Tests won't mess with you
production setup.

> === modified file 'lib/canonical/zeca/ftests/harness.py'
> --- lib/canonical/zeca/ftests/harness.py 2009-06-25 05:30:52 +0000
> +++ lib/canonical/zeca/ftests/harness.py 2009-08-20 13:09:20 +0000
> @@ -41,6 +41,8 @@
> ...
> <title>Results for Key 0xDFD20543</title>
> ...
> + pub 1024D/DFD20543 2005-04-13 Sample Person (revoked) &lt;<email address hidden>&gt;
> + ...
>
> A key content lookup form via GET.
>
> @@ -52,6 +54,41 @@
> ...
> <title>Results for Key 0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543</title>
> ...
> + -----BEGIN PGP PUBLIC KEY BLOCK-----
> + Version: GnuPG v1.4.9 (GNU/Linux)
> + <BLANKLINE>
> + mQGiBEJdmOcRBADkNJPTBuCIefBdRAhvWyD9SSVHh8GHQWS7l9sRLEsirQkKz1yB
> + ...
> +
> + We can also request a key ID instead of a fingerprint, and it will glob
> + for the fingerprint.
> +
> + >>> print urlopen(
> + ... '%s/pks/lookup?op=get&'
> + ... 'search=0xDFD20543' % root_url
> + ... ).read()
> + <html>
> + ...
> + <title>Results for Key 0xDFD20543</title>
> + ...
> + -----BEGIN PGP PUBLIC KEY BLOCK-----
> + Version: GnuPG v1.4.9 (GNU/Linux)
> + <BLANKLINE>
> + mQGiBEJdmOcRBADkNJPTBuCIefBdRAhvWyD9SSVHh8GHQWS7l9sRLEsirQkKz1yB
> + ...
> +
> + If we request a nonexistent key, we get a nice error.
> +
> + >>> print urlopen(
> + ... '%s/pks/lookup?op=get&'
> + ... 'search=0xDFD20544' % root_url
> + ... ).read()
> + <html>
> + ...
> + <title>Results for Key 0xDFD20544</title>
> + ...
> + Key Not Found
> + ...
>
> A key submit form via POST (see doc/gpghandler.txt for more information).
>

Cool! functional tests documenting key-lookups using keyids.

> === added file 'lib/canonical/zeca/ftests/test_locate_key.py'
> --- lib/canonical/zeca/ftests/test_locate_key.py 1970-01-01 00:00:00 +0000
> +++ lib/canonical/zeca/ftests/test_locate_key.py 2009-08-20 13:09:20 +0000
> @@ -0,0 +1,43 @@
> +# Copyright 2009 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +import unittest
> +import os.path
> +
> +from canonical.zeca.zeca import locate_key
> +
> +
> +class LocateKeyTestCase(unittest.TestCase):
> + root = os.path.join(os.path.dirname(__file__), 'keys')
> +
> + def assertKeyFile(self, suffix, filename):
> + """Verify that a suffix maps to the given filename."""
> +
> + if filename is not None:
> + filename = os.path.join(self.root, filename)
> + self.assertEqual(locate_key(self.root, suffix), filename)
> +
> + def test_exact_fingerprint_match(self):
> + ...

Read more...

Revision history for this message
William Grant (wgrant) wrote :

Hi Celso,

On Thu, 2009-08-20 at 13:36 +0000, Celso Providelo wrote:
> [snip]
> That's it, summing up, if you agree with the changes I suggested:
>
> 1. Rename new unittest test cases.
> 2. Update the visible copyright on zeca home page.
>
> we can land it.

I've fixed those, thanks! Diff attached.

take-three.diff
1=== modified file 'lib/canonical/zeca/ftests/harness.py'
2--- lib/canonical/zeca/ftests/harness.py 2009-08-15 03:46:59 +0000
3+++ lib/canonical/zeca/ftests/harness.py 2009-08-20 13:44:41 +0000
4@@ -29,7 +29,7 @@
5 >>> from urllib import urlopen
6
7 >>> print urlopen(root_url).read()
8- Copyright 2004-2008 Canonical Ltd.
9+ Copyright 2004-2009 Canonical Ltd.
10 <BLANKLINE>
11
12 A key index lookup form via GET.
13@@ -105,7 +105,7 @@
14 >>> ZecaTestSetup().setUp()
15
16 >>> print urlopen(root_url).readline()
17- Copyright 2004-2008 Canonical Ltd.
18+ Copyright 2004-2009 Canonical Ltd.
19 <BLANKLINE>
20
21 >>> ZecaTestSetup().tearDown()
22
23=== modified file 'lib/canonical/zeca/ftests/test_locate_key.py'
24--- lib/canonical/zeca/ftests/test_locate_key.py 2009-08-20 00:27:11 +0000
25+++ lib/canonical/zeca/ftests/test_locate_key.py 2009-08-20 13:50:23 +0000
26@@ -17,22 +17,22 @@
27 filename = os.path.join(self.root, filename)
28 self.assertEqual(locate_key(self.root, suffix), filename)
29
30- def test_exact_fingerprint_match(self):
31+ def test_locate_key_exact_fingerprint_match(self):
32 self.assertKeyFile(
33 '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get',
34 '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get')
35
36- def test_keyid_glob_match(self):
37+ def test_locate_key_keyid_glob_match(self):
38 self.assertKeyFile(
39 '0xDFD20543.get',
40 '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get')
41
42- def test_keyid_without_prefix_glob_match(self):
43+ def test_locate_key_keyid_without_prefix_glob_match(self):
44 self.assertKeyFile(
45 'DFD20543.get',
46 '0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543.get')
47
48- def test_keyid_no_match(self):
49+ def test_locate_key_no_match(self):
50 self.assertKeyFile('0xDEADBEEF.get', None)
51
52
53
54=== modified file 'lib/canonical/zeca/zeca.py'
55--- lib/canonical/zeca/zeca.py 2009-08-20 01:07:58 +0000
56+++ lib/canonical/zeca/zeca.py 2009-08-20 13:44:16 +0000
57@@ -49,7 +49,7 @@
58 SecretGPGKeyImportDetected)
59
60
61-GREETING = 'Copyright 2004-2008 Canonical Ltd.\n'
62+GREETING = 'Copyright 2004-2009 Canonical Ltd.\n'
63
64
65 def locate_key(root, suffix):

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'configs/testrunner/launchpad-lazr.conf'
2--- configs/testrunner/launchpad-lazr.conf 2009-07-29 18:53:51 +0000
3+++ configs/testrunner/launchpad-lazr.conf 2009-08-15 03:43:17 +0000
4@@ -195,3 +195,6 @@
5
6 [vhosts]
7 use_https: False
8+
9+[zeca]
10+root: /var/tmp/zeca.test
11
12=== modified file 'lib/canonical/zeca/ftests/harness.py'
13--- lib/canonical/zeca/ftests/harness.py 2009-06-25 05:30:52 +0000
14+++ lib/canonical/zeca/ftests/harness.py 2009-08-15 03:46:59 +0000
15@@ -41,6 +41,8 @@
16 ...
17 <title>Results for Key 0xDFD20543</title>
18 ...
19+ pub 1024D/DFD20543 2005-04-13 Sample Person (revoked) &lt;sample.revoked@canonical.com&gt;
20+ ...
21
22 A key content lookup form via GET.
23
24@@ -52,6 +54,41 @@
25 ...
26 <title>Results for Key 0xA419AE861E88BC9E04B9C26FBA2B9389DFD20543</title>
27 ...
28+ -----BEGIN PGP PUBLIC KEY BLOCK-----
29+ Version: GnuPG v1.4.9 (GNU/Linux)
30+ <BLANKLINE>
31+ mQGiBEJdmOcRBADkNJPTBuCIefBdRAhvWyD9SSVHh8GHQWS7l9sRLEsirQkKz1yB
32+ ...
33+
34+ We can also request a key ID instead of a fingerprint, and it will glob
35+ for the fingerprint.
36+
37+ >>> print urlopen(
38+ ... '%s/pks/lookup?op=get&'
39+ ... 'search=0xDFD20543' % root_url
40+ ... ).read()
41+ <html>
42+ ...
43+ <title>Results for Key 0xDFD20543</title>
44+ ...
45+ -----BEGIN PGP PUBLIC KEY BLOCK-----
46+ Version: GnuPG v1.4.9 (GNU/Linux)
47+ <BLANKLINE>
48+ mQGiBEJdmOcRBADkNJPTBuCIefBdRAhvWyD9SSVHh8GHQWS7l9sRLEsirQkKz1yB
49+ ...
50+
51+ If we request a nonexistent key, we get a nice error.
52+
53+ >>> print urlopen(
54+ ... '%s/pks/lookup?op=get&'
55+ ... 'search=0xDFD20544' % root_url
56+ ... ).read()
57+ <html>
58+ ...
59+ <title>Results for Key 0xDFD20544</title>
60+ ...
61+ Key Not Found
62+ ...
63
64 A key submit form via POST (see doc/gpghandler.txt for more information).
65
66
67=== modified file 'lib/canonical/zeca/zeca.py'
68--- lib/canonical/zeca/zeca.py 2009-06-25 05:30:52 +0000
69+++ lib/canonical/zeca/zeca.py 2009-08-15 03:47:48 +0000
70@@ -35,6 +35,7 @@
71 'Zeca',
72 ]
73
74+import glob
75 import os
76 import cgi
77
78@@ -95,12 +96,19 @@
79
80 filename = '%s.%s' % (keyid, action)
81
82- path = os.path.join(self.root, filename)
83-
84 try:
85+ path = os.path.join(self.root, filename)
86 fp = open(path)
87 except IOError:
88- content = 'Key Not Found'
89+ # GPG might request a key ID from us, but we name the keys by
90+ # fingerprint. Let's glob.
91+ if filename.startswith('0x'):
92+ filename = filename[2:]
93+ keys = glob.glob(os.path.join(self.root, '*'+filename))
94+ if len(keys) == 1:
95+ content = open(keys[0]).read()
96+ else:
97+ content = 'Key Not Found'
98 else:
99 content = cgi.escape(fp.read())
100 fp.close()