From 96c20f0dd7108ab9599e680220d77572dc439be5 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Wed, 29 Jun 2016 08:58:33 -0500 Subject: [PATCH] communicator/{ssh,winrm}: seed random script paths Without a seed, the "random" script path locations for the remote-exec provisioner were actually deterministic! Every rand.Int31() would return the same pseudorandom chain starting w/ the numbers: 1298498081, 2019727887, 1427131847, 939984059, ... So here we properly seed the communicators so the script paths are actually random, and multiple runs on a single remote host have much less chance of clobbering each other. Fixes #4186 Kudos to @DustinChaloupka for the correct hunch leading to this fix! --- communicator/ssh/communicator.go | 5 ++++- communicator/ssh/communicator_test.go | 27 ++++++++++++++++++++++++- communicator/winrm/communicator.go | 5 ++++- communicator/winrm/communicator_test.go | 27 ++++++++++++++++++++++++- 4 files changed, 60 insertions(+), 4 deletions(-) diff --git a/communicator/ssh/communicator.go b/communicator/ssh/communicator.go index d37d6757b..0b3a222b3 100644 --- a/communicator/ssh/communicator.go +++ b/communicator/ssh/communicator.go @@ -34,6 +34,7 @@ type Communicator struct { config *sshConfig conn net.Conn address string + rand *rand.Rand } type sshConfig struct { @@ -68,6 +69,8 @@ func New(s *terraform.InstanceState) (*Communicator, error) { comm := &Communicator{ connInfo: connInfo, config: config, + // Seed our own rand source so that script paths are not deterministic + rand: rand.New(rand.NewSource(time.Now().UnixNano())), } return comm, nil @@ -185,7 +188,7 @@ func (c *Communicator) Timeout() time.Duration { func (c *Communicator) ScriptPath() string { return strings.Replace( c.connInfo.ScriptPath, "%RAND%", - strconv.FormatInt(int64(rand.Int31()), 10), -1) + strconv.FormatInt(int64(c.rand.Int31()), 10), -1) } // Start implementation of communicator.Communicator interface diff --git a/communicator/ssh/communicator_test.go b/communicator/ssh/communicator_test.go index 28755b6db..529dc49f0 100644 --- a/communicator/ssh/communicator_test.go +++ b/communicator/ssh/communicator_test.go @@ -225,7 +225,18 @@ func TestScriptPath(t *testing.T) { } for _, tc := range cases { - comm := &Communicator{connInfo: &connectionInfo{ScriptPath: tc.Input}} + r := &terraform.InstanceState{ + Ephemeral: terraform.EphemeralState{ + ConnInfo: map[string]string{ + "type": "winrm", + "script_path": tc.Input, + }, + }, + } + comm, err := New(r) + if err != nil { + t.Fatalf("err: %s", err) + } output := comm.ScriptPath() match, err := regexp.Match(tc.Pattern, []byte(output)) @@ -238,6 +249,20 @@ func TestScriptPath(t *testing.T) { } } +func TestScriptPath_randSeed(t *testing.T) { + // Pre GH-4186 fix, this value was the deterministic start the pseudorandom + // chain of unseeded math/rand values for Int31(). + staticSeedPath := "/tmp/terraform_1298498081.sh" + c, err := New(&terraform.InstanceState{}) + if err != nil { + t.Fatalf("err: %s", err) + } + path := c.ScriptPath() + if path == staticSeedPath { + t.Fatalf("rand not seeded! got: %s", path) + } +} + const testClientPrivateKey = `-----BEGIN RSA PRIVATE KEY----- MIIEpQIBAAKCAQEAxOgNXOJ/jrRDxBZTSk2X9otNy9zcpUmJr5ifDi5sy7j2ZiQS beBt1Wf+tLNWis8Cyq06ttEvjjRuM75yucyD6GrqDTXVCSm4PeOIQeDhPhw26wYZ diff --git a/communicator/winrm/communicator.go b/communicator/winrm/communicator.go index 177efdcb9..c25c2bc63 100644 --- a/communicator/winrm/communicator.go +++ b/communicator/winrm/communicator.go @@ -24,6 +24,7 @@ type Communicator struct { connInfo *connectionInfo client *winrm.Client endpoint *winrm.Endpoint + rand *rand.Rand } // New creates a new communicator implementation over WinRM. @@ -44,6 +45,8 @@ func New(s *terraform.InstanceState) (*Communicator, error) { comm := &Communicator{ connInfo: connInfo, endpoint: endpoint, + // Seed our own rand source so that script paths are not deterministic + rand: rand.New(rand.NewSource(time.Now().UnixNano())), } return comm, nil @@ -121,7 +124,7 @@ func (c *Communicator) Timeout() time.Duration { func (c *Communicator) ScriptPath() string { return strings.Replace( c.connInfo.ScriptPath, "%RAND%", - strconv.FormatInt(int64(rand.Int31()), 10), -1) + strconv.FormatInt(int64(c.rand.Int31()), 10), -1) } // Start implementation of communicator.Communicator interface diff --git a/communicator/winrm/communicator_test.go b/communicator/winrm/communicator_test.go index cf3a94ee8..d903fbc2e 100644 --- a/communicator/winrm/communicator_test.go +++ b/communicator/winrm/communicator_test.go @@ -131,7 +131,18 @@ func TestScriptPath(t *testing.T) { } for _, tc := range cases { - comm := &Communicator{connInfo: &connectionInfo{ScriptPath: tc.Input}} + r := &terraform.InstanceState{ + Ephemeral: terraform.EphemeralState{ + ConnInfo: map[string]string{ + "type": "winrm", + "script_path": tc.Input, + }, + }, + } + comm, err := New(r) + if err != nil { + t.Fatalf("err: %s", err) + } output := comm.ScriptPath() match, err := regexp.Match(tc.Pattern, []byte(output)) @@ -143,3 +154,17 @@ func TestScriptPath(t *testing.T) { } } } + +func TestScriptPath_randSeed(t *testing.T) { + // Pre GH-4186 fix, this value was the deterministic start the pseudorandom + // chain of unseeded math/rand values for Int31(). + staticSeedPath := "C:/Temp/terraform_1298498081.cmd" + c, err := New(&terraform.InstanceState{}) + if err != nil { + t.Fatalf("err: %s", err) + } + path := c.ScriptPath() + if path == staticSeedPath { + t.Fatalf("rand not seeded! got: %s", path) + } +}