VAULT-28677: Fix dangling entity-aliases in MemDB after invalidation (#…

…27750)

* properly cleanup aliases no longer in entity during invalidation

* test: verify proper alias removal from entity in invalidation

* add changelog entry

* document dangling entity-alias known issue

* improve entity-alias delete test

* fixup! document dangling entity-alias known issue

* use simpler approach to reconcile entity aliases in invalidation

* adjust comment to match previous code change

* add test covering local aliases

* pre-delete changed entity in invalidation

changelog/27750.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+core/identity: Fixed an issue where deleted/reassigned entity-aliases were not removed from in-memory database.
+```

vault/identity_store.go

@@ -721,34 +721,53 @@ func (i *IdentityStore) invalidateEntityBucket(ctx context.Context, key string)
 				}
 			}
 
-			// If the entity is not in MemDB or if it is but differs from the
-			// state that's in the bucket storage entry, upsert it into MemDB.
-
 			// We've considered the use of github.com/google/go-cmp here,
 			// but opted for sticking with reflect.DeepEqual because go-cmp
 			// is intended for testing and is able to panic in some
 			// situations.
-			if memDBEntity == nil || !reflect.DeepEqual(memDBEntity, bucketEntity) {
-				// The entity is not in MemDB, it's a new entity. Add it to MemDB.
-				err = i.upsertEntityInTxn(ctx, txn, bucketEntity, nil, false)
-				if err != nil {
-					i.logger.Error("failed to update entity in MemDB", "entity_id", bucketEntity.ID, "error", err)
+			if memDBEntity != nil && reflect.DeepEqual(memDBEntity, bucketEntity) {
+				// No changes on this entity, move on to the next one.
+				continue
+			}
+
+			// If the entity exists in MemDB it must differ from the entity in
+			// the storage bucket because of above test. Blindly delete the
+			// current aliases associated with the MemDB entity. The correct set
+			// of aliases will be created in MemDB by the upsertEntityInTxn
+			// function. We need to do this because the upsertEntityInTxn
+			// function does not delete those aliases, it only creates missing
+			// ones.
+			if memDBEntity != nil {
+				if err := i.deleteAliasesInEntityInTxn(txn, memDBEntity, memDBEntity.Aliases); err != nil {
+					i.logger.Error("failed to remove entity aliases from changed entity", "entity_id", memDBEntity.ID, "error", err)
 					return
 				}
 
-				// If this is a performance secondary, the entity created on
-				// this node would have been cached in a local cache based on
-				// the result of the CreateEntity RPC call to the primary
-				// cluster. Since this invalidation is signaling that the
-				// entity is now in the primary cluster's storage, the locally
-				// cached entry can be removed.
-				if i.localNode.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) && i.localNode.HAState() == consts.Active {
-					if err := i.localAliasPacker.DeleteItem(ctx, bucketEntity.ID+tmpSuffix); err != nil {
-						i.logger.Error("failed to clear local alias entity cache", "error", err, "entity_id", bucketEntity.ID)
-						return
-					}
+				if err := i.MemDBDeleteEntityByIDInTxn(txn, memDBEntity.ID); err != nil {
+					i.logger.Error("failed to delete changed entity", "entity_id", memDBEntity.ID, "error", err)
+					return
 				}
 			}
+
+			err = i.upsertEntityInTxn(ctx, txn, bucketEntity, nil, false)
+			if err != nil {
+				i.logger.Error("failed to update entity in MemDB", "entity_id", bucketEntity.ID, "error", err)
+				return
+			}
+
+			// If this is a performance secondary, the entity created on
+			// this node would have been cached in a local cache based on
+			// the result of the CreateEntity RPC call to the primary
+			// cluster. Since this invalidation is signaling that the
+			// entity is now in the primary cluster's storage, the locally
+			// cached entry can be removed.
+			if i.localNode.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) && i.localNode.HAState() == consts.Active {
+				if err := i.localAliasPacker.DeleteItem(ctx, bucketEntity.ID+tmpSuffix); err != nil {
+					i.logger.Error("failed to clear local alias entity cache", "error", err, "entity_id", bucketEntity.ID)
+					return
+				}
+			}
+
 		}
 	}
 

vault/identity_store_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/hashicorp/vault/sdk/logical"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/known/anypb"
 )
 
@@ -992,6 +993,278 @@ func TestIdentityStoreInvalidate_Entities(t *testing.T) {
 	txn.Commit()
 }
 
+// TestIdentityStoreInvalidate_EntityAliasDelete verifies that the
+// invalidateEntityBucket method properly cleans up aliases from
+// MemDB that are no longer associated with the entity in the
+// storage bucket.
+func TestIdentityStoreInvalidate_EntityAliasDelete(t *testing.T) {
+	ctx := namespace.ContextWithNamespace(context.Background(), namespace.RootNamespace)
+	c, _, root := TestCoreUnsealed(t)
+
+	// Enable a No-Op auth method
+	c.credentialBackends["noop"] = func(context.Context, *logical.BackendConfig) (logical.Backend, error) {
+		return &NoopBackend{
+			BackendType: logical.TypeCredential,
+		}, nil
+	}
+	mountAccessor1 := "noop-accessor1"
+	mountAccessor2 := "noop-accessor2"
+	mountAccessor3 := "noon-accessor3"
+
+	createMountEntry := func(path, uuid, mountAccessor string, local bool) *MountEntry {
+		return &MountEntry{
+			Table:            credentialTableType,
+			Path:             path,
+			Type:             "noop",
+			UUID:             uuid,
+			Accessor:         mountAccessor,
+			BackendAwareUUID: uuid + "backend",
+			NamespaceID:      namespace.RootNamespaceID,
+			namespace:        namespace.RootNamespace,
+			Local:            local,
+		}
+	}
+
+	c.auth = &MountTable{
+		Type: credentialTableType,
+		Entries: []*MountEntry{
+			createMountEntry("/noop1", "abcd", mountAccessor1, false),
+			createMountEntry("/noop2", "ghij", mountAccessor2, false),
+			createMountEntry("/noop3", "mnop", mountAccessor3, true),
+		},
+	}
+
+	require.NoError(t, c.setupCredentials(context.Background()))
+
+	// Create an entity
+	req := &logical.Request{
+		ClientToken: root,
+		Operation:   logical.UpdateOperation,
+		Path:        "entity",
+		Data: map[string]interface{}{
+			"name": "alice",
+		},
+	}
+
+	resp, err := c.identityStore.HandleRequest(ctx, req)
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.Contains(t, resp.Data, "id")
+
+	entityID := resp.Data["id"].(string)
+
+	createEntityAlias := func(name, mountAccessor string) string {
+		req = &logical.Request{
+			ClientToken: root,
+			Operation:   logical.UpdateOperation,
+			Path:        "entity-alias",
+			Data: map[string]interface{}{
+				"name":           name,
+				"canonical_id":   entityID,
+				"mount_accessor": mountAccessor,
+			},
+		}
+
+		resp, err = c.identityStore.HandleRequest(ctx, req)
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+		require.Contains(t, resp.Data, "id")
+
+		return resp.Data["id"].(string)
+	}
+
+	alias1ID := createEntityAlias("alias1", mountAccessor1)
+	alias2ID := createEntityAlias("alias2", mountAccessor2)
+	alias3ID := createEntityAlias("alias3", mountAccessor3)
+
+	// Update the entity in storage only to remove alias2 then call invalidate
+	bucketKey := c.identityStore.entityPacker.BucketKey(entityID)
+	bucket, err := c.identityStore.entityPacker.GetBucket(context.Background(), bucketKey)
+	require.NoError(t, err)
+	require.NotNil(t, bucket)
+
+	bucketEntityItem := bucket.Items[0] // since there's only 1 entity
+	bucketEntity, err := c.identityStore.parseEntityFromBucketItem(context.Background(), bucketEntityItem)
+	require.NoError(t, err)
+	require.NotNil(t, bucketEntity)
+
+	replacementAliases := make([]*identity.Alias, 1)
+	for _, a := range bucketEntity.Aliases {
+		if a.ID != alias2ID {
+			replacementAliases[0] = a
+			break
+		}
+	}
+
+	bucketEntity.Aliases = replacementAliases
+
+	bucketEntityItem.Message, err = anypb.New(bucketEntity)
+	require.NoError(t, err)
+
+	require.NoError(t, c.identityStore.entityPacker.PutItem(context.Background(), bucketEntityItem))
+
+	c.identityStore.Invalidate(context.Background(), bucketKey)
+
+	alias1, err := c.identityStore.MemDBAliasByID(alias1ID, false, false)
+	assert.NoError(t, err)
+	assert.NotNil(t, alias1)
+
+	alias2, err := c.identityStore.MemDBAliasByID(alias2ID, false, false)
+	assert.NoError(t, err)
+	assert.Nil(t, alias2)
+
+	alias3, err := c.identityStore.MemDBAliasByID(alias3ID, false, false)
+	assert.NoError(t, err)
+	assert.NotNil(t, alias3)
+}
+
+// TestIdentityStoreInvalidate_EntityLocalAliasDelete verifies that the
+// invalidateLocalAliasesBucket method properly cleans up aliases from
+// MemDB that are no longer associated with the entity in the
+// storage bucket.
+func TestIdentityStoreInvalidate_EntityLocalAliasDelete(t *testing.T) {
+	ctx := namespace.ContextWithNamespace(context.Background(), namespace.RootNamespace)
+	c, _, root := TestCoreUnsealed(t)
+
+	// Enable a No-Op auth method
+	c.credentialBackends["noop"] = func(context.Context, *logical.BackendConfig) (logical.Backend, error) {
+		return &NoopBackend{
+			BackendType: logical.TypeCredential,
+		}, nil
+	}
+	mountAccessor1 := "noop-accessor1"
+	mountAccessor2 := "noop-accessor2"
+	mountAccessor3 := "noon-accessor3"
+
+	createMountEntry := func(path, uuid, mountAccessor string, local bool) *MountEntry {
+		return &MountEntry{
+			Table:            credentialTableType,
+			Path:             path,
+			Type:             "noop",
+			UUID:             uuid,
+			Accessor:         mountAccessor,
+			BackendAwareUUID: uuid + "backend",
+			NamespaceID:      namespace.RootNamespaceID,
+			namespace:        namespace.RootNamespace,
+			Local:            local,
+		}
+	}
+
+	c.auth = &MountTable{
+		Type: credentialTableType,
+		Entries: []*MountEntry{
+			createMountEntry("/noop1", "abcd", mountAccessor1, true),
+			createMountEntry("/noop2", "ghij", mountAccessor2, true),
+			createMountEntry("/noop3", "mnop", mountAccessor3, true),
+		},
+	}
+
+	require.NoError(t, c.setupCredentials(context.Background()))
+
+	// Create an entity
+	req := &logical.Request{
+		ClientToken: root,
+		Operation:   logical.UpdateOperation,
+		Path:        "entity",
+		Data: map[string]interface{}{
+			"name": "alice",
+		},
+	}
+
+	resp, err := c.identityStore.HandleRequest(ctx, req)
+	require.NoError(t, err)
+	require.NotNil(t, resp)
+	require.Contains(t, resp.Data, "id")
+
+	entityID := resp.Data["id"].(string)
+
+	createEntityAlias := func(name, mountAccessor string) string {
+		req = &logical.Request{
+			ClientToken: root,
+			Operation:   logical.UpdateOperation,
+			Path:        "entity-alias",
+			Data: map[string]interface{}{
+				"name":           name,
+				"canonical_id":   entityID,
+				"mount_accessor": mountAccessor,
+			},
+		}
+
+		resp, err = c.identityStore.HandleRequest(ctx, req)
+		require.NoError(t, err)
+		require.NotNil(t, resp)
+		require.Contains(t, resp.Data, "id")
+
+		return resp.Data["id"].(string)
+	}
+
+	alias1ID := createEntityAlias("alias1", mountAccessor1)
+	alias2ID := createEntityAlias("alias2", mountAccessor2)
+	alias3ID := createEntityAlias("alias3", mountAccessor3)
+
+	for i, aliasID := range []string{alias1ID, alias2ID, alias3ID} {
+		alias, err := c.identityStore.MemDBAliasByID(aliasID, false, false)
+		require.NoError(t, err, i)
+		require.NotNil(t, alias, i)
+	}
+
+	// // Update the entity in storage only to remove alias2 then call invalidate
+	bucketKey := c.identityStore.entityPacker.BucketKey(entityID)
+	bucket, err := c.identityStore.entityPacker.GetBucket(context.Background(), bucketKey)
+	require.NoError(t, err)
+	require.NotNil(t, bucket)
+
+	bucketEntityItem := bucket.Items[0] // since there's only 1 entity
+	bucketEntity, err := c.identityStore.parseEntityFromBucketItem(context.Background(), bucketEntityItem)
+	require.NoError(t, err)
+	require.NotNil(t, bucketEntity)
+
+	bucketKey = c.identityStore.localAliasPacker.BucketKey(entityID)
+	bucketLocalAlias, err := c.identityStore.localAliasPacker.GetBucket(context.Background(), bucketKey)
+	require.NoError(t, err)
+	require.NotNil(t, bucketLocalAlias)
+
+	bucketLocalAliasItem := bucketLocalAlias.Items[0]
+	require.Equal(t, entityID, bucketLocalAliasItem.ID)
+
+	var localAliases identity.LocalAliases
+
+	err = anypb.UnmarshalTo(bucketLocalAliasItem.Message, &localAliases, proto.UnmarshalOptions{})
+	require.NoError(t, err)
+
+	memDBEntity, err := c.identityStore.MemDBEntityByID(entityID, false)
+	require.NoError(t, err)
+	require.NotNil(t, memDBEntity)
+
+	replacementAliases := make([]*identity.Alias, 0)
+	for _, a := range memDBEntity.Aliases {
+		if a.ID != alias2ID {
+			replacementAliases = append(replacementAliases, a)
+		}
+	}
+
+	localAliases.Aliases = replacementAliases
+
+	bucketLocalAliasItem.Message, err = anypb.New(&localAliases)
+	require.NoError(t, err)
+
+	require.NoError(t, c.identityStore.localAliasPacker.PutItem(context.Background(), bucketLocalAliasItem))
+
+	c.identityStore.Invalidate(context.Background(), bucketKey)
+
+	alias1, err := c.identityStore.MemDBAliasByID(alias1ID, false, false)
+	assert.NoError(t, err)
+	assert.NotNil(t, alias1)
+
+	alias2, err := c.identityStore.MemDBAliasByID(alias2ID, false, false)
+	assert.NoError(t, err)
+	assert.Nil(t, alias2)
+
+	alias3, err := c.identityStore.MemDBAliasByID(alias3ID, false, false)
+	assert.NoError(t, err)
+	assert.NotNil(t, alias3)
+}
+
 // TestIdentityStoreInvalidate_LocalAliasesWithEntity verifies the correct
 // handling of local aliases in the Invalidate method.
 func TestIdentityStoreInvalidate_LocalAliasesWithEntity(t *testing.T) {

website/content/docs/release-notes/1.16.1.mdx

@@ -24,6 +24,7 @@ description: |-
 | 1.16.1 - 1.16.3 | [New nodes added by autopilot upgrades provisioned with the wrong version](/vault/docs/upgrading/upgrade-to-1.15.x#new-nodes-added-by-autopilot-upgrades-provisioned-with-the-wrong-version) |
 | 1.15.8+         | [Autopilot upgrade for Vault Enterprise fails](/vault/docs/upgrading/upgrade-to-1.15.x#autopilot)                                                                                            |
 | 1.16.5          | [Listener stops listening on untrusted upstream connection with particular config settings](/vault/docs/upgrading/upgrade-to-1.16.x#listener-proxy-protocol-config)                          |
+| 1.16.3 - 1.16.6 | [Vault standby nodes not deleting removed entity-aliases from in-memory database](/vault/docs/upgrade-to-1.16.x#dangling-entity-alias-in-memory)                                             |
 
 ## Vault companion updates
 

website/content/docs/release-notes/1.17.0.mdx

@@ -22,6 +22,7 @@ description: |-
 | Known issue (1.17.0)           | [Vault Agent and Vault Proxy consume excessive amounts of CPU](/vault/docs/upgrading/upgrade-to-1.17.x#agent-proxy-cpu-1-17)                                        |
 | Known issue (1.15.8+)          | [Autopilot upgrade for Vault Enterprise fails](/vault/docs/upgrading/upgrade-to-1.15.x#autopilot)                                                                   |
 | Known issue (1.17.1)           | [Listener stops listening on untrusted upstream connection with particular config settings](/vault/docs/upgrading/upgrade-to-1.17.x#listener-proxy-protocol-config) |
+| Known issue (1.17.0 - 1.17.2)  | [Vault standby nodes not deleting removed entity-aliases from in-memory database](/vault/docs/upgrade-to-1.17.x#dangling-entity-alias-in-memory)
 
 ## Vault companion updates
 

website/content/docs/upgrading/upgrade-to-1.16.x.mdx

@@ -115,3 +115,5 @@ decides to trigger the flag. More information can be found in the
 @include 'known-issues/1_16_secrets-sync-chroot-activation.mdx'
 
 @include 'known-issues/config_listener_proxy_protocol_behavior_issue.mdx'
+
+@include 'known-issues/dangling-entity-aliases-in-memory.mdx'

website/content/docs/upgrading/upgrade-to-1.17.x.mdx

@@ -90,3 +90,5 @@ incorrectly. For additional details, refer to the
 @include 'known-issues/config_listener_proxy_protocol_behavior_issue.mdx'
 
 @include 'known-issues/transit-input-on-cmac-response.mdx'
+
+@include 'known-issues/dangling-entity-aliases-in-memory.mdx'