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

vault/token_store.go

@@ -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)

vault/token_store_test.go

@@ -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