Merge lp:~jml/launchpad/extract-ssh-server into lp:launchpad

[Jonathan Lange]

Refactor some codehosting SSH server stuff to not depend on codehosting configuration

This branch extracts some of the changes from my mega-branch lp:~jml/launchpad/ssh-key-auth.

In particular, it parametrizes three of the key classes so that they don't look up configuration variables themselves, but rather get passed the values that they need. This pushes most of the configuration up to the sftp.tac file, which is good, since tac files are all about configuration.

The three classes are SSHUserAuthServer, Factory and SSHService. The changes are pretty boring -- just moving config variables out into constructor parameters.

Other than that, there's a tweak to LoggingManager to remove an unused parameter.

I look forward to your review.

[Michael Hudson-Doyle (mwhudson)]

Well, I certainly like the idea of this branch.

I think SSHService.__init__'s port argument should perhaps be called strport. I'd expect 'port' to be an integer (I guess the config item is poorly named too, but that's harder to fix).

The parameterization around the key path names smells a little funny to me. Would it be safe for SSHService to just take the directory and assume the file names? Maybe not...

I think service.Factory.__doc__ could be clearer about the expected types of the key arguments.

Um, I think that's it.

Thanks for working on this!

review: Approve

=== modified file 'daemons/sftp.tac'
--- daemons/sftp.tac	2009-06-24 20:55:31 +0000
+++ daemons/sftp.tac	2010-04-15 13:43:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # This is a Twisted application config file.  To run, use:
@@ -7,13 +7,22 @@
 
 from twisted.application import service
 
+from canonical.config import config
 from canonical.launchpad.daemons import tachandler
-from lp.codehosting.sshserver.service import SSHService
-
-
-# Construct an Application that includes a supermirror SFTP service. 
+
+from lp.codehosting.sshserver.service import (
+    get_key_path, make_portal, PRIVATE_KEY_FILE, PUBLIC_KEY_FILE, SSHService)
+
+
+# Construct an Application that has the codehosting SSH server.
 application = service.Application('sftponly')
-svc = SSHService()
+svc = SSHService(
+    portal=make_portal(),
+    private_key_path=get_key_path(PRIVATE_KEY_FILE),
+    public_key_path=get_key_path(PUBLIC_KEY_FILE),
+    strport=config.codehosting.port,
+    idle_timeout=config.codehosting.idle_timeout,
+    banner=config.codehosting.banner)
 svc.setServiceParent(application)
 
 # Service that announces when the daemon is ready
 
=== modified file 'lib/lp/codehosting/sshserver/accesslog.py'
--- lib/lp/codehosting/sshserver/accesslog.py	2010-04-09 12:45:48 +0000
+++ lib/lp/codehosting/sshserver/accesslog.py	2010-04-15 13:43:32 +0000
@@ -36,7 +36,7 @@
 class LoggingManager:
     """Class for managing codehosting logging."""
 
-    def setUp(self, configure_oops_reporting=False, mangle_stdout=False):
+    def setUp(self, configure_oops_reporting=False):
         """Set up logging for the smart server.
 
         This sets up a debugging handler on the 'codehosting' logger, makes
@@ -45,9 +45,6 @@
 
         :param configure_oops_reporting: If True, install a Twisted log
             observer that ensures unhandled exceptions get reported as OOPSes.
-        :param mangle_stdout: If True and if configure_oops_reporting is True,
-            redirect standard error and standard output to the OOPS logging
-            observer.
         """
         log = get_codehosting_logger()
         self._orig_level = log.level
 
=== modified file 'lib/lp/codehosting/sshserver/auth.py'
--- lib/lp/codehosting/sshserver/auth.py	2010-04-10 13:55:56 +0000
+++ lib/lp/codehosting/sshserver/auth.py	2010-04-15 13:43:32 +0000
@@ -168,8 +168,9 @@
     paste and change the implementations of these methods.
     """
 
-    def __init__(self, transport=None):
+    def __init__(self, transport=None, banner=None):
         self.transport = transport
+        self._banner = banner
         self._configured_banner_sent = False
         self._mind = UserDetailsMind()
         self.interfaceToMethod = userauth.SSHUserAuthServer.interfaceToMethod
@@ -181,10 +182,9 @@
                                   NS(bytes) + NS(language))
 
     def _sendConfiguredBanner(self, passed_through):
-        if (not self._configured_banner_sent
-            and config.codehosting.banner is not None):
+        if not self._configured_banner_sent and self._banner:
             self._configured_banner_sent = True
-            self.sendBanner(config.codehosting.banner)
+            self.sendBanner(self._banner)
         return passed_through
 
     def ssh_USERAUTH_REQUEST(self, packet):
 
=== modified file 'lib/lp/codehosting/sshserver/service.py'
--- lib/lp/codehosting/sshserver/service.py	2010-03-19 10:43:51 +0000
+++ lib/lp/codehosting/sshserver/service.py	2010-04-15 13:43:32 +0000
@@ -30,6 +30,12 @@
 from lp.services.twistedsupport import gatherResults
 
 
+# The names of the key files of the server itself. The directory itself is
+# given in config.codehosting.host_key_pair_path.
+PRIVATE_KEY_FILE = 'ssh_host_key_rsa'
+PUBLIC_KEY_FILE = 'ssh_host_key_rsa.pub'
+
+
 class KeepAliveSettingSSHServerTransport(SSHServerTransport):
 
     def connectionMade(self):
@@ -37,6 +43,24 @@
         self.transport.setTcpKeepAlive(True)
 
 
+def get_key_path(key_filename):
+    key_directory = config.codehosting.host_key_pair_path
+    return os.path.join(config.root, key_directory, key_filename)
+
+
+def make_portal():
+    """Create and return a `Portal` for the SSH service.
+
+    This portal accepts SSH credentials and returns our customized SSH
+    avatars (see `lp.codehosting.sshserver.auth.LaunchpadAvatar`).
+    """
+    authentication_proxy = Proxy(
+        config.codehosting.authentication_endpoint)
+    branchfs_proxy = Proxy(config.codehosting.branchfs_endpoint)
+    return get_portal(authentication_proxy, branchfs_proxy)
+
+
+
 class Factory(SSHFactory):
     """SSH factory that uses the codehosting custom authentication.
 
@@ -47,13 +71,29 @@
 
     protocol = KeepAliveSettingSSHServerTransport
 
-    def __init__(self, portal):
+    def __init__(self, portal, private_key, public_key, banner=None):
+        """Construct an SSH factory.
+
+        :param portal: The portal used to turn credentials into users.
+        :param private_key: The private key of the server, must be an RSA
+            key, given as a `twisted.conch.ssh.keys.Key` object.
+        :param public_key: The public key of the server, must be an RSA
+            key, given as a `twisted.conch.ssh.keys.Key` object.
+        :param banner: The text to display when users successfully log in.
+        """
         # Although 'portal' isn't part of the defined interface for
         # `SSHFactory`, defining it here is how the `SSHUserAuthServer` gets
         # at it. (Look for the beautiful line "self.portal =
         # self.transport.factory.portal").
         self.portal = portal
-        self.services['ssh-userauth'] = SSHUserAuthServer
+        self.services['ssh-userauth'] = self._makeAuthServer
+        self._private_key = private_key
+        self._public_key = public_key
+        self._banner = banner
+
+    def _makeAuthServer(self, *args, **kwargs):
+        kwargs['banner'] = self._banner
+        return SSHUserAuthServer(*args, **kwargs)
 
     def buildProtocol(self, address):
         """Build an SSH protocol instance, logging the event.
@@ -87,58 +127,51 @@
                 notify(accesslog.AuthenticationFailed(transport))
             notify(accesslog.UserDisconnected(transport))
 
-    def _loadKey(self, key_filename):
-        key_directory = config.codehosting.host_key_pair_path
-        key_path = os.path.join(config.root, key_directory, key_filename)
-        return Key.fromFile(key_path)
-
     def getPublicKeys(self):
         """Return the server's configured public key.
 
         See `SSHFactory.getPublicKeys`.
         """
-        public_key = self._loadKey('ssh_host_key_rsa.pub')
-        return {'ssh-rsa': public_key}
+        return {'ssh-rsa': self._public_key}
 
     def getPrivateKeys(self):
         """Return the server's configured private key.
 
         See `SSHFactory.getPrivateKeys`.
         """
-        private_key = self._loadKey('ssh_host_key_rsa')
-        return {'ssh-rsa': private_key}
+        return {'ssh-rsa': self._private_key}
 
 
 class SSHService(service.Service):
     """A Twisted service for the codehosting SSH server."""
 
-    def __init__(self):
-        self.service = self.makeService()
-
-    def makePortal(self):
-        """Create and return a `Portal` for the SSH service.
-
-        This portal accepts SSH credentials and returns our customized SSH
-        avatars (see `lp.codehosting.sshserver.auth.LaunchpadAvatar`).
-        """
-        authentication_proxy = Proxy(
-            config.codehosting.authentication_endpoint)
-        branchfs_proxy = Proxy(config.codehosting.branchfs_endpoint)
-        return get_portal(authentication_proxy, branchfs_proxy)
-
-    def makeService(self):
-        """Return a service that provides an SFTP server. This is called in
-        the constructor.
+    def __init__(self, portal, private_key_path, public_key_path,
+                 strport='tcp:22', idle_timeout=3600, banner=None):
+        """Construct an SSH service.
+
+        :param portal: The `Portal` that turns authentication requests into
+            views on the system.
+        :param private_key_path: The path to the SSH server's private key.
+        :param public_key_path: The path to the SSH server's public key.
+        :param strport: The port to run the server on, expressed in Twisted's
+            "strports" mini-language. Defaults to 'tcp:22'.
+        :param idle_timeout: The number of seconds to wait before killing a
+            connection that isn't doing anything. Defaults to 3600.
+        :param banner: An announcement printed to users when they connect.
+            By default, announce nothing.
         """
         ssh_factory = TimeoutFactory(
-            Factory(self.makePortal()),
-            timeoutPeriod=config.codehosting.idle_timeout)
-        return strports.service(config.codehosting.port, ssh_factory)
+            Factory(
+                portal,
+                private_key=Key.fromFile(private_key_path),
+                public_key=Key.fromFile(public_key_path),
+                banner=banner),
+            timeoutPeriod=idle_timeout)
+        self.service = strports.service(strport, ssh_factory)
 
     def startService(self):
         """Start the SSH service."""
-        accesslog.LoggingManager().setUp(
-            configure_oops_reporting=True, mangle_stdout=True)
+        accesslog.LoggingManager().setUp(configure_oops_reporting=True)
         notify(accesslog.ServerStarting())
         # By default, only the owner of files should be able to write to them.
         # Perhaps in the future this line will be deleted and the umask
 
=== modified file 'lib/lp/codehosting/sshserver/tests/test_auth.py'
--- lib/lp/codehosting/sshserver/tests/test_auth.py	2010-03-19 10:43:51 +0000
+++ lib/lp/codehosting/sshserver/tests/test_auth.py	2010-04-15 13:43:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import os
@@ -193,11 +193,6 @@
 
     layer = TwistedLayer
 
-    banner_conf = """
-        [codehosting]
-        banner: banner
-        """
-
     def setUp(self):
         UserAuthServerMixin.setUp(self)
         self.portal.registerChecker(MockChecker())
@@ -240,25 +235,22 @@
             self.assertMessageOrder([userauth.MSG_USERAUTH_SUCCESS])
         return d.addCallback(check)
 
-    def test_configuredBannerSentOnSuccess(self):
-        # If a banner is set in the codehosting config then we send it to the
+    def test_defaultBannerSentOnSuccess(self):
+        # If a banner was passed to the user auth agent then we send it to the
         # user when they log in.
-        config.push('codehosting_overlay', self.banner_conf)
+        self.user_auth._banner = "Boogedy boo"
         d = self.requestSuccessfulAuthentication()
         def check(ignored):
             self.assertMessageOrder(
                 [userauth.MSG_USERAUTH_BANNER, userauth.MSG_USERAUTH_SUCCESS])
-            self.assertBannerSent(config.codehosting.banner + '\r\n')
-        def cleanup(ignored):
-            config.pop('codehosting_overlay')
-            return ignored
-        return d.addCallback(check).addBoth(cleanup)
+            self.assertBannerSent(self.user_auth._banner + '\r\n')
+        return d.addCallback(check)
 
-    def test_configuredBannerSentOnlyOnce(self):
+    def test_defaultBannerSentOnlyOnce(self):
         # We don't send the banner on each authentication attempt, just on the
         # first one. It is usual for there to be many authentication attempts
         # per SSH session.
-        config.push('codehosting_overlay', self.banner_conf)
+        self.user_auth._banner = "Boogedy boo"
 
         d = self.requestUnsupportedAuthentication()
         d.addCallback(lambda ignored: self.requestSuccessfulAuthentication())
@@ -268,17 +260,14 @@
             self.assertMessageOrder(
                 [userauth.MSG_USERAUTH_FAILURE, userauth.MSG_USERAUTH_BANNER,
                  userauth.MSG_USERAUTH_SUCCESS])
-            self.assertBannerSent(config.codehosting.banner + '\r\n')
-
-        def cleanup(ignored):
-            config.pop('codehosting_overlay')
-            return ignored
-        return d.addCallback(check).addBoth(cleanup)
-
-    def test_configuredBannerNotSentOnFailure(self):
-        # Failed authentication attempts do not get the configured banner
+            self.assertBannerSent(self.user_auth._banner + '\r\n')
+
+        return d.addCallback(check)
+
+    def test_defaultBannerNotSentOnFailure(self):
+        # Failed authentication attempts do not get the default banner
         # sent.
-        config.push('codehosting_overlay', self.banner_conf)
+        self.user_auth._banner = "You come away two hundred quid down"
 
         d = self.requestFailedAuthentication()
 
@@ -287,11 +276,7 @@
                 [userauth.MSG_USERAUTH_BANNER, userauth.MSG_USERAUTH_FAILURE])
             self.assertBannerSent(MockChecker.error_message + '\r\n')
 
-        def cleanup(ignored):
-            config.pop('codehosting_overlay')
-            return ignored
-
-        return d.addCallback(check).addBoth(cleanup)
+        return d.addCallback(check)
 
     def test_loggedToBanner(self):
         # When there's an authentication failure, we display an informative
@@ -532,7 +517,12 @@
 
     def makeFactory(self):
         """Create and start the factory that our SSH server uses."""
-        factory = service.Factory(auth.get_portal(None, None))
+        factory = service.Factory(
+            auth.get_portal(None, None),
+            private_key=Key.fromFile(
+                service.get_key_path(service.PRIVATE_KEY_FILE)),
+            public_key=Key.fromFile(
+                service.get_key_path(service.PUBLIC_KEY_FILE)))
         factory.startFactory()
         return factory