Merge lp:~axwalk/juju-core/gocryptossh-proxycommand into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

ProxyCommand support for go.crypto/ssh client

Implement support for the ProxyCommand option
in the go.crypto/ssh-based client. This is just
a matter of executing the provided command, and
using the process' stdin/stdout to form a
net.Conn on which the SSH session is created.

https://codereview.appspot.com/77300045/

ProxyCommand support for go.crypto/ssh client

Implement support for the ProxyCommand option
in the go.crypto/ssh-based client. This is just
a matter of executing the provided command, and
using the process' stdin/stdout to form a
net.Conn on which the SSH session is created.

https://codereview.appspot.com/77300045/

[Andrew Wilkins (axwalk)]

Reviewers: mp+211671_code.launchpad.net,

Message:
Please take a look.

Description:
ProxyCommand support for go.crypto/ssh client

Implement support for the ProxyCommand option
in the go.crypto/ssh-based client. This is just
a matter of executing the provided command, and
using the process' stdin/stdout to form a
net.Conn on which the SSH session is created.

https://code.launchpad.net/~axwalk/juju-core/gocryptossh-proxycommand/+merge/211671

(do not edit description out of merge proposal)

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

Affected files (+99, -17 lines):
   A [revision details]
   M cmd/juju/ssh.go
   M cmd/juju/ssh_test.go
   M utils/ssh/ssh_gocrypto.go
   M utils/ssh/ssh_gocrypto_test.go

[William Reade (fwereade)]

LGTM so long as it's been used live.

https://codereview.appspot.com/77300045/

[Andrew Wilkins (axwalk)]

On 2014/03/24 09:34:49, fwereade wrote:
> LGTM so long as it's been used live.

Thanks. It has been tested live, but the prereq was rolled back as it
broke local (if machine-0 doesn't have sshd). I'll wait till 1.18 it
out.

https://codereview.appspot.com/77300045/

[John A Meinel (jameinel)]

It seems odd to have an in-process SSH library that is doing our connecting, only to have it spawn out to a required "ssh" executable.
I thought the point of using gocrypto's ssh was only when an actual "ssh" wasn't available, which means ProxyCommand doesn't seem very useful.
I realize there could be other use cases for ProxyCommand, though I haven't seen them myself.
Is there a different use case that I'm missing?

Is this intended so that we can proxy via the API server?

[Andrew Wilkins (axwalk)]

> It seems odd to have an in-process SSH library that is doing our connecting,
> only to have it spawn out to a required "ssh" executable.
> I thought the point of using gocrypto's ssh was only when an actual "ssh"
> wasn't available, which means ProxyCommand doesn't seem very useful.

In cmd/juju/ssh.go, we set ProxyCommand to "juju ssh ...", not "ssh". Thus, it proxies through the same go.crypto/ssh-based implementation. There's still no requirement for OpenSSH to be available.

> I realize there could be other use cases for ProxyCommand, though I haven't
> seen them myself.
> Is there a different use case that I'm missing?
>
> Is this intended so that we can proxy via the API server?

Yes, that is currently the only use case.

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/77300045/

[Andrew Wilkins (axwalk)]

On 2014/03/26 08:20:05, axw wrote:
> Please take a look.

Updated this CL to add a proxy-config attribute to base config which
defaults to true for all providers but local.

https://codereview.appspot.com/77300045/

[William Reade (fwereade)]

Couple of significant fixes needed for the proxy stuff I'm afraid.

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

https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go#newcode151
cmd/juju/ssh.go:151: // this avoids another API round-trip.
Not valid, sadly: we can't depend on bootstrap config being available.

https://codereview.appspot.com/77300045/diff/20001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/77300045/diff/20001/environs/config/config.go#newcode450
environs/config/config.go:450: panic(fmt.Errorf("proxy-ssh missing in
configuration"))
Configs written by older versions must remain valid. Can we not just
treat it as false-if-missing?

https://codereview.appspot.com/77300045/diff/20001/provider/local/environprovider.go
File provider/local/environprovider.go (right):

https://codereview.appspot.com/77300045/diff/20001/provider/local/environprovider.go#newcode111
provider/local/environprovider.go:111: "proxy-ssh": false,
This is bugging me a bit. Rather than silently overwriting potential bad
config, can we instead just refuse it and error out?

https://codereview.appspot.com/77300045/

[Andrew Wilkins (axwalk)]

Please take a look.

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

https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go#newcode151
cmd/juju/ssh.go:151: // this avoids another API round-trip.
On 2014/03/27 12:28:57, fwereade wrote:
> Not valid, sadly: we can't depend on bootstrap config being available.

Updated to use bootstrap config if there, else go via API. Is this
valid?

https://codereview.appspot.com/77300045/diff/20001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/77300045/diff/20001/environs/config/config.go#newcode450
environs/config/config.go:450: panic(fmt.Errorf("proxy-ssh missing in
configuration"))
On 2014/03/27 12:28:57, fwereade wrote:
> Configs written by older versions must remain valid. Can we not just
treat it as
> false-if-missing?

We do. See alwaysOptional & allDefaults. If missing, proxy-ssh takes its
value from alwaysOptional (=false); new environments take the default
from allDefaults (=true).

Got rid of the panic anyway.

https://codereview.appspot.com/77300045/

[William Reade (fwereade)]

LGTM, if you drop the local config check -- it adds an implicit
dependency to cmd, on an implementation detail of environs/config; and
we already have a conn open, so it shouldn't take too long to just ask
remotely. Ping me to discuss if you feel really strongly...

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

https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go#newcode151
cmd/juju/ssh.go:151: // this avoids another API round-trip.
On 2014/03/27 13:50:54, axw wrote:
> On 2014/03/27 12:28:57, fwereade wrote:
> > Not valid, sadly: we can't depend on bootstrap config being
available.

> Updated to use bootstrap config if there, else go via API. Is this
valid?

Well, I don't love depending on the immutability, and we already have a
conn open. How much time do we save here, and how often?

https://codereview.appspot.com/77300045/

[Andrew Wilkins (axwalk)]

On 2014/03/28 13:38:15, fwereade wrote:
> LGTM, if you drop the local config check -- it adds an implicit
dependency to
> cmd, on an implementation detail of environs/config; and we already
have a conn
> open, so it shouldn't take too long to just ask remotely. Ping me to
discuss if
> you feel really strongly...

Fair call. I'll remove the check, and also make proxy-ssh mutable.

> https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go
> File cmd/juju/ssh.go (right):

https://codereview.appspot.com/77300045/diff/20001/cmd/juju/ssh.go#newcode151
> cmd/juju/ssh.go:151: // this avoids another API round-trip.
> On 2014/03/27 13:50:54, axw wrote:
> > On 2014/03/27 12:28:57, fwereade wrote:
> > > Not valid, sadly: we can't depend on bootstrap config being
available.
> >
> > Updated to use bootstrap config if there, else go via API. Is this
valid?

> Well, I don't love depending on the immutability, and we already have
a conn
> open. How much time do we save here, and how often?

It adds about 1s in my tests against Azure (10s vs. 9s). Not a big deal
I suppose.
I just realised that the second "juju ssh" call is making an unnecessary
API connection, so I'll fix that up.

https://codereview.appspot.com/77300045/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/77300045/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/77300045/

[John A Meinel (jameinel)]

I'm testing the bot, will Approve this in a moment.

[Go Bot (go-bot)]

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

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 1.039s
ok launchpad.net/juju-core/agent/mongo 0.509s
ok launchpad.net/juju-core/agent/tools 0.235s
ok launchpad.net/juju-core/bzr 5.129s
ok launchpad.net/juju-core/cert 2.674s
ok launchpad.net/juju-core/charm 0.388s
? 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.029s
ok launchpad.net/juju-core/cloudinit/sshinit 0.929s
ok launchpad.net/juju-core/cmd 0.166s
ok launchpad.net/juju-core/cmd/charm-admin 0.730s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/envcmd 0.181s
ok launchpad.net/juju-core/cmd/juju 205.919s
[jujuc whatever]
[remote]
[/path/to/remote]
[remote --help]
[unknown]
[remote --error borken]
[remote --unknown]
[remote unwanted]

----------------------------------------------------------------------
FAIL: unit_test.go:137: UnitSuite.TestRunStop

[LOG] 10.59072 DEBUG juju.environs.configstore Making /tmp/juju-core-test.Ax6TNv/gocheck-6334824724549167320/18/home/ubuntu/.juju/environments
[LOG] 10.65625 DEBUG juju.environs.tools reading v1.* tools
[LOG] 10.65628 INFO juju environs/testing: uploading FAKE tools 1.19.0-precise-amd64
[LOG] 10.65682 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 10.65684 DEBUG juju.environs.tools no series specified when finding tools, looking for any
[LOG] 10.65687 DEBUG juju.environs.simplestreams fetchData failed for "tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found not found
[LOG] 10.65689 DEBUG juju.environs.simplestreams cannot load index "streams/v1/index.sjson": invalid URL "tools/streams/v1/index.sjson" not found
[LOG] 10.65694 DEBUG juju.environs.simplestreams fetchData failed for "tools/streams/v1/index.json": file "tools/streams/v1/index.json" not found not found
[LOG] 10.65696 DEBUG juju.environs.simplestreams cannot load index "streams/v1/index.json": invalid URL "tools/streams/v1/index.json" not found
[LOG] 10.65716 INFO juju.environs.tools Writing tools/streams/v1/index.json
[LOG] 10.65720 INFO juju.environs.tools Writing tools/streams/v1/com.ubuntu.juju:released:tools.json
[LOG] 10.65725 DEBUG juju.environs.bootstrap environment "dummyenv" supports service/machine networks: true
[LOG] 10.65726 INFO juju.environs.bootstrap bootstrapping environment "dummyenv"
[LOG] 10.65728 DEBUG juju.environs.bootstrap looking for bootstrap tools: series="precise", arch=<nil>, version=1.19.0
[LOG] 10.65729 INFO juju.environs.tools reading tools with major.minor version 1.19
[LOG] 10.65730 INFO juju.environs.tools filtering tools by version: 1.19.0
[LOG] 10.65731 INFO juju.environs.tools filtering tools by series: precise
[LOG] 10.65732 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 10.65740 DEBUG juju.environs.simplestreams fetchData failed for "tools/streams/...

[Go Bot (go-bot)]

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

mktemp: failed to create directory via template `/tmp/juju-core-test.XXXXXX': No such file or directory
mkdir /tmp/go-build216606492: no such file or directory
mongod: no process found

=== modified file 'cmd/juju/debughooks.go'
--- cmd/juju/debughooks.go	2013-12-17 18:21:26 +0000
+++ cmd/juju/debughooks.go	2014-04-02 06:17:00 +0000
@@ -12,7 +12,6 @@
 	"launchpad.net/juju-core/charm/hooks"
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/names"
-	"launchpad.net/juju-core/state/api/params"
 	unitdebug "launchpad.net/juju-core/worker/uniter/debug"
 )
 
@@ -55,52 +54,12 @@
 	return nil
 }
 
-// getRelationNames1dot16 gets the list of relation hooks directly from the
-// database, in a fashion compatible with the API server in Juju 1.16 (which
-// doesn't have the ServiceCharmRelations API). This function can be removed
-// when we no longer maintain compatibility with 1.16
-func (c *DebugHooksCommand) getRelationNames1dot16() ([]string, error) {
-	err := c.ensureRawConn()
-	if err != nil {
-		return nil, err
-	}
-	unit, err := c.rawConn.State.Unit(c.Target)
-	if err != nil {
-		return nil, err
-	}
-	service, err := unit.Service()
-	if err != nil {
-		return nil, err
-	}
-	endpoints, err := service.Endpoints()
-	if err != nil {
-		return nil, err
-	}
-	relations := make([]string, len(endpoints))
-	for i, endpoint := range endpoints {
-		relations[i] = endpoint.Relation.Name
-	}
-	return relations, nil
-}
-
-func (c *DebugHooksCommand) getRelationNames(serviceName string) ([]string, error) {
-	relations, err := c.apiClient.ServiceCharmRelations(serviceName)
-	if params.IsCodeNotImplemented(err) {
-		logger.Infof("API server does not support Client.ServiceCharmRelations falling back to 1.16 compatibility mode (direct DB access)")
-		return c.getRelationNames1dot16()
-	}
-	if err != nil {
-		return nil, err
-	}
-	return relations, err
-}
-
 func (c *DebugHooksCommand) validateHooks() error {
 	if len(c.hooks) == 0 {
 		return nil
 	}
 	service := names.UnitService(c.Target)
-	relations, err := c.getRelationNames(service)
+	relations, err := c.apiClient.ServiceCharmRelations(service)
 	if err != nil {
 		return err
 	}

=== modified file 'cmd/juju/scp.go'
--- cmd/juju/scp.go	2014-03-20 02:30:15 +0000
+++ cmd/juju/scp.go	2014-04-02 06:17:00 +0000
@@ -106,5 +106,15 @@
 			targets = append(targets, arg)
 		}
 	}
-	return ssh.Copy(targets, extraArgs, nil)
+
+	var options *ssh.Options
+	if proxy, err := c.proxySSH(); err != nil {
+		return err
+	} else if proxy {
+		options = new(ssh.Options)
+		if err := c.setProxyCommand(options); err != nil {
+			return err
+		}
+	}
+	return ssh.Copy(targets, extraArgs, options)
 }

=== modified file 'cmd/juju/scp_test.go'
--- cmd/juju/scp_test.go	2014-03-30 22:34:01 +0000
+++ cmd/juju/scp_test.go	2014-04-02 06:17:00 +0000
@@ -27,67 +27,64 @@
 	about  string
 	args   []string
 	result string
+	proxy  bool
 	error  string
 }{
 	{
-		"scp from machine 0 to current dir",
-		[]string{"0:foo", "."},
-		commonArgs + "[e-mail]:foo .\n",
-		"",
-	},
-	{
-		"scp from machine 0 to current dir with extra args",
-		[]string{"0:foo", ".", "-rv -o SomeOption"},
-		commonArgs + "-rv -o SomeOption [e-mail]:foo .\n",
-		"",
-	},
-	{
-		"scp from current dir to machine 0",
-		[]string{"foo", "0:"},
-		commonArgs + "foo [e-mail]:\n",
-		"",
-	},
-	{
-		"scp from current dir to machine 0 with extra args",
-		[]string{"foo", "0:", "-r -v"},
-		commonArgs + "-r -v foo [e-mail]:\n",
-		"",
-	},
-	{
-		"scp from machine 0 to unit mysql/0",
-		[]string{"0:foo", "mysql/0:/foo"},
-		commonArgs + "[e-mail]:foo [e-mail]:/foo\n",
-		"",
-	},
-	{
-		"scp from machine 0 to unit mysql/0 and extra args",
-		[]string{"0:foo", "mysql/0:/foo", "-q"},
-		commonArgs + "-q [e-mail]:foo [e-mail]:/foo\n",
-		"",
-	},
-	{
-		"scp from machine 0 to unit mysql/0 and extra args before",
-		[]string{"-q", "-r", "0:foo", "mysql/0:/foo"},
-		"",
-		`unexpected argument "-q"; extra arguments must be last`,
-	},
-	{
-		"scp two local files to unit mysql/0",
-		[]string{"file1", "file2", "mysql/0:/foo/"},
-		commonArgs + "file1 file2 [e-mail]:/foo/\n",
-		"",
-	},
-	{
-		"scp from unit mongodb/1 to unit mongodb/0 and multiple extra args",
-		[]string{"mongodb/1:foo", "mongodb/0:", "-r -v -q -l5"},
-		commonArgs + "-r -v -q -l5 [e-mail]:foo [e-mail]:\n",
-		"",
-	},
-	{
-		"scp works with IPv6 addresses",
-		[]string{"ipv6-svc/0:foo", "bar"},
-		commonArgs + `ubuntu@\[2001:db8::\]:foo bar` + "\n",
-		"",
+		about:  "scp from machine 0 to current dir",
+		args:   []string{"0:foo", "."},
+		result: commonArgsNoProxy + "[e-mail]:foo .\n",
+	},
+	{
+		about:  "scp from machine 0 to current dir with extra args",
+		args:   []string{"0:foo", ".", "-rv -o SomeOption"},
+		result: commonArgsNoProxy + "-rv -o SomeOption [e-mail]:foo .\n",
+	},
+	{
+		about:  "scp from current dir to machine 0",
+		args:   []string{"foo", "0:"},
+		result: commonArgsNoProxy + "foo [e-mail]:\n",
+	},
+	{
+		about:  "scp from current dir to machine 0 with extra args",
+		args:   []string{"foo", "0:", "-r -v"},
+		result: commonArgsNoProxy + "-r -v foo [e-mail]:\n",
+	},
+	{
+		about:  "scp from machine 0 to unit mysql/0",
+		args:   []string{"0:foo", "mysql/0:/foo"},
+		result: commonArgsNoProxy + "[e-mail]:foo [e-mail]:/foo\n",
+	},
+	{
+		about:  "scp from machine 0 to unit mysql/0 and extra args",
+		args:   []string{"0:foo", "mysql/0:/foo", "-q"},
+		result: commonArgsNoProxy + "-q [e-mail]:foo [e-mail]:/foo\n",
+	},
+	{
+		about: "scp from machine 0 to unit mysql/0 and extra args before",
+		args:  []string{"-q", "-r", "0:foo", "mysql/0:/foo"},
+		error: `unexpected argument "-q"; extra arguments must be last`,
+	},
+	{
+		about:  "scp two local files to unit mysql/0",
+		args:   []string{"file1", "file2", "mysql/0:/foo/"},
+		result: commonArgsNoProxy + "file1 file2 [e-mail]:/foo/\n",
+	},
+	{
+		about:  "scp from unit mongodb/1 to unit mongodb/0 and multiple extra args",
+		args:   []string{"mongodb/1:foo", "mongodb/0:", "-r -v -q -l5"},
+		result: commonArgsNoProxy + "-r -v -q -l5 [e-mail]:foo [e-mail]:\n",
+	},
+	{
+		about:  "scp works with IPv6 addresses",
+		args:   []string{"ipv6-svc/0:foo", "bar"},
+		result: commonArgsNoProxy + `ubuntu@\[2001:db8::\]:foo bar` + "\n",
+	},
+	{
+		about:  "scp from machine 0 to unit mysql/0 with proxy",
+		args:   []string{"0:foo", "mysql/0:/foo"},
+		result: commonArgs + "[e-mail]:foo [e-mail]:/foo\n",
+		proxy:  true,
 	},
 }
 
@@ -122,6 +119,7 @@
 		c.Logf("test %d: %s -> %s\n", i, t.about, t.args)
 		ctx := coretesting.Context(c)
 		scpcmd := &SCPCommand{}
+		scpcmd.proxy = t.proxy
 
 		err := scpcmd.Init(t.args)
 		c.Check(err, gc.IsNil)

=== modified file 'cmd/juju/ssh.go'
--- cmd/juju/ssh.go	2014-04-01 04:53:43 +0000
+++ cmd/juju/ssh.go	2014-04-02 06:17:00 +0000
@@ -6,15 +6,19 @@
 import (
 	"errors"
 	"fmt"
+	"net"
+	"os"
+	"os/exec"
 	"time"
 
+	"launchpad.net/gnuflag"
+
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/cmd/envcmd"
-	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/juju"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state/api"
-	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/utils"
 	"launchpad.net/juju-core/utils/ssh"
 )
@@ -27,11 +31,32 @@
 // SSHCommon provides common methods for SSHCommand, SCPCommand and DebugHooksCommand.
 type SSHCommon struct {
 	envcmd.EnvCommandBase
+	proxy     bool
+	pty       bool
 	Target    string
 	Args      []string
 	apiClient *api.Client
-	// Only used for compatibility with 1.16
-	rawConn *juju.Conn
+	apiAddr   string
+}
+
+func (c *SSHCommon) SetFlags(f *gnuflag.FlagSet) {
+	c.EnvCommandBase.SetFlags(f)
+	f.BoolVar(&c.proxy, "proxy", true, "proxy through the API server")
+	f.BoolVar(&c.pty, "pty", true, "enable pseudo-tty allocation")
+}
+
+// setProxyCommand sets the proxy command option.
+func (c *SSHCommon) setProxyCommand(options *ssh.Options) error {
+	apiServerHost, _, err := net.SplitHostPort(c.apiAddr)
+	if err != nil {
+		return fmt.Errorf("failed to get proxy address: %v", err)
+	}
+	juju, err := getJujuExecutable()
+	if err != nil {
+		return fmt.Errorf("failed to get juju executable path: %v", err)
+	}
+	options.SetProxyCommand(juju, "ssh", "--proxy=false", "--pty=false", apiServerHost, "nc", "-q0", "%h", "%p")
+	return nil
 }
 
 const sshDoc = `
@@ -80,23 +105,40 @@
 	return nil
 }
 
+// getJujuExecutable returns the path to the juju
+// executable, or an error if it could not be found.
+var getJujuExecutable = func() (string, error) {
+	return exec.LookPath(os.Args[0])
+}
+
 // Run resolves c.Target to a machine, to the address of a i
 // machine or unit forks ssh passing any arguments provided.
 func (c *SSHCommand) Run(ctx *cmd.Context) error {
 	if c.apiClient == nil {
-		var err error
-		c.apiClient, err = c.initAPIClient()
-		if err != nil {
-			return err
-		}
-		defer c.apiClient.Close()
+		// If the apClient is not already opened and it is opened
+		// by ensureAPIClient, then close it when we're done.
+		defer func() {
+			if c.apiClient != nil {
+				c.apiClient.Close()
+				c.apiClient = nil
+			}
+		}()
 	}
 	host, err := c.hostFromTarget(c.Target)
 	if err != nil {
 		return err
 	}
 	var options ssh.Options
-	options.EnablePTY()
+	if c.pty {
+		options.EnablePTY()
+	}
+	if proxy, err := c.proxySSH(); err != nil {
+		return err
+	} else if proxy {
+		if err := c.setProxyCommand(&options); err != nil {
+			return err
+		}
+	}
 	cmd := ssh.Command("ubuntu@"+host, c.Args, &options)
 	cmd.Stdin = ctx.Stdin
 	cmd.Stdout = ctx.Stdout
@@ -104,12 +146,45 @@
 	return cmd.Run()
 }
 
+// proxySSH returns true iff both c.proxy and
+// the proxy-ssh environment configuration
+// are true.
+func (c *SSHCommon) proxySSH() (bool, error) {
+	if !c.proxy {
+		return false, nil
+	}
+	if _, err := c.ensureAPIClient(); err != nil {
+		return false, err
+	}
+	var cfg *config.Config
+	attrs, err := c.apiClient.EnvironmentGet()
+	if err == nil {
+		cfg, err = config.New(config.NoDefaults, attrs)
+	}
+	if err != nil {
+		return false, err
+	}
+	logger.Debugf("proxy-ssh is %v", cfg.ProxySSH())
+	return cfg.ProxySSH(), nil
+}
+
+func (c *SSHCommon) ensureAPIClient() (*api.Client, error) {
+	if c.apiClient != nil {
+		return c.apiClient, nil
+	}
+	return c.initAPIClient()
+}
+
 // initAPIClient initialises the API connection.
 // It is the caller's responsibility to close the connection.
 func (c *SSHCommon) initAPIClient() (*api.Client, error) {
-	var err error
-	c.apiClient, err = juju.NewAPIClientFromName(c.EnvName)
-	return c.apiClient, err
+	st, err := juju.NewAPIFromName(c.EnvName)
+	if err != nil {
+		return nil, err
+	}
+	c.apiClient = st.Client()
+	c.apiAddr = st.Addr()
+	return c.apiClient, nil
 }
 
 // attemptStarter is an interface corresponding to utils.AttemptStrategy
@@ -132,86 +207,31 @@
 	Delay: 500 * time.Millisecond,
 }
 
-// ensureRawConn ensures that c.rawConn is valid (or returns an error)
-// This is only for compatibility with a 1.16 API server (that doesn't have
-// some of the API added more recently.) It can be removed once we no longer
-// need compatibility with direct access to the state database
-func (c *SSHCommon) ensureRawConn() error {
-	if c.rawConn != nil {
-		return nil
-	}
-	var err error
-	c.rawConn, err = juju.NewConnFromName(c.EnvName)
-	return err
-}
-
-func (c *SSHCommon) hostFromTarget1dot16(target string) (string, error) {
-	err := c.ensureRawConn()
-	if err != nil {
-		return "", err
-	}
-	// is the target the id of a machine ?
-	if names.IsMachine(target) {
-		logger.Infof("looking up address for machine %s...", target)
-		// This is not the exact code from the 1.16 client
-		// (machinePublicAddress), however it is the code used in the
-		// apiserver behind the PublicAddress call. (1.16 didn't know
-		// about SelectPublicAddress)
-		// The old code watched for changes on the Machine until it had
-		// an InstanceId and then would return the instance.WaitDNS()
-		machine, err := c.rawConn.State.Machine(target)
-		if err != nil {
-			return "", err
-		}
-		addr := instance.SelectPublicAddress(machine.Addresses())
-		if addr == "" {
-			return "", fmt.Errorf("machine %q has no public address", machine)
-		}
-		return addr, nil
-	}
-	// maybe the target is a unit ?
-	if names.IsUnit(target) {
-		logger.Infof("looking up address for unit %q...", c.Target)
-		unit, err := c.rawConn.State.Unit(target)
-		if err != nil {
-			return "", err
-		}
-		addr, ok := unit.PublicAddress()
-		if !ok {
-			return "", fmt.Errorf("unit %q has no public address", unit)
-		}
-		return addr, nil
-	}
-	return "", fmt.Errorf("unknown unit or machine %q", target)
-}
-
 func (c *SSHCommon) hostFromTarget(target string) (string, error) {
-	var addr string
-	var err error
-	var useStateConn bool
+	// If the target is neither a machine nor a unit,
+	// assume it's a hostname and try it directly.
+	if !names.IsMachine(target) && !names.IsUnit(target) {
+		return target, nil
+	}
 	// A target may not initially have an address (e.g. the
 	// address updater hasn't yet run), so we must do this in
 	// a loop.
+	if _, err := c.ensureAPIClient(); err != nil {
+		return "", err
+	}
+	var err error
 	for a := sshHostFromTargetAttemptStrategy.Start(); a.Next(); {
-		if !useStateConn {
+		var addr string
+		if c.proxy {
+			addr, err = c.apiClient.PrivateAddress(target)
+		} else {
 			addr, err = c.apiClient.PublicAddress(target)
-			if params.IsCodeNotImplemented(err) {
-				logger.Infof("API server does not support Client.PublicAddress falling back to 1.16 compatibility mode (direct DB access)")
-				useStateConn = true
-			}
-		}
-		if useStateConn {
-			addr, err = c.hostFromTarget1dot16(target)
 		}
 		if err == nil {
-			break
+			return addr, nil
 		}
 	}
-	if err != nil {
-		return "", err
-	}
-	logger.Infof("Resolved public address of %q: %q", target, addr)
-	return addr, nil
+	return "", err
 }
 
 // AllowInterspersedFlags for ssh/scp is set to false so that

=== modified file 'cmd/juju/ssh_test.go'
--- cmd/juju/ssh_test.go	2014-04-01 03:37:13 +0000
+++ cmd/juju/ssh_test.go	2014-04-02 06:17:00 +0000
@@ -39,6 +39,7 @@
 
 func (s *SSHCommonSuite) SetUpTest(c *gc.C) {
 	s.JujuConnSuite.SetUpTest(c)
+	s.PatchValue(&getJujuExecutable, func() (string, error) { return "juju", nil })
 
 	s.bin = c.MkDir()
 	s.PatchEnvPathPrepend(s.bin)
@@ -53,8 +54,10 @@
 }
 
 const (
-	commonArgs = `-o StrictHostKeyChecking no -o PasswordAuthentication no `
-	sshArgs    = commonArgs + `-t -t `
+	commonArgsNoProxy = `-o StrictHostKeyChecking no -o PasswordAuthentication no `
+	commonArgs        = `-o StrictHostKeyChecking no -o ProxyCommand juju ssh --proxy=false --pty=false 127.0.0.1 nc -q0 %h %p -o PasswordAuthentication no `
+	sshArgs           = commonArgs + `-t -t `
+	sshArgsNoProxy    = commonArgsNoProxy + `-t -t `
 )
 
 var sshTests = []struct {
@@ -82,6 +85,11 @@
 		[]string{"ssh", "mongodb/1", "ls", "/"},
 		sshArgs + "[e-mail] ls /\n",
 	},
+	{
+		"connect to unit mysql/0 without proxy",
+		[]string{"ssh", "--proxy=false", "mysql/0"},
+		sshArgsNoProxy + "[e-mail]\n",
+	},
 }
 
 func (s *SSHSuite) TestSSHCommand(c *gc.C) {
@@ -114,6 +122,20 @@
 	}
 }
 
+func (s *SSHSuite) TestSSHCommandEnvironProxySSH(c *gc.C) {
+	s.makeMachines(1, c, true)
+	// Setting proxy-ssh=false in the environment overrides --proxy.
+	err := s.State.UpdateEnvironConfig(map[string]interface{}{"proxy-ssh": false}, nil, nil)
+	c.Assert(err, gc.IsNil)
+	ctx := coretesting.Context(c)
+	jujucmd := cmd.NewSuperCommand(cmd.SuperCommandParams{})
+	jujucmd.Register(&SSHCommand{})
+	code := cmd.Main(jujucmd, ctx, []string{"ssh", "0"})
+	c.Check(code, gc.Equals, 0)
+	c.Check(ctx.Stderr.(*bytes.Buffer).String(), gc.Equals, "")
+	c.Check(ctx.Stdout.(*bytes.Buffer).String(), gc.Equals, sshArgsNoProxy+"[e-mail]\n")
+}
+
 type callbackAttemptStarter struct {
 	next func() bool
 }
@@ -131,10 +153,16 @@
 }
 
 func (s *SSHSuite) TestSSHCommandHostAddressRetry(c *gc.C) {
+	s.testSSHCommandHostAddressRetry(c, false)
+}
+
+func (s *SSHSuite) TestSSHCommandHostAddressRetryProxy(c *gc.C) {
+	s.testSSHCommandHostAddressRetry(c, true)
+}
+
+func (s *SSHSuite) testSSHCommandHostAddressRetry(c *gc.C, proxy bool) {
 	m := s.makeMachines(1, c, false)
 	ctx := coretesting.Context(c)
-	jujucmd := cmd.NewSuperCommand(cmd.SuperCommandParams{})
-	jujucmd.Register(&SSHCommand{})
 
 	var called int
 	next := func() bool {
@@ -146,18 +174,21 @@
 
 	// Ensure that the ssh command waits for a public address, or the attempt
 	// strategy's Done method returns false.
-	code := cmd.Main(jujucmd, ctx, []string{"ssh", "0"})
+	args := []string{"--proxy=" + fmt.Sprint(proxy), "0"}
+	code := cmd.Main(&SSHCommand{}, ctx, args)
 	c.Check(code, gc.Equals, 1)
 	c.Assert(called, gc.Equals, 2)
 	called = 0
 	attemptStarter.next = func() bool {
 		called++
-		s.setAddress(m[0], c)
-		return false
+		if called > 1 {
+			s.setAddress(m[0], c)
+		}
+		return true
 	}
-	code = cmd.Main(jujucmd, ctx, []string{"ssh", "0"})
+	code = cmd.Main(&SSHCommand{}, ctx, args)
 	c.Check(code, gc.Equals, 0)
-	c.Assert(called, gc.Equals, 1)
+	c.Assert(called, gc.Equals, 2)
 }
 
 func (s *SSHCommonSuite) setAddress(m *state.Machine, c *gc.C) {

=== modified file 'environs/config/config.go'
--- environs/config/config.go	2014-03-20 22:35:39 +0000
+++ environs/config/config.go	2014-04-02 06:17:00 +0000
@@ -442,6 +442,13 @@
 	return c.mustString("authorized-keys")
 }
 
+// ProxySSH returns a flag indicating whether SSH commands
+// should be proxied through the API server.
+func (c *Config) ProxySSH() bool {
+	value, _ := c.defined["proxy-ssh"].(bool)
+	return value
+}
+
 // ProxySettings returns all four proxy settings; http, https, ftp, and no
 // proxy.
 func (c *Config) ProxySettings() osenv.ProxySettings {
@@ -716,6 +723,7 @@
 	"bootstrap-retry-delay":     schema.ForceInt(),
 	"bootstrap-addresses-delay": schema.ForceInt(),
 	"test-mode":                 schema.Bool(),
+	"proxy-ssh":                 schema.Bool(),
 
 	// Deprecated fields, retain for backwards compatibility.
 	"tools-url": schema.String(),
@@ -773,6 +781,7 @@
 	// Previously image-stream could be set to an empty value
 	"image-stream": "",
 	"test-mode":    false,
+	"proxy-ssh":    false,
 }
 
 func allowEmpty(attr string) bool {
@@ -796,6 +805,7 @@
 		"bootstrap-timeout":         DefaultBootstrapSSHTimeout,
 		"bootstrap-retry-delay":     DefaultBootstrapSSHRetryDelay,
 		"bootstrap-addresses-delay": DefaultBootstrapSSHAddressesDelay,
+		"proxy-ssh":                 true,
 	}
 	for attr, val := range alwaysOptional {
 		if _, ok := d[attr]; !ok {

=== modified file 'environs/config/config_test.go'
--- environs/config/config_test.go	2014-03-14 02:10:35 +0000
+++ environs/config/config_test.go	2014-04-02 06:17:00 +0000
@@ -1026,6 +1026,7 @@
 	attrs["tools-metadata-url"] = ""
 	attrs["tools-url"] = ""
 	attrs["image-stream"] = ""
+	attrs["proxy-ssh"] = false
 
 	// Default firewall mode is instance
 	attrs["firewall-mode"] = string(config.FwInstance)

=== modified file 'provider/local/environprovider.go'
--- provider/local/environprovider.go	2014-03-31 03:36:17 +0000
+++ provider/local/environprovider.go	2014-04-02 06:17:00 +0000
@@ -103,7 +103,13 @@
 	}
 	// If the user has specified no values for any of the three normal
 	// proxies, then look in the environment and set them.
-	attrs := make(map[string]interface{})
+	attrs := map[string]interface{}{
+		// We must not proxy SSH through the API server in a
+		// local provider environment. Besides not being useful,
+		// it may not work; there is no requirement for sshd to
+		// be available on machine-0.
+		"proxy-ssh": false,
+	}
 	setIfNotBlank := func(key, value string) {
 		if value != "" {
 			attrs[key] = value

=== modified file 'provider/local/environprovider_test.go'
--- provider/local/environprovider_test.go	2014-03-19 04:06:58 +0000
+++ provider/local/environprovider_test.go	2014-04-02 06:17:00 +0000
@@ -380,3 +380,19 @@
 		c.Assert(value, gc.Equals, test.expectAUFS)
 	}
 }
+
+func (s *prepareSuite) TestPrepareProxySSH(c *gc.C) {
+	s.PatchValue(local.DetectAptProxies, func() (osenv.ProxySettings, error) {
+		return osenv.ProxySettings{}, nil
+	})
+	basecfg, err := config.New(config.UseDefaults, map[string]interface{}{
+		"type": "local",
+		"name": "test",
+	})
+	provider, err := environs.Provider("local")
+	c.Assert(err, gc.IsNil)
+	env, err := provider.Prepare(coretesting.Context(c), basecfg)
+	c.Assert(err, gc.IsNil)
+	// local provider sets proxy-ssh to false
+	c.Assert(env.Config().ProxySSH(), gc.Equals, false)
+}

=== modified file 'utils/ssh/ssh_gocrypto.go'
--- utils/ssh/ssh_gocrypto.go	2014-02-24 16:21:43 +0000
+++ utils/ssh/ssh_gocrypto.go	2014-04-02 06:17:00 +0000
@@ -7,6 +7,9 @@
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net"
+	"os"
+	"os/exec"
 	"os/user"
 	"strings"
 
@@ -45,17 +48,20 @@
 	}
 	user, host := splitUserHost(host)
 	port := sshDefaultPort
+	var proxyCommand []string
 	if options != nil {
 		if options.port != 0 {
 			port = options.port
 		}
+		proxyCommand = options.proxyCommand
 	}
 	logger.Debugf(`running (equivalent of): ssh "%s@%s" -p %d '%s'`, user, host, port, shellCommand)
 	return &Cmd{impl: &goCryptoCommand{
-		signers: signers,
-		user:    user,
-		addr:    fmt.Sprintf("%s:%d", host, port),
-		command: shellCommand,
+		signers:      signers,
+		user:         user,
+		addr:         fmt.Sprintf("%s:%d", host, port),
+		command:      shellCommand,
+		proxyCommand: proxyCommand,
 	}}
 }
 
@@ -67,19 +73,50 @@
 }
 
 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
+	signers      []ssh.Signer
+	user         string
+	addr         string
+	command      string
+	proxyCommand []string
+	stdin        io.Reader
+	stdout       io.Writer
+	stderr       io.Writer
+	conn         *ssh.ClientConn
+	sess         *ssh.Session
 }
 
 var sshDial = ssh.Dial
 
+var sshDialWithProxy = func(addr string, proxyCommand []string, config *ssh.ClientConfig) (*ssh.ClientConn, error) {
+	if len(proxyCommand) == 0 {
+		return sshDial("tcp", addr, config)
+	}
+	// User has specified a proxy. Create a pipe and
+	// redirect the proxy command's stdin/stdout to it.
+	host, port, err := net.SplitHostPort(addr)
+	if err != nil {
+		host = addr
+	}
+	for i, arg := range proxyCommand {
+		arg = strings.Replace(arg, "%h", host, -1)
+		if port != "" {
+			arg = strings.Replace(arg, "%p", port, -1)
+		}
+		arg = strings.Replace(arg, "%r", config.User, -1)
+		proxyCommand[i] = arg
+	}
+	client, server := net.Pipe()
+	logger.Debugf(`executing proxy command %q`, proxyCommand)
+	cmd := exec.Command(proxyCommand[0], proxyCommand[1:]...)
+	cmd.Stdin = server
+	cmd.Stdout = server
+	cmd.Stderr = os.Stderr
+	if err := cmd.Start(); err != nil {
+		return nil, err
+	}
+	return ssh.Client(client, config)
+}
+
 func (c *goCryptoCommand) ensureSession() (*ssh.Session, error) {
 	if c.sess != nil {
 		return c.sess, nil
@@ -100,7 +137,7 @@
 			ssh.ClientAuthKeyring(keyring{c.signers}),
 		},
 	}
-	conn, err := sshDial("tcp", c.addr, config)
+	conn, err := sshDialWithProxy(c.addr, c.proxyCommand, config)
 	if err != nil {
 		return nil, err
 	}

=== modified file 'utils/ssh/ssh_gocrypto_test.go'
--- utils/ssh/ssh_gocrypto_test.go	2014-03-13 07:54:56 +0000
+++ utils/ssh/ssh_gocrypto_test.go	2014-04-02 06:17:00 +0000
@@ -6,7 +6,11 @@
 import (
 	"encoding/binary"
 	"errors"
+	"fmt"
+	"io/ioutil"
 	"net"
+	"os/exec"
+	"path/filepath"
 	"sync"
 
 	cryptossh "code.google.com/p/go.crypto/ssh"
@@ -148,3 +152,38 @@
 	err = client.Copy([]string{"0.1.2.3:b", c.MkDir()}, nil, nil)
 	c.Assert(err, gc.ErrorMatches, `scp command is not implemented \(OpenSSH scp not available in PATH\)`)
 }
+
+func (s *SSHGoCryptoCommandSuite) TestProxyCommand(c *gc.C) {
+	realNetcat, err := exec.LookPath("nc")
+	if err != nil {
+		c.Skip("skipping test, couldn't find netcat: %v")
+		return
+	}
+	netcat := filepath.Join(c.MkDir(), "nc")
+	err = ioutil.WriteFile(netcat, []byte("#!/bin/sh\necho $0 \"$@\" > $0.args && exec "+realNetcat+" \"$@\""), 0755)
+	c.Assert(err, gc.IsNil)
+
+	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
+	port := server.Addr().(*net.TCPAddr).Port
+	opts.SetProxyCommand(netcat, "-q0", "%h", "%p")
+	opts.SetPort(port)
+	cmd := client.Command("127.0.0.1", testCommand, &opts)
+	server.cfg.PublicKeyCallback = func(conn *cryptossh.ServerConn, user, algo string, pubkey []byte) bool {
+		return true
+	}
+	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)
+	// Ensure the proxy command was executed with the appropriate arguments.
+	data, err := ioutil.ReadFile(netcat + ".args")
+	c.Assert(err, gc.IsNil)
+	c.Assert(string(data), gc.Equals, fmt.Sprintf("%s -q0 127.0.0.1 %v\n", netcat, port))
+}