Launchpad itself

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

  1. extract-ssh-server
  2. Merge into devel
Proposed by Jonathan Lange
Status: Merged
Approved by: Michael Hudson-Doyle
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~jml/launchpad/extract-ssh-server
Merge into: lp:launchpad
Diff against target: 351 lines (+108/-79)
5 files modified
daemons/sftp.tac (+15/-6)
lib/lp/codehosting/sshserver/accesslog.py (+1/-4)
lib/lp/codehosting/sshserver/auth.py (+4/-4)
lib/lp/codehosting/sshserver/service.py (+66/-33)
lib/lp/codehosting/sshserver/tests/test_auth.py (+22/-32)
To merge this branch: bzr merge lp:~jml/launchpad/extract-ssh-server
Reviewer Review Type Date Requested Status
Michael Hudson-Doyle Approve
Canonical Launchpad Engineering Pending
Review via email: mp+23350@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'daemons/sftp.tac'
2--- daemons/sftp.tac 2009-06-24 20:55:31 +0000
3+++ daemons/sftp.tac 2010-04-15 13:43:32 +0000
4@@ -1,4 +1,4 @@
5-# Copyright 2009 Canonical Ltd. This software is licensed under the
6+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
7 # GNU Affero General Public License version 3 (see the file LICENSE).
8
9 # This is a Twisted application config file. To run, use:
10@@ -7,13 +7,22 @@
11
12 from twisted.application import service
13
14+from canonical.config import config
15 from canonical.launchpad.daemons import tachandler
16-from lp.codehosting.sshserver.service import SSHService
17-
18-
19-# Construct an Application that includes a supermirror SFTP service.
20+
21+from lp.codehosting.sshserver.service import (
22+ get_key_path, make_portal, PRIVATE_KEY_FILE, PUBLIC_KEY_FILE, SSHService)
23+
24+
25+# Construct an Application that has the codehosting SSH server.
26 application = service.Application('sftponly')
27-svc = SSHService()
28+svc = SSHService(
29+ portal=make_portal(),
30+ private_key_path=get_key_path(PRIVATE_KEY_FILE),
31+ public_key_path=get_key_path(PUBLIC_KEY_FILE),
32+ strport=config.codehosting.port,
33+ idle_timeout=config.codehosting.idle_timeout,
34+ banner=config.codehosting.banner)
35 svc.setServiceParent(application)
36
37 # Service that announces when the daemon is ready
38
39=== modified file 'lib/lp/codehosting/sshserver/accesslog.py'
40--- lib/lp/codehosting/sshserver/accesslog.py 2010-04-09 12:45:48 +0000
41+++ lib/lp/codehosting/sshserver/accesslog.py 2010-04-15 13:43:32 +0000
42@@ -36,7 +36,7 @@
43 class LoggingManager:
44 """Class for managing codehosting logging."""
45
46- def setUp(self, configure_oops_reporting=False, mangle_stdout=False):
47+ def setUp(self, configure_oops_reporting=False):
48 """Set up logging for the smart server.
49
50 This sets up a debugging handler on the 'codehosting' logger, makes
51@@ -45,9 +45,6 @@
52
53 :param configure_oops_reporting: If True, install a Twisted log
54 observer that ensures unhandled exceptions get reported as OOPSes.
55- :param mangle_stdout: If True and if configure_oops_reporting is True,
56- redirect standard error and standard output to the OOPS logging
57- observer.
58 """
59 log = get_codehosting_logger()
60 self._orig_level = log.level
61
62=== modified file 'lib/lp/codehosting/sshserver/auth.py'
63--- lib/lp/codehosting/sshserver/auth.py 2010-04-10 13:55:56 +0000
64+++ lib/lp/codehosting/sshserver/auth.py 2010-04-15 13:43:32 +0000
65@@ -168,8 +168,9 @@
66 paste and change the implementations of these methods.
67 """
68
69- def __init__(self, transport=None):
70+ def __init__(self, transport=None, banner=None):
71 self.transport = transport
72+ self._banner = banner
73 self._configured_banner_sent = False
74 self._mind = UserDetailsMind()
75 self.interfaceToMethod = userauth.SSHUserAuthServer.interfaceToMethod
76@@ -181,10 +182,9 @@
77 NS(bytes) + NS(language))
78
79 def _sendConfiguredBanner(self, passed_through):
80- if (not self._configured_banner_sent
81- and config.codehosting.banner is not None):
82+ if not self._configured_banner_sent and self._banner:
83 self._configured_banner_sent = True
84- self.sendBanner(config.codehosting.banner)
85+ self.sendBanner(self._banner)
86 return passed_through
87
88 def ssh_USERAUTH_REQUEST(self, packet):
89
90=== modified file 'lib/lp/codehosting/sshserver/service.py'
91--- lib/lp/codehosting/sshserver/service.py 2010-03-19 10:43:51 +0000
92+++ lib/lp/codehosting/sshserver/service.py 2010-04-15 13:43:32 +0000
93@@ -30,6 +30,12 @@
94 from lp.services.twistedsupport import gatherResults
95
96
97+# The names of the key files of the server itself. The directory itself is
98+# given in config.codehosting.host_key_pair_path.
99+PRIVATE_KEY_FILE = 'ssh_host_key_rsa'
100+PUBLIC_KEY_FILE = 'ssh_host_key_rsa.pub'
101+
102+
103 class KeepAliveSettingSSHServerTransport(SSHServerTransport):
104
105 def connectionMade(self):
106@@ -37,6 +43,24 @@
107 self.transport.setTcpKeepAlive(True)
108
109
110+def get_key_path(key_filename):
111+ key_directory = config.codehosting.host_key_pair_path
112+ return os.path.join(config.root, key_directory, key_filename)
113+
114+
115+def make_portal():
116+ """Create and return a `Portal` for the SSH service.
117+
118+ This portal accepts SSH credentials and returns our customized SSH
119+ avatars (see `lp.codehosting.sshserver.auth.LaunchpadAvatar`).
120+ """
121+ authentication_proxy = Proxy(
122+ config.codehosting.authentication_endpoint)
123+ branchfs_proxy = Proxy(config.codehosting.branchfs_endpoint)
124+ return get_portal(authentication_proxy, branchfs_proxy)
125+
126+
127+
128 class Factory(SSHFactory):
129 """SSH factory that uses the codehosting custom authentication.
130
131@@ -47,13 +71,29 @@
132
133 protocol = KeepAliveSettingSSHServerTransport
134
135- def __init__(self, portal):
136+ def __init__(self, portal, private_key, public_key, banner=None):
137+ """Construct an SSH factory.
138+
139+ :param portal: The portal used to turn credentials into users.
140+ :param private_key: The private key of the server, must be an RSA
141+ key, given as a `twisted.conch.ssh.keys.Key` object.
142+ :param public_key: The public key of the server, must be an RSA
143+ key, given as a `twisted.conch.ssh.keys.Key` object.
144+ :param banner: The text to display when users successfully log in.
145+ """
146 # Although 'portal' isn't part of the defined interface for
147 # `SSHFactory`, defining it here is how the `SSHUserAuthServer` gets
148 # at it. (Look for the beautiful line "self.portal =
149 # self.transport.factory.portal").
150 self.portal = portal
151- self.services['ssh-userauth'] = SSHUserAuthServer
152+ self.services['ssh-userauth'] = self._makeAuthServer
153+ self._private_key = private_key
154+ self._public_key = public_key
155+ self._banner = banner
156+
157+ def _makeAuthServer(self, *args, **kwargs):
158+ kwargs['banner'] = self._banner
159+ return SSHUserAuthServer(*args, **kwargs)
160
161 def buildProtocol(self, address):
162 """Build an SSH protocol instance, logging the event.
163@@ -87,58 +127,51 @@
164 notify(accesslog.AuthenticationFailed(transport))
165 notify(accesslog.UserDisconnected(transport))
166
167- def _loadKey(self, key_filename):
168- key_directory = config.codehosting.host_key_pair_path
169- key_path = os.path.join(config.root, key_directory, key_filename)
170- return Key.fromFile(key_path)
171-
172 def getPublicKeys(self):
173 """Return the server's configured public key.
174
175 See `SSHFactory.getPublicKeys`.
176 """
177- public_key = self._loadKey('ssh_host_key_rsa.pub')
178- return {'ssh-rsa': public_key}
179+ return {'ssh-rsa': self._public_key}
180
181 def getPrivateKeys(self):
182 """Return the server's configured private key.
183
184 See `SSHFactory.getPrivateKeys`.
185 """
186- private_key = self._loadKey('ssh_host_key_rsa')
187- return {'ssh-rsa': private_key}
188+ return {'ssh-rsa': self._private_key}
189
190
191 class SSHService(service.Service):
192 """A Twisted service for the codehosting SSH server."""
193
194- def __init__(self):
195- self.service = self.makeService()
196-
197- def makePortal(self):
198- """Create and return a `Portal` for the SSH service.
199-
200- This portal accepts SSH credentials and returns our customized SSH
201- avatars (see `lp.codehosting.sshserver.auth.LaunchpadAvatar`).
202- """
203- authentication_proxy = Proxy(
204- config.codehosting.authentication_endpoint)
205- branchfs_proxy = Proxy(config.codehosting.branchfs_endpoint)
206- return get_portal(authentication_proxy, branchfs_proxy)
207-
208- def makeService(self):
209- """Return a service that provides an SFTP server. This is called in
210- the constructor.
211+ def __init__(self, portal, private_key_path, public_key_path,
212+ strport='tcp:22', idle_timeout=3600, banner=None):
213+ """Construct an SSH service.
214+
215+ :param portal: The `Portal` that turns authentication requests into
216+ views on the system.
217+ :param private_key_path: The path to the SSH server's private key.
218+ :param public_key_path: The path to the SSH server's public key.
219+ :param strport: The port to run the server on, expressed in Twisted's
220+ "strports" mini-language. Defaults to 'tcp:22'.
221+ :param idle_timeout: The number of seconds to wait before killing a
222+ connection that isn't doing anything. Defaults to 3600.
223+ :param banner: An announcement printed to users when they connect.
224+ By default, announce nothing.
225 """
226 ssh_factory = TimeoutFactory(
227- Factory(self.makePortal()),
228- timeoutPeriod=config.codehosting.idle_timeout)
229- return strports.service(config.codehosting.port, ssh_factory)
230+ Factory(
231+ portal,
232+ private_key=Key.fromFile(private_key_path),
233+ public_key=Key.fromFile(public_key_path),
234+ banner=banner),
235+ timeoutPeriod=idle_timeout)
236+ self.service = strports.service(strport, ssh_factory)
237
238 def startService(self):
239 """Start the SSH service."""
240- accesslog.LoggingManager().setUp(
241- configure_oops_reporting=True, mangle_stdout=True)
242+ accesslog.LoggingManager().setUp(configure_oops_reporting=True)
243 notify(accesslog.ServerStarting())
244 # By default, only the owner of files should be able to write to them.
245 # Perhaps in the future this line will be deleted and the umask
246
247=== modified file 'lib/lp/codehosting/sshserver/tests/test_auth.py'
248--- lib/lp/codehosting/sshserver/tests/test_auth.py 2010-03-19 10:43:51 +0000
249+++ lib/lp/codehosting/sshserver/tests/test_auth.py 2010-04-15 13:43:32 +0000
250@@ -1,4 +1,4 @@
251-# Copyright 2009 Canonical Ltd. This software is licensed under the
252+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
253 # GNU Affero General Public License version 3 (see the file LICENSE).
254
255 import os
256@@ -193,11 +193,6 @@
257
258 layer = TwistedLayer
259
260- banner_conf = """
261- [codehosting]
262- banner: banner
263- """
264-
265 def setUp(self):
266 UserAuthServerMixin.setUp(self)
267 self.portal.registerChecker(MockChecker())
268@@ -240,25 +235,22 @@
269 self.assertMessageOrder([userauth.MSG_USERAUTH_SUCCESS])
270 return d.addCallback(check)
271
272- def test_configuredBannerSentOnSuccess(self):
273- # If a banner is set in the codehosting config then we send it to the
274+ def test_defaultBannerSentOnSuccess(self):
275+ # If a banner was passed to the user auth agent then we send it to the
276 # user when they log in.
277- config.push('codehosting_overlay', self.banner_conf)
278+ self.user_auth._banner = "Boogedy boo"
279 d = self.requestSuccessfulAuthentication()
280 def check(ignored):
281 self.assertMessageOrder(
282 [userauth.MSG_USERAUTH_BANNER, userauth.MSG_USERAUTH_SUCCESS])
283- self.assertBannerSent(config.codehosting.banner + '\r\n')
284- def cleanup(ignored):
285- config.pop('codehosting_overlay')
286- return ignored
287- return d.addCallback(check).addBoth(cleanup)
288+ self.assertBannerSent(self.user_auth._banner + '\r\n')
289+ return d.addCallback(check)
290
291- def test_configuredBannerSentOnlyOnce(self):
292+ def test_defaultBannerSentOnlyOnce(self):
293 # We don't send the banner on each authentication attempt, just on the
294 # first one. It is usual for there to be many authentication attempts
295 # per SSH session.
296- config.push('codehosting_overlay', self.banner_conf)
297+ self.user_auth._banner = "Boogedy boo"
298
299 d = self.requestUnsupportedAuthentication()
300 d.addCallback(lambda ignored: self.requestSuccessfulAuthentication())
301@@ -268,17 +260,14 @@
302 self.assertMessageOrder(
303 [userauth.MSG_USERAUTH_FAILURE, userauth.MSG_USERAUTH_BANNER,
304 userauth.MSG_USERAUTH_SUCCESS])
305- self.assertBannerSent(config.codehosting.banner + '\r\n')
306-
307- def cleanup(ignored):
308- config.pop('codehosting_overlay')
309- return ignored
310- return d.addCallback(check).addBoth(cleanup)
311-
312- def test_configuredBannerNotSentOnFailure(self):
313- # Failed authentication attempts do not get the configured banner
314+ self.assertBannerSent(self.user_auth._banner + '\r\n')
315+
316+ return d.addCallback(check)
317+
318+ def test_defaultBannerNotSentOnFailure(self):
319+ # Failed authentication attempts do not get the default banner
320 # sent.
321- config.push('codehosting_overlay', self.banner_conf)
322+ self.user_auth._banner = "You come away two hundred quid down"
323
324 d = self.requestFailedAuthentication()
325
326@@ -287,11 +276,7 @@
327 [userauth.MSG_USERAUTH_BANNER, userauth.MSG_USERAUTH_FAILURE])
328 self.assertBannerSent(MockChecker.error_message + '\r\n')
329
330- def cleanup(ignored):
331- config.pop('codehosting_overlay')
332- return ignored
333-
334- return d.addCallback(check).addBoth(cleanup)
335+ return d.addCallback(check)
336
337 def test_loggedToBanner(self):
338 # When there's an authentication failure, we display an informative
339@@ -532,7 +517,12 @@
340
341 def makeFactory(self):
342 """Create and start the factory that our SSH server uses."""
343- factory = service.Factory(auth.get_portal(None, None))
344+ factory = service.Factory(
345+ auth.get_portal(None, None),
346+ private_key=Key.fromFile(
347+ service.get_key_path(service.PRIVATE_KEY_FILE)),
348+ public_key=Key.fromFile(
349+ service.get_key_path(service.PUBLIC_KEY_FILE)))
350 factory.startFactory()
351 return factory
352