client endpoint: command executed over ssh using Twisted Conch

[Jean-Paul Calderone]

It should be possible to create a client endpoint like this:

clientFromString('ssh:host=example.com:port=22:username=alice:privateKeyFile=~/.ssh/id_rsa:command=cat /etc/passwd')

The password argument should also be supported.

This endpoint would establish an SSH connection to the specified address and execute the specified command. The endpoint would connect the protocol to the stdio of the executed command.

[Glyph]

An incomplete sketch which may be helpful to a would-be implementor: ​http://twistedmatrix.com/~exarkun/sshendpoint.py

[Jean-Paul Calderone]

#1975 was a lot like this, but before we had endpoints.

[Jean-Paul Calderone]

(In [36773]) Branching to 'sshcommandendpoint-4698'

[Glyph]

Just noticed some commits to this branch today, and can I just say (A) it is so awesome that you're working on this, and (B) oh my goodness this takes a depressing amount of code. We should probably work on (B) a bit later, but the fact that you're working it all out now and wrapping it into a nice, abstract bundle just makes (A) that much more awesome.

[Jean-Paul Calderone]

(In [37637]) Branching to 'sshcommandendpoint-4698-2'

[Jean-Paul Calderone]

Okay, I think this could be reviewed now. It is a bit more code than I would prefer to submit all at once, but my plan for splitting it up fell through. If a reviewer feels it is too long to make sense of, I am happy to find another way to split it into smaller pieces.

​Build results

Note that despite the clientFromString example in the description, I did not implement a plugin for this endpoint in this branch. There's enough code here already, that can come later.

[therve]

• Some pydoctor failures:

  twisted.conch.endpoints.SSHCommandEndpoint.newConnection: invalid ref to KnownHostsKey
  twisted.conch.endpoints.SSHCommandEndpoint.newConnection: invalid ref to NoneType
  twisted.conch.endpoints.UserAuth.getPublicKey: invalid ref to NoneType
  twisted.conch.endpoints._CommandTransport.verifyHostKey: invalid ref to IKnownHosts
  twisted.conch.endpoints._CommandTransport.verifyHostKey: invalid ref to IKnownHosts.verifyHostKey
  twisted.conch.endpoints._ConnectionReady: invalid ref to twisted.conch.ssh.transport.service.SSHService
  

• There are several failures in the build, mostly about skipping tests when dependencies are missing. But there is also one weird failure in the fedora builder.

• Some twistedchecker failures too

• +      from that protocol with input the input of that command.  Effectively
  

  "with the input" ?

• It'd be nice if the examples were a little bit more conform to the coding standards.
• The shared connection example is not linked in the howto.

• -            return klass(remoteWindow = windowSize, 
  -                         remoteMaxPacket = maxPacket, 
  +            return klass(remoteWindow = windowSize,
  +                         remoteMaxPacket = maxPacket,
  

  Maybe you can take the opportunity to remove the extra whitespaces.
• An SSH session could not be established A SSH session.
• The @param for commandConnected is empty.
• We need to think about stderr
• Same thing with exit-status

• class _ConnectionReady(SSHConnection):
      """
      L{_ConnectionReady} is an L{twisted.conch.ssh.transport.service.SSHService}
  

  Any reason you advertize the Service inheritance instead of Connection?

•         @param factory: Some random object with a C{connectionReady} attribute   
              which is a L{Deferred} which will get fired when I{serviceStarted}   
              happens.  TODO This is a poor docstring
  

  Indeed.
• UserAuth can be private I think.

• reason = Failure(AuthenticationFailed("Doh")) meaningful error message or something.

• L{SSHCommandEndpoint} can set up a new SSH, a new SSH connection?

• _KNOWN_HOSTS = "~/.ssh/known_hosts" It'd be nice if it were somewhere else.

• # failure. One should attempt should be sufficient to test authentication extra "should"

•   # factory = _SSHSecureConnectionFactory()
    # secure = factory.getSecureConnection()
    # authenticated = secure.getAuthenticatedConnection()
    # command = authenticated.getCommandChannel()
    # transport = command.getTransport()
    # clientProtocol.makeConnection(transport
  

  What's that?

•   class Composite(object):
        """
        A helper to compose other objects based on their declared (zope.interface)
        interfaces.
  

  It looks like it shouldn't be there maybe? Or just make MemoryReactor a Clock?
• SSHCommandEndpointTestsMixin should talk about create being a requirement. Maybe assertClientTransportState too, thought I'm not sure it makes sense to not have it in the mixin?

•           If a connection cannot be established, the L{Deferred} returned by
            L{SSHCommandEndpoint.connect} fires with a L{Failure} the reason for
            the connection setup failure.
  

  The second part of that sentence is weird.

Almost there.

[therve]

•        # Let the agent client talk with the agent server
         for i in range(20): # XXX Argh unroll this loop... or something?
  

  No XXX, or it needs a ticket number.

• finishConnection is another thing that needs to be document along with create.

*

          # helper = _NewConnectionHelper(*[None] * 10)
          helper = _NewConnectionHelper(
              None, None, None, None, None, None, None, None, None, None)

Maybe the first line should be removed?

Alright that seems to be it for now.

[Jean-Paul Calderone]

(In [37672]) Branching to 'sshcommandendpoint-4698-3'

[Jean-Paul Calderone]

Some pydoctor failures There are several failures in the build Some twistedchecker failures too + from that protocol with input the input

Think I got all of those.

It'd be nice if the examples were a little bit more conform to the coding standards.

Okey dokey.

The shared connection example is not linked in the howto.

Done

Maybe you can take the opportunity to remove the extra whitespaces.

Done

An SSH session could not be established A SSH session

I apologize for the confusingness (not a cromulent word) of english. I think "an SSH" is correct here (because of how the letter "S" is pronounced, starting with a vowel sound). However, thank you for your attention to detail.

The @param for commandConnected is empty

Oops! Fixed.

We need to think about stderr Same thing with exit-status

Yea. :( More on this at the end.

Any reason you advertize the Service inheritance instead of Connection?

Hmm. Not really, I suppose. Probably because I wanted to advertise an interface and then discovered there is no interface. Fixed.

TODO This is a poor docstring

Er indeed. Fixed.

UserAuth can be private I think

Yay good catch.

reason = Failure(AuthenticationFailed("Doh")) meaningful error message or something

Fixed

L{SSHCommandEndpoint} can set up a new SSH, a new SSH connection?

Fixed

_KNOWN_HOSTS = "~/.ssh/known_hosts" It'd be nice if it were somewhere else.

Refactored!!!!!!!!!

# failure. One should attempt should be sufficient to test authentication extra "should"

Fixed

[random comments] What's that?

Oops, some random comments. Deleted. Almost (except exit-status/stderr) items up to this point addressed in r37656.

class Composite(object):

Moved out to a separate ticket. Just using MemoryClockReactor (or whatever) now.

SSHCommandEndpointTestsMixin should talk about create being a requirement. Maybe assertClientTransportState too, thought I'm not sure it makes sense to not have it in the mixin?

Yes, good points about documenting requirements. Fixed that. I think the assertClientTransportState does make sense on the mixin, hopefully the documentation I wrote makes the justification for this clearer.

If a connection cannot be established, the L{Deferred} returned by L{SSHCommandEndpoint.connect} fires with a L{Failure} the reason for the connection setup failure.

Oops, fixed.

for i in range(20): # XXX Argh unroll this loop... or something?

Fixed... sort of, I think.

finishConnection is another thing that needs to be document along with create

Fixed.

Maybe the first line should be removed?

Ah yea I guess.

So, about stderr and exit-status. I implemented exit-status support because I guess there's an obvious thing to do for that. I made it so the protocol gets ProcessTerminated if there is a non-zero exit status. It still gets ConnectionDone if it is zero or if the server does not report the exit status. Handling stderr somehow seems like a good idea, but I guess you'll be happy if that comes along later in another ticket.

Thanks for the review! ​Build results (in progress).

[therve]

1. There are some test failures on Windows.

2.

          channel = _CommandChannel(
              self._creator, self._command, protocolFactory, commandConnected)
          channel._creator = self._creator

The channel._creator assignment looks redundant.

3. In the examples, the fact that you're calling readKey means that I have to type my passphrase everytime, even if the agent is used. Sadness.

4. I'm not sure I see the usefulness of ISSHConnectionCreator. It's unlikely that people will ever need to create such an object, or even instantiate one without the class methods. Can't we just rely on an private contract, and put a warning in init doctring ("don't call that, please use existingConnection or newConnection")? At least those 2 methods should be referenced, I think.

5. The examples still have a bit of boilerplate that seems to be useful default. Maybe we can open a bug to add a new class method to do this.

6. In the shared example, it's not really great to have to use the cat command to maintain an open connection, and then access "transport.conn" to get the need object. Do you have a plan for a better interface? I'm thinking a SSHClientEndpoint may be interesting, although I'm not sure what would be its interface.

OK! Please merge when at least 1-2 are fixed, and you thought at least 3sec on each of the remaining points :).

[Jean-Paul Calderone]

There are some test failures on Windows

Weee... SSHPublicKeyDatabase doesn't work on Windows for slightly lame reasons. :( I hope that #6405 deals with that. I'll double-check after that ticket is resolved.

The channel._creator assignment looks redundant

Oops, yes. Fixed in r37798.

In the examples, the fact that you're calling readKey means that I have to type my passphrase everytime, even if the agent is used. Sadness.

Yea, that does suck a little bit. Fixing it for real is about 70 more lines of example code. I tried another approach in r37928. Maybe it's better. For the long term, I think someone could implement a better ssh user auth client service (eg, it could have callbacks for getting various kinds of credentials just in time, instead of making it all happen up front as the code currently does) and the endpoint could benefit from that. I think I'll file some tickets to the tune of:

• remove the use of an "options" class from the twisted.conch.client.default.SSHUserAuthClient API
• convert the synchronous prompting in twisted.conch.client.default.SSHUserAuthClient to asynchronous
• make it possible to use twisted.conch.client.default.SSHUserAuthClient in unit testable code
• document the usage of twisted.conch.client.default.SSHUserAuthClient

. I'm not sure I see the usefulness of ISSHConnectionCreator. It's unlikely that people will ever need to create such an object, or even instantiate one without the class methods. Can't we just rely on an private contract, and put a warning in init doctring ("don't call that, please use existingConnection or newConnection")? At least those 2 methods should be referenced, I think.

Hm. I was initially convinced, but in light of the previous point, I think it's useful. If someone wanted to implement their own authentication hooks, this is exactly how they'd do it.

The examples still have a bit of boilerplate that seems to be useful default. Maybe we can open a bug to add a new class method to do this.

Something like SSHCommandClientEndpoint.figureItOutYourself()? Maybe with a couple optional parameters? Yea, makes sense.

In the shared example, it's not really great to have to use the cat command to maintain an open connection, and then access "transport.conn" to get the need object. Do you have a plan for a better interface? I'm thinking a SSHClientEndpoint may be interesting, although I'm not sure what would be its interface.

Yea, I agree, not so great. I guess I imagined someday there'd be a simple non-endpoint for setting up an SSH connection with conch, and then you could hand the result of that off to the endpoint. As I was typing that, though, I realized that there basically is that API already, it's _NewConnectionHelper.secureConnection. Perhaps that suggests something about that _.

You said merge after fixing the first couple points, but I wouldn't mind having another quick look - mostly at the new behavior of the example wrt passphrases and at my comments to the review (not much code has changed anyway).

[therve]

Looks great, please merge, and open a couple of bugs summarizing some possible improvements.

[Jean-Paul Calderone]

(In [37950]) Branching to 'sshcommandendpoint-4698-4'

[Jean-Paul Calderone]

(In [37954]) Merge sshcommandendpoint-4698-4

Author: exarkun Reviewer: therve Fixes: #4698

Add twisted.conch.endpoints.SSHCommandClientEndpoint, an IStreamClientEndpoint implementation which runs commands over ssh connections and associates protocols with their stdin/stdout.