From 0383875a04083134d965c650a7ac2e870d061632 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 8 May 2018 10:15:34 -0400 Subject: [PATCH] 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 }