From 462caf4c6d0e453fa547261ecdef6666333e702f Mon Sep 17 00:00:00 2001
From: Vishal Nayak <vishalnayak@users.noreply.github.com>
Date: Tue, 6 Feb 2018 15:44:48 -0500
Subject: [PATCH] AppRole: Cleanup accessor indexes and dangling accessor
 indexes (#3924)

* Cleanup accessor indexes and dangling accessor indexes

* Add a test that exercises the accessor cleanup
---
 .../credential/approle/path_tidy_user_id.go   | 42 ++++++++++
 .../approle/path_tidy_user_id_test.go         | 79 +++++++++++++++++++
 2 files changed, 121 insertions(+)
 create mode 100644 builtin/credential/approle/path_tidy_user_id_test.go

diff --git a/builtin/credential/approle/path_tidy_user_id.go b/builtin/credential/approle/path_tidy_user_id.go
index 686a8f2e8012..437d5c8137e3 100644
--- a/builtin/credential/approle/path_tidy_user_id.go
+++ b/builtin/credential/approle/path_tidy_user_id.go
@@ -38,6 +38,16 @@ func (b *backend) tidySecretID(ctx context.Context, s logical.Storage) error {
 		return err
 	}
 
+	// List all the accessors and add them all to a map
+	accessorHashes, err := s.List(ctx, "accessor/")
+	if err != nil {
+		return err
+	}
+	accessorMap := make(map[string]bool, len(accessorHashes))
+	for _, accessorHash := range accessorHashes {
+		accessorMap[accessorHash] = true
+	}
+
 	var result error
 	for _, roleNameHMAC := range roleNameHMACs {
 		// roleNameHMAC will already have a '/' suffix. Don't append another one.
@@ -77,14 +87,46 @@ func (b *backend) tidySecretID(ctx context.Context, s logical.Storage) error {
 
 			// ExpirationTime not being set indicates non-expiring SecretIDs
 			if !result.ExpirationTime.IsZero() && time.Now().After(result.ExpirationTime) {
+				// Clean up the accessor of the secret ID first
+				err = b.deleteSecretIDAccessorEntry(ctx, s, result.SecretIDAccessor)
+				if err != nil {
+					lock.Unlock()
+					return err
+				}
+
 				if err := s.Delete(ctx, entryIndex); err != nil {
 					lock.Unlock()
 					return fmt.Errorf("error deleting SecretID %s from storage: %s", secretIDHMAC, err)
 				}
 			}
+
+			// At this point, the secret ID is not expired and is valid. Delete
+			// the corresponding accessor from the accessorMap. This will leave
+			// only the dangling accessors in the map which can then be cleaned
+			// up later.
+			salt, err := b.Salt()
+			if err != nil {
+				lock.Unlock()
+				return err
+			}
+			delete(accessorMap, salt.SaltID(result.SecretIDAccessor))
+
 			lock.Unlock()
 		}
 	}
+
+	// Accessor indexes were not getting cleaned up until 0.9.3. This is a fix
+	// to clean up the dangling accessor entries.
+	for accessorHash, _ := range accessorMap {
+		// Ideally, locking should be performed here. But for that, accessors
+		// are required in plaintext, which are not available. Hence performing
+		// a racy cleanup.
+		err = s.Delete(ctx, "accessor/"+accessorHash)
+		if err != nil {
+			return err
+		}
+	}
+
 	return result
 }
 
diff --git a/builtin/credential/approle/path_tidy_user_id_test.go b/builtin/credential/approle/path_tidy_user_id_test.go
new file mode 100644
index 000000000000..b52b711356f4
--- /dev/null
+++ b/builtin/credential/approle/path_tidy_user_id_test.go
@@ -0,0 +1,79 @@
+package approle
+
+import (
+	"context"
+	"testing"
+
+	"github.com/hashicorp/vault/logical"
+)
+
+func TestAppRole_TidyDanglingAccessors(t *testing.T) {
+	var resp *logical.Response
+	var err error
+	b, storage := createBackendWithStorage(t)
+
+	// Create a role
+	createRole(t, b, storage, "role1", "a,b,c")
+
+	// Create a secret-id
+	roleSecretIDReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "role/role1/secret-id",
+		Storage:   storage,
+	}
+	resp, err = b.HandleRequest(context.Background(), roleSecretIDReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+
+	accessorHashes, err := storage.List(context.Background(), "accessor/")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(accessorHashes) != 1 {
+		t.Fatalf("bad: len(accessorHashes); expect 1, got %d", len(accessorHashes))
+	}
+
+	entry1, err := logical.StorageEntryJSON(
+		"accessor/invalid1",
+		&secretIDAccessorStorageEntry{
+			SecretIDHMAC: "samplesecretidhmac",
+		},
+	)
+	err = storage.Put(context.Background(), entry1)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	entry2, err := logical.StorageEntryJSON(
+		"accessor/invalid2",
+		&secretIDAccessorStorageEntry{
+			SecretIDHMAC: "samplesecretidhmac2",
+		},
+	)
+	err = storage.Put(context.Background(), entry2)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	accessorHashes, err = storage.List(context.Background(), "accessor/")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(accessorHashes) != 3 {
+		t.Fatalf("bad: len(accessorHashes); expect 3, got %d", len(accessorHashes))
+	}
+
+	err = b.tidySecretID(context.Background(), storage)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	accessorHashes, err = storage.List(context.Background(), "accessor/")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(accessorHashes) != 1 {
+		t.Fatalf("bad: len(accessorHashes); expect 1, got %d", len(accessorHashes))
+	}
+}