FAQ
Reviewers: golang-dev1,

Message:
Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go.crypto


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.

Please review this at https://codereview.appspot.com/12837048/

Affected files:
    M ssh/client.go
    M ssh/session.go


Index: ssh/client.go
===================================================================
--- a/ssh/client.go
+++ b/ssh/client.go
@@ -38,6 +38,12 @@
    response chan interface{}
   }

+// AgentConnector connects to the proper ssh-agent
+// for key forwarding
+type AgentConnector interface {
+ Connect() (io.ReadWriteCloser, error)
+}
+
   // Client returns a new SSH client connection using c as the underlying
transport.
   func Client(c net.Conn, config *ClientConfig) (*ClientConn, error) {
    return clientWithAddress(c, "", config)
@@ -55,6 +61,7 @@
     conn.Close()
     return nil, fmt.Errorf("handshake failed: %v", err)
    }
+
    go conn.mainLoop()
    return conn, nil
   }
@@ -208,6 +215,16 @@
    return H, K, nil
   }

+// Run a very simple proxy back to the ssh-agent when
+// the server has requested an auth channel (and the client
+// has enabled proxying
+func (c *ClientConn) agentForward(agentSock io.ReadWriteCloser, ch
*clientChan) {
+ // Server -> Agent
+ go io.Copy(agentSock, ch.stdout)
+ // Agent -> Server
+ go io.Copy(ch.stdin, agentSock)
+}
+
   // mainLoop reads incoming messages and routes channel messages
   // to their respective ClientChans.
   func (c *ClientConn) mainLoop() {
@@ -398,15 +415,36 @@

     c.writePacket(marshal(msgChannelOpenConfirm, m))
     l <- forward{ch, raddr}
+ case "auth-agent@openssh.com":
+ if c.config.ForwardingAgentConnector != nil {
+ agentConn, err := c.config.ForwardingAgentConnector.Connect()
+ if err != nil {
+ c.sendConnectionFailed(msg.PeersId)
+ } else {
+ ch := c.newChan(c.transport)
+ ch.remoteId = msg.PeersId
+ ch.remoteWin.add(msg.PeersWindow)
+ ch.maxPacket = msg.MaxPacketSize
+ c.agentForward(agentConn, ch)
+
+ m := channelOpenConfirmMsg{
+ PeersId: ch.remoteId,
+ MyId: ch.localId,
+ MyWindow: 1 << 14,
+
+ // As per RFC 4253 6.1, 32k is also the minimum.
+ MaxPacketSize: 1 << 15,
+ }
+
+ c.writePacket(marshal(msgChannelOpenConfirm, m))
+ }
+ } else {
+ // Client did not ask for key forwarding!
+ c.sendUnknownChannel(msg.PeersId)
+ }
    default:
     // unknown channel type
- m := channelOpenFailureMsg{
- PeersId: msg.PeersId,
- Reason: UnknownChannelType,
- Message: fmt.Sprintf("unknown channel type: %v", msg.ChanType),
- Language: "en_US.UTF-8",
- }
- c.writePacket(marshal(msgChannelOpenFailure, m))
+ c.sendUnknownChannel(msg.PeersId)
    }
   }

@@ -438,6 +476,18 @@
    return c.writePacket(marshal(msgChannelOpenFailure, m))
   }

+// sendUnknownChannel rejects an incoming channel identified
+// by remoteId on the basis of invalid or unrecognized type
+func (c *ClientConn) sendUnknownChannel(remoteId uint32) error {
+ m := channelOpenFailureMsg{
+ PeersId: remoteId,
+ Reason: UnknownChannelType,
+ Message: "unknown or invalid channel type",
+ Language: "en_US.UTF-8",
+ }
+ return c.writePacket(marshal(msgChannelOpenFailure, m))
+}
+
   // parseTCPAddr parses the originating address from the remote into a
*net.TCPAddr.
   // RFC 4254 section 7.2 is mute on what to do if parsing fails but the
forwardlist
   // requires a valid *net.TCPAddr to operate, so we enforce that
restriction here.
@@ -489,6 +539,11 @@

    // Cryptographic-related configuration.
    Crypto CryptoConfig
+
+ // Connector to the local ssh-agent for key forwarding.
+ // If nil, no key forwarding will take place
+ // See `man ssh_config` on "ForwardAgent" for security implications
+ ForwardingAgentConnector AgentConnector
   }

   func (c *ClientConfig) rand() io.Reader {
Index: ssh/session.go
===================================================================
--- a/ssh/session.go
+++ b/ssh/session.go
@@ -225,6 +225,18 @@
    return s.waitForResponse()
   }

+func (s *Session) issueAgentRequest() error {
+ req := channelRequestMsg{
+ PeersId: s.remoteId,
+ Request: "auth-agent-req@openssh.com",
+ WantReply: false,
+ }
+ if err := s.writePacket(marshal(msgChannelRequest, req)); err != nil {
+ return err
+ }
+ return nil
+}
+
   // RFC 4254 Section 6.9.
   type signalMsg struct {
    PeersId uint32
@@ -348,7 +360,7 @@
     return err
    }
    if err := s.waitForResponse(); err != nil {
- return fmt.Errorf("ssh: cound not execute shell: %v", err)
+ return fmt.Errorf("ssh: could not execute shell: %v", err)
    }
    return s.start()
   }
@@ -577,9 +589,16 @@
     c.chanList.remove(ch.localId)
     return nil, fmt.Errorf("ssh: unable to open session: %v", err)
    }
- return &Session{
+ s := &Session{
     clientChan: ch,
- }, nil
+ }
+ if c.config.ForwardingAgentConnector != nil {
+ err := s.issueAgentRequest()
+ if err != nil {
+ return nil, fmt.Errorf("ssh: unable to request agent forwarding: %v",
err)
+ }
+ }
+ return s, nil
   }

   // An ExitError reports unsuccessful completion of a remote command.


--

---
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.

Search Discussions

  • Jamwt at Aug 22, 2013 at 8:45 pm

    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

    https://codereview.appspot.com/12837048/

    --

    ---
    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.
  • Jpsugar at Aug 23, 2013 at 5:24 pm
    I'm not privileged to approve things, but this looks good to me. :-) You
    just beat me to it.

    https://codereview.appspot.com/12837048/

    --

    ---
    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.
  • Jpsugar at Aug 23, 2013 at 6:46 pm
    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.

    https://codereview.appspot.com/12837048/

    --

    ---
    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.
  • Jpsugar at Aug 23, 2013 at 6:50 pm
    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.

    https://codereview.appspot.com/12837048/

    --

    ---
    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.
  • Jamwt at Aug 23, 2013 at 7:17 pm
    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?

    https://codereview.appspot.com/12837048/

    --

    ---
    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.
  • Jpsugar at Aug 23, 2013 at 7:53 pm
    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.

    https://codereview.appspot.com/12837048/

    --

    ---
    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.
  • Jpsugar at Aug 23, 2013 at 7:55 pm
    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.

    https://codereview.appspot.com/12837048/

    --

    ---
    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.
  • Jamwt at Aug 24, 2013 at 4:59 am

    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

    https://codereview.appspot.com/12837048/

    --

    ---
    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.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedAug 22, '13 at 8:45p
activeAug 24, '13 at 4:59a
posts9
users2
websitegolang.org

2 users in discussion

Jpsugar: 5 posts Jamwt: 4 posts

People

Translate

site design / logo © 2021 Grokbase