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

@@ -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

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")
+	}
+}

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
+```