Re: [pdm-devel] [PATCH datacenter-manager 1/2] fix #6901: api: add permission checks for PBS rrd endpoints

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

On Fri Oct 10, 2025 at 5:18 PM CEST, Shan Shaji wrote:
> When a non-root user tried to view the RRD data of the PBS node or
> datastores, even with Administrator privileges, the API was
> returning a "403: permission check failed" error. This occured
> because the access property was not defined inside the `api` macro.
>
> To fix the issue, a resource level permission check was added.
> Now if a user has atleast the `Resource.Audit` permission, then they
> can access the RRD data of the node and datastores.
>
> Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
> ---
>  server/src/api/pbs/rrddata.rs | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/server/src/api/pbs/rrddata.rs b/server/src/api/pbs/rrddata.rs
> index aa980d4..c8885de 100644
> --- a/server/src/api/pbs/rrddata.rs
> +++ b/server/src/api/pbs/rrddata.rs
> @@ -2,8 +2,9 @@ use anyhow::Error;
>  use pdm_api_types::{
>      remotes::REMOTE_ID_SCHEMA,
>      rrddata::{PbsDatastoreDataPoint, PbsNodeDataPoint},
> +    PRIV_RESOURCE_AUDIT,
>  };
> -use proxmox_router::Router;
> +use proxmox_router::{Permission, Router};
>  use proxmox_rrd_api_types::{RrdMode, RrdTimeframe};
>  use proxmox_schema::api;
>  use serde_json::Value;
> @@ -100,6 +101,10 @@ impl DataPoint for PbsDatastoreDataPoint {
>              },
>          },
>      },
> +    access: {
> +        permission: &Permission::Privilege(&["resource", "{remote}"], PRIV_RESOURCE_AUDIT, false),
> +        description: "The user needs to have atleast `Resource.Audit` privilege under `/resource`."

nit: a typo snuck in here, this should be `at least`

also correct me if im wrong, but this is wrong? for the user to be able
to access this endpoint, they would need `Resource.Audit` permissions on
the specific remote that they are trying to get the RRD data for.
otherwise they'll get a 403 error.

by default permissions propagate down the the acl tree, but that can be
disabled. so `Resource.Audit` on `/resource` does not always imply
`Resource.Audit` on `/resource/my-pbs-remote` etc. hence, this should
probably be:

The user needs to have at least the `Resource.Audit` privilege on `/resource/{remote}`.

note that for pbs we autogenerate descriptions for endpoints that only
set "permission" but not "description" [1]. while this has the upside of
always being in-line with the actual code, the descriptions might be a
bit difficult to parse especially for newbies to our codebase.

[1]: https://pbs.proxmox.com/docs/api-viewer/index.html#/tape/drive/{drive}/clean

> +    }
>  )]
>  /// Read PBS node stats
>  async fn get_pbs_node_rrd_data(
> @@ -125,6 +130,10 @@ async fn get_pbs_node_rrd_data(
>              },
>          },
>      },
> +    access: {
> +        permission: &Permission::Privilege(&["resource", "{remote}", "datastore", "{datastore}"], PRIV_RESOURCE_AUDIT, false),
> +        description: "The user needs to have atleast `Resource.Audit` privilege under `/resource`."

nit: same as above, should probably be:

The user needs to have at least the `Resource.Audit` privilege on `/resource/{remote}/datastore/{datastore}`.

> +    }
>  )]
>  /// Read PBS datastore stats
>  async fn get_pbs_datastore_rrd_data(



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