From 10e534b25c5816f290f16bfb9febd11c83897a88 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Thu, 8 Nov 2018 10:15:50 -0500 Subject: [PATCH 1/2] Fix two problems with entity alias updating See https://github.com/hashicorp/vault/issues/5729#issuecomment-436882254 for details Fixes #5729 --- vault/identity_store_aliases.go | 15 +++++++++++++-- vault/identity_store_util.go | 30 ++++++++++++++++++++++++++++-- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/vault/identity_store_aliases.go b/vault/identity_store_aliases.go index e81851283a6f..84c7f717f184 100644 --- a/vault/identity_store_aliases.go +++ b/vault/identity_store_aliases.go @@ -181,6 +181,13 @@ func (i *IdentityStore) handleAliasUpdateCommon() framework.OperationFunc { if entity == nil { return nil, fmt.Errorf("existing alias is not associated with an entity") } + } else if alias.ID != "" { + // This is an update, not a create; if we have an associated entity + // already, load it + entity, err = i.MemDBEntityByAliasID(alias.ID, true) + if err != nil { + return nil, err + } } resp := &logical.Response{} @@ -210,21 +217,25 @@ func (i *IdentityStore) handleAliasUpdateCommon() framework.OperationFunc { // If entity is nil, we didn't find a previous alias from factors, // so append to this entity entity = canonicalEntity + i.logger.Warn("entity was nil, appending alias to this entity") entity.Aliases = append(entity.Aliases, alias) } else if entity.ID != canonicalEntity.ID { - // In this case we found an entity from alias factors but it's not - // the same, so it's a migration + // In this case we found an entity from alias factors or given + // alias ID but it's not the same, so it's a migration previousEntity = entity entity = canonicalEntity for aliasIndex, item := range previousEntity.Aliases { if item.ID == alias.ID { + i.logger.Warn("eliding entity", "previous", previousEntity.Aliases) previousEntity.Aliases = append(previousEntity.Aliases[:aliasIndex], previousEntity.Aliases[aliasIndex+1:]...) + i.logger.Warn("eliding entity", "after", previousEntity.Aliases) break } } entity.Aliases = append(entity.Aliases, alias) + i.logger.Warn("entity was found, appending alias to this entity") resp.AddWarning(fmt.Sprintf("alias is being transferred from entity %q to %q", previousEntity.ID, entity.ID)) } } diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 0777ef8df5f0..04fe3313f813 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -290,8 +290,34 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e return err } - if aliasByFactors != nil && aliasByFactors.CanonicalID != entity.ID { - i.logger.Warn("alias is already tied to a different entity; these entities are being merged", "alias_id", alias.ID, "other_entity_id", aliasByFactors.CanonicalID) + switch { + case aliasByFactors == nil: + // Not found, no merging needed + case aliasByFactors.CanonicalID == entity.ID: + // Lookup found the same entity, so it's already attached to the + // right place + case previousEntity != nil && aliasByFactors.CanonicalID == previousEntity.ID: + // previousEntity isn't upserted yet so may still contain the old + // alias reference in memdb if it was just changed; validate + // whether or not it's _actually_ still tied to the entity + var found bool + for _, prevEntAlias := range previousEntity.Aliases { + if prevEntAlias.ID == alias.ID { + found = true + break + } + } + // If we didn't find the alias still tied to previousEntity, we + // shouldn't use the merging logic and should bail + if !found { + break + } + + // Otherwise it's still tied to previousEntity and fall through + // into merging + fallthrough + default: + i.logger.Warn("alias is already tied to a different entity; these entities are being merged", "alias_id", alias.ID, "other_entity_id", aliasByFactors.CanonicalID, "entity_aliases", entity.Aliases, "alias_by_factors", aliasByFactors) respErr, intErr := i.mergeEntity(ctx, txn, entity, []string{aliasByFactors.CanonicalID}, true, false, true) switch { case respErr != nil: From ce4d90eb11826d22c4781fa0683451371c8605ec Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Thu, 8 Nov 2018 12:45:06 -0500 Subject: [PATCH 2/2] Remove comments --- vault/identity_store_aliases.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/vault/identity_store_aliases.go b/vault/identity_store_aliases.go index 84c7f717f184..59b0fbd5c064 100644 --- a/vault/identity_store_aliases.go +++ b/vault/identity_store_aliases.go @@ -217,7 +217,6 @@ func (i *IdentityStore) handleAliasUpdateCommon() framework.OperationFunc { // If entity is nil, we didn't find a previous alias from factors, // so append to this entity entity = canonicalEntity - i.logger.Warn("entity was nil, appending alias to this entity") entity.Aliases = append(entity.Aliases, alias) } else if entity.ID != canonicalEntity.ID { // In this case we found an entity from alias factors or given @@ -227,15 +226,12 @@ func (i *IdentityStore) handleAliasUpdateCommon() framework.OperationFunc { for aliasIndex, item := range previousEntity.Aliases { if item.ID == alias.ID { - i.logger.Warn("eliding entity", "previous", previousEntity.Aliases) previousEntity.Aliases = append(previousEntity.Aliases[:aliasIndex], previousEntity.Aliases[aliasIndex+1:]...) - i.logger.Warn("eliding entity", "after", previousEntity.Aliases) break } } entity.Aliases = append(entity.Aliases, alias) - i.logger.Warn("entity was found, appending alias to this entity") resp.AddWarning(fmt.Sprintf("alias is being transferred from entity %q to %q", previousEntity.ID, entity.ID)) } }