Re: [pve-devel] [PATCH v4 access-control++ 00/18] SuperUser privilege

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On June 2, 2022 9:24 am, Oguz Bektas wrote:
> big thanks to Fabian G. for the earlier reviews :)
> 
> v3 was not reviewed but i thought i should rebase it to make it easier.
> i also noticed some things that weren't addressed or were
> incorrect, so those are hopefully fixed now.

sorry for the long delay!

> please note that the privilege columns of the role selector in widget-toolkit
> is broken (reverting commit 49275c6726e7bf40f6d79e7b62eb4ad490a75119 fixes
> that, the custom renderer broke the view but i wasn't able to come up with a
> fix besides that -- extjs is pretty weird)

is this fixed nowadays?

> 
> v3->v4
> * rebased to latest commits
> * fix incorrect check for login prompt in pve-manager for termproxy and friends
> * slightly changed warning message when checking propagate
> * show warning to user via raise_perm_exc() when changing passwords
> * mark shortcut in parse_backup_hints
> 
> 
> v2->v3:
> * fixed some incorrect checks in qemu-server
> * further restrict changing passwords and tfa settings for superusers
> * gui fix for lxc features
> * slight improvements to docs, added some notes
> 
> see the separate patches for further details :)
> 
> intentionally left out ceph and cluster-related endpoints for checking SU
> privileges:
> 
> * addnode
> * copy
> * create
> * delnode
> * index
> * join
> 

I can guess that addnode,delnode,create and join are the respective 
cluster endpoints (although for them the full API path or module would 
obviously make sense as well!)

copy and index could be anything?

> * destroyosd
> * createosd
> 

couldn't these also move to Sys.Modify like the rest of disk-management?

> and some other endpoints:
> * register_account
> * deactivate_account
> 

these are ACME I guess? why would they need to be SU only?

> i left these endpoints alone since we have plans to introduce separate
> privileges for cluster-related actions in the future.

some indication which parts you actively tested would be nice as well.

please don't forget to re-check the API tree and other code for any 
root@pam-only stuff that might have been added in the meantime!

>  access-control: Oguz Bektas (5):
>   add "SuperAdministrator" role with the new "SuperUser" privilege
>   RPC env: add SuperUser API permission for GUI capabilities
>   api: acl: only allow granting SU privilege if user already has it
>   api: roles: only allow modifying roles to add/remove SU if user has SU
>     themselves
>   api: allow superusers to edit tfa and password settings
> 
>  src/PVE/API2/ACL.pm           | 16 ++++++++++++++++
>  src/PVE/API2/AccessControl.pm | 24 ++++++++++++++----------
>  src/PVE/API2/Role.pm          | 21 +++++++++++++++++++++
>  src/PVE/API2/TFA.pm           | 16 +++++++++++++++-
>  src/PVE/AccessControl.pm      |  9 ++++++---
>  src/PVE/RPCEnvironment.pm     | 29 ++++++++++++++++++++++-------
>  6 files changed, 94 insertions(+), 21 deletions(-)
> 
>  qemu-server: Oguz Bektas (4):
>   api: allow SU privileged users to edit root-only options for VM
>     configs
>   migration tests: mock $rpcenv->check subroutine
>   api: allow superusers to use 'skiplock' option
>   parse_backup_hints: add comment for root shortcut and fix typos
> 
>  PVE/API2/Qemu.pm             | 133 +++++++++++++++++++++--------------
>  PVE/QemuServer.pm            |   6 +-
>  test/MigrationTest/QmMock.pm |   5 ++
>  3 files changed, 87 insertions(+), 57 deletions(-)
> 
>  manager: Oguz Bektas (6):
>   api: backup: allow SUs to use 'tmpdir', 'dumpdir' and 'script' options
>   api: vzdump: allow SUs to use 'bwlimit' and 'ionice' parameters
>   api: always drop to login prompt for non-root users on terminal proxy
>     calls
>   ui: include "SuperUser" in privilege selector
>   ui: lxc features: check for SU instead of 'root@pam'
>   ui: adapt sensible 'root@pam' checks to SU
> 
>  PVE/API2/Backup.pm                      | 11 +++++++----
>  PVE/API2/Nodes.pm                       | 11 ++++++++---
>  PVE/API2/VZDump.pm                      |  8 +++++---
>  www/manager6/form/PrivilegesSelector.js |  2 +-
>  www/manager6/lxc/Options.js             |  8 ++++++--
>  www/manager6/lxc/Resources.js           |  6 +++---
>  www/manager6/window/Migrate.js          |  4 ++--
>  7 files changed, 32 insertions(+), 18 deletions(-)
> 
>  container: Oguz Bektas (1):
>   fix #2582: api: add checks for 'SuperUser' privilege for root-only
>     options
> 
>  src/PVE/API2/LXC.pm        | 19 +++++++++----------
>  src/PVE/API2/LXC/Config.pm |  2 +-
>  src/PVE/API2/LXC/Status.pm | 12 ++++++++----
>  src/PVE/LXC.pm             | 21 ++++++++++++---------
>  src/PVE/LXC/Create.pm      |  2 +-
>  5 files changed, 31 insertions(+), 25 deletions(-)
> 
>  storage: Oguz Bektas (1):
>   check_volume_access: allow superusers to pass arbitrary fs paths
> 
>  PVE/Storage.pm | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
>  docs: Oguz Bektas (1):
>   pveum: add SU privilege and SA role
> 
>  pveum.adoc | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>