From 9987a235a5e3165511851098b937f66899709dab Mon Sep 17 00:00:00 2001
From: Alex Dadgar <alex.dadgar@gmail.com>
Date: Wed, 1 Feb 2017 16:25:59 -0800
Subject: [PATCH] Fix race condition with Deriving vault tokens

This PR fixes a race condition in which the client was not locked while
deriving Vault tokens. This allowed the token to be set which would
cause subsequent Vault requests to fail with permission denied because
the incorrect Vault token was being used.

Further this PR makes the unsetting and unlocking of the client atomic
to avoid an even harder to hit race condition (not sure it was ever hit
but was still incorrect).
---
 client/vaultclient/vaultclient.go | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/client/vaultclient/vaultclient.go b/client/vaultclient/vaultclient.go
index e665421d1..701b2d479 100644
--- a/client/vaultclient/vaultclient.go
+++ b/client/vaultclient/vaultclient.go
@@ -224,6 +224,13 @@ func (c *vaultClient) Stop() {
 	close(c.stopCh)
 }
 
+// unlockAndUnset is used to unset the vault token on the client and release the
+// lock. Helper method for deferring a call that does both.
+func (c *vaultClient) unlockAndUnset() {
+	c.client.SetToken("")
+	c.lock.Unlock()
+}
+
 // DeriveToken takes in an allocation and a set of tasks and for each of the
 // task, it derives a vault token from nomad server and unwraps it using vault.
 // The return value is a map containing all the unwrapped tokens indexed by the
@@ -236,6 +243,12 @@ func (c *vaultClient) DeriveToken(alloc *structs.Allocation, taskNames []string)
 		return nil, fmt.Errorf("vault client is not running")
 	}
 
+	c.lock.Lock()
+	defer c.unlockAndUnset()
+
+	// Use the token supplied to interact with vault
+	c.client.SetToken("")
+
 	return c.tokenDeriver(alloc, taskNames, c.client)
 }
 
@@ -253,14 +266,11 @@ func (c *vaultClient) GetConsulACL(token, path string) (*vaultapi.Secret, error)
 	}
 
 	c.lock.Lock()
-	defer c.lock.Unlock()
+	defer c.unlockAndUnset()
 
 	// Use the token supplied to interact with vault
 	c.client.SetToken(token)
 
-	// Reset the token before returning
-	defer c.client.SetToken("")
-
 	// Read the consul ACL token and return the secret directly
 	return c.client.Logical().Read(path)
 }
@@ -377,9 +387,6 @@ func (c *vaultClient) renew(req *vaultClientRenewalRequest) error {
 	var renewalErr error
 	leaseDuration := req.increment
 	if req.isToken {
-		// Reset the token in the API client before returning
-		defer c.client.SetToken("")
-
 		// Set the token in the API client to the one that needs
 		// renewal
 		c.client.SetToken(req.id)
@@ -394,6 +401,9 @@ func (c *vaultClient) renew(req *vaultClientRenewalRequest) error {
 			// Don't set this if renewal fails
 			leaseDuration = renewResp.Auth.LeaseDuration
 		}
+
+		// Reset the token in the API client before returning
+		c.client.SetToken("")
 	} else {
 		// Renew the secret
 		renewResp, err := c.client.Sys().Renew(req.id, req.increment)