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

[Andrew Wilkins]

Introduce utils/ssh.Client interface

This is a refactoring of utils/ssh to introduce
a Client interface which consists of Command and
Copy methods. The existing Command and ScpCommand
methods have been subsumed by the new OpenSSHClient
implementation.

All of the packages using ssh.Command/ScpCommand
have been updated to use the new client interface.
The options have also be changed to a struct which
is interpreted by the implementation. There is one
change in default options: password authentication
is now disabled by default, and must be explicitly
enabled.

There is a new option, which is identity files to
specify when authenticating. Clients are expected
to give these precedence over their client-specific
defaults (~/.ssh/id_rsa, etc.)

Parts of environs/manual/fake.ssh have been moved
to utils/ssh/testing.

https://codereview.appspot.com/49950044/

Introduce utils/ssh.Client interface

This is a refactoring of utils/ssh to introduce
a Client interface which consists of Command and
Copy methods. The existing Command and ScpCommand
methods have been subsumed by the new OpenSSHClient
implementation.

All of the packages using ssh.Command/ScpCommand
have been updated to use the new client interface.
The options have also be changed to a struct which
is interpreted by the implementation. There is one
change in default options: password authentication
is now disabled by default, and must be explicitly
enabled.

There is a new option, which is identity files to
specify when authenticating. Clients are expected
to give these precedence over their client-specific
defaults (~/.ssh/id_rsa, etc.)

Parts of environs/manual/fake.ssh have been moved
to utils/ssh/testing.

https://codereview.appspot.com/49950044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+201334_code.launchpad.net,

Message:
Please take a look.

Description:
Introduce utils/ssh.Client interface

This is a refactoring of utils/ssh to introduce
a Client interface which consists of Command and
Copy methods. The existing Command and ScpCommand
methods have been subsumed by the new OpenSSHClient
implementation.

All of the packages using ssh.Command/ScpCommand
have been updated to use the new client interface.
The options have also be changed to a struct which
is interpreted by the implementation. There is one
change in default options: password authentication
is now disabled by default, and must be explicitly
enabled.

There is a new option, which is identity files to
specify when authenticating. Clients are expected
to give these precedence over their client-specific
defaults (~/.ssh/id_rsa, etc.)

Parts of environs/manual/fake.ssh have been moved
to utils/ssh/testing.

https://code.launchpad.net/~axwalk/juju-core/ssh-client-interface/+merge/201334

(do not edit description out of merge proposal)

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

Affected files (+590, -221 lines):
   A [revision details]
   M cloudinit/sshinit/configure.go
   M cmd/juju/scp.go
   M cmd/juju/scp_test.go
   M cmd/juju/ssh.go
   M cmd/juju/ssh_test.go
   M environs/manual/fakessh.go
   M environs/manual/init.go
   M environs/manual/init_test.go
   M environs/sshstorage/storage.go
   M environs/sshstorage/storage_test.go
   M environs/testing/bootstrap.go
   M provider/common/bootstrap.go
   M provider/common/bootstrap_test.go
   M provider/null/environ.go
   M utils/ssh/run.go
   A utils/ssh/ssh.go
   M utils/ssh/ssh_openssh.go
   M utils/ssh/ssh_test.go
   A utils/ssh/testing/fakessh.go

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/49950044/

[Tim Penhey (thumper)]

https://codereview.appspot.com/49950044/diff/20001/cmd/juju/scp.go
File cmd/juju/scp.go (right):

https://codereview.appspot.com/49950044/diff/20001/cmd/juju/scp.go#newcode84
cmd/juju/scp.go:84: options.EnablePTY()
why does scp need this?

https://codereview.appspot.com/49950044/diff/20001/cmd/juju/ssh_test.go
File cmd/juju/ssh_test.go (right):

https://codereview.appspot.com/49950044/diff/20001/cmd/juju/ssh_test.go#newcode44
cmd/juju/ssh_test.go:44: s.PatchEnvironment("PATH",
s.bin+":"+os.Getenv("PATH"))
yay \o/

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

https://codereview.appspot.com/49950044/diff/20001/utils/ssh/ssh.go#newcode50
utils/ssh/ssh.go:50: o.identities = append([]string{}, identityFiles...)
why not just do
   o.identites = identityFiles?
it is a slice right?

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

https://codereview.appspot.com/49950044/diff/20001/utils/ssh/ssh_openssh.go#newcode15
utils/ssh/ssh_openssh.go:15: var opensshCommonOptions = []string{"-o",
"StrictHostKeyChecking no"}
Would love to kill this :-)

https://codereview.appspot.com/49950044/diff/20001/utils/ssh/testing/fakessh.go
File utils/ssh/testing/fakessh.go (right):

https://codereview.appspot.com/49950044/diff/20001/utils/ssh/testing/fakessh.go#newcode68
utils/ssh/testing/fakessh.go:68: case []string:
worth
   c.Check(output, gc.HasLen, 2)
?

https://codereview.appspot.com/49950044/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/49950044/diff/20001/cmd/juju/scp.go
File cmd/juju/scp.go (right):

https://codereview.appspot.com/49950044/diff/20001/cmd/juju/scp.go#newcode84
cmd/juju/scp.go:84: options.EnablePTY()
On 2014/01/14 02:20:29, thumper wrote:
> why does scp need this?

It doesn't. I don't know why I did that. Removed.

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

https://codereview.appspot.com/49950044/diff/20001/utils/ssh/ssh.go#newcode50
utils/ssh/ssh.go:50: o.identities = append([]string{}, identityFiles...)
On 2014/01/14 02:20:29, thumper wrote:
> why not just do
> o.identites = identityFiles?
> it is a slice right?

I want it to take a copy, making the value of o.identities immutable.
Otherwise we have to worry about things synchronisation, for one thing.

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

https://codereview.appspot.com/49950044/diff/20001/utils/ssh/ssh_openssh.go#newcode15
utils/ssh/ssh_openssh.go:15: var opensshCommonOptions = []string{"-o",
"StrictHostKeyChecking no"}
On 2014/01/14 02:20:29, thumper wrote:
> Would love to kill this :-)

Me too.

https://codereview.appspot.com/49950044/diff/20001/utils/ssh/testing/fakessh.go
File utils/ssh/testing/fakessh.go (right):

https://codereview.appspot.com/49950044/diff/20001/utils/ssh/testing/fakessh.go#newcode68
utils/ssh/testing/fakessh.go:68: case []string:
On 2014/01/14 02:20:29, thumper wrote:
> worth
> c.Check(output, gc.HasLen, 2)
> ?

Done - could help debug broken tests. I also added an assertion on
input's type.

https://codereview.appspot.com/49950044/

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/49950044/diff/30001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/49950044/diff/30001/provider/common/bootstrap.go#newcode317
provider/common/bootstrap.go:317: addr: addr,
Need to initialise client field here.

https://codereview.appspot.com/49950044/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/49950044/diff/30001/provider/common/bootstrap.go
File provider/common/bootstrap.go (right):

https://codereview.appspot.com/49950044/diff/30001/provider/common/bootstrap.go#newcode317
provider/common/bootstrap.go:317: addr: addr,
On 2014/01/14 08:23:37, axw wrote:
> Need to initialise client field here.

Done.

https://codereview.appspot.com/49950044/

[Tim Penhey (thumper)]

LGTM

https://codereview.appspot.com/49950044/

=== modified file 'cloudinit/sshinit/configure.go'
--- cloudinit/sshinit/configure.go	2014-01-07 13:01:01 +0000
+++ cloudinit/sshinit/configure.go	2014-01-14 09:50:43 +0000
@@ -4,7 +4,6 @@
 package sshinit
 
 import (
-	"encoding/base64"
 	"fmt"
 	"io"
 	"strings"
@@ -22,6 +21,10 @@
 	// Host is the host to configure, in the format [user@]hostname.
 	Host string
 
+	// Client is the SSH client to connect with.
+	// If Client is nil, ssh.DefaultClient will be used.
+	Client ssh.Client
+
 	// Config is the cloudinit config to carry out.
 	Config *cloudinit.Config
 
@@ -38,13 +41,12 @@
 		return err
 	}
 	logger.Debugf("running script on %s: %s", params.Host, script)
-	scriptBase64 := base64.StdEncoding.EncodeToString([]byte(script))
-	script = fmt.Sprintf(`F=$(mktemp); echo %s | base64 -d > $F; . $F`, scriptBase64)
-	cmd := ssh.Command(
-		params.Host,
-		[]string{"sudo", "-n", fmt.Sprintf("bash -c '%s'", script)},
-		ssh.NoPasswordAuthentication,
-	)
+	client := params.Client
+	if client == nil {
+		client = ssh.DefaultClient
+	}
+	cmd := ssh.Command(params.Host, []string{"sudo", "/bin/bash"}, nil)
+	cmd.Stdin = strings.NewReader(script)
 	cmd.Stderr = params.Stderr
 	return cmd.Run()
 }

=== modified file 'cmd/juju/scp.go'
--- cmd/juju/scp.go	2013-12-03 06:04:43 +0000
+++ cmd/juju/scp.go	2014-01-14 09:50:43 +0000
@@ -79,10 +79,5 @@
 			c.Args[i] = "ubuntu@" + host + ":" + v[1]
 		}
 	}
-
-	cmd := ssh.ScpCommand(c.Args[0], c.Args[1], ssh.NoPasswordAuthentication)
-	cmd.Stdin = ctx.Stdin
-	cmd.Stdout = ctx.Stdout
-	cmd.Stderr = ctx.Stderr
-	return cmd.Run()
+	return ssh.Copy(c.Args[0], c.Args[1], nil)
 }

=== modified file 'cmd/juju/scp_test.go'
--- cmd/juju/scp_test.go	2013-12-03 14:19:47 +0000
+++ cmd/juju/scp_test.go	2014-01-14 09:50:43 +0000
@@ -6,7 +6,9 @@
 import (
 	"bytes"
 	"fmt"
+	"io/ioutil"
 	"net/url"
+	"path/filepath"
 
 	gc "launchpad.net/gocheck"
 
@@ -69,7 +71,11 @@
 		ctx := coretesting.Context(c)
 		code := cmd.Main(&SCPCommand{}, ctx, t.args)
 		c.Check(code, gc.Equals, 0)
+		// we suppress stdout from scp
 		c.Check(ctx.Stderr.(*bytes.Buffer).String(), gc.Equals, "")
-		c.Check(ctx.Stdout.(*bytes.Buffer).String(), gc.Equals, t.result)
+		c.Check(ctx.Stdout.(*bytes.Buffer).String(), gc.Equals, "")
+		data, err := ioutil.ReadFile(filepath.Join(s.bin, "scp.args"))
+		c.Assert(err, gc.IsNil)
+		c.Assert(string(data), gc.Equals, t.result)
 	}
 }

=== modified file 'cmd/juju/ssh.go'
--- cmd/juju/ssh.go	2013-12-17 18:21:26 +0000
+++ cmd/juju/ssh.go	2014-01-14 09:50:43 +0000
@@ -88,7 +88,9 @@
 		// it from the CLI for backwards compatibility.
 		args = args[1:]
 	}
-	cmd := ssh.Command("ubuntu@"+host, args, ssh.NoPasswordAuthentication, ssh.AllocateTTY)
+	var options ssh.Options
+	options.EnablePTY()
+	cmd := ssh.Command("ubuntu@"+host, args, &options)
 	cmd.Stdin = ctx.Stdin
 	cmd.Stdout = ctx.Stdout
 	cmd.Stderr = ctx.Stderr

=== modified file 'cmd/juju/ssh_test.go'
--- cmd/juju/ssh_test.go	2013-12-03 14:19:47 +0000
+++ cmd/juju/ssh_test.go	2014-01-14 09:50:43 +0000
@@ -28,23 +28,22 @@
 
 type SSHCommonSuite struct {
 	testing.JujuConnSuite
-	oldpath string
+	bin string
 }
 
 // fakecommand outputs its arguments to stdout for verification
 var fakecommand = `#!/bin/bash
 
-echo $@
+echo $@ | tee $0.args
 `
 
 func (s *SSHCommonSuite) SetUpTest(c *gc.C) {
 	s.JujuConnSuite.SetUpTest(c)
 
-	path := c.MkDir()
-	s.oldpath = os.Getenv("PATH")
-	os.Setenv("PATH", path+":"+s.oldpath)
+	s.bin = c.MkDir()
+	s.PatchEnvironment("PATH", s.bin+":"+os.Getenv("PATH"))
 	for _, name := range []string{"ssh", "scp"} {
-		f, err := os.OpenFile(filepath.Join(path, name), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0777)
+		f, err := os.OpenFile(filepath.Join(s.bin, name), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0777)
 		c.Assert(err, gc.IsNil)
 		_, err = f.Write([]byte(fakecommand))
 		c.Assert(err, gc.IsNil)
@@ -53,11 +52,6 @@
 	}
 }
 
-func (s *SSHCommonSuite) TearDownTest(c *gc.C) {
-	os.Setenv("PATH", s.oldpath)
-	s.JujuConnSuite.TearDownTest(c)
-}
-
 const (
 	commonArgs = `-o StrictHostKeyChecking no -o PasswordAuthentication no `
 	sshArgs    = commonArgs + `-t `

=== modified file 'environs/manual/fakessh.go'
--- environs/manual/fakessh.go	2014-01-06 07:17:46 +0000
+++ environs/manual/fakessh.go	2014-01-14 09:50:43 +0000
@@ -4,69 +4,14 @@
 package manual
 
 import (
-	"fmt"
-	"io/ioutil"
-	"os"
-	"path/filepath"
 	"strings"
 
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/testing/testbase"
+	sshtesting "launchpad.net/juju-core/utils/ssh/testing"
 )
 
-// sshscript should only print the result on the first execution,
-// to handle the case where it's called multiple times. On
-// subsequent executions, it should find the next 'ssh' in $PATH
-// and exec that.
-var sshscript = `#!/bin/bash --norc
-if [ ! -e "$0.run" ]; then
-    touch "$0.run"
-    diff "$0.expected-input" -
-    exitcode=$?
-    if [ $exitcode -ne 0 ]; then
-        echo "ERROR: did not match expected input" >&2
-        exit $exitcode
-    fi
-    # stdout
-    %s
-    # stderr
-    %s
-    exit %d
-else
-    export PATH=${PATH#*:}
-    exec ssh $*
-fi`
-
-// installFakeSSH creates a fake "ssh" command in a new $PATH,
-// updates $PATH, and returns a function to reset $PATH to
-// its original value when called.
-//
-// output may be:
-//    - nil (no output)
-//    - a string (stdout)
-//    - a slice of strings, of length two (stdout, stderr)
-func installFakeSSH(c *gc.C, input string, output interface{}, rc int) testbase.Restorer {
-	fakebin := c.MkDir()
-	ssh := filepath.Join(fakebin, "ssh")
-	sshexpectedinput := ssh + ".expected-input"
-	var stdout, stderr string
-	switch output := output.(type) {
-	case nil:
-	case string:
-		stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output)
-	case []string:
-		stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output[0])
-		stderr = fmt.Sprintf("cat>&2<<EOF\n%s\nEOF", output[1])
-	}
-	script := fmt.Sprintf(sshscript, stdout, stderr, rc)
-	err := ioutil.WriteFile(ssh, []byte(script), 0777)
-	c.Assert(err, gc.IsNil)
-	err = ioutil.WriteFile(sshexpectedinput, []byte(input), 0644)
-	c.Assert(err, gc.IsNil)
-	return testbase.PatchEnvironment("PATH", fakebin+":"+os.Getenv("PATH"))
-}
-
 // installDetectionFakeSSH installs a fake SSH command, which will respond
 // to the series/hardware detection script with the specified
 // series/arch.
@@ -83,10 +28,10 @@
 		"MemTotal: 4096 kB",
 		"processor: 0",
 	}, "\n")
-	return installFakeSSH(c, detectionScript, detectionoutput, 0)
+	return sshtesting.InstallFakeSSH(c, detectionScript, detectionoutput, 0)
 }
 
-// FakeSSH wraps the invocation of installFakeSSH based on the parameters.
+// FakeSSH wraps the invocation of InstallFakeSSH based on the parameters.
 type fakeSSH struct {
 	Series string
 	Arch   string
@@ -121,11 +66,11 @@
 // output and exit codes.
 func (r fakeSSH) install(c *gc.C) testbase.Restorer {
 	var restore testbase.Restorer
-	add := func(input string, output interface{}, rc int) {
-		restore = restore.Add(installFakeSSH(c, input, output, rc))
+	add := func(input, output interface{}, rc int) {
+		restore = restore.Add(sshtesting.InstallFakeSSH(c, input, output, rc))
 	}
 	if !r.SkipProvisionAgent {
-		add("", nil, r.ProvisionAgentExitCode)
+		add(nil, nil, r.ProvisionAgentExitCode)
 	}
 	if !r.SkipDetection {
 		restore.Add(installDetectionFakeSSH(c, r.Series, r.Arch))
@@ -134,7 +79,7 @@
 	if r.Provisioned {
 		checkProvisionedOutput = "/etc/init/jujud-machine-0.conf"
 	}
-	add("", checkProvisionedOutput, r.CheckProvisionedExitCode)
+	add(checkProvisionedScript, checkProvisionedOutput, r.CheckProvisionedExitCode)
 	if r.InitUbuntuUser {
 		add("", nil, 0)
 	}

=== modified file 'environs/manual/init.go'
--- environs/manual/init.go	2014-01-03 05:14:11 +0000
+++ environs/manual/init.go	2014-01-14 09:50:43 +0000
@@ -27,10 +27,11 @@
 // exist on the host machine.
 func checkProvisioned(host string) (bool, error) {
 	logger.Infof("Checking if %s is already provisioned", host)
-	cmd := ssh.Command("ubuntu@"+host, []string{"bash", "-c", utils.ShQuote(checkProvisionedScript)}, ssh.NoPasswordAuthentication)
+	cmd := ssh.Command("ubuntu@"+host, []string{"/bin/bash"}, nil)
 	var stdout, stderr bytes.Buffer
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr
+	cmd.Stdin = strings.NewReader(checkProvisionedScript)
 	if err := cmd.Run(); err != nil {
 		if stderr.Len() != 0 {
 			err = fmt.Errorf("%v (%v)", err, strings.TrimSpace(stderr.String()))
@@ -52,7 +53,7 @@
 // by connecting to the machine and executing a bash script.
 func DetectSeriesAndHardwareCharacteristics(host string) (hc instance.HardwareCharacteristics, series string, err error) {
 	logger.Infof("Detecting series and characteristics on %s", host)
-	cmd := ssh.Command("ubuntu@"+host, []string{"bash"}, ssh.NoPasswordAuthentication)
+	cmd := ssh.Command("ubuntu@"+host, []string{"/bin/bash"}, nil)
 	var stdout, stderr bytes.Buffer
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr
@@ -160,8 +161,9 @@
 	//
 	// Note that we explicitly do not allocate a PTY, so we
 	// get a failure if sudo prompts.
-	cmd := ssh.Command("ubuntu@"+host, []string{"sudo", "-n", "true"}, ssh.NoPasswordAuthentication)
+	cmd := ssh.Command("ubuntu@"+host, []string{"sudo", "-n", "true"}, nil)
 	if cmd.Run() == nil {
+		logger.Infof("ubuntu user is already initialised")
 		return nil
 	}
 
@@ -171,7 +173,10 @@
 		host = login + "@" + host
 	}
 	script := fmt.Sprintf(initUbuntuScript, utils.ShQuote(authorizedKeys))
-	cmd = ssh.Command(host, []string{"sudo", "bash -c " + utils.ShQuote(script)}, ssh.AllocateTTY)
+	var options ssh.Options
+	options.AllowPasswordAuthentication()
+	options.EnablePTY()
+	cmd = ssh.Command(host, []string{"sudo", "/bin/bash -c " + utils.ShQuote(script)}, &options)
 	var stderr bytes.Buffer
 	cmd.Stdin = stdin
 	cmd.Stdout = stdout // for sudo prompt

=== modified file 'environs/manual/init_test.go'
--- environs/manual/init_test.go	2014-01-03 05:14:11 +0000
+++ environs/manual/init_test.go	2014-01-14 09:50:43 +0000
@@ -10,6 +10,7 @@
 
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
+	sshtesting "launchpad.net/juju-core/utils/ssh/testing"
 )
 
 type initialisationSuite struct {
@@ -25,7 +26,7 @@
 		"MemTotal: 4096 kB",
 		"processor: 0",
 	}, "\n")
-	defer installFakeSSH(c, detectionScript, response, 0)()
+	defer sshtesting.InstallFakeSSH(c, detectionScript, response, 0)()
 	_, series, err := DetectSeriesAndHardwareCharacteristics("whatever")
 	c.Assert(err, gc.IsNil)
 	c.Assert(series, gc.Equals, "edgy")
@@ -40,11 +41,11 @@
 	}, "\n")
 	// if the script fails for whatever reason, then checkProvisioned
 	// will return an error. stderr will be included in the error message.
-	defer installFakeSSH(c, detectionScript, []string{scriptResponse, "oh noes"}, 33)()
+	defer sshtesting.InstallFakeSSH(c, detectionScript, []string{scriptResponse, "oh noes"}, 33)()
 	hc, _, err := DetectSeriesAndHardwareCharacteristics("hostname")
 	c.Assert(err, gc.ErrorMatches, "exit status 33 \\(oh noes\\)")
 	// if the script doesn't fail, stderr is simply ignored.
-	defer installFakeSSH(c, detectionScript, []string{scriptResponse, "non-empty-stderr"}, 0)()
+	defer sshtesting.InstallFakeSSH(c, detectionScript, []string{scriptResponse, "non-empty-stderr"}, 0)()
 	hc, _, err = DetectSeriesAndHardwareCharacteristics("hostname")
 	c.Assert(err, gc.IsNil)
 	c.Assert(hc.String(), gc.Equals, "arch=arm cpu-cores=1 mem=4M")
@@ -105,7 +106,7 @@
 	for i, test := range tests {
 		c.Logf("test %d: %s", i, test.summary)
 		scriptResponse := strings.Join(test.scriptResponse, "\n")
-		defer installFakeSSH(c, detectionScript, scriptResponse, 0)()
+		defer sshtesting.InstallFakeSSH(c, detectionScript, scriptResponse, 0)()
 		hc, _, err := DetectSeriesAndHardwareCharacteristics("hostname")
 		c.Assert(err, gc.IsNil)
 		c.Assert(hc.String(), gc.Equals, test.expectedHc)
@@ -113,44 +114,44 @@
 }
 
 func (s *initialisationSuite) TestCheckProvisioned(c *gc.C) {
-	defer installFakeSSH(c, "", "", 0)()
+	defer sshtesting.InstallFakeSSH(c, checkProvisionedScript, "", 0)()
 	provisioned, err := checkProvisioned("example.com")
 	c.Assert(err, gc.IsNil)
 	c.Assert(provisioned, jc.IsFalse)
 
-	defer installFakeSSH(c, "", "non-empty", 0)()
+	defer sshtesting.InstallFakeSSH(c, checkProvisionedScript, "non-empty", 0)()
 	provisioned, err = checkProvisioned("example.com")
 	c.Assert(err, gc.IsNil)
 	c.Assert(provisioned, jc.IsTrue)
 
 	// stderr should not affect result.
-	defer installFakeSSH(c, "", []string{"", "non-empty-stderr"}, 0)()
+	defer sshtesting.InstallFakeSSH(c, checkProvisionedScript, []string{"", "non-empty-stderr"}, 0)()
 	provisioned, err = checkProvisioned("example.com")
 	c.Assert(err, gc.IsNil)
 	c.Assert(provisioned, jc.IsFalse)
 
 	// if the script fails for whatever reason, then checkProvisioned
 	// will return an error. stderr will be included in the error message.
-	defer installFakeSSH(c, "", []string{"non-empty-stdout", "non-empty-stderr"}, 255)()
+	defer sshtesting.InstallFakeSSH(c, checkProvisionedScript, []string{"non-empty-stdout", "non-empty-stderr"}, 255)()
 	_, err = checkProvisioned("example.com")
 	c.Assert(err, gc.ErrorMatches, "exit status 255 \\(non-empty-stderr\\)")
 }
 
 func (s *initialisationSuite) TestInitUbuntuUserNonExisting(c *gc.C) {
-	defer installFakeSSH(c, "", "", 0)() // successful creation of ubuntu user
-	defer installFakeSSH(c, "", "", 1)() // simulate failure of ubuntu@ login
+	defer sshtesting.InstallFakeSSH(c, "", "", 0)() // successful creation of ubuntu user
+	defer sshtesting.InstallFakeSSH(c, "", "", 1)() // simulate failure of ubuntu@ login
 	err := InitUbuntuUser("testhost", "testuser", "", nil, nil)
 	c.Assert(err, gc.IsNil)
 }
 
 func (s *initialisationSuite) TestInitUbuntuUserExisting(c *gc.C) {
-	defer installFakeSSH(c, "", nil, 0)()
+	defer sshtesting.InstallFakeSSH(c, "", nil, 0)()
 	InitUbuntuUser("testhost", "testuser", "", nil, nil)
 }
 
 func (s *initialisationSuite) TestInitUbuntuUserError(c *gc.C) {
-	defer installFakeSSH(c, "", []string{"", "failed to create ubuntu user"}, 123)()
-	defer installFakeSSH(c, "", "", 1)() // simulate failure of ubuntu@ login
+	defer sshtesting.InstallFakeSSH(c, "", []string{"", "failed to create ubuntu user"}, 123)()
+	defer sshtesting.InstallFakeSSH(c, "", "", 1)() // simulate failure of ubuntu@ login
 	err := InitUbuntuUser("testhost", "testuser", "", nil, nil)
 	c.Assert(err, gc.ErrorMatches, "exit status 123 \\(failed to create ubuntu user\\)")
 }

=== modified file 'environs/sshstorage/storage.go'
--- environs/sshstorage/storage.go	2014-01-03 05:14:11 +0000
+++ environs/sshstorage/storage.go	2014-01-14 09:50:43 +0000
@@ -11,7 +11,6 @@
 	"fmt"
 	"io"
 	"io/ioutil"
-	"os/exec"
 	"path"
 	"sort"
 	"strconv"
@@ -37,14 +36,14 @@
 	remotepath string
 	tmpdir     string
 
-	cmd     *exec.Cmd
+	cmd     *ssh.Cmd
 	stdin   io.WriteCloser
 	stdout  io.ReadCloser
 	scanner *bufio.Scanner
 }
 
-var sshCommand = func(host string, command string) *exec.Cmd {
-	return ssh.Command(host, []string{command}, ssh.NoPasswordAuthentication)
+var sshCommand = func(host string, command ...string) *ssh.Cmd {
+	return ssh.Command(host, command, nil)
 }
 
 type flockmode string
@@ -87,9 +86,10 @@
 		utils.ShQuote(params.TmpDir),
 	)
 
-	cmd := sshCommand(params.Host, fmt.Sprintf("sudo -n bash -c %s", utils.ShQuote(script)))
+	cmd := sshCommand(params.Host, "sudo", "-n", "/bin/bash")
 	var stderr bytes.Buffer
 	cmd.Stderr = &stderr
+	cmd.Stdin = strings.NewReader(script)
 	if err := cmd.Run(); err != nil {
 		err = fmt.Errorf("failed to create storage dir: %v (%v)", err, strings.TrimSpace(stderr.String()))
 		return nil, err

=== modified file 'environs/sshstorage/storage_test.go'
--- environs/sshstorage/storage_test.go	2014-01-03 05:14:11 +0000
+++ environs/sshstorage/storage_test.go	2014-01-14 09:50:43 +0000
@@ -13,6 +13,7 @@
 	"path"
 	"path/filepath"
 	"regexp"
+	"strings"
 	"time"
 
 	gc "launchpad.net/gocheck"
@@ -22,22 +23,23 @@
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/utils"
+	"launchpad.net/juju-core/utils/ssh"
 )
 
 type storageSuite struct {
 	testbase.LoggingSuite
+	bin string
 }
 
 var _ = gc.Suite(&storageSuite{})
 
-func sshCommandTesting(host string, command string) *exec.Cmd {
-	cmd := exec.Command("bash", "-c", command)
-	uid := fmt.Sprint(os.Getuid())
-	gid := fmt.Sprint(os.Getgid())
-	defer testbase.PatchEnvironment("SUDO_UID", uid)()
-	defer testbase.PatchEnvironment("SUDO_GID", gid)()
-	cmd.Env = os.Environ()
-	return cmd
+func (s *storageSuite) sshCommand(c *gc.C, host string, command ...string) *ssh.Cmd {
+	script := []byte("#!/bin/bash\n" + strings.Join(command, " "))
+	err := ioutil.WriteFile(filepath.Join(s.bin, "ssh"), script, 0755)
+	c.Assert(err, gc.IsNil)
+	client, err := ssh.NewOpenSSHClient()
+	c.Assert(err, gc.IsNil)
+	return client.Command(host, command, nil)
 }
 
 func newSSHStorage(host, storageDir, tmpDir string) (*SSHStorage, error) {
@@ -59,19 +61,24 @@
 	flockBin, err = exec.LookPath("flock")
 	c.Assert(err, gc.IsNil)
 
-	bin := c.MkDir()
-	restoreEnv := testbase.PatchEnvironment("PATH", bin+":"+os.Getenv("PATH"))
+	s.bin = c.MkDir()
+	restoreEnv := testbase.PatchEnvironment("PATH", s.bin+":"+os.Getenv("PATH"))
 	s.AddSuiteCleanup(func(*gc.C) { restoreEnv() })
 
-	// Create a "sudo" command which shifts away the "-n" and executes the remaining args.
-	err = ioutil.WriteFile(filepath.Join(bin, "sudo"), []byte("#!/bin/sh\nshift; exec \"$@\""), 0755)
+	// Create a "sudo" command which shifts away the "-n", sets
+	// SUDO_UID/SUDO_GID, and executes the remaining args.
+	err = ioutil.WriteFile(filepath.Join(s.bin, "sudo"), []byte(
+		"#!/bin/sh\nshift; export SUDO_UID=`id -u` SUDO_GID=`id -g`; exec \"$@\"",
+	), 0755)
 	c.Assert(err, gc.IsNil)
-	restoreSshCommand := testbase.PatchValue(&sshCommand, sshCommandTesting)
+	restoreSshCommand := testbase.PatchValue(&sshCommand, func(host string, command ...string) *ssh.Cmd {
+		return s.sshCommand(c, host, command...)
+	})
 	s.AddSuiteCleanup(func(*gc.C) { restoreSshCommand() })
 
 	// Create a new "flock" which calls the original, but in non-blocking mode.
 	data := []byte(fmt.Sprintf("#!/bin/sh\nexec %s --nonblock \"$@\"", flockBin))
-	err = ioutil.WriteFile(filepath.Join(bin, "flock"), data, 0755)
+	err = ioutil.WriteFile(filepath.Join(s.bin, "flock"), data, 0755)
 	c.Assert(err, gc.IsNil)
 }
 
@@ -167,18 +174,18 @@
 	//  3: second "install"
 	//  4: touch
 	var invocations int
-	badSshCommand := func(host string, command string) *exec.Cmd {
+	badSshCommand := func(host string, command ...string) *ssh.Cmd {
 		invocations++
 		switch invocations {
 		case 1, 3:
-			return exec.Command("true")
+			return s.sshCommand(c, host, "true")
 		case 2:
 			// Note: must close stdin before responding the first time, or
 			// the second command will race with closing stdin, and may
 			// flush first.
-			return exec.Command("bash", "-c", "head -n 1 > /dev/null; exec 0<&-; echo JUJU-RC: 0; echo blah blah; echo more")
+			return s.sshCommand(c, host, "head -n 1 > /dev/null; exec 0<&-; echo JUJU-RC: 0; echo blah blah; echo more")
 		case 4:
-			return exec.Command("bash", "-c", `head -n 1 > /dev/null; echo "Hey it's JUJU-RC: , but not at the beginning of the line"; echo more`)
+			return s.sshCommand(c, host, `head -n 1 > /dev/null; echo "Hey it's JUJU-RC: , but not at the beginning of the line"; echo more`)
 		default:
 			c.Errorf("unexpected invocation: #%d, %s", invocations, command)
 			return nil

=== modified file 'environs/testing/bootstrap.go'
--- environs/testing/bootstrap.go	2013-12-20 02:38:56 +0000
+++ environs/testing/bootstrap.go	2014-01-14 09:50:43 +0000
@@ -14,6 +14,7 @@
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/provider/common"
 	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/utils/ssh"
 )
 
 var logger = loggo.GetLogger("juju.environs.testing")
@@ -22,7 +23,7 @@
 // do not attempt to SSH to non-existent machines. The result is a function
 // that restores finishBootstrap.
 func DisableFinishBootstrap() func() {
-	f := func(environs.BootstrapContext, instance.Instance, *cloudinit.MachineConfig) error {
+	f := func(environs.BootstrapContext, ssh.Client, instance.Instance, *cloudinit.MachineConfig) error {
 		logger.Warningf("provider/common.FinishBootstrap is disabled")
 		return nil
 	}

=== modified file 'provider/common/bootstrap.go'
--- provider/common/bootstrap.go	2014-01-06 07:38:53 +0000
+++ provider/common/bootstrap.go	2014-01-14 09:50:43 +0000
@@ -39,6 +39,15 @@
 	var inst instance.Instance
 	defer func() { handleBootstrapError(err, ctx, inst, env) }()
 
+	// Get the bootstrap SSH client. Do this early, so we know
+	// not to bother with any of the below if we can't finish the job.
+	client := ssh.DefaultClient
+	if client == nil {
+		// This should never happen: if we don't have OpenSSH, then
+		// go.crypto/ssh should be used with an auto-generated key.
+		return fmt.Errorf("no SSH client available")
+	}
+
 	// Create an empty bootstrap state file so we can get its URL.
 	// It will be updated with the instance id and hardware characteristics
 	// after the bootstrap instance is started.
@@ -78,7 +87,7 @@
 	if err != nil {
 		return fmt.Errorf("cannot save state: %v", err)
 	}
-	return FinishBootstrap(ctx, inst, machineConfig)
+	return FinishBootstrap(ctx, client, inst, machineConfig)
 }
 
 // GenerateSystemSSHKey creates a new key for the system identity. The
@@ -143,7 +152,7 @@
 // to the instance via SSH and carrying out the cloud-config.
 //
 // Note: FinishBootstrap is exposed so it can be replaced for testing.
-var FinishBootstrap = func(ctx environs.BootstrapContext, inst instance.Instance, machineConfig *cloudinit.MachineConfig) error {
+var FinishBootstrap = func(ctx environs.BootstrapContext, client ssh.Client, inst instance.Instance, machineConfig *cloudinit.MachineConfig) error {
 	interrupted := make(chan os.Signal, 1)
 	ctx.InterruptNotify(interrupted)
 	defer ctx.StopInterruptNotify(interrupted)
@@ -168,7 +177,7 @@
 	// TODO: jam 2013-12-04 bug #1257649
 	// It would be nice if users had some controll over their bootstrap
 	// timeout, since it is unlikely to be a perfect match for all clouds.
-	addr, err := waitSSH(ctx, interrupted, checkNonceCommand, inst, DefaultBootstrapSSHTimeout())
+	addr, err := waitSSH(ctx, interrupted, client, checkNonceCommand, inst, DefaultBootstrapSSHTimeout())
 	if err != nil {
 		return err
 	}
@@ -184,6 +193,7 @@
 	}
 	return sshinit.Configure(sshinit.ConfigureParams{
 		Host:   "ubuntu@" + addr,
+		Client: client,
 		Config: cloudcfg,
 		Stderr: ctx.Stderr(),
 	})
@@ -227,7 +237,8 @@
 }
 
 type hostChecker struct {
-	addr instance.Address
+	addr   instance.Address
+	client ssh.Client
 
 	// checkDelay is the amount of time to wait between retries.
 	checkDelay time.Duration
@@ -256,7 +267,7 @@
 	var lastErr error
 	for {
 		go func() {
-			done <- connectSSH(hc.addr.Value, hc.checkHostScript)
+			done <- connectSSH(hc.client, hc.addr.Value, hc.checkHostScript)
 		}()
 		select {
 		case <-hc.closed:
@@ -278,7 +289,7 @@
 
 type parallelHostChecker struct {
 	*parallel.Try
-
+	client ssh.Client
 	stderr io.Writer
 
 	// active is a map of adresses to channels for addresses actively
@@ -304,6 +315,7 @@
 		closed := make(chan struct{})
 		hc := &hostChecker{
 			addr:            addr,
+			client:          p.client,
 			checkDelay:      p.checkDelay,
 			checkHostScript: p.checkHostScript,
 			closed:          closed,
@@ -329,10 +341,9 @@
 
 // connectSSH is called to connect to the specified host and
 // execute the "checkHostScript" bash script on it.
-var connectSSH = func(host, checkHostScript string) error {
-	cmd := ssh.Command("ubuntu@"+host, []string{
-		"/bin/bash", "-c", utils.ShQuote(checkHostScript),
-	}, ssh.NoPasswordAuthentication)
+var connectSSH = func(client ssh.Client, host, checkHostScript string) error {
+	cmd := client.Command("ubuntu@"+host, []string{"/bin/bash"}, nil)
+	cmd.Stdin = strings.NewReader(checkHostScript)
 	output, err := cmd.CombinedOutput()
 	if err != nil && len(output) > 0 {
 		err = fmt.Errorf("%s", strings.TrimSpace(string(output)))
@@ -349,7 +360,7 @@
 // the presence of a file on the machine that contains the
 // machine's nonce. The "checkHostScript" is a bash script
 // that performs this file check.
-func waitSSH(ctx environs.BootstrapContext, interrupted <-chan os.Signal, checkHostScript string, inst addresser, timeout SSHTimeoutOpts) (addr string, err error) {
+func waitSSH(ctx environs.BootstrapContext, interrupted <-chan os.Signal, client ssh.Client, checkHostScript string, inst addresser, timeout SSHTimeoutOpts) (addr string, err error) {
 	globalTimeout := time.After(timeout.Timeout)
 	pollAddresses := time.NewTimer(0)
 
@@ -358,6 +369,7 @@
 	// or the tomb is killed.
 	checker := parallelHostChecker{
 		Try:             parallel.NewTry(0, nil),
+		client:          client,
 		stderr:          ctx.Stderr(),
 		active:          make(map[instance.Address]chan struct{}),
 		checkDelay:      timeout.ConnectDelay,

=== modified file 'provider/common/bootstrap_test.go'
--- provider/common/bootstrap_test.go	2014-01-06 01:38:49 +0000
+++ provider/common/bootstrap_test.go	2014-01-14 09:50:43 +0000
@@ -25,6 +25,7 @@
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/tools"
+	"launchpad.net/juju-core/utils/ssh"
 )
 
 type BootstrapSuite struct {
@@ -41,7 +42,7 @@
 func (s *BootstrapSuite) SetUpTest(c *gc.C) {
 	s.LoggingSuite.SetUpTest(c)
 	s.ToolsFixture.SetUpTest(c)
-	s.PatchValue(common.ConnectSSH, func(host, checkHostScript string) error {
+	s.PatchValue(common.ConnectSSH, func(_ ssh.Client, host, checkHostScript string) error {
 		return fmt.Errorf("mock connection failure to %s", host)
 	})
 }
@@ -268,7 +269,7 @@
 
 func (s *BootstrapSuite) TestWaitSSHTimesOutWaitingForAddresses(c *gc.C) {
 	ctx, stderr := bootstrapContext(c)
-	_, err := common.WaitSSH(ctx, nil, "/bin/true", neverAddresses{}, testSSHTimeout)
+	_, err := common.WaitSSH(ctx, nil, ssh.DefaultClient, "/bin/true", neverAddresses{}, testSSHTimeout)
 	c.Check(err, gc.ErrorMatches, `waited for `+testSSHTimeout.Timeout.String()+` without getting any addresses`)
 	c.Check(stderr.String(), gc.Matches, "Waiting for address\n")
 }
@@ -280,7 +281,7 @@
 		<-time.After(2 * time.Millisecond)
 		interrupted <- os.Interrupt
 	}()
-	_, err := common.WaitSSH(ctx, interrupted, "/bin/true", neverAddresses{}, testSSHTimeout)
+	_, err := common.WaitSSH(ctx, interrupted, ssh.DefaultClient, "/bin/true", neverAddresses{}, testSSHTimeout)
 	c.Check(err, gc.ErrorMatches, "interrupted")
 	c.Check(stderr.String(), gc.Matches, "Waiting for address\n")
 }
@@ -295,7 +296,7 @@
 
 func (s *BootstrapSuite) TestWaitSSHStopsOnBadError(c *gc.C) {
 	ctx, stderr := bootstrapContext(c)
-	_, err := common.WaitSSH(ctx, nil, "/bin/true", brokenAddresses{}, testSSHTimeout)
+	_, err := common.WaitSSH(ctx, nil, ssh.DefaultClient, "/bin/true", brokenAddresses{}, testSSHTimeout)
 	c.Check(err, gc.ErrorMatches, "getting addresses: Addresses will never work")
 	c.Check(stderr.String(), gc.Equals, "Waiting for address\n")
 }
@@ -312,7 +313,7 @@
 func (s *BootstrapSuite) TestWaitSSHTimesOutWaitingForDial(c *gc.C) {
 	ctx, stderr := bootstrapContext(c)
 	// 0.x.y.z addresses are always invalid
-	_, err := common.WaitSSH(ctx, nil, "/bin/true", &neverOpensPort{addr: "0.1.2.3"}, testSSHTimeout)
+	_, err := common.WaitSSH(ctx, nil, ssh.DefaultClient, "/bin/true", &neverOpensPort{addr: "0.1.2.3"}, testSSHTimeout)
 	c.Check(err, gc.ErrorMatches,
 		`waited for `+testSSHTimeout.Timeout.String()+` without being able to connect: mock connection failure to 0.1.2.3`)
 	c.Check(stderr.String(), gc.Matches,
@@ -342,7 +343,7 @@
 	timeout := testSSHTimeout
 	timeout.Timeout = 1 * time.Minute
 	interrupted := make(chan os.Signal, 1)
-	_, err := common.WaitSSH(ctx, interrupted, "", &interruptOnDial{name: "0.1.2.3", interrupted: interrupted}, timeout)
+	_, err := common.WaitSSH(ctx, interrupted, ssh.DefaultClient, "", &interruptOnDial{name: "0.1.2.3", interrupted: interrupted}, timeout)
 	c.Check(err, gc.ErrorMatches, "interrupted")
 	// Exact timing is imprecise but it should have tried a few times before being killed
 	c.Check(stderr.String(), gc.Matches,
@@ -371,7 +372,7 @@
 
 func (s *BootstrapSuite) TestWaitSSHRefreshAddresses(c *gc.C) {
 	ctx, stderr := bootstrapContext(c)
-	_, err := common.WaitSSH(ctx, nil, "", &addressesChange{addrs: [][]string{
+	_, err := common.WaitSSH(ctx, nil, ssh.DefaultClient, "", &addressesChange{addrs: [][]string{
 		nil,
 		nil,
 		[]string{"0.1.2.3"},

=== modified file 'provider/null/environ.go'
--- provider/null/environ.go	2014-01-08 05:49:24 +0000
+++ provider/null/environ.go	2014-01-14 09:50:43 +0000
@@ -234,7 +234,7 @@
 }
 
 var runSSHCommand = func(host string, command []string) (stderr string, err error) {
-	cmd := ssh.Command(host, command, ssh.NoPasswordAuthentication)
+	cmd := ssh.Command(host, command, nil)
 	var stderrBuf bytes.Buffer
 	cmd.Stderr = &stderrBuf
 	err = cmd.Run()

=== modified file 'utils/ssh/run.go'
--- utils/ssh/run.go	2014-01-10 01:57:51 +0000
+++ utils/ssh/run.go	2014-01-14 09:50:43 +0000
@@ -33,11 +33,11 @@
 		return result, fmt.Errorf("missing host address")
 	}
 	logger.Debugf("execute on %s", params.Host)
-	options := []Option{NoPasswordAuthentication}
+	var options Options
 	if params.IdentityFile != "" {
-		options = append(options, Option{"-i", params.IdentityFile})
+		options.SetIdentities(params.IdentityFile)
 	}
-	command := Command(params.Host, []string{"/bin/bash", "-s"}, options...)
+	command := Command(params.Host, []string{"/bin/bash", "-s"}, &options)
 	// start a go routine to do the actual execution
 	var stdout, stderr bytes.Buffer
 	command.Stdout = &stdout
@@ -71,7 +71,7 @@
 	case <-time.After(params.Timeout):
 		logger.Infof("killing the command due to timeout")
 		err = fmt.Errorf("command timed out")
-		command.Process.Kill()
+		command.Kill()
 	}
 	// In either case, gather as much as we have from stdout and stderr
 	result.Stderr = stderr.Bytes()

=== added file 'utils/ssh/ssh.go'
--- utils/ssh/ssh.go	1970-01-01 00:00:00 +0000
+++ utils/ssh/ssh.go	2014-01-14 09:50:43 +0000
@@ -0,0 +1,202 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+// Package ssh contains utilities for dealing with SSH connections,
+// key management, and so on. All SSH-based command executions in
+// Juju should use the Command/ScpCommand functions in this package.
+//
+// TODO(axw) fallback to go.crypto/ssh if no native client is available.
+package ssh
+
+import (
+	"bytes"
+	"errors"
+	"io"
+)
+
+// Options is a client-implementation independent SSH options set.
+type Options struct {
+	// no PTY forced by default
+	allocatePTY bool
+	// password authentication is disallowed by default
+	passwordAuthAllowed bool
+	// identities is a sequence of paths to private key/identity files
+	// to use when attempting to login. A client implementaton may attempt
+	// with additional identities, but must give preference to these
+	identities []string
+}
+
+// EnablePTY forces the allocation of a pseudo-TTY.
+//
+// Forcing a pseudo-TTY is required, for example, for sudo
+// prompts on the target host.
+func (o *Options) EnablePTY() {
+	o.allocatePTY = true
+}
+
+// AllowPasswordAuthentication allows the SSH
+// client to prompt the user for a password.
+//
+// Password authentication is disallowed by default.
+func (o *Options) AllowPasswordAuthentication() {
+	o.passwordAuthAllowed = true
+}
+
+// SetIdentities sets a sequence of paths to private key/identity files
+// to use when attempting login. Client implementations may attempt to
+// use additional identities, but must give preference to the ones
+// specified here.
+func (o *Options) SetIdentities(identityFiles ...string) {
+	o.identities = append([]string{}, identityFiles...)
+}
+
+// Client is an interface for SSH clients to implement
+type Client interface {
+	// Command returns a Command for executing a command
+	// on the specified host. Each Command is executed
+	// within its own SSH session.
+	//
+	// Host is specified in the format [user@]host.
+	Command(host string, command []string, options *Options) *Cmd
+
+	// Copy copies a file between the local host and
+	// target host. Paths are specified in the scp format,
+	// [[user@]host:]path.
+	Copy(source, dest string, options *Options) error
+}
+
+// Cmd represents a command to be (or being) executed
+// on a remote host.
+type Cmd struct {
+	Stdin  io.Reader
+	Stdout io.Writer
+	Stderr io.Writer
+	impl   command
+}
+
+// CombinedOutput runs the command, and returns the
+// combined stdout/stderr output and result of
+// executing the command.
+func (c *Cmd) CombinedOutput() ([]byte, error) {
+	if c.Stdout != nil {
+		return nil, errors.New("ssh: Stdout already set")
+	}
+	if c.Stderr != nil {
+		return nil, errors.New("ssh: Stderr already set")
+	}
+	var b bytes.Buffer
+	c.Stdout = &b
+	c.Stderr = &b
+	err := c.Run()
+	return b.Bytes(), err
+}
+
+// Output runs the command, and returns the stdout
+// output and result of executing the command.
+func (c *Cmd) Output() ([]byte, error) {
+	if c.Stdout != nil {
+		return nil, errors.New("ssh: Stdout already set")
+	}
+	var b bytes.Buffer
+	c.Stdout = &b
+	err := c.Run()
+	return b.Bytes(), err
+}
+
+// Run runs the command, and returns the result as an error.
+func (c *Cmd) Run() error {
+	if err := c.Start(); err != nil {
+		return err
+	}
+	return c.Wait()
+}
+
+// Start starts the command running, but does not wait for
+// it to complete. If the command could not be started, an
+// error is returned.
+func (c *Cmd) Start() error {
+	c.impl.SetStdio(c.Stdin, c.Stdout, c.Stderr)
+	return c.impl.Start()
+}
+
+// Wait waits for the started command to complete,
+// and returns the result as an error.
+func (c *Cmd) Wait() error {
+	return c.impl.Wait()
+}
+
+// Kill kills the started command.
+func (c *Cmd) Kill() error {
+	return c.impl.Kill()
+}
+
+// StdinPipe creates a pipe and connects it to
+// the command's stdin. The read end of the pipe
+// is assigned to c.Stdin.
+func (c *Cmd) StdinPipe() (io.WriteCloser, error) {
+	wc, r, err := c.impl.StdinPipe()
+	if err != nil {
+		return nil, err
+	}
+	c.Stdin = r
+	return wc, nil
+}
+
+// StdoutPipe creates a pipe and connects it to
+// the command's stdout. The write end of the pipe
+// is assigned to c.Stdout.
+func (c *Cmd) StdoutPipe() (io.ReadCloser, error) {
+	rc, w, err := c.impl.StdoutPipe()
+	if err != nil {
+		return nil, err
+	}
+	c.Stdout = w
+	return rc, nil
+}
+
+// StderrPipe creates a pipe and connects it to
+// the command's stderr. The write end of the pipe
+// is assigned to c.Stderr.
+func (c *Cmd) StderrPipe() (io.ReadCloser, error) {
+	rc, w, err := c.impl.StderrPipe()
+	if err != nil {
+		return nil, err
+	}
+	c.Stderr = w
+	return rc, nil
+}
+
+// command is an implementation-specific representation of a
+// command prepared to execute against a specific host.
+type command interface {
+	Start() error
+	Wait() error
+	Kill() error
+	SetStdio(stdin io.Reader, stdout, stderr io.Writer)
+	StdinPipe() (io.WriteCloser, io.Reader, error)
+	StdoutPipe() (io.ReadCloser, io.Writer, error)
+	StderrPipe() (io.ReadCloser, io.Writer, error)
+}
+
+// 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.
+var DefaultClient Client
+
+func init() {
+	if client, err := NewOpenSSHClient(); err == nil {
+		DefaultClient = client
+	}
+}
+
+// Command is a short-cut for DefaultClient.Command.
+func Command(host string, command []string, options *Options) *Cmd {
+	return DefaultClient.Command(host, command, options)
+}
+
+// Copy is a short-cut for DefaultClient.Copy.
+func Copy(source, dest string, options *Options) error {
+	return DefaultClient.Copy(source, dest, options)
+}

=== renamed file 'utils/ssh/ssh.go' => 'utils/ssh/ssh_openssh.go'
--- utils/ssh/ssh.go	2013-12-17 08:43:06 +0000
+++ utils/ssh/ssh_openssh.go	2014-01-14 09:50:43 +0000
@@ -1,31 +1,18 @@
 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-// Package ssh contains utilities for dealing with SSH connections,
-// key management, and so on. All SSH-based command executions in
-// Juju should use the Command/ScpCommand functions in this package.
-//
-// TODO(axw) use PuTTY/plink if it's available on Windows.
-// TODO(axw) fallback to go.crypto/ssh if no native client is available.
 package ssh
 
 import (
+	"bytes"
+	"fmt"
+	"io"
 	"os"
 	"os/exec"
-)
-
-type Option []string
-
-var (
-	commonOptions Option = []string{"-o", "StrictHostKeyChecking no"}
-
-	// AllocateTTY forces pseudo-TTY allocation, which is required,
-	// for example, for sudo password prompts on the target host.
-	AllocateTTY Option = []string{"-t"}
-
-	// NoPasswordAuthentication disallows password-based authentication.
-	NoPasswordAuthentication Option = []string{"-o", "PasswordAuthentication no"}
-)
+	"strings"
+)
+
+var opensshCommonOptions = []string{"-o", "StrictHostKeyChecking no"}
 
 // sshpassWrap wraps the command/args with sshpass if it is found in $PATH
 // and the SSHPASS environment variable is set. Otherwise, the original
@@ -39,34 +26,111 @@
 	return cmd, args
 }
 
-// Command initialises an os/exec.Cmd to execute the native ssh program.
-//
-// If the SSHPASS environment variable is set, and the sshpass program
-// is available in $PATH, then the ssh command will be run with "sshpass -e".
-func Command(host string, command []string, options ...Option) *exec.Cmd {
-	args := append([]string{}, commonOptions...)
-	for _, option := range options {
-		args = append(args, option...)
-	}
+// OpenSSHClient is an implementation of Client that
+// uses the ssh and scp executables found in $PATH.
+type OpenSSHClient struct{}
+
+// NewOpenSSHClient creates a new OpenSSHClient.
+// If the ssh and scp programs cannot be found
+// in $PATH, then an error is returned.
+func NewOpenSSHClient() (*OpenSSHClient, error) {
+	var c OpenSSHClient
+	if _, err := exec.LookPath("ssh"); err != nil {
+		return nil, err
+	}
+	if _, err := exec.LookPath("scp"); err != nil {
+		return nil, err
+	}
+	return &c, nil
+}
+
+func opensshOptions(options *Options) []string {
+	args := append([]string{}, opensshCommonOptions...)
+	if options == nil {
+		options = &Options{}
+	}
+	if !options.passwordAuthAllowed {
+		args = append(args, "-o", "PasswordAuthentication no")
+	}
+	if options.allocatePTY {
+		args = append(args, "-t")
+	}
+	for _, identity := range options.identities {
+		args = append(args, "-i", identity)
+	}
+	return args
+}
+
+// Command implements Client.Command.
+func (c *OpenSSHClient) Command(host string, command []string, options *Options) *Cmd {
+	args := opensshOptions(options)
 	args = append(args, host)
 	if len(command) > 0 {
 		args = append(args, "--")
 		args = append(args, command...)
 	}
 	bin, args := sshpassWrap("ssh", args)
-	return exec.Command(bin, args...)
+	return &Cmd{impl: &opensshCmd{exec.Command(bin, args...)}}
 }
 
-// ScpCommand initialises an os/exec.Cmd to execute the native scp program.
-//
-// If the SSHPASS environment variable is set, and the sshpass program
-// is available in $PATH, then the scp command will be run with "sshpass -e".
-func ScpCommand(source, destination string, options ...Option) *exec.Cmd {
-	args := append([]string{}, commonOptions...)
-	for _, option := range options {
-		args = append(args, option...)
+// Copy implements Client.Copy.
+func (c *OpenSSHClient) Copy(source, dest string, userOptions *Options) error {
+	var options Options
+	if userOptions != nil {
+		options = *userOptions
+		options.allocatePTY = false // doesn't make sense for scp
 	}
-	args = append(args, source, destination)
+	args := opensshOptions(&options)
+	args = append(args, source, dest)
 	bin, args := sshpassWrap("scp", args)
-	return exec.Command(bin, args...)
+	cmd := exec.Command(bin, args...)
+	var stderr bytes.Buffer
+	cmd.Stderr = &stderr
+	if err := cmd.Run(); err != nil {
+		stderr := strings.TrimSpace(stderr.String())
+		if len(stderr) > 0 {
+			err = fmt.Errorf("%v (%v)", err, stderr)
+		}
+		return err
+	}
+	return nil
+}
+
+type opensshCmd struct {
+	*exec.Cmd
+}
+
+func (c *opensshCmd) SetStdio(stdin io.Reader, stdout, stderr io.Writer) {
+	c.Stdin, c.Stdout, c.Stderr = stdin, stdout, stderr
+}
+
+func (c *opensshCmd) StdinPipe() (io.WriteCloser, io.Reader, error) {
+	wc, err := c.Cmd.StdinPipe()
+	if err != nil {
+		return nil, nil, err
+	}
+	return wc, c.Stdin, nil
+}
+
+func (c *opensshCmd) StdoutPipe() (io.ReadCloser, io.Writer, error) {
+	rc, err := c.Cmd.StdoutPipe()
+	if err != nil {
+		return nil, nil, err
+	}
+	return rc, c.Stdout, nil
+}
+
+func (c *opensshCmd) StderrPipe() (io.ReadCloser, io.Writer, error) {
+	rc, err := c.Cmd.StderrPipe()
+	if err != nil {
+		return nil, nil, err
+	}
+	return rc, c.Stderr, nil
+}
+
+func (c *opensshCmd) Kill() error {
+	if c.Process == nil {
+		return fmt.Errorf("process has not been started")
+	}
+	return c.Process.Kill()
 }

=== modified file 'utils/ssh/ssh_test.go'
--- utils/ssh/ssh_test.go	2013-12-17 08:43:06 +0000
+++ utils/ssh/ssh_test.go	2014-01-14 09:50:43 +0000
@@ -7,6 +7,7 @@
 	"io/ioutil"
 	"os"
 	"path/filepath"
+	"strings"
 
 	gc "launchpad.net/gocheck"
 
@@ -18,49 +19,101 @@
 	testbase.LoggingSuite
 	testbin string
 	fakessh string
+	fakescp string
+	client  ssh.Client
 }
 
 var _ = gc.Suite(&SSHCommandSuite{})
 
+const echoCommandScript = "#!/bin/sh\necho $0 \"$@\" | tee $0.args"
+
 func (s *SSHCommandSuite) SetUpTest(c *gc.C) {
 	s.LoggingSuite.SetUpTest(c)
 	s.testbin = c.MkDir()
 	s.fakessh = filepath.Join(s.testbin, "ssh")
-	err := ioutil.WriteFile(s.fakessh, nil, 0755)
-	c.Assert(err, gc.IsNil)
-	s.PatchEnvironment("PATH", s.testbin)
-}
-
-func (s *SSHCommandSuite) TestCommand(c *gc.C) {
-	s.assertCommandArgs(c, "localhost", []string{"echo", "123"}, []string{
-		"ssh", "-o", "StrictHostKeyChecking no", "localhost", "--", "echo", "123",
-	})
-}
-
-func (s *SSHCommandSuite) assertCommandArgs(c *gc.C, hostname string, command []string, expected []string) {
-	cmd := ssh.Command("localhost", []string{"echo", "123"})
-	c.Assert(cmd, gc.NotNil)
-	c.Assert(cmd.Args, gc.DeepEquals, expected)
+	s.fakescp = filepath.Join(s.testbin, "scp")
+	err := ioutil.WriteFile(s.fakessh, []byte(echoCommandScript), 0755)
+	c.Assert(err, gc.IsNil)
+	err = ioutil.WriteFile(s.fakescp, []byte(echoCommandScript), 0755)
+	c.Assert(err, gc.IsNil)
+	s.PatchEnvironment("PATH", s.testbin+":"+os.Getenv("PATH"))
+	s.client, err = ssh.NewOpenSSHClient()
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *SSHCommandSuite) command(args ...string) *ssh.Cmd {
+	return s.commandOptions(args, nil)
+}
+
+func (s *SSHCommandSuite) commandOptions(args []string, opts *ssh.Options) *ssh.Cmd {
+	return s.client.Command("localhost", args, opts)
+}
+
+func (s *SSHCommandSuite) assertCommandArgs(c *gc.C, cmd *ssh.Cmd, expected string) {
+	out, err := cmd.Output()
+	c.Assert(err, gc.IsNil)
+	c.Assert(strings.TrimSpace(string(out)), gc.Equals, expected)
 }
 
 func (s *SSHCommandSuite) TestCommandSSHPass(c *gc.C) {
-	// First create a fake sshpass, but don't set SSHPASS
+	// First create a fake sshpass, but don't set $SSHPASS
 	fakesshpass := filepath.Join(s.testbin, "sshpass")
-	err := ioutil.WriteFile(fakesshpass, nil, 0755)
-	s.assertCommandArgs(c, "localhost", []string{"echo", "123"}, []string{
-		"ssh", "-o", "StrictHostKeyChecking no", "localhost", "--", "echo", "123",
-	})
-
-	// Now set SSHPASS.
+	err := ioutil.WriteFile(fakesshpass, []byte(echoCommandScript), 0755)
+	s.assertCommandArgs(c, s.command("echo", "123"),
+		s.fakessh+" -o StrictHostKeyChecking no -o PasswordAuthentication no localhost -- echo 123",
+	)
+	// Now set $SSHPASS.
 	s.PatchEnvironment("SSHPASS", "anyoldthing")
-	s.assertCommandArgs(c, "localhost", []string{"echo", "123"}, []string{
-		fakesshpass, "-e", "ssh", "-o", "StrictHostKeyChecking no", "localhost", "--", "echo", "123",
-	})
-
+	s.assertCommandArgs(c, s.command("echo", "123"),
+		fakesshpass+" -e ssh -o StrictHostKeyChecking no -o PasswordAuthentication no localhost -- echo 123",
+	)
 	// Finally, remove sshpass from $PATH.
 	err = os.Remove(fakesshpass)
 	c.Assert(err, gc.IsNil)
-	s.assertCommandArgs(c, "localhost", []string{"echo", "123"}, []string{
-		"ssh", "-o", "StrictHostKeyChecking no", "localhost", "--", "echo", "123",
-	})
+	s.assertCommandArgs(c, s.command("echo", "123"),
+		s.fakessh+" -o StrictHostKeyChecking no -o PasswordAuthentication no localhost -- echo 123",
+	)
+}
+
+func (s *SSHCommandSuite) TestCommand(c *gc.C) {
+	s.assertCommandArgs(c, s.command("echo", "123"),
+		s.fakessh+" -o StrictHostKeyChecking no -o PasswordAuthentication no localhost -- echo 123",
+	)
+}
+
+func (s *SSHCommandSuite) TestCommandEnablePTY(c *gc.C) {
+	var opts ssh.Options
+	opts.EnablePTY()
+	s.assertCommandArgs(c, s.commandOptions([]string{"echo", "123"}, &opts),
+		s.fakessh+" -o StrictHostKeyChecking no -o PasswordAuthentication no -t localhost -- echo 123",
+	)
+}
+
+func (s *SSHCommandSuite) TestCommandAllowPasswordAuthentication(c *gc.C) {
+	var opts ssh.Options
+	opts.AllowPasswordAuthentication()
+	s.assertCommandArgs(c, s.commandOptions([]string{"echo", "123"}, &opts),
+		s.fakessh+" -o StrictHostKeyChecking no localhost -- echo 123",
+	)
+}
+
+func (s *SSHCommandSuite) TestCommandIdentities(c *gc.C) {
+	var opts ssh.Options
+	opts.SetIdentities("x", "y")
+	s.assertCommandArgs(c, s.commandOptions([]string{"echo", "123"}, &opts),
+		s.fakessh+" -o StrictHostKeyChecking no -o PasswordAuthentication no -i x -i y localhost -- echo 123",
+	)
+}
+
+func (s *SSHCommandSuite) TestCopy(c *gc.C) {
+	var opts ssh.Options
+	opts.EnablePTY()
+	opts.AllowPasswordAuthentication()
+	opts.SetIdentities("x", "y")
+	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")
 }

=== added file 'utils/ssh/testing/fakessh.go'
--- utils/ssh/testing/fakessh.go	1970-01-01 00:00:00 +0000
+++ utils/ssh/testing/fakessh.go	2014-01-14 09:50:43 +0000
@@ -0,0 +1,81 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package testing
+
+import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/testing/testbase"
+)
+
+// sshscript should only print the result on the first execution,
+// to handle the case where it's called multiple times. On
+// subsequent executions, it should find the next 'ssh' in $PATH
+// and exec that.
+var sshscript = `#!/bin/bash --norc
+if [ ! -e "$0.run" ]; then
+    touch "$0.run"
+    if [ -e "$0.expected-input" ]; then
+        diff "$0.expected-input" -
+        exitcode=$?
+        if [ $exitcode -ne 0 ]; then
+            echo "ERROR: did not match expected input" >&2
+            exit $exitcode
+        fi
+        else
+            head >/dev/null
+        fi
+    # stdout
+    %s
+    # stderr
+    %s
+    exit %d
+else
+    export PATH=${PATH#*:}
+    exec ssh $*
+fi`
+
+// InstallFakeSSH creates a fake "ssh" command in a new $PATH,
+// updates $PATH, and returns a function to reset $PATH to its
+// original value when called.
+//
+// input may be:
+//    - nil (ignore input)
+//    - a string (match input exactly)
+// output may be:
+//    - nil (no output)
+//    - a string (stdout)
+//    - a slice of strings, of length two (stdout, stderr)
+func InstallFakeSSH(c *gc.C, input, output interface{}, rc int) testbase.Restorer {
+	fakebin := c.MkDir()
+	ssh := filepath.Join(fakebin, "ssh")
+	switch input := input.(type) {
+	case nil:
+	case string:
+		sshexpectedinput := ssh + ".expected-input"
+		err := ioutil.WriteFile(sshexpectedinput, []byte(input), 0644)
+		c.Assert(err, gc.IsNil)
+	default:
+		c.Errorf("input has invalid type: %T", input)
+	}
+	var stdout, stderr string
+	switch output := output.(type) {
+	case nil:
+	case string:
+		stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output)
+	case []string:
+		c.Assert(output, gc.HasLen, 2)
+		stdout = fmt.Sprintf("cat<<EOF\n%s\nEOF", output[0])
+		stderr = fmt.Sprintf("cat>&2<<EOF\n%s\nEOF", output[1])
+	}
+	script := fmt.Sprintf(sshscript, stdout, stderr, rc)
+	err := ioutil.WriteFile(ssh, []byte(script), 0777)
+	c.Assert(err, gc.IsNil)
+	return testbase.PatchEnvironment("PATH", fakebin+":"+os.Getenv("PATH"))
+}