Token store deleted parent #4193
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1169,6 +1169,33 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedId string) (ret er | |
| } | ||
| } | ||
|
|
||
| // Mark all children token as orphan by removing | ||
| // their parent index, and clear the parent entry | ||
| parentPath := parentPrefix + saltedId + "/" | ||
| children, err := ts.view.List(ctx, parentPath) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to scan for children: %v", err) | ||
| } | ||
| for _, child := range children { | ||
| entry, err := ts.lookupSalted(ctx, child, true) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get child token: %v", err) | ||
| } | ||
| lock := locksutil.LockForKey(ts.tokenLocks, entry.ID) | ||
|
There was a problem hiding this comment. If this lock happens to be the same lock that is being used outside of this loop, acquiring this will deadlock. No? |
||
| lock.Lock() | ||
|
|
||
| entry.Parent = "" | ||
| err = ts.store(ctx, entry) | ||
| if err != nil { | ||
| lock.Unlock() | ||
| return fmt.Errorf("failed to update child token: %v", err) | ||
| } | ||
| lock.Unlock() | ||
| } | ||
| if err = logical.ClearView(ctx, ts.view.SubView(parentPath)); err != nil { | ||
| return fmt.Errorf("failed to delete entry: %v", err) | ||
| } | ||
|
|
||
| // Now that the entry is not usable for any revocation tasks, nuke it | ||
| path := lookupPrefix + saltedId | ||
| if err = ts.view.Delete(ctx, path); err != nil { | ||
|
|
@@ -1322,17 +1349,30 @@ func (ts *TokenStore) handleTidy(ctx context.Context, req *logical.Request, data | |
| return nil, fmt.Errorf("failed to fetch secondary index entries: %v", err) | ||
| } | ||
|
|
||
| var countParentEntries, deletedCountParentEntries, countParentList, deletedCountParentList int64 | ||
|
|
||
| // Scan through the secondary index entries; if there is an entry | ||
| // with the token's salt ID at the end, remove it | ||
| for _, parent := range parentList { | ||
| countParentEntries++ | ||
|
|
||
| // Get the children | ||
| children, err := ts.view.List(ctx, parentPrefix+parent) | ||
| if err != nil { | ||
| tidyErrors = multierror.Append(tidyErrors, fmt.Errorf("failed to read secondary index: %v", err)) | ||
| continue | ||
| } | ||
|
|
||
| // First check if the salt ID of the parent exists, and if not mark this so | ||
| // that deletion of children later with this loop below applies to all | ||
| // children | ||
| originalChildrenCount := int64(len(children)) | ||
| exists, _ := ts.lookupSalted(ctx, strings.TrimSuffix(parent, "/"), true) | ||
| if exists == nil { | ||
| ts.logger.Trace("token: deleting invalid parent prefix entry", "index", parentPrefix+parent) | ||
| } | ||
|
|
||
| var deletedChildrenCount int64 | ||
| for _, child := range children { | ||
| countParentList++ | ||
| if countParentList%500 == 0 { | ||
|
|
@@ -1342,16 +1382,49 @@ func (ts *TokenStore) handleTidy(ctx context.Context, req *logical.Request, data | |
| // Look up tainted entries so we can be sure that if this isn't | ||
| // found, it doesn't exist. Doing the following without locking | ||
| // since appropriate locks cannot be held with salted token IDs. | ||
| // Also perform deletion if the parent doesn't exist any more. | ||
| te, _ := ts.lookupSalted(ctx, child, true) | ||
| // If the child entry is not nil, but the parent doesn't exist, then turn | ||
| // that child token into an orphan token. Theres no deletion in this case. | ||
| if te != nil && exists == nil { | ||
| lock := locksutil.LockForKey(ts.tokenLocks, te.ID) | ||
| lock.Lock() | ||
|
|
||
| te.Parent = "" | ||
| err = ts.store(ctx, te) | ||
| if err != nil { | ||
| tidyErrors = multierror.Append(tidyErrors, fmt.Errorf("failed to convert child token into an orphan token: %v", err)) | ||
| } | ||
| lock.Unlock() | ||
| continue | ||
| } | ||
| // Otherwise, if the entry doesn't exist, or if the parent doesn't exist go | ||
| // on with the delete on the secondary index | ||
| if te == nil || exists == nil { | ||
| index := parentPrefix + parent + child | ||
| ts.logger.Trace("token: deleting invalid secondary index", "index", index) | ||
| err = ts.view.Delete(ctx, index) | ||
| if err != nil { | ||
| tidyErrors = multierror.Append(tidyErrors, fmt.Errorf("failed to delete secondary index: %v", err)) | ||
| continue | ||
| } | ||
| deletedChildrenCount++ | ||
| } | ||
| } | ||
| // Add current children deleted count to the total count | ||
| deletedCountParentList += deletedChildrenCount | ||
| // If we deleted all the children, then add that to our deleted parent entries count | ||
|
There was a problem hiding this comment. Should the parent prefix be cleaned out here if There was a problem hiding this comment. Consul will remove the prefix entry if the "directory" is empty so I didn't have an explicit delete in here, but I guess we can't guarantee that this is the behavior for other storage backends so I might be good to explicitly call a delete (though I'd do it inside |
||
| if originalChildrenCount == deletedChildrenCount { | ||
| // Make sure that we only delete if parent doesn't exist. | ||
| // N.B.: This is a no-op for the consul storage backend since the delete doesn't support | ||
| // recursive deletes, and delete on a path is a no-op. | ||
| if exists == nil { | ||
| err := ts.view.Delete(ctx, parentPrefix+parent) | ||
|
There was a problem hiding this comment. @jefferai @vishalnayak I added the explicit delete in here to ensure that the parent prefix gets deleted if there are no children as per previous discussion in here. In the case for Consul, it should automatically remove this (and even if it didn't, this would result in a no-op since the underlying ConsulBackend.Delete would have to call DeleteTree to handle tree deletions and deletes on a non-existing key is simply treated as a success). However, other storage backend implementations might error on this since the delete method is usually done against an entry, and not a path/tree (thinking that this might be an issue for Cassandra's delete, for instance). This would result in the tidy operation being treated as a failure, so I wonder if we should a) not do this at all or b) treat this as warning instead of an error. There was a problem hiding this comment. @calvn Apologies -- I think this needs to come back out. You're right that we don't know what storage implementations will do, but I believe the expected behavior is that they clean up their own empty prefixes. I remember making this change with There was a problem hiding this comment. No worries, I'll revert! Glad this discussion happened; I learn something new every day :) |
||
| if err != nil { | ||
| tidyErrors = multierror.Append(tidyErrors, fmt.Errorf("failed to delete parent prefix entry: %v", err)) | ||
| } | ||
| } | ||
| deletedCountParentEntries++ | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1447,6 +1520,8 @@ func (ts *TokenStore) handleTidy(ctx context.Context, req *logical.Request, data | |
| } | ||
| } | ||
|
|
||
| ts.logger.Debug("token: number of entries scanned in parent prefix", "count", countParentEntries) | ||
| ts.logger.Debug("token: number of entries deleted in parent prefix", "count", deletedCountParentEntries) | ||
| ts.logger.Debug("token: number of tokens scanned in parent index list", "count", countParentList) | ||
| ts.logger.Debug("token: number of tokens revoked in parent index list", "count", deletedCountParentList) | ||
| ts.logger.Debug("token: number of accessors scanned", "count", countAccessorList) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add to this comment explaining that marking orphan is correct since revokeTree will ensure that any are deleted anyways if it is not an explicit call to orphan the child tokens.