From b2170210af1378598c7268ea57a9981a2cbe4504 Mon Sep 17 00:00:00 2001 From: Marc Boudreau Date: Fri, 5 Jul 2024 17:14:44 -0400 Subject: [PATCH 01/10] properly cleanup aliases no longer in entity during invalidation --- vault/identity_store.go | 70 ++++++++++++++++++++++++++++++----------- 1 file changed, 51 insertions(+), 19 deletions(-) diff --git a/vault/identity_store.go b/vault/identity_store.go index 8d53f4c35682..d722f0c91685 100644 --- a/vault/identity_store.go +++ b/vault/identity_store.go @@ -7,6 +7,7 @@ import ( "context" "fmt" "reflect" + "slices" "strings" "time" @@ -721,34 +722,65 @@ 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) - return + 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. Go through all of the + // aliases currently set in the memDB entity and check if any of + // those are not in the bucket entity. Those aliases that are only + // found in the memDB entity, need to be deleted from MemDB because + // the upsertEntityInTxn function does not delete those aliases. + if memDBEntity != nil { + aliasesToDelete := make([]*identity.Alias, 0) + bucketEntityAliases := bucketEntity.Aliases + + slices.SortFunc(bucketEntityAliases, func(a, b *identity.Alias) int { + return strings.Compare(a.ID, b.ID) + }) + + for _, a := range memDBEntity.Aliases { + _, found := slices.BinarySearchFunc(bucketEntityAliases, a.ID, func(a *identity.Alias, b string) int { + return strings.Compare(a.ID, b) + }) + if !found { + aliasesToDelete = append(aliasesToDelete, a) + } } - // 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 len(aliasesToDelete) > 0 { + err := i.deleteAliasesInEntityInTxn(txn, memDBEntity, aliasesToDelete) + if err != nil { + i.logger.Error("failed to remove entity alias from MemDB", "entity_id", memDBEntity.ID, "error", err) } } } + + 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 + } + } + } } From a230738f9e87ac4f51c853eb9180b8508a079f5f Mon Sep 17 00:00:00 2001 From: Marc Boudreau Date: Wed, 10 Jul 2024 12:10:25 -0400 Subject: [PATCH 02/10] test: verify proper alias removal from entity in invalidation --- vault/identity_store_test.go | 114 +++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/vault/identity_store_test.go b/vault/identity_store_test.go index 7c826dfa0c33..41a6a42aab3d 100644 --- a/vault/identity_store_test.go +++ b/vault/identity_store_test.go @@ -992,6 +992,120 @@ func TestIdentityStoreInvalidate_Entities(t *testing.T) { txn.Commit() } +func TestIdentityStoreInvalidate_EntityAliasUpdate(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" + c.auth = &MountTable{ + Type: credentialTableType, + Entries: []*MountEntry{ + { + Table: credentialTableType, + Path: "/noop1", + Type: "noop", + UUID: "abcd", + Accessor: mountAccessor1, + BackendAwareUUID: "abcde", + NamespaceID: namespace.RootNamespaceID, + namespace: namespace.RootNamespace, + }, + { + Table: credentialTableType, + Path: "/noop2", + Type: "noop", + UUID: "wxyz", + Accessor: mountAccessor2, + BackendAwareUUID: "vwxyz", + NamespaceID: namespace.RootNamespaceID, + namespace: namespace.RootNamespace, + }, + }, + } + + 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) + + // Create two entity-aliases + 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) + } + + createEntityAlias("alias1", mountAccessor1) + alias2ID := createEntityAlias("alias2", mountAccessor2) + + // 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) + + alias2, err := c.identityStore.MemDBAliasByID(alias2ID, false, false) + assert.NoError(t, err) + assert.Nil(t, alias2) +} + // TestIdentityStoreInvalidate_LocalAliasesWithEntity verifies the correct // handling of local aliases in the Invalidate method. func TestIdentityStoreInvalidate_LocalAliasesWithEntity(t *testing.T) { From 4ba821ecf064800423972f33a57110d587ed6280 Mon Sep 17 00:00:00 2001 From: Marc Boudreau Date: Wed, 10 Jul 2024 14:27:37 -0400 Subject: [PATCH 03/10] add changelog entry --- changelog/27750.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/27750.txt diff --git a/changelog/27750.txt b/changelog/27750.txt new file mode 100644 index 000000000000..04c24fe59e7f --- /dev/null +++ b/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. +``` From 60210bfd27632fde808f48d25030239e7245b99b Mon Sep 17 00:00:00 2001 From: Marc Boudreau Date: Wed, 10 Jul 2024 15:31:22 -0400 Subject: [PATCH 04/10] document dangling entity-alias known issue --- website/content/docs/release-notes/1.16.1.mdx | 1 + website/content/docs/release-notes/1.17.0.mdx | 1 + .../docs/upgrading/upgrade-to-1.16.x.mdx | 2 ++ .../docs/upgrading/upgrade-to-1.17.x.mdx | 2 ++ .../dangling-entity-aliases-in-memory.mdx | 24 +++++++++++++++++++ 5 files changed, 30 insertions(+) create mode 100644 website/content/partials/known-issues/dangling-entity-aliases-in-memory.mdx diff --git a/website/content/docs/release-notes/1.16.1.mdx b/website/content/docs/release-notes/1.16.1.mdx index f9f79bf884b7..98d63ae4df3a 100644 --- a/website/content/docs/release-notes/1.16.1.mdx +++ b/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.7 | [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 diff --git a/website/content/docs/release-notes/1.17.0.mdx b/website/content/docs/release-notes/1.17.0.mdx index e0557991f715..7bfeaf4b707d 100644 --- a/website/content/docs/release-notes/1.17.0.mdx +++ b/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 diff --git a/website/content/docs/upgrading/upgrade-to-1.16.x.mdx b/website/content/docs/upgrading/upgrade-to-1.16.x.mdx index 0e3d6ea7f436..74c511f0b3a7 100644 --- a/website/content/docs/upgrading/upgrade-to-1.16.x.mdx +++ b/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' diff --git a/website/content/docs/upgrading/upgrade-to-1.17.x.mdx b/website/content/docs/upgrading/upgrade-to-1.17.x.mdx index f0651364ddf5..813d922f39bf 100644 --- a/website/content/docs/upgrading/upgrade-to-1.17.x.mdx +++ b/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' diff --git a/website/content/partials/known-issues/dangling-entity-aliases-in-memory.mdx b/website/content/partials/known-issues/dangling-entity-aliases-in-memory.mdx new file mode 100644 index 000000000000..c825725552e3 --- /dev/null +++ b/website/content/partials/known-issues/dangling-entity-aliases-in-memory.mdx @@ -0,0 +1,24 @@ + + +### Deleting an entity-aliases does not remove it from the in-memory database on standby nodes + +#### Affected versions + +##### Vault Community Edition + +- 1.16.3 +- 1.17.0 - 1.17.2 + +##### Enterprise + +- 1.16.3+ent - 1.16.6+ent +- 1.17.0+ent - 1.17.2+ent + +#### Issue + +Entity-aliases deleted from Vault are not removed from the in-memory database on +standby nodes within a cluster. As a result, API requests to create a new +entity-alias with the same mount accessor and name sent to a standby node will +fail. + +This bug is fixed in Vault 1.16.7+ent, 1.17.3, 1.17.3+ent and later. From 6b5c05d5e996abf3a1c1e4973626b7a4d644bceb Mon Sep 17 00:00:00 2001 From: Marc Boudreau Date: Wed, 10 Jul 2024 16:10:51 -0400 Subject: [PATCH 05/10] improve entity-alias delete test --- vault/identity_store_test.go | 56 ++++++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/vault/identity_store_test.go b/vault/identity_store_test.go index 41a6a42aab3d..5ce2cf40a074 100644 --- a/vault/identity_store_test.go +++ b/vault/identity_store_test.go @@ -992,7 +992,11 @@ func TestIdentityStoreInvalidate_Entities(t *testing.T) { txn.Commit() } -func TestIdentityStoreInvalidate_EntityAliasUpdate(t *testing.T) { +// 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) @@ -1004,29 +1008,28 @@ func TestIdentityStoreInvalidate_EntityAliasUpdate(t *testing.T) { } 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{ - { - Table: credentialTableType, - Path: "/noop1", - Type: "noop", - UUID: "abcd", - Accessor: mountAccessor1, - BackendAwareUUID: "abcde", - NamespaceID: namespace.RootNamespaceID, - namespace: namespace.RootNamespace, - }, - { - Table: credentialTableType, - Path: "/noop2", - Type: "noop", - UUID: "wxyz", - Accessor: mountAccessor2, - BackendAwareUUID: "vwxyz", - NamespaceID: namespace.RootNamespaceID, - namespace: namespace.RootNamespace, - }, + createMountEntry("/noop1", "abcd", mountAccessor1, false), + createMountEntry("/noop2", "ghij", mountAccessor2, false), + createMountEntry("/noop3", "mnop", mountAccessor3, true), }, } @@ -1070,8 +1073,9 @@ func TestIdentityStoreInvalidate_EntityAliasUpdate(t *testing.T) { return resp.Data["id"].(string) } - createEntityAlias("alias1", mountAccessor1) + 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) @@ -1101,9 +1105,17 @@ func TestIdentityStoreInvalidate_EntityAliasUpdate(t *testing.T) { 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 From 2669a067e51734ccb87cdbfcc87727a5fbb68d8b Mon Sep 17 00:00:00 2001 From: Marc Boudreau Date: Fri, 12 Jul 2024 13:37:41 -0400 Subject: [PATCH 06/10] fixup! document dangling entity-alias known issue --- website/content/docs/release-notes/1.16.1.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/content/docs/release-notes/1.16.1.mdx b/website/content/docs/release-notes/1.16.1.mdx index 98d63ae4df3a..a5a324516240 100644 --- a/website/content/docs/release-notes/1.16.1.mdx +++ b/website/content/docs/release-notes/1.16.1.mdx @@ -24,7 +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.7 | [Vault standby nodes not deleting removed entity-aliases from in-memory database](/vault/docs/upgrade-to-1.16.x#dangling-entity-alias-in-memory) | +| 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 From b2a78123878bc329f99ddd58e6e9d4fbbccba65f Mon Sep 17 00:00:00 2001 From: Marc Boudreau Date: Mon, 22 Jul 2024 10:50:53 -0400 Subject: [PATCH 07/10] use simpler approach to reconcile entity aliases in invalidation --- vault/identity_store.go | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/vault/identity_store.go b/vault/identity_store.go index d722f0c91685..d05fb9c31ff6 100644 --- a/vault/identity_store.go +++ b/vault/identity_store.go @@ -7,7 +7,6 @@ import ( "context" "fmt" "reflect" - "slices" "strings" "time" @@ -738,27 +737,9 @@ func (i *IdentityStore) invalidateEntityBucket(ctx context.Context, key string) // found in the memDB entity, need to be deleted from MemDB because // the upsertEntityInTxn function does not delete those aliases. if memDBEntity != nil { - aliasesToDelete := make([]*identity.Alias, 0) - bucketEntityAliases := bucketEntity.Aliases - - slices.SortFunc(bucketEntityAliases, func(a, b *identity.Alias) int { - return strings.Compare(a.ID, b.ID) - }) - - for _, a := range memDBEntity.Aliases { - _, found := slices.BinarySearchFunc(bucketEntityAliases, a.ID, func(a *identity.Alias, b string) int { - return strings.Compare(a.ID, b) - }) - if !found { - aliasesToDelete = append(aliasesToDelete, a) - } - } - - if len(aliasesToDelete) > 0 { - err := i.deleteAliasesInEntityInTxn(txn, memDBEntity, aliasesToDelete) - if err != nil { - i.logger.Error("failed to remove entity alias from MemDB", "entity_id", memDBEntity.ID, "error", err) - } + 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 } } From f7e47fff90d29c19fe1f6dcbf3254250801ecce2 Mon Sep 17 00:00:00 2001 From: Marc Boudreau Date: Tue, 23 Jul 2024 10:15:53 -0400 Subject: [PATCH 08/10] adjust comment to match previous code change --- vault/identity_store.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/vault/identity_store.go b/vault/identity_store.go index d05fb9c31ff6..a6af9a2b5b52 100644 --- a/vault/identity_store.go +++ b/vault/identity_store.go @@ -731,11 +731,12 @@ func (i *IdentityStore) invalidateEntityBucket(ctx context.Context, key string) } // If the entity exists in MemDB it must differ from the entity in - // the storage bucket because of above test. Go through all of the - // aliases currently set in the memDB entity and check if any of - // those are not in the bucket entity. Those aliases that are only - // found in the memDB entity, need to be deleted from MemDB because - // the upsertEntityInTxn function does not delete those aliases. + // 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) From eb7df5f1fcc57aa09e1d73962028d5a22f0b0d47 Mon Sep 17 00:00:00 2001 From: Marc Boudreau Date: Wed, 24 Jul 2024 14:52:44 -0400 Subject: [PATCH 09/10] add test covering local aliases --- vault/identity_store_test.go | 149 ++++++++++++++++++++++++++++++++++- 1 file changed, 148 insertions(+), 1 deletion(-) diff --git a/vault/identity_store_test.go b/vault/identity_store_test.go index 5ce2cf40a074..da16ae6fac92 100644 --- a/vault/identity_store_test.go +++ b/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" ) @@ -1052,7 +1053,6 @@ func TestIdentityStoreInvalidate_EntityAliasDelete(t *testing.T) { entityID := resp.Data["id"].(string) - // Create two entity-aliases createEntityAlias := func(name, mountAccessor string) string { req = &logical.Request{ ClientToken: root, @@ -1118,6 +1118,153 @@ func TestIdentityStoreInvalidate_EntityAliasDelete(t *testing.T) { 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) { From 6e7aea61cc4e2669556d00e360c3b43ad2b5aec6 Mon Sep 17 00:00:00 2001 From: Marc Boudreau Date: Wed, 24 Jul 2024 15:28:46 -0400 Subject: [PATCH 10/10] pre-delete changed entity in invalidation --- vault/identity_store.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/vault/identity_store.go b/vault/identity_store.go index a6af9a2b5b52..22196269c8ff 100644 --- a/vault/identity_store.go +++ b/vault/identity_store.go @@ -742,6 +742,11 @@ func (i *IdentityStore) invalidateEntityBucket(ctx context.Context, key string) i.logger.Error("failed to remove entity aliases from changed entity", "entity_id", memDBEntity.ID, "error", err) 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)