[pve-devel] applied: [RFC access-control] API/ticket: rework coarse grained permission computation

[Wolfgang Bumiller]

applied

On Thu, Sep 14, 2017 at 03:17:09PM +0200, Thomas Lamprecht wrote:
> We accessed methods from PVE::Storage here but did not define a
> "use PVE::Storage". This thus only worked if modules if the
> PVE::Storage module got pulled in by something else, by luck.
> Simply including said use statement is not an option because
> pve-storage is already dependent from pve-access-control, and we want
> to avoid cyclic dependencies, especially on the perl module level.
> 
> The reason the offending module was used in the first place here
> stems from the way how this coarse grained permissions are
> calculated.
> We check all permission object paths for privileges for an user.
> So we got all vmids and all storage ids and computed paths from them.
> This works, but is overkill and led to this "illegal" module use.
> 
> Instead I opt to not generating all possible paths, but just check
> the ones configured plus a small required static set of top level
> paths - this allows to generalize handling of the special root at pam
> and "normal" users.

It may be easier to summarize the function's behavior this way:
For each _type_ of object (vms, storage, nodes, ...) get a list of
_types_ of permissions which exist for the user at all and mark it in a
hash.
  - If you have permission X on *any* VM, $res->{vms}->{X} will be 1.
  - If you have permission Y on *any* storage, $res->{storage}->{Y} will
    be 1.

So indeed, going through the actual ACL entries and matching their patha
gainst the user and using their toplevel path entry for categorization
makes more sense, especially given that the actual vmids, storages etc.
never make it into the result anyway.

> It has to be noted that this method is in general just intended for a
> coarse capability check to allow hiding a few UI elements which are
> not generated by backend calls (which are already permission aware).
> The real checks get done by each backend call, automatically for
> simple ones and semi-automatically for complex ones.

Basically provides a list of things you can do "*some*where" to the UI
to hide elements you would never be able to use anyway.
So all that matters is that we don't return too little.

> ---
> 
> It could make sense to move this out from API to AccessControl, make
> it static and add tests for it?
> Because of the missing use its a bit harder to do so previous to
> applying this patch, sadly...

That definitely sounds useful.

> 
>  PVE/API2/AccessControl.pm | 99 +++++++++++++++++------------------------------
>  1 file changed, 36 insertions(+), 63 deletions(-)
> 
> diff --git a/PVE/API2/AccessControl.pm b/PVE/API2/AccessControl.pm
> index 5d59d9f..318ee15 100644
> --- a/PVE/API2/AccessControl.pm
> +++ b/PVE/API2/AccessControl.pm
> @@ -136,73 +136,46 @@ my $compute_api_permission = sub {
>  
>      my $usercfg = $rpcenv->{user_cfg};
>  
> -    my $nodelist = PVE::Cluster::get_nodelist();
> -    my $vmlist = PVE::Cluster::get_vmlist() || {};
> -    my $idlist = $vmlist->{ids} || {};
> -
> -    my $cfg = PVE::Storage::config();
> -    my @sids =  PVE::Storage::storage_ids ($cfg);
> -
> -    my $res = {
> -	vms => {},
> -	storage => {},
> -	access => {},
> -	nodes => {},
> -	dc => {},
> +    my $res = {};
> +    my $priv_re_map = {
> +	vms => qr/VM\.|Permissions\.Modify/,
> +	access => qr/(User|Group)\.|Permissions\.Modify/,
> +	storage => qr/Datastore\./,
> +	nodes => qr/Sys\.|Permissions\.Modify/,
> +	dc => qr/Sys\.Audit/,
>      };
> -
> -    my $extract_vm_caps = sub {
> -	my ($path) = @_;
> -	
> -	my $perm = $rpcenv->permissions($authuser, $path);
> -	foreach my $priv (keys %$perm) {
> -	    next if !($priv eq 'Permissions.Modify' || $priv =~ m/^VM\./);
> -	    $res->{vms}->{$priv} = 1;	
> -	}
> -    };
> -
> -    foreach my $pool (keys %{$usercfg->{pools}}) {
> -	&$extract_vm_caps("/pool/$pool");
> -    }
> -
> -    foreach my $vmid (keys %$idlist, '__phantom__') {
> -	&$extract_vm_caps("/vms/$vmid");
> -    }
> -
> -    foreach my $storeid (@sids, '__phantom__') {
> -	my $perm = $rpcenv->permissions($authuser, "/storage/$storeid");
> -	foreach my $priv (keys %$perm) {
> -	    next if !($priv eq 'Permissions.Modify' || $priv =~ m/^Datastore\./);
> -	    $res->{storage}->{$priv} = 1;
> -	}
> -    }
> -
> -    foreach my $path (('/access/groups')) {
> -	my $perm = $rpcenv->permissions($authuser, $path);
> -	foreach my $priv (keys %$perm) {
> -	    next if $priv !~ m/^(User|Group)\./;
> -	    $res->{access}->{$priv} = 1;
> -	}
> -    }
> -
> -    foreach my $group (keys %{$usercfg->{users}->{$authuser}->{groups}}, '__phantom__') {
> -	my $perm = $rpcenv->permissions($authuser, "/access/groups/$group");
> -	if ($perm->{'User.Modify'}) {
> -	    $res->{access}->{'User.Modify'} = 1;
> -	}
> -    }
> -
> -    foreach my $node (@$nodelist) {
> -	my $perm = $rpcenv->permissions($authuser, "/nodes/$node");
> -	foreach my $priv (keys %$perm) {
> -	    next if $priv !~ m/^Sys\./;
> -	    $res->{nodes}->{$priv} = 1;
> +    map { $res->{$_} = {} } keys %$priv_re_map;
> +
> +    my $required_paths = ['/', '/nodes', '/access/groups', '/vms', '/storage'];
> +
> +    my $checked_paths = {};
> +    foreach my $path (@$required_paths, keys %{$usercfg->{acl}}) {
> +	next if $checked_paths->{$path};
> +	$checked_paths->{$path} = 1;
> +
> +	my $path_perm = $rpcenv->permissions($authuser, $path);
> +
> +	my $toplevel = ($path =~ /^\/(\w+)/) ? $1 : 'dc';
> +	if ($toplevel eq 'pool') {
> +	    foreach my $priv (keys %$path_perm) {
> +		if ($priv =~ m/^VM\./) {
> +		    $res->{vms}->{$priv} = 1;
> +		} elsif ($priv =~ m/^Datastore\./) {
> +		    $res->{storage}->{$priv} = 1;
> +		} elsif ($priv eq 'Permissions.Modify') {
> +		    $res->{storage}->{$priv} = 1;
> +		    $res->{vms}->{$priv} = 1;
> +		}
> +	    }
> +	} else {
> +	    my $priv_regex = $priv_re_map->{$toplevel} // next;
> +	    foreach my $priv (keys %$path_perm) {
> +		next if $priv !~ m/^($priv_regex)/;
> +		$res->{$toplevel}->{$priv} = 1;
> +	    }
>  	}
>      }
>  
> -    my $perm = $rpcenv->permissions($authuser, "/");
> -    $res->{dc}->{'Sys.Audit'} = 1 if $perm->{'Sys.Audit'};
> -
>      return $res;
>  };
>  
> -- 
> 2.11.0