From 9e822a2e8fac1046072fffb4fb34582219dfc9f8 Mon Sep 17 00:00:00 2001 From: Alex Dadgar 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 e665421d1b1..701b2d479c0 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)