Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add locking when adding aliases to existing entities #4965

Merged
merged 4 commits into from
Jul 25, 2018
Merged

Add locking when adding aliases to existing entities #4965

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c980863
Add locking when adding aliases to existing entities
Jul 20, 2018
e53d808
adding alias locks and redoing entity locking
Jul 21, 2018
181efbb
adding back a nil check
Jul 21, 2018
0738940
Redo locking in entities/aliases to simplify logic
jefferai Jul 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 49 additions & 29 deletions vault/identity_store_aliases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,19 +206,10 @@ func (i *IdentityStore) handleAliasUpdateCommon(req *logical.Request, d *framewo
}

// Get entity id
canonicalID := d.Get("entity_id").(string)
canonicalID := d.Get("canonical_id").(string)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also put a deprecation notice for entity_id's pathHelp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if canonicalID == "" {
canonicalID = d.Get("canonical_id").(string)
}

if canonicalID != "" {
entity, err = i.MemDBEntityByID(canonicalID, true)
if err != nil {
return nil, err
}
if entity == nil {
return logical.ErrorResponse("invalid entity ID"), nil
}
// For backwards compatibility
canonicalID = d.Get("entity_id").(string)
}

// Get alias name
Expand Down Expand Up @@ -256,6 +247,51 @@ func (i *IdentityStore) handleAliasUpdateCommon(req *logical.Request, d *framewo
return nil, err
}

var existingEntity *identity.Entity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you need to grab the alias lock before the MemDBAliasByFactors call above? Otherwise what happens if an automatic call is registering an alias at the same time as this manual call?

var lockKeys []string
if !newAlias {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part should be after the locks are grabbed. The MemDB stuff happens in a transaction but it's still possible for that transaction to say, for instance, that there is no existingEntity, then have such an entity be created after the locking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we don't know the id of the entity to grab the lock for. This is what I alluded to as still a race but since we have to look it up, I'm not sure the alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One way to alleviate this issue would be to add locks for alias IDs. I'll went down that path at one point but pulled it out since it was only really used here. I think that would fully take care of this issue and will do that.

// 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")
}
lockKeys = append(lockKeys, existingEntity.ID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that canonical_id and existingEntity.ID can be the same ID, i think they can if you're updating the alias and keeping the same entityID. If so i think the below lock acquisition will deadlock

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it looks like the LocksForKeys function will dedup them for you

}

if canonicalID != "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you move lockKeys up to the top and add in the canonicalID up then, if we have one, you can pull the LocksForKeys and lock/unlock lines out of this if/else

// Acquire the lock to modify the entity storage entry
locks := locksutil.LocksForKeys(i.entityLocks, append(lockKeys, canonicalID))
for _, lock := range locks {
lock.Lock()
defer lock.Unlock()
}

entity, err = i.MemDBEntityByID(canonicalID, true)
if err != nil {
return nil, err
}
if entity == nil {
return logical.ErrorResponse("invalid canonical ID"), nil
}
} else {
// Acquire the lock to modify the entity storage entry
locks := locksutil.LocksForKeys(i.entityLocks, lockKeys)
for _, lock := range locks {
lock.Lock()
defer lock.Unlock()
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this it should probably check again to see whether the Alias is still a part of this entity.


resp := &logical.Response{}

if newAlias {
Expand All @@ -275,22 +311,6 @@ func (i *IdentityStore) handleAliasUpdateCommon(req *logical.Request, d *framewo
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 {
Expand Down Expand Up @@ -353,7 +373,7 @@ func (i *IdentityStore) handleAliasUpdateCommon(req *logical.Request, d *framewo
// 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)
err = i.upsertEntityNonLocked(entity, previousEntity, true)
if err != nil {
return nil, err
}
Expand Down
7 changes: 6 additions & 1 deletion vault/identity_store_entities.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,11 @@ func (i *IdentityStore) pathEntityIDUpdate() framework.OperationFunc {
return logical.ErrorResponse("missing entity id"), nil
}

// Acquire the lock to modify the entity storage entry
lock := locksutil.LockForKey(i.entityLocks, entityID)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to add this in pathEntityRegister too since it also calls handleEntityUpdateComon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only case left in pathEntityRegister is new entities since when the id is provided it comes through this method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but you changed the function in handleEntityUpdateCommon to use a non-locking version, and now if it goes through pathEntityRegister and don't provide an ID that function is called without a lock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added back the lock on a new entity by calling upsertEntity for new entities.

lock.Lock()
defer lock.Unlock()

entity, err := i.MemDBEntityByID(entityID, true)
if err != nil {
return nil, err
Expand Down Expand Up @@ -408,7 +413,7 @@ func (i *IdentityStore) handleEntityUpdateCommon(req *logical.Request, d *framew
respData["aliases"] = aliasIDs

// Update MemDB and persist entity object
err = i.upsertEntity(entity, nil, true)
err = i.upsertEntityNonLocked(entity, nil, true)
if err != nil {
return nil, err
}
Expand Down