[MERGE] Refactor loopback/paramiko/openssh/ssh connection opening
John Arbash Meinel
john at arbash-meinel.com
Fri Aug 25 15:54:33 BST 2006
Andrew Bennetts wrote:
> Hi all,
>
> This bundle gets rid of all the "if vendor == 'xxx'" statements scattered
> throughout bzrlib/transport/sftp.py, and instead moves all the connection
> establishing logic into a new file, bzrlib/transport/ssh.py. Code that's
> purely about establishing an SSH connection has been moved out of sftp.py,
> leaving sftp.py with just SFTP logic.
>
> In addition to the moved code, ssh.py defines a bunch of new classes inheriting
> from SSHVendor, which provides a consistent interface to establishing an SFTP
> connection, regardless of the method used to do this. Soon it will also do a
> similar thing for opening pipes to a command run over SSH; the smart server will
> use this to run over SSH.
>
> It's likely that I've missed adding some helpful docstrings, but overall I think
> this is much cleaner than what was there before. I think some further tidying
> is possible (e.g. I think a bunch of the host key management code belongs in
> paramiko, and may in fact be there already), but nothing is worse than it was
> before, just moved :)
>
> -Andrew.
>
>
I like the overall refactoring. I think you might be missing a couple
steps, but nothing critical at this point.
v-- In general, all of you SSHVendor children have a 'connect_sftp'
method which connects to SSH and then wraps it with a SFTPClient.
I think it would make sense to break out the SSH connection, and then
you don't have to re-implement it in the future. However, these changes
can be done at a different time, when you actually go to use ssh
connections.
> +class SSHVendor(object):
> + """Abstract base class for SSH vendor implementations."""
> +
> + def connect_sftp(self, username, password, host, port):
> + """Make an SSH connection, and return an SFTPClient.
> +
> + :param username: an ascii string
> + :param password: an ascii string
> + :param host: a host name as an ascii string
> + :param port: a port number
> + :type port: int
> +
> + :raises: ConnectionError if it cannot connect.
> +
> + :rtype: paramiko.sftp_client.SFTPClient
> + """
> + raise NotImplementedError(self.connect_sftp)
> +
> + def connect_ssh(self, username, password, host, port, command):
> + """Make an SSH connection, and return a pipe-like object."""
> + raise NotImplementedError(self.connect_ssh)
> +
^-- I realize you haven't implemented this yet. but it seems you would
need more than a 'pipe-like' object. Either you need 2 pipes for
bidirectional support, or you need a socket-like object. Or maybe even a
real class.
> +
> +class LoopbackVendor(SSHVendor):
> + """SSH "vendor" that connects over a plain TCP socket, not SSH."""
> +
> + def connect_sftp(self, username, password, host, port):
> + sock = socket.socket()
> + try:
> + sock.connect((host, port))
> + except socket.error, e:
> + raise ConnectionError('Unable to connect to SSH host %s:%s: %s'
> + % (host, port, e))
> + return SFTPClient(LoopbackSFTP(sock))
> +
^- loopback is fairly unique, but I think it could be:
def connect_sftp(self, username, password, host, port):
return SFTPClient(LoopbackSFTP(self.connect_ssh()))
def connect_ssh(self, username, password, host, port):
sock = socket.socket()
try:
sock.connect((host, port))
except socket.error, e:
raise ConnectionError('Unable to connect to SSH host %s:%s: %s'
% (host, port, e))
return sock
> +
> +class ParamikoVendor(SSHVendor):
> + """Vendor that uses paramiko."""
> +
> + def connect_sftp(self, username, password, host, port):
> + global SYSTEM_HOSTKEYS, BZR_HOSTKEYS
> +
> + load_host_keys()
> +
> + try:
> + t = paramiko.Transport((host, port or 22))
> + t.set_log_channel('bzr.paramiko')
> + t.start_client()
> + except (paramiko.SSHException, socket.error), e:
> + raise ConnectionError('Unable to reach SSH host %s:%s: %s'
> + % (host, port, e))
> +
> + server_key = t.get_remote_server_key()
> + server_key_hex = paramiko.util.hexify(server_key.get_fingerprint())
> + keytype = server_key.get_name()
> + if SYSTEM_HOSTKEYS.has_key(host) and SYSTEM_HOSTKEYS[host].has_key(keytype):
> + our_server_key = SYSTEM_HOSTKEYS[host][keytype]
> + our_server_key_hex = paramiko.util.hexify(our_server_key.get_fingerprint())
> + elif BZR_HOSTKEYS.has_key(host) and BZR_HOSTKEYS[host].has_key(keytype):
> + our_server_key = BZR_HOSTKEYS[host][keytype]
> + our_server_key_hex = paramiko.util.hexify(our_server_key.get_fingerprint())
> + else:
> + warning('Adding %s host key for %s: %s' % (keytype, host, server_key_hex))
> + if not BZR_HOSTKEYS.has_key(host):
> + BZR_HOSTKEYS[host] = {}
> + BZR_HOSTKEYS[host][keytype] = server_key
> + our_server_key = server_key
> + our_server_key_hex = paramiko.util.hexify(our_server_key.get_fingerprint())
> + save_host_keys()
> + if server_key != our_server_key:
> + filename1 = os.path.expanduser('~/.ssh/known_hosts')
> + filename2 = pathjoin(config_dir(), 'ssh_host_keys')
> + raise TransportError('Host keys for %s do not match! %s != %s' % \
> + (host, our_server_key_hex, server_key_hex),
> + ['Try editing %s or %s' % (filename1, filename2)])
> +
> + _paramiko_auth(username, password, host, t)
> +
Everything up to here is actually connecting to SSH, only the
'open_sftp_client()' step is creating a sftp connection.
Though I realize 'paramiko.Transport' is probably not a 'pipe like
object'. So probably you would need a helper function on the class that
returns a connected paramiko.Transport, and go from there.
> + try:
> + sftp = t.open_sftp_client()
> + except paramiko.SSHException, e:
> + raise ConnectionError('Unable to start sftp client %s:%d' %
> + (host, port), e)
> + return sftp
> +
> +
...
....
> +def os_specific_subprocess_params():
> + """Get O/S specific subprocess parameters."""
> + if sys.platform == 'win32':
> + # setting the process group and closing fds is not supported on
> + # win32
> + return {}
> + else:
> + # We close fds other than the pipes as the child process does not need
> + # them to be open.
> + #
> + # We also set the child process to ignore SIGINT. Normally the signal
> + # would be sent to every process in the foreground process group, but
> + # this causes it to be seen only by bzr and not by ssh. Python will
> + # generate a KeyboardInterrupt in bzr, and we will then have a chance
> + # to release locks or do other cleanup over ssh before the connection
> + # goes away.
> + # <https://launchpad.net/products/bzr/+bug/5987>
> + #
> + # Running it in a separate process group is not good because then it
> + # can't get non-echoed input of a password or passphrase.
> + # <https://launchpad.net/products/bzr/+bug/40508>
> + return {'preexec_fn': _ignore_sigint,
> + 'close_fds': True,
> + }
> +
-- You need an extra space here
> +class SSHSubprocess(object):
> + """A socket-like object that talks to an ssh subprocess via pipes."""
> + # TODO: this class probably belongs in bzrlib/transport/ssh.py
> +
> + def __init__(self, proc):
> + self.proc = proc
> +
...
Otherwise I'm happy to see the proper things moved into ssh.py
I'm a little concerned that a short name like 'ssh.py' might conflict
with another module in the future. But only a little.
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060825/daf2bc22/attachment.pgp
More information about the bazaar
mailing list