communicator/ssh: sort agent after static keyfile

In the SSH client configuration, we had SSH Agent authentication listed
before the static PrivateKey loaded from the `key_file` setting.
Switching the default of the `agent` setting exposed the fact that the
SSH agent overrides the `key_file` during the handshake. By listing the
`key_file` first, we catch the provided key before any query goes out to
the agent.

Adds a key-based authentication SSH test to cover this new behavior. It
fails without the reordering on any machine with an SSH agent running.

Fixes #2614

communicator/ssh/communicator_test.go

@@ -5,7 +5,9 @@ package ssh
 import (
 	"bytes"
 	"fmt"
+	"io/ioutil"
 	"net"
+	"os"
 	"regexp"
 	"strings"
 	"testing"
@@ -45,12 +47,8 @@ NsZoFj52ponUM6+99A2CmezFCN16c4mbA//luWF+k3VVqR6BpkrhKw==
 -----END RSA PRIVATE KEY-----`
 
 var serverConfig = &ssh.ServerConfig{
-	PasswordCallback: func(c ssh.ConnMetadata, pass []byte) (*ssh.Permissions, error) {
+	PasswordCallback:  acceptUserPass("user", "pass"),
-		if c.User() == "user" && string(pass) == "pass" {
+	PublicKeyCallback: acceptPublicKey(testClientPublicKey),
-			return nil, nil
-		}
-		return nil, fmt.Errorf("password rejected for %q", c.User())
-	},
 }
 
 func init() {
@@ -169,6 +167,48 @@ func TestStart(t *testing.T) {
 	}
 }
 
+func TestStart_KeyFile(t *testing.T) {
+	address := newMockLineServer(t)
+	parts := strings.Split(address, ":")
+
+	keyFile, err := ioutil.TempFile("", "tf")
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+	keyFilePath := keyFile.Name()
+	keyFile.Write([]byte(testClientPrivateKey))
+	keyFile.Close()
+	defer os.Remove(keyFilePath)
+
+	r := &terraform.InstanceState{
+		Ephemeral: terraform.EphemeralState{
+			ConnInfo: map[string]string{
+				"type":     "ssh",
+				"user":     "user",
+				"key_file": keyFilePath,
+				"host":     parts[0],
+				"port":     parts[1],
+				"timeout":  "30s",
+			},
+		},
+	}
+
+	c, err := New(r)
+	if err != nil {
+		t.Fatalf("error creating communicator: %s", err)
+	}
+
+	var cmd remote.Cmd
+	stdout := new(bytes.Buffer)
+	cmd.Command = "echo foo"
+	cmd.Stdout = stdout
+
+	err = c.Start(&cmd)
+	if err != nil {
+		t.Fatalf("error executing remote command: %s", err)
+	}
+}
+
 func TestScriptPath(t *testing.T) {
 	cases := []struct {
 		Input   string
@@ -197,3 +237,56 @@ func TestScriptPath(t *testing.T) {
 		}
 	}
 }
+
+const testClientPrivateKey = `-----BEGIN RSA PRIVATE KEY-----
+MIIEpQIBAAKCAQEAxOgNXOJ/jrRDxBZTSk2X9otNy9zcpUmJr5ifDi5sy7j2ZiQS
+beBt1Wf+tLNWis8Cyq06ttEvjjRuM75yucyD6GrqDTXVCSm4PeOIQeDhPhw26wYZ
+O0h/mFgrAiCwaEl8AFXVBInOhVn/0nqgUpkwckh76bjTsNeifkiugK3cfJOuBdrU
+ZGbgugJjmYo4Cmv7ECo1gCFT5N+BAjOji3z3N5ClBH5HaWC77jH7kTH0k5cZ+ZRQ
+tG9EqLyvWlnzTAR/Yly0JInkOa16Ms1Au5sIJwEoJfHKsNVK06IlLob53nblwYW0
+H5gv1Kb/rS+nUkpPtA5YFShB7iZnPLPPv6qXSwIDAQABAoIBAC0UY1rMkB9/rbQK
+2G6+bPgI1HrDydAdkeQdsOxyPH43jlG8GGwHYZ3l/S4pkLqewijcmACay6Rm5IP8
+Kg/XfquLLqJvnKJIZuHkYaGTdn3dv8T21Hf6FRwvs0j9auW1TSpWfDpZwmpNPIBX
+irTeVXUUmynbIrvt4km/IhRbuYrbbb964CLYD1DCl3XssXxoRNvPpc5EtOuyDorA
+5g1hvZR1FqbOAmOuNQMYJociMuWB8mCaHb+o1Sg4A65OLXxoKs0cuwInJ/n/R4Z3
++GrV+x5ypBMxXgjjQtKMLEOujkvxs1cp34hkbhKMHHXxbMu5jl74YtGGsLLk90rq
+ieZGIgECgYEA49OM9mMCrDoFUTZdJaSARA/MOXkdQgrqVTv9kUHee7oeMZZ6lS0i
+bPU7g+Bq+UAN0qcw9x992eAElKjBA71Q5UbZYWh29BDMZd8bRJmwz4P6aSMoYLWI
+Sr31caJU9LdmPFatarNeehjSJtlTuoZD9+NElnnUwNaTeOOo5UdhTQsCgYEA3UGm
+QWoDUttFwK9oL2KL8M54Bx6EzNhnyk03WrqBbR7PJcPKnsF0R/0soQ+y0FW0r8RJ
+TqG6ze5fUJII72B4GlMTQdP+BIvaKQttwWQTNIjbbv4NksF445gdVOO1xi9SvQ7k
+uvMVxOb+1jL3HAFa3furWu2tJRDs6dhuaILLxsECgYEAhnhlKUBDYZhVbxvhWsh/
+lKymY/3ikQqUSX7BKa1xPiIalDY3YDllql4MpMgfG8L85asdMZ96ztB0o7H/Ss/B
+IbLxt5bLLz+DBVXsaE82lyVU9h10RbCgI01/w3SHJHHjfBXFAcehKfvgfmGkE+IP
+2A5ie1aphrCgFqh5FetNuQUCgYEAibL42I804FUtFR1VduAa/dRRqQSaW6528dWa
+lLGsKRBalUNEEAeP6dmr89UEUVp1qEo94V0QGGe5FDi+rNPaC3AWdQqNdaDgNlkx
+hoFU3oYqIuqj4ejc5rBd2N4a2+vJz3W8bokozDGC+iYf2mMRfUPKwj1XW9Er0OFs
+3UhBsEECgYEAto/iJB7ZlCM7EyV9JW0tsEt83rbKMQ/Ex0ShbBIejej0Xx7bwx60
+tVgay+bzJnNkXu6J4XVI98A/WsdI2kW4hL0STYdHV5HVA1l87V4ZbvTF2Bx8a8RJ
+OF3UjpMTWKqOprw9nAu5VuwNRVzORF8ER8rgGeaR2/gsSvIYFy9VXq8=
+-----END RSA PRIVATE KEY-----`
+
+var testClientPublicKey = `ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDE6A1c4n+OtEPEFlNKTZf2i03L3NylSYmvmJ8OLmzLuPZmJBJt4G3VZ/60s1aKzwLKrTq20S+ONG4zvnK5zIPoauoNNdUJKbg944hB4OE+HDbrBhk7SH+YWCsCILBoSXwAVdUEic6FWf/SeqBSmTBySHvpuNOw16J+SK6Ardx8k64F2tRkZuC6AmOZijgKa/sQKjWAIVPk34ECM6OLfPc3kKUEfkdpYLvuMfuRMfSTlxn5lFC0b0SovK9aWfNMBH9iXLQkieQ5rXoyzUC7mwgnASgl8cqw1UrToiUuhvneduXBhbQfmC/Upv+tL6dSSk+0DlgVKEHuJmc8s8+/qpdL`
+
+func acceptUserPass(goodUser, goodPass string) func(ssh.ConnMetadata, []byte) (*ssh.Permissions, error) {
+	return func(c ssh.ConnMetadata, pass []byte) (*ssh.Permissions, error) {
+		if c.User() == goodUser && string(pass) == goodPass {
+			return nil, nil
+		}
+		return nil, fmt.Errorf("password rejected for %q", c.User())
+	}
+}
+
+func acceptPublicKey(keystr string) func(ssh.ConnMetadata, ssh.PublicKey) (*ssh.Permissions, error) {
+	goodkey, _, _, _, err := ssh.ParseAuthorizedKey([]byte(keystr))
+	if err != nil {
+		panic(fmt.Errorf("error parsing key: %s", err))
+	}
+	return func(_ ssh.ConnMetadata, inkey ssh.PublicKey) (*ssh.Permissions, error) {
+		if bytes.Compare(inkey.Marshal(), goodkey.Marshal()) == 0 {
+			return nil, nil
+		}
+
+		return nil, fmt.Errorf("public key rejected")
+	}
+}

communicator/ssh/provisioner.go

@@ -180,10 +180,6 @@ func buildSSHClientConfig(opts sshClientConfigOpts) (*ssh.ClientConfig, error) {
 		User: opts.user,
 	}
 
-	if opts.sshAgent != nil {
-		conf.Auth = append(conf.Auth, opts.sshAgent.Auth())
-	}
-
 	if opts.keyFile != "" {
 		pubKeyAuth, err := readPublicKeyFromPath(opts.keyFile)
 		if err != nil {
@@ -198,6 +194,10 @@ func buildSSHClientConfig(opts sshClientConfigOpts) (*ssh.ClientConfig, error) {
 			PasswordKeyboardInteractive(opts.password)))
 	}
 
+	if opts.sshAgent != nil {
+		conf.Auth = append(conf.Auth, opts.sshAgent.Auth())
+	}
+
 	return conf, nil
 }