launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02782
[Merge] lp:~julian-edwards/launchpad/poppy-warn-unsigned-bug-374019 into lp:launchpad
Julian Edwards has proposed merging lp:~julian-edwards/launchpad/poppy-warn-unsigned-bug-374019 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#374019 bad keys result in no response on ppa uploads
https://bugs.launchpad.net/bugs/374019
For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/poppy-warn-unsigned-bug-374019/+merge/51144
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.
--
https://code.launchpad.net/~julian-edwards/launchpad/poppy-warn-unsigned-bug-374019/+merge/51144
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/poppy-warn-unsigned-bug-374019 into lp:launchpad.
=== modified file 'daemons/poppy-sftp.tac'
--- daemons/poppy-sftp.tac 2011-02-23 11:52:58 +0000
+++ daemons/poppy-sftp.tac 2011-02-25 10:43:08 +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 10:43:08 +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 10:43:08 +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, '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)
+
+ # 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 10:43:08 +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,14 @@
"""
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:
+ return defer.fail()
+ return defer.succeed(PoppyFileWriter(fObj))
def makeDirectory(self, path):
"""Make a directory using the secure `UploadFileSystem`."""
@@ -128,6 +142,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`"""
