Opened 12 years ago
Closed 9 years ago
#4698 enhancement closed fixed (fixed)
client endpoint: command executed over ssh using Twisted Conch
Reported by: | Jean-Paul Calderone | Owned by: | Jean-Paul Calderone |
---|---|---|---|
Priority: | normal | Milestone: | |
Component: | conch | Keywords: | endpoint |
Cc: | Branch: |
branches/sshcommandendpoint-4698-4
branch-diff, diff-cov, branch-cov, buildbot |
|
Author: | exarkun |
Description
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.
Change History (18)
comment:1 Changed 12 years ago by
comment:2 Changed 11 years ago by
Keywords: | endpoint added |
---|
comment:3 Changed 11 years ago by
Owner: | z3p deleted |
---|
comment:5 Changed 9 years ago by
Owner: | set to Jean-Paul Calderone |
---|
comment:6 Changed 9 years ago by
Author: | → exarkun |
---|---|
Branch: | → branches/sshcommandendpoint-4698 |
(In [36773]) Branching to 'sshcommandendpoint-4698'
comment:7 Changed 9 years ago by
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.
comment:8 Changed 9 years ago by
Branch: | branches/sshcommandendpoint-4698 → branches/sshcommandendpoint-4698-2 |
---|
(In [37637]) Branching to 'sshcommandendpoint-4698-2'
comment:9 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
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.
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.
comment:10 Changed 9 years ago by
Owner: | set to 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.
comment:11 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from therve to Jean-Paul Calderone |
-
# 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.
comment:12 Changed 9 years ago by
Branch: | branches/sshcommandendpoint-4698-2 → branches/sshcommandendpoint-4698-3 |
---|
(In [37672]) Branching to 'sshcommandendpoint-4698-3'
comment:13 Changed 9 years ago by
Keywords: | review added |
---|---|
Owner: | Jean-Paul Calderone deleted |
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).
comment:14 Changed 9 years ago by
Keywords: | review removed |
---|---|
Owner: | set to Jean-Paul Calderone |
- 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.
- 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.
- 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.
- 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.
- 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 :).
comment:15 Changed 9 years ago by
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).
comment:16 Changed 9 years ago by
Looks great, please merge, and open a couple of bugs summarizing some possible improvements.
comment:17 Changed 9 years ago by
Branch: | branches/sshcommandendpoint-4698-3 → branches/sshcommandendpoint-4698-4 |
---|
(In [37950]) Branching to 'sshcommandendpoint-4698-4'
comment:18 Changed 9 years ago by
Resolution: | → fixed |
---|---|
Status: | new → closed |
An incomplete sketch which may be helpful to a would-be implementor: http://twistedmatrix.com/~exarkun/sshendpoint.py