From dd39b177f9c96042091edfa6912fc8196b32adfa Mon Sep 17 00:00:00 2001
From: davidadeleon <56207066+davidadeleon@users.noreply.github.com>
Date: Fri, 24 Feb 2023 13:18:08 -0500
Subject: [PATCH] add nil check for secret id entry on delete via accessor
 (#19186)

* add nil check for secret id entry on delete via accessor

* add changelog

* add godoc to test

* improve feedback on nil entry

* fix error reporting on invalid secret id accessor

* fix test to expect implemented error
---
 builtin/credential/approle/path_role.go      | 13 ++++-
 builtin/credential/approle/path_role_test.go | 56 ++++++++++++++++++++
 changelog/19186.txt                          |  3 ++
 3 files changed, 70 insertions(+), 2 deletions(-)
 create mode 100644 changelog/19186.txt

diff --git a/builtin/credential/approle/path_role.go b/builtin/credential/approle/path_role.go
index 35e3bfbbb..7759c0703 100644
--- a/builtin/credential/approle/path_role.go
+++ b/builtin/credential/approle/path_role.go
@@ -1964,12 +1964,21 @@ func (b *backend) pathRoleSecretIDAccessorDestroyUpdateDelete(ctx context.Contex
 		return nil, fmt.Errorf("failed to create HMAC of role_name: %w", err)
 	}
 
-	entryIndex := fmt.Sprintf("%s%s/%s", role.SecretIDPrefix, roleNameHMAC, accessorEntry.SecretIDHMAC)
-
 	lock := b.secretIDLock(accessorEntry.SecretIDHMAC)
 	lock.Lock()
 	defer lock.Unlock()
 
+	// Verify we have a valid SecretID Storage Entry
+	entry, err := b.nonLockedSecretIDStorageEntry(ctx, req.Storage, role.SecretIDPrefix, roleNameHMAC, accessorEntry.SecretIDHMAC)
+	if err != nil {
+		return nil, err
+	}
+	if entry == nil {
+		return logical.ErrorResponse("invalid secret id accessor"), logical.ErrPermissionDenied
+	}
+
+	entryIndex := fmt.Sprintf("%s%s/%s", role.SecretIDPrefix, roleNameHMAC, accessorEntry.SecretIDHMAC)
+
 	// Delete the accessor of the SecretID first
 	if err := b.deleteSecretIDAccessorEntry(ctx, req.Storage, secretIDAccessor, role.SecretIDPrefix); err != nil {
 		return nil, err
diff --git a/builtin/credential/approle/path_role_test.go b/builtin/credential/approle/path_role_test.go
index d42a0e1c7..7957a6997 100644
--- a/builtin/credential/approle/path_role_test.go
+++ b/builtin/credential/approle/path_role_test.go
@@ -2073,3 +2073,59 @@ func TestAppRole_SecretID_WithTTL(t *testing.T) {
 		})
 	}
 }
+
+// TestAppRole_RoleSecretIDAccessorCrossDelete tests deleting a secret id via
+// secret id accessor belonging to a different role
+func TestAppRole_RoleSecretIDAccessorCrossDelete(t *testing.T) {
+	var resp *logical.Response
+	var err error
+	b, storage := createBackendWithStorage(t)
+
+	// Create First Role
+	createRole(t, b, storage, "role1", "a,b")
+	_ = b.requestNoErr(t, &logical.Request{
+		Operation: logical.UpdateOperation,
+		Storage:   storage,
+		Path:      "role/role1/secret-id",
+	})
+
+	// Create Second Role
+	createRole(t, b, storage, "role2", "a,b")
+	_ = b.requestNoErr(t, &logical.Request{
+		Operation: logical.UpdateOperation,
+		Storage:   storage,
+		Path:      "role/role2/secret-id",
+	})
+
+	// Get role2 secretID Accessor
+	resp = b.requestNoErr(t, &logical.Request{
+		Operation: logical.ListOperation,
+		Storage:   storage,
+		Path:      "role/role2/secret-id",
+	})
+
+	// Read back role2 secretID Accessor information
+	hmacSecretID := resp.Data["keys"].([]string)[0]
+	_ = b.requestNoErr(t, &logical.Request{
+		Operation: logical.UpdateOperation,
+		Storage:   storage,
+		Path:      "role/role2/secret-id-accessor/lookup",
+		Data: map[string]interface{}{
+			"secret_id_accessor": hmacSecretID,
+		},
+	})
+
+	// Attempt to destroy role2 secretID accessor using role1 path
+	_, err = b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Storage:   storage,
+		Path:      "role/role1/secret-id-accessor/destroy",
+		Data: map[string]interface{}{
+			"secret_id_accessor": hmacSecretID,
+		},
+	})
+
+	if err == nil {
+		t.Fatalf("expected error")
+	}
+}
diff --git a/changelog/19186.txt b/changelog/19186.txt
new file mode 100644
index 000000000..cb3b59a9f
--- /dev/null
+++ b/changelog/19186.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+auth/approle: Add nil check for the secret ID entry when deleting via secret id accessor preventing cross role secret id deletion
+```