Token revocation refactor #4512
Conversation
vault/expiration.go
Outdated
| return "", consts.ErrPathContainsParentReferences | ||
| } | ||
|
|
||
| saltedID, err := m.tokenStore.SaltID(m.quitContext, auth.ClientToken) |
There was a problem hiding this comment.
You already have this value from earlier in the function.
| // We defer a revocation until after logic has run, since this is a | ||
| // valid request (this is the token's final use). We pass the ID in | ||
| // directly just to be safe in case something else modifies te later. | ||
| defer func(id string) { | ||
| err = c.tokenStore.Revoke(ctx, id) | ||
| leaseID, err := c.expiration.CreateOrFetchRevocationLeaseByToken(te) |
There was a problem hiding this comment.
The two places in core (Seal/StepDown) probably need this updated logic too right? (They're still using c.tokenStore.Revoke)
There was a problem hiding this comment.
I wonder if doing an async Revoke (i.e. via the expiration manager) is the right call in seal/step down. My concern is that it can be racy if core seals/give up the lock before the timer in expiration manager triggers to do the revocation via expireID. I assume there might be other storage operations during the process so if the storage call to delete blocks (such that the delete is delayed) we have bigger issues, but I'm not sure if we should be concerned that core could seal/give up the write lock before the token store gets updated.
There was a problem hiding this comment.
Actually nevermind, I got things mixed up. Revoke() in the expiration manager deletes the lease entry directly as well as the timer in the pending map so it shouldn't be a concern.
There was a problem hiding this comment.
Responding here for future reference. This should be addressed now that revokeSalted will mark the token with tokenRevocationPending early on so that it's invalidated for any lookup/use prior to async deletion.
vault/token_store.go
Outdated
| // failed (state is false) | ||
| state, loaded := ts.tokensPendingDeletion.LoadOrStore(saltedID, true) | ||
|
|
||
| // If the entry was loaded and it's state is true, we shortcircuit |
| // before we return, we can remove the token store entry | ||
| if ret == nil { | ||
| path := lookupPrefix + saltedID | ||
| if err := ts.view.Delete(ctx, path); err != nil { |
There was a problem hiding this comment.
Having this in the defer seems unnecessary, i think it makes more sense as the last thing in the function.
There was a problem hiding this comment.
Yea it makes no difference having it vs it being called last. I moved it here to guard against the possibility of accidentally adding token entry updates after the deletion if we appended code to the end of the function which would re-introduce the deleted token. We would still need the defer func even if we had the deletion outside of this in order to properly update ts.tokensPendingDeletion.
* Rename some functions and variables to be more clear * Change step-down and seal to use expmgr for revoke functionality like during request handling * Attempt to WAL the token as being invalid as soon as possible so that further usage will fail even if revocation does not fully complete
| return err | ||
| } | ||
|
|
||
| // If there's a lease, set expiration to now, persist, and call |
There was a problem hiding this comment.
Can we update this comment as this is already in expiration manager?
| func (m *ExpirationManager) lookupByToken(token string) ([]string, error) { | ||
| // CreateOrFetchRevocationLeaseByToken is used to create or fetch the matching | ||
| // leaseID for a particular token. The lease is set to expire immediately after | ||
| // it's created. |
There was a problem hiding this comment.
Why do we need to create a lease?
There was a problem hiding this comment.
We need to created a lease in case the token was created, but never added to the expiration manager (i.e. never auth'ed with) which would mean that there's no lease entry for it.
vault/expiration.go
Outdated
| // Create a lease entry | ||
| now := time.Now() | ||
| le = &leaseEntry{ | ||
| LeaseID: path.Join(te.Path, saltedID), |
There was a problem hiding this comment.
leaseID is also computed earlier in the function.
| } | ||
|
|
||
| // Encode the entry | ||
| if err := m.persistEntry(le); err != nil { |
There was a problem hiding this comment.
Don't we need to call updatePending after this?
There was a problem hiding this comment.
We don't need to call updatePending since this method should only be in charge of to creating or fetching us the corresponding lease to a token. This method is then used in conjunction with m.Revoke to remove the token and its associated entries.
| lock.Unlock() | ||
| if err != nil { | ||
| return err | ||
| if entry.NumUses != tokenRevocationPending { |
There was a problem hiding this comment.
It'd make sense to keep this if for the case that the token is already in this state so that we don't have to re-store it with the same value.
| // Revoke the token and its children | ||
| if err := ts.RevokeTree(ctx, aEntry.TokenID); err != nil { | ||
| return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest | ||
| te, err := ts.Lookup(ctx, aEntry.TokenID) |
There was a problem hiding this comment.
Lookup, CreateOr.. and Revoke. This pattern is repeated a couple of times. Can we abstract it out?
Fixes #4143