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

Detect and bypass cycles during token revocation #5364

Merged
merged 2 commits into from
Sep 20, 2018
Merged

Detect and bypass cycles during token revocation #5364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9016c68
Detect and bypass cycles during token revocation
Sep 17, 2018
410fd9d
Add to seen cache when visited, not when found
Sep 20, 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
31 changes: 23 additions & 8 deletions vault/token_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,8 +1404,8 @@ func (ts *TokenStore) revokeTree(ctx context.Context, le *leaseEntry) error {
// Updated to be non-recursive and revoke child tokens
// before parent tokens(DFS).
func (ts *TokenStore) revokeTreeInternal(ctx context.Context, id string) error {
var dfs []string
dfs = append(dfs, id)
dfs := []string{id}
seenIDs := make(map[string]struct{})

var ns *namespace.Namespace

Expand All @@ -1429,7 +1429,8 @@ func (ts *TokenStore) revokeTreeInternal(ctx context.Context, id string) error {
}

for l := len(dfs); l > 0; l = len(dfs) {
id := dfs[0]
id := dfs[len(dfs)-1]
seenIDs[id] = struct{}{}

saltedCtx := ctx
saltedNS := ns
Expand All @@ -1444,11 +1445,26 @@ func (ts *TokenStore) revokeTreeInternal(ctx context.Context, id string) error {
}

path := saltedID + "/"
children, err := ts.parentView(saltedNS).List(saltedCtx, path)
childrenRaw, err := ts.parentView(saltedNS).List(saltedCtx, path)
if err != nil {
return errwrap.Wrapf("failed to scan for children: {{err}}", err)
}

// Filter the child list to remove any items that have ever been in the dfs stack.
// This is a robustness check, as a parent/child cycle can lead to an OOM crash.
children := make([]string, 0, len(childrenRaw))
for _, child := range childrenRaw {
if _, seen := seenIDs[child]; !seen {
children = append(children, child)
} else {
if err = ts.parentView(saltedNS).Delete(saltedCtx, path+child); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ts.revokeInternal down below should be making this exact Delete call on the child token, so I wonder if the cycle is is due to the token not being properly cleaned up in there. IMO revokeTreeInternal should be only in charge of tree traversal and delegate deletion to revokeInternal to ensure that all related entries are properly removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ts.revokeInternal does make this delete for the parent reference in the token, but if we're here, there are extraneous parent references that will never get deleted. e.g. assume the state is:

parent/a/c
parent/b/c

If c's correct parent is a, that info is stored in c and will cause parent/a/c to be deleted in revokeInternal. But nothing would trigger deletion of parent/b/c.

A prior version of the PR didn't have the delete step. It worked fine and was resilient to cycles, but @briankassouf noted that it would leave stray parent references.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it can ever get to that state though right? c can only ever have one parent, unless entry creation was borked at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Root cause for how a cycle could be created is still unknown, but we did see it in user data (see: https://hashicorp.slack.com/archives/C04L34UGZ/p1536860851000100) and if present, the revocation will just consume memory until Vault crashes.

return errwrap.Wrapf("failed to delete entry: {{err}}", err)
}

ts.Logger().Warn("token cycle found", "token", child)
}
}

// If the length of the children array is zero,
// then we are at a leaf node.
if len(children) == 0 {
Expand All @@ -1464,11 +1480,10 @@ func (ts *TokenStore) revokeTreeInternal(ctx context.Context, id string) error {
if l == 1 {
return nil
}
dfs = dfs[1:]
dfs = dfs[:len(dfs)-1]
} else {
// If we make it here, there are children and they must
// be prepended.
dfs = append(children, dfs...)
// If we make it here, there are children and they must be appended.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to change from dfs to bfs, which was specifically what we got away from due to problems customers had with very large token trees. Is there a reason for this change?

Copy link
Contributor Author

@kalafut kalafut Sep 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a functional change and is still dfs. The previous version had all operations working at the front of the slice. i.e. pulling from the head (0th element) and prepending the children to the slice. This pattern causes a new allocation and full copy of the dfs slice on every append. I've simply flipped the access pattern to use the end of the slice instead. So the next element is pulled off the end (see 1434), and deletions and child appends are made to the end. This way the underlying dfs array is only reallocated when it grows to a new max capacity.

Before the change I did a micro benchmark of prepending vs. appending and it was a big (260x) difference in speed. Admittedly the macro operation here is likely gated by I/O, but I felt that the reduced GC churn made it worthwhile.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I see what you did now, looks fine.

dfs = append(dfs, children...)
}
}

Expand Down
49 changes: 44 additions & 5 deletions vault/token_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1004,17 +1004,46 @@ func TestTokenStore_Revoke_Orphan(t *testing.T) {
// This was the original function name, and now it just calls
// the non recursive version for a variety of depths.
func TestTokenStore_RevokeTree(t *testing.T) {
testTokenStore_RevokeTree_NonRecursive(t, 1)
testTokenStore_RevokeTree_NonRecursive(t, 2)
testTokenStore_RevokeTree_NonRecursive(t, 10)
testTokenStore_RevokeTree_NonRecursive(t, 1, false)
testTokenStore_RevokeTree_NonRecursive(t, 2, false)
testTokenStore_RevokeTree_NonRecursive(t, 10, false)

// corrupted trees with cycles
testTokenStore_RevokeTree_NonRecursive(t, 1, true)
testTokenStore_RevokeTree_NonRecursive(t, 10, true)
}

// Revokes a given Token Store tree non recursively.
// The second parameter refers to the depth of the tree.
func testTokenStore_RevokeTree_NonRecursive(t testing.TB, depth uint64) {
func testTokenStore_RevokeTree_NonRecursive(t testing.TB, depth uint64, injectCycles bool) {
c, _, _ := TestCoreUnsealed(t)
ts := c.tokenStore
root, children := buildTokenTree(t, ts, depth)

var cyclePaths []string
if injectCycles {
// Make the root the parent of itself
saltedRoot, _ := ts.SaltID(namespace.TestContext(), root.ID)
key := fmt.Sprintf("%s/%s", saltedRoot, saltedRoot)
cyclePaths = append(cyclePaths, key)
le := &logical.StorageEntry{Key: key}

if err := ts.parentView(namespace.TestNamespace()).Put(namespace.TestContext(), le); err != nil {
t.Fatalf("err: %v", err)
}

// Make a deep child the parent of a shallow child
shallow, _ := ts.SaltID(namespace.TestContext(), children[0].ID)
deep, _ := ts.SaltID(namespace.TestContext(), children[len(children)-1].ID)
key = fmt.Sprintf("%s/%s", deep, shallow)
cyclePaths = append(cyclePaths, key)
le = &logical.StorageEntry{Key: key}

if err := ts.parentView(namespace.TestNamespace()).Put(namespace.TestContext(), le); err != nil {
t.Fatalf("err: %v", err)
}
}

err := ts.revokeTree(namespace.TestContext(), &leaseEntry{})
if err.Error() != "cannot tree-revoke blank token" {
t.Fatal(err)
Expand Down Expand Up @@ -1049,6 +1078,16 @@ func testTokenStore_RevokeTree_NonRecursive(t testing.TB, depth uint64) {
t.Fatalf("bad: %#v", out)
}
}

for _, path := range cyclePaths {
entry, err := ts.parentView(namespace.TestNamespace()).Get(namespace.TestContext(), path)
if err != nil {
t.Fatalf("err: %v", err)
}
if entry != nil {
t.Fatalf("expected reference to be deleted: %v", entry)
}
}
}

// A benchmark function that tests testTokenStore_RevokeTree_NonRecursive
Expand All @@ -1058,7 +1097,7 @@ func BenchmarkTokenStore_RevokeTree(b *testing.B) {
for _, depth := range benchmarks {
b.Run(fmt.Sprintf("Tree of Depth %d", depth), func(b *testing.B) {
for i := 0; i < b.N; i++ {
testTokenStore_RevokeTree_NonRecursive(b, depth)
testTokenStore_RevokeTree_NonRecursive(b, depth, false)
}
})
}
Expand Down