Add check to entity merge, other review fixes

changelog/12747.txt

@@ -1,3 +1,3 @@
 ```release-note:bug
-core/identity: Disallow entity alias creation when an alias for the specified entity and mount exists
+core/identity: Disallow entity alias creation/update if a conflicting alias exists for the target entity and mount combination
 ```

vault/identity_store_aliases.go

@@ -267,8 +267,8 @@ func (i *IdentityStore) handleAliasCreate(ctx context.Context, req *logical.Requ
 		}
 	}
 
-	for _, alias := range entity.Aliases {
-		if alias.MountAccessor == mountAccessor {
+	for _, currentAlias := range entity.Aliases {
+		if currentAlias.MountAccessor == mountAccessor {
 			return logical.ErrorResponse("Alias already exists for requested entity and mount accessor"), nil
 		}
 	}
@@ -329,15 +329,15 @@ func (i *IdentityStore) handleAliasUpdate(ctx context.Context, req *logical.Requ
 	}
 
 	if currentEntity.NamespaceID != alias.NamespaceID {
-		return logical.ErrorResponse("alias associated with an entity in a different namespace"), logical.ErrPermissionDenied
+		return logical.ErrorResponse("alias and entity do not belong to the same namespace"), logical.ErrPermissionDenied
 	}
 
 	// If the accessor is being changed but the entity is not, check if the entity
 	// already has an alias corresponding to the new accessor
 	if mountAccessor != alias.MountAccessor && (canonicalID == "" || canonicalID == alias.CanonicalID) {
 		for _, currentAlias := range currentEntity.Aliases {
 			if currentAlias.MountAccessor == mountAccessor {
-				return logical.ErrorResponse("Alias cannot be updated as the current entity already has an alias for the given 'mount_accessor' "), nil
+				return logical.ErrorResponse("Alias cannot be updated as the entity already has an alias for the given 'mount_accessor' "), nil
 			}
 		}
 	}
@@ -389,7 +389,7 @@ func (i *IdentityStore) handleAliasUpdate(ctx context.Context, req *logical.Requ
 		// Check if the entity the alias is being updated to, already has an alias for the mount
 		for _, alias := range newEntity.Aliases {
 			if alias.MountAccessor == mountAccessor {
-				return logical.ErrorResponse("Alias cannot be updated as the given 'canonical_id' already has an alias for this mount "), nil
+				return logical.ErrorResponse("Alias cannot be updated as the given entity already has an alias for this mount "), nil
 			}
 		}
 

vault/identity_store_aliases_test.go

@@ -407,7 +407,7 @@ func TestIdentityStore_AliasUpdate(t *testing.T) {
 
 // Test to check that the alias cannot be updated with a new entity
 // which already has an alias for the mount on the alias to be updated
-func TestIdentityStore_AliasUpdate_InvalidEntity(t *testing.T) {
+func TestIdentityStore_AliasMove_DuplicateAccessor(t *testing.T) {
 	var err error
 	var resp *logical.Response
 	ctx := namespace.RootContext(nil)
@@ -492,7 +492,7 @@ func TestIdentityStore_AliasUpdate_InvalidEntity(t *testing.T) {
 
 // Test that the alias cannot be changed to a mount for which
 // the entity already has an alias
-func TestIdentityStore_AliasUpdate__DuplicateAccessor(t *testing.T) {
+func TestIdentityStore_AliasUpdate_DuplicateAccessor(t *testing.T) {
 	var err error
 	var resp *logical.Response
 	ctx := namespace.RootContext(nil)

vault/identity_store_entities.go

@@ -768,6 +768,15 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit
 	isPerfSecondaryOrStandby := i.localNode.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) ||
 		i.localNode.HAState() == consts.PerfStandby
 	var fromEntityGroups []*identity.Group
+
+	toEntityAccessors := make(map[string]struct{})
+
+	for _, alias := range toEntity.Aliases {
+		if _, ok := toEntityAccessors[alias.MountAccessor]; !ok {
+			toEntityAccessors[alias.MountAccessor] = struct{}{}
+		}
+	}
+
 	for _, fromEntityID := range fromEntityIDs {
 		if fromEntityID == toEntity.ID {
 			return errors.New("to_entity_id should not be present in from_entity_ids"), nil
@@ -797,6 +806,10 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit
 				return nil, fmt.Errorf("failed to update alias during merge: %w", err)
 			}
 
+			if _, ok := toEntityAccessors[alias.MountAccessor]; ok {
+				i.logger.Warn("skipping from_entity alias which has an accessor on which to_entity has an alias", "from_entity", fromEntityID, "skipped_alias", alias.ID)
+				continue
+			}
 			// Add the alias to the desired entity
 			toEntity.Aliases = append(toEntity.Aliases, alias)
 		}

vault/identity_store_entities_test.go

@@ -988,7 +988,7 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
 	var resp *logical.Response
 
 	ctx := namespace.RootContext(nil)
-	is, githubAccessor, _ := testIdentityStoreWithGithubAuth(ctx, t)
+	is, githubAccessor, upAccessor, _ := testIdentityStoreWithGithubUserpassAuth(ctx, t)
 
 	registerData := map[string]interface{}{
 		"name":     "testentityname2",
@@ -1012,6 +1012,12 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
 		"metadata":       []string{"organization=hashicorp", "team=vault"},
 	}
 
+	aliasRegisterData3 := map[string]interface{}{
+		"name":           "testaliasname3",
+		"mount_accessor": upAccessor,
+		"metadata":       []string{"organization=hashicorp", "team=vault"},
+	}
+
 	registerReq := &logical.Request{
 		Operation: logical.UpdateOperation,
 		Path:      "entity",
@@ -1088,6 +1094,15 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
 		t.Fatalf("err:%v resp:%#v", err, resp)
 	}
 
+	aliasRegisterData3["entity_id"] = entityID2
+
+	aliasReq.Data = aliasRegisterData3
+
+	// Register the alias
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
 	entity2, err := is.MemDBEntityByID(entityID2, false)
 	if err != nil {
 		t.Fatal(err)
@@ -1096,8 +1111,8 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
 		t.Fatalf("failed to create entity: %v", err)
 	}
 
-	if len(entity2.Aliases) != 1 {
-		t.Fatalf("bad: number of aliases in entity; expected: 1, actual: %d", len(entity2.Aliases))
+	if len(entity2.Aliases) != 2 {
+		t.Fatalf("bad: number of aliases in entity; expected: 2, actual: %d", len(entity2.Aliases))
 	}
 
 	entity2GroupReq := &logical.Request{
@@ -1151,6 +1166,7 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
 		t.Fatalf("bad: number of aliases in entity; expected: 2, actual: %d", len(entity1Aliases))
 	}
 
+	githubAliases := 0
 	for _, aliasRaw := range entity1Aliases {
 		alias := aliasRaw.(map[string]interface{})
 		aliasLookedUp, err := is.MemDBAliasByID(alias["id"].(string), false, false)
@@ -1160,6 +1176,15 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
 		if aliasLookedUp == nil {
 			t.Fatalf("index for alias id %q is not updated", alias["id"].(string))
 		}
+		if aliasLookedUp.MountAccessor == githubAccessor {
+			githubAliases += 1
+		}
+	}
+
+	// Test that only 1 alias for the githubAccessor is present in the merged entity,
+	// as the github alias on entity2 should've been skipped in the merge
+	if githubAliases != 1 {
+		t.Fatalf("Unexcepted number of github aliases in merged entity; expected: 1, actual: %d", githubAliases)
 	}
 
 	entity1Groups := resp.Data["direct_group_ids"].([]string)