Fix removing secondary index from exp manager.

Due to a typo, revoking ensures that index entries are created rather
than removed. This adds a failing, then fixed test case (and helper
function) to ensure that index entries are properly removed on revoke.

Fixes #749

CHANGELOG.md

@@ -56,6 +56,7 @@ generate them, leading to client errors.
    enabled [GH-694]
  * core: Fix an error that could happen in some failure scenarios where Vault
    could fail to revert to a clean state [GH-733]
+ * core: Ensure secondary indexes are removed when a lease is expired [GH-749]
  * everywhere: Don't use http.DefaultClient, as it shares state implicitly and
    is a source of hard-to-track-down bugs [GH-700]
  * credential/token: Allow creating orphan tokens via an API path [GH-748]

vault/expiration.go

@@ -195,7 +195,7 @@ func (m *ExpirationManager) Revoke(leaseID string) error {
 	}
 
 	// Delete the secondary index
-	if err := m.indexByToken(le.ClientToken, le.LeaseID); err != nil {
+	if err := m.removeIndexByToken(le.ClientToken, le.LeaseID); err != nil {
 		return err
 	}
 
@@ -387,7 +387,7 @@ func (m *ExpirationManager) Register(req *logical.Request, resp *logical.Respons
 	}
 
 	// Maintain secondary index by token
-	if err := m.indexByToken(le.ClientToken, le.LeaseID); err != nil {
+	if err := m.createIndexByToken(le.ClientToken, le.LeaseID); err != nil {
 		return "", err
 	}
 
@@ -567,8 +567,8 @@ func (m *ExpirationManager) deleteEntry(leaseID string) error {
 	return nil
 }
 
-// indexByToken creates a secondary index from the token to a lease entry
-func (m *ExpirationManager) indexByToken(token, leaseID string) error {
+// createIndexByToken creates a secondary index from the token to a lease entry
+func (m *ExpirationManager) createIndexByToken(token, leaseID string) error {
 	ent := logical.StorageEntry{
 		Key:   m.tokenStore.SaltID(token) + "/" + m.tokenStore.SaltID(leaseID),
 		Value: []byte(leaseID),
@@ -579,6 +579,16 @@ func (m *ExpirationManager) indexByToken(token, leaseID string) error {
 	return nil
 }
 
+// indexByToken looks up the secondary index from the token to a lease entry
+func (m *ExpirationManager) indexByToken(token, leaseID string) (*logical.StorageEntry, error) {
+	key := m.tokenStore.SaltID(token) + "/" + m.tokenStore.SaltID(leaseID)
+	entry, err := m.tokenView.Get(key)
+	if err != nil {
+		return nil, fmt.Errorf("failed to look up secondary index entry")
+	}
+	return entry, nil
+}
+
 // removeIndexByToken removes the secondary index from the token to a lease entry
 func (m *ExpirationManager) removeIndexByToken(token, leaseID string) error {
 	key := m.tokenStore.SaltID(token) + "/" + m.tokenStore.SaltID(leaseID)

vault/expiration_test.go

@@ -665,9 +665,26 @@ func TestExpiration_revokeEntry_token(t *testing.T) {
 				TTL: time.Minute,
 			},
 		},
-		Path:       "foo/bar",
-		IssueTime:  time.Now(),
-		ExpireTime: time.Now(),
+		ClientToken: root.ID,
+		Path:        "foo/bar",
+		IssueTime:   time.Now(),
+		ExpireTime:  time.Now(),
+	}
+
+	if err := exp.persistEntry(le); err != nil {
+		t.Fatalf("error persisting entry: %v", err)
+	}
+	if err := exp.createIndexByToken(le.ClientToken, le.LeaseID); err != nil {
+		t.Fatalf("error creating secondary index: %v", err)
+	}
+	exp.updatePending(le, le.Auth.LeaseTotal())
+
+	indexEntry, err := exp.indexByToken(le.ClientToken, le.LeaseID)
+	if err != nil {
+		t.Fatalf("err: %v")
+	}
+	if indexEntry == nil {
+		t.Fatalf("err: should have found a secondary index entry")
 	}
 
 	err = exp.revokeEntry(le)
@@ -675,13 +692,21 @@ func TestExpiration_revokeEntry_token(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
-	out, err := exp.tokenStore.Lookup(root.ID)
+	out, err := exp.tokenStore.Lookup(le.ClientToken)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
 	if out != nil {
 		t.Fatalf("bad: %v", out)
 	}
+
+	indexEntry, err = exp.indexByToken(le.ClientToken, le.LeaseID)
+	if err != nil {
+		t.Fatalf("err: %v")
+	}
+	if indexEntry != nil {
+		t.Fatalf("err: should not have found a secondary index entry")
+	}
 }
 
 func TestExpiration_renewEntry(t *testing.T) {