launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #06882
[Merge] lp:~wgrant/launchpad/neuter-poppy-db into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/neuter-poppy-db into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #798957 in Launchpad itself: "PPA Uploads are seemingly (but not actually) rejected"
https://bugs.launchpad.net/launchpad/+bug/798957
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/neuter-poppy-db/+merge/99258
Rip out poppy's GPG verification and DB access. The current implementation is disabled (due to bug #798957) and would be better off reimplemented as a service client.
--
https://code.launchpad.net/~wgrant/launchpad/neuter-poppy-db/+merge/99258
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/neuter-poppy-db into lp:launchpad.
=== modified file 'daemons/poppy-sftp.tac'
--- daemons/poppy-sftp.tac 2012-02-01 15:08:19 +0000
+++ daemons/poppy-sftp.tac 2012-03-26 07:12:35 +0000
@@ -18,15 +18,10 @@
from zope.interface import implements
-from lp.services.config import (
- config,
- dbconfig,
- )
+from lp.services.config import config
from lp.services.daemons import readyservice
-from lp.services.scripts import execute_zcml_for_scripts
from lp.poppy import get_poppy_root
-from lp.poppy.twistedconfigreset import GPGHandlerConfigResetJob
from lp.poppy.twistedftp import (
FTPServiceFactory,
)
@@ -38,10 +33,6 @@
from lp.services.twistedsupport.loggingsupport import set_up_oops_reporting
-# Use a unique db user per policy and Bug #732510.
-dbconfig.override(dbuser='poppy_sftp')
-
-
def make_portal():
"""Create and return a `Portal` for the SSH service.
@@ -124,11 +115,5 @@
banner=config.poppy.banner)
svc.setServiceParent(application)
-# We need Zope for looking up the GPG utilities.
-execute_zcml_for_scripts()
-
-# Set up the GPGHandler job
-GPGHandlerConfigResetJob().setServiceParent(application)
-
# Service that announces when the daemon is ready
readyservice.ReadyService().setServiceParent(application)
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2012-03-21 19:01:20 +0000
+++ database/schema/security.cfg 2012-03-26 07:12:35 +0000
@@ -958,12 +958,6 @@
type=user
groups=fiera
-[poppy_sftp]
-# This really should be inheriting from something more specific to the
-# soyuz realm.
-groups=launchpad_main
-type=user
-
[ppa-apache-log-parser]
groups=script
public.archive = SELECT
=== modified file 'lib/lp/poppy/tests/test_poppy.py'
--- lib/lp/poppy/tests/test_poppy.py 2012-02-01 14:30:51 +0000
+++ lib/lp/poppy/tests/test_poppy.py 2012-03-26 07:12:35 +0000
@@ -5,7 +5,6 @@
__metaclass__ = type
-import ftplib
import os
import shutil
import stat
@@ -32,8 +31,6 @@
from lp.services.config import config
from lp.services.daemons.tachandler import TacTestSetup
from lp.testing import TestCaseWithFactory
-from lp.testing.dbuser import switch_dbuser
-from lp.testing.keyserver import KeyServerTac
from lp.testing.layers import (
ZopelessAppServerLayer,
ZopelessDatabaseLayer,
@@ -194,7 +191,6 @@
"""Set up poppy in a temp dir."""
super(TestPoppy, self).setUp()
self.root_dir = self.makeTemporaryDirectory()
- switch_dbuser('poppy_sftp')
self.server = self.server_factory(self.root_dir, self.factory)
self.useFixture(self.server)
@@ -373,38 +369,6 @@
self.root_dir, upload_dirs[index], "test")).read()
self.assertEqual(content, expected_contents[index])
- # XXX: deryck, 2012-01-26, Bug 798957
- # PoppyFileWriter.close has been disabled, so disable test, too.
- def disabled_test_bad_gpg_on_changesfile(self):
- """Check that we get a rejection error when uploading .changes files
- with invalid GPG signatures.
- """
- # Start up the poppy server.
- self.server.waitForStartUp()
-
- transport = self.server.getTransport()
- if transport.external_url().startswith("sftp"):
- # We're not GPG-checking sftp uploads right now.
- return
-
- # Start up the test keyserver.
- tac = KeyServerTac()
- tac.setUp()
- self.addCleanup(tac.tearDown)
-
- fake_file = StringIO.StringIO("fake contents")
-
- # We can't use bzrlib's transport here because it uploads a
- # renamed file before renaming it on the server.
- f = ftplib.FTP()
- f.connect(host="localhost", port=config.poppy.ftp_port)
- f.login()
- self.assertRaises(
- ftplib.error_perm,
- f.storbinary,
- 'STOR ' + 'foo_source.changes',
- fake_file)
-
def test_suite():
tests = unittest.TestLoader().loadTestsFromName(__name__)
=== removed file 'lib/lp/poppy/tests/test_twistedconfigreset.py'
--- lib/lp/poppy/tests/test_twistedconfigreset.py 2012-01-01 02:58:52 +0000
+++ lib/lp/poppy/tests/test_twistedconfigreset.py 1970-01-01 00:00:00 +0000
@@ -1,31 +0,0 @@
-# Copyright 2011 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Tests for GPGHandler config reset job."""
-
-__metaclass__ = type
-
-
-from lp.poppy.twistedconfigreset import GPGHandlerConfigResetJob
-from lp.testing import TestCase
-from lp.testing.layers import ZopelessLayer
-
-
-class TestGPGHandlerConfigResetJob(TestCase):
-
- layer = ZopelessLayer
-
- def test_gpghandler_config_reset_job_setup(self):
- # Does the gpghandler job get setup correctly.
-
- job_instance = GPGHandlerConfigResetJob()
- job_instance.startService()
- self.assertIsNot(None, job_instance._gpghandler_job)
- self.assertTrue(job_instance._gpghandler_job.running)
-
- # It should be scheduled for every 12 hours.
- self.assertEqual(12 * 3600, job_instance._gpghandler_job.interval)
-
- # We should be able to stop the job.
- job_instance.stopService()
- self.assertFalse(job_instance._gpghandler_job.running)
=== removed file 'lib/lp/poppy/tests/test_twistedftp.py'
--- lib/lp/poppy/tests/test_twistedftp.py 2012-01-27 19:00:36 +0000
+++ lib/lp/poppy/tests/test_twistedftp.py 1970-01-01 00:00:00 +0000
@@ -1,111 +0,0 @@
-# Copyright 2011 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""Tests for Twisted Poppy FTP."""
-
-__metaclass__ = type
-
-import os
-
-from testtools.deferredruntest import AsynchronousDeferredRunTest
-import transaction
-from twisted.protocols import ftp
-from zope.component import getUtility
-
-from lp.poppy.twistedftp import PoppyFileWriter
-from lp.registry.interfaces.gpg import (
- GPGKeyAlgorithm,
- IGPGKeySet,
- )
-from lp.services.config import config
-from lp.services.database.isolation import check_no_transaction
-from lp.services.gpg.interfaces import IGPGHandler
-from lp.testing import TestCaseWithFactory
-from lp.testing.gpgkeys import gpgkeysdir
-from lp.testing.keyserver import KeyServerTac
-from lp.testing.layers import ZopelessDatabaseLayer
-
-
-class TestPoppyFileWriter(TestCaseWithFactory):
-
- layer = ZopelessDatabaseLayer
- run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=20)
-
- def setUp(self):
- TestCaseWithFactory.setUp(self)
-
- # Start the test keyserver. Starting up and tearing this down
- # for each test is expensive, but we don't have a way of having
- # cross-test persistent fixtures yet. See bug 724349.
- self.tac = KeyServerTac()
- self.tac.setUp()
- self.addCleanup(self.tac.tearDown)
-
- # Load a key.
- gpg_handler = getUtility(IGPGHandler)
- key_path = os.path.join(gpgkeysdir, 'ftpmaster@xxxxxxxxxxxxxxxxx')
- key_data = open(key_path).read()
- key = gpg_handler.importPublicKey(key_data)
- assert key is not None
-
- # Make a new user and add the above key to it.
- user = self.factory.makePerson()
- key_set = getUtility(IGPGKeySet)
- user_key = key_set.new(
- ownerID=user.id, keyid=key.keyid, fingerprint=key.fingerprint,
- algorithm=GPGKeyAlgorithm.items[key.algorithm],
- keysize=key.keysize, can_encrypt=key.can_encrypt,
- active=True)
- # validateGPG runs in its own transaction.
- transaction.commit()
-
- # Locate the directory with test files.
- self.test_files_dir = os.path.join(
- config.root, "lib/lp/soyuz/scripts/tests/upload_test_files/")
-
- def test_changes_file_with_valid_GPG(self):
- valid_changes_file = os.path.join(
- self.test_files_dir, "etherwake_1.08-1_source.changes")
-
- def callback(result):
- self.assertIs(None, result)
-
- with open(valid_changes_file) as opened_file:
- file_writer = PoppyFileWriter(opened_file)
- d = file_writer.close()
- d.addBoth(callback)
- return d
-
- # XXX: deryck, 2012-01-26, Bug 798957
- # Disable as we search for a better fix to bug.
- def disabled_test_changes_file_with_invalid_GPG(self):
- invalid_changes_file = os.path.join(
- self.test_files_dir, "broken_source.changes")
-
- def error_callback(failure):
- self.assertTrue(failure.check, ftp.PermissionDeniedError)
- self.assertIn(
- "Changes file must be signed with a valid GPG signature",
- failure.getErrorMessage())
-
- def success_callback(result):
- self.fail("Success when there should have been failure.")
-
- with open(invalid_changes_file) as opened_file:
- file_writer = PoppyFileWriter(opened_file)
- d = file_writer.close()
- d.addCallbacks(success_callback, error_callback)
- return d
-
- def test_aborts_transaction(self):
- valid_changes_file = os.path.join(
- self.test_files_dir, "etherwake_1.08-1_source.changes")
-
- def callback(result):
- check_no_transaction()
-
- with open(valid_changes_file) as opened_file:
- file_writer = PoppyFileWriter(opened_file)
- d = file_writer.close()
- d.addBoth(callback)
- return d
=== removed file 'lib/lp/poppy/twistedconfigreset.py'
--- lib/lp/poppy/twistedconfigreset.py 2011-12-09 00:20:44 +0000
+++ lib/lp/poppy/twistedconfigreset.py 1970-01-01 00:00:00 +0000
@@ -1,49 +0,0 @@
-# Copyright 2011 Canonical Ltd. This software is licensed under the
-# GNU Affero General Public License version 3 (see the file LICENSE).
-
-"""A Twisted job to touch the GPGHandler config files."""
-
-__metaclass__ = type
-__all__ = [
- 'GPGHandlerConfigResetJob',
- ]
-
-from twisted.application.service import Service
-from twisted.internet import task
-from twisted.internet.error import AlreadyCancelled
-from zope.component import getUtility
-from zope.component.interfaces import ComponentLookupError
-
-from lp.services.gpg.interfaces import IGPGHandler
-
-
-class GPGHandlerConfigResetJob(Service):
- """Manages twisted job to touch the files in the gpgconfig directory."""
-
- def startService(self):
- self._gpghandler_job = None
- # start the GPGHandler job
- self._scheduleGPGHandlerJob()
- Service.startService(self)
-
- def stopService(self):
- self._stopGPGHandlerJob()
- Service.stopService(self)
-
- def _scheduleGPGHandlerJob(self, touch_interval=12 * 3600):
- # Create a job to touch the GPGHandler home directory every so often
- # so that it does not get cleaned up by any reaper scripts which look
- # at time last modified.
-
- self._stopGPGHandlerJob()
- self._gpghandler_job = task.LoopingCall(
- getUtility(IGPGHandler).touchConfigurationDirectory)
- return self._gpghandler_job.start(touch_interval)
-
- def _stopGPGHandlerJob(self):
- try:
- if self._gpghandler_job and self._gpghandler_job.running:
- self._gpghandler_job.stop()
- except AlreadyCancelled:
- # So we're already cancelled, meh.
- pass
=== modified file 'lib/lp/poppy/twistedftp.py'
--- lib/lp/poppy/twistedftp.py 2012-01-26 15:52:04 +0000
+++ lib/lp/poppy/twistedftp.py 2012-03-26 07:12:35 +0000
@@ -34,13 +34,7 @@
from lp.poppy import get_poppy_root
from lp.poppy.filesystem import UploadFileSystem
from lp.poppy.hooks import Hooks
-from lp.registry.interfaces.gpg import IGPGKeySet
from lp.services.config import config
-from lp.services.database import read_transaction
-from lp.services.gpg.interfaces import (
- GPGVerificationError,
- IGPGHandler,
- )
class PoppyAccessCheck:
@@ -87,15 +81,7 @@
"""
filename = os.sep.join(file_segments)
self._create_missing_directories(filename)
- path = self._path(file_segments)
- try:
- fObj = path.open("w")
- except (IOError, OSError), e:
- return ftp.errnoToFailure(e.errno, path)
- except:
- # Push any other error up to Twisted to deal with.
- return defer.fail()
- return defer.succeed(PoppyFileWriter(fObj))
+ return super(PoppyAnonymousShell, self).openForWriting(file_segments)
def makeDirectory(self, path):
"""Make a directory using the secure `UploadFileSystem`."""
@@ -151,51 +137,6 @@
"Only IFTPShell interface is supported by this realm")
-class PoppyFileWriter(ftp._FileWriter):
- """An `IWriteFile` that checks for signed changes files."""
-
- # XXX: deryck, 2012-01-26, Bug 798957
- # Disable close() as we search for a better fix to bug.
- def disabled_close(self):
- """Called after the file has been completely downloaded."""
- if self.fObj.name.endswith(".changes"):
- error = self.validateGPG(self.fObj.name)
- if error is not None:
- # PermissionDeniedError is one of the few ftp exceptions
- # that lets us pass an error string back to the client.
- return defer.fail(ftp.PermissionDeniedError(error))
- return defer.succeed(None)
-
- @read_transaction
- def validateGPG(self, signed_file):
- """Check the GPG signature in the file referenced by signed_file.
-
- Return an error string if there's a problem, or None.
- """
- try:
- sig = getUtility(IGPGHandler).getVerifiedSignatureResilient(
- file(signed_file, "rb").read())
- except GPGVerificationError, error:
- log = logging.getLogger("poppy-sftp")
- log.info("GPGVerificationError, extra debug output follows:")
- for attr in ("args", "code", "signatures", "source"):
- if hasattr(error, attr):
- log.info("%s: %s", attr, getattr(error, attr))
- return ("Changes file must be signed with a valid GPG "
- "signature: %s" % error)
-
- key = getUtility(IGPGKeySet).getByFingerprint(sig.fingerprint)
- if key is None:
- return (
- "Signing key %s not registered in launchpad."
- % sig.fingerprint)
-
- if key.active == False:
- return "Changes file is signed with a deactivated key"
-
- return None
-
-
class FTPServiceFactory(service.Service):
"""A factory that makes an `FTPService`"""
=== modified file 'lib/lp/services/gpg/handler.py'
--- lib/lp/services/gpg/handler.py 2011-12-30 08:13:14 +0000
+++ lib/lp/services/gpg/handler.py 2012-03-26 07:12:35 +0000
@@ -128,20 +128,6 @@
if os.path.exists(filename):
os.remove(filename)
- def touchConfigurationDirectory(self):
- """See IGPGHandler."""
- os.utime(self.home, None)
- for file in os.listdir(self.home):
- try:
- os.utime(os.path.join(self.home, file), None)
- except OSError as e:
- if e.errno == errno.ENOENT:
- # The file has been deleted.
- pass
- else:
- # Some other unexpected error.
- raise e
-
def verifySignature(self, content, signature=None):
"""See IGPGHandler."""
try:
=== modified file 'lib/lp/services/gpg/interfaces.py'
--- lib/lp/services/gpg/interfaces.py 2011-12-09 00:20:44 +0000
+++ lib/lp/services/gpg/interfaces.py 2012-03-26 07:12:35 +0000
@@ -282,16 +282,6 @@
"""
#FIXME RBC: this should be a zope test cleanup thing per SteveA.
- def touchConfigurationDirectory():
- """Touch the home directory and all files within.
-
- This function is called so that the configuration directory does not
- get cleaned up by any reaper scripts which look at time last modified.
- It is only required in the case where a long running daemon uses an
- IGPGHandler instance such that the lifetime of the daemon exceeds the
- reaping (ie tmp clean up) interval.
- """
-
class IPymeSignature(Interface):
"""pyME signature container."""
=== modified file 'lib/lp/services/gpg/tests/test_gpghandler.py'
--- lib/lp/services/gpg/tests/test_gpghandler.py 2011-12-30 08:13:14 +0000
+++ lib/lp/services/gpg/tests/test_gpghandler.py 2012-03-26 07:12:35 +0000
@@ -1,22 +1,7 @@
# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
-from calendar import timegm
-from datetime import (
- datetime,
- timedelta,
- )
-from math import floor
-import os
-from time import time
-
-from pytz import UTC
-from testtools.matchers import (
- LessThan,
- Not,
- )
from zope.component import getUtility
-from zope.security.proxy import removeSecurityProxy
from lp.services.gpg.interfaces import (
GPGKeyDoesNotExistOnServer,
@@ -156,32 +141,6 @@
"""Do we have the expected test keyring files"""
self.assertEqual(len(list(test_keyrings())), 1)
- def testHomeDirectoryJob(self):
- """Does the job to touch the home work."""
- gpghandler = getUtility(IGPGHandler)
- naked_gpghandler = removeSecurityProxy(gpghandler)
-
- # Get a list of all the files in the home directory.
- files_to_check = [os.path.join(naked_gpghandler.home, f)
- for f in os.listdir(naked_gpghandler.home)]
- files_to_check.append(naked_gpghandler.home)
- self.assertTrue(len(files_to_check) > 1)
-
- # Set the last modified times to 12 hours ago
- nowless12 = (datetime.now(UTC) - timedelta(hours=12)).utctimetuple()
- lm_time = timegm(nowless12)
- for fname in files_to_check:
- os.utime(fname, (lm_time, lm_time))
-
- # Touch the files and re-check the last modified times have been
- # updated to "now".
- now = floor(time())
- gpghandler.touchConfigurationDirectory()
- for fname in files_to_check:
- file_time = os.path.getmtime(fname)
- self.assertThat(
- file_time, Not(LessThan(now)), fname)
-
def test_retrieveKey_raises_GPGKeyDoesNotExistOnServer(self):
# GPGHandler.retrieveKey() raises GPGKeyDoesNotExistOnServer
# when called for a key that does not exist on the key server.
