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 3 commits
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
1 change: 1 addition & 0 deletions vault/identity_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ func NewIdentityStore(ctx context.Context, core *Core, config *logical.BackendCo
view: config.StorageView,
db: db,
entityLocks: locksutil.CreateLocks(),
aliasLocks: locksutil.CreateLocks(),
logger: logger,
core: core,
}
Expand Down
95 changes: 67 additions & 28 deletions vault/identity_store_aliases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ 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 to.
This field is deprecated, use canonical_id.`,
},
"canonical_id": {
Type: framework.TypeString,
Expand Down Expand Up @@ -71,8 +72,9 @@ vault <command> <path> 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,
Expand Down Expand Up @@ -111,8 +113,9 @@ vault <command> <path> 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,
Expand Down Expand Up @@ -179,6 +182,10 @@ func (i *IdentityStore) pathAliasIDUpdate() framework.OperationFunc {
return logical.ErrorResponse("empty alias ID"), nil
}

lock := locksutil.LockForKey(i.aliasLocks, aliasID)
lock.Lock()
defer lock.Unlock()

alias, err := i.MemDBAliasByID(aliasID, true, false)
if err != nil {
return nil, err
Expand All @@ -197,6 +204,7 @@ func (i *IdentityStore) handleAliasUpdateCommon(req *logical.Request, d *framewo
var newAlias bool
var entity *identity.Entity
var previousEntity *identity.Entity
var lockKeys []string

// Alias will be nil when a new alias is being registered; create a
// new struct in that case.
Expand All @@ -206,19 +214,13 @@ 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)
// For backwards compatibility
canonicalID = d.Get("entity_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
}
lockKeys = append(lockKeys, canonicalID)
}

// Get alias name
Expand Down Expand Up @@ -256,8 +258,46 @@ 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?

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

}

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

if canonicalID != "" {
entity, err = i.MemDBEntityByID(canonicalID, true)
if err != nil {
return nil, err
}
if entity == nil {
return logical.ErrorResponse("invalid canonical ID"), nil
}
}
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{}

var newEntity bool
if newAlias {
if aliasByFactors != nil {
return logical.ErrorResponse("combination of mount and alias name is already in use"), nil
Expand All @@ -271,22 +311,16 @@ func (i *IdentityStore) handleAliasUpdateCommon(req *logical.Request, d *framewo
alias,
},
}
newEntity = true
} 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)
// Reread existing entity now that we have the lock on the entity id
existingEntity, err := i.MemDBEntityByID(existingEntity.ID, true)
if err != nil {
return nil, err
}

if existingEntity == nil {
return nil, fmt.Errorf("alias is not associated with an entity")
}
Expand Down Expand Up @@ -353,9 +387,14 @@ 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)
if err != nil {
return nil, err
if newEntity {
if err := i.upsertEntity(entity, previousEntity, true); err != nil {
return nil, err
}
} else {
if err := i.upsertEntityNonLocked(entity, previousEntity, true); err != nil {
return nil, err
}
}

// Return ID of both alias and entity
Expand Down
19 changes: 15 additions & 4 deletions 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 @@ -407,10 +412,16 @@ func (i *IdentityStore) handleEntityUpdateCommon(req *logical.Request, d *framew

respData["aliases"] = aliasIDs

// Update MemDB and persist entity object
err = i.upsertEntity(entity, nil, true)
if err != nil {
return nil, err
// 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 newEntity {
if err := i.upsertEntity(entity, nil, true); err != nil {
return nil, err
}
} else {
if err := i.upsertEntityNonLocked(entity, nil, true); err != nil {
return nil, err
}
}

// Return ID of the entity that was either created or updated along with
Expand Down
4 changes: 4 additions & 0 deletions vault/identity_store_structs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ type IdentityStore struct {
// categorized to while performing storage modifications.
entityLocks []*locksutil.LockEntry

// aliasLocks are a set of 256 locks to which all the aliases will be
// categorized to while performing storage modifications.
aliasLocks []*locksutil.LockEntry

// groupLock is used to protect modifications to group entries
groupLock sync.RWMutex

Expand Down