Re: [PATCH access-control] rpc: do not check malformed acl path

[Dominik Csapak <d.csapak@proxmox.com>]

high level: I would not touch the acl checks here in anyway, but
as I already wrote would like to do it like we do elsewhere:

for example: in the 'startall' api call, we have:

permissions => { user => 'all' },

so every api user can call it, but then inside the api handler
we check the permission manually and do a `raise_perm_exc`


so in our case here, we first check if the parameter is a
pciid or a mapping, and check the permission accordingly.

then we don't need to adapt the ACL part, and have still
proper acl validation.

does that make sense?

On 8/26/26 12:37 PM, Elias Huhsovitz wrote:
> Signed-off-by: Elias Huhsovitz <e.huhsovitz@proxmox.com>
> ---
> Thank you for the feedback. I looked into this a bit more and
> provided an experimental patch as a reference.
> 
> The Problem
> -----------
> The core problem is indeed the colon ':', which makes sense.
> 
> I did some further testing and the same error occurs when sending a
> request to the enpoint:
> 
> GET /cluster/mapping/pci/{id}
> 
> It outputs the errors:
> pveproxy[42711]: Failed to normalize '/mapping/pci/0000:01:00.0'
> pveproxy[42711]: internal error: ACL check called for undefined ACL path!
> pveproxy[42711]: Use of uninitialized value $path in concatenation (.) or string at /usr/share/perl5/PVE/RPCEnvironment.pm line 325.
> 
> Mismatch
> ---------
> I think explicitely checking the permissions in the api handler itself
> will work, but to my understanding there is an underlying mismatch:
> 
> The API schema explicitly allows colons (for raw PCI IDs), but the
> ACL path normalizer explicitly rejects them.
> 
> Proposed solution
> -----------------
> IMHO providing explicit permissions on a raw PCI ID like 0000:01:00.0
> instead of a mapping should not happen (i think) and should not be
> checked.
> 
> What is your opinion on simply not checking invalid ACL
> paths?
> 
> This would cause a more graceful failure.
> 
> Caveats
> -------
> (I fear that such a check might causes problems in existing systems,
> that rely on "invalid" ACL paths. But i would love to hear your input
> on this)
> 
> Side note
> ---------
> What aboud allowing encoded PCI IDs instead? e.g.,
> GET /nodes/lisa/hardware/pci/0000%3A01%3A00.0
> 
> Summary of changes
> ------------------
> 
>   src/PVE/RPCEnvironment.pm | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
> index 7591aa9..61ef28b 100644
> --- a/src/PVE/RPCEnvironment.pm
> +++ b/src/PVE/RPCEnvironment.pm
> @@ -487,7 +487,11 @@ sub exec_api2_perm_check {
>           }
>           my $path = PVE::Tools::template_replace($tmplpath, $param);
>           my $normpath = PVE::AccessControl::normalize_path($path);
> -        warn "Failed to normalize '$path'\n" if !defined($normpath) && defined($path);
> +
> +        if (!defined($normpath)) {
> +            warn "Failed to normalize ACL path '$path'\n" if defined($path);
> +            return 0;
> +        }
>   
>           return $self->check_full($username, $normpath, $privs, $any, $noerr);
>       } elsif ($test eq 'userid-group') {