Skip to content

Commit

Permalink
identity: Resolve conflicts with rename (#29356)
Browse files Browse the repository at this point in the history 
This PR introduces a new type of conflict resolution for duplicate
Entities and Groups. Renaming provides a way of preventing Vault from
entering case-sensitive mode, which is the current behavior for any kind
of duplicate.

Renames append the conflicting identity artifact's UUID to its name and
updates a metadata field to indicate the pre-existing artifact's UUID.

The feature is gated by the force-identity-deduplication activation flag.

In order to maintain consistent behavior between the reporting resolver
and the rename operation, we need to adjust the behavior of generated
reports. Previously, they intentionally preserved existing Group merge
determinism, wherein the last MemDB update would win and all others
would be renamed. This approach is more complicated for the rename
resolver, since we would need to update any duplicated entity in the
cache while inserting the new duplicate (resulting in two MemDB
operations). Though we can ensure atomic updates of the two identity
artifacts with transactions (which we could get for groups with a minor
adjustment, and we will get along with batching of Entity upserts on 
load), it's far simpler to just rename all but the first insert as proposed
in the current PR.

Since the feature is gated by an activation flag with appropriate 
warnings of potential changes via the reporting resolver, we opt
for simplicity over maintaining pre-existing behavior. We can revisit
this assumption later if we think alignment with existing behavior
outweighs any potential complexity in the rename operation.

Entity alias resolution is left alone as a destructive merge operation
to prevent a potentially high-impact change in existing behavior.
  • Loading branch information
@mpalmi
mpalmi authored Jan 15, 2025
1 parent a0ecbe9 commit f503f73
Show file tree
Hide file tree
Showing 11 changed files with 275 additions and 66 deletions.
5 changes: 5 additions & 0 deletions changelog/29356.txt
View file
Original file line number Diff line number Diff line change
@@ -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.
```
1 change: 1 addition & 0 deletions helper/activationflags/activation_flags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

const (
storagePathActivationFlags = "activation-flags"
IdentityDeduplication = "force-identity-deduplication"
)

type FeatureActivationFlags struct {
Expand Down
16 changes: 16 additions & 0 deletions helper/identity/identity.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
4 changes: 2 additions & 2 deletions vault/identity_store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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
}
Expand Down
75 changes: 70 additions & 5 deletions vault/identity_store_conflicts.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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"
}
Expand Down Expand Up @@ -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
}
121 changes: 98 additions & 23 deletions vault/identity_store_conflicts_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.:
`
Expand Down Expand Up @@ -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.
}
6 changes: 6 additions & 0 deletions vault/identity_store_entities.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
}
Expand Down
Loading

0 comments on commit f503f73

Please sign in to comment.