launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03780
[Merge] lp:~adeuring/launchpad/bug-486824 into lp:launchpad
Abel Deuring has proposed merging lp:~adeuring/launchpad/bug-486824 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~adeuring/launchpad/bug-486824/+merge/62883
This branch fixes bug 486824: LoginToken:+validategpg timeouts :
requests to the keyserver must have a timeout.
The core of the fix is trivial:
I added another config option, the timeout value for accesses of
the keyserver; this parameter is used when GPGHandler.retrueveKey()
tries to access the keyserver.
The value, 1 second, is a bit arbitrary and probably much larger
than necessary. IIRC, it is a socket timeout value, so it does not
limit the time between the begin of the HTTP request and the
complete delivery of the response; it is more like the maximum time
the client waits until it notices "some movement" from the server.
I played with smaller timeout values for running
url = (http://keyserver.ubuntu.com:11371/pks/lookup?'
'search=%s&op=%s') % ('0x6F0DAED1', 'get')
urllib2.urlopen(url, timeout=0.5).read()
on my development machine, and values as low as 0.05 seconds work fine.
The error handling in class GPGHandler was, let's say, a bit incomplete
and convoluted, so I refactored it for the method
GPGHandler.retrieveKey() and its helpers.
I defined two new execptions, GPGKeyTemporarilyNotFoundError and
GPGKeyDoesNotExistOnServer, both derived from GPGKeyNotFoundError.
The method GPGHandler._getPubKey() (only called by retireveKey()) now
checks for the exceptions urllib2.HTTPError and urllib2.URLError and
raise one of the new execptions, depending on one detail:
The keyserver responds with a 500 error for request to retrieve
a key which is not stored on the server. (really, I am not joking:
try the URL from the example above...)
An informational OOPS is logged when GPGKeyTemporarilyNotFoundError
is raised: While it is not a coding issue, it is clearly an
"operational problem" which needs fixing if it occurs too often.
Testing the "except urllib2.HTTPError" part required some modifications
of the test key server: Like the real keyserver, it should respond with
a 500 status code when a non-existent key is requested.
Finally, one existing test required a modification,
lib/canonical/launchpad/doc/gpghandler.txt.
Getting the GPGKeyDoesNotExistOnServer for GPGHandler.uploadPublicKey()
is a bit weird, but I think it is better to raise more specific
errors in GPGHandler.retrieveKey() than to avoid the response "sorry,
the keyserver does not know about the key you want to upload to
the keyserver". After all, the latter is caused by the fact that
uploadKey() calls retrieveKey(), which in turn tries to get the key
from the server if it is not stored locally. We could explicitly
prevent this by adding an optional parameter "no_server_retrieval" to
retrieveKey() -- but this branch started with a slightly different
problem, so I did not want to modify retrieveKey() and uploadKey() even
further. And for follow-up work, I think it makes more sense to actually
use the new execption GPGKeyDoesNotExistOnServer to give users a better
clue why something did not work than to fix uploadKey().
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/canonical/config/schema-lazr.conf
lib/canonical/launchpad/doc/gpghandler.txt
lib/canonical/launchpad/interfaces/gpghandler.py
lib/canonical/launchpad/utilities/gpghandler.py
lib/canonical/launchpad/utilities/ftests/test_gpghandler.py
lib/lp/testing/keyserver/web.py
lib/lp/testing/keyserver/tests/test_web.py
./lib/canonical/config/schema-lazr.conf
538: Line exceeds 78 characters.
621: Line exceeds 78 characters.
1001: Line exceeds 78 characters.
1094: Line exceeds 78 characters.
./lib/canonical/launchpad/doc/gpghandler.txt
1: narrative uses a moin header.
18: narrative uses a moin header.
136: narrative uses a moin header.
176: narrative uses a moin header.
263: narrative uses a moin header.
272: want exceeds 78 characters.
277: want exceeds 78 characters.
283: want exceeds 78 characters.
286: narrative uses a moin header.
337: narrative uses a moin header.
./lib/lp/testing/keyserver/tests/test_web.py
117: Line exceeds 78 characters.
117: E501 line too long (91 characters)
--
https://code.launchpad.net/~adeuring/launchpad/bug-486824/+merge/62883
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~adeuring/launchpad/bug-486824 into lp:launchpad.
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf 2011-05-06 13:55:19 +0000
+++ lib/canonical/config/schema-lazr.conf 2011-05-30 13:53:32 +0000
@@ -985,6 +985,10 @@
# datatype: int
port: 11371
+# The socket timeout value used for connections to the keyserver.
+# datatype: float
+timeout: 1.0
+
[haproxy_status_view]
# Configuration for the +haproxy monitoring URL
=== modified file 'lib/canonical/launchpad/doc/gpghandler.txt'
--- lib/canonical/launchpad/doc/gpghandler.txt 2011-02-17 15:52:05 +0000
+++ lib/canonical/launchpad/doc/gpghandler.txt 2011-05-30 13:53:32 +0000
@@ -315,8 +315,8 @@
>>> gpghandler.uploadPublicKey('F' * 40)
Traceback (most recent call last):
...
- GPGKeyNotFoundError: No GPG key found with the given content:
- ...
+ GPGKeyDoesNotExistOnServer: GPG key
+ FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF does not exist on the keyserver.
Uploading the same key more than once is fine, it is handled on the
keyserver side.
=== modified file 'lib/canonical/launchpad/interfaces/gpghandler.py'
--- lib/canonical/launchpad/interfaces/gpghandler.py 2011-05-16 23:59:05 +0000
+++ lib/canonical/launchpad/interfaces/gpghandler.py 2011-05-30 13:53:32 +0000
@@ -10,9 +10,11 @@
__all__ = [
+ 'GPGKeyDoesNotExistOnServer',
'GPGKeyExpired',
'GPGKeyRevoked',
'GPGKeyNotFoundError',
+ 'GPGKeyTemporarilyNotFoundError',
'GPGUploadFailure',
'GPGVerificationError',
'IGPGHandler',
@@ -32,13 +34,41 @@
class GPGKeyNotFoundError(Exception):
- """The given GPG key was not found in the keyserver."""
+ """The GPG key with the given fingerprint was not found on the keyserver.
+ """
- def __init__(self, fingerprint, pubkey=None):
+ def __init__(self, fingerprint, message=None):
self.fingerprint = fingerprint
- self.pubkey = pubkey
- super(GPGKeyNotFoundError, self).__init__(
+ if message is None:
+ message = (
"No GPG key found with the given content: %s" % (fingerprint, ))
+ super(GPGKeyNotFoundError, self).__init__(message)
+
+
+class GPGKeyTemporarilyNotFoundError(GPGKeyNotFoundError):
+ """The GPG key with the given fingerprint was not found on the keyserver.
+
+ The reason is a timeout while accessing the server, a general
+ server error, a network problem or some other temporary issue.
+ """
+ def __init__(self, fingerprint):
+ message = (
+ "GPG key %s not found due to a server or network failure."
+ % fingerprint)
+ super(GPGKeyTemporarilyNotFoundError, self).__init__(
+ fingerprint, message)
+
+
+class GPGKeyDoesNotExistOnServer(GPGKeyNotFoundError):
+ """The GPG key with the given fingerprint was not found on the keyserver.
+
+ The server returned an explicit "not found".
+ """
+ def __init__(self, fingerprint):
+ message = (
+ "GPG key %s does not exist on the keyserver." % fingerprint)
+ super(GPGKeyDoesNotExistOnServer, self).__init__(
+ fingerprint, message)
class GPGKeyRevoked(Exception):
=== modified file 'lib/canonical/launchpad/utilities/ftests/test_gpghandler.py'
--- lib/canonical/launchpad/utilities/ftests/test_gpghandler.py 2011-05-16 23:59:05 +0000
+++ lib/canonical/launchpad/utilities/ftests/test_gpghandler.py 2011-05-30 13:53:32 +0000
@@ -9,29 +9,37 @@
from math import floor
import os
from time import time
-import unittest
import gpgme
from pytz import UTC
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
+from canonical.config import config
from canonical.launchpad.ftests import (
ANONYMOUS,
keys_for_tests,
login,
logout,
)
-from canonical.launchpad.interfaces.gpghandler import IGPGHandler
+from canonical.launchpad.interfaces.gpghandler import (
+ GPGKeyDoesNotExistOnServer,
+ GPGKeyTemporarilyNotFoundError,
+ IGPGHandler,
+ )
+from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
from canonical.testing.layers import LaunchpadFunctionalLayer
-
-
-class TestImportKeyRing(unittest.TestCase):
+from lp.testing import TestCase
+from lp.testing.keyserver import KeyServerTac
+
+
+class TestImportKeyRing(TestCase):
"""Tests for keyring imports"""
layer = LaunchpadFunctionalLayer
def setUp(self):
"""Get a gpghandler and login"""
+ super(TestImportKeyRing, self).setUp()
login(ANONYMOUS)
self.gpg_handler = getUtility(IGPGHandler)
self.gpg_handler.resetLocalState()
@@ -42,6 +50,7 @@
# This should be a zope test cleanup thing per SteveA.
self.gpg_handler.resetLocalState()
logout()
+ super(TestImportKeyRing, self).tearDown()
def populateKeyring(self):
for email in keys_for_tests.iter_test_key_emails():
@@ -208,3 +217,37 @@
gpghandler.touchConfigurationDirectory()
for fname in files_to_check:
self.assertTrue(now <= floor(os.path.getmtime(fname)))
+
+ def test_retrieveKey_raises_GPGKeyDoesNotExistOnServer(self):
+ # GPGHandler.retrieveKey() raises GPGKeyDoesNotExistOnServer
+ # when called for a key that does not exist on the key server.
+ tac = KeyServerTac()
+ tac.setUp()
+ self.addCleanup(tac.tearDown)
+ gpghandler = getUtility(IGPGHandler)
+ self.assertRaises(
+ GPGKeyDoesNotExistOnServer, gpghandler.retrieveKey,
+ 'non-existent-fp')
+
+ def test_retrieveKey_raises_GPGKeyTemporarilyNotFoundError_for_timeout(
+ self):
+ # If the keyserver responds too slowly, GPGHandler.retrieveKey()
+ # raises GPGKeyTemporarilyNotFoundError.
+ config.push(
+ 'short keyserver timeout',
+ """
+ [gpghandler]
+ timeout: 0.000001""")
+ tac = KeyServerTac()
+ tac.setUp()
+ self.addCleanup(tac.tearDown)
+ gpghandler = getUtility(IGPGHandler)
+ self.assertRaises(
+ GPGKeyTemporarilyNotFoundError, gpghandler.retrieveKey,
+ 'non-existent-fp')
+ # An informational OOPS report is generated for the timeout.
+ error_utility = ErrorReportingUtility()
+ error_report = error_utility.getLastOopsReport()
+ self.assertTrue(error_report.informational)
+ self.assertEqual('URLError', error_report.type)
+ self.assertEqual('<urlopen error timed out>', error_report.value)
=== modified file 'lib/canonical/launchpad/utilities/gpghandler.py'
--- lib/canonical/launchpad/utilities/gpghandler.py 2011-05-17 04:48:34 +0000
+++ lib/canonical/launchpad/utilities/gpghandler.py 2011-05-30 13:53:32 +0000
@@ -11,13 +11,14 @@
]
import atexit
+import errno
import httplib
import os
-import re
import shutil
import socket
from StringIO import StringIO
import subprocess
+import sys
import tempfile
import urllib
import urllib2
@@ -28,9 +29,11 @@
from canonical.config import config
from canonical.launchpad.interfaces.gpghandler import (
+ GPGKeyDoesNotExistOnServer,
GPGKeyExpired,
GPGKeyNotFoundError,
GPGKeyRevoked,
+ GPGKeyTemporarilyNotFoundError,
GPGUploadFailure,
GPGVerificationError,
IGPGHandler,
@@ -40,6 +43,8 @@
MoreThanOneGPGKeyFound,
SecretGPGKeyImportDetected,
)
+from canonical.launchpad.webapp import errorlog
+from canonical.lazr.utils import get_current_browser_request
from lp.app.validators.email import valid_email
from lp.registry.interfaces.gpg import (
GPGKeyAlgorithm,
@@ -91,6 +96,7 @@
conf.close()
# create a local atexit handler to remove the configuration directory
# on normal termination.
+
def removeHome(home):
"""Remove GNUPGHOME directory."""
if os.path.exists(home):
@@ -125,7 +131,7 @@
try:
os.utime(os.path.join(self.home, file), None)
except OSError as e:
- if e.errno == ENOENT:
+ if e.errno == errno.ENOENT:
# The file has been deleted.
pass
else:
@@ -434,15 +440,7 @@
# key ring, but it needs "specing"
key = PymeKey(fingerprint.encode('ascii'))
if not key.exists_in_local_keyring:
- result, pubkey = self._getPubKey(fingerprint)
- if not result:
- if "Connection refused" in pubkey:
- raise AssertionError(
- "The keyserver is not running, help!")
- else:
- raise GPGKeyNotFoundError(fingerprint, pubkey)
-
- # Import in the local key ring
+ pubkey = self._getPubKey(fingerprint)
key = self.importPublicKey(pubkey)
return key
@@ -501,55 +499,35 @@
return 'http://%s:%s/pks/lookup?%s' % (host, config.gpghandler.port,
urllib.urlencode(params))
- def _getKeyIndex(self, fingerprint):
- """See IGPGHandler for further information."""
- # Grab Page from keyserver
- result, page = self._grabPage('index', fingerprint)
-
- if not result:
- return result, page
-
- # regexps to extract information
- htmltag_re = re.compile('<[^>]+>')
- keyinfo_re = re.compile('([\d]*)([RgDG])\/([\dABCDEF]*)')
- emailaddresses_re = re.compile('[^;]+@[^&]*')
-
- # clean html tags from page
- page = htmltag_re.sub('', page)
-
- # extract key info as [(size, type, id)]
- keyinfo = keyinfo_re.findall(page)
- # extract UIDs as sorted list
- uids = emailaddresses_re.findall(page)
-
- # sort the UID list
- uids.sort()
-
- return keyinfo, uids
-
def _getPubKey(self, fingerprint):
"""See IGPGHandler for further information."""
- return self._grabPage('get', fingerprint)
+ try:
+ return self._grabPage('get', fingerprint)
+ except urllib2.HTTPError, exc:
+ # The key server behaves a bit odd when queried for non
+ # existent keys: Instead of responding with a 404, it
+ # returns a 500 error. But we can extract the fact that
+ # the key is unknown by looking into the response's content.
+ if exc.code == 500 and exc.fp is not None:
+ content = exc.fp.read()
+ no_key_message = 'Error handling request: No keys found'
+ if content.find(no_key_message) >= 0:
+ raise GPGKeyDoesNotExistOnServer(fingerprint)
+ errorlog.globalErrorUtility.handling(
+ sys.exc_info(), get_current_browser_request())
+ raise GPGKeyTemporarilyNotFoundError(fingerprint)
+ except urllib2.URLError, exc:
+ errorlog.globalErrorUtility.handling(
+ sys.exc_info(), get_current_browser_request())
+ raise GPGKeyTemporarilyNotFoundError(fingerprint)
def _grabPage(self, action, fingerprint):
"""Wrapper to collect KeyServer Pages."""
- # XXX cprov 2005-05-16:
- # What if something went wrong ?
- # 1 - Not Found
- # 2 - Revoked Key
- # 3 - Server Error (solved with urllib2.HTTPError exception)
- # it needs more love
url = self.getURLForKeyInServer(fingerprint, action)
- # read and store html page
- try:
- f = urllib2.urlopen(url)
- except urllib2.URLError, e:
- return False, '%s at %s' % (e, url)
-
+ f = urllib2.urlopen(url, timeout=float(config.gpghandler.timeout))
page = f.read()
f.close()
-
- return True, page
+ return page
def checkTrustDb(self):
"""See IGPGHandler"""
=== modified file 'lib/lp/testing/keyserver/tests/test_web.py'
--- lib/lp/testing/keyserver/tests/test_web.py 2011-02-17 15:05:52 +0000
+++ lib/lp/testing/keyserver/tests/test_web.py 2011-05-30 13:53:32 +0000
@@ -11,6 +11,7 @@
from testtools.deferredruntest import AsynchronousDeferredRunTest
from twisted.internet.endpoints import serverFromString
+from twisted.python.failure import Failure
from twisted.web.client import getPage
from twisted.web.server import Site
@@ -20,6 +21,11 @@
from lp.testing.matchers import DocTestMatches
+class RegularCallbackExecuted(Exception):
+ """Raised if a regular Twisted callback is called when the request
+ is supposed to return an HTTP error."""
+
+
class TestWebResources(TestCase):
run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=2)
@@ -49,6 +55,7 @@
def getURL(self, path):
"""Start a test key server and get the content at 'path'."""
d = self.makeService()
+
def service_started(port):
self.addCleanup(port.stopListening)
return self.fetchResource(port, path)
@@ -59,6 +66,45 @@
d = self.getURL(path)
return d.addCallback(self.assertThat, DocTestMatches(content))
+ def assertRaises500ErrorForKeyNotFound(self, path):
+ """Assert that the test server returns a 500 response
+ for attempts to retrieve an unknown key.
+ ."""
+ d = self.getURL(path)
+
+ def regular_execution_callback(content):
+ # A really Twisted(tm) error check:
+ #
+ # This callback should _not_ be called, because setting
+ # the HTTP status code to 500 in Lookup.processRequest()
+ # prevents this. On the other hand, if the status code is
+ # _not_ set, the callback check_error_details below is
+ # is not executed, and the test would simply pass, while
+ # it shouldn't.
+ #
+ # So we should assert that this callback is not executed.
+ # But raising an exception here leads again to the
+ # execution of check_error_details() -- for this exception.
+ # So we can't simply call self.fail(), but we can raise
+ # a custom exception and we can check in the error
+ # callback that this exception was _not_ raised.
+ raise RegularCallbackExecuted
+
+ def check_error_details(failure):
+ if isinstance(failure.value, RegularCallbackExecuted):
+ self.fail('Response was not an HTTP error response.')
+ if not isinstance(failure, Failure):
+ raise failure
+ self.assertEqual('500', failure.value.status)
+ self.assertEqual(
+ '<html><head><title>Error handling request</title></head>\n'
+ '<body><h1>Error handling request</h1>'
+ 'Error handling request: No keys found</body></html>',
+ failure.value.response)
+
+ d.addCallback(regular_execution_callback)
+ return d.addErrback(check_error_details)
+
def test_index_lookup(self):
# A key index lookup form via GET.
return self.assertContentMatches(
@@ -108,16 +154,8 @@
def test_nonexistent_key(self):
# If we request a nonexistent key, we get a nice error.
- return self.assertContentMatches(
- '/pks/lookup?op=get&search=0xDFD20544',
- '''\
-<html>
-...
-<title>Results for Key 0xDFD20544</title>
-...
-Key Not Found
-...
-''')
+ return self.assertRaises500ErrorForKeyNotFound(
+ '/pks/lookup?op=get&search=0xDFD20544')
def test_add_key(self):
# A key submit form via POST (see doc/gpghandler.txt for more
=== modified file 'lib/lp/testing/keyserver/web.py'
--- lib/lp/testing/keyserver/web.py 2011-02-17 13:52:17 +0000
+++ lib/lp/testing/keyserver/web.py 2011-05-30 13:53:32 +0000
@@ -69,7 +69,7 @@
# fingerprint. Let's glob.
if suffix.startswith('0x'):
suffix = suffix[2:]
- keys = glob.glob(os.path.join(root, '*'+suffix))
+ keys = glob.glob(os.path.join(root, '*' + suffix))
if len(keys) == 1:
path = keys[0]
else:
@@ -110,6 +110,12 @@
return 'Welcome To Fake SKS service.\n'
+KEY_NOT_FOUND_BODY = (
+ "<html><head><title>Error handling request</title></head>\n"
+ "<body><h1>Error handling request</h1>Error handling request: "
+ "No keys found</body></html>")
+
+
class LookUp(Resource):
isLeaf = True
@@ -126,29 +132,28 @@
except KeyError:
return 'Invalid Arguments %s' % request.args
- return self.processRequest(action, keyid)
+ return self.processRequest(action, keyid, request)
- def processRequest(self, action, keyid):
+ def processRequest(self, action, keyid, request):
if (action not in self.permitted_actions) or not keyid:
return 'Forbidden: "%s" on ID "%s"' % (action, keyid)
- page = ('<html>\n<head>\n'
- '<title>Results for Key %s</title>\n'
- '</head>\n<body>'
- '<h1>Results for Key %s</h1>\n'
- % (keyid, keyid))
-
filename = '%s.%s' % (keyid, action)
path = locate_key(self.root, filename)
if path is not None:
content = cgi.escape(open(path).read())
+ page = ('<html>\n<head>\n'
+ '<title>Results for Key %s</title>\n'
+ '</head>\n<body>'
+ '<h1>Results for Key %s</h1>\n'
+ '<pre>\n%s\n</pre>\n</html>') % (keyid, keyid, content)
+ return page
else:
- content = 'Key Not Found'
-
- page += '<pre>\n%s\n</pre>\n</html>' % content
-
- return page
+ # No joke: our real-world keyserver returns a 500 error
+ # if it does not know about a key with the given ID.
+ request.setResponseCode(500)
+ return KEY_NOT_FOUND_BODY
SUBMIT_KEY_PAGE = """
