bug(vault): Correctly handle credential stores with expired tokens

Boundary makes use of a database view to perform CRUD operations on Vault credential stores and libraries. This view did not include credential stores with tokens that have expired in Vault, causing errors when attempting to perform any action against them.
hashicorp · Aug 27, 2022 · 9ec7ec1 · 9ec7ec1
1 parent 547e7a5
commit 9ec7ec1
Show file tree

Hide file tree

Showing 16 changed files with 496 additions and 32 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,8 +5,13 @@ Canonical reference for changes, improvements, and bugfixes for Boundary.
 ## Next
 
 ### Bug Fixes
-* Sessions: Fix an issue where sessions could not have more than one connection 
-([Issue](https://github.com/hashicorp/boundary/issues/2362)), ([PR](https://github.com/hashicorp/boundary/pull/2369))
+
+* Vault: Correctly handle Vault credential stores and libraries that are linked to an
+  expired Vault token. ([Issue](https://github.com/hashicorp/boundary/issues/2179),
+  [PR](https://github.com/hashicorp/boundary/pull/2399)).
+* Sessions: Fix an issue where sessions could not have more than one connection
+  ([Issue](https://github.com/hashicorp/boundary/issues/2362),
+  [PR](https://github.com/hashicorp/boundary/pull/2369)).
 
 ## 0.10.2 (2022/08/23)
 

diff --git a/api/credentialstores/vault_credential_store_attributes.gen.go b/api/credentialstores/vault_credential_store_attributes.gen.go
diff --git a/internal/cmd/commands/credentialstorescmd/funcs.go b/internal/cmd/commands/credentialstorescmd/funcs.go
@@ -205,6 +205,7 @@ var keySubstMap = map[string]string{
 	"tls_server_name":             "TLS Server Name",
 	"tls_skip_verify":             "Skip TLS Verification",
 	"token_hmac":                  "Token HMAC",
+	"token_status":                "Token Status",
 	"client_certificate":          "Client Certificate",
 	"client_certificate_key_hmac": "Client Certificate Key HMAC",
 	"worker_filter":               "Worker Filter",

diff --git a/internal/credential/vault/jobs.go b/internal/credential/vault/jobs.go
@@ -139,7 +139,7 @@ func (r *TokenRenewalJob) Run(ctx context.Context) error {
 		return errors.Wrap(ctx, err, op)
 	}
 
-	var ps []*privateStore
+	var ps []*privateActiveStore
 	// Fetch all tokens that will reach their renewal point within the renewalWindow.
 	// This is done to avoid constantly scheduling the token renewal job when there are multiple tokens
 	// set to renew in sequence.
@@ -151,7 +151,8 @@ func (r *TokenRenewalJob) Run(ctx context.Context) error {
 	// Set numProcessed and numTokens for status report
 	r.numProcessed, r.numTokens = 0, len(ps)
 
-	for _, s := range ps {
+	for _, as := range ps {
+		s := as.Store
 		// Verify context is not done before renewing next token
 		if err := ctx.Err(); err != nil {
 			return errors.Wrap(ctx, err, op)
@@ -373,15 +374,16 @@ or
 ))
 `
 
-	var ps []*privateStore
+	var ps []*privateActiveStore
 	err := r.reader.SearchWhere(ctx, &ps, where, nil, db.WithLimit(r.limit))
 	if err != nil {
 		return errors.Wrap(ctx, err, op)
 	}
 
 	// Set numProcessed and numTokens for s report
 	r.numProcessed, r.numTokens = 0, len(ps)
-	for _, s := range ps {
+	for _, as := range ps {
+		s := as.Store
 		// Verify context is not done before renewing next token
 		if err := ctx.Err(); err != nil {
 			return errors.Wrap(ctx, err, op)

diff --git a/internal/credential/vault/private_store.go b/internal/credential/vault/private_store.go
@@ -21,22 +21,25 @@ func (r *Repository) listRevokePrivateStores(ctx context.Context, opt ...Option)
 		limit = opts.withLimit
 	}
 
-	var stores []*privateStore
+	var stores []*privateActiveStore
 	where, values := "token_status = ?", []interface{}{"revoke"}
 	if err := r.reader.SearchWhere(ctx, &stores, where, values, db.WithLimit(limit)); err != nil {
 		return nil, errors.Wrap(ctx, err, op)
 	}
 
+	var returnPrivStores []*privateStore
 	for _, store := range stores {
-		databaseWrapper, err := r.kms.GetWrapper(ctx, store.ProjectId, kms.KeyPurposeDatabase)
+		privStore := store.Store
+		databaseWrapper, err := r.kms.GetWrapper(ctx, privStore.ProjectId, kms.KeyPurposeDatabase)
 		if err != nil {
 			return nil, errors.Wrap(ctx, err, op, errors.WithMsg("unable to get database wrapper"))
 		}
-		if err := store.decrypt(ctx, databaseWrapper); err != nil {
+		if err := privStore.decrypt(ctx, databaseWrapper); err != nil {
 			return nil, errors.Wrap(ctx, err, op)
 		}
+		returnPrivStores = append(returnPrivStores, privStore)
 	}
-	return stores, nil
+	return returnPrivStores, nil
 }
 
 func (r *Repository) lookupPrivateStore(ctx context.Context, publicId string) (*privateStore, error) {
@@ -45,7 +48,7 @@ func (r *Repository) lookupPrivateStore(ctx context.Context, publicId string) (*
 		return nil, errors.New(ctx, errors.InvalidParameter, op, "no public id")
 	}
 	ps := allocPrivateStore()
-	if err := r.reader.LookupWhere(ctx, &ps, "public_id = ? and token_status = ?", []interface{}{publicId, CurrentToken}); err != nil {
+	if err := r.reader.LookupWhere(ctx, &ps, "public_id = ?", []interface{}{publicId}); err != nil {
 		if errors.IsNotFoundError(err) {
 			return nil, nil
 		}
@@ -135,13 +138,13 @@ func (ps *privateStore) token() *Token {
 		tk := allocToken()
 		tk.StoreId = ps.StoreId
 		tk.TokenHmac = ps.TokenHmac
+		tk.Status = ps.TokenStatus
 		tk.LastRenewalTime = ps.TokenLastRenewalTime
 		tk.ExpirationTime = ps.TokenExpirationTime
 		tk.CreateTime = ps.TokenCreateTime
 		tk.UpdateTime = ps.TokenUpdateTime
 		tk.CtToken = ps.CtToken
 		tk.KeyId = ps.TokenKeyId
-		tk.Status = ps.TokenStatus
 
 		return tk
 	}
@@ -212,3 +215,12 @@ func (ps *privateStore) GetPublicId() string { return ps.PublicId }
 func (ps *privateStore) TableName() string {
 	return "credential_vault_store_private"
 }
+
+type privateActiveStore struct {
+	Store *privateStore `gorm:"embedded"`
+}
+
+// TableName returns the table name for gorm.
+func (ps *privateActiveStore) TableName() string {
+	return "credential_vault_store_private_active"
+}
diff --git a/internal/credential/vault/repository_credential_library_test.go b/internal/credential/vault/repository_credential_library_test.go
@@ -1621,6 +1621,47 @@ func TestRepository_UpdateCredentialLibrary(t *testing.T) {
 		assert.NoError(db.TestVerifyOplog(t, rw, got2.GetPublicId(), db.WithOperation(oplog.OpType_OP_TYPE_UPDATE), db.WithCreateNotBefore(10*time.Second)))
 	})
 
+	t.Run("valid-update-with-expired-store-token", func(t *testing.T) {
+		assert, require := assert.New(t), require.New(t)
+		ctx := context.Background()
+		kms := kms.TestKms(t, conn, wrapper)
+		sche := scheduler.TestScheduler(t, conn, wrapper)
+		repo, err := NewRepository(rw, rw, kms, sche)
+		assert.NoError(err)
+		require.NotNil(repo)
+
+		_, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper))
+		css := TestCredentialStores(t, conn, wrapper, prj.GetPublicId(), 1)
+		cs := css[0]
+
+		in := &CredentialLibrary{
+			CredentialLibrary: &store.CredentialLibrary{
+				HttpMethod: "GET",
+				VaultPath:  "/some/path",
+				Name:       "test-name-repo",
+			},
+		}
+
+		in.StoreId = cs.GetPublicId()
+		got, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), in)
+		assert.NoError(err)
+		require.NotNil(got)
+
+		// Expire the credential store Vault token
+		rows, err := rw.Exec(context.Background(),
+			"update credential_vault_token set status = ? where token_hmac = ?",
+			[]interface{}{ExpiredToken, cs.Token().TokenHmac})
+		require.NoError(err)
+		require.Equal(1, rows)
+
+		got.Name = "new-name"
+		updated, gotCount, err := repo.UpdateCredentialLibrary(ctx, prj.GetPublicId(), got, 1, []string{"name"})
+		assert.NoError(err)
+		require.NotNil(updated)
+		assert.Equal("new-name", updated.Name)
+		assert.Equal(1, gotCount)
+	})
+
 	t.Run("change-project-id", func(t *testing.T) {
 		assert, require := assert.New(t), require.New(t)
 		ctx := context.Background()
@@ -1662,7 +1703,15 @@ func TestRepository_LookupCredentialLibrary(t *testing.T) {
 
 	{
 		_, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper))
-		cs := TestCredentialStores(t, conn, wrapper, prj.GetPublicId(), 1)[0]
+		css := TestCredentialStores(t, conn, wrapper, prj.GetPublicId(), 2)
+
+		cs := css[0]
+		csWithExpiredToken := css[1]
+		rows, err := rw.Exec(context.Background(),
+			"update credential_vault_token set status = ? where token_hmac = ?",
+			[]interface{}{ExpiredToken, csWithExpiredToken.Token().TokenHmac})
+		require.NoError(t, err)
+		require.Equal(t, 1, rows)
 
 		tests := []struct {
 			name string
@@ -1678,6 +1727,17 @@ func TestRepository_LookupCredentialLibrary(t *testing.T) {
 					},
 				},
 			},
+
+			{
+				name: "valid-with-expired-cred-store-token",
+				in: &CredentialLibrary{
+					CredentialLibrary: &store.CredentialLibrary{
+						StoreId:    csWithExpiredToken.GetPublicId(),
+						HttpMethod: "GET",
+						VaultPath:  "/some/path",
+					},
+				},
+			},
 			{
 				name: "valid-username-password-credential-type",
 				in: &CredentialLibrary{

diff --git a/internal/credential/vault/repository_credential_store.go b/internal/credential/vault/repository_credential_store.go
@@ -279,6 +279,7 @@ type publicStore struct {
 	TokenUpdateTime      *timestamp.Timestamp
 	TokenLastRenewalTime *timestamp.Timestamp
 	TokenExpirationTime  *timestamp.Timestamp
+	TokenStatus          string
 	ClientCert           []byte
 	ClientCertKeyHmac    []byte
 }
@@ -306,6 +307,7 @@ func (ps *publicStore) toCredentialStore() *CredentialStore {
 	if ps.TokenHmac != nil {
 		tk := allocToken()
 		tk.TokenHmac = ps.TokenHmac
+		tk.Status = ps.TokenStatus
 		tk.LastRenewalTime = ps.TokenLastRenewalTime
 		tk.ExpirationTime = ps.TokenExpirationTime
 		tk.CreateTime = ps.TokenCreateTime

diff --git a/internal/credential/vault/repository_credential_store_test.go b/internal/credential/vault/repository_credential_store_test.go
@@ -235,16 +235,23 @@ func TestRepository_LookupCredentialStore(t *testing.T) {
 	sche := scheduler.TestScheduler(t, conn, wrapper)
 
 	_, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper))
-	stores := TestCredentialStores(t, conn, wrapper, prj.PublicId, 2)
+	stores := TestCredentialStores(t, conn, wrapper, prj.PublicId, 3)
 	csWithClientCert := stores[0]
 	csWithoutClientCert := stores[1]
+	csWithExpiredToken := stores[2]
 
 	ccert := allocClientCertificate()
 	ccert.StoreId = csWithoutClientCert.GetPublicId()
 	rows, err := rw.Delete(context.Background(), ccert, db.WithWhere("store_id = ?", csWithoutClientCert.GetPublicId()))
 	require.NoError(t, err)
 	require.Equal(t, 1, rows)
 
+	rows, err = rw.Exec(context.Background(),
+		"update credential_vault_token set status = ? where token_hmac = ?",
+		[]interface{}{ExpiredToken, csWithExpiredToken.Token().TokenHmac})
+	require.NoError(t, err)
+	require.Equal(t, 1, rows)
+
 	badId, err := newCredentialStoreId()
 	assert.NoError(t, err)
 	require.NotNil(t, badId)
@@ -262,6 +269,12 @@ func TestRepository_LookupCredentialStore(t *testing.T) {
 			want:           csWithClientCert,
 			wantClientCert: true,
 		},
+		{
+			name:           "valid-with-expired-token",
+			id:             csWithExpiredToken.GetPublicId(),
+			want:           csWithExpiredToken,
+			wantClientCert: true,
+		},
 		{
 			name:           "valid-without-client-cert",
 			id:             csWithoutClientCert.GetPublicId(),
@@ -1073,6 +1086,7 @@ func TestRepository_UpdateCredentialStore_VaultToken(t *testing.T) {
 		name               string
 		newTokenOpts       []TestOption
 		wantOldTokenStatus TokenStatus
+		updateToken        func(ctx context.Context, tokenHmac []byte)
 		wantCount          int
 		wantErr            errors.Code
 	}{
@@ -1081,6 +1095,17 @@ func TestRepository_UpdateCredentialStore_VaultToken(t *testing.T) {
 			wantOldTokenStatus: MaintainingToken,
 			wantCount:          1,
 		},
+		{
+			name:               "valid-token-expired",
+			wantOldTokenStatus: ExpiredToken,
+			updateToken: func(ctx context.Context, tokenHmac []byte) {
+				_, err := rw.Exec(ctx,
+					"update credential_vault_token set status = ? where token_hmac = ?",
+					[]interface{}{ExpiredToken, tokenHmac})
+				require.NoError(t, err)
+			},
+			wantCount: 1,
+		},
 		{
 			name:         "token-missing-capabilities",
 			newTokenOpts: []TestOption{WithPolicies([]string{"default"})},
@@ -1127,6 +1152,10 @@ func TestRepository_UpdateCredentialStore_VaultToken(t *testing.T) {
 			assert.NoError(err)
 			require.NotNil(orig)
 
+			if tt.updateToken != nil {
+				tt.updateToken(ctx, orig.outputToken.TokenHmac)
+			}
+
 			// update
 			_, updateToken := v.CreateToken(t, tt.newTokenOpts...)
 
@@ -1324,6 +1353,20 @@ func TestRepository_ListCredentialStores_Multiple_Scopes(t *testing.T) {
 		total += numPerScope
 	}
 
+	// Add some credential stores with expired tokens
+	_, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper))
+	prjs = append(prjs, prj.GetPublicId())
+
+	stores := TestCredentialStores(t, conn, wrapper, prj.GetPublicId(), numPerScope)
+	for _, cs := range stores {
+		rows, err := rw.Exec(context.Background(),
+			"update credential_vault_token set status = ? where token_hmac = ?",
+			[]interface{}{ExpiredToken, cs.Token().TokenHmac})
+		require.NoError(err)
+		require.Equal(1, rows)
+	}
+	total += numPerScope
+
 	got, err := repo.ListCredentialStores(context.Background(), prjs)
 	require.NoError(err)
 	assert.Equal(total, len(got))

diff --git a/internal/daemon/controller/handlers/credentialstores/credentialstore_service.go b/internal/daemon/controller/handlers/credentialstores/credentialstore_service.go
@@ -675,6 +675,7 @@ func toProto(ctx context.Context, in credential.Store, opt ...handlers.Option) (
 			}
 			if vaultIn.Token() != nil {
 				attrs.TokenHmac = base64.RawURLEncoding.EncodeToString(vaultIn.Token().GetTokenHmac())
+				attrs.TokenStatus = vaultIn.Token().GetStatus()
 			}
 			if vaultIn.GetWorkerFilter() != "" {
 				if vaultWorkerFilterToProto {

diff --git a/internal/daemon/controller/handlers/credentialstores/credentialstore_service_test.go b/internal/daemon/controller/handlers/credentialstores/credentialstore_service_test.go
@@ -96,6 +96,7 @@ func TestList(t *testing.T) {
 				VaultCredentialStoreAttributes: &pb.VaultCredentialStoreAttributes{
 					Address:                  wrapperspb.String(s.GetVaultAddress()),
 					TokenHmac:                base64.RawURLEncoding.EncodeToString(s.Token().GetTokenHmac()),
+					TokenStatus:              s.Token().GetStatus(),
 					ClientCertificate:        wrapperspb.String(string(s.ClientCertificate().GetCertificate())),
 					ClientCertificateKeyHmac: base64.RawURLEncoding.EncodeToString(s.ClientCertificate().GetCertificateKeyHmac()),
 					// TODO: Add all fields including tls related fields, namespace, etc...
@@ -471,6 +472,7 @@ func TestCreateVault(t *testing.T) {
 							CaCert:                   wrapperspb.String(string(v.CaCert)),
 							Address:                  wrapperspb.String(v.Addr),
 							TokenHmac:                "<hmac>",
+							TokenStatus:              "current",
 							ClientCertificate:        wrapperspb.String(string(v.ClientCert)),
 							ClientCertificateKeyHmac: "<hmac>",
 						},
@@ -512,6 +514,7 @@ func TestCreateVault(t *testing.T) {
 							CaCert:                   wrapperspb.String(string(v.CaCert)),
 							Address:                  wrapperspb.String(v.Addr),
 							TokenHmac:                "<hmac>",
+							TokenStatus:              "current",
 							ClientCertificate:        wrapperspb.String(string(v.ClientCert)),
 							ClientCertificateKeyHmac: "<hmac>",
 						},
@@ -795,6 +798,7 @@ func TestGet(t *testing.T) {
 						VaultCredentialStoreAttributes: &pb.VaultCredentialStoreAttributes{
 							Address:                  wrapperspb.String(vaultStore.GetVaultAddress()),
 							TokenHmac:                base64.RawURLEncoding.EncodeToString(vaultStore.Token().GetTokenHmac()),
+							TokenStatus:              vaultStore.Token().GetStatus(),
 							ClientCertificate:        wrapperspb.String(string(vaultStore.ClientCertificate().GetCertificate())),
 							ClientCertificateKeyHmac: base64.RawURLEncoding.EncodeToString(vaultStore.ClientCertificate().GetCertificateKeyHmac()),
 						},
@@ -1068,6 +1072,7 @@ func TestUpdateVault(t *testing.T) {
 			res: func(in *pb.CredentialStore) *pb.CredentialStore {
 				out := proto.Clone(in).(*pb.CredentialStore)
 				out.GetVaultCredentialStoreAttributes().TokenHmac = "<hmac>"
+				out.GetVaultCredentialStoreAttributes().TokenStatus = "current"
 				return out
 			},
 		},