Merge lp:~axwalk/juju-core/add-machine-ssh-password into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

utils/ssh: use sshpass if found and $SSHPASS set

This enables scripted manual provisioning when all
that's available is a password. This is not as secure
as using a key pair, or when using a real TTY; that's
a security consideration the user must make.

https://codereview.appspot.com/43310043/

utils/ssh: use sshpass if found and $SSHPASS set

This enables scripted manual provisioning when all
that's available is a password. This is not as secure
as using a key pair, or when using a real TTY; that's
a security consideration the user must make.

https://codereview.appspot.com/43310043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+199233_code.launchpad.net,

Message:
Please take a look.

Description:
utils/ssh: use sshpass if found and $SSHPASS set

This enables scripted manual provisioning when all
that's available is a password. This is not as secure
as using a key pair, or when using a real TTY; that's
a security consideration the user must make.

https://code.launchpad.net/~axwalk/juju-core/add-machine-ssh-password/+merge/199233

(do not edit description out of merge proposal)

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

Affected files (+98, -3 lines):
   A [revision details]
   M cmd/jujud/machine_test.go
   M utils/ssh/ssh.go
   A utils/ssh/ssh_test.go

[William Reade (fwereade)]

LGTM

https://codereview.appspot.com/43310043/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/43310043/diff/1/cmd/jujud/machine_test.go#newcode29
cmd/jujud/machine_test.go:29: "launchpad.net/juju-core/testing"
already imported as "coretesting", I can see you didn't do it but please
do tidy it up ;).

https://codereview.appspot.com/43310043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/43310043/diff/1/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/43310043/diff/1/cmd/jujud/machine_test.go#newcode29
cmd/jujud/machine_test.go:29: "launchpad.net/juju-core/testing"
On 2013/12/18 15:01:03, fwereade wrote:
> already imported as "coretesting", I can see you didn't do it but
please do tidy
> it up ;).

Done.

https://codereview.appspot.com/43310043/

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/add-machine-ssh-password into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.753s
ok launchpad.net/juju-core/agent/tools 0.280s
ok launchpad.net/juju-core/bzr 7.413s
ok launchpad.net/juju-core/cert 3.465s
ok launchpad.net/juju-core/charm 0.601s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.075s
ok launchpad.net/juju-core/cloudinit/sshinit 1.189s
ok launchpad.net/juju-core/cmd 0.412s
? 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 185.090s
FAIL launchpad.net/juju-core/cmd/jujud [build failed]
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.708s
? launchpad.net/juju-core/cmd/plugins/juju-update-bootstrap [no test files]
ok launchpad.net/juju-core/constraints 0.041s
ok launchpad.net/juju-core/container 0.035s
ok launchpad.net/juju-core/container/factory 0.049s
ok launchpad.net/juju-core/container/kvm 0.293s
ok launchpad.net/juju-core/container/kvm/mock 0.035s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.330s
? 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.358s
ok launchpad.net/juju-core/environs 3.084s
ok launchpad.net/juju-core/environs/bootstrap 4.468s
ok launchpad.net/juju-core/environs/cloudinit 0.857s
ok launchpad.net/juju-core/environs/config 1.046s
ok launchpad.net/juju-core/environs/configstore 0.058s
ok launchpad.net/juju-core/environs/filestorage 0.061s
ok launchpad.net/juju-core/environs/httpstorage 1.127s
ok launchpad.net/juju-core/environs/imagemetadata 0.568s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.094s
ok launchpad.net/juju-core/environs/jujutest 0.277s
ok launchpad.net/juju-core/environs/manual 5.858s
ok launchpad.net/juju-core/environs/simplestreams 0.341s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.167s
ok launchpad.net/juju-core/environs/storage 1.134s
ok launchpad.net/juju-core/environs/sync 31.313s
ok launchpad.net/juju-core/environs/testing 0.199s
ok launchpad.net/juju-core/environs/tools 6.931s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.041s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 17.143s
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.022s
ok launchpad.net/juju-core/names 0.026s
? launchpad.net/juju-core/provider [no test files]
? launchpad.net/juju-core/provider/all [no test files]
ok l...

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/add-machine-ssh-password into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.726s
ok launchpad.net/juju-core/agent/tools 0.224s
ok launchpad.net/juju-core/bzr 7.541s
ok launchpad.net/juju-core/cert 3.263s
ok launchpad.net/juju-core/charm 0.562s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.069s
ok launchpad.net/juju-core/cloudinit/sshinit 1.143s
ok launchpad.net/juju-core/cmd 0.240s
? 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 184.368s
FAIL launchpad.net/juju-core/cmd/jujud [build failed]
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 2.802s
? launchpad.net/juju-core/cmd/plugins/juju-update-bootstrap [no test files]
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container 0.037s
ok launchpad.net/juju-core/container/factory 0.049s
ok launchpad.net/juju-core/container/kvm 0.273s
ok launchpad.net/juju-core/container/kvm/mock 0.095s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.324s
? 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.292s
ok launchpad.net/juju-core/environs 3.034s
ok launchpad.net/juju-core/environs/bootstrap 4.542s
ok launchpad.net/juju-core/environs/cloudinit 0.852s
ok launchpad.net/juju-core/environs/config 1.975s
ok launchpad.net/juju-core/environs/configstore 0.043s
ok launchpad.net/juju-core/environs/filestorage 0.032s
ok launchpad.net/juju-core/environs/httpstorage 0.889s
ok launchpad.net/juju-core/environs/imagemetadata 0.541s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.070s
ok launchpad.net/juju-core/environs/jujutest 0.255s
ok launchpad.net/juju-core/environs/manual 5.924s
ok launchpad.net/juju-core/environs/simplestreams 0.319s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.367s
ok launchpad.net/juju-core/environs/storage 1.151s
ok launchpad.net/juju-core/environs/sync 30.983s
ok launchpad.net/juju-core/environs/testing 0.216s
ok launchpad.net/juju-core/environs/tools 6.933s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.017s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 17.149s
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.021s
ok launchpad.net/juju-core/names 0.026s
? launchpad.net/juju-core/provider [no test files]
? launchpad.net/juju-core/provider/all [no test files]
ok l...

[Andrew Wilkins (axwalk)]

I don't get it, I removed that import. It's in the branch. It's not in the diff though...

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/43310043/

=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2013-12-18 15:38:24 +0000
+++ cmd/jujud/machine_test.go	2013-12-19 02:24:09 +0000
@@ -26,7 +26,6 @@
 	"launchpad.net/juju-core/state/api/params"
 	statetesting "launchpad.net/juju-core/state/testing"
 	"launchpad.net/juju-core/state/watcher"
-	"launchpad.net/juju-core/testing"
 	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
@@ -236,7 +235,7 @@
 	// The deployer actually removes the unit just after
 	// removing its deployment, so we need to poll here
 	// until it actually happens.
-	for attempt := testing.LongAttempt.Start(); attempt.Next(); {
+	for attempt := coretesting.LongAttempt.Start(); attempt.Next(); {
 		err := u0.Refresh()
 		if err == nil && attempt.HasNext() {
 			continue
@@ -346,7 +345,7 @@
 	addrs := []instance.Address{instance.NewAddress("1.2.3.4")}
 	dummy.SetInstanceAddresses(insts[0], addrs)
 
-	for a := testing.LongAttempt.Start(); a.Next(); {
+	for a := coretesting.LongAttempt.Start(); a.Next(); {
 		if !a.HasNext() {
 			c.Logf("final machine addresses: %#v", m.Addresses())
 			c.Fatalf("timed out waiting for machine to get address")
@@ -368,7 +367,7 @@
 	c.Assert(err, gc.IsNil)
 	w := m.Watch()
 	defer w.Stop()
-	timeout := time.After(testing.LongWait)
+	timeout := time.After(coretesting.LongWait)
 	for {
 		select {
 		case <-timeout:
@@ -395,8 +394,8 @@
 }
 
 var fastDialOpts = api.DialOpts{
-	Timeout:    testing.LongWait,
-	RetryDelay: testing.ShortWait,
+	Timeout:    coretesting.LongWait,
+	RetryDelay: coretesting.ShortWait,
 }
 
 func (s *MachineSuite) waitStopped(c *gc.C, job state.MachineJob, a *MachineAgent, done chan error) {
@@ -448,7 +447,7 @@
 	case agentAPI := <-agentAPIs:
 		c.Assert(agentAPI, gc.NotNil)
 		test(conf, agentAPI)
-	case <-time.After(testing.LongWait):
+	case <-time.After(coretesting.LongWait):
 		c.Fatalf("API not opened")
 	}
 
@@ -481,7 +480,7 @@
 	case agentState := <-agentStates:
 		c.Assert(agentState, gc.NotNil)
 		test(conf, agentState)
-	case <-time.After(testing.LongWait):
+	case <-time.After(coretesting.LongWait):
 		c.Fatalf("state not opened")
 	}
 
@@ -522,12 +521,12 @@
 		// Trigger a sync on the state used by the agent, and wait
 		// for the unit to be removed.
 		agentState.StartSync()
-		timeout := time.After(testing.LongWait)
+		timeout := time.After(coretesting.LongWait)
 		for done := false; !done; {
 			select {
 			case <-timeout:
 				c.Fatalf("unit not cleaned up")
-			case <-time.After(testing.ShortWait):
+			case <-time.After(coretesting.ShortWait):
 				s.State.StartSync()
 			case <-w.Changes():
 				err := unit.Refresh()
@@ -555,12 +554,12 @@
 		// Trigger a sync on the state used by the agent, and wait for the unit
 		// to be created.
 		agentState.StartSync()
-		timeout := time.After(testing.LongWait)
+		timeout := time.After(coretesting.LongWait)
 		for {
 			select {
 			case <-timeout:
 				c.Fatalf("unit not created")
-			case <-time.After(testing.ShortWait):
+			case <-time.After(coretesting.ShortWait):
 				s.State.StartSync()
 			case <-w.Changes():
 				units, err := service.AllUnits()
@@ -590,13 +589,13 @@
 
 	// Wait for ssh keys file to be updated.
 	s.State.StartSync()
-	timeout := time.After(testing.LongWait)
+	timeout := time.After(coretesting.LongWait)
 	sshKeyWithCommentPrefix := sshtesting.ValidKeyOne.Key + " Juju:user@host"
 	for {
 		select {
 		case <-timeout:
 			c.Fatalf("timeout while waiting for authorised ssh keys to change")
-		case <-time.After(testing.ShortWait):
+		case <-time.After(coretesting.ShortWait):
 			keys, err := ssh.ListKeys(ssh.FullKeys)
 			c.Assert(err, gc.IsNil)
 			keysStr := strings.Join(keys, "\n")

=== modified file 'utils/ssh/ssh.go'
--- utils/ssh/ssh.go	2013-12-03 05:20:25 +0000
+++ utils/ssh/ssh.go	2013-12-19 02:24:09 +0000
@@ -1,9 +1,16 @@
 // 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 (
+	"os"
 	"os/exec"
 )
 
@@ -20,7 +27,22 @@
 	NoPasswordAuthentication Option = []string{"-o", "PasswordAuthentication no"}
 )
 
+// 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.
+func sshpassWrap(cmd string, args []string) (string, []string) {
+	if os.Getenv("SSHPASS") != "" {
+		if path, err := exec.LookPath("sshpass"); err == nil {
+			return path, append([]string{"-e", cmd}, args...)
+		}
+	}
+	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 {
@@ -31,15 +53,20 @@
 		args = append(args, "--")
 		args = append(args, command...)
 	}
-	return exec.Command("ssh", args...)
+	bin, args := sshpassWrap("ssh", args)
+	return 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...)
 	}
 	args = append(args, source, destination)
-	return exec.Command("scp", args...)
+	bin, args := sshpassWrap("scp", args)
+	return exec.Command(bin, args...)
 }

=== added file 'utils/ssh/ssh_test.go'
--- utils/ssh/ssh_test.go	1970-01-01 00:00:00 +0000
+++ utils/ssh/ssh_test.go	2013-12-19 02:24:09 +0000
@@ -0,0 +1,66 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package ssh_test
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/utils/ssh"
+)
+
+type SSHCommandSuite struct {
+	testbase.LoggingSuite
+	testbin string
+	fakessh string
+}
+
+var _ = gc.Suite(&SSHCommandSuite{})
+
+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)
+}
+
+func (s *SSHCommandSuite) TestCommandSSHPass(c *gc.C) {
+	// 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.
+	s.PatchEnvironment("SSHPASS", "anyoldthing")
+	s.assertCommandArgs(c, "localhost", []string{"echo", "123"}, []string{
+		fakesshpass, "-e", "ssh", "-o", "StrictHostKeyChecking 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",
+	})
+}