Token store deleted parent (#4193)
* Handle removal of parent index on revoke-orphan and tidy operations * Refactor handleTidy to use same for loop children deletion of invalid parent entry * Update comments * Add logic for revoke-orphan and tidy to turn no-parent tokens into orphans * Add orphan check to test * Update test comments * Fix TestTokenStore_Revoke_Orphan test * Address feedback, add explicit delete when parent prefix is empty * Revert explicit delete, add comment on why it's not done * Update comment to indicate ok on marking token as orphan * Fix test
This commit is contained in:
Calvin Leung Huang
GitHub
parent
d03056eed3
commit
8d9295c539
|
|
@ -1169,6 +1169,39 @@ 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.
|
||||
//
|
||||
// Marking the token as orphan is the correct behavior in here since
|
||||
// revokeTreeSalted will ensure that they are deleted anyways if it's not an
|
||||
// explicit call to orphan the child tokens (the delete occurs at the leaf
|
||||
// node and uses parent prefix, not entry.Parent, to build the tree for
|
||||
// traversal).
|
||||
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)
|
||||
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 +1355,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 countParentList, deletedCountParentList int64
|
||||
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,17 +1388,43 @@ 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 te == nil {
|
||||
// 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
|
||||
}
|
||||
deletedCountParentList++
|
||||
deletedChildrenCount++
|
||||
}
|
||||
}
|
||||
// Add current children deleted count to the total count
|
||||
deletedCountParentList += deletedChildrenCount
|
||||
// N.B.: We don't call delete on the parent prefix since physical.Backend.Delete
|
||||
// implementations should be in charge of deleting empty prefixes.
|
||||
// If we deleted all the children, then add that to our deleted parent entries count.
|
||||
if originalChildrenCount == deletedChildrenCount {
|
||||
deletedCountParentEntries++
|
||||
}
|
||||
}
|
||||
|
||||
var countAccessorList,
|
||||
|
|
@ -1447,6 +1519,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)
|
||||
|
|
|
|||
|
|
@ -758,6 +758,10 @@ func TestTokenStore_Revoke_Orphan(t *testing.T) {
|
|||
if err != nil {
|
||||
t.Fatalf("err: %v", err)
|
||||
}
|
||||
|
||||
// Unset the expected token parent's ID
|
||||
ent2.Parent = ""
|
||||
|
||||
if !reflect.DeepEqual(out, ent2) {
|
||||
t.Fatalf("bad: expected:%#v\nactual:%#v", ent2, out)
|
||||
}
|
||||
|
|
@ -1417,6 +1421,19 @@ func TestTokenStore_HandleRequest_RevokeOrphan(t *testing.T) {
|
|||
t.Fatalf("bad: %v", out)
|
||||
}
|
||||
|
||||
// Check that the parent entry is properly cleaned up
|
||||
saltedID, err := ts.SaltID(context.Background(), "child")
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
children, err := ts.view.List(context.Background(), parentPrefix+saltedID+"/")
|
||||
if err != nil {
|
||||
t.Fatalf("err: %v", err)
|
||||
}
|
||||
if len(children) != 0 {
|
||||
t.Fatalf("bad: %v", children)
|
||||
}
|
||||
|
||||
// Sub-child should exist!
|
||||
out, err = ts.Lookup(context.Background(), "sub-child")
|
||||
if err != nil {
|
||||
|
|
@ -3514,6 +3531,166 @@ func TestTokenStore_HandleTidyCase1(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
// Create a set of tokens along with a child token for each of them, delete the
|
||||
// token entry while leaking accessors, invoke tidy and check if the dangling
|
||||
// accessor entry is getting removed and check if child tokens are still present
|
||||
// and turned into orphan tokens.
|
||||
func TestTokenStore_HandleTidy_parentCleanup(t *testing.T) {
|
||||
var resp *logical.Response
|
||||
var err error
|
||||
|
||||
_, ts, _, root := TestCoreWithTokenStore(t)
|
||||
|
||||
// List the number of accessors. Since there is only root token
|
||||
// present, the list operation should return only one key.
|
||||
accessorListReq := &logical.Request{
|
||||
Operation: logical.ListOperation,
|
||||
Path: "accessors/",
|
||||
ClientToken: root,
|
||||
}
|
||||
resp, err = ts.HandleRequest(context.Background(), accessorListReq)
|
||||
if err != nil || (resp != nil && resp.IsError()) {
|
||||
t.Fatalf("err:%v resp:%v", err, resp)
|
||||
}
|
||||
|
||||
numberOfAccessors := len(resp.Data["keys"].([]string))
|
||||
if numberOfAccessors != 1 {
|
||||
t.Fatalf("bad: number of accessors. Expected: 1, Actual: %d", numberOfAccessors)
|
||||
}
|
||||
|
||||
for i := 1; i <= 100; i++ {
|
||||
// Create a token
|
||||
tokenReq := &logical.Request{
|
||||
Operation: logical.UpdateOperation,
|
||||
Path: "create",
|
||||
ClientToken: root,
|
||||
Data: map[string]interface{}{
|
||||
"policies": []string{"policy1"},
|
||||
},
|
||||
}
|
||||
resp, err = ts.HandleRequest(context.Background(), tokenReq)
|
||||
if err != nil || (resp != nil && resp.IsError()) {
|
||||
t.Fatalf("err:%v resp:%v", err, resp)
|
||||
}
|
||||
tut := resp.Auth.ClientToken
|
||||
|
||||
// Create a child token
|
||||
tokenReq = &logical.Request{
|
||||
Operation: logical.UpdateOperation,
|
||||
Path: "create",
|
||||
ClientToken: tut,
|
||||
Data: map[string]interface{}{
|
||||
"policies": []string{"policy1"},
|
||||
},
|
||||
}
|
||||
resp, err = ts.HandleRequest(context.Background(), tokenReq)
|
||||
if err != nil || (resp != nil && resp.IsError()) {
|
||||
t.Fatalf("err:%v resp:%v", err, resp)
|
||||
}
|
||||
|
||||
// Creation of another token should end up with incrementing the number of
|
||||
// accessors the storage
|
||||
resp, err = ts.HandleRequest(context.Background(), accessorListReq)
|
||||
if err != nil || (resp != nil && resp.IsError()) {
|
||||
t.Fatalf("err:%v resp:%v", err, resp)
|
||||
}
|
||||
|
||||
numberOfAccessors = len(resp.Data["keys"].([]string))
|
||||
if numberOfAccessors != (i*2)+1 {
|
||||
t.Fatalf("bad: number of accessors. Expected: %d, Actual: %d", i+1, numberOfAccessors)
|
||||
}
|
||||
|
||||
// Revoke the token while leaking other items associated with the
|
||||
// token. Do this by doing what revokeSalted used to do before it was
|
||||
// fixed, i.e., by deleting the storage entry for token and its
|
||||
// cubbyhole and by not deleting its secondary index, its accessor and
|
||||
// associated leases.
|
||||
|
||||
saltedTut, err := ts.SaltID(context.Background(), tut)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
_, err = ts.lookupSalted(context.Background(), saltedTut, true)
|
||||
if err != nil {
|
||||
t.Fatalf("failed to lookup token: %v", err)
|
||||
}
|
||||
|
||||
// Destroy the token index
|
||||
path := lookupPrefix + saltedTut
|
||||
if ts.view.Delete(context.Background(), path); err != nil {
|
||||
t.Fatalf("failed to delete token entry: %v", err)
|
||||
}
|
||||
|
||||
// Destroy the cubby space
|
||||
err = ts.destroyCubbyhole(context.Background(), saltedTut)
|
||||
if err != nil {
|
||||
t.Fatalf("failed to destroyCubbyhole: %v", err)
|
||||
}
|
||||
|
||||
// Leaking of accessor should have resulted in no change to the number
|
||||
// of accessors
|
||||
resp, err = ts.HandleRequest(context.Background(), accessorListReq)
|
||||
if err != nil || (resp != nil && resp.IsError()) {
|
||||
t.Fatalf("err:%v resp:%v", err, resp)
|
||||
}
|
||||
|
||||
numberOfAccessors = len(resp.Data["keys"].([]string))
|
||||
if numberOfAccessors != (i*2)+1 {
|
||||
t.Fatalf("bad: number of accessors. Expected: %d, Actual: %d", (i*2)+1, numberOfAccessors)
|
||||
}
|
||||
}
|
||||
|
||||
tidyReq := &logical.Request{
|
||||
Path: "tidy",
|
||||
Operation: logical.UpdateOperation,
|
||||
ClientToken: root,
|
||||
}
|
||||
resp, err = ts.HandleRequest(context.Background(), tidyReq)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if resp != nil && resp.IsError() {
|
||||
t.Fatalf("resp: %#v", resp)
|
||||
}
|
||||
if err != nil || (resp != nil && resp.IsError()) {
|
||||
t.Fatalf("err:%v resp:%v", err, resp)
|
||||
}
|
||||
|
||||
// Tidy should have removed all the dangling accessor entries
|
||||
resp, err = ts.HandleRequest(context.Background(), accessorListReq)
|
||||
if err != nil || (resp != nil && resp.IsError()) {
|
||||
t.Fatalf("err:%v resp:%v", err, resp)
|
||||
}
|
||||
|
||||
// The number of accessors should be equal to number of valid child tokens
|
||||
// (100) + the root token (1)
|
||||
keys := resp.Data["keys"].([]string)
|
||||
numberOfAccessors = len(keys)
|
||||
if numberOfAccessors != 101 {
|
||||
t.Fatalf("bad: number of accessors. Expected: 1, Actual: %d", numberOfAccessors)
|
||||
}
|
||||
|
||||
req := logical.TestRequest(t, logical.UpdateOperation, "lookup-accessor")
|
||||
|
||||
for _, accessor := range keys {
|
||||
req.Data = map[string]interface{}{
|
||||
"accessor": accessor,
|
||||
}
|
||||
|
||||
resp, err := ts.HandleRequest(context.Background(), req)
|
||||
if err != nil {
|
||||
t.Fatalf("err: %s", err)
|
||||
}
|
||||
if resp.Data == nil {
|
||||
t.Fatalf("response should contain data")
|
||||
}
|
||||
// These tokens should now be orphaned
|
||||
if resp.Data["orphan"] != true {
|
||||
t.Fatalf("token should be orphan")
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestTokenStore_TidyLeaseRevocation(t *testing.T) {
|
||||
exp := mockExpiration(t)
|
||||
ts := exp.tokenStore
|
||||
|
|
|
|||
Loading…
Reference in New Issue