Skip to content

Commit

Permalink
Various updates
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@jefferai
jefferai committed May 8, 2018
1 parent 397f313 commit 0383875
Show file tree
Hide file tree
Showing 8 changed files with 59 additions and 38 deletions.
14 changes: 10 additions & 4 deletions vault/core.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion vault/expiration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
2 changes: 1 addition & 1 deletion vault/generate_root.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions vault/logical_system.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions vault/request_handling.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
35 changes: 25 additions & 10 deletions vault/token_store.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
}
}
Expand All @@ -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")
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 == "" {
Expand Down Expand Up @@ -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
}

Expand Down
20 changes: 10 additions & 10 deletions vault/token_store_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}

Expand Down
14 changes: 7 additions & 7 deletions vault/wrapping.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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
}
Expand Down

0 comments on commit 0383875

Please sign in to comment.