From 8152811b38d0978d5db1cdfa24f552bc74789e9b Mon Sep 17 00:00:00 2001 From: Chris Hoffman Date: Tue, 24 Jul 2018 22:01:58 -0400 Subject: [PATCH] Add locking when adding aliases to existing entities (#4965) --- CHANGELOG.md | 4 + vault/identity_store.go | 17 +- vault/identity_store_aliases.go | 433 ++++++++++----------------- vault/identity_store_aliases_test.go | 16 +- vault/identity_store_entities.go | 256 ++++++---------- vault/identity_store_structs.go | 6 +- vault/identity_store_upgrade.go | 44 +-- vault/identity_store_util.go | 37 +-- 8 files changed, 271 insertions(+), 542 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a1eef01be66..f20da8e7b372 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ DEPRECATIONS/CHANGES: * CLI Retries: The CLI will no longer retry commands on 5xx errors. This was a source of confusion to users as to why Vault would "hang" before returning a 5xx error. The Go API client still defaults to two retries. + * Identity Entity Alias metadata: You can no longer manually set metadata on + entity aliases. All alias data (except the canonical entity ID it refers to) + is intended to be managed by the plugin providing the alias information, so + allowing it to be set manually didn't make sense. FEATURES: diff --git a/vault/identity_store.go b/vault/identity_store.go index 85403a179a26..13b17983e617 100644 --- a/vault/identity_store.go +++ b/vault/identity_store.go @@ -10,7 +10,6 @@ import ( log "github.com/hashicorp/go-hclog" memdb "github.com/hashicorp/go-memdb" "github.com/hashicorp/vault/helper/identity" - "github.com/hashicorp/vault/helper/locksutil" "github.com/hashicorp/vault/helper/storagepacker" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" @@ -35,11 +34,10 @@ func NewIdentityStore(ctx context.Context, core *Core, config *logical.BackendCo } iStore := &IdentityStore{ - view: config.StorageView, - db: db, - entityLocks: locksutil.CreateLocks(), - logger: logger, - core: core, + view: config.StorageView, + db: db, + logger: logger, + core: core, } iStore.entityPacker, err = storagepacker.NewStoragePacker(iStore.view, iStore.logger, "") @@ -144,7 +142,7 @@ func (i *IdentityStore) Invalidate(ctx context.Context, key string) { } // Only update MemDB and don't touch the storage - err = i.upsertEntityInTxn(txn, entity, nil, false, false) + err = i.upsertEntityInTxn(txn, entity, nil, false) if err != nil { i.logger.Error("failed to update entity in MemDB", "error", err) return @@ -346,6 +344,9 @@ func (i *IdentityStore) CreateOrFetchEntity(alias *logical.Alias) (*identity.Ent return entity, nil } + i.lock.Lock() + defer i.lock.Unlock() + // Create a MemDB transaction to update both alias and entity txn := i.db.Txn(true) defer txn.Abort() @@ -388,7 +389,7 @@ func (i *IdentityStore) CreateOrFetchEntity(alias *logical.Alias) (*identity.Ent } // Update MemDB and persist entity object - err = i.upsertEntityInTxn(txn, entity, nil, true, false) + err = i.upsertEntityInTxn(txn, entity, nil, true) if err != nil { return nil, err } diff --git a/vault/identity_store_aliases.go b/vault/identity_store_aliases.go index 7ade19435752..d4debc4c0c68 100644 --- a/vault/identity_store_aliases.go +++ b/vault/identity_store_aliases.go @@ -9,7 +9,6 @@ import ( "github.com/hashicorp/errwrap" memdb "github.com/hashicorp/go-memdb" "github.com/hashicorp/vault/helper/identity" - "github.com/hashicorp/vault/helper/locksutil" "github.com/hashicorp/vault/helper/storagepacker" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" @@ -30,73 +29,25 @@ func aliasPaths(i *IdentityStore) []*framework.Path { }, // entity_id is deprecated in favor of canonical_id "entity_id": { - Type: framework.TypeString, - Description: "Entity ID to which this alias belongs to", + Type: framework.TypeString, + Description: `Entity ID to which this alias belongs. +This field is deprecated, use canonical_id.`, }, "canonical_id": { Type: framework.TypeString, - Description: "Entity ID to which this alias belongs to", + Description: "Entity ID to which this alias belongs", }, "mount_accessor": { Type: framework.TypeString, - Description: "Mount accessor to which this alias belongs to", + Description: "Mount accessor to which this alias belongs to; unused for a modify", }, "name": { Type: framework.TypeString, - Description: "Name of the alias", - }, - "metadata": { - Type: framework.TypeKVPairs, - Description: `Metadata to be associated with the alias. -In CLI, this parameter can be repeated multiple times, and it all gets merged together. -For example: -vault metadata=key1=value1 metadata=key2=value2 - `, + Description: "Name of the alias; unused for a modify", }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: i.pathAliasRegister(), - }, - - HelpSynopsis: strings.TrimSpace(aliasHelp["alias"][0]), - HelpDescription: strings.TrimSpace(aliasHelp["alias"][1]), - }, - // BC path for identity/entity-alias - { - Pattern: "alias$", - Fields: map[string]*framework.FieldSchema{ - "id": { - Type: framework.TypeString, - Description: "ID of the alias", - }, - // entity_id is deprecated - "entity_id": { - Type: framework.TypeString, - Description: "Entity ID to which this alias belongs to", - }, - "canonical_id": { - Type: framework.TypeString, - Description: "Entity ID to which this alias belongs to", - }, - "mount_accessor": { - Type: framework.TypeString, - Description: "Mount accessor to which this alias belongs to", - }, - "name": { - Type: framework.TypeString, - Description: "Name of the alias", - }, - "metadata": { - Type: framework.TypeKVPairs, - Description: `Metadata to be associated with the alias. -In CLI, this parameter can be repeated multiple times, and it all gets merged together. -For example: -vault metadata=key1=value1 metadata=key2=value2 - `, - }, - }, - Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: i.pathAliasRegister(), + logical.UpdateOperation: i.handleAliasUpdateCommon(), }, HelpSynopsis: strings.TrimSpace(aliasHelp["alias"][0]), @@ -111,8 +62,9 @@ vault metadata=key1=value1 metadata=key2=value2 }, // entity_id is deprecated "entity_id": { - Type: framework.TypeString, - Description: "Entity ID to which this alias belongs to", + Type: framework.TypeString, + Description: `Entity ID to which this alias belongs to. +This field is deprecated, use canonical_id.`, }, "canonical_id": { Type: framework.TypeString, @@ -120,23 +72,15 @@ vault metadata=key1=value1 metadata=key2=value2 }, "mount_accessor": { Type: framework.TypeString, - Description: "Mount accessor to which this alias belongs to", + Description: "(Unused)", }, "name": { Type: framework.TypeString, - Description: "Name of the alias", - }, - "metadata": { - Type: framework.TypeKVPairs, - Description: `Metadata to be associated with the alias. -In CLI, this parameter can be repeated multiple times, and it all gets merged together. -For example: -vault metadata=key1=value1 metadata=key2=value2 - `, + Description: "(Unused)", }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: i.pathAliasIDUpdate(), + logical.UpdateOperation: i.handleAliasUpdateCommon(), logical.ReadOperation: i.pathAliasIDRead(), logical.DeleteOperation: i.pathAliasIDDelete(), }, @@ -156,215 +100,178 @@ vault metadata=key1=value1 metadata=key2=value2 } } -// pathAliasRegister is used to register new alias -func (i *IdentityStore) pathAliasRegister() framework.OperationFunc { +// handleAliasUpdateCommon is used to update an alias +func (i *IdentityStore) handleAliasUpdateCommon() framework.OperationFunc { return func(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - _, ok := d.GetOk("id") - if ok { - return i.pathAliasIDUpdate()(ctx, req, d) - } + var err error + var alias *identity.Alias + var entity *identity.Entity + var previousEntity *identity.Entity - return i.handleAliasUpdateCommon(req, d, nil) - } -} + i.lock.Lock() + defer i.lock.Unlock() -// pathAliasIDUpdate is used to update an alias based on the given -// alias ID -func (i *IdentityStore) pathAliasIDUpdate() framework.OperationFunc { - return func(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - // Get alias id + // Check for update or create aliasID := d.Get("id").(string) - - if aliasID == "" { - return logical.ErrorResponse("empty alias ID"), nil + if aliasID != "" { + alias, err = i.MemDBAliasByID(aliasID, true, false) + if err != nil { + return nil, err + } + if alias == nil { + return logical.ErrorResponse("invalid alias id"), nil + } + } else { + alias = &identity.Alias{} } - alias, err := i.MemDBAliasByID(aliasID, true, false) - if err != nil { - return nil, err - } - if alias == nil { - return logical.ErrorResponse("invalid alias id"), nil + // Get entity id + canonicalID := d.Get("canonical_id").(string) + if canonicalID == "" { + // For backwards compatibility + canonicalID = d.Get("entity_id").(string) } - return i.handleAliasUpdateCommon(req, d, alias) - } -} + // Get alias name + if aliasName := d.Get("name").(string); aliasName == "" { + if alias.Name == "" { + return logical.ErrorResponse("missing alias name"), nil + } + } else { + alias.Name = aliasName + } -// handleAliasUpdateCommon is used to update an alias -func (i *IdentityStore) handleAliasUpdateCommon(req *logical.Request, d *framework.FieldData, alias *identity.Alias) (*logical.Response, error) { - var err error - var newAlias bool - var entity *identity.Entity - var previousEntity *identity.Entity - - // Alias will be nil when a new alias is being registered; create a - // new struct in that case. - if alias == nil { - alias = &identity.Alias{} - newAlias = true - } + // Get mount accessor + if mountAccessor := d.Get("mount_accessor").(string); mountAccessor == "" { + if alias.MountAccessor == "" { + return logical.ErrorResponse("missing mount_accessor"), nil + } + } else { + alias.MountAccessor = mountAccessor + } - // Get entity id - canonicalID := d.Get("entity_id").(string) - if canonicalID == "" { - canonicalID = d.Get("canonical_id").(string) - } + mountValidationResp := i.core.router.validateMountByAccessor(alias.MountAccessor) + if mountValidationResp == nil { + return logical.ErrorResponse(fmt.Sprintf("invalid mount accessor %q", alias.MountAccessor)), nil + } + if mountValidationResp.MountLocal { + return logical.ErrorResponse(fmt.Sprintf("mount_accessor %q is of a local mount", alias.MountAccessor)), nil + } - if canonicalID != "" { - entity, err = i.MemDBEntityByID(canonicalID, true) + // Verify that the combination of alias name and mount is not + // already tied to a different alias + aliasByFactors, err := i.MemDBAliasByFactors(mountValidationResp.MountAccessor, alias.Name, false, false) if err != nil { return nil, err } - if entity == nil { - return logical.ErrorResponse("invalid entity ID"), nil - } - } - - // Get alias name - aliasName := d.Get("name").(string) - if aliasName == "" { - return logical.ErrorResponse("missing alias name"), nil - } - - mountAccessor := d.Get("mount_accessor").(string) - if mountAccessor == "" { - return logical.ErrorResponse("missing mount_accessor"), nil - } - - mountValidationResp := i.core.router.validateMountByAccessor(mountAccessor) - if mountValidationResp == nil { - return logical.ErrorResponse(fmt.Sprintf("invalid mount accessor %q", mountAccessor)), nil - } - - if mountValidationResp.MountLocal { - return logical.ErrorResponse(fmt.Sprintf("mount_accessor %q is of a local mount", mountAccessor)), nil - } - - // Get alias metadata - metadata, ok, err := d.GetOkErr("metadata") - if err != nil { - return logical.ErrorResponse(fmt.Sprintf("failed to parse metadata: %v", err)), nil - } - var aliasMetadata map[string]string - if ok { - aliasMetadata = metadata.(map[string]string) - } - - aliasByFactors, err := i.MemDBAliasByFactors(mountValidationResp.MountAccessor, aliasName, false, false) - if err != nil { - return nil, err - } - - resp := &logical.Response{} - - if newAlias { if aliasByFactors != nil { - return logical.ErrorResponse("combination of mount and alias name is already in use"), nil + // If it's a create we won't have an alias ID so this will correctly + // bail. If it's an update alias will be the same as aliasbyfactors so + // we don't need to transfer any info over + if aliasByFactors.ID != alias.ID { + return logical.ErrorResponse("combination of mount and alias name is already in use"), nil + } + + // Fetch the entity to which the alias is tied. We don't need to append + // here, so the only further checking is whether the canonical ID is + // different + entity, err = i.MemDBEntityByAliasID(alias.ID, true) + if err != nil { + return nil, err + } + if entity == nil { + return nil, fmt.Errorf("existing alias is not associated with an entity") + } + if canonicalID == "" || entity.ID == canonicalID { + // Nothing to do + return nil, nil + } } - // If this is an alias being tied to a non-existent entity, create - // a new entity for it. - if entity == nil { + resp := &logical.Response{} + + // If we found an exisitng alias we won't hit this condition because + // canonicalID being empty will result in nil being returned in the block + // above, so in this case we know that creating a new entity is the right + // thing. + if canonicalID == "" { entity = &identity.Entity{ Aliases: []*identity.Alias{ alias, }, } } else { - entity.Aliases = append(entity.Aliases, alias) - } - } else { - // Verify that the combination of alias name and mount is not - // already tied to a different alias - if aliasByFactors != nil && aliasByFactors.ID != alias.ID { - return logical.ErrorResponse("combination of mount and alias name is already in use"), nil - } - - // Fetch the entity to which the alias is tied to - existingEntity, err := i.MemDBEntityByAliasID(alias.ID, true) - if err != nil { - return nil, err - } - - if existingEntity == nil { - return nil, fmt.Errorf("alias is not associated with an entity") - } - - if entity != nil && entity.ID != existingEntity.ID { - // Alias should be transferred from 'existingEntity' to 'entity' - for aliasIndex, item := range existingEntity.Aliases { - if item.ID == alias.ID { - entity.Aliases = append(existingEntity.Aliases[:aliasIndex], existingEntity.Aliases[aliasIndex+1:]...) - break - } + // If we can look up by the given canonical ID, see if this is a + // transfer; otherwise if we found no previous entity but we find one + // here, use it. + canonicalEntity, err := i.MemDBEntityByID(canonicalID, true) + if err != nil { + return nil, err } - - previousEntity = existingEntity - entity.Aliases = append(entity.Aliases, alias) - resp.AddWarning(fmt.Sprintf("alias is being transferred from entity %q to %q", existingEntity.ID, entity.ID)) - } else { - // Update entity with modified alias - aliasFound := false - for aliasIndex, item := range existingEntity.Aliases { - if item.ID == alias.ID { - aliasFound = true - existingEntity.Aliases[aliasIndex] = alias - break - } + if canonicalEntity == nil { + return logical.ErrorResponse("invalid canonical ID"), nil } + if entity == nil { + // If entity is nil, we didn't find a previous alias from factors, + // so append to this entity + entity = canonicalEntity + 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 + previousEntity = entity + entity = canonicalEntity + + for aliasIndex, item := range previousEntity.Aliases { + if item.ID == alias.ID { + previousEntity.Aliases = append(previousEntity.Aliases[:aliasIndex], previousEntity.Aliases[aliasIndex+1:]...) + break + } + } - if !aliasFound { - return nil, fmt.Errorf("alias does not exist in entity") + entity.Aliases = append(entity.Aliases, alias) + resp.AddWarning(fmt.Sprintf("alias is being transferred from entity %q to %q", previousEntity.ID, entity.ID)) } - - entity = existingEntity } - } - // ID creation and other validations; This is more useful for new entities - // and may not perform anything for the existing entities. Placing the - // check here to make the flow common for both new and existing entities. - err = i.sanitizeEntity(entity) - if err != nil { - return nil, err - } + // ID creation and other validations; This is more useful for new entities + // and may not perform anything for the existing entities. Placing the + // check here to make the flow common for both new and existing entities. + err = i.sanitizeEntity(entity) + if err != nil { + return nil, err + } - // Update the fields - alias.Name = aliasName - alias.Metadata = aliasMetadata - alias.MountAccessor = mountValidationResp.MountAccessor + // Explicitly set to empty as in the past we incorrectly saved it + alias.MountPath = "" + alias.MountType = "" - // Explicitly set to empty as in the past we incorrectly saved it - alias.MountPath = "" - alias.MountType = "" + // Set the canonical ID in the alias index. This should be done after + // sanitizing entity. + alias.CanonicalID = entity.ID - // Set the canonical ID in the alias index. This should be done after - // sanitizing entity. - alias.CanonicalID = entity.ID + // ID creation and other validations + err = i.sanitizeAlias(alias) + if err != nil { + return nil, err + } - // ID creation and other validations - err = i.sanitizeAlias(alias) - if err != nil { - return nil, err - } + // Index entity and its aliases in MemDB and persist entity along with + // aliases in storage. If the alias is being transferred over from + // one entity to another, previous entity needs to get refreshed in MemDB + // and persisted in storage as well. + if err := i.upsertEntity(entity, previousEntity, true); err != nil { + return nil, err + } - // Index entity and its aliases in MemDB and persist entity along with - // aliases in storage. If the alias is being transferred over from - // one entity to another, previous entity needs to get refreshed in MemDB - // and persisted in storage as well. - err = i.upsertEntity(entity, previousEntity, true) - if err != nil { - return nil, err - } + // Return ID of both alias and entity + resp.Data = map[string]interface{}{ + "id": alias.ID, + "canonical_id": entity.ID, + } - // Return ID of both alias and entity - resp.Data = map[string]interface{}{ - "id": alias.ID, - "canonical_id": entity.ID, + return resp, nil } - - return resp, nil } // pathAliasIDRead returns the properties of an alias for a given @@ -420,40 +327,15 @@ func (i *IdentityStore) pathAliasIDDelete() framework.OperationFunc { return logical.ErrorResponse("missing alias ID"), nil } - // Fetch the alias using its ID - alias, err := i.MemDBAliasByID(aliasID, false, false) - if err != nil { - return nil, err - } - - // If there is no alias for the ID, do nothing - if alias == nil { - return nil, nil - } - - // Find the entity to which the alias is tied to - lockEntity, err := i.MemDBEntityByAliasID(alias.ID, false) - if err != nil { - return nil, err - } - - // If there is no entity tied to a valid alias, something is wrong - if lockEntity == nil { - return nil, fmt.Errorf("alias not associated to an entity") - } - - // Acquire the lock to modify the entity storage entry - lock := locksutil.LockForKey(i.entityLocks, lockEntity.ID) - lock.Lock() - defer lock.Unlock() + i.lock.Lock() + defer i.lock.Unlock() // Create a MemDB transaction to delete entity txn := i.db.Txn(true) defer txn.Abort() - // Fetch the alias again after acquiring the lock using the transaction - // created above - alias, err = i.MemDBAliasByIDInTxn(txn, aliasID, false, false) + // Fetch the alias + alias, err := i.MemDBAliasByIDInTxn(txn, aliasID, false, false) if err != nil { return nil, err } @@ -463,8 +345,7 @@ func (i *IdentityStore) pathAliasIDDelete() framework.OperationFunc { return nil, nil } - // Fetch the entity again after acquiring the lock using the transaction - // created above + // Fetch the associated entity entity, err := i.MemDBEntityByAliasIDInTxn(txn, alias.ID, true) if err != nil { return nil, err @@ -475,12 +356,6 @@ func (i *IdentityStore) pathAliasIDDelete() framework.OperationFunc { return nil, fmt.Errorf("alias not associated to an entity") } - // Lock switching should not end up in this code pointing to different - // entities - if lockEntity.ID != entity.ID { - return nil, fmt.Errorf("operating on an entity to which the lock doesn't belong to") - } - aliases := []*identity.Alias{ alias, } diff --git a/vault/identity_store_aliases_test.go b/vault/identity_store_aliases_test.go index 57102efaa9ec..e9b6012be9ad 100644 --- a/vault/identity_store_aliases_test.go +++ b/vault/identity_store_aliases_test.go @@ -200,7 +200,6 @@ func TestIdentityStore_AliasUpdate(t *testing.T) { aliasData := map[string]interface{}{ "name": "testaliasname", "mount_accessor": githubAccessor, - "metadata": []string{"organization=hashicorp", "team=vault"}, } aliasReq := &logical.Request{ @@ -219,7 +218,6 @@ func TestIdentityStore_AliasUpdate(t *testing.T) { updateData := map[string]interface{}{ "name": "updatedaliasname", "mount_accessor": githubAccessor, - "metadata": []string{"organization=updatedorganization", "team=updatedteam"}, } aliasReq.Data = updateData @@ -235,11 +233,7 @@ func TestIdentityStore_AliasUpdate(t *testing.T) { t.Fatalf("err:%v resp:%#v", err, resp) } - aliasMetadata := resp.Data["metadata"].(map[string]string) - updatedOrg := aliasMetadata["organization"] - updatedTeam := aliasMetadata["team"] - - if resp.Data["name"] != "updatedaliasname" || updatedOrg != "updatedorganization" || updatedTeam != "updatedteam" { + if resp.Data["name"] != "updatedaliasname" { t.Fatalf("failed to update alias information; \n response data: %#v\n", resp.Data) } } @@ -252,7 +246,6 @@ func TestIdentityStore_AliasUpdate_ByID(t *testing.T) { updateData := map[string]interface{}{ "name": "updatedaliasname", "mount_accessor": githubAccessor, - "metadata": []string{"organization=updatedorganization", "team=updatedteam"}, } updateReq := &logical.Request{ @@ -273,7 +266,6 @@ func TestIdentityStore_AliasUpdate_ByID(t *testing.T) { registerData := map[string]interface{}{ "name": "testaliasname", "mount_accessor": githubAccessor, - "metadata": []string{"organization=hashicorp", "team=vault"}, } registerReq := &logical.Request{ @@ -311,11 +303,7 @@ func TestIdentityStore_AliasUpdate_ByID(t *testing.T) { t.Fatalf("err:%v resp:%#v", err, resp) } - aliasMetadata := resp.Data["metadata"].(map[string]string) - updatedOrg := aliasMetadata["organization"] - updatedTeam := aliasMetadata["team"] - - if resp.Data["name"] != "updatedaliasname" || updatedOrg != "updatedorganization" || updatedTeam != "updatedteam" { + if resp.Data["name"] != "updatedaliasname" { t.Fatalf("failed to update alias information; \n response data: %#v\n", resp.Data) } diff --git a/vault/identity_store_entities.go b/vault/identity_store_entities.go index 904275a7bc26..cc3231d2a917 100644 --- a/vault/identity_store_entities.go +++ b/vault/identity_store_entities.go @@ -9,7 +9,6 @@ import ( "github.com/hashicorp/errwrap" memdb "github.com/hashicorp/go-memdb" "github.com/hashicorp/vault/helper/identity" - "github.com/hashicorp/vault/helper/locksutil" "github.com/hashicorp/vault/helper/storagepacker" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" @@ -51,7 +50,7 @@ vault metadata=key1=value1 metadata=key2=value2 }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: i.pathEntityRegister(), + logical.UpdateOperation: i.handleEntityUpdateCommon(), }, HelpSynopsis: strings.TrimSpace(entityHelp["entity"][0]), @@ -86,7 +85,7 @@ vault metadata=key1=value1 metadata=key2=value2 }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: i.pathEntityIDUpdate(), + logical.UpdateOperation: i.handleEntityUpdateCommon(), logical.ReadOperation: i.pathEntityIDRead(), logical.DeleteOperation: i.pathEntityIDDelete(), }, @@ -144,25 +143,13 @@ func (i *IdentityStore) pathEntityMergeID() framework.OperationFunc { force := d.Get("force").(bool) - toEntityForLocking, err := i.MemDBEntityByID(toEntityID, false) - if err != nil { - return nil, err - } - - if toEntityForLocking == nil { - return logical.ErrorResponse("entity id to merge to is invalid"), nil - } - - // Acquire the lock to modify the entity storage entry to merge to - toEntityLock := locksutil.LockForKey(i.entityLocks, toEntityForLocking.ID) - toEntityLock.Lock() - defer toEntityLock.Unlock() + i.lock.Lock() + defer i.lock.Unlock() // Create a MemDB transaction to merge entities txn := i.db.Txn(true) defer txn.Abort() - // Re-read post lock acquisition toEntity, err := i.MemDBEntityByID(toEntityID, true) if err != nil { return nil, err @@ -172,61 +159,21 @@ func (i *IdentityStore) pathEntityMergeID() framework.OperationFunc { return logical.ErrorResponse("entity id to merge to is invalid"), nil } - if toEntity.ID != toEntityForLocking.ID { - return logical.ErrorResponse("acquired lock for an undesired entity"), nil - } - var conflictErrors error for _, fromEntityID := range fromEntityIDs { if fromEntityID == toEntityID { return logical.ErrorResponse("to_entity_id should not be present in from_entity_ids"), nil } - lockFromEntity, err := i.MemDBEntityByID(fromEntityID, false) - if err != nil { - return nil, err - } - - if lockFromEntity == nil { - return logical.ErrorResponse("entity id to merge from is invalid"), nil - } - - // Acquire the lock to modify the entity storage entry to merge from - fromEntityLock := locksutil.LockForKey(i.entityLocks, lockFromEntity.ID) - - fromLockHeld := false - - // There are only 256 lock buckets and the chances of entity ID collision - // is fairly high. When we are merging entities belonging to the same - // bucket, multiple attempts to acquire the same lock should be avoided. - if fromEntityLock != toEntityLock { - fromEntityLock.Lock() - fromLockHeld = true - } - - // Re-read the entities post lock acquisition fromEntity, err := i.MemDBEntityByID(fromEntityID, false) if err != nil { - if fromLockHeld { - fromEntityLock.Unlock() - } return nil, err } if fromEntity == nil { - if fromLockHeld { - fromEntityLock.Unlock() - } return logical.ErrorResponse("entity id to merge from is invalid"), nil } - if fromEntity.ID != lockFromEntity.ID { - if fromLockHeld { - fromEntityLock.Unlock() - } - return logical.ErrorResponse("acquired lock for an undesired entity"), nil - } - for _, alias := range fromEntity.Aliases { // Set the desired canonical ID alias.CanonicalID = toEntity.ID @@ -235,9 +182,6 @@ func (i *IdentityStore) pathEntityMergeID() framework.OperationFunc { err = i.MemDBUpsertAliasInTxn(txn, alias, false) if err != nil { - if fromLockHeld { - fromEntityLock.Unlock() - } return nil, errwrap.Wrapf("failed to update alias during merge: {{err}}", err) } @@ -257,24 +201,14 @@ func (i *IdentityStore) pathEntityMergeID() framework.OperationFunc { // Delete the entity which we are merging from in MemDB using the same transaction err = i.MemDBDeleteEntityByIDInTxn(txn, fromEntity.ID) if err != nil { - if fromLockHeld { - fromEntityLock.Unlock() - } return nil, err } // Delete the entity which we are merging from in storage err = i.entityPacker.DeleteItem(fromEntity.ID) if err != nil { - if fromLockHeld { - fromEntityLock.Unlock() - } return nil, err } - - if fromLockHeld { - fromEntityLock.Unlock() - } } if conflictErrors != nil && !force { @@ -310,114 +244,105 @@ func (i *IdentityStore) pathEntityMergeID() framework.OperationFunc { } } -// pathEntityRegister is used to register a new entity -func (i *IdentityStore) pathEntityRegister() framework.OperationFunc { +// handleEntityUpdateCommon is used to update an entity +func (i *IdentityStore) handleEntityUpdateCommon() framework.OperationFunc { return func(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - _, ok := d.GetOk("id") - if ok { - return i.pathEntityIDUpdate()(ctx, req, d) - } + i.lock.Lock() + defer i.lock.Unlock() - return i.handleEntityUpdateCommon(req, d, nil) - } -} + var entity *identity.Entity + var err error -// pathEntityIDUpdate is used to update an entity based on the given entity ID -func (i *IdentityStore) pathEntityIDUpdate() framework.OperationFunc { - return func(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) { - // Get entity id entityID := d.Get("id").(string) - - if entityID == "" { - return logical.ErrorResponse("missing entity id"), nil + if entityID != "" { + entity, err = i.MemDBEntityByID(entityID, true) + if err != nil { + return nil, err + } + if entity == nil { + return logical.ErrorResponse("entity not found from id"), nil + } } - entity, err := i.MemDBEntityByID(entityID, true) - if err != nil { - return nil, err + // Get the name + entityName := d.Get("name").(string) + if entityName != "" { + entityByName, err := i.MemDBEntityByName(entityName, false) + if err != nil { + return nil, err + } + switch { + case entityByName == nil: + // Not found, safe to use this name with an existing or new entity + case entity == nil: + // We found an entity by name, but we don't currently allow + // updating based on name, only ID, so return an error + return logical.ErrorResponse("entity name is already in use"), nil + case entity.ID == entityByName.ID: + // Same exact entity, carry on (this is basically a noop then) + default: + return logical.ErrorResponse("entity name is already in use"), nil + } } + + // Entity will be nil when a new entity is being registered; create a new + // struct in that case. if entity == nil { - return nil, fmt.Errorf("invalid entity id") + entity = &identity.Entity{} + } + if entityName != "" { + entity.Name = entityName } - return i.handleEntityUpdateCommon(req, d, entity) - } -} - -// handleEntityUpdateCommon is used to update an entity -func (i *IdentityStore) handleEntityUpdateCommon(req *logical.Request, d *framework.FieldData, entity *identity.Entity) (*logical.Response, error) { - var err error - var newEntity bool - - // Entity will be nil when a new entity is being registered; create a new - // struct in that case. - if entity == nil { - entity = &identity.Entity{} - newEntity = true - } - - // Update the policies if supplied - entityPoliciesRaw, ok := d.GetOk("policies") - if ok { - entity.Policies = entityPoliciesRaw.([]string) - } + // Update the policies if supplied + entityPoliciesRaw, ok := d.GetOk("policies") + if ok { + entity.Policies = entityPoliciesRaw.([]string) + } - disabledRaw, ok := d.GetOk("disabled") - if ok { - entity.Disabled = disabledRaw.(bool) - } + disabledRaw, ok := d.GetOk("disabled") + if ok { + entity.Disabled = disabledRaw.(bool) + } - // Get the name - entityName := d.Get("name").(string) - if entityName != "" { - entityByName, err := i.MemDBEntityByName(entityName, false) + // Get entity metadata + metadata, ok, err := d.GetOkErr("metadata") if err != nil { - return nil, err + return logical.ErrorResponse(fmt.Sprintf("failed to parse metadata: %v", err)), nil + } + if ok { + entity.Metadata = metadata.(map[string]string) } - switch { - case (newEntity && entityByName != nil), (entityByName != nil && entity.ID != "" && entityByName.ID != entity.ID): - return logical.ErrorResponse("entity name is already in use"), nil + // ID creation and some validations + err = i.sanitizeEntity(entity) + if err != nil { + return nil, err } - entity.Name = entityName - } - // Get entity metadata - metadata, ok, err := d.GetOkErr("metadata") - if err != nil { - return logical.ErrorResponse(fmt.Sprintf("failed to parse metadata: %v", err)), nil - } - if ok { - entity.Metadata = metadata.(map[string]string) - } - // ID creation and some validations - err = i.sanitizeEntity(entity) - if err != nil { - return nil, err - } + // Prepare the response + respData := map[string]interface{}{ + "id": entity.ID, + } - // Prepare the response - respData := map[string]interface{}{ - "id": entity.ID, - } + var aliasIDs []string + for _, alias := range entity.Aliases { + aliasIDs = append(aliasIDs, alias.ID) + } - var aliasIDs []string - for _, alias := range entity.Aliases { - aliasIDs = append(aliasIDs, alias.ID) - } + respData["aliases"] = aliasIDs - respData["aliases"] = aliasIDs + // Update MemDB and persist entity object. New entities have not been + // looked up yet so we need to take the lock on the entity on upsert + if err := i.upsertEntity(entity, nil, true); err != nil { + return nil, err + } - // Update MemDB and persist entity object - err = i.upsertEntity(entity, nil, true) - if err != nil { - return nil, err + // Return ID of the entity that was either created or updated along with + // its aliases + return &logical.Response{ + Data: respData, + }, nil } - - // Return ID of the entity that was either created or updated along with - // its aliases - return &logical.Response{ - Data: respData, - }, nil } // pathEntityIDRead returns the properties of an entity for a given entity ID @@ -511,21 +436,8 @@ func (i *IdentityStore) pathEntityIDDelete() framework.OperationFunc { return logical.ErrorResponse("missing entity id"), nil } - // Since an entity ID is required to acquire the lock to modify the - // storage, fetch the entity without acquiring the lock - lockEntity, err := i.MemDBEntityByID(entityID, false) - if err != nil { - return nil, err - } - - if lockEntity == nil { - return nil, nil - } - - // Acquire the lock to modify the entity storage entry - lock := locksutil.LockForKey(i.entityLocks, lockEntity.ID) - lock.Lock() - defer lock.Unlock() + i.lock.Lock() + defer i.lock.Unlock() // Create a MemDB transaction to delete entity txn := i.db.Txn(true) diff --git a/vault/identity_store_structs.go b/vault/identity_store_structs.go index 0f9435cf7fc3..c9ddb245bd11 100644 --- a/vault/identity_store_structs.go +++ b/vault/identity_store_structs.go @@ -7,7 +7,6 @@ import ( log "github.com/hashicorp/go-hclog" memdb "github.com/hashicorp/go-memdb" "github.com/hashicorp/vault/helper/identity" - "github.com/hashicorp/vault/helper/locksutil" "github.com/hashicorp/vault/helper/storagepacker" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" @@ -52,9 +51,8 @@ type IdentityStore struct { // to enable richer queries based on multiple indexes. db *memdb.MemDB - // entityLocks are a set of 256 locks to which all the entities will be - // categorized to while performing storage modifications. - entityLocks []*locksutil.LockEntry + // A lock to make sure things are consistent + lock sync.RWMutex // groupLock is used to protect modifications to group entries groupLock sync.RWMutex diff --git a/vault/identity_store_upgrade.go b/vault/identity_store_upgrade.go index 9399e6259990..ebf3e5582f32 100644 --- a/vault/identity_store_upgrade.go +++ b/vault/identity_store_upgrade.go @@ -14,23 +14,23 @@ func upgradePaths(i *IdentityStore) []*framework.Path { Fields: map[string]*framework.FieldSchema{ "id": { Type: framework.TypeString, - Description: "ID of the alias", + Description: "ID of the persona", }, "entity_id": { Type: framework.TypeString, - Description: "Entity ID to which this alias belongs to", + Description: "Entity ID to which this persona belongs to", }, "mount_accessor": { Type: framework.TypeString, - Description: "Mount accessor to which this alias belongs to", + Description: "Mount accessor to which this persona belongs to", }, "name": { Type: framework.TypeString, - Description: "Name of the alias", + Description: "Name of the persona", }, "metadata": { Type: framework.TypeKVPairs, - Description: `Metadata to be associated with the alias. + Description: `Metadata to be associated with the persona. In CLI, this parameter can be repeated multiple times, and it all gets merged together. For example: vault metadata=key1=value1 metadata=key2=value2 @@ -38,7 +38,7 @@ vault metadata=key1=value1 metadata=key2=value2 }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: i.pathAliasRegister(), + logical.UpdateOperation: i.handleEntityUpdateCommon(), }, HelpSynopsis: strings.TrimSpace(aliasHelp["alias"][0]), @@ -49,23 +49,23 @@ vault metadata=key1=value1 metadata=key2=value2 Fields: map[string]*framework.FieldSchema{ "id": { Type: framework.TypeString, - Description: "ID of the alias", + Description: "ID of the persona", }, "entity_id": { Type: framework.TypeString, - Description: "Entity ID to which this alias should be tied to", + Description: "Entity ID to which this persona should be tied to", }, "mount_accessor": { Type: framework.TypeString, - Description: "Mount accessor to which this alias belongs to", + Description: "Mount accessor to which this persona belongs to", }, "name": { Type: framework.TypeString, - Description: "Name of the alias", + Description: "Name of the persona", }, "metadata": { Type: framework.TypeKVPairs, - Description: `Metadata to be associated with the alias. + Description: `Metadata to be associated with the persona. In CLI, this parameter can be repeated multiple times, and it all gets merged together. For example: vault metadata=key1=value1 metadata=key2=value2 @@ -73,7 +73,7 @@ vault metadata=key1=value1 metadata=key2=value2 }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: i.pathAliasIDUpdate(), + logical.UpdateOperation: i.handleEntityUpdateCommon(), logical.ReadOperation: i.pathAliasIDRead(), logical.DeleteOperation: i.pathAliasIDDelete(), }, @@ -113,17 +113,9 @@ vault metadata=key1=value1 metadata=key2=value2 Type: framework.TypeString, Description: "Name of the alias", }, - "metadata": { - Type: framework.TypeKVPairs, - Description: `Metadata to be associated with the alias. -In CLI, this parameter can be repeated multiple times, and it all gets merged together. -For example: -vault metadata=key1=value1 metadata=key2=value2 -`, - }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: i.pathAliasRegister(), + logical.UpdateOperation: i.handleAliasUpdateCommon(), }, HelpSynopsis: strings.TrimSpace(aliasHelp["alias"][0]), @@ -153,17 +145,9 @@ vault metadata=key1=value1 metadata=key2=value2 Type: framework.TypeString, Description: "Name of the alias", }, - "metadata": { - Type: framework.TypeKVPairs, - Description: `Metadata to be associated with the alias. -In CLI, this parameter can be repeated multiple times, and it all gets merged together. -For example: -vault metadata=key1=value1 metadata=key2=value2 -`, - }, }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.UpdateOperation: i.pathAliasIDUpdate(), + logical.UpdateOperation: i.handleAliasUpdateCommon(), logical.ReadOperation: i.pathAliasIDRead(), logical.DeleteOperation: i.pathAliasIDDelete(), }, diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index a860ba09877c..ab580ef1db26 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -12,7 +12,6 @@ import ( uuid "github.com/hashicorp/go-uuid" "github.com/hashicorp/vault/helper/consts" "github.com/hashicorp/vault/helper/identity" - "github.com/hashicorp/vault/helper/locksutil" "github.com/hashicorp/vault/helper/storagepacker" "github.com/hashicorp/vault/helper/strutil" "github.com/hashicorp/vault/logical" @@ -46,9 +45,6 @@ func (i *IdentityStore) loadGroups(ctx context.Context) error { } i.logger.Debug("groups collected", "num_existing", len(existing)) - i.groupLock.Lock() - defer i.groupLock.Unlock() - for _, key := range existing { bucket, err := i.groupPacker.GetBucket(i.groupPacker.BucketPath(key)) if err != nil { @@ -208,18 +204,13 @@ func (i *IdentityStore) loadEntities(ctx context.Context) error { return nil } -// LockForEntityID returns the lock used to modify the entity. -func (i *IdentityStore) LockForEntityID(entityID string) *locksutil.LockEntry { - return locksutil.LockForKey(i.entityLocks, entityID) -} - // 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 // one entity to another, both the source and destination entities should get // updated, in which case, callers should send in both entity and // previousEntity. -func (i *IdentityStore) upsertEntityInTxn(txn *memdb.Txn, entity *identity.Entity, previousEntity *identity.Entity, persist, lockHeld bool) error { +func (i *IdentityStore) upsertEntityInTxn(txn *memdb.Txn, entity *identity.Entity, previousEntity *identity.Entity, persist bool) error { var err error if txn == nil { @@ -230,13 +221,6 @@ func (i *IdentityStore) upsertEntityInTxn(txn *memdb.Txn, entity *identity.Entit return fmt.Errorf("entity is nil") } - // Acquire the lock to modify the entity storage entry - if !lockHeld { - lock := locksutil.LockForKey(i.entityLocks, entity.ID) - lock.Lock() - defer lock.Unlock() - } - for _, alias := range entity.Aliases { // Verify that alias is not associated to a different one already aliasByFactors, err := i.MemDBAliasByFactors(alias.MountAccessor, alias.Name, false, false) @@ -314,24 +298,7 @@ func (i *IdentityStore) upsertEntity(entity *identity.Entity, previousEntity *id txn := i.db.Txn(true) defer txn.Abort() - err := i.upsertEntityInTxn(txn, entity, previousEntity, persist, false) - if err != nil { - return err - } - - txn.Commit() - - return nil -} - -// upsertEntityNonLocked creates or updates an entity. The lock to modify the -// entity should be held before calling this function. -func (i *IdentityStore) upsertEntityNonLocked(entity *identity.Entity, previousEntity *identity.Entity, persist bool) error { - // Create a MemDB transaction to update both alias and entity - txn := i.db.Txn(true) - defer txn.Abort() - - err := i.upsertEntityInTxn(txn, entity, previousEntity, persist, true) + err := i.upsertEntityInTxn(txn, entity, previousEntity, persist) if err != nil { return err }