Merge lp:~wgrant/launchpad/neuter-poppy-db into lp:launchpad

[William Grant]

Rip out poppy's disabled GPG verification and DB access.

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.

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== 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, '[e-mail]')
-        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.