* [pve-devel] [PATCH access-control 1/2] fix #1148: allow up to three levels of pool nesting
2023-11-20 7:22 [pve-devel] [PATCH access-control/manager 0/4] fix #1148: nested pools Fabian Grünbichler
@ 2023-11-20 7:22 ` Fabian Grünbichler
2023-11-20 7:22 ` [pve-devel] [PATCH access-control 2/2] pools: record parent/subpool information Fabian Grünbichler
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2023-11-20 7:22 UTC (permalink / raw)
To: pve-devel
with ACLs being inherited along the pool hierarchy.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Notes:
v1: encode max pool nesting in REs
src/PVE/AccessControl.pm | 10 ++++++++--
src/test/perm-test6.pl | 16 ++++++++++++++++
src/test/test6.cfg | 5 +++++
3 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index 9600e59..4e3b077 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1264,7 +1264,7 @@ sub check_path {
|/nodes
|/nodes/[[:alnum:]\.\-\_]+
|/pool
- |/pool/[[:alnum:]\.\-\_]+
+ |/pool/[A-Za-z0-9\.\-_]+(?:/[A-Za-z0-9\.\-_]+){0,2}
|/sdn
|/sdn/controllers
|/sdn/controllers/[[:alnum:]\_\-]+
@@ -1318,8 +1318,14 @@ PVE::JSONSchema::register_format('pve-poolid', \&verify_poolname);
sub verify_poolname {
my ($poolname, $noerr) = @_;
- if ($poolname !~ m/^[A-Za-z0-9\.\-_]+$/) {
+ if (split("/", $poolname) > 3) {
+ die "pool name '$poolname' nested too deeply (max levels = 3)\n" if !$noerr;
+ return undef;
+ }
+
+ # also adapt check_path above if changed!
+ if ($poolname !~ m!^[A-Za-z0-9\.\-_]+(?:/[A-Za-z0-9\.\-_]+){0,2}$!) {
die "pool name '$poolname' contains invalid characters\n" if !$noerr;
return undef;
diff --git a/src/test/perm-test6.pl b/src/test/perm-test6.pl
index 0b0d036..c2d40fc 100755
--- a/src/test/perm-test6.pl
+++ b/src/test/perm-test6.pl
@@ -75,6 +75,22 @@ check_roles('User4@pve', '/vms/500', '');
# with pool
check_permissions('User4@pve', '/vms/500', '');
+# without pool, checking no access on parent pool
+check_roles('intern@pve', '/vms/600', '');
+# once more, with VM in nested pool
+check_roles('intern@pve', '/vms/700', '');
+# with propagated ACL
+check_roles('User4@pve', '/vms/700', '');
+# with pool, checking no access on parent pool
+check_permissions('intern@pve', '/vms/600', '');
+# once more, with VM in nested pool
+check_permissions('intern@pve', '/vms/700', 'VM.Audit');
+# with propagated ACL
+check_permissions('User4@pve', '/vms/700', 'VM.Console');
+
+# check nested pool permissions
+check_roles('intern@pve', '/pool/marketing/interns', 'RoleINTERN');
+check_roles('User4@pve', '/pool/marketing/interns', 'RoleMARKETING');
check_permissions('User1@pve', '/vms/600', 'VM.Console');
check_permissions('User2@pve', '/vms/600', 'VM.Console');
diff --git a/src/test/test6.cfg b/src/test/test6.cfg
index 4986910..661f56a 100644
--- a/src/test/test6.cfg
+++ b/src/test/test6.cfg
@@ -2,16 +2,20 @@ user:User1@pve:1:
user:User2@pve:1:
user:User3@pve:1:
user:User4@pve:1:
+user:intern@pve:1:
group:DEVEL:User1@pve,User2@pve,User3@pve:
group:MARKETING:User1@pve,User4@pve:
+group:INTERNS:intern@pve:
role:RoleDEVEL:VM.PowerMgmt:
role:RoleMARKETING:VM.Console:
+role:RoleINTERN:VM.Audit:
role:RoleTEST1:VM.Console:
acl:1:/pool/devel:@DEVEL:RoleDEVEL:
acl:1:/pool/marketing:@MARKETING:RoleMARKETING:
+acl:1:/pool/marketing/interns:@INTERNS:RoleINTERN:
acl:1:/vms:@DEVEL:RoleTEST1:
acl:1:/vms:User3@pve:NoAccess:
@@ -19,3 +23,4 @@ acl:1:/vms/300:@MARKETING:RoleTEST1:
pool:devel:MITS development:500,501,502:store1 store2:
pool:marketing:MITS marketing:600:store1:
+pool:marketing/interns:MITS marketing intern:700:store3:
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] [PATCH manager 1/2] fix #1148: api: pools: support nested pools
2023-11-20 7:22 [pve-devel] [PATCH access-control/manager 0/4] fix #1148: nested pools Fabian Grünbichler
2023-11-20 7:22 ` [pve-devel] [PATCH access-control 1/2] fix #1148: allow up to three levels of pool nesting Fabian Grünbichler
2023-11-20 7:22 ` [pve-devel] [PATCH access-control 2/2] pools: record parent/subpool information Fabian Grünbichler
@ 2023-11-20 7:22 ` Fabian Grünbichler
2023-11-20 7:22 ` [pve-devel] [PATCH manager 2/2] ui: pools: switch to new API endpoints Fabian Grünbichler
2023-11-20 11:27 ` [pve-devel] applied-series: [PATCH access-control/manager 0/4] fix #1148: nested pools Wolfgang Bumiller
4 siblings, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2023-11-20 7:22 UTC (permalink / raw)
To: pve-devel
since poolid can now contain `/`, it's not possible to use it (properly) as
path parameter anymore.
accordingly:
- merge `read_pool` (`GET /pools/{poolid}`) into 'index' (`GET
/pools/?poolid={poolid}`) (requires clients to extract the only member of the returned array if they want to query an individual pool)
- move `update_pool` to `/pools`, deprecating the old variant with path parameter
- move `delete_pool` to `/pools`, deprecating the old variant with path parameter
- deprecate `read_pool` API endpoint
pool creation is blocked for nested pools where the parent does not already
exist. similarly, the checks for deletion are extended to block deletion if
sub-pools still exist.
the old API endpoints continue to work for non-nested pools. `pvesh ls /pools`
is semi-broken for nested pools, listing the entries, but no methods on them,
since they reference the old API. fixing this would require extending the REST
handling to support a new type of child reference.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Notes:
requires bumped pve-access-control
PVE/API2/Pool.pm | 243 +++++++++++++++++++++++++++++++++++------------
1 file changed, 184 insertions(+), 59 deletions(-)
diff --git a/PVE/API2/Pool.pm b/PVE/API2/Pool.pm
index 51ac71941..54e744558 100644
--- a/PVE/API2/Pool.pm
+++ b/PVE/API2/Pool.pm
@@ -20,14 +20,26 @@ __PACKAGE__->register_method ({
name => 'index',
path => '',
method => 'GET',
- description => "Pool index.",
+ description => "List pools or get pool configuration.",
permissions => {
- description => "List all pools where you have Pool.Audit permissions on /pool/<pool>.",
+ description => "List all pools where you have Pool.Audit permissions on /pool/<pool>, or the pool specific with {poolid}",
user => 'all',
},
parameters => {
additionalProperties => 0,
- properties => {},
+ properties => {
+ poolid => {
+ type => 'string',
+ format => 'pve-poolid',
+ optional => 1,
+ },
+ type => {
+ type => 'string',
+ enum => [ 'qemu', 'lxc', 'storage' ],
+ optional => 1,
+ requires => 'poolid',
+ },
+ },
},
returns => {
type => 'array',
@@ -35,6 +47,38 @@ __PACKAGE__->register_method ({
type => "object",
properties => {
poolid => { type => 'string' },
+ comment => {
+ type => 'string',
+ optional => 1,
+ },
+ members => {
+ type => 'array',
+ optional => 1,
+ items => {
+ type => "object",
+ additionalProperties => 1,
+ properties => {
+ type => {
+ type => 'string',
+ enum => [ 'qemu', 'lxc', 'openvz', 'storage' ],
+ },
+ id => {
+ type => 'string',
+ },
+ node => {
+ type => 'string',
+ },
+ vmid => {
+ type => 'integer',
+ optional => 1,
+ },
+ storage => {
+ type => 'string',
+ optional => 1,
+ },
+ },
+ },
+ },
},
},
links => [ { rel => 'child', href => "{poolid}" } ],
@@ -47,15 +91,63 @@ __PACKAGE__->register_method ({
my $usercfg = $rpcenv->{user_cfg};
-
my $res = [];
- for my $pool (sort keys %{$usercfg->{pools}}) {
- next if !$rpcenv->check($authuser, "/pool/$pool", [ 'Pool.Audit' ], 1);
+ if (my $poolid = $param->{poolid}) {
+ $rpcenv->check($authuser, "/pool/$poolid", [ 'Pool.Audit' ], 1);
- my $entry = { poolid => $pool };
- my $pool_config = $usercfg->{pools}->{$pool};
- $entry->{comment} = $pool_config->{comment} if defined($pool_config->{comment});
- push @$res, $entry;
+ my $vmlist = PVE::Cluster::get_vmlist() || {};
+ my $idlist = $vmlist->{ids} || {};
+
+ my $rrd = PVE::Cluster::rrd_dump();
+
+ my $pool_config = $usercfg->{pools}->{$poolid};
+
+ die "pool '$poolid' does not exist\n" if !$pool_config;
+
+ my $members = [];
+ for my $vmid (sort keys %{$pool_config->{vms}}) {
+ my $vmdata = $idlist->{$vmid};
+ next if !$vmdata || defined($param->{type}) && $param->{type} ne $vmdata->{type};
+ my $entry = PVE::API2Tools::extract_vm_stats($vmid, $vmdata, $rrd);
+ push @$members, $entry;
+ }
+
+ my $nodename = PVE::INotify::nodename();
+ my $cfg = PVE::Storage::config();
+ if (!defined($param->{type}) || $param->{type} eq 'storage') {
+ for my $storeid (sort keys %{$pool_config->{storage}}) {
+ my $scfg = PVE::Storage::storage_config ($cfg, $storeid, 1);
+ next if !$scfg;
+
+ my $storage_node = $nodename; # prefer local node
+ if ($scfg->{nodes} && !$scfg->{nodes}->{$storage_node}) {
+ for my $node (sort keys(%{$scfg->{nodes}})) {
+ $storage_node = $node;
+ last;
+ }
+ }
+
+ my $entry = PVE::API2Tools::extract_storage_stats($storeid, $scfg, $storage_node, $rrd);
+ push @$members, $entry;
+ }
+ }
+
+ my $pool_info = {
+ members => $members,
+ };
+ $pool_info->{comment} = $pool_config->{comment} if defined($pool_config->{comment});
+ $pool_info->{poolid} = $poolid;
+
+ push @$res, $pool_info;
+ } else {
+ for my $pool (sort keys %{$usercfg->{pools}}) {
+ next if !$rpcenv->check($authuser, "/pool/$pool", [ 'Pool.Audit' ], 1);
+
+ my $entry = { poolid => $pool };
+ my $pool_config = $usercfg->{pools}->{$pool};
+ $entry->{comment} = $pool_config->{comment} if defined($pool_config->{comment});
+ push @$res, $entry;
+ }
}
return $res;
@@ -92,6 +184,11 @@ __PACKAGE__->register_method ({
my $pool = $param->{poolid};
die "pool '$pool' already exists\n" if $usercfg->{pools}->{$pool};
+ if ($pool =~ m!^(.*)/[^/]+$!) {
+ my $parent = $1;
+ die "parent '$parent' of pool '$pool' does not exist\n"
+ if !defined($usercfg->{pools}->{$parent});
+ }
$usercfg->{pools}->{$pool} = {
vms => {},
@@ -107,7 +204,7 @@ __PACKAGE__->register_method ({
}});
__PACKAGE__->register_method ({
- name => 'update_pool',
+ name => 'update_pool_deprecated',
protected => 1,
path => '{poolid}',
method => 'PUT',
@@ -115,9 +212,56 @@ __PACKAGE__->register_method ({
description => "You also need the right to modify permissions on any object you add/delete.",
check => ['perm', '/pool/{poolid}', ['Pool.Allocate']],
},
- description => "Update pool data.",
+ description => "Update pool data (deprecated, no support for nested pools - use 'PUT /pools/?poolid={poolid}' instead).",
parameters => {
- additionalProperties => 0,
+ additionalProperties => 0,
+ properties => {
+ poolid => { type => 'string', format => 'pve-poolid' },
+ comment => { type => 'string', optional => 1 },
+ vms => {
+ description => 'List of guest VMIDs to add or remove from this pool.',
+ type => 'string', format => 'pve-vmid-list',
+ optional => 1,
+ },
+ storage => {
+ description => 'List of storage IDs to add or remove from this pool.',
+ type => 'string', format => 'pve-storage-id-list',
+ optional => 1,
+ },
+ 'allow-move' => {
+ description => 'Allow adding a guest even if already in another pool.'
+ .' The guest will be removed from its current pool and added to this one.',
+ type => 'boolean',
+ optional => 1,
+ default => 0,
+ },
+ delete => {
+ description => 'Remove the passed VMIDs and/or storage IDs instead of adding them.',
+ type => 'boolean',
+ optional => 1,
+ default => 0,
+ },
+ },
+ },
+ returns => { type => 'null' },
+ code => sub {
+ my ($param) = @_;
+
+ return __PACKAGE__->update_pool($param);
+ }});
+
+__PACKAGE__->register_method ({
+ name => 'update_pool',
+ protected => 1,
+ path => '',
+ method => 'PUT',
+ permissions => {
+ description => "You also need the right to modify permissions on any object you add/delete.",
+ check => ['perm', '/pool/{poolid}', ['Pool.Allocate']],
+ },
+ description => "Update pool.",
+ parameters => {
+ additionalProperties => 0,
properties => {
poolid => { type => 'string', format => 'pve-poolid' },
comment => { type => 'string', optional => 1 },
@@ -215,7 +359,7 @@ __PACKAGE__->register_method ({
permissions => {
check => ['perm', '/pool/{poolid}', ['Pool.Audit']],
},
- description => "Get pool configuration.",
+ description => "Get pool configuration (deprecated, no support for nested pools, use 'GET /pools/?poolid={poolid}').",
parameters => {
additionalProperties => 0,
properties => {
@@ -270,60 +414,38 @@ __PACKAGE__->register_method ({
code => sub {
my ($param) = @_;
- my $usercfg = cfs_read_file("user.cfg");
-
- my $vmlist = PVE::Cluster::get_vmlist() || {};
- my $idlist = $vmlist->{ids} || {};
-
- my $rrd = PVE::Cluster::rrd_dump();
-
- my $pool = $param->{poolid};
-
- my $pool_config = $usercfg->{pools}->{$pool};
-
- die "pool '$pool' does not exist\n" if !$pool_config;
-
- my $members = [];
- for my $vmid (sort keys %{$pool_config->{vms}}) {
- my $vmdata = $idlist->{$vmid};
- next if !$vmdata || defined($param->{type}) && $param->{type} ne $vmdata->{type};
- my $entry = PVE::API2Tools::extract_vm_stats($vmid, $vmdata, $rrd);
- push @$members, $entry;
- }
+ my $pool_info = __PACKAGE__->index($param);
+ return $pool_info->[0];
+ }});
- my $nodename = PVE::INotify::nodename();
- my $cfg = PVE::Storage::config();
- if (!defined($param->{type}) || $param->{type} eq 'storage') {
- for my $storeid (sort keys %{$pool_config->{storage}}) {
- my $scfg = PVE::Storage::storage_config ($cfg, $storeid, 1);
- next if !$scfg;
-
- my $storage_node = $nodename; # prefer local node
- if ($scfg->{nodes} && !$scfg->{nodes}->{$storage_node}) {
- for my $node (sort keys(%{$scfg->{nodes}})) {
- $storage_node = $node;
- last;
- }
- }
- my $entry = PVE::API2Tools::extract_storage_stats($storeid, $scfg, $storage_node, $rrd);
- push @$members, $entry;
- }
+__PACKAGE__->register_method ({
+ name => 'delete_pool_deprecated',
+ protected => 1,
+ path => '{poolid}',
+ method => 'DELETE',
+ permissions => {
+ description => "You can only delete empty pools (no members).",
+ check => ['perm', '/pool/{poolid}', ['Pool.Allocate']],
+ },
+ description => "Delete pool (deprecated, no support for nested pools, use 'DELETE /pools/?poolid={poolid}').",
+ parameters => {
+ additionalProperties => 0,
+ properties => {
+ poolid => { type => 'string', format => 'pve-poolid' },
}
+ },
+ returns => { type => 'null' },
+ code => sub {
+ my ($param) = @_;
- my $res = {
- members => $members,
- };
- $res->{comment} = $pool_config->{comment} if defined($pool_config->{comment});
-
- return $res;
+ return __PACKAGE__->delete_pool($param);
}});
-
__PACKAGE__->register_method ({
name => 'delete_pool',
protected => 1,
- path => '{poolid}',
+ path => '',
method => 'DELETE',
permissions => {
description => "You can only delete empty pools (no members).",
@@ -354,6 +476,9 @@ __PACKAGE__->register_method ({
my $pool_config = $usercfg->{pools}->{$pool};
die "pool '$pool' does not exist\n" if !$pool_config;
+ for my $subpool (sort keys %{$pool_config->{pools}}) {
+ die "pool '$pool' is not empty (contains pool '$subpool')\n";
+ }
for my $vmid (sort keys %{$pool_config->{vms}}) {
next if !$idlist->{$vmid}; # ignore destroyed guests
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread* [pve-devel] [PATCH manager 2/2] ui: pools: switch to new API endpoints
2023-11-20 7:22 [pve-devel] [PATCH access-control/manager 0/4] fix #1148: nested pools Fabian Grünbichler
` (2 preceding siblings ...)
2023-11-20 7:22 ` [pve-devel] [PATCH manager 1/2] fix #1148: api: pools: support nested pools Fabian Grünbichler
@ 2023-11-20 7:22 ` Fabian Grünbichler
2023-11-20 11:27 ` [pve-devel] applied-series: [PATCH access-control/manager 0/4] fix #1148: nested pools Wolfgang Bumiller
4 siblings, 0 replies; 6+ messages in thread
From: Fabian Grünbichler @ 2023-11-20 7:22 UTC (permalink / raw)
To: pve-devel
which support nested pools. mostly straight-forward, only pool deletion and the
members grid need some special attention.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Notes:
Thanks Dominik for pointing me at the 'root' part of the proxy :)
www/manager6/dc/PoolView.js | 3 +++
www/manager6/grid/PoolMembers.js | 14 ++++++++------
www/manager6/pool/StatusView.js | 2 +-
3 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/www/manager6/dc/PoolView.js b/www/manager6/dc/PoolView.js
index db97cbe72..741b2025b 100644
--- a/www/manager6/dc/PoolView.js
+++ b/www/manager6/dc/PoolView.js
@@ -31,6 +31,9 @@ Ext.define('PVE.dc.PoolView', {
callback: function() {
reload();
},
+ getUrl: function(rec) {
+ return '/pools/?poolid=' + rec.getId();
+ },
});
var run_editor = function() {
diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index 74950d80e..75f20cab1 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -17,8 +17,9 @@ Ext.define('PVE.pool.AddVM', {
throw "no pool specified";
}
- me.url = "/pools/" + me.pool;
+ me.url = '/pools/';
me.method = 'PUT';
+ me.extraRequestParams.poolid = me.pool;
var vmsField = Ext.create('Ext.form.field.Text', {
name: 'vms',
@@ -120,8 +121,9 @@ Ext.define('PVE.pool.AddStorage', {
me.isCreate = true;
me.isAdd = true;
- me.url = "/pools/" + me.pool;
+ me.url = "/pools/";
me.method = 'PUT';
+ me.extraRequestParams.poolid = me.pool;
Ext.apply(me, {
subject: gettext('Storage'),
@@ -168,8 +170,8 @@ Ext.define('PVE.grid.PoolMembers', {
],
proxy: {
type: 'proxmox',
- root: 'data.members',
- url: "/api2/json/pools/" + me.pool,
+ root: 'data[0].members',
+ url: "/api2/json/pools/?poolid=" + me.pool,
},
});
@@ -192,7 +194,7 @@ Ext.define('PVE.grid.PoolMembers', {
"'" + rec.data.id + "'");
},
handler: function(btn, event, rec) {
- var params = { 'delete': 1 };
+ var params = { 'delete': 1, poolid: me.pool };
if (rec.data.type === 'storage') {
params.storage = rec.data.storage;
} else if (rec.data.type === 'qemu' || rec.data.type === 'lxc' || rec.data.type === 'openvz') {
@@ -202,7 +204,7 @@ Ext.define('PVE.grid.PoolMembers', {
}
Proxmox.Utils.API2Request({
- url: '/pools/' + me.pool,
+ url: '/pools/',
method: 'PUT',
params: params,
waitMsgTarget: me,
diff --git a/www/manager6/pool/StatusView.js b/www/manager6/pool/StatusView.js
index 302ae5ab0..3d46b3b1a 100644
--- a/www/manager6/pool/StatusView.js
+++ b/www/manager6/pool/StatusView.js
@@ -24,7 +24,7 @@ Ext.define('PVE.pool.StatusView', {
};
Ext.apply(me, {
- url: "/api2/json/pools/" + pool,
+ url: "/api2/json/pools/?poolid=" + pool,
rows: rows,
});
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread