Re: [pdm-devel] [PATCH datacenter-manager 2/3] fix #6901: api: add explicit permission check for vnets list

["Shannon Sterz" <s.sterz@proxmox.com>]

On Wed Oct 15, 2025 at 5:20 PM CEST, Stefan Hanreich wrote:
> On 10/14/25 5:03 PM, Shan Shaji wrote:
>> When a non-root user tried to access the EVPN section, the API was
>> throwing "403: permission check failed" error. To fix this, add
>> explicit permission check for the `/vnets` endpoint.
>>
>> Now every authenticated user can access the endpoint however, the user
>> needs to have at least `Resource.Audit` permission under `/resource`.
>> Only the vnets from remotes which the user has `Resource.Audit`
>> privilege on `/remote/{remote_name}` will be included in the returned
>> list.
>>
>> Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
>> ---
>>  server/src/api/sdn/vnets.rs | 29 ++++++++++++++++++++++++++---
>>  1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/server/src/api/sdn/vnets.rs b/server/src/api/sdn/vnets.rs
>> index a8092cf..a2dfdb4 100644
>> --- a/server/src/api/sdn/vnets.rs
>> +++ b/server/src/api/sdn/vnets.rs
>> @@ -1,3 +1,4 @@
>> +use core::option::Option::Some;
>
> unneeded import?
>
>>  use std::collections::HashSet;
>>
>>  use anyhow::{format_err, Error};
>> @@ -5,10 +6,11 @@ use pbs_api_types::REMOTE_ID_SCHEMA;
>>  use pdm_api_types::{
>>      remotes::RemoteType,
>>      sdn::{CreateVnetRemote, ListVnet, SDN_ID_SCHEMA, VXLAN_ID_SCHEMA},
>> -    Authid,
>> +    Authid, PRIV_RESOURCE_AUDIT,
>>  };
>> +use proxmox_access_control::CachedUserInfo;
>>  use proxmox_rest_server::WorkerTask;
>> -use proxmox_router::{Router, RpcEnvironment};
>> +use proxmox_router::{http_bail, Permission, Router, RpcEnvironment};
>>  use proxmox_schema::api;
>>  use pve_api_types::{CreateVnet, SdnVnetType};
>>
>> @@ -52,16 +54,37 @@ pub const ROUTER: Router = Router::new()
>>              type: ListVnet,
>>          },
>>      },
>> +    access: {
>> +        permission: &Permission::Anybody,
>> +        description: "The user needs to have at least the `Resource.Audit` privilege under `/resource`.
>> +        Only vnets from remotes for which the user has `Resource.Audit` on `/remote/{remote_name}`
>> +        will be included in the returned list."
>> +    }
>>  )]
>>  /// Query VNets of PVE remotes with optional filtering options
>>  async fn list_vnets(
>>      pending: Option<bool>,
>>      running: Option<bool>,
>>      remotes: Option<HashSet<String>>,
>> +    rpcenv: &mut dyn RpcEnvironment,
>>  ) -> Result<Vec<ListVnet>, Error> {
>> +    let user_info = CachedUserInfo::new()?;
>> +
>> +    let auth_id: Authid = rpcenv
>> +        .get_auth_id()
>> +        .ok_or_else(|| format_err!("no authid available"))?
>> +        .parse()?;
>> +
>> +    if !user_info.any_privs_below(&auth_id, &["resource"], PRIV_RESOURCE_AUDIT)? {
>> +        http_bail!(FORBIDDEN, "user has no access to resources");
>
> we use UNAUTHORIZED instead of FORBIDDEN in other places, so imo would
> be better to do that here too - same for the other 2 patches as well.

this was already discussed for a previous patch series. UNAUTHORIZED
there is arguably false, as we usually return a 403 FORBIDDEN when a
user does not have sufficient permissions (e.g. that's what you get if
you define permissions through the api macro and a user doesn't have
them). not a 401 UNAUTHORIZED, which we use to indicate that the user
has not been authenticated (i.e. no or invalid ticket).

iirc Permission::Anybody above means that anybody that has been
authenticated can access this endpoint (as opposed to Permission::World,
which means even unauthenticated users can access it). hence, we know
the user is authenticated here. so this is fine, but returning 401
UNAUTHORIZED in those other endpoints is wrong imo.

note that the ui relies on that in many cases: a 401 will trigger the
login prompt to be shown again, a 403 will simply show an error.

>> +    }
>> +
>>      let (remote_config, _) = pdm_config::remotes::config()?;
>> +    let authorized_remotes = remote_config.into_iter().filter(|(remote_name, _)| {
>> +        user_info.lookup_privs(&auth_id, &["resource", &remote_name]) & PRIV_RESOURCE_AUDIT != 0
>> +    });
>>
>> -    let filtered_remotes = remote_config.into_iter().filter_map(|(_, remote)| {
>> +    let filtered_remotes = authorized_remotes.filter_map(|(_, remote)| {
>>          if remote.ty == RemoteType::Pve
>>              && remotes
>>                  .as_ref()
>
>
>
> _______________________________________________
> pdm-devel mailing list
> pdm-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel