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!
hashicorp · Jun 29, 2016 · 96c20f0 · 96c20f0
1 parent ebb9b7a
commit 96c20f0
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 4 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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)
+	}
+}