From 8d9295c539801653a0149bfa6f2e143ba886dfba Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Tue, 27 Mar 2018 11:12:06 -0400 Subject: [PATCH] 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 | 80 ++++++++++++++++- vault/token_store_test.go | 177 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 254 insertions(+), 3 deletions(-) diff --git a/vault/token_store.go b/vault/token_store.go index 66ea8bea1..14fcdc53f 100644 --- a/vault/token_store.go +++ b/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) diff --git a/vault/token_store_test.go b/vault/token_store_test.go index f39a0ed7f..dee26268f 100644 --- a/vault/token_store_test.go +++ b/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