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