From 10e77c1569a3685eb9e3a368d4bcfc9cd03459ce Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 27 Apr 2018 18:00:11 -0400 Subject: [PATCH 1/3] Use bool instead of storage call to update NumUses --- vault/token_store.go | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/vault/token_store.go b/vault/token_store.go index ab85283791ce..3ade82896788 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -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. @@ -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) - 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) } @@ -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 } From 251848cbda9414db83e05b04ff6a5a6d64d703ec Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 27 Apr 2018 20:43:07 -0400 Subject: [PATCH 2/3] Move token deletion inside defer func --- vault/token_store.go | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/vault/token_store.go b/vault/token_store.go index 3ade82896788..4d6fb289220d 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -1078,7 +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 + deleteEntry := 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. @@ -1128,17 +1128,24 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret er // failed revocation. This way we don't try to clean up during active // revocation (-2). defer func() { - if ret != nil { - lock.Lock() - defer lock.Unlock() - - // 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 !entryDeleted { - entry.NumUses = tokenRevocationFailed - ts.storeCommon(ctx, entry, false) + lock.Lock() + defer lock.Unlock() + + if deleteEntry { + path := lookupPrefix + saltedID + if err := ts.view.Delete(ctx, path); err != nil { + ret = err + } else { + // If the delete was sucessful, then we short-circuit and don't mark the + // revocation as failed. + return } } + + if ret != nil { + entry.NumUses = tokenRevocationFailed + ts.storeCommon(ctx, entry, false) + } }() // Destroy the token's cubby. This should go first as it's a @@ -1213,13 +1220,8 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret er return errwrap.Wrapf("failed to delete entry: {{err}}", 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 { - return errwrap.Wrapf("failed to delete entry: {{err}}", err) - } - // Mark the deletion as successful for the defer function - entryDeleted = true + // Mark the entry for deletion in the defer function + deleteEntry = true return nil } From f0fbd47b82d4ff0baccbc7f561f16c0f8c4ba53d Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Mon, 30 Apr 2018 11:38:53 -0400 Subject: [PATCH 3/3] Conditional lock --- vault/token_store.go | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/vault/token_store.go b/vault/token_store.go index 4d6fb289220d..400e932b1f3d 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -1124,16 +1124,17 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret er return err } - // If we are returning an error, mark the entry with -3 to indicate - // failed revocation. This way we don't try to clean up during active - // revocation (-2). defer func() { - lock.Lock() - defer lock.Unlock() + if deleteEntry || ret != nil { + lock.Lock() + defer lock.Unlock() + } if deleteEntry { path := lookupPrefix + saltedID if err := ts.view.Delete(ctx, path); err != nil { + // We set ret like so since ret can be from other errors and we don't + // want to override with nil ret = err } else { // If the delete was sucessful, then we short-circuit and don't mark the @@ -1142,6 +1143,9 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret er } } + // If we are returning an error, mark the entry with -3 to indicate + // failed revocation. This way we don't try to clean up during active + // revocation (-2). if ret != nil { entry.NumUses = tokenRevocationFailed ts.storeCommon(ctx, entry, false)