[pve-devel] [RFC access-control common container qemu-server 0/4] #2582: Sys.Root privilege

[Fabian Grünbichler]

On January 5, 2022 4:22 pm, Oguz Bektas wrote:
> sending this in as RFC, because i think it needs a bit of ironing
> out and discussing the quirky bits ;)

thanks for picking up the work on this seemingly-open-forever bug ;) 

summarizing the previous discussions would be helpful for a 
cover-letter:
- why do we want/need/... this? (i.e., what problem(s) does this solve?)
- other possible approaches besides a new privilege (and why prefer a 
  priv over them?)
- clear semantics of the new privilege (e.g., below about whether it 
  implies all other privs or not)

> i'm not done with pve-manager patch so that'll come with the v1, though API
> should work for testing the changes.

besides pve-manager, there are also possible call-sites in 
pve-access-control and pve-cluster - did you check those?

> 
> it probably makes sense to also add a helper there, since at the moment
> we only check if Proxmox.UserName === 'root at pam' or in some cases
> specific permissions for storage and so forth, in order to decide
> whether to show/enable some GUI elements.
> 
> container API already works pretty well. VM API should also work but i haven't
> tested this extensively w.r.t. storage and migration.
> 
> some questions that popped up in my head:
> 
> + should adding 'Sys.Root' privilege to a user give them all available
> privileges on that path? this would make sense as having a
> root-equivalent privilege should be already enough (otherwise it doesn't
> have much of a point?). since at some places we have things like:

hmm, this is a tricky question and probably one that we should document 
explicitly. given that Sys.Root implies that you are allowed to do 
dangerous stuff that easily allows you to obtain root-level access, 
using the existing "'root at pam' has all privileges" as "'Sys.root' 
implies all other privileges on current path" probably makes sense and 
simplifies the logic considerably.

> 
> -------
>         } elsif ($target_vmid) {
>             $rpcenv->check_vm_perm($authuser, $target_vmid, undef, ['VM.Config.Disk'])
> -               if $authuser ne 'root at pam';
> +               if !$is_root;

this is just a short-cut to avoid the (possibly expensive) check, as the 
check cannot ever fail for root at pam by definition. I think these could 
just stay as checking for the user - if we include 'Sys.Root' the 
short-cut is no longer cheap (and obviously, if 'Sys.Root' does not 
imply having all the other privs, changing it would be wrong as well).

> -------
> 
> where one could theoretically have root-eq privs but not the 'VM.Config.Disk' for the target vm...
> 
> 
> + $authuser could also be an (optional?) parameter for the helper in
> PVE::Tools, so that we could check arbitrary users and not only the current
> one. also on most of these we already call $rpcenv->get_user() before doing the
> check, so we could spare that call inside the helper if we do that
> consistently.

see review for that patch

> 
> + would it make sense to be able to give 'Sys.Root' on a single node (like on
> /nodes/foo instead of the whole cluster)? this seemed like a rabbithole to me
> since we'd have to lock down quite a bit of stuff to limit movement from one
> cluster member to the other, without any(?) worthwhile benefits? or might make
> sense to just allow 'Sys.Root' to be given on '/' (since it should be
> equivalent to root at pam anyway)

like you said, this is a totally different can of worms and orthogonal 
to introducing this privilege. there are some areas where adding new ACL 
namespaces and paths might make sense (like nodes, but also things like 
the firewall, networks/.., ..), but those require really careful 
evaluation as changing stuff afterwards is quite involved.

this series might be a good way to go through all the places where we 
don't have real ACLs at the moment though and categorize them:

- local HW access
- 'forcing' stuff (skiplock et al)
- ...

to determine areas where we can improve without the root at pam/Sys.Root 
cludge (by introducing new abstractions / ACL namespaces / privileges).

> 
> + should root at pam have 'Sys.Root' by default? or does it make sense to still
> differentiate the "real" root user and the "impersonated" one?

see review for helper patch and comments above ;)

> 
>  pve-access-control:
> 
>  Oguz Bektas (1):
>   add Sys.Root privilege
> 
>  src/PVE/AccessControl.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
>  pve-common:
> 
>  Oguz Bektas (1):
>   tools: add 'check_for_root' helper
> 
>  src/PVE/Tools.pm | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
>  pve-container:
> 
>  Oguz Bektas (1):
>   fix #2582: api: use common helper for checking root privileges
> 
>  src/PVE/API2/LXC.pm | 5 ++---
>  src/PVE/LXC.pm      | 8 +++++---
>  2 files changed, 7 insertions(+), 6 deletions(-)
> 
>  qemu-server:
> 
>  Oguz Bektas (1):
>   api: use common helper for checking root privileges
> 
>  PVE/API2/Qemu.pm | 74 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 49 insertions(+), 25 deletions(-)
> 
> 
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>