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

Backport 1.7: On lease deletion, also delete non-orphan batch token parent index (#… #11386

Merged
merged 1 commit into from
Apr 19, 2021
Merged

Backport 1.7: On lease deletion, also delete non-orphan batch token parent index (#… #11386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions changelog/11377.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
core: Fix storage entry leak when revoking leases created with non-orphan batch tokens.
```
51 changes: 35 additions & 16 deletions vault/expiration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,8 +915,24 @@ func (m *ExpirationManager) revokeCommon(ctx context.Context, leaseID string, fo

// Delete the secondary index, but only if it's a leased secret (not auth)
if le.Secret != nil {
if err := m.removeIndexByToken(ctx, le); err != nil {
return err
var indexToken string
// Maintain secondary index by token, except for orphan batch tokens
switch le.ClientTokenType {
case logical.TokenTypeBatch:
te, err := m.tokenStore.lookupBatchTokenInternal(ctx, le.ClientToken)
if err != nil {
return err
}
// If it's a non-orphan batch token, assign the secondary index to its
// parent
indexToken = te.Parent
default:
indexToken = le.ClientToken
}
if indexToken != "" {
if err := m.removeIndexByToken(ctx, le, indexToken); err != nil {
return err
}
}
}

Expand Down Expand Up @@ -1364,6 +1380,17 @@ func (m *ExpirationManager) Register(ctx context.Context, req *logical.Request,
Version: 1,
}

var indexToken string
// Maintain secondary index by token, except for orphan batch tokens
switch {
case te.Type != logical.TokenTypeBatch:
indexToken = le.ClientToken
case te.Parent != "":
// If it's a non-orphan batch token, assign the secondary index to its
// parent
indexToken = te.Parent
}

defer func() {
// If there is an error we want to rollback as much as possible (note
// that errors here are ignored to do as much cleanup as we can). We
Expand All @@ -1382,7 +1409,7 @@ func (m *ExpirationManager) Register(ctx context.Context, req *logical.Request,
retErr = multierror.Append(retErr, errwrap.Wrapf("an additional error was encountered deleting any lease associated with the newly-generated secret: {{err}}", err))
}

if err := m.removeIndexByToken(ctx, le); err != nil {
if err := m.removeIndexByToken(ctx, le, indexToken); err != nil {
retErr = multierror.Append(retErr, errwrap.Wrapf("an additional error was encountered removing lease indexes associated with the newly-generated secret: {{err}}", err))
}
}
Expand All @@ -1408,16 +1435,8 @@ func (m *ExpirationManager) Register(ctx context.Context, req *logical.Request,
return "", err
}

// Maintain secondary index by token, except for orphan batch tokens
switch {
case te.Type != logical.TokenTypeBatch:
if err := m.createIndexByToken(ctx, le, le.ClientToken); err != nil {
return "", err
}
case te.Parent != "":
// If it's a non-orphan batch token, assign the secondary index to its
// parent
if err := m.createIndexByToken(ctx, le, te.Parent); err != nil {
if indexToken != "" {
if err := m.createIndexByToken(ctx, le, indexToken); err != nil {
return "", err
}
}
Expand Down Expand Up @@ -1966,10 +1985,10 @@ func (m *ExpirationManager) indexByToken(ctx context.Context, le *leaseEntry) (*
}

// removeIndexByToken removes the secondary index from the token to a lease entry
func (m *ExpirationManager) removeIndexByToken(ctx context.Context, le *leaseEntry) error {
func (m *ExpirationManager) removeIndexByToken(ctx context.Context, le *leaseEntry, token string) error {
tokenNS := namespace.RootNamespace
saltCtx := namespace.ContextWithNamespace(ctx, namespace.RootNamespace)
_, nsID := namespace.SplitIDFromString(le.ClientToken)
_, nsID := namespace.SplitIDFromString(token)
if nsID != "" {
var err error
tokenNS, err = NamespaceByID(ctx, nsID, m.core)
Expand All @@ -1990,7 +2009,7 @@ func (m *ExpirationManager) removeIndexByToken(ctx context.Context, le *leaseEnt
}
}

saltedID, err := m.tokenStore.SaltID(saltCtx, le.ClientToken)
saltedID, err := m.tokenStore.SaltID(saltCtx, token)
if err != nil {
return err
}
Expand Down
33 changes: 22 additions & 11 deletions vault/expiration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,33 +681,37 @@ func TestExpiration_Register(t *testing.T) {
}

func TestExpiration_Register_BatchToken(t *testing.T) {
exp := mockExpiration(t)
c, _, rootToken := TestCoreUnsealed(t)
exp := c.expiration
noop := &NoopBackend{
RequestHandler: func(ctx context.Context, req *logical.Request) (*logical.Response, error) {
resp := &logical.Response{Secret: req.Secret}
resp.Secret.TTL = time.Hour
return resp, 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", namespace: namespace.RootNamespace}, view)
if err != nil {
t.Fatal(err)
{
_, 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", namespace: namespace.RootNamespace}, view)
if err != nil {
t.Fatal(err)
}
}

te := &logical.TokenEntry{
Type: logical.TokenTypeBatch,
TTL: 1 * time.Second,
NamespaceID: "root",
CreationTime: time.Now().Unix(),
Parent: rootToken,
}

err = exp.tokenStore.create(context.Background(), te)
err := exp.tokenStore.create(context.Background(), te)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -764,6 +768,13 @@ func TestExpiration_Register_BatchToken(t *testing.T) {

break
}
idEnts, err := exp.tokenView.List(context.Background(), "")
if err != nil {
t.Fatal(err)
}
if len(idEnts) != 0 {
t.Fatalf("expected no entries in sys/expire/token, got: %v", idEnts)
}
}

func TestExpiration_RegisterAuth(t *testing.T) {
Expand Down
13 changes: 11 additions & 2 deletions vault/token_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1123,7 +1123,7 @@ func (ts *TokenStore) lookupTainted(ctx context.Context, id string) (*logical.To
return ts.lookupInternal(ctx, id, false, true)
}

func (ts *TokenStore) lookupBatchToken(ctx context.Context, id string) (*logical.TokenEntry, error) {
func (ts *TokenStore) lookupBatchTokenInternal(ctx context.Context, id string) (*logical.TokenEntry, error) {
// Strip the b. from the front and namespace ID from the back
bEntry, _ := namespace.SplitIDFromString(id[2:])

Expand All @@ -1147,6 +1147,16 @@ func (ts *TokenStore) lookupBatchToken(ctx context.Context, id string) (*logical
return nil, err
}

te.ID = id
return te, nil
}

func (ts *TokenStore) lookupBatchToken(ctx context.Context, id string) (*logical.TokenEntry, error) {
te, err := ts.lookupBatchTokenInternal(ctx, id)
if err != nil {
return nil, err
}

if time.Now().After(time.Unix(te.CreationTime, 0).Add(te.TTL)) {
return nil, nil
}
Expand All @@ -1161,7 +1171,6 @@ func (ts *TokenStore) lookupBatchToken(ctx context.Context, id string) (*logical
}
}

te.ID = id
return te, nil
}

Expand Down