Merge Identity Entities if two claim the same alias

Past bugs/race conditions meant two entities could be created each
claiming the same alias. There are planned longer term fixes for this
(outside of the race condition being fixed in 0.10.4) that involve
changing the data model, but this is an immediate workaround that has
the same net effect: if two entities claim the same alias, assume they
were created due to this race condition and merge them.

In this situation, also automatically merge policies so we don't lose
e.g. RGPs.

helper/strutil/strutil.go

@@ -329,3 +329,20 @@ func AppendIfMissing(slice []string, i string) []string {
 	}
 	return append(slice, i)
 }
+
+// MergeSlices adds an arbitrary number of slices together, uniquely
+func MergeSlices(args ...[]string) []string {
+	all := map[string]struct{}{}
+	for _, slice := range args {
+		for _, v := range slice {
+			all[v] = struct{}{}
+		}
+	}
+
+	result := make([]string, 0, len(all))
+	for k, _ := range all {
+		result = append(result, k)
+	}
+	sort.Strings(result)
+	return result
+}

helper/strutil/strutil_test.go

@@ -449,3 +449,13 @@ func TestStrUtil_ParseStringSlice(t *testing.T) {
 		}
 	}
 }
+
+func TestStrUtil_MergeSlices(t *testing.T) {
+	res := MergeSlices([]string{"a", "c", "d"}, []string{}, []string{"c", "f", "a"}, nil, []string{"foo"})
+
+	expect := []string{"a", "c", "d", "f", "foo"}
+
+	if !reflect.DeepEqual(res, expect) {
+		t.Fatalf("expected %v, got %v", expect, res)
+	}
+}

vault/identity_store_entities.go

@@ -2,6 +2,7 @@ package vault
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -10,6 +11,7 @@ import (
 	memdb "github.com/hashicorp/go-memdb"
 	"github.com/hashicorp/vault/helper/identity"
 	"github.com/hashicorp/vault/helper/storagepacker"
+	"github.com/hashicorp/vault/helper/strutil"
 	"github.com/hashicorp/vault/logical"
 	"github.com/hashicorp/vault/logical/framework"
 )
@@ -143,9 +145,6 @@ func (i *IdentityStore) pathEntityMergeID() framework.OperationFunc {
 
 		force := d.Get("force").(bool)
 
-		i.lock.Lock()
-		defer i.lock.Unlock()
-
 		// Create a MemDB transaction to merge entities
 		txn := i.db.Txn(true)
 		defer txn.Abort()
@@ -155,85 +154,12 @@ func (i *IdentityStore) pathEntityMergeID() framework.OperationFunc {
 			return nil, err
 		}
 
-		if toEntity == nil {
-			return logical.ErrorResponse("entity id to merge to is invalid"), nil
+		userErr, intErr := i.mergeEntity(txn, toEntity, fromEntityIDs, force, true, false)
+		if userErr != nil {
+			return logical.ErrorResponse(userErr.Error()), nil
 		}
-
-		var conflictErrors error
-		for _, fromEntityID := range fromEntityIDs {
-			if fromEntityID == toEntityID {
-				return logical.ErrorResponse("to_entity_id should not be present in from_entity_ids"), nil
-			}
-
-			fromEntity, err := i.MemDBEntityByID(fromEntityID, false)
-			if err != nil {
-				return nil, err
-			}
-
-			if fromEntity == nil {
-				return logical.ErrorResponse("entity id to merge from is invalid"), nil
-			}
-
-			for _, alias := range fromEntity.Aliases {
-				// Set the desired canonical ID
-				alias.CanonicalID = toEntity.ID
-
-				alias.MergedFromCanonicalIDs = append(alias.MergedFromCanonicalIDs, fromEntity.ID)
-
-				err = i.MemDBUpsertAliasInTxn(txn, alias, false)
-				if err != nil {
-					return nil, errwrap.Wrapf("failed to update alias during merge: {{err}}", err)
-				}
-
-				// Add the alias to the desired entity
-				toEntity.Aliases = append(toEntity.Aliases, alias)
-			}
-
-			// If the entity from which we are merging from was already a merged
-			// entity, transfer over the Merged set to the entity we are
-			// merging into.
-			toEntity.MergedEntityIDs = append(toEntity.MergedEntityIDs, fromEntity.MergedEntityIDs...)
-
-			// Add the entity from which we are merging from to the list of entities
-			// the entity we are merging into is composed of.
-			toEntity.MergedEntityIDs = append(toEntity.MergedEntityIDs, fromEntity.ID)
-
-			// Delete the entity which we are merging from in MemDB using the same transaction
-			err = i.MemDBDeleteEntityByIDInTxn(txn, fromEntity.ID)
-			if err != nil {
-				return nil, err
-			}
-
-			// Delete the entity which we are merging from in storage
-			err = i.entityPacker.DeleteItem(fromEntity.ID)
-			if err != nil {
-				return nil, err
-			}
-		}
-
-		if conflictErrors != nil && !force {
-			return logical.ErrorResponse(conflictErrors.Error()), nil
-		}
-
-		// Update MemDB with changes to the entity we are merging to
-		err = i.MemDBUpsertEntityInTxn(txn, toEntity)
-		if err != nil {
-			return nil, err
-		}
-
-		// Persist the entity which we are merging to
-		toEntityAsAny, err := ptypes.MarshalAny(toEntity)
-		if err != nil {
-			return nil, err
-		}
-		item := &storagepacker.Item{
-			ID:      toEntity.ID,
-			Message: toEntityAsAny,
-		}
-
-		err = i.entityPacker.PutItem(item)
-		if err != nil {
-			return nil, err
+		if intErr != nil {
+			return nil, intErr
 		}
 
 		// Committing the transaction *after* successfully performing storage
@@ -549,6 +475,101 @@ func (i *IdentityStore) pathEntityIDList() framework.OperationFunc {
 	}
 }
 
+func (i *IdentityStore) mergeEntity(txn *memdb.Txn, toEntity *identity.Entity, fromEntityIDs []string, force, grabLock, mergePolicies bool) (error, error) {
+	if grabLock {
+		i.lock.Lock()
+		defer i.lock.Unlock()
+	}
+
+	if toEntity == nil {
+		return errors.New("entity id to merge to is invalid"), nil
+	}
+
+	var conflictErrors error
+	for _, fromEntityID := range fromEntityIDs {
+		if fromEntityID == toEntity.ID {
+			return errors.New("to_entity_id should not be present in from_entity_ids"), nil
+		}
+
+		fromEntity, err := i.MemDBEntityByID(fromEntityID, false)
+		if err != nil {
+			return nil, err
+		}
+
+		if fromEntity == nil {
+			return errors.New("entity id to merge from is invalid"), nil
+		}
+
+		for _, alias := range fromEntity.Aliases {
+			// Set the desired canonical ID
+			alias.CanonicalID = toEntity.ID
+
+			alias.MergedFromCanonicalIDs = append(alias.MergedFromCanonicalIDs, fromEntity.ID)
+
+			err = i.MemDBUpsertAliasInTxn(txn, alias, false)
+			if err != nil {
+				return nil, errwrap.Wrapf("failed to update alias during merge: {{err}}", err)
+			}
+
+			// Add the alias to the desired entity
+			toEntity.Aliases = append(toEntity.Aliases, alias)
+		}
+
+		// If told to, merge policies
+		if mergePolicies {
+			toEntity.Policies = strutil.MergeSlices(toEntity.Policies, fromEntity.Policies)
+		}
+
+		// If the entity from which we are merging from was already a merged
+		// entity, transfer over the Merged set to the entity we are
+		// merging into.
+		toEntity.MergedEntityIDs = append(toEntity.MergedEntityIDs, fromEntity.MergedEntityIDs...)
+
+		// Add the entity from which we are merging from to the list of entities
+		// the entity we are merging into is composed of.
+		toEntity.MergedEntityIDs = append(toEntity.MergedEntityIDs, fromEntity.ID)
+
+		// Delete the entity which we are merging from in MemDB using the same transaction
+		err = i.MemDBDeleteEntityByIDInTxn(txn, fromEntity.ID)
+		if err != nil {
+			return nil, err
+		}
+
+		// Delete the entity which we are merging from in storage
+		err = i.entityPacker.DeleteItem(fromEntity.ID)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	if conflictErrors != nil && !force {
+		return conflictErrors, nil
+	}
+
+	// Update MemDB with changes to the entity we are merging to
+	err := i.MemDBUpsertEntityInTxn(txn, toEntity)
+	if err != nil {
+		return nil, err
+	}
+
+	// Persist the entity which we are merging to
+	toEntityAsAny, err := ptypes.MarshalAny(toEntity)
+	if err != nil {
+		return nil, err
+	}
+	item := &storagepacker.Item{
+		ID:      toEntity.ID,
+		Message: toEntityAsAny,
+	}
+
+	err = i.entityPacker.PutItem(item)
+	if err != nil {
+		return nil, err
+	}
+
+	return nil, nil
+}
+
 var entityHelp = map[string][2]string{
 	"entity": {
 		"Create a new entity",

vault/identity_store_test.go

@@ -5,8 +5,12 @@ import (
 	"testing"
 	"time"
 
+	"github.com/go-test/deep"
+	"github.com/golang/protobuf/ptypes"
 	uuid "github.com/hashicorp/go-uuid"
 	credGithub "github.com/hashicorp/vault/builtin/credential/github"
+	"github.com/hashicorp/vault/helper/identity"
+	"github.com/hashicorp/vault/helper/storagepacker"
 	"github.com/hashicorp/vault/logical"
 )
 
@@ -296,6 +300,121 @@ func TestIdentityStore_TokenEntityInheritance(t *testing.T) {
 	}
 }
 
+func TestIdentityStore_MergeConflictingAliases(t *testing.T) {
+	err := AddTestCredentialBackend("github", credGithub.Factory)
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	c, unsealKey, root := TestCoreUnsealed(t)
+
+	meGH := &MountEntry{
+		Table:       credentialTableType,
+		Path:        "github/",
+		Type:        "github",
+		Description: "github auth",
+	}
+
+	err = c.enableCredential(context.Background(), meGH)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	alias := &identity.Alias{
+		ID:            "alias1",
+		CanonicalID:   "entity1",
+		MountType:     "github",
+		MountAccessor: meGH.Accessor,
+		Name:          "githubuser",
+	}
+	entity := &identity.Entity{
+		ID:       "entity1",
+		Name:     "name1",
+		Policies: []string{"foo", "bar"},
+		Aliases: []*identity.Alias{
+			alias,
+		},
+	}
+	entity.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity.ID)
+
+	// Now add the alias to two entities, skipping all existing checking by
+	// writing directly
+	entityAny, err := ptypes.MarshalAny(entity)
+	if err != nil {
+		t.Fatal(err)
+	}
+	item := &storagepacker.Item{
+		ID:      entity.ID,
+		Message: entityAny,
+	}
+	if err = c.identityStore.entityPacker.PutItem(item); err != nil {
+		t.Fatal(err)
+	}
+
+	entity.ID = "entity2"
+	entity.Name = "name2"
+	entity.Policies = []string{"bar", "baz"}
+	entity.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity.ID)
+	entityAny, err = ptypes.MarshalAny(entity)
+	if err != nil {
+		t.Fatal(err)
+	}
+	item = &storagepacker.Item{
+		ID:      entity.ID,
+		Message: entityAny,
+	}
+	if err = c.identityStore.entityPacker.PutItem(item); err != nil {
+		t.Fatal(err)
+	}
+
+	// Seal and unseal. If things are broken, we will now fail to unseal.
+	if err = c.Seal(root); err != nil {
+		t.Fatal(err)
+	}
+
+	var unsealed bool
+	for i := 0; i < 3; i++ {
+		unsealed, err = c.Unseal(unsealKey[i])
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
+	if !unsealed {
+		t.Fatal("still sealed")
+	}
+
+	newEntity, err := c.identityStore.CreateOrFetchEntity(&logical.Alias{
+		MountAccessor: meGH.Accessor,
+		MountType:     "github",
+		Name:          "githubuser",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if newEntity == nil {
+		t.Fatal("nil new entity")
+	}
+
+	entityToUse := "entity1"
+	if newEntity.ID == "entity1" {
+		entityToUse = "entity2"
+	}
+	if len(newEntity.MergedEntityIDs) != 1 || newEntity.MergedEntityIDs[0] != entityToUse {
+		t.Fatalf("bad merged entity ids: %v", newEntity.MergedEntityIDs)
+	}
+	if diff := deep.Equal(newEntity.Policies, []string{"bar", "baz", "foo"}); diff != nil {
+		t.Fatal(diff)
+	}
+
+	newEntity, err = c.identityStore.MemDBEntityByID(entityToUse, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if newEntity != nil {
+		t.Fatal("got a non-nil entity")
+	}
+}
+
 func testCoreWithIdentityTokenGithub(t *testing.T) (*Core, *IdentityStore, *TokenStore, string) {
 	is, ghAccessor, core := testIdentityStoreWithGithubAuth(t)
 	return core, is, core.tokenStore, ghAccessor