|
|
Description crypto/ssh: ssh-agent forwarding support
Add an argument to the client config that creates a connection to the local
ssh-agent. If this argument is non-nil, the ssh library will send
a channel request to the server to enable agent forwarding when the session
is started (session.NewSession()).
When the remote shell tries to connect to another host, the server will
open an auth channel with the client. If the client has enabled agent forwarding,
the client will call the connector and link/proxy the ssh channel with the agent
socket.
Note: this changeset protects against a malicious server requesting an agent
forwarding channel when the client did not request one.
The flow of how this all works was cribbed from the openssh sources.
Fixes #6223
Patch Set 1 #Patch Set 2 : diff -r 1e7a3e301825 https://code.google.com/p/go.crypto #Patch Set 3 : diff -r 1e7a3e301825 https://code.google.com/p/go.crypto #Patch Set 4 : diff -r 1e7a3e301825 https://code.google.com/p/go.crypto #
Total comments: 3
Patch Set 5 : diff -r 1e7a3e301825 https://code.google.com/p/go.crypto #
Total comments: 6
Patch Set 6 : diff -r 1e7a3e301825 https://code.google.com/p/go.crypto #Patch Set 7 : diff -r 1e7a3e301825 https://code.google.com/p/go.crypto #Patch Set 8 : diff -r 1e7a3e301825 https://code.google.com/p/go.crypto #
Total comments: 2
MessagesTotal messages: 31
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.crypto
Sign in to reply to this message.
On 2013/08/22 19:32:49, jamwt wrote: > Hello mailto:golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go.crypto Example of use: https://gist.github.com/anonymous/ea32f3df09bd7b5eb98d
Sign in to reply to this message.
I'm not privileged to approve things, but this looks good to me. :-) You just beat me to it.
Sign in to reply to this message.
One possible change to the protocol. Have Connect() accept an io.Reader and io.WriteCloser. If the user wants to hook up io.Copy goroutines, he can. You could even provide an adapter if you think this is a common use-case.
Sign in to reply to this message.
https://codereview.appspot.com/12837048/diff/9001/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/12837048/diff/9001/ssh/client.go#newcode425 ssh/client.go:425: ch.remoteWin.add(msg.PeersWindow) This needs to be moved down. If the window is opened here, you could send data before sending OpenConfirm.
Sign in to reply to this message.
https://codereview.appspot.com/12837048/diff/9001/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/12837048/diff/9001/ssh/client.go#newcode425 ssh/client.go:425: ch.remoteWin.add(msg.PeersWindow) On 2013/08/23 18:50:37, jpsugar wrote: > This needs to be moved down. If the window is opened here, you could send data > before sending OpenConfirm. I see what you're saying, but I copied the pattern from forwarded-tcpip (the clause above); so you think the concern applies to both forwarding modes?
Sign in to reply to this message.
https://codereview.appspot.com/12837048/diff/9001/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/12837048/diff/9001/ssh/client.go#newcode425 ssh/client.go:425: ch.remoteWin.add(msg.PeersWindow) On 2013/08/23 19:17:46, jamwt wrote: > On 2013/08/23 18:50:37, jpsugar wrote: > > This needs to be moved down. If the window is opened here, you could send data > > before sending OpenConfirm. > > I see what you're saying, but I copied the pattern from forwarded-tcpip (the > clause above); so you think the concern applies to both forwarding modes? No, because there cannot be any data sent on the forwarded connection until l <- forward{ch, raddr}. You could move the c.agentForward() line down instead, I suppose, but I feel it is safer to just leave the window closed until OpenConfirm is sent. Defense in depth and all. :) If you switch to sending the ch.stdin/stdout to Connect(), you will need to move the window since you don't have a split operation.
Sign in to reply to this message.
FYI my use case for this involves an in-process synthetic agent, so I have a strong preference for avoiding the io.Copy goroutines in that case.
Sign in to reply to this message.
On 2013/08/23 19:55:35, jpsugar wrote: > FYI my use case for this involves an in-process synthetic agent, so I have a > strong preference for avoiding the io.Copy goroutines in that case. Okay, thanks for the explanation. I've moved the proxying line down to prevent a packet being forwarded on the channel before the channel open response (ala the normal forwarding). On the API front, I'm inclined to leave it as is just b/c it fits the common case so well; in the _uncommon_ case, couldn't you still write a connector that returns a "smart" object that implements readwritecloser? It could be an in-memory thing speaking the protocol. I don't think the use of io.Copy() would preclude this, unless I'm missing something... - Jamie
Sign in to reply to this message.
On 2013/08/24 04:59:41, jamwt wrote: > On the API front, I'm inclined to leave it as is just b/c it fits the common > case so well; in the _uncommon_ case, couldn't you still write a connector that > returns a "smart" object that implements readwritecloser? It could be an > in-memory thing speaking the protocol. I don't think the use of io.Copy() would > preclude this, unless I'm missing something... Doesn't preclude, no, but just makes me twitch. :) - JP
Sign in to reply to this message.
https://codereview.appspot.com/12837048/diff/21001/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode222 ssh/client.go:222: go io.Copy(agentSock, ch.stdout) You need to EOF agentSock somehow here. Possibly via Close. https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode224 ssh/client.go:224: go io.Copy(ch.stdin, agentSock) You need to close ch.stdin after this.
Sign in to reply to this message.
https://codereview.appspot.com/12837048/diff/21001/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode41 ssh/client.go:41: // AgentConnector connects to the proper ssh-agent Rename AgentDialer, with method Dial. (This is similar to net.Dial, although the signature is different because no network and address is given.) https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode219 ssh/client.go:219: // has enabled proxying missing ) https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode418 ssh/client.go:418: if c.config.ForwardingAgentConnector != nil { Seems that this condition could be inverted and an early return used to save a level of indentation in the following code. https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode543 ssh/client.go:543: // Connector to the local ssh-agent for key forwarding. // AgentForwardingDialer contains a Dialer that is used to connect to the SSH agent. If nil, no key ...
Sign in to reply to this message.
Also need periods at the ends of all the sentences in comments.
Sign in to reply to this message.
On 2013/08/26 17:45:22, jpsugar wrote: > Also need periods at the ends of all the sentences in comments. Thanks for the review. I have made all those changes except short-circuiting the condition in the case statement. My reasoning is this: This would be the only case statement (currently) that aborts via return. Defensive coding would suggest _not_ returning to protect against a future author adding more code in the body of this function after the switch() statement on the assumption that all cases will continue execution of the function body after the case block. Thoughts?
Sign in to reply to this message.
On 2013/08/26 18:27:25, jamwt wrote: > On 2013/08/26 17:45:22, jpsugar wrote: > > Also need periods at the ends of all the sentences in comments. > > Thanks for the review. I have made all those changes except short-circuiting > the condition in the case statement. My reasoning is this: > > This would be the only case statement (currently) that aborts via return. > Defensive coding would suggest _not_ returning to protect against a future > author adding more code in the body of this function after the switch() > statement on the assumption that all cases will continue execution of the > function body after the case block. > > Thoughts? Nevermind, I withdraw this line of thinking. The forwarded-tcpip case already makes liberal use of return. Okay, I agree, I'll implement that change too.
Sign in to reply to this message.
On 2013/08/26 18:28:39, jamwt wrote: > On 2013/08/26 18:27:25, jamwt wrote: > > On 2013/08/26 17:45:22, jpsugar wrote: > > > Also need periods at the ends of all the sentences in comments. > > > > Thanks for the review. I have made all those changes except short-circuiting > > the condition in the case statement. My reasoning is this: > > > > This would be the only case statement (currently) that aborts via return. > > Defensive coding would suggest _not_ returning to protect against a future > > author adding more code in the body of this function after the switch() > > statement on the assumption that all cases will continue execution of the > > function body after the case block. > > > > Thoughts? > > Nevermind, I withdraw this line of thinking. The forwarded-tcpip case already > makes liberal use of return. > > Okay, I agree, I'll implement that change too. In that spirit I've done an early return on the second error condition as well (failure to connect to the agent).
Sign in to reply to this message.
https://codereview.appspot.com/12837048/diff/33002/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/12837048/diff/33002/ssh/client.go#newcode41 ssh/client.go:41: // AgentDialer connects to the proper ssh-agent Un-wrap this comment now? Lines seem a little short. https://codereview.appspot.com/12837048/diff/33002/ssh/session.go File ssh/session.go (right): https://codereview.appspot.com/12837048/diff/33002/ssh/session.go#newcode229 ssh/session.go:229: req := channelRequestMsg{ Please run gofmt. These appear to be spaces.
Sign in to reply to this message.
On 2013/08/26 19:13:33, jpsugar wrote: > https://codereview.appspot.com/12837048/diff/33002/ssh/client.go > File ssh/client.go (right): > > https://codereview.appspot.com/12837048/diff/33002/ssh/client.go#newcode41 > ssh/client.go:41: // AgentDialer connects to the proper ssh-agent > Un-wrap this comment now? Lines seem a little short. > > https://codereview.appspot.com/12837048/diff/33002/ssh/session.go > File ssh/session.go (right): > > https://codereview.appspot.com/12837048/diff/33002/ssh/session.go#newcode229 > ssh/session.go:229: req := channelRequestMsg{ > Please run gofmt. These appear to be spaces. I have been running gofmt.. maybe I'm running the wrong one? `hg gofmt`. IME the codereview tool will refuse to upload if it determines gofmt needs to be run.
Sign in to reply to this message.
This is close enough that I can fix when landing, but I don't see jamwt at dropbox.com in the CLA list. Have you submitted a CLA? http://golang.org/doc/contribute.html#copyright
Sign in to reply to this message.
On 2013/08/27 16:35:29, agl1 wrote: > This is close enough that I can fix when landing, but I don't see jamwt at > http://dropbox.com in the CLA list. Have you submitted a CLA? > http://golang.org/doc/contribute.html#copyright I'm working with the folks here at dropbox to get that done, bear with me!
Sign in to reply to this message.
On 2013/08/28 04:29:32, jamwt wrote: > On 2013/08/27 16:35:29, agl1 wrote: > > This is close enough that I can fix when landing, but I don't see jamwt at > > http://dropbox.com in the CLA list. Have you submitted a CLA? > > http://golang.org/doc/contribute.html#copyright > > I'm working with the folks here at dropbox to get that done, bear with me! Update: Dropbox has sent a signed contribution agreement to Google with me listed as a contributor. Not sure what needs to be done to process that.
Sign in to reply to this message.
is there any way that we can add a test for this?
Sign in to reply to this message.
A+C change: https://codereview.appspot.com/14765043/
Sign in to reply to this message.
Jamie, would you please merge your CL with tip and remail it, hg mail 12837048 abort: codereview issue 12837048 is out of date: patch and recent changes conflict (1e7a3e301825->5ff5636e18c9) On Thu, Oct 17, 2013 at 11:31 AM, David Symonds <dsymonds@golang.org> wrote: > A+C change: https://codereview.appspot.com/14765043/ > > -- > > --- > You received this message because you are subscribed to the Google Groups "golang-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out.
Sign in to reply to this message.
On 2013/10/17 00:36:28, dfc wrote: > Jamie, would you please merge your CL with tip and remail it, hg mail 12837048 > > > abort: codereview issue 12837048 is out of date: patch and recent > changes conflict (1e7a3e301825->5ff5636e18c9) > > On Thu, Oct 17, 2013 at 11:31 AM, David Symonds <mailto:dsymonds@golang.org> wrote: > > A+C change: https://codereview.appspot.com/14765043/ > > > > -- > > > > --- > > You received this message because you are subscribed to the Google Groups > "golang-dev" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email to mailto:golang-dev+unsubscribe@googlegroups.com. > > For more options, visit https://groups.google.com/groups/opt_out. ping, please also take the opportunity to change the description to go.crypto/ssh: add ssh-agent forwarding support
Sign in to reply to this message.
ping, please also take the opportunity to change the description to go.crypto/ssh: add ssh-agent forwarding support
Sign in to reply to this message.
Replacing golang-dev with golang-codereviews.
Sign in to reply to this message.
Replacing golang-dev with golang-codereviews.
Sign in to reply to this message.
This should be migrated to gosshnew. - JP
Sign in to reply to this message.
On 2013/12/20 16:56:55, jpsugar wrote: > This should be migrated to gosshnew. > > - JP Jamie please re-propose this change on top of the gosshnew repo where most of the development is landing at the moment. https://code.google.com/p/gosshnew/ R=close
Sign in to reply to this message.
On 2013/12/20 22:25:38, dfc wrote: > On 2013/12/20 16:56:55, jpsugar wrote: > > This should be migrated to gosshnew. > > > > - JP > > Jamie please re-propose this change on top of the gosshnew repo where most of > the development is landing at the moment. > > https://code.google.com/p/gosshnew/ > > R=close I've added agent forwarding to gosshnew myself. R=close
Sign in to reply to this message.
|