diff --git a/changelog/29356.txt b/changelog/29356.txt new file mode 100644 index 000000000000..904646271c71 --- /dev/null +++ b/changelog/29356.txt @@ -0,0 +1,5 @@ +```release-note:feature +**Identity De-duplication**: Vault can now automatically resolve duplicate +Entities and Groups by renaming them. This feature is disabled by default and +can be enabled through the `force_identity_deduplication` activation flag. +``` diff --git a/helper/activationflags/activation_flags.go b/helper/activationflags/activation_flags.go index 66579b3da85d..948181141fed 100644 --- a/helper/activationflags/activation_flags.go +++ b/helper/activationflags/activation_flags.go @@ -14,6 +14,7 @@ import ( const ( storagePathActivationFlags = "activation-flags" + IdentityDeduplication = "force-identity-deduplication" ) type FeatureActivationFlags struct { diff --git a/helper/identity/identity.go b/helper/identity/identity.go index 2d625c4bcc6c..bda14f224759 100644 --- a/helper/identity/identity.go +++ b/helper/identity/identity.go @@ -58,6 +58,22 @@ func (e *Entity) UpsertAlias(alias *Alias) { e.Aliases = append(e.Aliases, alias) } +func (e *Entity) DeleteAliasByID(aliasID string) { + idx := -1 + for i, item := range e.Aliases { + if item.ID == aliasID { + idx = i + break + } + } + + if idx < 0 { + return + } + + e.Aliases = append(e.Aliases[:idx], e.Aliases[idx+1:]...) +} + func (p *Alias) Clone() (*Alias, error) { if p == nil { return nil, fmt.Errorf("nil alias") diff --git a/vault/identity_store.go b/vault/identity_store.go index 22196269c8ff..6fec26332c60 100644 --- a/vault/identity_store.go +++ b/vault/identity_store.go @@ -45,7 +45,7 @@ func (c *Core) IdentityStore() *IdentityStore { return c.identityStore } -func (i *IdentityStore) resetDB(ctx context.Context) error { +func (i *IdentityStore) resetDB() error { var err error i.db, err = memdb.NewMemDB(identityStoreSchema(!i.disableLowerCasedNames)) @@ -76,7 +76,7 @@ func NewIdentityStore(ctx context.Context, core *Core, config *logical.BackendCo // Create a memdb instance, which by default, operates on lower cased // identity names - err := iStore.resetDB(ctx) + err := iStore.resetDB() if err != nil { return nil, err } diff --git a/vault/identity_store_conflicts.go b/vault/identity_store_conflicts.go index 23cabcf10ed0..2d27309a551a 100644 --- a/vault/identity_store_conflicts.go +++ b/vault/identity_store_conflicts.go @@ -27,7 +27,7 @@ type ConflictResolver interface { ResolveAliases(ctx context.Context, parent *identity.Entity, existing, duplicate *identity.Alias) error } -// The errorResolver is a ConflictResolver that logs a warning message when a +// errorResolver is a ConflictResolver that logs a warning message when a // pre-existing Identity artifact is found with the same factors as a new one. type errorResolver struct { logger log.Logger @@ -207,7 +207,7 @@ func (r *duplicateReportingErrorResolver) Report() identityDuplicateReport { index: idx, numOthers: len(entities) - 1, } - if idx < len(entities)-1 { + if idx > 0 { r.resolutionHint = fmt.Sprintf("would rename to %s-%s", entity.Name, entity.ID) } else { r.resolutionHint = "would not rename" @@ -234,7 +234,7 @@ func (r *duplicateReportingErrorResolver) Report() identityDuplicateReport { index: idx, numOthers: len(groups) - 1, } - if idx < len(groups)-1 { + if idx > 0 { r.resolutionHint = fmt.Sprintf("would rename to %s-%s", group.Name, group.ID) } else { r.resolutionHint = "would not rename" @@ -278,8 +278,8 @@ func reportAliases(report *identityDuplicateReport, seen map[string][]*identity. index: idx, numOthers: len(aliases) - 1, } - if idx < len(aliases)-1 { - r.resolutionHint = fmt.Sprintf("would merge into entity %s", aliases[len(aliases)-1].CanonicalID) + if idx > 0 { + r.resolutionHint = fmt.Sprintf("would merge into entity %s", aliases[0].CanonicalID) } else { r.resolutionHint = "would merge others into this entity" } @@ -361,3 +361,68 @@ func (r *duplicateReportingErrorResolver) LogReport(log Warner) { log.Warn("end of identity duplicate report, refer to " + identityDuplicateReportUrl + " for resolution.") } + +// renameResolver is a ConflictResolver that appends the artifact's UUID to its +// name to resolve potential conflicts. The renamed resource is associated with +// the duplicated artifact by adding a `duplicate_of_canonical_id` metadata +// field. +type renameResolver struct { + logger log.Logger +} + +// ResolveEntities renames an entity duplicate in a deterministic way so that +// all entities end up addressable by a unique name still. We rename the +// pre-existing entity such that only the last occurrence retains its unmodified +// name. Note that this is potentially destructive but is the best option +// available to resolve duplicates in storage caused by bugs in our validation. +func (r *renameResolver) ResolveEntities(ctx context.Context, existing, duplicate *identity.Entity) error { + if existing == nil { + return nil + } + + duplicate.Name = duplicate.Name + "-" + duplicate.ID + if duplicate.Metadata == nil { + duplicate.Metadata = make(map[string]string) + } + duplicate.Metadata["duplicate_of_canonical_id"] = existing.ID + + r.logger.Warn("renaming entity with duplicate name", + "namespace_id", duplicate.NamespaceID, + "entity_id", duplicate.ID, + "duplicate_of_canonical_id", existing.ID, + "renamed_from", duplicate.Name, + "renamed_to", duplicate.Name, + ) + + return nil +} + +// ResolveGroups deals with group name duplicates by renaming those that +// were "hidden" in memDB so they are queryable. It's important this is +// deterministic so we don't end up with different group names on different +// nodes. We use the ID to ensure the new name is unique bit also +// deterministic. For now, don't persist this. The user can choose to +// resolve it permanently by renaming or deleting explicitly. +func (r *renameResolver) ResolveGroups(ctx context.Context, existing, duplicate *identity.Group) error { + if existing == nil { + return nil + } + + duplicate.Name = duplicate.Name + "-" + duplicate.ID + if duplicate.Metadata == nil { + duplicate.Metadata = make(map[string]string) + } + duplicate.Metadata["duplicate_of_canonical_id"] = existing.ID + r.logger.Warn("renaming group with duplicate name", + "namespace_id", duplicate.NamespaceID, + "group_id", duplicate.ID, + "duplicate_of_canonical_id", existing.ID, + "new_name", duplicate.Name, + ) + return nil +} + +// ResolveAliases is a no-op for the renameResolver implementation. +func (r *renameResolver) ResolveAliases(ctx context.Context, parent *identity.Entity, existing, duplicate *identity.Alias) error { + return nil +} diff --git a/vault/identity_store_conflicts_test.go b/vault/identity_store_conflicts_test.go index ac757ec95069..e7fe84458677 100644 --- a/vault/identity_store_conflicts_test.go +++ b/vault/identity_store_conflicts_test.go @@ -67,37 +67,37 @@ func TestDuplicateReportingErrorResolver(t *testing.T) { expectReport := ` DUPLICATES DETECTED, see following logs for details and refer to https://developer.hashicorp.com/vault/docs/upgrading/identity-deduplication for resolution.: 1 different-case local entity alias duplicates found (potential security risk): -local entity-alias "DIFFERENT-CASE-ALIAS-1" with mount accessor "local-mount" duplicates 1 others: id="00000000-0000-0000-0000-000000000009" canonical_id="11111111-0000-0000-0000-000000000009" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000010" -local entity-alias "different-CASE-ALIAS-1" with mount accessor "local-mount" duplicates 1 others: id="00000000-0000-0000-0000-000000000010" canonical_id="11111111-0000-0000-0000-000000000010" force_deduplication="would merge others into this entity" +local entity-alias "DIFFERENT-CASE-ALIAS-1" with mount accessor "local-mount" duplicates 1 others: id="00000000-0000-0000-0000-000000000009" canonical_id="11111111-0000-0000-0000-000000000009" force_deduplication="would merge others into this entity" +local entity-alias "different-CASE-ALIAS-1" with mount accessor "local-mount" duplicates 1 others: id="00000000-0000-0000-0000-000000000010" canonical_id="11111111-0000-0000-0000-000000000010" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000009" end of different-case local entity-alias duplicates: 2 different-case entity alias duplicates found (potential security risk): -entity-alias "different-case-alias-1" with mount accessor "mount1" duplicates 1 others: id="00000000-0000-0000-0000-000000000004" canonical_id="11111111-0000-0000-0000-000000000004" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000005" -entity-alias "DIFFERENT-CASE-ALIAS-1" with mount accessor "mount1" duplicates 1 others: id="00000000-0000-0000-0000-000000000005" canonical_id="11111111-0000-0000-0000-000000000005" force_deduplication="would merge others into this entity" -entity-alias "different-case-alias-1" with mount accessor "mount2" duplicates 2 others: id="00000000-0000-0000-0000-000000000006" canonical_id="11111111-0000-0000-0000-000000000006" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000008" -entity-alias "DIFFERENT-CASE-ALIAS-1" with mount accessor "mount2" duplicates 2 others: id="00000000-0000-0000-0000-000000000007" canonical_id="11111111-0000-0000-0000-000000000007" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000008" -entity-alias "different-CASE-ALIAS-1" with mount accessor "mount2" duplicates 2 others: id="00000000-0000-0000-0000-000000000008" canonical_id="11111111-0000-0000-0000-000000000008" force_deduplication="would merge others into this entity" +entity-alias "different-case-alias-1" with mount accessor "mount1" duplicates 1 others: id="00000000-0000-0000-0000-000000000004" canonical_id="11111111-0000-0000-0000-000000000004" force_deduplication="would merge others into this entity" +entity-alias "DIFFERENT-CASE-ALIAS-1" with mount accessor "mount1" duplicates 1 others: id="00000000-0000-0000-0000-000000000005" canonical_id="11111111-0000-0000-0000-000000000005" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000004" +entity-alias "different-case-alias-1" with mount accessor "mount2" duplicates 2 others: id="00000000-0000-0000-0000-000000000006" canonical_id="11111111-0000-0000-0000-000000000006" force_deduplication="would merge others into this entity" +entity-alias "DIFFERENT-CASE-ALIAS-1" with mount accessor "mount2" duplicates 2 others: id="00000000-0000-0000-0000-000000000007" canonical_id="11111111-0000-0000-0000-000000000007" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000006" +entity-alias "different-CASE-ALIAS-1" with mount accessor "mount2" duplicates 2 others: id="00000000-0000-0000-0000-000000000008" canonical_id="11111111-0000-0000-0000-000000000008" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000006" end of different-case entity-alias duplicates: 4 entity duplicates found: -entity "different-case-dupe-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000010" force_deduplication="would rename to different-case-dupe-1-00000000-0000-0000-0000-000000000010" +entity "different-case-dupe-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000010" force_deduplication="would not rename" entity "DIFFERENT-case-DUPE-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000011" force_deduplication="would rename to DIFFERENT-case-DUPE-1-00000000-0000-0000-0000-000000000011" -entity "different-case-DUPE-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000012" force_deduplication="would not rename" -entity "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000006" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000006" -entity "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000007" force_deduplication="would not rename" -entity "different-case-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000008" force_deduplication="would rename to different-case-dupe-1-00000000-0000-0000-0000-000000000008" -entity "DIFFERENT-CASE-DUPE-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000009" force_deduplication="would not rename" -entity "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000004" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000004" -entity "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000005" force_deduplication="would not rename" +entity "different-case-DUPE-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000012" force_deduplication="would rename to different-case-DUPE-1-00000000-0000-0000-0000-000000000012" +entity "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000006" force_deduplication="would not rename" +entity "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000007" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000007" +entity "different-case-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000008" force_deduplication="would not rename" +entity "DIFFERENT-CASE-DUPE-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000009" force_deduplication="would rename to DIFFERENT-CASE-DUPE-1-00000000-0000-0000-0000-000000000009" +entity "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000004" force_deduplication="would not rename" +entity "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000005" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000005" end of entity duplicates: 4 group duplicates found: -group "different-case-dupe-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000010" force_deduplication="would rename to different-case-dupe-1-00000000-0000-0000-0000-000000000010" +group "different-case-dupe-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000010" force_deduplication="would not rename" group "DIFFERENT-case-DUPE-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000011" force_deduplication="would rename to DIFFERENT-case-DUPE-1-00000000-0000-0000-0000-000000000011" -group "different-case-DUPE-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000012" force_deduplication="would not rename" -group "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000006" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000006" -group "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000007" force_deduplication="would not rename" -group "different-case-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000008" force_deduplication="would rename to different-case-dupe-1-00000000-0000-0000-0000-000000000008" -group "DIFFERENT-CASE-DUPE-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000009" force_deduplication="would not rename" -group "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000004" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000004" -group "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000005" force_deduplication="would not rename" +group "different-case-DUPE-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000012" force_deduplication="would rename to different-case-DUPE-1-00000000-0000-0000-0000-000000000012" +group "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000006" force_deduplication="would not rename" +group "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000007" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000007" +group "different-case-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000008" force_deduplication="would not rename" +group "DIFFERENT-CASE-DUPE-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000009" force_deduplication="would rename to DIFFERENT-CASE-DUPE-1-00000000-0000-0000-0000-000000000009" +group "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000004" force_deduplication="would not rename" +group "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000005" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000005" end of group duplicates: end of identity duplicate report, refer to https://developer.hashicorp.com/vault/docs/upgrading/identity-deduplication for resolution.: ` @@ -178,3 +178,78 @@ func (l *identityTestWarnLogger) Warn(msg string, args ...interface{}) { } l.buf.WriteString("\n") } + +// TestDuplicateRenameResolver tests that the rename resolver +// correctly renames pre-existing entities and groups. +func TestDuplicateRenameResolver(t *testing.T) { + t.Parallel() + + entities := map[string][]string{ + // 2 non-duplicates and 2 duplicates + "root": { + "foo", + "bar", + "exact-dupe-1", "exact-dupe-1", + "different-case-dupe-1", "DIFFERENT-CASE-DUPE-1", + }, + // 1 non-duplicate and 3 duplicates + "admin": { + "foo", + "exact-dupe-1", "exact-dupe-1", + "different-case-dupe-1", "DIFFERENT-case-DUPE-1", "different-case-DUPE-1", + }, + "developers": {"BAR"}, + } + + // Create a new errorResolver + r := &renameResolver{log.NewNullLogger()} + + seenEntities := make(map[string]*identity.Entity) + seenGroups := make(map[string]*identity.Group) + + for ns, entityList := range entities { + for i, name := range entityList { + + id := fmt.Sprintf("00000000-0000-0000-0000-%012d", i) + // Create a new entity with the name/ns pair + entity := &identity.Entity{ + ID: id, + Name: name, + NamespaceID: ns, + } + + // Simulate a MemDB lookup + existingEntity := seenEntities[name] + err := r.ResolveEntities(context.Background(), existingEntity, entity) + require.NoError(t, err) + + if existingEntity != nil { + require.Equal(t, name+"-"+id, entity.Name) + require.Equal(t, existingEntity.ID, entity.Metadata["duplicate_of_canonical_id"]) + } else { + seenEntities[name] = entity + } + + // Also, since the data model is the same, pretend these are groups too + group := &identity.Group{ + ID: id, + Name: name, + NamespaceID: ns, + } + + // More MemDB mocking + existingGroup := seenGroups[name] + err = r.ResolveGroups(context.Background(), existingGroup, group) + require.NoError(t, err) + + if existingGroup != nil { + require.Equal(t, name+"-"+id, group.Name) + require.Equal(t, existingGroup.ID, group.Metadata["duplicate_of_canonical_id"]) + } else { + seenGroups[name] = group + } + } + } + + // No need to test entity alias merges here, since that's handled separately. +} diff --git a/vault/identity_store_entities.go b/vault/identity_store_entities.go index 5c6e091bb24a..8f668023f54c 100644 --- a/vault/identity_store_entities.go +++ b/vault/identity_store_entities.go @@ -1038,12 +1038,16 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit if err != nil { return nil, fmt.Errorf("aborting entity merge - failed to delete orphaned alias %q during merge into entity %q: %w", toAliasId, toEntity.ID, err), nil } + // Remove the alias from the entity's list in memory too! + toEntity.DeleteAliasByID(toAliasId) } else if strutil.StrListContains(conflictingAliasIDsToKeep, toAliasId) { i.logger.Info("Deleting from_entity alias during entity merge", "from_entity", fromEntityID, "deleted_alias", fromAlias.ID) err := i.MemDBDeleteAliasByIDInTxn(txn, fromAlias.ID, false) if err != nil { return nil, fmt.Errorf("aborting entity merge - failed to delete orphaned alias %q during merge into entity %q: %w", fromAlias.ID, toEntity.ID, err), nil } + // Remove the alias from the entity's list in memory too! + toEntity.DeleteAliasByID(toAliasId) // Continue to next alias, as there's no alias to merge left in the from_entity continue @@ -1053,6 +1057,8 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit if err != nil { return nil, fmt.Errorf("aborting entity merge - failed to delete orphaned alias %q during merge into entity %q: %w", toAliasId, toEntity.ID, err), nil } + // Remove the alias from the entity's list in memory too! + toEntity.DeleteAliasByID(toAliasId) } else { return fmt.Errorf("conflicting mount accessors in following alias IDs and neither were present in conflicting_alias_ids_to_keep: %s, %s", fromAlias.ID, toAliasId), nil, nil } diff --git a/vault/identity_store_test.go b/vault/identity_store_test.go index 7ad41261ed13..2eee8ad7d9b3 100644 --- a/vault/identity_store_test.go +++ b/vault/identity_store_test.go @@ -21,6 +21,7 @@ import ( credGithub "github.com/hashicorp/vault/builtin/credential/github" "github.com/hashicorp/vault/builtin/credential/userpass" credUserpass "github.com/hashicorp/vault/builtin/credential/userpass" + "github.com/hashicorp/vault/helper/activationflags" "github.com/hashicorp/vault/helper/identity" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/helper/storagepacker" @@ -1411,14 +1412,27 @@ func TestIdentityStoreInvalidate_TemporaryEntity(t *testing.T) { assert.Nil(t, item) } -// TestEntityStoreLoadingIsDeterministic is a property-based test that ensures -// the loading logic of the entity store is deterministic. This is important -// because we perform certain merges and corrections of duplicates on load and -// non-deterministic order can cause divergence between different nodes or even -// after seal/unseal cycles on one node. Loading _should_ be deterministic -// anyway if all data in storage was correct see comments inline for examples of -// ways storage can be corrupt with respect to the expected schema invariants. -func TestEntityStoreLoadingIsDeterministic(t *testing.T) { +// TestIdentityStoreLoadingIsDeterministic tests the default error resolver and +// the identity cleanup rename resolver to ensure that loading is deterministic +// for both. +func TestIdentityStoreLoadingIsDeterministic(t *testing.T) { + t.Run(t.Name()+"error-resolver", func(t *testing.T) { + identityStoreLoadingIsDeterministic(t, false) + }) + t.Run(t.Name()+"identity-cleanup", func(t *testing.T) { + identityStoreLoadingIsDeterministic(t, true) + }) +} + +// identityStoreLoadingIsDeterministic is a property-based test helper that +// ensures the loading logic of the entity store is deterministic. This is +// important because we perform certain merges and corrections of duplicates on +// load and non-deterministic order can cause divergence between different nodes +// or even after seal/unseal cycles on one node. Loading _should_ be +// deterministic anyway if all data in storage was correct see comments inline +// for examples of ways storage can be corrupt with respect to the expected +// schema invariants. +func identityStoreLoadingIsDeterministic(t *testing.T, identityDeduplication bool) { // Create some state in store that could trigger non-deterministic behavior. // The nature of the identity store schema is such that the order of loading // entities etc shouldn't matter even if it was non-deterministic, however due @@ -1456,6 +1470,11 @@ func TestEntityStoreLoadingIsDeterministic(t *testing.T) { ctx := context.Background() + if identityDeduplication { + err = c.FeatureActivationFlags.Write(ctx, activationflags.IdentityDeduplication, true) + require.NoError(t, err) + } + // We create 100 entities each with 1 non-local alias and 1 local alias. We // then randomly create duplicate alias or local alias entries with a // probability that is unrealistic but ensures we have duplicates on every @@ -1626,9 +1645,9 @@ func TestEntityStoreLoadingIsDeterministic(t *testing.T) { } } -// TestEntityStoreLoadingDuplicateReporting tests the reporting of different +// TestIdentityStoreLoadingDuplicateReporting tests the reporting of different // types of duplicates during unseal when in case-sensitive mode. -func TestEntityStoreLoadingDuplicateReporting(t *testing.T) { +func TestIdentityStoreLoadingDuplicateReporting(t *testing.T) { logger := corehelpers.NewTestLogger(t) ims, err := inmem.NewTransactionalInmemHA(nil, logger) require.NoError(t, err) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 602f34016e3e..b5f83d4f49b8 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -17,15 +17,17 @@ import ( memdb "github.com/hashicorp/go-memdb" "github.com/hashicorp/go-secure-stdlib/strutil" uuid "github.com/hashicorp/go-uuid" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/anypb" + "google.golang.org/protobuf/types/known/timestamppb" + + "github.com/hashicorp/vault/helper/activationflags" "github.com/hashicorp/vault/helper/identity" "github.com/hashicorp/vault/helper/identity/mfa" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/helper/storagepacker" "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/logical" - "github.com/stretchr/testify/require" - "google.golang.org/protobuf/types/known/anypb" - "google.golang.org/protobuf/types/known/timestamppb" ) var ( @@ -33,54 +35,73 @@ var ( tmpSuffix = ".tmp" ) +// loadIdentityStoreArtifacts is responsible for loading entities, groups, and aliases from storage into MemDB. func (c *Core) loadIdentityStoreArtifacts(ctx context.Context) error { if c.identityStore == nil { c.logger.Warn("identity store is not setup, skipping loading") return nil } - // Resolve all conflicts by logging a warning and returning - // errDuplicateIdentityName. The error will flip the identityStore into - // case-sensitive mode by switching the underlying schema to one with a - // relaxed lowerCase constraint and reload all artifacts into MemDB. - c.identityStore.conflictResolver = &errorResolver{c.identityStore.logger} - loadFunc := func(context.Context) error { if err := c.identityStore.loadEntities(ctx); err != nil { - return err + return fmt.Errorf("failed to load entities: %w", err) } if err := c.identityStore.loadGroups(ctx); err != nil { - return err + return fmt.Errorf("failed to load groups: %w", err) } if err := c.identityStore.loadOIDCClients(ctx); err != nil { - return err + return fmt.Errorf("failed to load OIDC clients: %w", err) } if err := c.identityStore.loadCachedEntitiesOfLocalAliases(ctx); err != nil { - return err + return fmt.Errorf("failed to load cached local alias entities: %w", err) } return nil } - // Load everything when memdb is set to operate on lower cased names + // Resolve all conflicts by logging a warning and returning + // errDuplicateIdentityName by default. The error will flip the + // identityStore into case-sensitive mode by switching the underlying + // schema to one with a relaxed lowerCase constraint and reload all + // artifacts into MemDB. + c.identityStore.conflictResolver = &errorResolver{c.identityStore.logger} + + // If the identity deduplication cleanup flag is activated, instead + // deal with duplicate entities and groups by renaming with a -UUID + // suffix. N.B. *entity alias* duplicates will still be merged as before. + if c.FeatureActivationFlags.IsActivationFlagEnabled(activationflags.IdentityDeduplication) { + c.identityStore.conflictResolver = &renameResolver{c.identityStore.logger} + } + + // Load everything when MemDB is set to operate on lower cased names. + // errDuplicateIdentityName below should only happen if we're using the + // errorResolver (i.e. identity deduplication is not activated) and we + // encounter non-alias duplicates. err := loadFunc(ctx) switch { case err == nil: - // If it succeeds, all is well + // No error implies we've loaded the artifacts successfully + // with no duplicates detected. This means there were no + // unmerged duplicates detected while loading with the + // errorResolver, or the renameResolver was activated and + // resolved all duplicates. In either case, we can return + // early, since there's nothing left to do. return nil case !errwrap.Contains(err, errDuplicateIdentityName.Error()): + // All other errors are unexpected and should be returned. return err } + // If we're here, it means we've encountered duplicates while loading + // with the errorResolver. c.identityStore.logger.Warn("enabling case sensitive identity names") // Set identity store to operate on case sensitive identity names c.identityStore.disableLowerCasedNames = true - // Swap the memdb instance by the one which operates on case sensitive - // names, hence obviating the need to unload anything that's already - // loaded. - if err := c.identityStore.resetDB(ctx); err != nil { + // Swap out the MemDB instance and reload artifacts with the + // new schema. + if err := c.identityStore.resetDB(); err != nil { return err } @@ -339,7 +360,6 @@ func (i *IdentityStore) loadCachedEntitiesOfLocalAliases(ctx context.Context) er return err } nsCtx := namespace.ContextWithNamespace(ctx, ns) - err = i.upsertEntity(nsCtx, entity, nil, false) if err != nil { return fmt.Errorf("failed to update entity in MemDB: %w", err) @@ -1857,7 +1877,7 @@ func (i *IdentityStore) UpsertGroup(ctx context.Context, group *identity.Group, txn := i.db.Txn(true) defer txn.Abort() - err := i.UpsertGroupInTxn(ctx, txn, group, true) + err := i.UpsertGroupInTxn(ctx, txn, group, persist) if err != nil { return err } @@ -2695,6 +2715,7 @@ func attachAlias(t *testing.T, e *identity.Entity, name string, me *MountEntry) if e.NamespaceID != me.NamespaceID { panic("mount and entity in different namespaces") } + require.NoError(t, err) a := &identity.Alias{ ID: id, Name: name, diff --git a/vault/logical_system_activation_flags.go b/vault/logical_system_activation_flags.go index 09dd2fbb3556..7cd1cc6116a4 100644 --- a/vault/logical_system_activation_flags.go +++ b/vault/logical_system_activation_flags.go @@ -9,6 +9,7 @@ import ( "slices" "strings" + "github.com/hashicorp/vault/helper/activationflags" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" ) @@ -35,8 +36,7 @@ This path responds to the following HTTP methods. PUT|POST //activate Activates the specified feature. Cannot be undone.` - activationFlagIdentityCleanup = "force-identity-deduplication" - activationFlagTest = "activation-test" + activationFlagTest = "activation-test" ) // These variables should only be mutated during initialization or server construction. @@ -86,7 +86,7 @@ func (b *SystemBackend) activationFlagsPaths() []*framework.Path { HelpDescription: helpDescription, }, { - Pattern: fmt.Sprintf("%s/%s/%s", prefixActivationFlags, activationFlagIdentityCleanup, verbActivationFlagsActivate), + Pattern: fmt.Sprintf("%s/%s/%s", prefixActivationFlags, activationflags.IdentityDeduplication, verbActivationFlagsActivate), DisplayAttrs: &framework.DisplayAttributes{ OperationPrefix: prefixActivationFlags, OperationVerb: verbActivationFlagsActivate, diff --git a/vault/logical_system_activation_flags_test.go b/vault/logical_system_activation_flags_test.go index 95834a88e001..3aedbdcbc986 100644 --- a/vault/logical_system_activation_flags_test.go +++ b/vault/logical_system_activation_flags_test.go @@ -10,6 +10,7 @@ import ( "fmt" "testing" + "github.com/hashicorp/vault/helper/activationflags" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/logical" "github.com/stretchr/testify/require" @@ -94,7 +95,7 @@ func TestActivationFlags_Write(t *testing.T) { context.Background(), &logical.Request{ Operation: logical.UpdateOperation, - Path: fmt.Sprintf("%s/%s/%s", prefixActivationFlags, activationFlagIdentityCleanup, verbActivationFlagsActivate), + Path: fmt.Sprintf("%s/%s/%s", prefixActivationFlags, activationflags.IdentityDeduplication, verbActivationFlagsActivate), Storage: core.systemBarrierView, }, ) @@ -103,6 +104,6 @@ func TestActivationFlags_Write(t *testing.T) { require.NotNil(t, resp) require.NotEmpty(t, resp.Data) require.NotNil(t, resp.Data["activated"]) - require.Contains(t, resp.Data["activated"], activationFlagIdentityCleanup) + require.Contains(t, resp.Data["activated"], activationflags.IdentityDeduplication) }) }