Re: [pve-devel] [PATCH pve-access-control] fix #1500: permission path syntax check for access control

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 16.04.21 14:34, Lorenz Stechauner wrote:
> Syntax for permission paths is now checked on API calls for
> creation or update on permissions.
> 

great first patch, it seems functional FWICT, but I'd still change the way
we build the regex, see below.

> Signed-off-by: Lorenz Stechauner <l.stechauner@proxmox.com>
> ---
>  PVE/API2/ACL.pm      |  4 ++++
>  PVE/AccessControl.pm | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/PVE/API2/ACL.pm b/PVE/API2/ACL.pm
> index c340267..857c672 100644
> --- a/PVE/API2/ACL.pm
> +++ b/PVE/API2/ACL.pm
> @@ -141,6 +141,10 @@ __PACKAGE__->register_method ({
>  	my $path = PVE::AccessControl::normalize_path($param->{path});
>  	raise_param_exc({ path => "invalid ACL path '$param->{path}'" }) if !$path;
>  
> +	if (!$param->{delete} && !PVE::AccessControl::check_path($path)) {
> +	    raise_param_exc({ path => "invalid ACL path '$param->{path}'" });
> +	}
> +
>  	PVE::AccessControl::lock_user_config(
>  	    sub {
>  
> diff --git a/PVE/AccessControl.pm b/PVE/AccessControl.pm
> index 8b5be1e..5ac2df2 100644
> --- a/PVE/AccessControl.pm
> +++ b/PVE/AccessControl.pm
> @@ -60,6 +60,24 @@ cfs_register_file('priv/tfa.cfg',
>  		  \&parse_priv_tfa_config,
>  		  \&write_priv_tfa_config);
>  
> +sub get_permission_paths {
> +	return (
> +	'/',
> +	'/access',
> +	'/access/groups',
> +	'/access/realm',
> +	'/nodes',
> +	'/nodes/{node}',
> +	'/pool',
> +	'/pool/{poolid}',
> +	'/sdn',
> +	'/storage',
> +	'/storage/{storage}',
> +	'/vms',
> +	'/vms/{vmid}',
> +    )
> +}
> +
>  sub verify_username {
>      PVE::Auth::Plugin::verify_username(@_);
>  }
> @@ -929,6 +947,19 @@ sub normalize_path {
>      return $path;
>  }
>  
> +sub check_path {
> +    my $path = normalize_path(shift);
> +    my @regex_str_arr = ();
> +    foreach (get_permission_paths()) {
> +	    my $regex_str = $_;

nit: indentation error above

> +	$regex_str =~ s/\{vmid\}/\\d{3,}/;
> +	$regex_str =~ s/\{[a-z]+\}/[[:alnum:]\\.\\-\\_]+/;
> +	push(@regex_str_arr, $regex_str);
> +    }
> +    my $regex_str = '^(' . join('|', @regex_str_arr) . ')$';

why don't we return already a regex array by `get_permission_paths`, which may then be
named `get_permission_path_regex` ?

Else this seems to be a bit much of manual translations and also an unnecessary extra
overhead, I'd like to avoid custom template interpretations at runtime.


get_permission_paths (or whatever it will be named then) could look like:

sub ... {
    my $paths = (
        qr!/!,
        qr!/access!,
        # ...
        qr!/vms/\d{3,}!
    );

    return join("|", @paths);
}

or as above then has not really much extra value, especially as it's only used in one place,
just directly define the actual regex

sub check_path {
    my ($path) = @_;

    my $re = qr!^(
        /
        |/access
        # ...
        |/vms/\d{3,}
    )$!;

    return $path =~ $re;
}

would IMO be even nicer

> +    return $path =~ m@$regex_str@;
> +}
> +
>  PVE::JSONSchema::register_format('pve-groupid', \&verify_groupname);
>  sub verify_groupname {
>      my ($groupname, $noerr) = @_;
>