roles: restrict Permissions.Modify to Administrator

to reduce the chances of accidentally handing out privilege modification
privileges. the old default setup of having Permissions.Modify in PVESysAdmin
and PVEAdmin weakened the distinction between those roles and Administrator.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index c1ade4e8fff8cc7155a7fd5cff58bc59ab533405..04dc7d9f8fef0d23c52fab5291d1009f165876d6 100644 (file)
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1060,7 +1060,6 @@ my $privgroups = {
            'Sys.Incoming', # incoming storage/guest migrations
        ],
        admin => [
-           'Permissions.Modify',
            'Sys.Console',
            'Sys.Syslog',
        ],
@@ -1118,7 +1117,9 @@ my $privgroups = {
     },
 };
 
-my $valid_privs = {};
+my $valid_privs = {
+    'Permissions.Modify' => 1, # not contained in a group
+};
 
 my $special_roles = {
     'NoAccess' => {}, # no privileges

diff --git a/src/test/perm-test1.pl b/src/test/perm-test1.pl
index d8527e7a125d3a3ac12bf3d3aa209a8132751e29..d7cab3d104aa2905d503e69eb23df9b848d99cc7 100755 (executable)
--- a/src/test/perm-test1.pl
+++ b/src/test/perm-test1.pl
@@ -58,6 +58,10 @@ check_permission('max@pve', '/vms/100', 'VM.Audit,VM.PowerMgmt');
 check_permission('alex@pve', '/vms', '');
 check_permission('alex@pve', '/vms/100', 'VM.Audit,VM.PowerMgmt');
 
+# PVEVMAdmin -> no Permissions.Modify!
+check_permission('alex@pve', '/vms/300', 'VM.Allocate,VM.Audit,VM.Backup,VM.Clone,VM.Config.CDROM,VM.Config.CPU,VM.Config.Cloudinit,VM.Config.Disk,VM.Config.HWType,VM.Config.Memory,VM.Config.Network,VM.Config.Options,VM.Console,VM.Migrate,VM.Monitor,VM.PowerMgmt,VM.Snapshot,VM.Snapshot.Rollback');
+# Administrator -> Permissions.Modify!
+check_permission('alex@pve', '/vms/400', 'Datastore.Allocate,Datastore.AllocateSpace,Datastore.AllocateTemplate,Datastore.Audit,Group.Allocate,Permissions.Modify,Pool.Allocate,Pool.Audit,Realm.Allocate,Realm.AllocateUser,SDN.Allocate,SDN.Audit,Sys.Audit,Sys.Console,Sys.Incoming,Sys.Modify,Sys.PowerMgmt,Sys.Syslog,User.Modify,VM.Allocate,VM.Audit,VM.Backup,VM.Clone,VM.Config.CDROM,VM.Config.CPU,VM.Config.Cloudinit,VM.Config.Disk,VM.Config.HWType,VM.Config.Memory,VM.Config.Network,VM.Config.Options,VM.Console,VM.Migrate,VM.Monitor,VM.PowerMgmt,VM.Snapshot,VM.Snapshot.Rollback');
 
 check_roles('max@pve', '/vms/200', 'storage_manager');
 check_roles('joe@pve', '/vms/200', 'vm_admin');

diff --git a/src/test/perm-test8.pl b/src/test/perm-test8.pl
index 980a990c25a225ebcaceeab6a9b8912a85c32113..21bf1d32e80c4a7032268ec03dc4bd703fb73d68 100644 (file)
--- a/src/test/perm-test8.pl
+++ b/src/test/perm-test8.pl
@@ -50,7 +50,7 @@ check_roles('max@pve', '/vms/100', 'customer');
 check_roles('max@pve', '/vms/101', 'vm_admin');
 
 check_permission('max@pve', '/', '');
-check_permission('max@pve', '/vms', 'Permissions.Modify,VM.Allocate,VM.Audit,VM.Console');
+check_permission('max@pve', '/vms', 'VM.Allocate,VM.Audit,VM.Console');
 check_permission('max@pve', '/vms/100', 'VM.Audit,VM.PowerMgmt');
 
 check_permission('alex@pve', '/vms', '');
@@ -66,7 +66,7 @@ check_roles('max@pve!token', '/vms/200', 'storage_manager');
 check_roles('max@pve!token2', '/vms/200', 'customer');
 
 # check intersection -> token has Administrator, but user only vm_admin
-check_permission('max@pve!token2', '/vms/300', 'Permissions.Modify,VM.Allocate,VM.Audit,VM.Console,VM.PowerMgmt');
+check_permission('max@pve!token2', '/vms/300', 'VM.Allocate,VM.Audit,VM.Console,VM.PowerMgmt');
 
 print "all tests passed\n";
 

diff --git a/src/test/test1.cfg b/src/test/test1.cfg
index d27c5d6dd18a5038262dee163ac82255079ac4f2..0b1b587858795f5e642ca87912a8651f80718de5 100644 (file)
--- a/src/test/test1.cfg
+++ b/src/test/test1.cfg
@@ -19,4 +19,6 @@ acl:1:/users:max@pve:Administrator:
 
 acl:1:/vms/200:@testgroup3:storage_manager:
 acl:1:/vms/200:@testgroup2:NoAccess:
+acl:1:/vms/300:alex@pve:PVEVMAdmin:
+acl:1:/vms/400:alex@pve:Administrator:
 

diff --git a/src/test/test8.cfg b/src/test/test8.cfg
index d5c7e86b0f15a6b978cbb6cacd1af5cd5ac3c416..ce704ef66c6a564ee905ecf481e22a7dbc67489f 100644 (file)
--- a/src/test/test8.cfg
+++ b/src/test/test8.cfg
@@ -13,7 +13,7 @@ group:testgroup3:max@pve:
 
 role:storage_manager:Datastore.AllocateSpace,Datastore.Audit:
 role:customer:VM.Audit,VM.PowerMgmt:
-role:vm_admin:VM.Audit,VM.Allocate,Permissions.Modify,VM.Console:
+role:vm_admin:VM.Audit,VM.Allocate,VM.Console:
 
 acl:1:/vms:@testgroup1:vm_admin:
 acl:0:/vms/300:max@pve:customer: