Detect and bypass cycles during token revocation #5335
Conversation
vault/token_store.go
Outdated
| if _, seen := seenIDs[child]; !seen { | ||
| children = append(children, child) | ||
| } else { | ||
| ts.Logger().Warn("token cycle found", "token", child) |
There was a problem hiding this comment.
It might be best to remove the key that is causing the cycle here, otherwise we will lose all reference to this key and it will live forever
There was a problem hiding this comment.
If we delete the key here, the depth-first order is bypassed, which could break references if the whole operation doesn't complete. e.g. if ID root showed up as a child and we deleted it, revoke fails for some reason, now there are possibly tokens with root as an ancestor but root is gone.
But as to whether everything gets cleaned up the way it's written, I think it does:
- If the ID is showing up as as "seen", then it was definitely added to the dfs queue (line 1248).
- Eventually it was or will be at the front of the queue, with no more of its own children to delete, and it will be deleted.
I'm happy to add tests of any corner cases for this.
There was a problem hiding this comment.
I think it's worth testing out, I could see a situation where parentPrefix + id + "/" + child is never cleaned up if the child token has the parent encoded incorrectly. Which if we are in this situation is likely.
There was a problem hiding this comment.
Also i wasn't proposing deleting the whole token here, just the extra parent reference that is causing it to be seen more than once. The previously seen version of the token will still allow for it to be cleaned up.
There was a problem hiding this comment.
Yep, adding a delete of the parent reference here is a good improvement.
Fixes #4803