Merge lp:~gz/juju-core/gosshnew_1312940 into lp:~go-bot/juju-core/trunk

[Martin Packman]

utils/ssh: Update to use latest go.crypto

Upstream go.crypto has a cleaned up ssh package in r192
and beyond. This has a few interface changes we need to
adapt to, but should make testing easier in future.

Also bumps dependencies.tsv to go.crypto tip.

https://codereview.appspot.com/95770045/

R=axwalk

utils/ssh: Update to use latest go.crypto

Upstream go.crypto has a cleaned up ssh package in r192
and beyond. This has a few interface changes we need to
adapt to, but should make testing easier in future.

Also bumps dependencies.tsv to go.crypto tip.

https://codereview.appspot.com/95770045/

[Martin Packman (gz)]

Reviewers: mp+217308_code.launchpad.net,

Message:
Please take a look.

Description:
utils/ssh: Update to use latest go.crypto

Upstream go.crypto has a cleaned up ssh package in r192
and beyond. This has a few interface changes we need to
adapt to, but should make testing easier in future.

Also bumps dependencies.tsv to go.crypto tip.

https://code.launchpad.net/~gz/juju-core/gosshnew_1312940/+merge/217308

(do not edit description out of merge proposal)

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

Affected files (+62, -65 lines):
   A [revision details]
   M dependencies.tsv
   M utils/ssh/authorisedkeys.go
   M utils/ssh/ssh_gocrypto.go
   M utils/ssh/ssh_gocrypto_test.go

[Andrew Wilkins (axwalk)]

LGTM, with a few naming changes

Thanks for doing that, I keep on meaning to do it and then promptly
forget.

https://codereview.appspot.com/95770045/diff/1/utils/ssh/authorisedkeys.go
File utils/ssh/authorisedkeys.go (left):

https://codereview.appspot.com/95770045/diff/1/utils/ssh/authorisedkeys.go#oldcode48
utils/ssh/authorisedkeys.go:48: return nil, fmt.Errorf("invalid
authorized_key %q", line)
Include the ParseAuthorizedKey error in the message?

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto.go
File utils/ssh/ssh_gocrypto.go (right):

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto.go#newcode84
utils/ssh/ssh_gocrypto.go:84: conn *ssh.Client
s/conn/client/
(and wherever else we refer to Client as conn)
?

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto.go#newcode152
utils/ssh/ssh_gocrypto.go:152: conn.Conn.Close()
ssh.Client embeds Conn, so conn.Close (or client.Close) would be better
IMO

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto_test.go
File utils/ssh/ssh_gocrypto_test.go (right):

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto_test.go#newcode31
utils/ssh/ssh_gocrypto_test.go:31: Listener net.Listener
s/Listener/listener/
?
(I would prefer we don't export fields unless we need to for reflection
or whatever.)

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto_test.go#newcode154
utils/ssh/ssh_gocrypto_test.go:154: c.Assert(string(out), gc.Equals,
"abc value\n")
Yay! Thanks.

https://codereview.appspot.com/95770045/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/95770045/diff/1/utils/ssh/authorisedkeys.go
File utils/ssh/authorisedkeys.go (left):

https://codereview.appspot.com/95770045/diff/1/utils/ssh/authorisedkeys.go#oldcode48
utils/ssh/authorisedkeys.go:48: return nil, fmt.Errorf("invalid
authorized_key %q", line)
On 2014/04/26 02:09:29, axw wrote:
> Include the ParseAuthorizedKey error in the message?

Was going to errgo wrap it, but the error we get is always "ssh: no key
found" which would actually make our message *worse* if we included it.

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto.go
File utils/ssh/ssh_gocrypto.go (right):

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto.go#newcode84
utils/ssh/ssh_gocrypto.go:84: conn *ssh.Client
On 2014/04/26 02:09:29, axw wrote:
> s/conn/client/
> (and wherever else we refer to Client as conn)
> ?

Done.

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto.go#newcode152
utils/ssh/ssh_gocrypto.go:152: conn.Conn.Close()
On 2014/04/26 02:09:29, axw wrote:
> ssh.Client embeds Conn, so conn.Close (or client.Close) would be
better IMO

Done.

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto_test.go
File utils/ssh/ssh_gocrypto_test.go (right):

https://codereview.appspot.com/95770045/diff/1/utils/ssh/ssh_gocrypto_test.go#newcode31
utils/ssh/ssh_gocrypto_test.go:31: Listener net.Listener
On 2014/04/26 02:09:29, axw wrote:
> s/Listener/listener/
> ?
> (I would prefer we don't export fields unless we need to for
reflection or
> whatever.)

Okay. I guess really we don't need client in this struct, but I'll leave
that addition for now.

https://codereview.appspot.com/95770045/

[Go Bot (go-bot)]

The attempt to merge lp:~gz/juju-core/gosshnew_1312940 into lp:juju-core failed. Below is the output from the failed tests.

godeps: cannot update "/home/tarmac/trees/src/code.google.com/p/go.crypto": abort: unknown revision '5478be1963aafa9025e9bf0837aff6013eb92e5b'!
mongod: no process found

[Go Bot (go-bot)]

The attempt to merge lp:~gz/juju-core/gosshnew_1312940 into lp:juju-core failed. Below is the output from the failed tests.

# code.google.com/p/go.crypto/ssh
../../code.google.com/p/go.crypto/ssh/cipher.go:241: undefined: cipher.AEAD
../../code.google.com/p/go.crypto/ssh/cipher.go:253: undefined: cipher.NewGCM
mongod: no process found

[Martin Packman (gz)]

Blocked for now as golang 1.1.2 lacks a couple of cipher things that the newer go.crypto is using.

[William Reade (fwereade)]

Setting to WIP then.

Unmerged revisions

2672. By Martin Packman on 2014-04-26

Naming changes suggested by axw in review

2671. By Martin Packman on 2014-04-25

Update utils/ssh for gosshnew with latest go.crypto

=== modified file 'dependencies.tsv'
--- dependencies.tsv	2014-04-23 08:53:35 +0000
+++ dependencies.tsv	2014-04-26 17:03:23 +0000
@@ -1,4 +1,4 @@
-code.google.com/p/go.crypto	hg	6478cc9340cbbe6c04511280c5007722269108e9	184
+code.google.com/p/go.crypto	hg	5478be1963aafa9025e9bf0837aff6013eb92e5b	199
 code.google.com/p/go.net	hg	c17ad62118ea511e1051721b429779fa40bddc74	116
 github.com/errgo/errgo	git	93d72bf813883d1054cae1c001d3a46603f7f559	
 github.com/joyent/gocommon	git	98b151a080efe19bcde223d2d3b04389963d2347	

=== modified file 'utils/ssh/authorisedkeys.go'
--- utils/ssh/authorisedkeys.go	2014-04-02 11:35:49 +0000
+++ utils/ssh/authorisedkeys.go	2014-04-26 17:03:23 +0000
@@ -35,6 +35,7 @@
 )
 
 type AuthorisedKey struct {
+	Type    string
 	Key     []byte
 	Comment string
 }
@@ -43,13 +44,13 @@
 // authorized_keys file and returns the constituent parts.
 // Based on description in "man sshd".
 func ParseAuthorisedKey(line string) (*AuthorisedKey, error) {
-	key, comment, _, _, ok := ssh.ParseAuthorizedKey([]byte(line))
-	if !ok {
+	key, comment, _, _, err := ssh.ParseAuthorizedKey([]byte(line))
+	if err != nil {
 		return nil, fmt.Errorf("invalid authorized_key %q", line)
 	}
-	keyBytes := ssh.MarshalPublicKey(key)
 	return &AuthorisedKey{
-		Key:     keyBytes,
+		Type:    key.Type(),
+		Key:     key.Marshal(),
 		Comment: comment,
 	}, nil
 }

=== modified file 'utils/ssh/ssh_gocrypto.go'
--- utils/ssh/ssh_gocrypto.go	2014-04-14 19:19:59 +0000
+++ utils/ssh/ssh_gocrypto.go	2014-04-26 17:03:23 +0000
@@ -81,13 +81,13 @@
 	stdin        io.Reader
 	stdout       io.Writer
 	stderr       io.Writer
-	conn         *ssh.ClientConn
+	client       *ssh.Client
 	sess         *ssh.Session
 }
 
 var sshDial = ssh.Dial
 
-var sshDialWithProxy = func(addr string, proxyCommand []string, config *ssh.ClientConfig) (*ssh.ClientConn, error) {
+var sshDialWithProxy = func(addr string, proxyCommand []string, config *ssh.ClientConfig) (*ssh.Client, error) {
 	if len(proxyCommand) == 0 {
 		return sshDial("tcp", addr, config)
 	}
@@ -114,7 +114,11 @@
 	if err := cmd.Start(); err != nil {
 		return nil, err
 	}
-	return ssh.Client(client, config)
+	conn, chans, reqs, err := ssh.NewClientConn(client, addr, config)
+	if err != nil {
+		return nil, err
+	}
+	return ssh.NewClient(conn, chans, reqs), nil
 }
 
 func (c *goCryptoCommand) ensureSession() (*ssh.Session, error) {
@@ -133,20 +137,22 @@
 	}
 	config := &ssh.ClientConfig{
 		User: c.user,
-		Auth: []ssh.ClientAuth{
-			ssh.ClientAuthKeyring(keyring{c.signers}),
+		Auth: []ssh.AuthMethod{
+			ssh.PublicKeysCallback(func() ([]ssh.Signer, error) {
+				return c.signers, nil
+			}),
 		},
 	}
-	conn, err := sshDialWithProxy(c.addr, c.proxyCommand, config)
-	if err != nil {
-		return nil, err
-	}
-	sess, err := conn.NewSession()
-	if err != nil {
-		conn.Close()
-		return nil, err
-	}
-	c.conn = conn
+	client, err := sshDialWithProxy(c.addr, c.proxyCommand, config)
+	if err != nil {
+		return nil, err
+	}
+	sess, err := client.NewSession()
+	if err != nil {
+		client.Close()
+		return nil, err
+	}
+	c.client = client
 	c.sess = sess
 	c.sess.Stdin = c.stdin
 	c.sess.Stdout = c.stdout
@@ -170,12 +176,12 @@
 		return nil
 	}
 	err0 := c.sess.Close()
-	err1 := c.conn.Close()
+	err1 := c.client.Close()
 	if err0 == nil {
 		err0 = err1
 	}
 	c.sess = nil
-	c.conn = nil
+	c.client = nil
 	return err0
 }
 
@@ -228,26 +234,6 @@
 	return ioutil.NopCloser(wc), sess.Stderr, err
 }
 
-// keyring implements ssh.ClientKeyring
-type keyring struct {
-	signers []ssh.Signer
-}
-
-func (k keyring) Key(i int) (ssh.PublicKey, error) {
-	if i < 0 || i >= len(k.signers) {
-		// nil key marks the end of the keyring; must not return an error.
-		return nil, nil
-	}
-	return k.signers[i].PublicKey(), nil
-}
-
-func (k keyring) Sign(i int, rand io.Reader, data []byte) ([]byte, error) {
-	if i < 0 || i >= len(k.signers) {
-		return nil, fmt.Errorf("no key at position %d", i)
-	}
-	return k.signers[i].Sign(rand, data)
-}
-
 func splitUserHost(s string) (user, host string) {
 	userHost := strings.SplitN(s, "@", 2)
 	if len(userHost) == 2 {

=== modified file 'utils/ssh/ssh_gocrypto_test.go'
--- utils/ssh/ssh_gocrypto_test.go	2014-04-25 23:47:12 +0000
+++ utils/ssh/ssh_gocrypto_test.go	2014-04-26 17:03:23 +0000
@@ -27,8 +27,9 @@
 )
 
 type sshServer struct {
-	cfg *cryptossh.ServerConfig
-	*cryptossh.Listener
+	cfg      *cryptossh.ServerConfig
+	listener net.Listener
+	client   *cryptossh.Client
 }
 
 func newServer(c *gc.C) *sshServer {
@@ -40,40 +41,49 @@
 		cfg: &cryptossh.ServerConfig{},
 	}
 	server.cfg.AddHostKey(key)
-	server.Listener, err = cryptossh.Listen("tcp", "127.0.0.1:0", server.cfg)
+	server.listener, err = net.Listen("tcp", "127.0.0.1:0")
 	c.Assert(err, gc.IsNil)
 	return server
 }
 
 func (s *sshServer) run(c *gc.C) {
-	conn, err := s.Accept()
+	netconn, err := s.listener.Accept()
 	c.Assert(err, gc.IsNil)
 	defer func() {
-		err = conn.Close()
+		err = netconn.Close()
 		c.Assert(err, gc.IsNil)
 	}()
-	err = conn.Handshake()
+	conn, chans, reqs, err := cryptossh.NewServerConn(netconn, s.cfg)
 	c.Assert(err, gc.IsNil)
+	s.client = cryptossh.NewClient(conn, chans, reqs)
 	var wg sync.WaitGroup
 	defer wg.Wait()
-	for {
-		channel, err := conn.Accept()
+	sessionChannels := s.client.HandleChannelOpen("session")
+	c.Assert(sessionChannels, gc.NotNil)
+	for newChannel := range sessionChannels {
+		c.Assert(newChannel.ChannelType(), gc.Equals, "session")
+		channel, reqs, err := newChannel.Accept()
 		c.Assert(err, gc.IsNil)
-		c.Assert(channel.ChannelType(), gc.Equals, "session")
-		channel.Accept()
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
 			defer channel.Close()
-			_, err := channel.Read(nil)
-			c.Assert(err, gc.FitsTypeOf, cryptossh.ChannelRequest{})
-			req := err.(cryptossh.ChannelRequest)
-			c.Assert(req.Request, gc.Equals, "exec")
-			c.Assert(req.WantReply, jc.IsTrue)
-			n := binary.BigEndian.Uint32(req.Payload[:4])
-			command := string(req.Payload[4 : n+4])
-			c.Assert(command, gc.Equals, testCommandFlat)
-			// TODO(axw) when gosshnew is ready, send reply to client.
+			for req := range reqs {
+				switch req.Type {
+				case "exec":
+					c.Assert(req.WantReply, jc.IsTrue)
+					n := binary.BigEndian.Uint32(req.Payload[:4])
+					command := string(req.Payload[4 : n+4])
+					c.Assert(command, gc.Equals, testCommandFlat)
+					req.Reply(true, nil)
+					channel.Write([]byte("abc value\n"))
+					_, err := channel.SendRequest("exit-status", false, cryptossh.Marshal(&struct{ n uint32 }{0}))
+					c.Assert(err, gc.IsNil)
+					return
+				default:
+					c.Fatalf("Unexpected request type: %v", req.Type)
+				}
+			}
 		}()
 	}
 }
@@ -115,7 +125,7 @@
 	err = ssh.LoadClientKeys(c.MkDir())
 	c.Assert(err, gc.IsNil)
 
-	s.PatchValue(ssh.SSHDial, func(network, address string, cfg *cryptossh.ClientConfig) (*cryptossh.ClientConn, error) {
+	s.PatchValue(ssh.SSHDial, func(network, address string, cfg *cryptossh.ClientConfig) (*cryptossh.Client, error) {
 		return nil, errors.New("ssh.Dial failed")
 	})
 	cmd = client.Command("0.1.2.3", []string{"echo", "123"}, nil)
@@ -132,19 +142,18 @@
 	c.Assert(err, gc.IsNil)
 	server := newServer(c)
 	var opts ssh.Options
-	opts.SetPort(server.Addr().(*net.TCPAddr).Port)
+	opts.SetPort(server.listener.Addr().(*net.TCPAddr).Port)
 	cmd := client.Command("127.0.0.1", testCommand, &opts)
 	checkedKey := false
-	server.cfg.PublicKeyCallback = func(conn *cryptossh.ServerConn, user, algo string, pubkey []byte) bool {
-		c.Check(pubkey, gc.DeepEquals, cryptossh.MarshalPublicKey(key.PublicKey()))
+	server.cfg.PublicKeyCallback = func(conn cryptossh.ConnMetadata, pubkey cryptossh.PublicKey) (*cryptossh.Permissions, error) {
+		c.Check(pubkey, gc.DeepEquals, key.PublicKey())
 		checkedKey = true
-		return true
+		return nil, nil
 	}
 	go server.run(c)
 	out, err := cmd.Output()
-	c.Assert(err, gc.ErrorMatches, "ssh: could not execute command.*")
-	// TODO(axw) when gosshnew is ready, expect reply from server.
-	c.Assert(out, gc.IsNil)
+	c.Assert(err, gc.IsNil)
+	c.Assert(string(out), gc.Equals, "abc value\n")
 	c.Assert(checkedKey, jc.IsTrue)
 }
 
@@ -172,18 +181,17 @@
 	c.Assert(err, gc.IsNil)
 	server := newServer(c)
 	var opts ssh.Options
-	port := server.Addr().(*net.TCPAddr).Port
+	port := server.listener.Addr().(*net.TCPAddr).Port
 	opts.SetProxyCommand(netcat, "-q0", "%h", "%p")
 	opts.SetPort(port)
 	cmd := client.Command("127.0.0.1", testCommand, &opts)
-	server.cfg.PublicKeyCallback = func(conn *cryptossh.ServerConn, user, algo string, pubkey []byte) bool {
-		return true
+	server.cfg.PublicKeyCallback = func(_ cryptossh.ConnMetadata, pubkey cryptossh.PublicKey) (*cryptossh.Permissions, error) {
+		return nil, nil
 	}
 	go server.run(c)
 	out, err := cmd.Output()
-	c.Assert(err, gc.ErrorMatches, "ssh: could not execute command.*")
-	// TODO(axw) when gosshnew is ready, expect reply from server.
-	c.Assert(out, gc.IsNil)
+	c.Assert(err, gc.IsNil)
+	c.Assert(string(out), gc.Equals, "abc value\n")
 	// Ensure the proxy command was executed with the appropriate arguments.
 	data, err := ioutil.ReadFile(netcat + ".args")
 	c.Assert(err, gc.IsNil)