[MERGE] [0.11] Working bzr+ssh://, make "bzr serve --inet" read-only by default.

[Andrew Bennetts]

John Arbash Meinel wrote:
> Andrew Bennetts wrote:
> > Hi all,
> > 
> > http://people.ubuntu.com/~andrew/bzr/bzr-ssh-testing/ has some changes I'd like
> > to see merged, ideally for 0.11 despite the freeze:
[...]
> 
> First, a small comment that if you want to get something reviewed
> quickly, it really is easier to review if you include at least a diff.
> (A bundle can be nice too, since we can directly merge from that, but a
> diff at a minimum, so that we can make comments directly, rather than
> looking at it separately.)

Oops.  I meant to do that, but I forgot.

> Can we ensure that doing 'del transport' actually cleans up the
> connection? (I assume you are doing this because you want to avoid
> getting an exception from the Transport object, which thinks it is still
> connected).
> 
> I'm fine with doing it, but it seems like we may have an api deficiency
> if you need to do it.

Unfortunately, there's no good way to inform a transport that it's no longer
wanted, and so should release any resources it may have acquired (like
connections to remote hosts).

I don't think the 'del transport' statements are really necessary, though.
assertInetServerShutsdownCleanly will forcefully disconnect.  Removing the del
lines doesn't alter the results of the test run.

I'll check with Robert; I know they make Robert a bit comfortable, because it's
the closest thing we have to a "close this transport" API, so it's arguably more
correct, but I'd be happy to see those statements disappear.

> The ugly hack that is StubSSHServer...
> 
> Using 1 thread per file is how subprocess.py does I/O in subprocess.py
> (if running under windows). So I don't know that there is much else we
> can do. I know paramiko uses threads as well, so it is easily possible
> that this is the best we can do.

I'm not so much bothered by the gratuitous threads (which I agree are the
easiest way to do this portably) or even the way it operates on single bytes, as
much as the fact I have to write it at all.  I would have thought that paramiko
itself would already have implemented, or at least an example of, a non-trivial
check_channel_exec_request like this.  But I could find no code in paramiko to
help me here, which seems to be a strange lack -- surely hooking up a subprocess
to a channel is a common enough use-case?  I guess not.

Basically, aside from the initial line where I keep track of the command the
server was asked to execute, I want the rest of check_channel_exec_request to
just invoke the default implementation.  It's just that apparently no such thing
exists!

> I'm a little concerned that you combine stdout with stderr and send them
> both over the pipe. I would assume real ssh uses a separate channel for
> stderr. This is probably okay for now, but I have the feeling it
> indicates a limitation of our model if we expect it to work that way.

Oops, that wasn't intentional.  I've fixed that; the implementation of
check_channel_exec_request is now:

    def check_channel_exec_request(self, channel, command):
        [...]
        
        # XXX: horribly inefficient, not to mention ugly.
        # Start a thread for each of stdin/out/err, and relay bytes from
        # the subprocess to channel and vice versa.
        def ferry_bytes(read, write, close):
            while True:
                bytes = read(1)
                if bytes == '':
                    close()
                    break
                write(bytes)

        file_functions = [
            (channel.recv, proc.stdin.write, proc.stdin.close),
            (proc.stdout.read, channel.sendall, channel.close),
            (proc.stderr.read, channel.sendall_stderr, channel.close)]
        for read, write, close in file_functions:
            t = threading.Thread(
                target=ferry_bytes, args=(read, write, close))
            t.start()

        return True

> It seems like 'self.command_executed' should actually be a list, so we
> can tell if there are multiple connection attempts.

Good idea.

> Also, it would probably be better to have the StubSSHServer not defined
> inside the test function, so that it could be accessed by other tests in
> the future.

That would be nice, but it's awkward.  StubSSHServer needs paramiko to be
present, and it needs a reference to the test case.  So it'd need to be guarded
from ImportErrors (not needed inside the test method, because it will raise
TestSkipped).  Also, it needs a reference to the test case to stash the
command_executed value, and despite the 'test_case' parameter in its __init__,
it's actually passed an SFTPServer when constructed.  Oh, I guess I can use
'logs' attribute of SFTPServer instead.

Ok, I've made that change.

> Why are we translating ReadOnlyError => TransportNotPossible. It seems
> like we would want to tell the user that the remote directory is readonly.

I'll let Robert answer this.

> How do you handle ssh connecting to the local SSH connection without a
> password?

The same way as for SFTP -- the authentication code paths are basically the
same.  I only specify a username and password in the test because that's what
the StubServer expects -- I considered allowing 'none' auth instead of
'password' in the test, but I'm not sure how to make paramiko perform 'none'
auth as a client.

> Anyway, the changes look good. And while I don't think it is a trivial
> change, it does fit Robert's new definition of trivial (does not
> destabilize existing apis).
> 
> So I have some comments, but nothing that would really block it from
> 0.11. (+0.9, which you can upgrade to 1.0 if you like)

I hope the changes I've made earn the extra +0.1 ;)

Thanks for the review!

-Andrew.