vault: fix namespace reset for clients with unset namespace

.changelog/23491.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+vault: Fixed a bug where requests to derive or renew tokens could be sent to the wrong namespace
+```

client/vaultclient/vaultclient.go

@@ -15,7 +15,6 @@ import (
 
 	metrics "github.com/armon/go-metrics"
 	hclog "github.com/hashicorp/go-hclog"
-
 	"github.com/hashicorp/nomad/helper/useragent"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/nomad/structs/config"
@@ -65,9 +64,6 @@ type VaultClient interface {
 	// endpoint, along with whether or not the token is renewable.
 	DeriveTokenWithJWT(context.Context, JWTLoginRequest) (string, bool, error)
 
-	// GetConsulACL fetches the Consul ACL token required for the task
-	GetConsulACL(string, string) (*vaultapi.Secret, error)
-
 	// RenewToken renews a token with the given increment and adds it to
 	// the min-heap for periodic renewal.
 	RenewToken(string, int) (<-chan error, error)
@@ -250,13 +246,11 @@ func (c *vaultClient) Stop() {
 }
 
 // unlockAndUnset is used to unset the vault token on the client, restore the
-// client's namespace, and release the lock. Helper method for deferring a call
-// that does both.
-func (c *vaultClient) unlockAndUnset(previousNs string) {
+// client's default configured namespace, and release the lock. Helper method
+// for deferring a call that does both.
+func (c *vaultClient) unlockAndUnset() {
 	c.client.SetToken("")
-	if previousNs != "" {
-		c.client.SetNamespace(previousNs)
-	}
+	c.client.SetNamespace(c.config.Namespace)
 	c.lock.Unlock()
 }
 
@@ -273,7 +267,7 @@ func (c *vaultClient) DeriveToken(alloc *structs.Allocation, taskNames []string)
 	}
 
 	c.lock.Lock()
-	defer c.unlockAndUnset(c.client.Namespace())
+	defer c.unlockAndUnset()
 
 	// Use the token supplied to interact with vault
 	c.client.SetToken("")
@@ -297,7 +291,7 @@ func (c *vaultClient) DeriveTokenWithJWT(ctx context.Context, req JWTLoginReques
 	}
 
 	c.lock.Lock()
-	defer c.unlockAndUnset(c.client.Namespace())
+	defer c.unlockAndUnset()
 
 	// Make sure the login request is not passing any token and that we're using
 	// the expected namespace to login
@@ -330,29 +324,6 @@ func (c *vaultClient) DeriveTokenWithJWT(ctx context.Context, req JWTLoginReques
 	return s.Auth.ClientToken, s.Auth.Renewable, nil
 }
 
-// GetConsulACL creates a vault API client and reads from vault a consul ACL
-// token used by the task.
-func (c *vaultClient) GetConsulACL(token, path string) (*vaultapi.Secret, error) {
-	if !c.config.IsEnabled() {
-		return nil, fmt.Errorf("vault client not enabled")
-	}
-	if token == "" {
-		return nil, fmt.Errorf("missing token")
-	}
-	if path == "" {
-		return nil, fmt.Errorf("missing consul ACL token vault path")
-	}
-
-	c.lock.Lock()
-	defer c.unlockAndUnset(c.client.Namespace())
-
-	// Use the token supplied to interact with vault
-	c.client.SetToken(token)
-
-	// Read the consul ACL token and return the secret directly
-	return c.client.Logical().Read(path)
-}
-
 // RenewToken renews the supplied token for a given duration (in seconds) and
 // adds it to the min-heap so that it is renewed periodically by the renewal
 // loop. Any error returned during renewal will be written to a buffered

client/vaultclient/vaultclient_test.go

@@ -677,3 +677,33 @@ func TestVaultClient_RenewalConcurrent(t *testing.T) {
 		}
 	}
 }
+
+func TestVaultClient_NamespaceReset(t *testing.T) {
+
+	// Mock Vault API that always returns an error
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+		fmt.Fprintln(w, "error")
+	}))
+	defer ts.Close()
+
+	conf := structsc.DefaultVaultConfig()
+	conf.Addr = ts.URL
+	conf.Enabled = pointer.Of(true)
+
+	for _, ns := range []string{"", "foo"} {
+		conf.Namespace = ns
+
+		vc, err := NewVaultClient(conf, testlog.HCLogger(t), nil)
+		must.NoError(t, err)
+		vc.Start()
+
+		_, _, err = vc.DeriveTokenWithJWT(context.Background(), JWTLoginRequest{
+			JWT:       "bogus",
+			Namespace: "bar",
+		})
+		must.Error(t, err)
+		must.Eq(t, ns, vc.client.Namespace(),
+			must.Sprintf("expected %q, not %q", ns, vc.client.Namespace()))
+	}
+}

client/vaultclient/vaultclient_testing.go

@@ -10,7 +10,6 @@ import (
 
 	"github.com/hashicorp/nomad/helper/uuid"
 	"github.com/hashicorp/nomad/nomad/structs"
-	vaultapi "github.com/hashicorp/vault/api"
 )
 
 // MockVaultClient is used for testing the vaultclient integration and is safe
@@ -162,8 +161,6 @@ func (vc *MockVaultClient) SetRenewable(renewable bool) {
 	vc.renewable = renewable
 }
 
-func (vc *MockVaultClient) GetConsulACL(string, string) (*vaultapi.Secret, error) { return nil, nil }
-
 // LegacyTokens returns the tokens generated using the legacy flow.
 func (vc *MockVaultClient) LegacyTokens() map[string]string {
 	vc.mu.Lock()