From 2d46fa26084117e23d39ee300cf66fdb5f0c4f14 Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Wed, 13 Oct 2021 10:05:05 -0700 Subject: [PATCH] Add check to entity merge, other review fixes --- changelog/12747.txt | 2 +- vault/identity_store_aliases.go | 10 ++++----- vault/identity_store_aliases_test.go | 4 ++-- vault/identity_store_entities.go | 13 +++++++++++ vault/identity_store_entities_test.go | 31 ++++++++++++++++++++++++--- 5 files changed, 49 insertions(+), 11 deletions(-) diff --git a/changelog/12747.txt b/changelog/12747.txt index 07ec6d0f7771b..2347d30236fc7 100644 --- a/changelog/12747.txt +++ b/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 ``` \ No newline at end of file diff --git a/vault/identity_store_aliases.go b/vault/identity_store_aliases.go index 5affc5d85b217..4ba3356b8b86c 100644 --- a/vault/identity_store_aliases.go +++ b/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,7 +329,7 @@ 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 @@ -337,7 +337,7 @@ func (i *IdentityStore) handleAliasUpdate(ctx context.Context, req *logical.Requ 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 } } diff --git a/vault/identity_store_aliases_test.go b/vault/identity_store_aliases_test.go index b2740f3dea128..c44a9c52e5e62 100644 --- a/vault/identity_store_aliases_test.go +++ b/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) diff --git a/vault/identity_store_entities.go b/vault/identity_store_entities.go index 4a0efc7a97d4b..f95fa5e0cf98f 100644 --- a/vault/identity_store_entities.go +++ b/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) } diff --git a/vault/identity_store_entities_test.go b/vault/identity_store_entities_test.go index 35ed6f06ba4d4..e111dc0056ef6 100644 --- a/vault/identity_store_entities_test.go +++ b/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)