From aabd0db82a36d9ef07138a75f4633add33fe4016 Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Tue, 5 Oct 2021 13:59:12 -0700 Subject: [PATCH 01/10] Disallow alias creation if entity/accessor combination exists --- vault/identity_store_aliases.go | 6 ++++ vault/identity_store_aliases_test.go | 50 ++++++++++++++++++++++++++++ vault/identity_store_util.go | 25 ++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/vault/identity_store_aliases.go b/vault/identity_store_aliases.go index 54e4ac1217f50..fce2ddd9a1282 100644 --- a/vault/identity_store_aliases.go +++ b/vault/identity_store_aliases.go @@ -267,6 +267,12 @@ func (i *IdentityStore) handleAliasCreate(ctx context.Context, req *logical.Requ } } + for _, alias := range entity.Aliases { + if alias.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 diff --git a/vault/identity_store_aliases_test.go b/vault/identity_store_aliases_test.go index bf70e4010adbb..ebedc53800545 100644 --- a/vault/identity_store_aliases_test.go +++ b/vault/identity_store_aliases_test.go @@ -405,6 +405,56 @@ func TestIdentityStore_AliasUpdate(t *testing.T) { } } +// 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 and a corresponding 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 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 diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 0d10617263be7..7ad6cf831e797 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -341,6 +341,12 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { } } + duplicateAccessors := getDuplicateAccessorsOnAliases(ctx, entity.Aliases) + + // Log a warning if an entity has multiple aliases with the same accessor + if len(duplicateAccessors) > 0 { + i.logger.Warn("Duplicate aliases present for the same entity and accessor", "entity_name", entity.Name, "namespace_id", entity.NamespaceID, "duplicate_accessors", duplicateAccessors) + } // Only update MemDB and don't hit the storage again err = i.upsertEntity(nsCtx, entity, nil, false) if err != nil { @@ -360,6 +366,25 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { return nil } +// getDuplicateAccessorsOnAliases returns a list of duplicate accessors present in the +// passed in list of aliases +func getDuplicateAccessorsOnAliases(ctx context.Context, aliases []*identity.Alias) []string { + accessorCounts := make(map[string]int) + duplicateAccessors := make([]string, 0) + + for _, alias := range aliases { + accessorCounts[alias.MountAccessor] += 1 + } + + for accessor, accessorCount := range accessorCounts { + if accessorCount > 1 { + duplicateAccessors = append(duplicateAccessors, accessor) + } + } + + return duplicateAccessors +} + // upsertEntityInTxn either creates or updates an existing entity. The // operations will be updated in both MemDB and storage. If 'persist' is set to // false, then storage will not be updated. When an alias is transferred from From 76e4855d3879c19f98801f7775bd5f545be3f209 Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Tue, 5 Oct 2021 14:11:54 -0700 Subject: [PATCH 02/10] Add changelog --- changelog/12747.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/12747.txt diff --git a/changelog/12747.txt b/changelog/12747.txt new file mode 100644 index 0000000000000..07ec6d0f7771b --- /dev/null +++ b/changelog/12747.txt @@ -0,0 +1,3 @@ +```release-note:bug +core/identity: Disallow entity alias creation when an alias for the specified entity and mount exists +``` \ No newline at end of file From 9382ac4863aa8ce72845d5eb3b26986ca3fa8602 Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Wed, 6 Oct 2021 13:28:56 -0700 Subject: [PATCH 03/10] Address review comments --- vault/identity_store_aliases_test.go | 3 +++ vault/identity_store_util.go | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/vault/identity_store_aliases_test.go b/vault/identity_store_aliases_test.go index ebedc53800545..b61ef85dab1d7 100644 --- a/vault/identity_store_aliases_test.go +++ b/vault/identity_store_aliases_test.go @@ -450,6 +450,9 @@ func TestIdentityStore_AliasCreate_DuplicateAccessor(t *testing.T) { // 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") } diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 7ad6cf831e797..292d39b3cfd13 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -341,7 +341,7 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { } } - duplicateAccessors := getDuplicateAccessorsOnAliases(ctx, entity.Aliases) + duplicateAccessors := getDuplicateAccessorsOnAliases(entity.Aliases) // Log a warning if an entity has multiple aliases with the same accessor if len(duplicateAccessors) > 0 { @@ -368,7 +368,7 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { // getDuplicateAccessorsOnAliases returns a list of duplicate accessors present in the // passed in list of aliases -func getDuplicateAccessorsOnAliases(ctx context.Context, aliases []*identity.Alias) []string { +func getDuplicateAccessorsOnAliases(aliases []*identity.Alias) []string { accessorCounts := make(map[string]int) duplicateAccessors := make([]string, 0) From 7d1926b272d2b30707af8c76c8ad3af06c603dce Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Fri, 8 Oct 2021 06:33:23 -0700 Subject: [PATCH 04/10] Add handling to aliasUpdate, some field renaming --- vault/identity_store_aliases.go | 42 ++++-- vault/identity_store_aliases_test.go | 200 ++++++++++++++++++++++++++- vault/identity_store_util.go | 18 +-- 3 files changed, 238 insertions(+), 22 deletions(-) diff --git a/vault/identity_store_aliases.go b/vault/identity_store_aliases.go index fce2ddd9a1282..5affc5d85b217 100644 --- a/vault/identity_store_aliases.go +++ b/vault/identity_store_aliases.go @@ -318,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 associated with an entity in a different 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 + } + } + } // 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. @@ -349,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 { @@ -375,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 'canonical_id' 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) diff --git a/vault/identity_store_aliases_test.go b/vault/identity_store_aliases_test.go index b61ef85dab1d7..2cbc8cfb035c6 100644 --- a/vault/identity_store_aliases_test.go +++ b/vault/identity_store_aliases_test.go @@ -5,6 +5,8 @@ import ( "strings" "testing" + credGithub "github.com/hashicorp/vault/builtin/credential/github" + credUserpass "github.com/hashicorp/vault/builtin/credential/userpass" "github.com/hashicorp/vault/helper/identity" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/logical" @@ -405,6 +407,202 @@ 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) { + 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) + + // Setup 2 auth backends, github and userpass + err = AddTestCredentialBackend("github", credGithub.Factory) + if err != nil { + t.Fatalf("err: %s", err) + } + + err = AddTestCredentialBackend("userpass", credUserpass.Factory) + if err != nil { + t.Fatalf("err: %s", err) + } + + c, _, _ := TestCoreUnsealed(t) + + githubMe := &MountEntry{ + Table: credentialTableType, + Path: "github/", + Type: "github", + Description: "github auth", + } + + err = c.enableCredential(ctx, githubMe) + if err != nil { + t.Fatal(err) + } + + userpassMe := &MountEntry{ + Table: credentialTableType, + Path: "userpass/", + Type: "userpass", + Description: "userpass", + } + + err = c.enableCredential(ctx, userpassMe) + if err != nil { + t.Fatal(err) + } + + is, githubAccessor := c.identityStore, githubMe.Accessor + + // 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": githubAccessor, + "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": userpassMe.Accessor, + "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": githubAccessor, + } + + 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) { @@ -434,7 +632,7 @@ func TestIdentityStore_AliasCreate_DuplicateAccessor(t *testing.T) { Data: aliasData, } - // This will create an alias and a corresponding entity + // 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) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 292d39b3cfd13..b7a73bfef801c 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -341,11 +341,11 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { } } - duplicateAccessors := getDuplicateAccessorsOnAliases(entity.Aliases) + mountAccessors := getAccessorsOnDuplicateAliases(entity.Aliases) // Log a warning if an entity has multiple aliases with the same accessor - if len(duplicateAccessors) > 0 { - i.logger.Warn("Duplicate aliases present for the same entity and accessor", "entity_name", entity.Name, "namespace_id", entity.NamespaceID, "duplicate_accessors", duplicateAccessors) + if len(mountAccessors) > 0 { + i.logger.Warn("Multiple aliases present for entity on mount(s), remove duplicates to avoid ACL templating issues", "entity_name", entity.Name, "namespace_id", entity.NamespaceID, "mount_accessors", mountAccessors) } // Only update MemDB and don't hit the storage again err = i.upsertEntity(nsCtx, entity, nil, false) @@ -366,11 +366,11 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { return nil } -// getDuplicateAccessorsOnAliases returns a list of duplicate accessors present in the -// passed in list of aliases -func getDuplicateAccessorsOnAliases(aliases []*identity.Alias) []string { +// getAccessorsOnDuplicateAliases returns a list of accessors by checking aliases in +// the passed in list which belong to the same accessor(s) +func getAccessorsOnDuplicateAliases(aliases []*identity.Alias) []string { accessorCounts := make(map[string]int) - duplicateAccessors := make([]string, 0) + var mountAccessors []string for _, alias := range aliases { accessorCounts[alias.MountAccessor] += 1 @@ -378,11 +378,11 @@ func getDuplicateAccessorsOnAliases(aliases []*identity.Alias) []string { for accessor, accessorCount := range accessorCounts { if accessorCount > 1 { - duplicateAccessors = append(duplicateAccessors, accessor) + mountAccessors = append(mountAccessors, accessor) } } - return duplicateAccessors + return mountAccessors } // upsertEntityInTxn either creates or updates an existing entity. The From 75c300a3c68e7aaaaecf5fe6df80588934bfb919 Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Fri, 8 Oct 2021 08:26:37 -0700 Subject: [PATCH 05/10] Update tests to work under new entity-alias constraint --- vault/core_metrics_test.go | 24 ++++++++++---- vault/identity_store_aliases_test.go | 47 +++----------------------- vault/identity_store_entities_test.go | 44 +++++------------------- vault/identity_store_test.go | 48 +++++++++++++++++++++++++-- 4 files changed, 75 insertions(+), 88 deletions(-) diff --git a/vault/core_metrics_test.go b/vault/core_metrics_test.go index 7558637efef84..3651df0b9c972 100644 --- a/vault/core_metrics_test.go +++ b/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/", + }) } diff --git a/vault/identity_store_aliases_test.go b/vault/identity_store_aliases_test.go index 2cbc8cfb035c6..b2740f3dea128 100644 --- a/vault/identity_store_aliases_test.go +++ b/vault/identity_store_aliases_test.go @@ -5,8 +5,6 @@ import ( "strings" "testing" - credGithub "github.com/hashicorp/vault/builtin/credential/github" - credUserpass "github.com/hashicorp/vault/builtin/credential/userpass" "github.com/hashicorp/vault/helper/identity" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/sdk/logical" @@ -499,44 +497,7 @@ func TestIdentityStore_AliasUpdate__DuplicateAccessor(t *testing.T) { var resp *logical.Response ctx := namespace.RootContext(nil) - // Setup 2 auth backends, github and userpass - err = AddTestCredentialBackend("github", credGithub.Factory) - if err != nil { - t.Fatalf("err: %s", err) - } - - err = AddTestCredentialBackend("userpass", credUserpass.Factory) - if err != nil { - t.Fatalf("err: %s", err) - } - - c, _, _ := TestCoreUnsealed(t) - - githubMe := &MountEntry{ - Table: credentialTableType, - Path: "github/", - Type: "github", - Description: "github auth", - } - - err = c.enableCredential(ctx, githubMe) - if err != nil { - t.Fatal(err) - } - - userpassMe := &MountEntry{ - Table: credentialTableType, - Path: "userpass/", - Type: "userpass", - Description: "userpass", - } - - err = c.enableCredential(ctx, userpassMe) - if err != nil { - t.Fatal(err) - } - - is, githubAccessor := c.identityStore, githubMe.Accessor + 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{ @@ -553,7 +514,7 @@ func TestIdentityStore_AliasUpdate__DuplicateAccessor(t *testing.T) { alias1Data := map[string]interface{}{ "name": "testaliasname1", - "mount_accessor": githubAccessor, + "mount_accessor": ghAccessor, "canonical_id": entityID, } @@ -571,7 +532,7 @@ func TestIdentityStore_AliasUpdate__DuplicateAccessor(t *testing.T) { alias2Data := map[string]interface{}{ "name": "testaliasname2", - "mount_accessor": userpassMe.Accessor, + "mount_accessor": upAccessor, "canonical_id": entityID, } @@ -586,7 +547,7 @@ func TestIdentityStore_AliasUpdate__DuplicateAccessor(t *testing.T) { // Attempt to update the userpass mount to point to the github mount updateData := map[string]interface{}{ - "mount_accessor": githubAccessor, + "mount_accessor": ghAccessor, } aliasReq.Data = updateData diff --git a/vault/identity_store_entities_test.go b/vault/identity_store_entities_test.go index 910b3ff1dd1af..35ed6f06ba4d4 100644 --- a/vault/identity_store_entities_test.go +++ b/vault/identity_store_entities_test.go @@ -1012,18 +1012,6 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) { "metadata": []string{"organization=hashicorp", "team=vault"}, } - aliasRegisterData3 := map[string]interface{}{ - "name": "testaliasname3", - "mount_accessor": githubAccessor, - "metadata": []string{"organization=hashicorp", "team=vault"}, - } - - aliasRegisterData4 := map[string]interface{}{ - "name": "testaliasname4", - "mount_accessor": githubAccessor, - "metadata": []string{"organization=hashicorp", "team=vault"}, - } - registerReq := &logical.Request{ Operation: logical.UpdateOperation, Path: "entity", @@ -1040,7 +1028,6 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) { // Set entity ID in alias registration data and register alias aliasRegisterData1["entity_id"] = entityID1 - aliasRegisterData2["entity_id"] = entityID1 aliasReq := &logical.Request{ Operation: logical.UpdateOperation, @@ -1054,13 +1041,6 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) { t.Fatalf("err:%v resp:%#v", err, resp) } - // Register the alias - aliasReq.Data = aliasRegisterData2 - resp, err = is.HandleRequest(ctx, aliasReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("err:%v resp:%#v", err, resp) - } - entity1, err := is.MemDBEntityByID(entityID1, false) if err != nil { t.Fatal(err) @@ -1068,8 +1048,8 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) { if entity1 == nil { t.Fatalf("failed to create entity: %v", err) } - if len(entity1.Aliases) != 2 { - t.Fatalf("bad: number of aliases in entity; expected: 2, actual: %d", len(entity1.Aliases)) + if len(entity1.Aliases) != 1 { + t.Fatalf("bad: number of aliases in entity; expected: 1, actual: %d", len(entity1.Aliases)) } entity1GroupReq := &logical.Request{ @@ -1094,23 +1074,15 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) { entityID2 := resp.Data["id"].(string) // Set entity ID in alias registration data and register alias - aliasRegisterData3["entity_id"] = entityID2 - aliasRegisterData4["entity_id"] = entityID2 + aliasRegisterData2["entity_id"] = entityID2 aliasReq = &logical.Request{ Operation: logical.UpdateOperation, Path: "alias", - 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) + Data: aliasRegisterData2, } // Register the alias - aliasReq.Data = aliasRegisterData4 resp, err = is.HandleRequest(ctx, aliasReq) if err != nil || (resp != nil && resp.IsError()) { t.Fatalf("err:%v resp:%#v", err, resp) @@ -1124,8 +1096,8 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) { t.Fatalf("failed to create entity: %v", err) } - if len(entity2.Aliases) != 2 { - t.Fatalf("bad: number of aliases in entity; expected: 2, actual: %d", len(entity2.Aliases)) + if len(entity2.Aliases) != 1 { + t.Fatalf("bad: number of aliases in entity; expected: 1, actual: %d", len(entity2.Aliases)) } entity2GroupReq := &logical.Request{ @@ -1175,8 +1147,8 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) { } entity1Aliases := resp.Data["aliases"].([]interface{}) - if len(entity1Aliases) != 4 { - t.Fatalf("bad: number of aliases in entity; expected: 4, actual: %d", len(entity1Aliases)) + if len(entity1Aliases) != 2 { + t.Fatalf("bad: number of aliases in entity; expected: 2, actual: %d", len(entity1Aliases)) } for _, aliasRaw := range entity1Aliases { diff --git a/vault/identity_store_test.go b/vault/identity_store_test.go index 82ad82cab59c3..78a409e268adc 100644 --- a/vault/identity_store_test.go +++ b/vault/identity_store_test.go @@ -11,6 +11,7 @@ import ( "github.com/golang/protobuf/ptypes" uuid "github.com/hashicorp/go-uuid" credGithub "github.com/hashicorp/vault/builtin/credential/github" + credUserpass "github.com/hashicorp/vault/builtin/credential/userpass" "github.com/hashicorp/vault/helper/identity" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/helper/storagepacker" @@ -187,7 +188,8 @@ func TestIdentityStore_EntityIDPassthrough(t *testing.T) { func TestIdentityStore_CreateOrFetchEntity(t *testing.T) { ctx := namespace.RootContext(nil) - is, ghAccessor, _ := testIdentityStoreWithGithubAuth(ctx, t) + is, ghAccessor, upAccessor, _ := testIdentityStoreWithGithubUserpassAuth(ctx, t) + alias := &logical.Alias{ MountType: "github", MountAccessor: ghAccessor, @@ -239,7 +241,7 @@ func TestIdentityStore_CreateOrFetchEntity(t *testing.T) { Data: map[string]interface{}{ "name": "githubuser2", "canonical_id": entity.ID, - "mount_accessor": ghAccessor, + "mount_accessor": upAccessor, }, } @@ -612,6 +614,48 @@ func testIdentityStoreWithGithubAuthRoot(ctx context.Context, t *testing.T) (*Id return c.identityStore, meGH.Accessor, c, root } +func testIdentityStoreWithGithubUserpassAuth(ctx context.Context, t *testing.T) (*IdentityStore, string, string, *Core) { + // Setup 2 auth backends, github and userpass + err := AddTestCredentialBackend("github", credGithub.Factory) + if err != nil { + t.Fatalf("err: %s", err) + } + + err = AddTestCredentialBackend("userpass", credUserpass.Factory) + if err != nil { + t.Fatalf("err: %s", err) + } + + c, _, _ := TestCoreUnsealed(t) + + githubMe := &MountEntry{ + Table: credentialTableType, + Path: "github/", + Type: "github", + Description: "github auth", + } + + err = c.enableCredential(ctx, githubMe) + if err != nil { + t.Fatal(err) + } + + userpassMe := &MountEntry{ + Table: credentialTableType, + Path: "userpass/", + Type: "userpass", + Description: "userpass", + } + + err = c.enableCredential(ctx, userpassMe) + if err != nil { + t.Fatal(err) + } + + return c.identityStore, githubMe.Accessor, userpassMe.Accessor, c + +} + func TestIdentityStore_MetadataKeyRegex(t *testing.T) { key := "validVALID012_-=+/" From 2d46fa26084117e23d39ee300cf66fdb5f0c4f14 Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Wed, 13 Oct 2021 10:05:05 -0700 Subject: [PATCH 06/10] 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) From 5ad1a363453198c988d3a9630491aa5eea064db2 Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Wed, 13 Oct 2021 12:37:55 -0700 Subject: [PATCH 07/10] Log duplicated accessors only once --- vault/identity_store_util.go | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index b7a73bfef801c..614eef952a051 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -222,6 +222,7 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { } i.logger.Debug("entities collected", "num_existing", len(existing)) + duplicatedAccessors := make(map[string]struct{}) // Make the channels used for the worker pool broker := make(chan string) quit := make(chan bool) @@ -343,9 +344,10 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { mountAccessors := getAccessorsOnDuplicateAliases(entity.Aliases) - // Log a warning if an entity has multiple aliases with the same accessor - if len(mountAccessors) > 0 { - i.logger.Warn("Multiple aliases present for entity on mount(s), remove duplicates to avoid ACL templating issues", "entity_name", entity.Name, "namespace_id", entity.NamespaceID, "mount_accessors", mountAccessors) + for _, accessor := range mountAccessors { + if _, ok := duplicatedAccessors[accessor]; !ok { + duplicatedAccessors[accessor] = struct{}{} + } } // Only update MemDB and don't hit the storage again err = i.upsertEntity(nsCtx, entity, nil, false) @@ -359,6 +361,18 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { // Let all go routines finish wg.Wait() + // Flatten the map into a list of keys, in order to log them + duplicatedAccessorsList := make([]string, len(duplicatedAccessors)) + accessorCounter := 0 + for accessor := range duplicatedAccessors { + duplicatedAccessorsList[accessorCounter] = accessor + accessorCounter++ + } + + if len(duplicatedAccessorsList) > 0 { + i.logger.Warn("One or more entities have multiple aliases on the same mount(s), remove duplicates to avoid ACL templating issues", "mount_accessors", duplicatedAccessorsList) + } + if i.logger.IsInfo() { i.logger.Info("entities restored") } From 3c47f127aa92eaa495ce94fa0f0e43333dfff5a4 Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Wed, 13 Oct 2021 14:06:52 -0700 Subject: [PATCH 08/10] Fix flaky test --- vault/core_metrics_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/vault/core_metrics_test.go b/vault/core_metrics_test.go index 3651df0b9c972..543e970c70d99 100644 --- a/vault/core_metrics_test.go +++ b/vault/core_metrics_test.go @@ -2,6 +2,7 @@ package vault import ( "errors" + "sort" "strings" "testing" "time" @@ -323,6 +324,15 @@ func TestCoreMetrics_EntityGauges(t *testing.T) { t.Errorf("Alias count %v, expected %v", glv[0].Value, 1.0) } + // Sort both metrics.Label slices by Name, causing the Label + // with Name auth_method to be first in both arrays + sort.Slice(glv[0].Labels, func(i, j int) bool { return glv[0].Labels[i].Name < glv[0].Labels[j].Name }) + sort.Slice(glv[1].Labels, func(i, j int) bool { return glv[1].Labels[i].Name < glv[1].Labels[j].Name }) + + // Sort the GaugeLabelValues slice by the Value of the first metric, + // in this case auth_method, in each metrics.Label slice + sort.Slice(glv, func(i, j int) bool { return glv[i].Labels[0].Value < glv[j].Labels[0].Value }) + metricLabelsMatch(t, glv[0].Labels, map[string]string{ "namespace": "root", From db2e611b6a6a7141baee52859d7641c7f27d7711 Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Wed, 13 Oct 2021 14:35:07 -0700 Subject: [PATCH 09/10] Add note about new constraint to docs --- website/content/docs/secrets/identity.mdx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/website/content/docs/secrets/identity.mdx b/website/content/docs/secrets/identity.mdx index cb5a5e0b494c1..79dfd6139d14d 100644 --- a/website/content/docs/secrets/identity.mdx +++ b/website/content/docs/secrets/identity.mdx @@ -39,7 +39,8 @@ disabled or moved. Each user will have multiple accounts with various identity providers. Users can now be mapped as `Entities` and their corresponding accounts with authentication providers can be mapped as `Aliases`. In essence, each entity is -made up of zero or more aliases. +made up of zero or more aliases. An entity cannot have more than one alias for +a particular authentication backend. ### Entity Management From b465411a7a9c95ce657032d5d587d75e5b35884b Mon Sep 17 00:00:00 2001 From: Pratyoy Mukhopadhyay Date: Thu, 14 Oct 2021 09:17:41 -0700 Subject: [PATCH 10/10] Update entity merge warn log --- vault/identity_store_entities.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/identity_store_entities.go b/vault/identity_store_entities.go index 00ed84248cc90..542edaa1e0692 100644 --- a/vault/identity_store_entities.go +++ b/vault/identity_store_entities.go @@ -809,7 +809,7 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit } 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) + i.logger.Warn("skipping from_entity alias during entity merge as to_entity has an alias with its accessor", "from_entity", fromEntityID, "skipped_alias", alias.ID) continue } // Add the alias to the desired entity