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

Use bool instead of storage call to update NumUses #4479

Closed
wants to merge 3 commits into from
Closed

Use bool instead of storage call to update NumUses #4479

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
10e77c1
Use bool instead of storage call to update NumUses
calvn Apr 27, 2018
251848c
Move token deletion inside defer func
calvn Apr 28, 2018
f0fbd47
Conditional lock
calvn Apr 30, 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
12 changes: 4 additions & 8 deletions vault/token_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1078,6 +1078,7 @@ func (ts *TokenStore) Revoke(ctx context.Context, id string) error {
// revokeSalted is used to invalidate a given salted token,
// any child tokens will be orphaned.
func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret error) {
entryDeleted := false
// Protect the entry lookup/writing with locks. The rub here is that we
// don't know the ID until we look it up once, so first we look it up, then
// do a locked lookup.
Expand Down Expand Up @@ -1131,16 +1132,9 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret er
lock.Lock()
defer lock.Unlock()

// Lookup the token again to make sure something else didn't
// revoke in the interim
entry, err := ts.lookupSalted(ctx, saltedID, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe removing this call introduces a race condition. If another thread removes the token before we grab the write lock above we will restore the deleted entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah good point.

Copy link
Member

Choose a reason for hiding this comment

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

I've been thinking about this, and I think removing this is okay. Marking useCount as -2 is in a protected critical path, so the only way we should be here is if the token was set to -2 by this goroutine.

Copy link
Member

Choose a reason for hiding this comment

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

Although this actually begs another question: do we even need to grab the lock here, then? Lookup won't return the token if use count is negative, so locking when the Delete happens shouldn't matter. Nothing else should be operating on it because at that point useCount is -2 so any other revocation will skip being run. If the Delete fails, we update the entry with -3, but the timing of that also doesn't matter -- anything that tries to revoke before that won't work (it'll hit the -2), and anything that tries after will keep going, but this goroutine will have exited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to still grab the lock since the entry.NumUses == tokenRevocationInProgress check happens before the write. There could theoretically be a case where 2 goroutines calling revokeSalted have checked that entry.NumUses is not -2 before either one got to update it, and so the revokeSalted operation would go through twice.

	// On failure we write -3, so if we hit -2 here we're already running a
	// revocation operation. This can happen due to e.g. recursion into this
	// function via the expiration manager's RevokeByToken.

I wonder if we need tokenRevocationInProgress since RevokeByToken does not remove the actual token due to more recent changes (m.revokeCommon(tokenLeaseID, false, true) in here) so the recursion in the comment above might no longer be an issue.

if err != nil {
return
}

// If it exists just taint to -3 rather than trying to figure
// out what it means if it's already -3 after the -2 above
if entry != nil {
if !entryDeleted {
entry.NumUses = tokenRevocationFailed
ts.storeCommon(ctx, entry, false)
}
Expand Down Expand Up @@ -1224,6 +1218,8 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret er
if err = ts.view.Delete(ctx, path); err != nil {
return errwrap.Wrapf("failed to delete entry: {{err}}", err)
}
// Mark the deletion as successful for the defer function
entryDeleted = true

return nil
}
Expand Down