From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 388841FF17E for ; Thu, 16 Oct 2025 10:12:10 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AD7E6C58B; Thu, 16 Oct 2025 10:12:30 +0200 (CEST) Mime-Version: 1.0 Date: Thu, 16 Oct 2025 10:12:28 +0200 Message-Id: To: "Stefan Hanreich" X-Mailer: aerc 0.20.0 References: <20251014150358.553238-1-s.shaji@proxmox.com> <20251014150358.553238-3-s.shaji@proxmox.com> <52b7cfb5-83bf-4717-b93d-14f5e013aa99@proxmox.com> In-Reply-To: <52b7cfb5-83bf-4717-b93d-14f5e013aa99@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1760602345431 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, vnets.rs] Subject: Re: [pdm-devel] [PATCH datacenter-manager 2/3] fix #6901: api: add explicit permission check for vnets list X-BeenThere: pdm-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Datacenter Manager development discussion Cc: Proxmox Datacenter Manager development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pdm-devel-bounces@lists.proxmox.com Sender: "pdm-devel" 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 >> --- >> 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, >> running: Option, >> remotes: Option>, >> + rpcenv: &mut dyn RpcEnvironment, >> ) -> Result, 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