launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02717
[Merge] lp:~julian-edwards/launchpad/twisted-ftp-poppy into lp:launchpad
Julian Edwards has proposed merging lp:~julian-edwards/launchpad/twisted-ftp-poppy into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/twisted-ftp-poppy/+merge/50721
= Summary =
Extend poppy-sftp to also handle ftp.
== Proposed fix ==
Package uploads are currently handled either in Poppy (FTP) or Poppy-sftp
(SFTP). The former is written using Zope's FTP server, the latter using
Twisted.
The Zope FTP server has a number of bugs, so this branch adds FTP support via
the Twisted libraries.
== Pre-implementation notes ==
None, I suck.
== Implementation details ==
Some interesting points:
* Poppy doesn't really care about authentication, it lets any credentials
upload files, whether it be anonymous or otherwise. Twisted has a special
mode where if anyone logs in as "anonymous" it will prevent any file uploads.
This behaviour is hard-coded in the base FTPShell class. To get around this,
the FTPFactory's "userAnonymous" is modified to a user called
"userthatwillneverhappen" so that the read-only mode is not encountered for
anonymous access.
* The old port number was passed on the command line, it's now set in config.
* The tests were easy to fix since they just needed the PoppyTac fixture
swapped in in favour of the old PoppyTestSetup.
* The old Poppy had a file mode bug where it set group execute on uploaded
files, this is reflected in the test changes for the new ftp service.
* The poppy server does post-processing of the upload. There's no way to
know when this is finished, so the test just sleeps for a few seconds. See
bug 586695. The old poppy code had some weird hook to print output to stdout
when it finished uploading, we might need to do that here too.
== Tests ==
bin/test -cvv test_poppy
== Demo and Q/A ==
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/poppy/twistedftp.py
lib/canonical/config/schema-lazr.conf
lib/lp/poppy/tests/test_poppy.py
daemons/poppy-sftp.tac
./lib/canonical/config/schema-lazr.conf
522: Line exceeds 78 characters.
1034: Line exceeds 78 characters.
--
https://code.launchpad.net/~julian-edwards/launchpad/twisted-ftp-poppy/+merge/50721
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/twisted-ftp-poppy into lp:launchpad.
=== modified file 'daemons/poppy-sftp.tac'
--- daemons/poppy-sftp.tac 2011-02-02 01:24:33 +0000
+++ daemons/poppy-sftp.tac 2011-02-22 10:21:46 +0000
@@ -7,10 +7,11 @@
import os
-from twisted.application import service
+from twisted.application import service, strports
from twisted.conch.interfaces import ISession
from twisted.conch.ssh import filetransfer
from twisted.cred.portal import IRealm, Portal
+from twisted.protocols import ftp
from twisted.protocols.policies import TimeoutFactory
from twisted.python import components
from twisted.web.xmlrpc import Proxy
@@ -20,6 +21,10 @@
from canonical.config import config
from canonical.launchpad.daemons import readyservice
+from lp.poppy.twistedftp import (
+ FTPRealm,
+ PoppyAccessCheck,
+ )
from lp.poppy.twistedsftp import SFTPServer
from lp.services.sshserver.auth import (
LaunchpadAvatar, PublicKeyFromLaunchpadChecker)
@@ -85,9 +90,41 @@
components.registerAdapter(DoNothingSession, LaunchpadAvatar, ISession)
-# Construct an Application that has the Poppy SSH server.
+class FTPServiceFactory(service.Service):
+ def __init__(self, port):
+ factory = ftp.FTPFactory()
+ realm = FTPRealm(get_poppy_root())
+ portal = Portal(realm)
+ portal.registerChecker(PoppyAccessCheck())
+
+ factory.tld = get_poppy_root()
+ factory.portal = portal
+ factory.protocol = ftp.FTP
+ factory.welcomeMessage = "Launchpad upload server"
+ factory.timeOut = config.poppy.idle_timeout
+
+ # Setting this works around the fact that the twistd FTP server
+ # invokes a special restricted shell when someone logs in as
+ # "anonymous" which is the default for 'dput'.
+ factory.userAnonymous = "userthatwillneverhappen"
+ self.ftpfactory = factory
+ self.portno = port
+
+ @staticmethod
+ def makeFTPService(port=2121):
+ strport = "tcp:%s" % port
+ factory = FTPServiceFactory(port)
+ return strports.service(strport, factory.ftpfactory)
+
+# ftpport defaults to 2121 in schema-lazr.conf
+ftpservice = FTPServiceFactory.makeFTPService(port=config.poppy.ftpport)
+
+# Construct an Application that has the Poppy SSH server,
+# and the Poppy FTP server.
application = service.Application('poppy-sftp')
+ftpservice.setServiceParent(application)
+
def timeout_decorator(factory):
"""Add idle timeouts to a factory."""
return TimeoutFactory(factory, timeoutPeriod=config.poppy.idle_timeout)
=== modified file 'lib/canonical/config/schema-lazr.conf'
--- lib/canonical/config/schema-lazr.conf 2011-02-21 18:54:53 +0000
+++ lib/canonical/config/schema-lazr.conf 2011-02-22 10:21:46 +0000
@@ -1654,6 +1654,9 @@
# datatype: string
port: tcp:5022
+# datatype: string
+ftpport: 2121
+
# See [error_reports].
copy_to_zlog: False
=== modified file 'lib/lp/poppy/tests/test_poppy.py'
--- lib/lp/poppy/tests/test_poppy.py 2010-11-07 12:10:38 +0000
+++ lib/lp/poppy/tests/test_poppy.py 2011-02-22 10:21:46 +0000
@@ -1,14 +1,12 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Functional tests for poppy FTP daemon."""
__metaclass__ = type
-import ftplib
import os
import shutil
-import socket
import StringIO
import tempfile
import time
@@ -33,7 +31,6 @@
ZopelessAppServerLayer,
ZopelessDatabaseLayer,
)
-from lp.poppy.tests.helpers import PoppyTestSetup
from lp.registry.interfaces.ssh import (
ISSHKeySet,
)
@@ -45,38 +42,21 @@
def __init__(self, root_dir, factory):
self.root_dir = root_dir
- self.port = 3421
+ self.port = 2121
def setUp(self):
super(FTPServer, self).setUp()
- self.poppy = PoppyTestSetup(
- self.root_dir, port=self.port, cmd='echo CLOSED')
- self.poppy.startPoppy()
- self.addCleanup(self.poppy.killPoppy)
+ self.useFixture(PoppyTac(self.root_dir))
def getTransport(self):
return get_transport('ftp://ubuntu:@localhost:%s/' % (self.port,))
def disconnect(self, transport):
- transport._get_connection().quit()
-
- def _getFTPConnection(self):
- # poppy usually takes sometime to come up, we need to wait, or insist.
- conn = ftplib.FTP()
- while True:
- try:
- conn.connect("localhost", self.port)
- except socket.error:
- if not self.poppy.alive:
- raise
- else:
- break
- return conn
+ transport._get_connection().close()
def waitForStartUp(self):
"""Wait for the FTP server to start up."""
- conn = self._getFTPConnection()
- conn.quit()
+ pass
def waitForClose(self):
"""Wait for an FTP connection to close.
@@ -86,7 +66,7 @@
output as a way to tell that the server has finished with the
connection.
"""
- self.poppy.verify_output(['CLOSED'])
+ time.sleep(5)
class SFTPServer(Fixture):
@@ -269,7 +249,11 @@
wanted_path = self._uploadPath('foo/bar/baz')
fs_content = open(os.path.join(wanted_path)).read()
self.assertEqual(fs_content, "fake contents")
- self.assertEqual(os.stat(wanted_path).st_mode, 0102674)
+ # This is a magic and very opaque number. It corresponds to
+ # the following stat.S_* masks:
+ # S_IROTH, S_ISGID, S_IRGRP, S_IWGRP, S_IWUSR, S_IWUSR
+ # which in readable terms is: -rw-rwSr--
+ self.assertEqual(os.stat(wanted_path).st_mode, 0102664)
def test_full_source_upload(self):
"""Check that the connection will deal with multiple files being
@@ -298,13 +282,17 @@
if transport._user == 'joe':
self.assertEqual(dir_name.startswith('upload-sftp-2'), True)
elif transport._user == 'ubuntu':
- self.assertEqual(dir_name.startswith('upload-2'), True)
+ self.assertEqual(dir_name.startswith('upload-ftp-2'), True)
for upload in files:
wanted_path = self._uploadPath(
"~ppa-user/ppa/ubuntu/%s" % upload)
fs_content = open(os.path.join(wanted_path)).read()
self.assertEqual(fs_content, upload)
- self.assertEqual(os.stat(wanted_path).st_mode, 0102674)
+ # This is a magic and very opaque number. It corresponds to
+ # the following stat.S_* masks:
+ # S_IROTH, S_ISGID, S_IRGRP, S_IWGRP, S_IWUSR, S_IWUSR
+ # which in readable terms is: -rw-rwSr--
+ self.assertEqual(os.stat(wanted_path).st_mode, 0102664)
def test_upload_isolation(self):
"""Check if poppy isolates the uploads properly.
=== added file 'lib/lp/poppy/twistedftp.py'
--- lib/lp/poppy/twistedftp.py 1970-01-01 00:00:00 +0000
+++ lib/lp/poppy/twistedftp.py 2011-02-22 10:21:46 +0000
@@ -0,0 +1,124 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Twisted FTP implementation of the Poppy upload server."""
+
+__metaclass__ = type
+__all__ = [
+ 'FTPRealm',
+ 'PoppyAnonymousShell',
+ ]
+
+import logging
+import os
+import tempfile
+
+from twisted.cred import checkers, credentials
+from twisted.cred.portal import IRealm
+from twisted.internet import defer
+from twisted.protocols import ftp
+from twisted.python import filepath
+
+from zope.interface import implements
+
+from lp.poppy.filesystem import UploadFileSystem
+from lp.poppy.hooks import Hooks
+
+
+class PoppyAccessCheck:
+ """An `ICredentialsChecker` for Poppy FTP sessions."""
+ implements(checkers.ICredentialsChecker)
+ credentialInterfaces = credentials.IUsernamePassword,
+
+ def requestAvatarId(self, credentials):
+ # Poppy allows any credentials. People can use "anonymous" if
+ # they want but anything goes. Returning "poppy" here is
+ # a totally arbitrary avatar.
+ return "poppy"
+
+
+class PoppyAnonymousShell(ftp.FTPShell):
+ """The 'command' interface for sessions.
+
+ Roughly equivalent to the SFTPServer in the sftp side of things.
+ """
+
+ def __init__(self, fsroot):
+ self._fs_root = fsroot
+ self.uploadfilesystem = UploadFileSystem(tempfile.mkdtemp())
+ self._current_upload = self.uploadfilesystem.rootpath
+ os.chmod(self._current_upload, 0770)
+ self._log = logging.getLogger("poppy-sftp")
+ self.hook = Hooks(
+ self._fs_root, self._log, "ubuntu", perms='g+rws',
+ prefix='-ftp')
+ self.hook.new_client_hook(self._current_upload, 0, 0)
+ self.hook.auth_verify_hook(self._current_upload, None, None)
+ super(PoppyAnonymousShell, self).__init__(
+ filepath.FilePath(self._current_upload))
+
+ def openForWriting(self, file_segments):
+ """Write the uploaded file to disk, safely.
+
+ :param file_segments: A list containing string items, one for each
+ path component of the file being uploaded. The file referenced
+ is relative to the temporary root for this session.
+
+ If the file path contains directories, we create them.
+ """
+ filename = os.sep.join(file_segments)
+ self._create_missing_directories(filename)
+ return super(PoppyAnonymousShell, self).openForWriting(file_segments)
+
+ def makeDirectory(self, path):
+ """Make a directory using the secure `UploadFileSystem`."""
+ path = os.sep.join(path)
+ return defer.maybeDeferred(self.uploadfilesystem.mkdir, path)
+
+ def access(self, segments):
+ """Permissive CWD that auto-creates target directories."""
+ if segments:
+ path = self._path(segments)
+ path.makedirs()
+ return super(PoppyAnonymousShell, self).access(segments)
+
+ def logout(self):
+ """Called when the client disconnects.
+
+ We need to post-process the upload.
+ """
+ self.hook.client_done_hook(self._current_upload, 0, 0)
+
+ def _create_missing_directories(self, filename):
+ # Same as SFTPServer
+ new_dir, new_file = os.path.split(
+ self.uploadfilesystem._sanitize(filename))
+ if new_dir != '':
+ if not os.path.exists(
+ os.path.join(self._current_upload, new_dir)):
+ self.uploadfilesystem.mkdir(new_dir)
+
+ def list(self, path_segments, attrs):
+ return defer.fail(ftp.CmdNotImplementedError("LIST"))
+
+
+class FTPRealm:
+ """FTP Realm that lets anyone in."""
+ implements(IRealm)
+
+ def __init__(self, root):
+ self.root = root
+
+ def requestAvatar(self, avatarId, mind, *interfaces):
+ """Return a Poppy avatar - that is, an "authorisation".
+
+ Poppy FTP avatars are totally fake, we don't care about credentials.
+ See `PoppyAccessCheck` above.
+ """
+ for iface in interfaces:
+ if iface is ftp.IFTPShell:
+ avatar = PoppyAnonymousShell(self.root)
+ return ftp.IFTPShell, avatar, getattr(
+ avatar, 'logout', lambda: None)
+ raise NotImplementedError(
+ "Only IFTPShell interface is supported by this realm")
