Merge lp:~julian-edwards/launchpad/poppy-warn-unsigned-bug-374019 into lp:launchpad

[Julian Edwards]

[r=adeuring][bug=374019] Add support to the new twisted-based poppy ftp server to reject invalidly gpg-signed uploaded .changes files directly in the FTP session.

Add support to the new twisted-based poppy ftp server to reject invalidly gpg-signed uploaded .changes files directly in the FTP session.

This change depends on a patch to Twisted being applied, which is being tracked at http://twistedmatrix.com/trac/ticket/4909

It works by creating a new custom file-writer, which checks to see if the file is a .changes file, and does a quick lookup of the GPG key before ending the transfer session.

I've tested this approach using dput, and dput gets a lovely error message printed out like this:

Uploading to dogfood (via ftp to dogfood.launchpad.net):
  Uploading hello_2.5-1ubuntu1.dsc: done.
  Uploading hello_2.5-1ubuntu1.diff.gz: done.
  Uploading hello_2.5-1ubuntu1_source.changes: 1k/2k550 ('Changes file must be signed with a valid GPG signature: Verification failed 3 times: ["(7, 8, u\'Bad signature\')", "(7, 8, u\'Bad signature\')", "(7, 8, u\'Bad signature\')"] ',): Permission denied.

[Abel Deuring (adeuring)]

(15:48:46) adeuring: bigjools: PoppyAnonymousShell.openForWriting() has a "catch-all" except. I think we do not want to catch KeyboardInterrupt and SystemExit. And I think we should log an error for other exceptions.
(15:49:00) adeuring: otherwise, the branch looks good, I think
(15:50:07) bigjools: adeuring: that was cargo-culted from the parent class... I wasn't sure what to do about it TBH
(15:51:40) adeuring: bigjools: admittedly, I am not familiar with twisted, so I am not sure about KBInterrupt and SystemExit. But nevertheless I think we should log other exceptions.
(15:52:54) adeuring: "eating" exceptions simply scares me ;)
(15:53:12) bigjools: adeuring: yeah
(15:54:27) bigjools: adeuring: so I'll re-raise KeyboardInterrupt and SystemExit
(15:54:44) bigjools: adeuring: oh wait
(15:55:23) bigjools: adeuring: yes twisted has confused us both momentarily - the defer.fail() is effectively the same as re-raising the exception, it gets dealt with later
(15:56:10) adeuring: bigjools: intersting. So, IOError and OSError are re-raised too?
(15:56:19) adeuring: ah, no, there no fail() call
(15:56:26) bigjools: adeuring: no, they are converted to ftp errors
(15:56:48) bigjools: ftp.errnoToFailure will return a defer.fail
(15:57:23) adeuring: bigjools: right, thanks for the explanation, so r=me. But perhaps a small comment that twisted properly deals with the exceptions? (just to avoid the same question I asked again)?
(15:57:36) bigjools: adeuring: yup, will do, thanks for the review

review: Approve (code)

=== modified file 'daemons/poppy-sftp.tac'
--- daemons/poppy-sftp.tac	2011-02-23 11:52:58 +0000
+++ daemons/poppy-sftp.tac	2011-02-25 16:39:05 +0000
@@ -19,6 +19,7 @@
 
 from canonical.config import config
 from canonical.launchpad.daemons import readyservice
+from canonical.launchpad.scripts import execute_zcml_for_scripts
 
 from lp.poppy import get_poppy_root
 from lp.poppy.twistedftp import (
@@ -105,5 +106,8 @@
     banner=config.poppy.banner)
 svc.setServiceParent(application)
 
+# We need Zope for looking up the GPG utilities.
+execute_zcml_for_scripts()
+
 # Service that announces when the daemon is ready
 readyservice.ReadyService().setServiceParent(application)

=== modified file 'lib/lp/poppy/tests/test_poppy.py'
--- lib/lp/poppy/tests/test_poppy.py	2011-02-24 17:23:57 +0000
+++ lib/lp/poppy/tests/test_poppy.py	2011-02-25 16:39:05 +0000
@@ -5,6 +5,7 @@
 
 __metaclass__ = type
 
+import ftplib
 import os
 import shutil
 import stat
@@ -37,6 +38,7 @@
     )
 from lp.poppy.hooks import Hooks
 from lp.testing import TestCaseWithFactory
+from lp.testing.keyserver import KeyServerTac
 
 
 class FTPServer(Fixture):
@@ -371,6 +373,36 @@
                 self.root_dir, upload_dirs[index], "test")).read()
             self.assertEqual(content, expected_contents[index])
 
+    def 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__)

=== added file 'lib/lp/poppy/tests/test_twistedftp.py'
--- lib/lp/poppy/tests/test_twistedftp.py	1970-01-01 00:00:00 +0000
+++ lib/lp/poppy/tests/test_twistedftp.py	2011-02-25 16:39:05 +0000
@@ -0,0 +1,94 @@
+# 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,
+    )
+from twisted.protocols import ftp
+from zope.component import getUtility
+
+from canonical.config import config
+from canonical.launchpad.ftests.keys_for_tests import gpgkeysdir
+from canonical.launchpad.interfaces.gpghandler import IGPGHandler
+from canonical.testing.layers import ZopelessDatabaseLayer
+
+from lp.poppy.twistedftp import PoppyFileWriter
+from lp.registry.interfaces.gpg import (
+    GPGKeyAlgorithm,
+    IGPGKeySet)
+from lp.testing import TestCaseWithFactory
+from lp.testing.keyserver import KeyServerTac
+
+
+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)
+
+        # 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
+
+    def 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

=== modified file 'lib/lp/poppy/twistedftp.py'
--- lib/lp/poppy/twistedftp.py	2011-02-23 14:24:43 +0000
+++ lib/lp/poppy/twistedftp.py	2011-02-25 16:39:05 +0000
@@ -21,11 +21,18 @@
 from twisted.python import filepath
 
 from zope.interface import implements
+from zope.component import getUtility
+
+from canonical.launchpad.interfaces.gpghandler import (
+    GPGVerificationError,
+    IGPGHandler,
+    )
 
 from canonical.config import config
 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
 
 
 class PoppyAccessCheck:
@@ -72,7 +79,15 @@
         """
         filename = os.sep.join(file_segments)
         self._create_missing_directories(filename)
-        return super(PoppyAnonymousShell, self).openForWriting(file_segments)
+        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))
 
     def makeDirectory(self, path):
         """Make a directory using the secure `UploadFileSystem`."""
@@ -128,6 +143,43 @@
             "Only IFTPShell interface is supported by this realm")
 
 
+class PoppyFileWriter(ftp._FileWriter):
+    """An `IWriteFile` that checks for signed changes files."""
+
+    def 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)
+
+    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:
+            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 'versions.cfg'
--- versions.cfg	2011-02-23 16:14:05 +0000
+++ versions.cfg	2011-02-25 16:39:05 +0000
@@ -75,7 +75,7 @@
 storm = 0.18
 testtools = 0.9.8
 transaction = 1.0.0
-Twisted = 10.2.0-4395fix-1
+Twisted = 10.2.0-4395fix-1-4909
 uuid = 1.30
 van.testing = 2.0.1
 wadllib = 1.2.0