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

core: revoke the proper token on partial failures from token-related requests #7835

Merged
merged 4 commits into from
Nov 8, 2019
Merged

core: revoke the proper token on partial failures from token-related requests #7835

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f59cb6a
core: revoke the proper token on partial failures from token-related …
calvn Nov 7, 2019
bf17088
move test to vault package, move test trigger to expiration manager
calvn Nov 8, 2019
60ff5b9
update logging messages for clarity
calvn Nov 8, 2019
0da7a49
docstring fix
calvn Nov 8, 2019
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
60 changes: 60 additions & 0 deletions vault/core_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2386,3 +2386,63 @@ func TestCore_HandleRequest_Headers_denyList(t *testing.T) {
t.Fatalf("did not expect %q to be in the headers map", consts.AuthHeaderName)
}
}

func TestCore_HandleRequest_TokenCreate_RegisterAuthFailure(t *testing.T) {
core, _, root := TestCoreUnsealed(t)

// Create a root token and use that for subsequent requests
req := logical.TestRequest(t, logical.CreateOperation, "auth/token/create")
req.Data = map[string]interface{}{
"policies": []string{"root"},
}
req.ClientToken = root
resp, err := core.HandleRequest(namespace.RootContext(nil), req)
if err != nil {
t.Fatal(err)
}
if resp == nil || resp.Auth == nil || resp.Auth.ClientToken == "" {
t.Fatalf("expected a response from token creation, got: %#v", resp)
}
tokenWithRootPolicy := resp.Auth.ClientToken

// Use new token to create yet a new token, this should succeed
req = logical.TestRequest(t, logical.CreateOperation, "auth/token/create")
req.ClientToken = tokenWithRootPolicy
_, err = core.HandleRequest(namespace.RootContext(nil), req)
if err != nil {
t.Fatal(err)
}

// Try again but force failure on RegisterAuth to simulate a network failure
// when registering the lease (e.g. a storage failure). This should trigger
// an expiration manager cleanup on the newly created token
core.expiration.testRegisterAuthFailure.Store(true)
req = logical.TestRequest(t, logical.CreateOperation, "auth/token/create")
req.ClientToken = tokenWithRootPolicy
resp, err = core.HandleRequest(namespace.RootContext(nil), req)
if err == nil {
t.Fatalf("expected error, got a response: %#v", resp)
}
core.expiration.testRegisterAuthFailure.Store(false)

// Do a lookup against the client token that we used for the failed request.
// It should still be present
req = logical.TestRequest(t, logical.UpdateOperation, "auth/token/lookup")
req.Data = map[string]interface{}{
"token": tokenWithRootPolicy,
}
req.ClientToken = root
_, err = core.HandleRequest(namespace.RootContext(nil), req)
if err != nil {
t.Fatal(err)
}

// Do a token creation request with the token to ensure that it's still
// valid, should succeed.
req = logical.TestRequest(t, logical.CreateOperation, "auth/token/create")
req.ClientToken = tokenWithRootPolicy
resp, err = core.HandleRequest(namespace.RootContext(nil), req)
if err != nil {
t.Fatal(err)
}
}
14 changes: 13 additions & 1 deletion vault/expiration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"github.com/hashicorp/vault/sdk/helper/jsonutil"
"github.com/hashicorp/vault/sdk/helper/locksutil"
"github.com/hashicorp/vault/sdk/logical"
uberAtomic "go.uber.org/atomic"
)

const (
Expand All @@ -48,7 +49,7 @@ const (
// defaultLeaseDuration is the default lease duration used when no lease is specified
defaultLeaseTTL = maxLeaseTTL

//maxLeaseThreshold is the maximum lease count before generating log warning
// maxLeaseThreshold is the maximum lease count before generating log warning
maxLeaseThreshold = 256000
)

Expand Down Expand Up @@ -87,6 +88,11 @@ type ExpirationManager struct {

logLeaseExpirations bool
expireFunc ExpireLeaseStrategy

// testRegisterAuthFailure, if set to true, triggers an explicit failure on
// RegisterAuth to simulate a partial failure during a token creation
// request. This value should only be set by tests.
testRegisterAuthFailure uberAtomic.Bool
}

type ExpireLeaseStrategy func(context.Context, *ExpirationManager, *leaseEntry)
Expand Down Expand Up @@ -1147,6 +1153,12 @@ func (m *ExpirationManager) Register(ctx context.Context, req *logical.Request,
func (m *ExpirationManager) RegisterAuth(ctx context.Context, te *logical.TokenEntry, auth *logical.Auth) error {
defer metrics.MeasureSince([]string{"expire", "register-auth"}, time.Now())

// Triggers failure of RegisterAuth. This should only be set and triggered
// by tests to simulate partial failure during a token creation request.
if m.testRegisterAuthFailure.Load() {
return fmt.Errorf("failing explicitly on RegisterAuth")
}

authExpirationTime := auth.ExpirationTime()

if te.TTL == 0 && authExpirationTime.IsZero() && (len(te.Policies) != 1 || te.Policies[0] != "root") {
Expand Down
16 changes: 13 additions & 3 deletions vault/request_handling.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -860,7 +860,11 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp

_, identityPolicies, err := c.fetchEntityAndDerivedPolicies(ctx, tokenNS, resp.Auth.EntityID)
if err != nil {
c.tokenStore.revokeOrphan(ctx, te.ID)
Copy link

Choose a reason for hiding this comment

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

we probably should log the error here as well?

// Best-effort clean up on error, so we log the cleanup error as a
// warning but still return as internal error.
if err := c.tokenStore.revokeOrphan(ctx, resp.Auth.ClientToken); err != nil {
c.logger.Warn("failed to clean up token lease", "request_path", req.Path, "error", err)
}
return nil, nil, ErrInternalError
}

Expand All @@ -874,7 +878,11 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp
Path: resp.Auth.CreationPath,
NamespaceID: ns.ID,
}, resp.Auth); err != nil {
c.tokenStore.revokeOrphan(ctx, te.ID)
// Best-effort clean up on error, so we log the cleanup error as
// a warning but still return as internal error.
if err := c.tokenStore.revokeOrphan(ctx, resp.Auth.ClientToken); err != nil {
c.logger.Warn("failed to clean up token lease", "request_path", req.Path, "error", err)
}
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 @@ -1209,7 +1217,9 @@ func (c *Core) RegisterAuth(ctx context.Context, tokenTTL time.Duration, path st
case logical.TokenTypeService:
// Register with the expiration manager
if err := c.expiration.RegisterAuth(ctx, &te, auth); err != nil {
c.tokenStore.revokeOrphan(ctx, te.ID)
if err := c.tokenStore.revokeOrphan(ctx, te.ID); err != nil {
c.logger.Warn("failed to clean up token lease", "request_path", path, "error", err)
Copy link

Choose a reason for hiding this comment

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

Perhaps failing here should produce a slightly different log message to make orphan revocation by ID distinct from revocation by client token lease. Something like failed to clean up token lease - id not found vs failed to clean up token lease - not found. If err will be sufficiently unique and the logger in fact captures that output, then this isn't necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to the difference between using te.ID here and resp.Auth.ClientToken above? The revoke orphan call here ultimately points to to the same thing (i.e. the token ID that we're trying to revoke) since the auth that we pass into this func is from resp.Auth and we set auth.ClientToken = te.ID up in L1208. The revocation is not by client token lease ID.

The reason why we're able to use te.ID is because we instantiate a new instance of te within the func, and in all three revoke orphan cases we're now referencing the token ID that's being created instead of the request's client token.

Copy link

@roberteckert roberteckert Nov 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I was indeed referring to te.ID vs resp.Auth.ClientToken. If it's all the same value and call then the messaging is sensible. Thanks.

}
c.logger.Error("failed to register token lease", "request_path", path, "error", err)
return ErrInternalError
}
Expand Down