[VAULT-3252] Disallow alias creation if entity/accessor combination exists

changelog/12747.txt

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

vault/core_metrics_test.go

@@ -259,7 +259,7 @@ func metricLabelsMatch(t *testing.T, actual []metrics.Label, expected map[string
 
 func TestCoreMetrics_EntityGauges(t *testing.T) {
 	ctx := namespace.RootContext(nil)
-	is, ghAccessor, core := testIdentityStoreWithGithubAuth(ctx, t)
+	is, ghAccessor, upAccessor, core := testIdentityStoreWithGithubUserpassAuth(ctx, t)
 
 	// Create an entity
 	alias1 := &logical.Alias{
@@ -278,12 +278,11 @@ func TestCoreMetrics_EntityGauges(t *testing.T) {
 		Operation: logical.UpdateOperation,
 		Path:      "entity-alias",
 		Data: map[string]interface{}{
-			"name":           "githubuser2",
+			"name":           "userpassuser",
 			"canonical_id":   entity.ID,
-			"mount_accessor": ghAccessor,
+			"mount_accessor": upAccessor,
 		},
 	}
-
 	resp, err := is.HandleRequest(ctx, registerReq)
 	if err != nil || (resp != nil && resp.IsError()) {
 		t.Fatalf("err:%v resp:%#v", err, resp)
@@ -312,12 +311,16 @@ func TestCoreMetrics_EntityGauges(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
-	if len(glv) != 1 {
+	if len(glv) != 2 {
 		t.Fatalf("Wrong number of gauges %v, expected %v", len(glv), 1)
 	}
 
-	if glv[0].Value != 2.0 {
-		t.Errorf("Alias count %v, expected %v", glv[0].Value, 2.0)
+	if glv[0].Value != 1.0 {
+		t.Errorf("Alias count %v, expected %v", glv[0].Value, 1.0)
+	}
+
+	if glv[1].Value != 1.0 {
+		t.Errorf("Alias count %v, expected %v", glv[0].Value, 1.0)
 	}
 
 	metricLabelsMatch(t, glv[0].Labels,
@@ -326,4 +329,11 @@ func TestCoreMetrics_EntityGauges(t *testing.T) {
 			"auth_method": "github",
 			"mount_point": "auth/github/",
 		})
+
+	metricLabelsMatch(t, glv[1].Labels,
+		map[string]string{
+			"namespace":   "root",
+			"auth_method": "userpass",
+			"mount_point": "auth/userpass/",
+		})
 }

vault/identity_store_aliases.go

@@ -267,6 +267,12 @@ func (i *IdentityStore) handleAliasCreate(ctx context.Context, req *logical.Requ
 		}
 	}
 
+	for _, currentAlias := range entity.Aliases {
+		if currentAlias.MountAccessor == mountAccessor {
+			return logical.ErrorResponse("Alias already exists for requested entity and mount accessor"), nil
+		}
+	}
+
 	entity.Aliases = append(entity.Aliases, alias)
 
 	// ID creation and other validations; This is more useful for new entities
@@ -312,6 +318,29 @@ func (i *IdentityStore) handleAliasUpdate(ctx context.Context, req *logical.Requ
 
 	alias.LastUpdateTime = ptypes.TimestampNow()
 
+	// Get our current entity, which may be the same as the new one if the
+	// canonical ID hasn't changed
+	currentEntity, err := i.MemDBEntityByAliasID(alias.ID, true)
+	if err != nil {
+		return nil, err
+	}
+	if currentEntity == nil {
+		return logical.ErrorResponse("given alias is not associated with an entity"), nil
+	}
+
+	if currentEntity.NamespaceID != alias.NamespaceID {
+		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 entity already has an alias for the given 'mount_accessor' "), nil
+			}
+		}
+	}
 	// If we're changing one or the other or both of these, make sure that
 	// there isn't a matching alias already, and make sure it's in the same
 	// namespace.
@@ -343,18 +372,6 @@ func (i *IdentityStore) handleAliasUpdate(ctx context.Context, req *logical.Requ
 		alias.MountAccessor = mountAccessor
 		alias.CustomMetadata = customMetadata
 	}
-	// Get our current entity, which may be the same as the new one if the
-	// canonical ID hasn't changed
-	currentEntity, err := i.MemDBEntityByAliasID(alias.ID, true)
-	if err != nil {
-		return nil, err
-	}
-	if currentEntity == nil {
-		return logical.ErrorResponse("given alias is not associated with an entity"), nil
-	}
-	if currentEntity.NamespaceID != alias.NamespaceID {
-		return logical.ErrorResponse("alias associated with an entity in a different namespace"), logical.ErrPermissionDenied
-	}
 
 	newEntity := currentEntity
 	if canonicalID != "" && canonicalID != alias.CanonicalID {
@@ -369,6 +386,13 @@ func (i *IdentityStore) handleAliasUpdate(ctx context.Context, req *logical.Requ
 			return logical.ErrorResponse("given 'canonical_id' associated with entity in a different namespace from the alias"), logical.ErrPermissionDenied
 		}
 
+		// 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 entity already has an alias for this mount "), nil
+			}
+		}
+
 		// Update the canonical ID value and move it from the current entity to the new one
 		alias.CanonicalID = newEntity.ID
 		newEntity.Aliases = append(newEntity.Aliases, alias)

vault/identity_store_aliases_test.go

@@ -405,6 +405,218 @@ 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_AliasMove_DuplicateAccessor(t *testing.T) {
+	var err error
+	var resp *logical.Response
+	ctx := namespace.RootContext(nil)
+	is, githubAccessor, _ := testIdentityStoreWithGithubAuth(ctx, t)
+
+	// Create 2 entities and 1 alias on each, against the same github mount
+	resp, err = is.HandleRequest(ctx, &logical.Request{
+		Path:      "entity",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": "testentity1",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	entity1ID := resp.Data["id"].(string)
+
+	alias1Data := map[string]interface{}{
+		"name":           "testaliasname1",
+		"mount_accessor": githubAccessor,
+		"canonical_id":   entity1ID,
+	}
+
+	aliasReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "entity-alias",
+		Data:      alias1Data,
+	}
+
+	// This will create an alias against the requested entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+
+	resp, err = is.HandleRequest(ctx, &logical.Request{
+		Path:      "entity",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": "testentity2",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	entity2ID := resp.Data["id"].(string)
+
+	alias2Data := map[string]interface{}{
+		"name":           "testaliasname2",
+		"mount_accessor": githubAccessor,
+		"canonical_id":   entity2ID,
+	}
+
+	aliasReq.Data = alias2Data
+
+	// This will create an alias against the requested entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+	alias2ID := resp.Data["id"].(string)
+
+	// Attempt to update the second alias to point to the first entity
+	updateData := map[string]interface{}{
+		"canonical_id": entity1ID,
+	}
+
+	aliasReq.Data = updateData
+	aliasReq.Path = "entity-alias/id/" + alias2ID
+	resp, err = is.HandleRequest(ctx, aliasReq)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if resp == nil || !resp.IsError() {
+		t.Fatalf("expected an error as alias on the github accessor exists for testentity1")
+	}
+
+}
+
+// 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) {
+	var err error
+	var resp *logical.Response
+	ctx := namespace.RootContext(nil)
+
+	is, ghAccessor, upAccessor, _ := testIdentityStoreWithGithubUserpassAuth(ctx, t)
+
+	// Create 1 entity and 2 aliases on it, one for each mount
+	resp, err = is.HandleRequest(ctx, &logical.Request{
+		Path:      "entity",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": "testentity",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	entityID := resp.Data["id"].(string)
+
+	alias1Data := map[string]interface{}{
+		"name":           "testaliasname1",
+		"mount_accessor": ghAccessor,
+		"canonical_id":   entityID,
+	}
+
+	aliasReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "entity-alias",
+		Data:      alias1Data,
+	}
+
+	// This will create an alias against the requested entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+
+	alias2Data := map[string]interface{}{
+		"name":           "testaliasname2",
+		"mount_accessor": upAccessor,
+		"canonical_id":   entityID,
+	}
+
+	aliasReq.Data = alias2Data
+
+	// This will create an alias against the requested entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+	alias2ID := resp.Data["id"].(string)
+
+	// Attempt to update the userpass mount to point to the github mount
+	updateData := map[string]interface{}{
+		"mount_accessor": ghAccessor,
+	}
+
+	aliasReq.Data = updateData
+	aliasReq.Path = "entity-alias/id/" + alias2ID
+	resp, err = is.HandleRequest(ctx, aliasReq)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if resp == nil || !resp.IsError() {
+		t.Fatalf("expected an error as an alias on the github accessor already exists for testentity")
+	}
+
+}
+
+// Test that alias creation fails if an alias for the specified mount
+// and entity has already been created
+func TestIdentityStore_AliasCreate_DuplicateAccessor(t *testing.T) {
+	var err error
+	var resp *logical.Response
+	ctx := namespace.RootContext(nil)
+	is, githubAccessor, _ := testIdentityStoreWithGithubAuth(ctx, t)
+
+	resp, err = is.HandleRequest(ctx, &logical.Request{
+		Path:      "entity",
+		Operation: logical.UpdateOperation,
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	entityID := resp.Data["id"].(string)
+
+	aliasData := map[string]interface{}{
+		"name":           "testaliasname",
+		"mount_accessor": githubAccessor,
+		"canonical_id":   entityID,
+	}
+
+	aliasReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "entity-alias",
+		Data:      aliasData,
+	}
+
+	// This will create an alias against the requested entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+
+	aliasData["name"] = "testaliasname2"
+	aliasReq = &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "entity-alias",
+		Data:      aliasData,
+	}
+
+	// This will try to create a new alias with the same accessor and entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+	if resp == nil || !resp.IsError() {
+		t.Fatalf("expected an error as alias already exists for this accessor and entity")
+	}
+}
+
 func TestIdentityStore_AliasUpdate_ByID(t *testing.T) {
 	var err error
 	var resp *logical.Response

vault/identity_store_entities.go

@@ -770,6 +770,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 sanitizedFromEntityIDs {
 		if fromEntityID == toEntity.ID {
 			return errors.New("to_entity_id should not be present in from_entity_ids"), nil
@@ -799,6 +808,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)
 		}