From 7ffa66d1a536428dcdf16e68c32ff7ca2b83674a Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Thu, 12 Nov 2015 14:39:41 -0600 Subject: [PATCH] ssh: accept private key contents instead of path We've been moving away from config fields expecting file paths that Terraform will load, instead prefering fields that expect file contents, leaning on `file()` to do loading from a path. This helps with consistency and also flexibility - since this makes it easier to shift sensitive files into environment variables. Here we add a little helper package to manage the transitional period for these fields where we support both behaviors. Also included is the first of several fields being shifted over - SSH private keys in provisioner connection config. We're moving to new field names so the behavior is more intuitive, so instead of `key_file` it's `private_key` now. Additional field shifts will be included in follow up PRs so they can be reviewed and discussed individually. --- communicator/ssh/communicator.go | 4 +- communicator/ssh/provisioner.go | 78 +++++----- communicator/ssh/provisioner_test.go | 42 ++++-- helper/pathorcontents/read.go | 40 +++++ helper/pathorcontents/read_test.go | 140 ++++++++++++++++++ .../provisioners/connection.html.markdown | 27 +++- 6 files changed, 281 insertions(+), 50 deletions(-) create mode 100644 helper/pathorcontents/read.go create mode 100644 helper/pathorcontents/read_test.go diff --git a/communicator/ssh/communicator.go b/communicator/ssh/communicator.go index 14f356358..d37d6757b 100644 --- a/communicator/ssh/communicator.go +++ b/communicator/ssh/communicator.go @@ -93,7 +93,7 @@ func (c *Communicator) Connect(o terraform.UIOutput) (err error) { " SSH Agent: %t", c.connInfo.Host, c.connInfo.User, c.connInfo.Password != "", - c.connInfo.KeyFile != "", + c.connInfo.PrivateKey != "", c.connInfo.Agent, )) @@ -107,7 +107,7 @@ func (c *Communicator) Connect(o terraform.UIOutput) (err error) { " SSH Agent: %t", c.connInfo.BastionHost, c.connInfo.BastionUser, c.connInfo.BastionPassword != "", - c.connInfo.BastionKeyFile != "", + c.connInfo.BastionPrivateKey != "", c.connInfo.Agent, )) } diff --git a/communicator/ssh/provisioner.go b/communicator/ssh/provisioner.go index 813db5728..a45c9345f 100644 --- a/communicator/ssh/provisioner.go +++ b/communicator/ssh/provisioner.go @@ -3,14 +3,13 @@ package ssh import ( "encoding/pem" "fmt" - "io/ioutil" "log" "net" "os" "time" + "github.com/hashicorp/terraform/helper/pathorcontents" "github.com/hashicorp/terraform/terraform" - "github.com/mitchellh/go-homedir" "github.com/mitchellh/mapstructure" "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" @@ -37,7 +36,7 @@ const ( type connectionInfo struct { User string Password string - KeyFile string `mapstructure:"key_file"` + PrivateKey string `mapstructure:"private_key"` Host string Port int Agent bool @@ -45,11 +44,15 @@ type connectionInfo struct { ScriptPath string `mapstructure:"script_path"` TimeoutVal time.Duration `mapstructure:"-"` - BastionUser string `mapstructure:"bastion_user"` - BastionPassword string `mapstructure:"bastion_password"` - BastionKeyFile string `mapstructure:"bastion_key_file"` - BastionHost string `mapstructure:"bastion_host"` - BastionPort int `mapstructure:"bastion_port"` + BastionUser string `mapstructure:"bastion_user"` + BastionPassword string `mapstructure:"bastion_password"` + BastionPrivateKey string `mapstructure:"bastion_private_key"` + BastionHost string `mapstructure:"bastion_host"` + BastionPort int `mapstructure:"bastion_port"` + + // Deprecated + KeyFile string `mapstructure:"key_file"` + BastionKeyFile string `mapstructure:"bastion_key_file"` } // parseConnectionInfo is used to convert the ConnInfo of the InstanceState into @@ -92,6 +95,15 @@ func parseConnectionInfo(s *terraform.InstanceState) (*connectionInfo, error) { connInfo.TimeoutVal = DefaultTimeout } + // Load deprecated fields; we can handle either path or contents in + // underlying implementation. + if connInfo.PrivateKey == "" && connInfo.KeyFile != "" { + connInfo.PrivateKey = conninfo.KeyFile + } + if connInfo.BastionPrivateKey == "" && connInfo.BastionKeyFile != "" { + connInfo.BastionPrivateKey = conninfo.BastionKeyFile + } + // Default all bastion config attrs to their non-bastion counterparts if connInfo.BastionHost != "" { if connInfo.BastionUser == "" { @@ -100,8 +112,8 @@ func parseConnectionInfo(s *terraform.InstanceState) (*connectionInfo, error) { if connInfo.BastionPassword == "" { connInfo.BastionPassword = connInfo.Password } - if connInfo.BastionKeyFile == "" { - connInfo.BastionKeyFile = connInfo.KeyFile + if connInfo.BastionPrivateKey == "" { + connInfo.BastionPrivateKey = connInfo.PrivateKey } if connInfo.BastionPort == 0 { connInfo.BastionPort = connInfo.Port @@ -130,10 +142,10 @@ func prepareSSHConfig(connInfo *connectionInfo) (*sshConfig, error) { } sshConf, err := buildSSHClientConfig(sshClientConfigOpts{ - user: connInfo.User, - keyFile: connInfo.KeyFile, - password: connInfo.Password, - sshAgent: sshAgent, + user: connInfo.User, + privateKey: connInfo.PrivateKey, + password: connInfo.Password, + sshAgent: sshAgent, }) if err != nil { return nil, err @@ -142,10 +154,10 @@ func prepareSSHConfig(connInfo *connectionInfo) (*sshConfig, error) { var bastionConf *ssh.ClientConfig if connInfo.BastionHost != "" { bastionConf, err = buildSSHClientConfig(sshClientConfigOpts{ - user: connInfo.BastionUser, - keyFile: connInfo.BastionKeyFile, - password: connInfo.BastionPassword, - sshAgent: sshAgent, + user: connInfo.BastionUser, + privateKey: connInfo.BastionPrivateKey, + password: connInfo.BastionPassword, + sshAgent: sshAgent, }) if err != nil { return nil, err @@ -169,10 +181,10 @@ func prepareSSHConfig(connInfo *connectionInfo) (*sshConfig, error) { } type sshClientConfigOpts struct { - keyFile string - password string - sshAgent *sshAgent - user string + privateKey string + password string + sshAgent *sshAgent + user string } func buildSSHClientConfig(opts sshClientConfigOpts) (*ssh.ClientConfig, error) { @@ -181,7 +193,7 @@ func buildSSHClientConfig(opts sshClientConfigOpts) (*ssh.ClientConfig, error) { } if opts.keyFile != "" { - pubKeyAuth, err := readPublicKeyFromPath(opts.keyFile) + pubKeyAuth, err := readPrivateKey(opts.privateKey) if err != nil { return nil, err } @@ -201,31 +213,27 @@ func buildSSHClientConfig(opts sshClientConfigOpts) (*ssh.ClientConfig, error) { return conf, nil } -func readPublicKeyFromPath(path string) (ssh.AuthMethod, error) { - fullPath, err := homedir.Expand(path) +func readPrivateKey(pk string) (ssh.AuthMethod, error) { + key, _, err := pathorcontents.Read(pk) if err != nil { - return nil, fmt.Errorf("Failed to expand home directory: %s", err) - } - key, err := ioutil.ReadFile(fullPath) - if err != nil { - return nil, fmt.Errorf("Failed to read key file %q: %s", path, err) + return nil, fmt.Errorf("Failed to read private key %q: %s", pk, err) } // We parse the private key on our own first so that we can // show a nicer error if the private key has a password. - block, _ := pem.Decode(key) + block, _ := pem.Decode([]byte(key)) if block == nil { - return nil, fmt.Errorf("Failed to read key %q: no key found", path) + return nil, fmt.Errorf("Failed to read key %q: no key found", pk) } if block.Headers["Proc-Type"] == "4,ENCRYPTED" { return nil, fmt.Errorf( "Failed to read key %q: password protected keys are\n"+ - "not supported. Please decrypt the key prior to use.", path) + "not supported. Please decrypt the key prior to use.", pk) } - signer, err := ssh.ParsePrivateKey(key) + signer, err := ssh.ParsePrivateKey([]byte(key)) if err != nil { - return nil, fmt.Errorf("Failed to parse key file %q: %s", path, err) + return nil, fmt.Errorf("Failed to parse key file %q: %s", pk, err) } return ssh.PublicKeys(signer), nil diff --git a/communicator/ssh/provisioner_test.go b/communicator/ssh/provisioner_test.go index fc6b686fb..aa029dad8 100644 --- a/communicator/ssh/provisioner_test.go +++ b/communicator/ssh/provisioner_test.go @@ -10,13 +10,13 @@ func TestProvisioner_connInfo(t *testing.T) { r := &terraform.InstanceState{ Ephemeral: terraform.EphemeralState{ ConnInfo: map[string]string{ - "type": "ssh", - "user": "root", - "password": "supersecret", - "key_file": "/my/key/file.pem", - "host": "127.0.0.1", - "port": "22", - "timeout": "30s", + "type": "ssh", + "user": "root", + "password": "supersecret", + "private_key": "someprivatekeycontents", + "host": "127.0.0.1", + "port": "22", + "timeout": "30s", "bastion_host": "127.0.1.1", }, @@ -34,7 +34,7 @@ func TestProvisioner_connInfo(t *testing.T) { if conf.Password != "supersecret" { t.Fatalf("bad: %v", conf) } - if conf.KeyFile != "/my/key/file.pem" { + if conf.PrivateKey != "someprivatekeycontents" { t.Fatalf("bad: %v", conf) } if conf.Host != "127.0.0.1" { @@ -61,7 +61,31 @@ func TestProvisioner_connInfo(t *testing.T) { if conf.BastionPassword != "supersecret" { t.Fatalf("bad: %v", conf) } - if conf.BastionKeyFile != "/my/key/file.pem" { + if conf.BastionPrivateKey != "someprivatekeycontents" { + t.Fatalf("bad: %v", conf) + } +} + +func TestProvisioner_connInfoLegacy(t *testing.T) { + r := &terraform.InstanceState{ + Ephemeral: terraform.EphemeralState{ + ConnInfo: map[string]string{ + "type": "ssh", + "key_file": "/my/key/file.pem", + "bastion_host": "127.0.1.1", + }, + }, + } + + conf, err := parseConnectionInfo(r) + if err != nil { + t.Fatalf("err: %v", err) + } + + if conf.PrivateKey != "/my/key/file.pem" { + t.Fatalf("bad: %v", conf) + } + if conf.BastionPrivateKey != "/my/key/file.pem" { t.Fatalf("bad: %v", conf) } } diff --git a/helper/pathorcontents/read.go b/helper/pathorcontents/read.go new file mode 100644 index 000000000..d0d4847da --- /dev/null +++ b/helper/pathorcontents/read.go @@ -0,0 +1,40 @@ +// Helpers for dealing with file paths and their contents +package pathorcontents + +import ( + "io/ioutil" + "os" + + "github.com/mitchellh/go-homedir" +) + +// If the argument is a path, Read loads it and returns the contents, +// otherwise the argument is assumed to be the desired contents and is simply +// returned. +// +// The boolean second return value can be called `wasPath` - it indicates if a +// path was detected and a file loaded. +func Read(poc string) (string, bool, error) { + if len(poc) == 0 { + return poc, false, nil + } + + path := poc + if path[0] == '~' { + var err error + path, err = homedir.Expand(path) + if err != nil { + return path, true, err + } + } + + if _, err := os.Stat(path); err == nil { + contents, err := ioutil.ReadFile(path) + if err != nil { + return string(contents), true, err + } + return string(contents), true, nil + } + + return poc, false, nil +} diff --git a/helper/pathorcontents/read_test.go b/helper/pathorcontents/read_test.go new file mode 100644 index 000000000..8313894ea --- /dev/null +++ b/helper/pathorcontents/read_test.go @@ -0,0 +1,140 @@ +package pathorcontents + +import ( + "io" + "io/ioutil" + "os" + "strings" + "testing" + + "github.com/mitchellh/go-homedir" +) + +func TestRead_Path(t *testing.T) { + isPath := true + f, cleanup := testTempFile(t) + defer cleanup() + + if _, err := io.WriteString(f, "foobar"); err != nil { + t.Fatalf("err: %s", err) + } + f.Close() + + contents, wasPath, err := Read(f.Name()) + + if err != nil { + t.Fatalf("err: %s", err) + } + if wasPath != isPath { + t.Fatalf("expected wasPath: %t, got %t", isPath, wasPath) + } + if contents != "foobar" { + t.Fatalf("expected contents %s, got %s", "foobar", contents) + } +} + +func TestRead_TildePath(t *testing.T) { + isPath := true + home, err := homedir.Dir() + if err != nil { + t.Fatalf("err: %s", err) + } + f, cleanup := testTempFile(t, home) + defer cleanup() + + if _, err := io.WriteString(f, "foobar"); err != nil { + t.Fatalf("err: %s", err) + } + f.Close() + + r := strings.NewReplacer(home, "~") + homePath := r.Replace(f.Name()) + contents, wasPath, err := Read(homePath) + + if err != nil { + t.Fatalf("err: %s", err) + } + if wasPath != isPath { + t.Fatalf("expected wasPath: %t, got %t", isPath, wasPath) + } + if contents != "foobar" { + t.Fatalf("expected contents %s, got %s", "foobar", contents) + } +} + +func TestRead_PathNoPermission(t *testing.T) { + isPath := true + f, cleanup := testTempFile(t) + defer cleanup() + + if _, err := io.WriteString(f, "foobar"); err != nil { + t.Fatalf("err: %s", err) + } + f.Close() + + if err := os.Chmod(f.Name(), 0); err != nil { + t.Fatalf("err: %s", err) + } + + contents, wasPath, err := Read(f.Name()) + + if err == nil { + t.Fatal("Expected error, got none!") + } + if wasPath != isPath { + t.Fatalf("expected wasPath: %t, got %t", isPath, wasPath) + } + if contents != "" { + t.Fatalf("expected contents %s, got %s", "", contents) + } +} + +func TestRead_Contents(t *testing.T) { + isPath := false + input := "hello" + + contents, wasPath, err := Read(input) + + if err != nil { + t.Fatalf("err: %s", err) + } + if wasPath != isPath { + t.Fatalf("expected wasPath: %t, got %t", isPath, wasPath) + } + if contents != input { + t.Fatalf("expected contents %s, got %s", input, contents) + } +} + +func TestRead_TildeContents(t *testing.T) { + isPath := false + input := "~/hello/notafile" + + contents, wasPath, err := Read(input) + + if err != nil { + t.Fatalf("err: %s", err) + } + if wasPath != isPath { + t.Fatalf("expected wasPath: %t, got %t", isPath, wasPath) + } + if contents != input { + t.Fatalf("expected contents %s, got %s", input, contents) + } +} + +// Returns an open tempfile based at baseDir and a function to clean it up. +func testTempFile(t *testing.T, baseDir ...string) (*os.File, func()) { + base := "" + if len(baseDir) == 1 { + base = baseDir[0] + } + f, err := ioutil.TempFile(base, "tf") + if err != nil { + t.Fatalf("err: %s", err) + } + + return f, func() { + os.Remove(f.Name()) + } +} diff --git a/website/source/docs/provisioners/connection.html.markdown b/website/source/docs/provisioners/connection.html.markdown index 6efd73e83..83fa8ebb4 100644 --- a/website/source/docs/provisioners/connection.html.markdown +++ b/website/source/docs/provisioners/connection.html.markdown @@ -68,8 +68,10 @@ provisioner "file" { **Additional arguments only supported by the "ssh" connection type:** -* `key_file` - The SSH key to use for the connection. This takes preference over the - password if provided. +* `private_key` - The contents of an SSH key to use for the connection. These can + be loaded from a file on disk using the [`file()` interpolation + function](/docs/configuration/interpolation.html#file_path_). This takes + preference over the password if provided. * `agent` - Set to false to disable using ssh-agent to authenticate. @@ -99,5 +101,22 @@ The `ssh` connection additionally supports the following fields to facilitate a * `bastion_password` - The password we should use for the bastion host. Defaults to the value of `password`. -* `bastion_key_file` - The SSH key to use for the bastion host. Defaults to the - value of `key_file`. +* `bastion_private_key` - The contents of an SSH key file to use for the bastion + host. These can be loaded from a file on disk using the [`file()` + interpolation function](/docs/configuration/interpolation.html#file_path_). + Defaults to the value of `private_key`. + +## Deprecations + +These are supported for backwards compatibility and may be removed in a +future version: + +* `key_file` - A path to or the contents of an SSH key to use for the + connection. These can be loaded from a file on disk using the [`file()` + interpolation function](/docs/configuration/interpolation.html#file_path_). + This takes preference over the password if provided. + +* `bastion_key_file` - The contents of an SSH key file to use for the bastion + host. These can be loaded from a file on disk using the [`file()` + interpolation function](/docs/configuration/interpolation.html#file_path_). + Defaults to the value of `key_file`.