[VAULT-3252] Disallow alias creation if entity/accessor combination exists (#12747)

* Disallow alias creation if entity/accessor combination exists

* Add changelog

* Address review comments

* Add handling to aliasUpdate, some field renaming

* Update tests to work under new entity-alias constraint

* Add check to entity merge, other review fixes

* Log duplicated accessors only once

* Fix flaky test

* Add note about new constraint to docs

* Update entity merge warn log
This commit is contained in:
Pratyoy Mukhopadhyay 2021-10-14 09:52:07 -07:00 committed by GitHub
parent 57c568e511
commit 148109b8ed
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 400 additions and 47 deletions

3
changelog/12747.txt Normal file
View File

@ -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
```

View File

@ -2,6 +2,7 @@ package vault
import (
"errors"
"sort"
"strings"
"testing"
"time"
@ -259,7 +260,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 +279,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,18 +312,38 @@ 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)
}
// 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",
"auth_method": "github",
"mount_point": "auth/github/",
})
metricLabelsMatch(t, glv[1].Labels,
map[string]string{
"namespace": "root",
"auth_method": "userpass",
"mount_point": "auth/userpass/",
})
}

View File

@ -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)

View File

@ -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

View File

@ -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 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
toEntity.Aliases = append(toEntity.Aliases, alias)
}

View File

@ -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",
@ -1014,13 +1014,7 @@ func TestIdentityStore_MergeEntitiesByID(t *testing.T) {
aliasRegisterData3 := map[string]interface{}{
"name": "testaliasname3",
"mount_accessor": githubAccessor,
"metadata": []string{"organization=hashicorp", "team=vault"},
}
aliasRegisterData4 := map[string]interface{}{
"name": "testaliasname4",
"mount_accessor": githubAccessor,
"mount_accessor": upAccessor,
"metadata": []string{"organization=hashicorp", "team=vault"},
}
@ -1040,7 +1034,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 +1047,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 +1054,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,13 +1080,12 @@ 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,
Data: aliasRegisterData2,
}
// Register the alias
@ -1109,13 +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
aliasReq.Data = aliasRegisterData4
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)
@ -1175,10 +1162,11 @@ 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))
}
githubAliases := 0
for _, aliasRaw := range entity1Aliases {
alias := aliasRaw.(map[string]interface{})
aliasLookedUp, err := is.MemDBAliasByID(alias["id"].(string), false, false)
@ -1188,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)

View File

@ -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_-=+/"

View File

@ -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)
@ -341,6 +342,13 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error {
}
}
mountAccessors := getAccessorsOnDuplicateAliases(entity.Aliases)
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)
if err != nil {
@ -353,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")
}
@ -360,6 +380,25 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error {
return nil
}
// 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)
var mountAccessors []string
for _, alias := range aliases {
accessorCounts[alias.MountAccessor] += 1
}
for accessor, accessorCount := range accessorCounts {
if accessorCount > 1 {
mountAccessors = append(mountAccessors, accessor)
}
}
return mountAccessors
}
// 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

View File

@ -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