* [pve-devel] [PATCH access-control] api: role: remove role references from acl rules on role deletion
@ 2024-12-04 15:11 Daniel Kral
2025-02-03 11:49 ` Fiona Ebner
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Kral @ 2024-12-04 15:11 UTC (permalink / raw)
To: pve-devel
Let the API endpoint `DELETE /access/roles/{roleid}` or command
`pveum role delete <roleid>` remove any ACL rules in the user
configuration, which reference the removed role.
Before this change, the removal of a role has caused the role to remain
in existing ACL rules, which referenced the removed role. Therefore, on
each parse of the user configuration, a warning was be displayed:
user config - ignore invalid acl role '<role>'
Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
src/PVE/API2/Role.pm | 2 +-
src/PVE/AccessControl.pm | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/PVE/API2/Role.pm b/src/PVE/API2/Role.pm
index a924018..6e75a3c 100644
--- a/src/PVE/API2/Role.pm
+++ b/src/PVE/API2/Role.pm
@@ -212,7 +212,7 @@ __PACKAGE__->register_method ({
delete ($usercfg->{roles}->{$role});
- # fixme: delete role from acl?
+ PVE::AccessControl::delete_role_acl($role, $usercfg);
cfs_write_file("user.cfg", $usercfg);
}, "delete role failed");
diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 47f2d38..4bbbe80 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1022,6 +1022,29 @@ sub delete_group_acl {
iterate_acl_tree("/", $usercfg->{acl_root}, $code);
}
+sub delete_role_acl_for_each {
+ my ($role, $acl_subjects) = @_;
+
+ for my $subject (sort keys %$acl_subjects) {
+ delete ($acl_subjects->{$subject}->{$role})
+ if $acl_subjects->{$subject}->{$role};
+ }
+}
+
+sub delete_role_acl {
+ my ($role, $usercfg) = @_;
+
+ my $code = sub {
+ my ($path, $acl_node) = @_;
+
+ delete_role_acl_for_each($role, $acl_node->{groups});
+ delete_role_acl_for_each($role, $acl_node->{users});
+ delete_role_acl_for_each($role, $acl_node->{tokens});
+ };
+
+ iterate_acl_tree("/", $usercfg->{acl_root}, $code);
+}
+
sub delete_pool_acl {
my ($pool, $usercfg) = @_;
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH access-control] api: role: remove role references from acl rules on role deletion
2024-12-04 15:11 [pve-devel] [PATCH access-control] api: role: remove role references from acl rules on role deletion Daniel Kral
@ 2025-02-03 11:49 ` Fiona Ebner
2025-02-05 9:21 ` Daniel Kral
0 siblings, 1 reply; 4+ messages in thread
From: Fiona Ebner @ 2025-02-03 11:49 UTC (permalink / raw)
To: Proxmox VE development discussion, Daniel Kral
Am 04.12.24 um 16:11 schrieb Daniel Kral:
> Let the API endpoint `DELETE /access/roles/{roleid}` or command
> `pveum role delete <roleid>` remove any ACL rules in the user
> configuration, which reference the removed role.
>
> Before this change, the removal of a role has caused the role to remain
> in existing ACL rules, which referenced the removed role. Therefore, on
> each parse of the user configuration, a warning was be displayed:
>
> user config - ignore invalid acl role '<role>'
>
Might be good to note that the next modification of the configuration
would drop the unknown role (even if a role with the same name is
re-added right away).
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
Just a minor nit below, otherwise:
Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
Tested-by: Fiona Ebner <f.ebner@proxmox.com>
What would be really nice is to have some tests for various
add/modify/delete sequences touching user.cfg :) I don't think current
tests cover that yet.
> ---
> src/PVE/API2/Role.pm | 2 +-
> src/PVE/AccessControl.pm | 23 +++++++++++++++++++++++
> 2 files changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/src/PVE/API2/Role.pm b/src/PVE/API2/Role.pm
> index a924018..6e75a3c 100644
> --- a/src/PVE/API2/Role.pm
> +++ b/src/PVE/API2/Role.pm
> @@ -212,7 +212,7 @@ __PACKAGE__->register_method ({
>
> delete ($usercfg->{roles}->{$role});
>
> - # fixme: delete role from acl?
> + PVE::AccessControl::delete_role_acl($role, $usercfg);
>
> cfs_write_file("user.cfg", $usercfg);
> }, "delete role failed");
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index 47f2d38..4bbbe80 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -1022,6 +1022,29 @@ sub delete_group_acl {
> iterate_acl_tree("/", $usercfg->{acl_root}, $code);
> }
>
> +sub delete_role_acl_for_each {
Nit: could be a private "my sub"
> + my ($role, $acl_subjects) = @_;
> +
> + for my $subject (sort keys %$acl_subjects) {
> + delete ($acl_subjects->{$subject}->{$role})
> + if $acl_subjects->{$subject}->{$role};
> + }
> +}
> +
> +sub delete_role_acl {
> + my ($role, $usercfg) = @_;
> +
> + my $code = sub {
> + my ($path, $acl_node) = @_;
> +
> + delete_role_acl_for_each($role, $acl_node->{groups});
> + delete_role_acl_for_each($role, $acl_node->{users});
> + delete_role_acl_for_each($role, $acl_node->{tokens});
> + };
> +
> + iterate_acl_tree("/", $usercfg->{acl_root}, $code);
> +}
> +
> sub delete_pool_acl {
> my ($pool, $usercfg) = @_;
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH access-control] api: role: remove role references from acl rules on role deletion
2025-02-03 11:49 ` Fiona Ebner
@ 2025-02-05 9:21 ` Daniel Kral
2025-02-05 10:00 ` Fiona Ebner
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Kral @ 2025-02-05 9:21 UTC (permalink / raw)
To: Fiona Ebner, Proxmox VE development discussion
On 2/3/25 12:49, Fiona Ebner wrote:
> Am 04.12.24 um 16:11 schrieb Daniel Kral:
>> Let the API endpoint `DELETE /access/roles/{roleid}` or command
>> `pveum role delete <roleid>` remove any ACL rules in the user
>> configuration, which reference the removed role.
>>
>> Before this change, the removal of a role has caused the role to remain
>> in existing ACL rules, which referenced the removed role. Therefore, on
>> each parse of the user configuration, a warning was be displayed:
>>
>> user config - ignore invalid acl role '<role>'
>>
>
> Might be good to note that the next modification of the configuration
> would drop the unknown role (even if a role with the same name is
> re-added right away).
Thanks, will mention that in the v2!
Just for clarification, what could be an/the use case of deleting and
re-adding the role? It could be certainly beneficial to add a small
reminder in the WebUI, that removing a user/group/role will also delete
its dependents.
On 2/3/25 12:49, Fiona Ebner wrote:
> What would be really nice is to have some tests for various
> add/modify/delete sequences touching user.cfg :) I don't think current
> tests cover that yet.
I'll gladly provide these with a v2 to document the changes and also
just enforce this behavior in the future :).
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [pve-devel] [PATCH access-control] api: role: remove role references from acl rules on role deletion
2025-02-05 9:21 ` Daniel Kral
@ 2025-02-05 10:00 ` Fiona Ebner
0 siblings, 0 replies; 4+ messages in thread
From: Fiona Ebner @ 2025-02-05 10:00 UTC (permalink / raw)
To: Daniel Kral, Proxmox VE development discussion
Am 05.02.25 um 10:21 schrieb Daniel Kral:
> On 2/3/25 12:49, Fiona Ebner wrote:
>> Am 04.12.24 um 16:11 schrieb Daniel Kral:
>>> Let the API endpoint `DELETE /access/roles/{roleid}` or command
>>> `pveum role delete <roleid>` remove any ACL rules in the user
>>> configuration, which reference the removed role.
>>>
>>> Before this change, the removal of a role has caused the role to remain
>>> in existing ACL rules, which referenced the removed role. Therefore, on
>>> each parse of the user configuration, a warning was be displayed:
>>>
>>> user config - ignore invalid acl role '<role>'
>>>
>>
>> Might be good to note that the next modification of the configuration
>> would drop the unknown role (even if a role with the same name is
>> re-added right away).
>
> Thanks, will mention that in the v2!
>
> Just for clarification, what could be an/the use case of deleting and
> re-adding the role? It could be certainly beneficial to add a small
> reminder in the WebUI, that removing a user/group/role will also delete
> its dependents.
Could happen by accident, or could just be the want to use a new role
with the same name for something (slightly) different. But I mentioned
this, because one could suspect that re-adding right away could be a
scenario where the left-overs from the deleted role are not dropped. And
a new role starting out with ACLs from a previous one would be
surprising and have security-critical implications. It's not the case
however, the left-overs are dropped even then.
Still, if you ever suspect you came across something with security
implications, best to contact a member of the security team, or you can
also just use the standard channels:
https://pve.proxmox.com/wiki/Security_Reporting )
>
> On 2/3/25 12:49, Fiona Ebner wrote:
>> What would be really nice is to have some tests for various
>> add/modify/delete sequences touching user.cfg :) I don't think current
>> tests cover that yet.
>
> I'll gladly provide these with a v2 to document the changes and also
> just enforce this behavior in the future :).
Great!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-02-05 10:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-04 15:11 [pve-devel] [PATCH access-control] api: role: remove role references from acl rules on role deletion Daniel Kral
2025-02-03 11:49 ` Fiona Ebner
2025-02-05 9:21 ` Daniel Kral
2025-02-05 10:00 ` Fiona Ebner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal