Merge lp:~axwalk/juju-core/ssh-gocrypto-client-take2 into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

utils/ssh: introduce go.crypto/ssh client

Bootstrap now requires an ssh client. Users
may not have an SSH client available or
configured; this is typical of many Windows
users.

We introduce a go.crypto/ssh-based embedded
client that will be used if OpenSSH is not
available. The embedded client will use any
keys found in $JUJU_HOME/ssh.

Fixes #1263851

https://codereview.appspot.com/52570043/

utils/ssh: introduce go.crypto/ssh client

Bootstrap now requires an ssh client. Users
may not have an SSH client available or
configured; this is typical of many Windows
users.

We introduce a go.crypto/ssh-based embedded
client that will be used if OpenSSH is not
available. The embedded client will use any
keys found in $JUJU_HOME/ssh.

Fixes #1263851

https://codereview.appspot.com/52570043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+201706_code.launchpad.net,

Message:
Please take a look.

Description:
utils/ssh: introduce go.crypto/ssh client

Bootstrap now requires an ssh client. Users
may not have an SSH client available or
configured; this is typical of many Windows
users.

We introduce a go.crypto/ssh-based embedded
client that will be used if OpenSSH is not
available. The embedded client will use any
keys found in $JUJU_HOME/ssh.

Fixes #1263851

https://code.launchpad.net/~axwalk/juju-core/ssh-gocrypto-client-take2/+merge/201706

(do not edit description out of merge proposal)

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

Affected files (+270, -6 lines):
   A [revision details]
   M utils/ssh/clientkeys_test.go
   M utils/ssh/ssh.go
   A utils/ssh/ssh_gocrypto.go
   M utils/trivial.go
   M utils/trivial_test.go

[Ian Booth (wallyworld)]

A few questions. I can't see any tests for fallback to the Go ssh client
if OpenSSH not available, nor for the Go ssh client wrapper itself. Does
it matter that copy is not implemented? I guess it's not used during
bootstrap so it doesn't need to be.

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

https://codereview.appspot.com/52570043/diff/1/utils/ssh/ssh.go#newcode183
utils/ssh/ssh.go:183: // There is currently only one client available:
OpenSSH.
This comment is now out of date

https://codereview.appspot.com/52570043/diff/1/utils/trivial_test.go
File utils/trivial_test.go (right):

https://codereview.appspot.com/52570043/diff/1/utils/trivial_test.go#newcode63
utils/trivial_test.go:63: {[]string{"a\n"}, "\"a\n\""},
There are a some test cases missing eg $

https://codereview.appspot.com/52570043/

[Andrew Wilkins (axwalk)]

I've added some tests for the go.crypto client, but they're not
complete. The problem is that the go.crypto *server* support is not all
there; it has been expanded in a development branch, but it's not quite
ready for consumption yet. I've left a TODO to expand the tests when it
is.

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

https://codereview.appspot.com/52570043/diff/1/utils/ssh/ssh.go#newcode183
utils/ssh/ssh.go:183: // There is currently only one client available:
OpenSSH.
On 2014/01/15 23:52:19, wallyworld wrote:
> This comment is now out of date

Done.

https://codereview.appspot.com/52570043/diff/1/utils/trivial_test.go
File utils/trivial_test.go (right):

https://codereview.appspot.com/52570043/diff/1/utils/trivial_test.go#newcode63
utils/trivial_test.go:63: {[]string{"a\n"}, "\"a\n\""},
On 2014/01/15 23:52:19, wallyworld wrote:
> There are a some test cases missing eg $

Done.

https://codereview.appspot.com/52570043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/52570043/

[Tim Penhey (thumper)]

LGTM with one small adjustment :-)

https://codereview.appspot.com/52570043/diff/20001/utils/ssh/ssh_openssh.go
File utils/ssh/ssh_openssh.go (right):

https://codereview.appspot.com/52570043/diff/20001/utils/ssh/ssh_openssh.go#newcode74
utils/ssh/ssh_openssh.go:74: args := opensshOptions(options, false)
Man I hate magic bools. Can we have some constants or types or
something: asSCP, asSSH ?

https://codereview.appspot.com/52570043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/52570043/diff/20001/utils/ssh/ssh_openssh.go
File utils/ssh/ssh_openssh.go (right):

https://codereview.appspot.com/52570043/diff/20001/utils/ssh/ssh_openssh.go#newcode74
utils/ssh/ssh_openssh.go:74: args := opensshOptions(options, false)
On 2014/01/17 03:15:23, thumper wrote:
> Man I hate magic bools. Can we have some constants or types or
something: asSCP,
> asSSH ?

Done.

https://codereview.appspot.com/52570043/

[Andrew Wilkins (axwalk)]

On 2014/01/17 03:37:42, axw wrote:
> Please take a look.

https://codereview.appspot.com/52570043/diff/20001/utils/ssh/ssh_openssh.go
> File utils/ssh/ssh_openssh.go (right):

https://codereview.appspot.com/52570043/diff/20001/utils/ssh/ssh_openssh.go#newcode74
> utils/ssh/ssh_openssh.go:74: args := opensshOptions(options, false)
> On 2014/01/17 03:15:23, thumper wrote:
> > Man I hate magic bools. Can we have some constants or types or
something:
> asSCP,
> > asSSH ?

> Done.

Just going to do another live test and then I'll land it.

https://codereview.appspot.com/52570043/

[Andrew Wilkins (axwalk)]

Confirmed that it works on both Linux (disabling OpenSSH in code) and Windows (no code changes; OpenSSH not installed).

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/ssh-gocrypto-client-take2 into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.851s
ok launchpad.net/juju-core/agent/tools 0.304s
ok launchpad.net/juju-core/bzr 7.500s
ok launchpad.net/juju-core/cert 3.225s
ok launchpad.net/juju-core/charm 0.659s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.033s
ok launchpad.net/juju-core/cloudinit/sshinit 1.009s
ok launchpad.net/juju-core/cmd 0.221s
ok launchpad.net/juju-core/cmd/charm-admin 0.869s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 225.609s
ok launchpad.net/juju-core/cmd/jujud 57.725s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 10.019s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.030s
ok launchpad.net/juju-core/container 0.038s
ok launchpad.net/juju-core/container/factory 0.069s
ok launchpad.net/juju-core/container/kvm 0.317s
ok launchpad.net/juju-core/container/kvm/mock 0.043s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.376s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.285s
ok launchpad.net/juju-core/environs 3.125s
ok launchpad.net/juju-core/environs/bootstrap 4.559s
ok launchpad.net/juju-core/environs/cloudinit 0.561s
ok launchpad.net/juju-core/environs/config 4.943s
ok launchpad.net/juju-core/environs/configstore 0.044s
ok launchpad.net/juju-core/environs/filestorage 0.033s
ok launchpad.net/juju-core/environs/httpstorage 0.990s
ok launchpad.net/juju-core/environs/imagemetadata 0.483s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.058s
ok launchpad.net/juju-core/environs/jujutest 0.267s
ok launchpad.net/juju-core/environs/manual 11.024s
ok launchpad.net/juju-core/environs/simplestreams 0.362s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.490s
ok launchpad.net/juju-core/environs/storage 1.228s
ok launchpad.net/juju-core/environs/sync 33.102s
ok launchpad.net/juju-core/environs/testing 0.206s
ok launchpad.net/juju-core/environs/tools 6.839s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.018s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 23.221s
ok launchpad.net/juju-core/juju/osenv 0.018s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.025s
ok launchpad.net/juju-core/names 0.054s
? launchpad.net/j...

=== modified file 'utils/ssh/clientkeys_test.go'
--- utils/ssh/clientkeys_test.go	2014-01-14 03:35:21 +0000
+++ utils/ssh/clientkeys_test.go	2014-01-17 04:36:38 +0000
@@ -9,7 +9,6 @@
 
 	gc "launchpad.net/gocheck"
 
-	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
@@ -58,14 +57,14 @@
 	// All files ending with .pub in the client key dir get picked up.
 	priv, pub, err := ssh.GenerateKey("whatever")
 	c.Assert(err, gc.IsNil)
-	err = ioutil.WriteFile(config.JujuHomePath("ssh", "whatever.pub"), []byte(pub), 0600)
+	err = ioutil.WriteFile(testing.HomePath(".juju", "ssh", "whatever.pub"), []byte(pub), 0600)
 	c.Assert(err, gc.IsNil)
 	err = ssh.LoadClientKeys("~/.juju/ssh")
 	c.Assert(err, gc.IsNil)
 	// The new public key won't be observed until the
 	// corresponding private key exists.
 	checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_rsa.pub")
-	err = ioutil.WriteFile(config.JujuHomePath("ssh", "whatever"), []byte(priv), 0600)
+	err = ioutil.WriteFile(testing.HomePath(".juju", "ssh", "whatever"), []byte(priv), 0600)
 	c.Assert(err, gc.IsNil)
 	err = ssh.LoadClientKeys("~/.juju/ssh")
 	c.Assert(err, gc.IsNil)
@@ -81,14 +80,14 @@
 	checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_rsa")
 	priv, pub, err := ssh.GenerateKey("whatever")
 	c.Assert(err, gc.IsNil)
-	err = ioutil.WriteFile(config.JujuHomePath("ssh", "whatever"), []byte(priv), 0600)
+	err = ioutil.WriteFile(testing.HomePath(".juju", "ssh", "whatever"), []byte(priv), 0600)
 	c.Assert(err, gc.IsNil)
 	err = ssh.LoadClientKeys("~/.juju/ssh")
 	c.Assert(err, gc.IsNil)
 	// The new private key won't be observed until the
 	// corresponding public key exists.
 	checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_rsa")
-	err = ioutil.WriteFile(config.JujuHomePath("ssh", "whatever.pub"), []byte(pub), 0600)
+	err = ioutil.WriteFile(testing.HomePath(".juju", "ssh", "whatever.pub"), []byte(pub), 0600)
 	c.Assert(err, gc.IsNil)
 	// new keys won't be reported until we call LoadClientKeys again
 	checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_rsa.pub")
@@ -100,7 +99,7 @@
 }
 
 func (s *ClientKeysSuite) TestLoadClientKeysDirExists(c *gc.C) {
-	err := os.MkdirAll(config.JujuHomePath("ssh"), 0755)
+	err := os.MkdirAll(testing.HomePath(".juju", "ssh"), 0755)
 	c.Assert(err, gc.IsNil)
 	err = ssh.LoadClientKeys("~/.juju/ssh")
 	c.Assert(err, gc.IsNil)

=== modified file 'utils/ssh/export_test.go'
--- utils/ssh/export_test.go	2013-12-12 13:44:42 +0000
+++ utils/ssh/export_test.go	2014-01-17 04:36:38 +0000
@@ -6,4 +6,5 @@
 var (
 	ReadAuthorisedKeys  = readAuthorisedKeys
 	WriteAuthorisedKeys = writeAuthorisedKeys
+	InitDefaultClient   = initDefaultClient
 )

=== modified file 'utils/ssh/ssh.go'
--- utils/ssh/ssh.go	2014-01-13 06:25:28 +0000
+++ utils/ssh/ssh.go	2014-01-17 04:36:38 +0000
@@ -16,6 +16,8 @@
 
 // Options is a client-implementation independent SSH options set.
 type Options struct {
+	// ssh server port; zero means use the default (22)
+	port int
 	// no PTY forced by default
 	allocatePTY bool
 	// password authentication is disallowed by default
@@ -26,6 +28,11 @@
 	identities []string
 }
 
+// SetPort sets the SSH server port to connect to.
+func (o *Options) SetPort(port int) {
+	o.port = port
+}
+
 // EnablePTY forces the allocation of a pseudo-TTY.
 //
 // Forcing a pseudo-TTY is required, for example, for sudo
@@ -180,14 +187,20 @@
 
 // DefaultClient is the default SSH client for the process.
 //
-// There is currently only one client available: OpenSSH.
-// If the OpenSSH client is not installed, then DefaultClient
-// will be nil.
+// If the OpenSSH client is found in $PATH, then it will be
+// used for DefaultClient; otherwise, DefaultClient will use
+// an embedded client based on go.crypto/ssh.
 var DefaultClient Client
 
 func init() {
+	initDefaultClient()
+}
+
+func initDefaultClient() {
 	if client, err := NewOpenSSHClient(); err == nil {
 		DefaultClient = client
+	} else if client, err := NewGoCryptoClient(); err == nil {
+		DefaultClient = client
 	}
 }
 

=== added file 'utils/ssh/ssh_gocrypto.go'
--- utils/ssh/ssh_gocrypto.go	1970-01-01 00:00:00 +0000
+++ utils/ssh/ssh_gocrypto.go	2014-01-17 04:36:38 +0000
@@ -0,0 +1,217 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package ssh
+
+import (
+	"fmt"
+	"io"
+	"io/ioutil"
+	"os/user"
+	"strings"
+
+	"code.google.com/p/go.crypto/ssh"
+
+	"launchpad.net/juju-core/utils"
+)
+
+const sshDefaultPort = 22
+
+// GoCryptoClient is an implementation of Client that
+// uses the embedded go.crypto/ssh SSH client.
+//
+// GoCryptoClient is intentionally limited in the
+// functionality that it enables, as it is currently
+// intended to be used only for non-interactive command
+// execution.
+type GoCryptoClient struct {
+	signers []ssh.Signer
+}
+
+// NewGoCryptoClient creates a new GoCryptoClient.
+//
+// If no signers are specified, NewGoCryptoClient will
+// use the private key generated by LoadClientKeys.
+func NewGoCryptoClient(signers ...ssh.Signer) (*GoCryptoClient, error) {
+	return &GoCryptoClient{signers: signers}, nil
+}
+
+// Command implements Client.Command.
+func (c *GoCryptoClient) Command(host string, command []string, options *Options) *Cmd {
+	shellCommand := utils.CommandString(command...)
+	signers := c.signers
+	if len(signers) == 0 {
+		signers = privateKeys()
+	}
+	user, host := splitUserHost(host)
+	port := sshDefaultPort
+	if options != nil {
+		if options.port != 0 {
+			port = options.port
+		}
+	}
+	return &Cmd{impl: &goCryptoCommand{
+		signers: signers,
+		user:    user,
+		addr:    fmt.Sprintf("%s:%d", host, port),
+		command: shellCommand,
+	}}
+}
+
+// Copy implements Client.Copy.
+//
+// Copy is currently unimplemented, and will always return an error.
+func (c *GoCryptoClient) Copy(source, dest string, options *Options) error {
+	return fmt.Errorf("Copy is not implemented")
+}
+
+type goCryptoCommand struct {
+	signers []ssh.Signer
+	user    string
+	addr    string
+	command string
+	stdin   io.Reader
+	stdout  io.Writer
+	stderr  io.Writer
+	conn    *ssh.ClientConn
+	sess    *ssh.Session
+}
+
+func (c *goCryptoCommand) ensureSession() (*ssh.Session, error) {
+	if c.sess != nil {
+		return c.sess, nil
+	}
+	if len(c.signers) == 0 {
+		return nil, fmt.Errorf("no private keys available")
+	}
+	if c.user == "" {
+		currentUser, err := user.Current()
+		if err != nil {
+			return nil, fmt.Errorf("getting current user: %v", err)
+		}
+		c.user = currentUser.Username
+	}
+	config := &ssh.ClientConfig{
+		User: c.user,
+		Auth: []ssh.ClientAuth{
+			ssh.ClientAuthKeyring(keyring{c.signers}),
+		},
+	}
+	conn, err := ssh.Dial("tcp", c.addr, config)
+	if err != nil {
+		return nil, err
+	}
+	sess, err := conn.NewSession()
+	if err != nil {
+		conn.Close()
+		return nil, err
+	}
+	c.conn = conn
+	c.sess = sess
+	c.sess.Stdin = c.stdin
+	c.sess.Stdout = c.stdout
+	c.sess.Stderr = c.stderr
+	return sess, nil
+}
+
+func (c *goCryptoCommand) Start() error {
+	sess, err := c.ensureSession()
+	if err != nil {
+		return err
+	}
+	if c.command == "" {
+		return sess.Shell()
+	}
+	return sess.Start(c.command)
+}
+
+func (c *goCryptoCommand) Close() error {
+	if c.sess == nil {
+		return nil
+	}
+	err0 := c.sess.Close()
+	err1 := c.conn.Close()
+	if err0 == nil {
+		err0 = err1
+	}
+	c.sess = nil
+	c.conn = nil
+	return err0
+}
+
+func (c *goCryptoCommand) Wait() error {
+	if c.sess == nil {
+		return fmt.Errorf("Command has not been started")
+	}
+	err := c.sess.Wait()
+	c.Close()
+	return err
+}
+
+func (c *goCryptoCommand) Kill() error {
+	if c.sess == nil {
+		return fmt.Errorf("Command has not been started")
+	}
+	return c.sess.Signal(ssh.SIGKILL)
+}
+
+func (c *goCryptoCommand) SetStdio(stdin io.Reader, stdout, stderr io.Writer) {
+	c.stdin = stdin
+	c.stdout = stdout
+	c.stderr = stderr
+}
+
+func (c *goCryptoCommand) StdinPipe() (io.WriteCloser, io.Reader, error) {
+	sess, err := c.ensureSession()
+	if err != nil {
+		return nil, nil, err
+	}
+	wc, err := sess.StdinPipe()
+	return wc, sess.Stdin, err
+}
+
+func (c *goCryptoCommand) StdoutPipe() (io.ReadCloser, io.Writer, error) {
+	sess, err := c.ensureSession()
+	if err != nil {
+		return nil, nil, err
+	}
+	wc, err := sess.StdoutPipe()
+	return ioutil.NopCloser(wc), sess.Stdout, err
+}
+
+func (c *goCryptoCommand) StderrPipe() (io.ReadCloser, io.Writer, error) {
+	sess, err := c.ensureSession()
+	if err != nil {
+		return nil, nil, err
+	}
+	wc, err := sess.StderrPipe()
+	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 {
+		return userHost[0], userHost[1]
+	}
+	return "", userHost[0]
+}

=== added file 'utils/ssh/ssh_gocrypto_test.go'
--- utils/ssh/ssh_gocrypto_test.go	1970-01-01 00:00:00 +0000
+++ utils/ssh/ssh_gocrypto_test.go	2014-01-17 04:36:38 +0000
@@ -0,0 +1,145 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package ssh_test
+
+import (
+	"encoding/binary"
+	"net"
+	"sync"
+
+	cryptossh "code.google.com/p/go.crypto/ssh"
+	gc "launchpad.net/gocheck"
+
+	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/utils/ssh"
+)
+
+var (
+	testCommand     = []string{"echo", "$abc"}
+	testCommandFlat = `echo "\$abc"`
+)
+
+type sshServer struct {
+	cfg *cryptossh.ServerConfig
+	*cryptossh.Listener
+}
+
+func newServer(c *gc.C) *sshServer {
+	private, _, err := ssh.GenerateKey("test-server")
+	c.Assert(err, gc.IsNil)
+	key, err := cryptossh.ParsePrivateKey([]byte(private))
+	c.Assert(err, gc.IsNil)
+	server := &sshServer{
+		cfg: &cryptossh.ServerConfig{},
+	}
+	server.cfg.AddHostKey(key)
+	server.Listener, err = cryptossh.Listen("tcp", "127.0.0.1:0", server.cfg)
+	c.Assert(err, gc.IsNil)
+	return server
+}
+
+func (s *sshServer) run(c *gc.C) {
+	conn, err := s.Accept()
+	c.Assert(err, gc.IsNil)
+	defer func() {
+		err = conn.Close()
+		c.Assert(err, gc.IsNil)
+	}()
+	err = conn.Handshake()
+	c.Assert(err, gc.IsNil)
+	var wg sync.WaitGroup
+	defer wg.Wait()
+	for {
+		channel, err := conn.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.
+		}()
+	}
+}
+
+type SSHGoCryptoCommandSuite struct {
+	testbase.LoggingSuite
+	client ssh.Client
+}
+
+var _ = gc.Suite(&SSHGoCryptoCommandSuite{})
+
+func (s *SSHGoCryptoCommandSuite) SetUpTest(c *gc.C) {
+	s.LoggingSuite.SetUpTest(c)
+	client, err := ssh.NewGoCryptoClient()
+	c.Assert(err, gc.IsNil)
+	s.client = client
+}
+
+func (s *SSHGoCryptoCommandSuite) TestNewGoCryptoClient(c *gc.C) {
+	_, err := ssh.NewGoCryptoClient()
+	c.Assert(err, gc.IsNil)
+	private, _, err := ssh.GenerateKey("test-client")
+	c.Assert(err, gc.IsNil)
+	key, err := cryptossh.ParsePrivateKey([]byte(private))
+	c.Assert(err, gc.IsNil)
+	_, err = ssh.NewGoCryptoClient(key)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *SSHGoCryptoCommandSuite) TestClientNoKeys(c *gc.C) {
+	client, err := ssh.NewGoCryptoClient()
+	c.Assert(err, gc.IsNil)
+	cmd := client.Command("test.invalid", []string{"echo", "123"}, nil)
+	_, err = cmd.Output()
+	c.Assert(err, gc.ErrorMatches, "no private keys available")
+	defer ssh.ClearClientKeys()
+	err = ssh.LoadClientKeys(c.MkDir())
+	c.Assert(err, gc.IsNil)
+	cmd = client.Command("test.invalid", []string{"echo", "123"}, nil)
+	_, err = cmd.Output()
+	// error message differs based on whether using cgo or not
+	c.Assert(err, gc.ErrorMatches, "(dial tcp: )?lookup test.invalid: no such host")
+}
+
+func (s *SSHGoCryptoCommandSuite) TestCommand(c *gc.C) {
+	private, _, err := ssh.GenerateKey("test-server")
+	c.Assert(err, gc.IsNil)
+	key, err := cryptossh.ParsePrivateKey([]byte(private))
+	client, err := ssh.NewGoCryptoClient(key)
+	c.Assert(err, gc.IsNil)
+	server := newServer(c)
+	var opts ssh.Options
+	opts.SetPort(server.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()))
+		checkedKey = true
+		return true
+	}
+	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(checkedKey, jc.IsTrue)
+}
+
+func (s *SSHGoCryptoCommandSuite) TestCopy(c *gc.C) {
+	client, err := ssh.NewGoCryptoClient()
+	c.Assert(err, gc.IsNil)
+	err = client.Copy("test.invalid:b", c.MkDir(), nil)
+	c.Assert(err, gc.ErrorMatches, "Copy is not implemented")
+}

=== modified file 'utils/ssh/ssh_openssh.go'
--- utils/ssh/ssh_openssh.go	2014-01-13 06:25:28 +0000
+++ utils/ssh/ssh_openssh.go	2014-01-17 04:36:38 +0000
@@ -14,6 +14,13 @@
 
 var opensshCommonOptions = []string{"-o", "StrictHostKeyChecking no"}
 
+type opensshCommandKind int
+
+const (
+	sshKind opensshCommandKind = iota
+	scpKind
+)
+
 // sshpassWrap wraps the command/args with sshpass if it is found in $PATH
 // and the SSHPASS environment variable is set. Otherwise, the original
 // command/args are returned.
@@ -44,7 +51,7 @@
 	return &c, nil
 }
 
-func opensshOptions(options *Options) []string {
+func opensshOptions(options *Options, commandKind opensshCommandKind) []string {
 	args := append([]string{}, opensshCommonOptions...)
 	if options == nil {
 		options = &Options{}
@@ -58,12 +65,21 @@
 	for _, identity := range options.identities {
 		args = append(args, "-i", identity)
 	}
+	if options.port != 0 {
+		if commandKind == scpKind {
+			// scp uses -P instead of -p (-p means preserve).
+			args = append(args, "-P")
+		} else {
+			args = append(args, "-p")
+		}
+		args = append(args, fmt.Sprint(options.port))
+	}
 	return args
 }
 
 // Command implements Client.Command.
 func (c *OpenSSHClient) Command(host string, command []string, options *Options) *Cmd {
-	args := opensshOptions(options)
+	args := opensshOptions(options, sshKind)
 	args = append(args, host)
 	if len(command) > 0 {
 		args = append(args, "--")
@@ -80,7 +96,7 @@
 		options = *userOptions
 		options.allocatePTY = false // doesn't make sense for scp
 	}
-	args := opensshOptions(&options)
+	args := opensshOptions(&options, scpKind)
 	args = append(args, source, dest)
 	bin, args := sshpassWrap("scp", args)
 	cmd := exec.Command(bin, args...)

=== modified file 'utils/ssh/ssh_test.go'
--- utils/ssh/ssh_test.go	2014-01-13 06:25:28 +0000
+++ utils/ssh/ssh_test.go	2014-01-17 04:36:38 +0000
@@ -55,6 +55,14 @@
 	c.Assert(strings.TrimSpace(string(out)), gc.Equals, expected)
 }
 
+func (s *SSHCommandSuite) TestDefaultClient(c *gc.C) {
+	ssh.InitDefaultClient()
+	c.Assert(ssh.DefaultClient, gc.FitsTypeOf, &ssh.OpenSSHClient{})
+	s.PatchEnvironment("PATH", "")
+	ssh.InitDefaultClient()
+	c.Assert(ssh.DefaultClient, gc.FitsTypeOf, &ssh.GoCryptoClient{})
+}
+
 func (s *SSHCommandSuite) TestCommandSSHPass(c *gc.C) {
 	// First create a fake sshpass, but don't set $SSHPASS
 	fakesshpass := filepath.Join(s.testbin, "sshpass")
@@ -105,15 +113,24 @@
 	)
 }
 
+func (s *SSHCommandSuite) TestCommandPort(c *gc.C) {
+	var opts ssh.Options
+	opts.SetPort(2022)
+	s.assertCommandArgs(c, s.commandOptions([]string{"echo", "123"}, &opts),
+		s.fakessh+" -o StrictHostKeyChecking no -o PasswordAuthentication no -p 2022 localhost -- echo 123",
+	)
+}
+
 func (s *SSHCommandSuite) TestCopy(c *gc.C) {
 	var opts ssh.Options
 	opts.EnablePTY()
 	opts.AllowPasswordAuthentication()
 	opts.SetIdentities("x", "y")
+	opts.SetPort(2022)
 	err := s.client.Copy("/tmp/blah", "[e-mail]:baz", &opts)
 	c.Assert(err, gc.IsNil)
 	out, err := ioutil.ReadFile(s.fakescp + ".args")
 	c.Assert(err, gc.IsNil)
 	// EnablePTY has no effect for Copy
-	c.Assert(string(out), gc.Equals, s.fakescp+" -o StrictHostKeyChecking no -i x -i y /tmp/blah [e-mail]:baz\n")
+	c.Assert(string(out), gc.Equals, s.fakescp+" -o StrictHostKeyChecking no -i x -i y -P 2022 /tmp/blah [e-mail]:baz\n")
 }

=== modified file 'utils/trivial.go'
--- utils/trivial.go	2013-10-10 20:58:54 +0000
+++ utils/trivial.go	2014-01-17 04:36:38 +0000
@@ -11,6 +11,7 @@
 	"io/ioutil"
 	"os"
 	"strings"
+	"unicode"
 
 	"launchpad.net/goyaml"
 )
@@ -61,6 +62,38 @@
 	return `'` + strings.Replace(s, `'`, `'"'"'`, -1) + `'`
 }
 
+// CommandString flattens a sequence of command arguments into a
+// string suitable for executing in a shell, escaping slashes,
+// variables and quotes as necessary; each argument is double-quoted
+// if and only if necessary.
+func CommandString(args ...string) string {
+	var buf bytes.Buffer
+	for i, arg := range args {
+		needsQuotes := false
+		var argBuf bytes.Buffer
+		for _, r := range arg {
+			if unicode.IsSpace(r) {
+				needsQuotes = true
+			} else if r == '"' || r == '$' || r == '\\' {
+				needsQuotes = true
+				argBuf.WriteByte('\\')
+			}
+			argBuf.WriteRune(r)
+		}
+		if i > 0 {
+			buf.WriteByte(' ')
+		}
+		if needsQuotes {
+			buf.WriteByte('"')
+			argBuf.WriteTo(&buf)
+			buf.WriteByte('"')
+		} else {
+			argBuf.WriteTo(&buf)
+		}
+	}
+	return buf.String()
+}
+
 // Gzip compresses the given data.
 func Gzip(data []byte) []byte {
 	var buf bytes.Buffer

=== modified file 'utils/trivial_test.go'
--- utils/trivial_test.go	2013-11-05 05:40:47 +0000
+++ utils/trivial_test.go	2014-01-17 04:36:38 +0000
@@ -47,3 +47,27 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(data1, gc.DeepEquals, data)
 }
+
+func (utilsSuite) TestCommandString(c *gc.C) {
+	type test struct {
+		args     []string
+		expected string
+	}
+	tests := []test{
+		{nil, ""},
+		{[]string{"a"}, "a"},
+		{[]string{"a$"}, `"a\$"`},
+		{[]string{""}, ""},
+		{[]string{"\\"}, `"\\"`},
+		{[]string{"a", "'b'"}, "a 'b'"},
+		{[]string{"a b"}, `"a b"`},
+		{[]string{"a", `"b"`}, `a "\"b\""`},
+		{[]string{"a", `"b\"`}, `a "\"b\\\""`},
+		{[]string{"a\n"}, "\"a\n\""},
+	}
+	for i, test := range tests {
+		c.Logf("test %d: %q", i, test.args)
+		result := utils.CommandString(test.args...)
+		c.Assert(result, gc.Equals, test.expected)
+	}
+}