From 5474ec01038d45239dda6391240bdfc6e23151c3 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Tue, 1 May 2018 17:07:43 -0400 Subject: [PATCH 01/16] Hand off lease expiration to expiration manager via timers --- vault/expiration.go | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index 492396c9b24b..d26c164c0d5d 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -567,12 +567,27 @@ func (m *ExpirationManager) RevokeByToken(te *TokenEntry) error { } // Revoke all the keys - for idx, leaseID := range existing { - if err := m.revokeCommon(leaseID, false, false); err != nil { - return errwrap.Wrapf(fmt.Sprintf("failed to revoke %q (%d / %d): {{err}}", leaseID, idx+1, len(existing)), err) + for _, leaseID := range existing { + // Load the entry + le, err := m.loadEntry(leaseID) + if err != nil { + return err + } + + // If there's a lease, set expiration to now, persist, and call + // updatePending to hand off revocation to the expiration manager + if le != nil { + le.ExpireTime = time.Now() + + if err := m.persistEntry(le); err != nil { + return err + } + + m.updatePending(le, 0) } } + // te.Path should never be empty, but we check just in case if te.Path != "" { saltedID, err := m.tokenStore.SaltID(m.quitContext, te.ID) if err != nil { From d9edff16cd1275e21e79ad5959fdfd047a9790f5 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Wed, 2 May 2018 10:58:51 -0400 Subject: [PATCH 02/16] Use sync.Map as the cache to track token deletion state --- vault/token_store.go | 81 +++++++++++++-------------------------- vault/token_store_test.go | 2 + 2 files changed, 29 insertions(+), 54 deletions(-) diff --git a/vault/token_store.go b/vault/token_store.go index ab85283791ce..408c0a75b876 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -98,6 +98,12 @@ type TokenStore struct { tokenLocks []*locksutil.LockEntry + // tokenPendingDeletion stores tokens that are being revoked. If the token is + // not in the map, it means that there's no deletion in progress. If the value + // is true it means deletion is in progress, and if false it means deletion + // failed. Revocation needs to handle these states accordingly. + tokensPendingDeletion *sync.Map + cubbyholeDestroyer func(context.Context, *TokenStore, string) error logger log.Logger @@ -122,6 +128,7 @@ func NewTokenStore(ctx context.Context, logger log.Logger, c *Core, config *logi cubbyholeDestroyer: destroyCubbyhole, logger: logger, tokenLocks: locksutil.CreateLocks(), + tokensPendingDeletion: &sync.Map{}, saltLock: sync.RWMutex{}, identityPoliciesDeriverFunc: c.fetchEntityAndDerivedPolicies, } @@ -1078,72 +1085,33 @@ 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) { - // 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. - entry, err := ts.lookupSalted(ctx, saltedID, true) - if err != nil { - return err - } - if entry == nil { + // Check and set the token deletion state. We only proceed with the deletion + // if we don't have a pending deletion (empty), or if the deletion previously + // failed (state is false) + state, loaded := ts.tokensPendingDeletion.LoadOrStore(saltedID, true) + + // If the entry was loaded and it's state is true, we shortcircuit + if loaded && state == true { return nil } - lock := locksutil.LockForKey(ts.tokenLocks, entry.ID) - lock.Lock() - - // Lookup the token first - entry, err = ts.lookupSalted(ctx, saltedID, true) + // The map check above should protect use from any concurrent revocations, so + // doing a bare lookup here should be fine. + entry, err := ts.lookupSalted(ctx, saltedID, true) if err != nil { - lock.Unlock() return err } - if entry == nil { - lock.Unlock() return nil } - // 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. - if entry.NumUses == tokenRevocationInProgress { - lock.Unlock() - return nil - } - - // This acts as a WAL. lookupSalted will no longer return this entry, - // so the token cannot be used, but this way we can keep the entry - // around until after the rest of this function is attempted, and a - // tidy function can key off of this value to try again. - entry.NumUses = tokenRevocationInProgress - err = ts.storeCommon(ctx, entry, false) - lock.Unlock() - if err != nil { - 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() { if ret != nil { - 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 { - entry.NumUses = tokenRevocationFailed - ts.storeCommon(ctx, entry, false) - } + // If we failed we store the state as false, so that the next call to + // revokeSalted will retry + ts.tokensPendingDeletion.Store(saltedID, false) + } else { + ts.tokensPendingDeletion.Delete(saltedID) } }() @@ -1265,6 +1233,11 @@ func (ts *TokenStore) revokeTreeSalted(ctx context.Context, saltedID string) err // If the length of the children array is zero, // then we are at a leaf node. if len(children) == 0 { + // Whenever revokeSalted is called, the token will be removed immediately and + // any underlying secrets will be handed off to the expiration manager which will + // take care of expiring them. If Vault is restarted, any revoked tokens + // would have been deleted, and any pending leases for deletion will be restored + // by the expiration manager. if err := ts.revokeSalted(ctx, id); err != nil { return errwrap.Wrapf("failed to revoke entry: {{err}}", err) } diff --git a/vault/token_store_test.go b/vault/token_store_test.go index 3e6cf74c9f44..acdd37ad1008 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -3837,6 +3837,8 @@ func TestTokenStore_TidyLeaseRevocation(t *testing.T) { // Call tidy ts.handleTidy(context.Background(), nil, nil) + time.Sleep(1 * time.Second) + // Verify leases are gone storedLeases, err = exp.lookupByToken(tut) if err != nil { From 44984266f6c86b5c48e9ad1704171e6dc0861794 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Thu, 3 May 2018 21:44:06 -0400 Subject: [PATCH 03/16] Add CreateOrFetchRevocationLeaseByToken to hand off token revocation to exp manager --- vault/core.go | 4 +-- vault/expiration.go | 60 +++++++++++++++++++++++++++++++++++++-- vault/request_handling.go | 7 +++-- vault/token_store.go | 27 ++++++++---------- 4 files changed, 76 insertions(+), 22 deletions(-) diff --git a/vault/core.go b/vault/core.go index a6da77dcd2e5..27851aee32eb 100644 --- a/vault/core.go +++ b/vault/core.go @@ -1429,7 +1429,7 @@ func (c *Core) sealInitCommon(ctx context.Context, req *logical.Request) (retErr return retErr } - if te != nil && te.NumUses == -1 { + if te != nil && te.NumUses == tokenRevocationDeferred { // Token needs to be revoked. We do this immediately here because // we won't have a token store after sealing. err = c.tokenStore.Revoke(c.activeContext, te.ID) @@ -1540,7 +1540,7 @@ func (c *Core) StepDown(req *logical.Request) (retErr error) { return retErr } - if te != nil && te.NumUses == -1 { + if te != nil && te.NumUses == tokenRevocationDeferred { // Token needs to be revoked. We do this immediately here because // we won't have a token store after sealing. err = c.tokenStore.Revoke(c.activeContext, te.ID) diff --git a/vault/expiration.go b/vault/expiration.go index d26c164c0d5d..b5a7ff5a86bb 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -561,7 +561,7 @@ func (m *ExpirationManager) RevokeByToken(te *TokenEntry) error { defer metrics.MeasureSince([]string{"expire", "revoke-by-token"}, time.Now()) // Lookup the leases - existing, err := m.lookupByToken(te.ID) + existing, err := m.lookupLeasesByToken(te.ID) if err != nil { return errwrap.Wrapf("failed to scan for leases: {{err}}", err) } @@ -1262,8 +1262,62 @@ func (m *ExpirationManager) removeIndexByToken(token, leaseID string) error { return nil } -// lookupByToken is used to lookup all the leaseID's via the -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, or updated to immediately expire after it's fetched +func (m *ExpirationManager) CreateOrFetchRevocationLeaseByToken(te *TokenEntry) (string, error) { + // Fetch the saltedID of the token and construct the leaseID + saltedID, err := m.tokenStore.SaltID(m.quitContext, te.ID) + if err != nil { + return "", err + } + leaseID := path.Join(te.Path, saltedID) + + // Load the entry + le, err := m.loadEntry(leaseID) + if err != nil { + return "", err + } + + // If there's no associated leaseEntry for the token, we create one + if le == nil { + auth := &logical.Auth{ + ClientToken: te.ID, + LeaseOptions: logical.LeaseOptions{ + TTL: time.Nanosecond, + }, + } + + if strings.Contains(te.Path, "..") { + return "", consts.ErrPathContainsParentReferences + } + + saltedID, err := m.tokenStore.SaltID(m.quitContext, auth.ClientToken) + if err != nil { + return "", err + } + + // Create a lease entry + now := time.Now() + le = &leaseEntry{ + LeaseID: path.Join(te.Path, saltedID), + ClientToken: auth.ClientToken, + Auth: auth, + Path: te.Path, + IssueTime: now, + ExpireTime: now.Add(time.Nanosecond), + } + + // Encode the entry + if err := m.persistEntry(le); err != nil { + return "", err + } + } + + return le.LeaseID, nil +} + +// lookupLeasesByToken is used to lookup all the leaseID's via the tokenID +func (m *ExpirationManager) lookupLeasesByToken(token string) ([]string, error) { saltedID, err := m.tokenStore.SaltID(m.quitContext, token) if err != nil { return nil, err diff --git a/vault/request_handling.go b/vault/request_handling.go index 0536c8020d60..e2554429addc 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -178,12 +178,15 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp retErr = multierror.Append(retErr, logical.ErrPermissionDenied) return nil, nil, retErr } - if te.NumUses == -1 { + if te.NumUses == tokenRevocationDeferred { // 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) + if err == nil { + err = c.expiration.Revoke(leaseID) + } if err != nil { c.logger.Error("failed to revoke token", "error", err) retResp = nil diff --git a/vault/token_store.go b/vault/token_store.go index 408c0a75b876..1ec2f794104e 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -54,16 +54,6 @@ const ( // tokenRevocationDeferred indicates that the token should not be used // again but is currently fulfilling its final use tokenRevocationDeferred = -1 - - // tokenRevocationInProgress indicates that revocation of that token/its - // leases is ongoing - tokenRevocationInProgress = -2 - - // tokenRevocationFailed indicates that revocation failed; the entry is - // kept around so that when the tidy function is run it can be tried - // again (or when the revocation function is run again), but all other uses - // will report the token invalid - tokenRevocationFailed = -3 ) var ( @@ -923,9 +913,9 @@ func (ts *TokenStore) UseToken(ctx context.Context, te *TokenEntry) (*TokenEntry // manager revoking children) attempting to acquire the same lock // repeatedly. if te.NumUses == 1 { - te.NumUses = -1 + te.NumUses = tokenRevocationDeferred } else { - te.NumUses -= 1 + te.NumUses-- } err = ts.storeCommon(ctx, te, false) @@ -1590,9 +1580,16 @@ func (ts *TokenStore) handleUpdateRevokeAccessor(ctx context.Context, req *logic return nil, err } - // 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) + if err != nil { + return nil, err + } + + leaseID, err := ts.expiration.CreateOrFetchRevocationLeaseByToken(te) + + err = ts.expiration.Revoke(leaseID) + if err != nil { + return nil, err } if urlaccessor { From a882e21e20801a47061bab7b83fe1b0eed73c439 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 4 May 2018 10:43:22 -0400 Subject: [PATCH 04/16] Update revoke and revoke-self handlers --- vault/expiration.go | 5 +++-- vault/token_store.go | 48 ++++++++++++++++++++++++++++++++++++++------ 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index b5a7ff5a86bb..bd8d5d165c7f 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -1262,8 +1262,9 @@ func (m *ExpirationManager) removeIndexByToken(token, leaseID string) error { return nil } -// 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, or updated to immediately expire after it's fetched +// 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. func (m *ExpirationManager) CreateOrFetchRevocationLeaseByToken(te *TokenEntry) (string, error) { // Fetch the saltedID of the token and construct the leaseID saltedID, err := m.tokenStore.SaltID(m.quitContext, te.ID) diff --git a/vault/token_store.go b/vault/token_store.go index 1ec2f794104e..dd81ec6cf9d3 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -1585,7 +1585,14 @@ func (ts *TokenStore) handleUpdateRevokeAccessor(ctx context.Context, req *logic return nil, err } + if te == nil { + return logical.ErrorResponse("token not found"), logical.ErrInvalidRequest + } + leaseID, err := ts.expiration.CreateOrFetchRevocationLeaseByToken(te) + if err != nil { + return nil, err + } err = ts.expiration.Revoke(leaseID) if err != nil { @@ -2024,10 +2031,25 @@ func (ts *TokenStore) handleCreateCommon(ctx context.Context, req *logical.Reque // in a way that revokes all child tokens. Normally, using sys/revoke/leaseID will revoke // the token and all children anyways, but that is only available when there is a lease. func (ts *TokenStore) handleRevokeSelf(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) { - // Revoke the token and its children - if err := ts.RevokeTree(ctx, req.ClientToken); err != nil { - return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest + te, err := ts.Lookup(ctx, req.ClientToken) + if err != nil { + return nil, err + } + + if te == nil { + return logical.ErrorResponse("token not found"), logical.ErrInvalidRequest } + + leaseID, err := ts.expiration.CreateOrFetchRevocationLeaseByToken(te) + if err != nil { + return nil, err + } + + err = ts.expiration.Revoke(leaseID) + if err != nil { + return nil, err + } + return nil, nil } @@ -2045,9 +2067,23 @@ func (ts *TokenStore) handleRevokeTree(ctx context.Context, req *logical.Request urltoken = true } - // Revoke the token and its children - if err := ts.RevokeTree(ctx, id); err != nil { - return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest + te, err := ts.Lookup(ctx, id) + if err != nil { + return nil, err + } + + if te == nil { + return logical.ErrorResponse("token not found"), logical.ErrInvalidRequest + } + + leaseID, err := ts.expiration.CreateOrFetchRevocationLeaseByToken(te) + if err != nil { + return nil, err + } + + err = ts.expiration.Revoke(leaseID) + if err != nil { + return nil, err } if urltoken { From 48898259279c840bfd5981100eecb4b8406cb519 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 4 May 2018 12:05:10 -0400 Subject: [PATCH 05/16] Fix tests --- vault/expiration_test.go | 4 ++++ vault/token_store_test.go | 22 +++++++++------------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 285d446dead8..71758e9e3569 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -742,6 +742,8 @@ func TestExpiration_RevokeByToken(t *testing.T) { t.Fatalf("err: %v", err) } + time.Sleep(200 * time.Millisecond) + if len(noop.Requests) != 3 { t.Fatalf("Bad: %v", noop.Requests) } @@ -1239,6 +1241,8 @@ func TestExpiration_revokeEntry_token(t *testing.T) { t.Fatalf("err: %v", err) } + time.Sleep(200 * time.Millisecond) + out, err := exp.tokenStore.Lookup(context.Background(), le.ClientToken) if err != nil { t.Fatalf("err: %v", err) diff --git a/vault/token_store_test.go b/vault/token_store_test.go index acdd37ad1008..acbd99311a5e 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -727,6 +727,8 @@ func TestTokenStore_Revoke_Leases(t *testing.T) { t.Fatalf("err: %v", err) } + time.Sleep(200 * time.Millisecond) + // Verify the lease is gone out, err := ts.expiration.loadEntry(leaseID) if err != nil { @@ -3374,16 +3376,13 @@ func TestTokenStore_RevokeUseCountToken(t *testing.T) { t.Fatalf("expected err") } - // Since revocation failed we should see the tokenRevocationFailed canary value + // Since revocation failed we should still be able to get a token te, err = ts.lookupSalted(context.Background(), saltTut, true) if err != nil { t.Fatal(err) } if te == nil { - t.Fatal("nil entry") - } - if te.NumUses != tokenRevocationFailed { - t.Fatalf("bad: %d", te.NumUses) + t.Fatal("nil token entry") } // Check the race condition situation by making the process sleep @@ -3409,10 +3408,7 @@ func TestTokenStore_RevokeUseCountToken(t *testing.T) { t.Fatal(err) } if te == nil { - t.Fatal("nil entry") - } - if te.NumUses != tokenRevocationInProgress { - t.Fatalf("bad: %d", te.NumUses) + t.Fatal("nil token entry") } // Let things catch up @@ -3789,7 +3785,7 @@ func TestTokenStore_TidyLeaseRevocation(t *testing.T) { sort.Strings(leases) - storedLeases, err := exp.lookupByToken(tut) + storedLeases, err := exp.lookupLeasesByToken(tut) if err != nil { t.Fatal(err) } @@ -3825,7 +3821,7 @@ func TestTokenStore_TidyLeaseRevocation(t *testing.T) { } // Verify leases still exist - storedLeases, err = exp.lookupByToken(tut) + storedLeases, err = exp.lookupLeasesByToken(tut) if err != nil { t.Fatal(err) } @@ -3837,10 +3833,10 @@ func TestTokenStore_TidyLeaseRevocation(t *testing.T) { // Call tidy ts.handleTidy(context.Background(), nil, nil) - time.Sleep(1 * time.Second) + time.Sleep(200 * time.Millisecond) // Verify leases are gone - storedLeases, err = exp.lookupByToken(tut) + storedLeases, err = exp.lookupLeasesByToken(tut) if err != nil { t.Fatal(err) } From 21153612942d3eba75a816d7365cbbe80576eb74 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 4 May 2018 12:32:01 -0400 Subject: [PATCH 06/16] revokeSalted: Move token entry deletion into the deferred func --- vault/token_store.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/vault/token_store.go b/vault/token_store.go index dd81ec6cf9d3..9992269e7c66 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -1096,9 +1096,19 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret er } defer func() { + // If we succeeded in all other revocation operations after this defer and + // 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 { + ret = errwrap.Wrapf("failed to delete entry: {{err}}", err) + } + } + + // Check on ret again and update the sync.Map accordingly if ret != nil { - // If we failed we store the state as false, so that the next call to - // revokeSalted will retry + // If we failed on any of the calls within, we store the state as false + // so that the next call to revokeSalted will retry ts.tokensPendingDeletion.Store(saltedID, false) } else { ts.tokensPendingDeletion.Delete(saltedID) @@ -1177,12 +1187,6 @@ 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) - } - return nil } From 5095373c9b2a95c071ec1d4fd2f1020356426fe7 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Fri, 4 May 2018 18:06:30 -0400 Subject: [PATCH 07/16] Fix test race --- vault/expiration_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 71758e9e3569..a60aea50de95 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -744,6 +744,9 @@ func TestExpiration_RevokeByToken(t *testing.T) { time.Sleep(200 * time.Millisecond) + noop.Lock() + defer noop.Unlock() + if len(noop.Requests) != 3 { t.Fatalf("Bad: %v", noop.Requests) } From 77ae4f01c917af2a3f21d1e37a8c9067713f9f8c Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 4 May 2018 19:43:13 -0400 Subject: [PATCH 08/16] Add blocking lease revocation test --- vault/expiration_test.go | 97 ++++++++++++++++++++++++++++++++++++++++ vault/router_test.go | 11 ++++- 2 files changed, 107 insertions(+), 1 deletion(-) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index a60aea50de95..129a4b71a136 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -768,6 +768,103 @@ func TestExpiration_RevokeByToken(t *testing.T) { } } +func TestExpiration_RevokeByToken_Blocking(t *testing.T) { + exp := mockExpiration(t) + noop := &NoopBackend{} + // Request handle with a timeout context that simulates blocking lease revocation. + noop.RequestHandler = func(ctx context.Context, req *logical.Request) (*logical.Response, error) { + ctx, cancel := context.WithTimeout(ctx, 3*time.Second) + defer cancel() + + select { + case <-ctx.Done(): + return noop.Response, nil + } + } + + _, barrier, _ := mockBarrier(t) + view := NewBarrierView(barrier, "logical/") + meUUID, err := uuid.GenerateUUID() + if err != nil { + t.Fatal(err) + } + err = exp.router.Mount(noop, "prod/aws/", &MountEntry{Path: "prod/aws/", Type: "noop", UUID: meUUID, Accessor: "noop-accessor"}, view) + if err != nil { + t.Fatal(err) + } + + paths := []string{ + "prod/aws/foo", + "prod/aws/sub/bar", + "prod/aws/zip", + } + for _, path := range paths { + req := &logical.Request{ + Operation: logical.ReadOperation, + Path: path, + ClientToken: "foobarbaz", + } + resp := &logical.Response{ + Secret: &logical.Secret{ + LeaseOptions: logical.LeaseOptions{ + TTL: 1 * time.Minute, + }, + }, + Data: map[string]interface{}{ + "access_key": "xyz", + "secret_key": "abcd", + }, + } + _, err := exp.Register(req, resp) + if err != nil { + t.Fatalf("err: %v", err) + } + } + + // Should nuke all the keys + te := &TokenEntry{ + ID: "foobarbaz", + } + if err := exp.RevokeByToken(te); err != nil { + t.Fatalf("err: %v", err) + } + + // Lock and check that no requests has gone through yet + noop.Lock() + if len(noop.Requests) != 0 { + t.Logf("%v", noop.Requests[0].Path) + t.Fatalf("Bad: %v", noop.Requests) + } + noop.Unlock() + + // Wait for some time, and relock + time.Sleep(3 * time.Second) + + noop.Lock() + defer noop.Unlock() + + // Now make sure that all requests have gone through + if len(noop.Requests) != 3 { + t.Fatalf("Bad: %v", noop.Requests) + } + for _, req := range noop.Requests { + if req.Operation != logical.RevokeOperation { + t.Fatalf("Bad: %v", req) + } + } + + expect := []string{ + "foo", + "sub/bar", + "zip", + } + sort.Strings(noop.Paths) + sort.Strings(expect) + if !reflect.DeepEqual(noop.Paths, expect) { + t.Fatalf("bad: %v", noop.Paths) + } +} + func TestExpiration_RenewToken(t *testing.T) { exp := mockExpiration(t) root, err := exp.tokenStore.rootToken(context.Background()) diff --git a/vault/router_test.go b/vault/router_test.go index b5957ad819e7..462e238665ee 100644 --- a/vault/router_test.go +++ b/vault/router_test.go @@ -14,6 +14,8 @@ import ( "github.com/hashicorp/vault/logical" ) +type HandlerFunc func(context.Context, *logical.Request) (*logical.Response, error) + type NoopBackend struct { sync.Mutex @@ -22,6 +24,7 @@ type NoopBackend struct { Paths []string Requests []*logical.Request Response *logical.Response + RequestHandler HandlerFunc Invalidations []string DefaultLeaseTTL time.Duration MaxLeaseTTL time.Duration @@ -31,6 +34,12 @@ func (n *NoopBackend) HandleRequest(ctx context.Context, req *logical.Request) ( n.Lock() defer n.Unlock() + var err error + resp := n.Response + if n.RequestHandler != nil { + resp, err = n.RequestHandler(ctx, req) + } + requestCopy := *req n.Paths = append(n.Paths, req.Path) n.Requests = append(n.Requests, &requestCopy) @@ -38,7 +47,7 @@ func (n *NoopBackend) HandleRequest(ctx context.Context, req *logical.Request) ( return nil, fmt.Errorf("missing view") } - return n.Response, nil + return resp, err } func (n *NoopBackend) HandleExistenceCheck(ctx context.Context, req *logical.Request) (bool, bool, error) { From 87dfdb3700f9e049335bdf193b4678051a86ab1c Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 4 May 2018 19:43:52 -0400 Subject: [PATCH 09/16] Remove test log --- vault/expiration_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 129a4b71a136..6d2c497496df 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -832,7 +832,6 @@ func TestExpiration_RevokeByToken_Blocking(t *testing.T) { // Lock and check that no requests has gone through yet noop.Lock() if len(noop.Requests) != 0 { - t.Logf("%v", noop.Requests[0].Path) t.Fatalf("Bad: %v", noop.Requests) } noop.Unlock() From 80247263936ecb9988c5fc3bd77f6117f05effd3 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Sat, 5 May 2018 07:35:23 -0700 Subject: [PATCH 10/16] Add HandlerFunc on NoopBackend, adjust locks, and add test --- vault/expiration_test.go | 7 ++++--- vault/router_test.go | 6 +++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 6d2c497496df..0d524d236b62 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -773,7 +773,7 @@ func TestExpiration_RevokeByToken_Blocking(t *testing.T) { noop := &NoopBackend{} // Request handle with a timeout context that simulates blocking lease revocation. noop.RequestHandler = func(ctx context.Context, req *logical.Request) (*logical.Response, error) { - ctx, cancel := context.WithTimeout(ctx, 3*time.Second) + ctx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) defer cancel() select { @@ -836,8 +836,9 @@ func TestExpiration_RevokeByToken_Blocking(t *testing.T) { } noop.Unlock() - // Wait for some time, and relock - time.Sleep(3 * time.Second) + // Wait for a bit for timeouts to trigger and pending revocations to go + // through and then we relock + time.Sleep(200 * time.Millisecond) noop.Lock() defer noop.Unlock() diff --git a/vault/router_test.go b/vault/router_test.go index 462e238665ee..929ece733982 100644 --- a/vault/router_test.go +++ b/vault/router_test.go @@ -31,15 +31,15 @@ type NoopBackend struct { } func (n *NoopBackend) HandleRequest(ctx context.Context, req *logical.Request) (*logical.Response, error) { - n.Lock() - defer n.Unlock() - var err error resp := n.Response if n.RequestHandler != nil { resp, err = n.RequestHandler(ctx, req) } + n.Lock() + defer n.Unlock() + requestCopy := *req n.Paths = append(n.Paths, req.Path) n.Requests = append(n.Requests, &requestCopy) From 5a1441d094892d464b04362725c192c929a15553 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Mon, 7 May 2018 10:41:59 -0400 Subject: [PATCH 11/16] Add sleep to allow for revocations to settle --- vault/token_store_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/vault/token_store_test.go b/vault/token_store_test.go index acbd99311a5e..cbae3e70e167 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -402,6 +402,8 @@ func TestTokenStore_HandleRequest_RevokeAccessor(t *testing.T) { t.Fatalf("err: %s", err) } + time.Sleep(200 * time.Millisecond) + out, err = ts.Lookup(context.Background(), "tokenid") if err != nil { t.Fatalf("err: %s", err) @@ -886,6 +888,8 @@ func TestTokenStore_RevokeSelf(t *testing.T) { t.Fatalf("err: %v %v", err, resp) } + time.Sleep(200 * time.Millisecond) + lookup := []string{ent1.ID, ent2.ID, ent3.ID, ent4.ID} for _, id := range lookup { out, err := ts.Lookup(context.Background(), id) @@ -1380,6 +1384,8 @@ func TestTokenStore_HandleRequest_Revoke(t *testing.T) { t.Fatalf("bad: %#v", resp) } + time.Sleep(200 * time.Millisecond) + out, err := ts.Lookup(context.Background(), "child") if err != nil { t.Fatalf("err: %v", err) @@ -1416,6 +1422,8 @@ func TestTokenStore_HandleRequest_RevokeOrphan(t *testing.T) { t.Fatalf("bad: %#v", resp) } + time.Sleep(200 * time.Millisecond) + out, err := ts.Lookup(context.Background(), "child") if err != nil { t.Fatalf("err: %v", err) @@ -1469,6 +1477,8 @@ func TestTokenStore_HandleRequest_RevokeOrphan_NonRoot(t *testing.T) { t.Fatalf("did not get error when non-root revoking itself with orphan flag; resp is %#v", resp) } + time.Sleep(200 * time.Millisecond) + // Should still exist out, err = ts.Lookup(context.Background(), "child") if err != nil { From 0383875a04083134d965c650a7ac2e870d061632 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 8 May 2018 10:15:34 -0400 Subject: [PATCH 12/16] Various updates * 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 --- vault/core.go | 14 ++++++++++---- vault/expiration.go | 2 +- vault/generate_root.go | 2 +- vault/logical_system.go | 4 ++-- vault/request_handling.go | 6 +++--- vault/token_store.go | 35 +++++++++++++++++++++++++---------- vault/token_store_test.go | 20 ++++++++++---------- vault/wrapping.go | 14 +++++++------- 8 files changed, 59 insertions(+), 38 deletions(-) diff --git a/vault/core.go b/vault/core.go index 27851aee32eb..b76205e56114 100644 --- a/vault/core.go +++ b/vault/core.go @@ -1429,10 +1429,13 @@ func (c *Core) sealInitCommon(ctx context.Context, req *logical.Request) (retErr return retErr } - if te != nil && te.NumUses == tokenRevocationDeferred { + if te != nil && te.NumUses == tokenRevocationPending { // Token needs to be revoked. We do this immediately here because // we won't have a token store after sealing. - err = c.tokenStore.Revoke(c.activeContext, te.ID) + leaseID, err := c.expiration.CreateOrFetchRevocationLeaseByToken(te) + if err == nil { + err = c.expiration.Revoke(leaseID) + } if err != nil { c.logger.Error("token needed revocation before seal but failed to revoke", "error", err) retErr = multierror.Append(retErr, ErrInternalError) @@ -1540,10 +1543,13 @@ func (c *Core) StepDown(req *logical.Request) (retErr error) { return retErr } - if te != nil && te.NumUses == tokenRevocationDeferred { + if te != nil && te.NumUses == tokenRevocationPending { // Token needs to be revoked. We do this immediately here because // we won't have a token store after sealing. - err = c.tokenStore.Revoke(c.activeContext, te.ID) + leaseID, err := c.expiration.CreateOrFetchRevocationLeaseByToken(te) + if err == nil { + err = c.expiration.Revoke(leaseID) + } if err != nil { c.logger.Error("token needed revocation before step-down but failed to revoke", "error", err) retErr = multierror.Append(retErr, ErrInternalError) diff --git a/vault/expiration.go b/vault/expiration.go index bd8d5d165c7f..72522b8cf43d 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -1069,7 +1069,7 @@ func (m *ExpirationManager) revokeEntry(le *leaseEntry) error { // Revocation of login tokens is special since we can by-pass the // backend and directly interact with the token store if le.Auth != nil { - if err := m.tokenStore.RevokeTree(m.quitContext, le.ClientToken); err != nil { + if err := m.tokenStore.revokeTree(m.quitContext, le.ClientToken); err != nil { return errwrap.Wrapf("failed to revoke token: {{err}}", err) } diff --git a/vault/generate_root.go b/vault/generate_root.go index 16aad49ec468..37f408e02ec3 100644 --- a/vault/generate_root.go +++ b/vault/generate_root.go @@ -44,7 +44,7 @@ func (g generateStandardRootToken) generate(ctx context.Context, c *Core) (strin } cleanupFunc := func() { - c.tokenStore.Revoke(ctx, te.ID) + c.tokenStore.revokeOrphan(ctx, te.ID) } return te.ID, cleanupFunc, nil diff --git a/vault/logical_system.go b/vault/logical_system.go index f565241a03b2..d94533444805 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -3137,7 +3137,7 @@ func (b *SystemBackend) responseWrappingUnwrap(ctx context.Context, token string return "", errwrap.Wrapf("error decrementing wrapping token's use-count: {{err}}", err) } - defer b.Core.tokenStore.Revoke(ctx, token) + defer b.Core.tokenStore.revokeOrphan(ctx, token) } cubbyReq := &logical.Request{ @@ -3247,7 +3247,7 @@ func (b *SystemBackend) handleWrappingRewrap(ctx context.Context, req *logical.R if err != nil { return nil, errwrap.Wrapf("error decrementing wrapping token's use-count: {{err}}", err) } - defer b.Core.tokenStore.Revoke(ctx, token) + defer b.Core.tokenStore.revokeOrphan(ctx, token) } // Fetch the original TTL diff --git a/vault/request_handling.go b/vault/request_handling.go index e2554429addc..61d2b47ac174 100644 --- a/vault/request_handling.go +++ b/vault/request_handling.go @@ -178,7 +178,7 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp retErr = multierror.Append(retErr, logical.ErrPermissionDenied) return nil, nil, retErr } - if te.NumUses == tokenRevocationDeferred { + if te.NumUses == tokenRevocationPending { // 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. @@ -397,7 +397,7 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp } if err := c.expiration.RegisterAuth(te.Path, resp.Auth); err != nil { - c.tokenStore.Revoke(ctx, te.ID) + c.tokenStore.revokeOrphan(ctx, te.ID) c.logger.Error("failed to register token lease", "request_path", req.Path, "error", err) retErr = multierror.Append(retErr, ErrInternalError) return nil, auth, retErr @@ -601,7 +601,7 @@ func (c *Core) handleLoginRequest(ctx context.Context, req *logical.Request) (re // Register with the expiration manager if err := c.expiration.RegisterAuth(te.Path, auth); err != nil { - c.tokenStore.Revoke(ctx, te.ID) + c.tokenStore.revokeOrphan(ctx, te.ID) c.logger.Error("failed to register token lease", "request_path", req.Path, "error", err) return nil, auth, ErrInternalError } diff --git a/vault/token_store.go b/vault/token_store.go index 9992269e7c66..05498269910a 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -51,9 +51,11 @@ const ( // rolesPrefix is the prefix used to store role information rolesPrefix = "roles/" - // tokenRevocationDeferred indicates that the token should not be used - // again but is currently fulfilling its final use - tokenRevocationDeferred = -1 + // tokenRevocationPending indicates that the token should not be used + // again. If this is encountered during an existing request flow, it means + // that the token is but is currently fulfilling its final use; after this + // request it will not be able to be looked up as being valid. + tokenRevocationPending = -1 ) var ( @@ -913,12 +915,12 @@ func (ts *TokenStore) UseToken(ctx context.Context, te *TokenEntry) (*TokenEntry // manager revoking children) attempting to acquire the same lock // repeatedly. if te.NumUses == 1 { - te.NumUses = tokenRevocationDeferred + te.NumUses = tokenRevocationPending } else { te.NumUses-- } - err = ts.storeCommon(ctx, te, false) + err = ts.store(ctx, te) if err != nil { return nil, err } @@ -1049,7 +1051,7 @@ func (ts *TokenStore) lookupSalted(ctx context.Context, saltedID string, tainted // If fields are getting upgraded, store the changes if persistNeeded { - if err := ts.storeCommon(ctx, entry, false); err != nil { + if err := ts.store(ctx, entry); err != nil { return nil, errwrap.Wrapf("failed to persist token upgrade: {{err}}", err) } } @@ -1059,7 +1061,7 @@ func (ts *TokenStore) lookupSalted(ctx context.Context, saltedID string, tainted // Revoke is used to invalidate a given token, any child tokens // will be orphaned. -func (ts *TokenStore) Revoke(ctx context.Context, id string) error { +func (ts *TokenStore) revokeOrphan(ctx context.Context, id string) error { defer metrics.MeasureSince([]string{"token", "revoke"}, time.Now()) if id == "" { return fmt.Errorf("cannot revoke blank token") @@ -1095,6 +1097,19 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret er return nil } + if entry.NumUses != tokenRevocationPending { + entry.NumUses = tokenRevocationPending + if err := ts.store(ctx, entry); err != nil { + // The only real reason for this is an underlying storage error + // which also means that nothing else in this func or expmgr will + // really work either. So we clear revocation state so the user can + // try again. + ts.logger.Error("failed to mark token as revoked") + ts.tokensPendingDeletion.Store(saltedID, false) + return err + } + } + defer func() { // If we succeeded in all other revocation operations after this defer and // before we return, we can remove the token store entry @@ -1190,9 +1205,9 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret er return nil } -// RevokeTree is used to invalidate a given token and all +// revokeTree is used to invalidate a given token and all // child tokens. -func (ts *TokenStore) RevokeTree(ctx context.Context, id string) error { +func (ts *TokenStore) revokeTree(ctx context.Context, id string) error { defer metrics.MeasureSince([]string{"token", "revoke-tree"}, time.Now()) // Verify the token is not blank if id == "" { @@ -2131,7 +2146,7 @@ func (ts *TokenStore) handleRevokeOrphan(ctx context.Context, req *logical.Reque } // Revoke and orphan - if err := ts.Revoke(ctx, id); err != nil { + if err := ts.revokeOrphan(ctx, id); err != nil { return logical.ErrorResponse(err.Error()), logical.ErrInvalidRequest } diff --git a/vault/token_store_test.go b/vault/token_store_test.go index 8993fd778d80..22c7fade1dfd 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -638,10 +638,10 @@ func TestTokenStore_UseToken(t *testing.T) { if te == nil { t.Fatalf("token entry for use #2 was nil") } - if te.NumUses != tokenRevocationDeferred { + if te.NumUses != tokenRevocationPending { t.Fatalf("token entry after use #2 did not have revoke flag") } - ts.Revoke(context.Background(), te.ID) + ts.revokeOrphan(context.Background(), te.ID) // Lookup the token ent2, err = ts.Lookup(context.Background(), ent.ID) @@ -663,11 +663,11 @@ func TestTokenStore_Revoke(t *testing.T) { t.Fatalf("err: %v", err) } - err := ts.Revoke(context.Background(), "") + err := ts.revokeOrphan(context.Background(), "") if err.Error() != "cannot revoke blank token" { t.Fatalf("err: %v", err) } - err = ts.Revoke(context.Background(), ent.ID) + err = ts.revokeOrphan(context.Background(), ent.ID) if err != nil { t.Fatalf("err: %v", err) } @@ -721,7 +721,7 @@ func TestTokenStore_Revoke_Leases(t *testing.T) { } // Revoke the token - err = ts.Revoke(context.Background(), ent.ID) + err = ts.revokeOrphan(context.Background(), ent.ID) if err != nil { t.Fatalf("err: %v", err) } @@ -751,7 +751,7 @@ func TestTokenStore_Revoke_Orphan(t *testing.T) { t.Fatalf("err: %v", err) } - err := ts.Revoke(context.Background(), ent.ID) + err := ts.revokeOrphan(context.Background(), ent.ID) if err != nil { t.Fatalf("err: %v", err) } @@ -782,14 +782,14 @@ func TestTokenStore_RevokeTree(t *testing.T) { func testTokenStore_RevokeTree_NonRecursive(t testing.TB, depth uint64) { _, ts, _, _ := TestCoreWithTokenStore(t) root, children := buildTokenTree(t, ts, depth) - err := ts.RevokeTree(context.Background(), "") + err := ts.revokeTree(context.Background(), "") if err.Error() != "cannot tree-revoke blank token" { t.Fatalf("err: %v", err) } // Nuke tree non recursively. - err = ts.RevokeTree(context.Background(), root.ID) + err = ts.revokeTree(context.Background(), root.ID) if err != nil { t.Fatalf("err: %v", err) @@ -3335,7 +3335,7 @@ func TestTokenStore_RevokeUseCountToken(t *testing.T) { if te == nil { t.Fatal("nil entry") } - if te.NumUses != tokenRevocationDeferred { + if te.NumUses != tokenRevocationPending { t.Fatalf("bad: %d", te.NumUses) } @@ -3373,7 +3373,7 @@ func TestTokenStore_RevokeUseCountToken(t *testing.T) { if te == nil { t.Fatal("nil entry") } - if te.NumUses != tokenRevocationDeferred { + if te.NumUses != tokenRevocationPending { t.Fatalf("bad: %d", te.NumUses) } diff --git a/vault/wrapping.go b/vault/wrapping.go index 925e80b44c54..6c4361370720 100644 --- a/vault/wrapping.go +++ b/vault/wrapping.go @@ -159,7 +159,7 @@ DONELISTHANDLING: jwt := jws.NewJWT(claims, crypto.SigningMethodES512) serWebToken, err := jwt.Serialize(c.wrappingJWTKey) if err != nil { - c.tokenStore.Revoke(ctx, te.ID) + c.tokenStore.revokeOrphan(ctx, te.ID) c.logger.Error("failed to serialize JWT", "error", err) return nil, ErrInternalError } @@ -200,7 +200,7 @@ DONELISTHANDLING: marshaledResponse, err := json.Marshal(httpResponse) if err != nil { - c.tokenStore.Revoke(ctx, te.ID) + c.tokenStore.revokeOrphan(ctx, te.ID) c.logger.Error("failed to marshal wrapped response", "error", err) return nil, ErrInternalError } @@ -213,12 +213,12 @@ DONELISTHANDLING: cubbyResp, err := c.router.Route(ctx, cubbyReq) if err != nil { // Revoke since it's not yet being tracked for expiration - c.tokenStore.Revoke(ctx, te.ID) + c.tokenStore.revokeOrphan(ctx, te.ID) c.logger.Error("failed to store wrapped response information", "error", err) return nil, ErrInternalError } if cubbyResp != nil && cubbyResp.IsError() { - c.tokenStore.Revoke(ctx, te.ID) + c.tokenStore.revokeOrphan(ctx, te.ID) c.logger.Error("failed to store wrapped response information", "error", cubbyResp.Data["error"]) return cubbyResp, nil } @@ -239,12 +239,12 @@ DONELISTHANDLING: cubbyResp, err = c.router.Route(ctx, cubbyReq) if err != nil { // Revoke since it's not yet being tracked for expiration - c.tokenStore.Revoke(ctx, te.ID) + c.tokenStore.revokeOrphan(ctx, te.ID) c.logger.Error("failed to store wrapping information", "error", err) return nil, ErrInternalError } if cubbyResp != nil && cubbyResp.IsError() { - c.tokenStore.Revoke(ctx, te.ID) + c.tokenStore.revokeOrphan(ctx, te.ID) c.logger.Error("failed to store wrapping information", "error", cubbyResp.Data["error"]) return cubbyResp, nil } @@ -261,7 +261,7 @@ DONELISTHANDLING: // Register the wrapped token with the expiration manager if err := c.expiration.RegisterAuth(te.Path, wAuth); err != nil { // Revoke since it's not yet being tracked for expiration - c.tokenStore.Revoke(ctx, te.ID) + c.tokenStore.revokeOrphan(ctx, te.ID) c.logger.Error("failed to register cubbyhole wrapping token lease", "request_path", req.Path, "error", err) return nil, ErrInternalError } From 8d6ab912915c3beda84f157093f8104f5768d4aa Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Tue, 8 May 2018 11:22:47 -0400 Subject: [PATCH 13/16] Address feedback --- vault/expiration.go | 10 +++------- vault/token_store.go | 2 +- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/vault/expiration.go b/vault/expiration.go index 72522b8cf43d..362604541026 100644 --- a/vault/expiration.go +++ b/vault/expiration.go @@ -575,7 +575,8 @@ func (m *ExpirationManager) RevokeByToken(te *TokenEntry) error { } // If there's a lease, set expiration to now, persist, and call - // updatePending to hand off revocation to the expiration manager + // updatePending to hand off revocation to the expiration manager's pending + // timer map if le != nil { le.ExpireTime = time.Now() @@ -1292,15 +1293,10 @@ func (m *ExpirationManager) CreateOrFetchRevocationLeaseByToken(te *TokenEntry) return "", consts.ErrPathContainsParentReferences } - saltedID, err := m.tokenStore.SaltID(m.quitContext, auth.ClientToken) - if err != nil { - return "", err - } - // Create a lease entry now := time.Now() le = &leaseEntry{ - LeaseID: path.Join(te.Path, saltedID), + LeaseID: leaseID, ClientToken: auth.ClientToken, Auth: auth, Path: te.Path, diff --git a/vault/token_store.go b/vault/token_store.go index 05498269910a..2e1e526452d1 100644 --- a/vault/token_store.go +++ b/vault/token_store.go @@ -1082,7 +1082,7 @@ func (ts *TokenStore) revokeSalted(ctx context.Context, saltedID string) (ret er // failed (state is false) state, loaded := ts.tokensPendingDeletion.LoadOrStore(saltedID, true) - // If the entry was loaded and it's state is true, we shortcircuit + // If the entry was loaded and its state is true, we short-circuit if loaded && state == true { return nil } From a39597ecdc23cf7fc69fe003eef9f10d533551d8 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Tue, 8 May 2018 15:13:30 -0400 Subject: [PATCH 14/16] Return invalid lease on negative TTL --- vault/logical_system.go | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/vault/logical_system.go b/vault/logical_system.go index d94533444805..f78f9a223503 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -2215,34 +2215,36 @@ func (b *SystemBackend) handleLeaseLookup(ctx context.Context, req *logical.Requ logical.ErrInvalidRequest } - leaseTimes, err := b.Core.expiration.FetchLeaseTimes(leaseID) + le, err := b.Core.expiration.FetchLeaseTimes(leaseID) if err != nil { b.Backend.Logger().Error("error retrieving lease", "lease_id", leaseID, "error", err) return handleError(err) } - if leaseTimes == nil { + + if le == nil || le.ttl() < 0 { return logical.ErrorResponse("invalid lease"), logical.ErrInvalidRequest } resp := &logical.Response{ Data: map[string]interface{}{ "id": leaseID, - "issue_time": leaseTimes.IssueTime, + "issue_time": le.IssueTime, "expire_time": nil, "last_renewal": nil, "ttl": int64(0), }, } - renewable, _ := leaseTimes.renewable() + renewable, _ := le.renewable() resp.Data["renewable"] = renewable - if !leaseTimes.LastRenewalTime.IsZero() { - resp.Data["last_renewal"] = leaseTimes.LastRenewalTime + if !le.LastRenewalTime.IsZero() { + resp.Data["last_renewal"] = le.LastRenewalTime } - if !leaseTimes.ExpireTime.IsZero() { - resp.Data["expire_time"] = leaseTimes.ExpireTime - resp.Data["ttl"] = leaseTimes.ttl() + if !le.ExpireTime.IsZero() { + resp.Data["expire_time"] = le.ExpireTime + resp.Data["ttl"] = le.ttl() } + return resp, nil } From c107e0cc86f4b5c404405576b42cced8297034b0 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Tue, 8 May 2018 16:56:56 -0400 Subject: [PATCH 15/16] Revert "Return invalid lease on negative TTL" This reverts commit a39597ecdc23cf7fc69fe003eef9f10d533551d8. --- vault/logical_system.go | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/vault/logical_system.go b/vault/logical_system.go index f78f9a223503..d94533444805 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -2215,36 +2215,34 @@ func (b *SystemBackend) handleLeaseLookup(ctx context.Context, req *logical.Requ logical.ErrInvalidRequest } - le, err := b.Core.expiration.FetchLeaseTimes(leaseID) + leaseTimes, err := b.Core.expiration.FetchLeaseTimes(leaseID) if err != nil { b.Backend.Logger().Error("error retrieving lease", "lease_id", leaseID, "error", err) return handleError(err) } - - if le == nil || le.ttl() < 0 { + if leaseTimes == nil { return logical.ErrorResponse("invalid lease"), logical.ErrInvalidRequest } resp := &logical.Response{ Data: map[string]interface{}{ "id": leaseID, - "issue_time": le.IssueTime, + "issue_time": leaseTimes.IssueTime, "expire_time": nil, "last_renewal": nil, "ttl": int64(0), }, } - renewable, _ := le.renewable() + renewable, _ := leaseTimes.renewable() resp.Data["renewable"] = renewable - if !le.LastRenewalTime.IsZero() { - resp.Data["last_renewal"] = le.LastRenewalTime + if !leaseTimes.LastRenewalTime.IsZero() { + resp.Data["last_renewal"] = leaseTimes.LastRenewalTime } - if !le.ExpireTime.IsZero() { - resp.Data["expire_time"] = le.ExpireTime - resp.Data["ttl"] = le.ttl() + if !leaseTimes.ExpireTime.IsZero() { + resp.Data["expire_time"] = leaseTimes.ExpireTime + resp.Data["ttl"] = leaseTimes.ttl() } - return resp, nil } From 4455e38eeb0a4636732f2653cb5bf560cea8fb57 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Wed, 9 May 2018 12:58:33 -0400 Subject: [PATCH 16/16] Extend sleep on tests --- vault/expiration_test.go | 8 ++++---- vault/token_store_test.go | 14 +++++++------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/vault/expiration_test.go b/vault/expiration_test.go index 0d524d236b62..823a65099153 100644 --- a/vault/expiration_test.go +++ b/vault/expiration_test.go @@ -742,7 +742,7 @@ func TestExpiration_RevokeByToken(t *testing.T) { t.Fatalf("err: %v", err) } - time.Sleep(200 * time.Millisecond) + time.Sleep(300 * time.Millisecond) noop.Lock() defer noop.Unlock() @@ -773,7 +773,7 @@ func TestExpiration_RevokeByToken_Blocking(t *testing.T) { noop := &NoopBackend{} // Request handle with a timeout context that simulates blocking lease revocation. noop.RequestHandler = func(ctx context.Context, req *logical.Request) (*logical.Response, error) { - ctx, cancel := context.WithTimeout(ctx, 100*time.Millisecond) + ctx, cancel := context.WithTimeout(ctx, 200*time.Millisecond) defer cancel() select { @@ -838,7 +838,7 @@ func TestExpiration_RevokeByToken_Blocking(t *testing.T) { // Wait for a bit for timeouts to trigger and pending revocations to go // through and then we relock - time.Sleep(200 * time.Millisecond) + time.Sleep(300 * time.Millisecond) noop.Lock() defer noop.Unlock() @@ -1341,7 +1341,7 @@ func TestExpiration_revokeEntry_token(t *testing.T) { t.Fatalf("err: %v", err) } - time.Sleep(200 * time.Millisecond) + time.Sleep(300 * time.Millisecond) out, err := exp.tokenStore.Lookup(context.Background(), le.ClientToken) if err != nil { diff --git a/vault/token_store_test.go b/vault/token_store_test.go index 22c7fade1dfd..d6eff4ea1c85 100644 --- a/vault/token_store_test.go +++ b/vault/token_store_test.go @@ -399,7 +399,7 @@ func TestTokenStore_HandleRequest_RevokeAccessor(t *testing.T) { t.Fatalf("err: %s", err) } - time.Sleep(200 * time.Millisecond) + time.Sleep(300 * time.Millisecond) out, err = ts.Lookup(context.Background(), "tokenid") if err != nil { @@ -726,7 +726,7 @@ func TestTokenStore_Revoke_Leases(t *testing.T) { t.Fatalf("err: %v", err) } - time.Sleep(200 * time.Millisecond) + time.Sleep(300 * time.Millisecond) // Verify the lease is gone out, err := ts.expiration.loadEntry(leaseID) @@ -885,7 +885,7 @@ func TestTokenStore_RevokeSelf(t *testing.T) { t.Fatalf("err: %v\nresp: %#v", err, resp) } - time.Sleep(200 * time.Millisecond) + time.Sleep(300 * time.Millisecond) lookup := []string{ent1.ID, ent2.ID, ent3.ID, ent4.ID} for _, id := range lookup { @@ -1383,7 +1383,7 @@ func TestTokenStore_HandleRequest_Revoke(t *testing.T) { t.Fatalf("bad: %#v", resp) } - time.Sleep(200 * time.Millisecond) + time.Sleep(300 * time.Millisecond) out, err := ts.Lookup(context.Background(), "child") if err != nil { @@ -1421,7 +1421,7 @@ func TestTokenStore_HandleRequest_RevokeOrphan(t *testing.T) { t.Fatalf("bad: %#v", resp) } - time.Sleep(200 * time.Millisecond) + time.Sleep(300 * time.Millisecond) out, err := ts.Lookup(context.Background(), "child") if err != nil { @@ -1476,7 +1476,7 @@ func TestTokenStore_HandleRequest_RevokeOrphan_NonRoot(t *testing.T) { t.Fatalf("did not get error when non-root revoking itself with orphan flag; resp is %#v", resp) } - time.Sleep(200 * time.Millisecond) + time.Sleep(300 * time.Millisecond) // Should still exist out, err = ts.Lookup(context.Background(), "child") @@ -3845,7 +3845,7 @@ func TestTokenStore_TidyLeaseRevocation(t *testing.T) { // Call tidy ts.handleTidy(context.Background(), nil, nil) - time.Sleep(200 * time.Millisecond) + time.Sleep(300 * time.Millisecond) // Verify leases are gone storedLeases, err = exp.lookupLeasesByToken(tut)