* [PATCH proxmox v2 1/2] router: compile time check privilege path parameters for existence
2026-08-31 7:08 [PATCH datacenter-manager/proxmox v2 0/2] check privilege paths during compilation Dominik Csapak
@ 2026-08-31 7:08 ` Dominik Csapak
2026-08-31 7:08 ` [PATCH datacenter-manager v2 2/2] server: api: pve firewall: fix permission check Dominik Csapak
1 sibling, 0 replies; 4+ messages in thread
From: Dominik Csapak @ 2026-08-31 7:08 UTC (permalink / raw)
To: pdm-devel
In an API privilege, path components can be interpolated from
parameters, e.g. 'foo/{bar}'. If this parameter does not exist,
the api privilege check fails and the client gets a 403 error back.
Instead of only checking this at runtime, check the existence of the
parameter in the schema during compilation since the schemas need to
be const anyway. In case of 'additional_properties', we can only
check it during runtime, but currently there are no such cases for the
API where a dynamic parameter would be used in the privilege path.
(And it does not make sense to not statically define such a parameter)
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
proxmox-router/src/router.rs | 209 ++++++++++++++++++++++++++++++++++-
1 file changed, 208 insertions(+), 1 deletion(-)
diff --git a/proxmox-router/src/router.rs b/proxmox-router/src/router.rs
index fea47ba6..ca48dcba 100644
--- a/proxmox-router/src/router.rs
+++ b/proxmox-router/src/router.rs
@@ -16,7 +16,7 @@ use proxmox_http::Body;
use serde::Serialize;
use serde_json::Value;
-use proxmox_schema::{ObjectSchema, ParameterSchema, ReturnType, Schema};
+use proxmox_schema::{AllOfSchema, ObjectSchema, OneOfSchema, ParameterSchema, ReturnType, Schema};
use super::Permission;
use crate::RpcEnvironment;
@@ -831,6 +831,131 @@ impl std::fmt::Debug for ApiMethod {
}
}
+// const helpers to check privilege parameters
+
+const fn byte_slice_eq(a: &[u8], b: &[u8]) -> bool {
+ if a.len() != b.len() {
+ return false;
+ }
+ let mut i = 0;
+ while i < a.len() {
+ if a[i] != b[i] {
+ return false;
+ }
+ i += 1;
+ }
+ true
+}
+
+const fn object_schema_has_parameter(object: &ObjectSchema, name: &[u8]) -> bool {
+ // additional properties are not statically known, so any name could exist
+ if object.additional_properties {
+ return true;
+ }
+ let mut i = 0;
+ while i < object.properties.len() {
+ if byte_slice_eq(object.properties[i].0.as_bytes(), name) {
+ return true;
+ }
+ i += 1;
+ }
+ false
+}
+
+const fn all_of_schema_has_parameter(all_of: &AllOfSchema, name: &[u8]) -> bool {
+ let mut i = 0;
+ while i < all_of.list.len() {
+ if schema_has_parameter(all_of.list[i], name) {
+ return true;
+ }
+ i += 1;
+ }
+ false
+}
+
+const fn one_of_schema_has_parameter(one_of: &OneOfSchema, name: &[u8]) -> bool {
+ if byte_slice_eq(one_of.type_property_entry.0.as_bytes(), name) {
+ return true;
+ }
+ let mut i = 0;
+ while i < one_of.list.len() {
+ if schema_has_parameter(one_of.list[i].1, name) {
+ return true;
+ }
+ i += 1;
+ }
+ false
+}
+
+const fn schema_has_parameter(schema: &Schema, name: &[u8]) -> bool {
+ match schema {
+ Schema::Object(object) => object_schema_has_parameter(object, name),
+ Schema::AllOf(all_of) => all_of_schema_has_parameter(all_of, name),
+ Schema::OneOf(one_of) => one_of_schema_has_parameter(one_of, name),
+ _ => false,
+ }
+}
+
+const fn parameter_exists(parameters: ParameterSchema, name: &[u8]) -> bool {
+ match parameters {
+ ParameterSchema::Object(object) => object_schema_has_parameter(object, name),
+ ParameterSchema::AllOf(all_of) => all_of_schema_has_parameter(all_of, name),
+ ParameterSchema::OneOf(one_of) => one_of_schema_has_parameter(one_of, name),
+ }
+}
+
+// mirrors the splitting done by check_api_permission: a component can contain multiple '/'
+// separated parts, each of which may be a '{name}' parameter reference
+const fn check_privilege_path_components(component: &str, parameters: ParameterSchema) {
+ let bytes = component.as_bytes();
+ let mut component_start = 0;
+ let mut pos = 0;
+ while pos <= bytes.len() {
+ if pos == bytes.len() || bytes[pos] == b'/' {
+ let component_len = pos - component_start;
+ let component_end = pos - 1;
+ if component_len >= 2 && bytes[component_start] == b'{' && bytes[component_end] == b'}'
+ {
+ let name = bytes
+ .split_at(component_end)
+ .0
+ .split_at(component_start + 1)
+ .1;
+ if !parameter_exists(parameters, name) {
+ panic!(
+ "privilege path references a parameter that does not exist in the method's parameter schema"
+ );
+ }
+ }
+ component_start = pos + 1;
+ }
+ pos += 1;
+ }
+}
+
+const fn assert_path_parameters_exist(perm: &Permission, parameters: ParameterSchema) {
+ match perm {
+ Permission::WithParam(_, permission) => {
+ assert_path_parameters_exist(permission, parameters)
+ }
+ Permission::Privilege(path, _, _) => {
+ let mut i = 0;
+ while i < path.len() {
+ check_privilege_path_components(path[i], parameters);
+ i += 1;
+ }
+ }
+ Permission::And(permissions) | Permission::Or(permissions) => {
+ let mut i = 0;
+ while i < permissions.len() {
+ assert_path_parameters_exist(permissions[i], parameters);
+ i += 1;
+ }
+ }
+ _ => (),
+ }
+}
+
impl ApiMethod {
pub const fn new_full(handler: &'static ApiHandler, parameters: ParameterSchema) -> Self {
Self {
@@ -890,11 +1015,19 @@ impl ApiMethod {
self
}
+ /// Set the access permissions.
+ ///
+ /// This asserts that every '{name}' parameter reference in `Privilege` permission paths
+ /// exists in the method's parameter schema, since such a path could otherwise never match at
+ /// runtime. Since API methods are usually built in a const context, a violation is a compile
+ /// time error.
pub const fn access(
mut self,
description: Option<&'static str>,
permission: &'static Permission,
) -> Self {
+ assert_path_parameters_exist(permission, self.parameters);
+
self.access = ApiAccess {
description,
permission,
@@ -903,3 +1036,77 @@ impl ApiMethod {
self
}
}
+
+#[cfg(test)]
+mod test {
+ use super::*;
+
+ use proxmox_schema::StringSchema;
+
+ const STRING_SCHEMA: Schema = StringSchema::new("test").schema();
+
+ const PARAMETERS: ObjectSchema = ObjectSchema::new(
+ "test parameters",
+ &[
+ ("bar", true, &STRING_SCHEMA),
+ ("foo", false, &STRING_SCHEMA),
+ ],
+ );
+
+ const ADDITIONAL_PARAMETERS: ObjectSchema =
+ ObjectSchema::new("test parameters", &[]).additional_properties(true);
+
+ // compile time check that valid parameter references are accepted
+ const _: ApiMethod = ApiMethod::new_dummy(&PARAMETERS).access(
+ None,
+ &Permission::And(&[
+ &Permission::Privilege(&["foo", "{foo}", "{bar}"], 1, true),
+ &Permission::Privilege(&["foo", "{foo}/{bar}"], 1, true),
+ &Permission::WithParam(
+ "foo",
+ &Permission::Or(&[&Permission::Privilege(&["foo", "{foo}"], 1, true)]),
+ ),
+ ]),
+ );
+
+ #[test]
+ #[should_panic(expected = "privilege path references a parameter")]
+ fn missing_privilege_path_parameter() {
+ let _ = ApiMethod::new_dummy(&PARAMETERS)
+ .access(None, &Permission::Privilege(&["foo", "{baz}"], 1, true));
+ }
+
+ #[test]
+ #[should_panic(expected = "privilege path references a parameter")]
+ fn missing_parameter_in_combined_component() {
+ let _ = ApiMethod::new_dummy(&PARAMETERS).access(
+ None,
+ &Permission::Or(&[&Permission::Privilege(
+ &["datastore", "{foo}/{baz}"],
+ 0b01,
+ true,
+ )]),
+ );
+ }
+
+ #[test]
+ fn malformed_parameter_in_combined_component() {
+ // should work, components are not enclosed in brackets properly so no interpolation should
+ // be done
+ let _ = ApiMethod::new_dummy(&PARAMETERS).access(
+ None,
+ &Permission::Or(&[&Permission::Privilege(&["foo", "bar/{baz/}"], 1, true)]),
+ );
+
+ let _ = ApiMethod::new_dummy(&PARAMETERS).access(
+ None,
+ &Permission::Or(&[&Permission::Privilege(&["foo", "{bar/baz}/"], 1, true)]),
+ );
+ }
+
+ #[test]
+ fn additional_properties_allow_any_parameter() {
+ let _ = ApiMethod::new_dummy(&ADDITIONAL_PARAMETERS)
+ .access(None, &Permission::Privilege(&["foo", "{baz}"], 1, true));
+ }
+}
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH datacenter-manager v2 2/2] server: api: pve firewall: fix permission check
2026-08-31 7:08 [PATCH datacenter-manager/proxmox v2 0/2] check privilege paths during compilation Dominik Csapak
2026-08-31 7:08 ` [PATCH proxmox v2 1/2] router: compile time check privilege path parameters for existence Dominik Csapak
@ 2026-08-31 7:08 ` Dominik Csapak
2026-08-31 7:58 ` partially-applied: " Lukas Wagner
1 sibling, 1 reply; 4+ messages in thread
From: Dominik Csapak @ 2026-08-31 7:08 UTC (permalink / raw)
To: pdm-devel
This is the api call for the overall firewall status, not a specific
remote, so there isn't a 'remote' parameter to limit us to.
Instead, check the users permission against /resource/{resource} for
each remote and only show those where the user has permission.
This fixes a 403 error on the firewall panel for non root@pam users.
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
server/src/api/pve/firewall.rs | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/server/src/api/pve/firewall.rs b/server/src/api/pve/firewall.rs
index 5a6a209e..ceddf205 100644
--- a/server/src/api/pve/firewall.rs
+++ b/server/src/api/pve/firewall.rs
@@ -1,8 +1,10 @@
use std::collections::HashMap;
use std::sync::Arc;
-use anyhow::Error;
+use anyhow::{Context, Error};
+use proxmox_access_control::CachedUserInfo;
+use proxmox_auth_api::types::Authid;
use proxmox_router::{Permission, Router, RpcEnvironment, SubdirMap, list_subdirs_api_method};
use proxmox_schema::api;
use proxmox_sortable_macro::sortable;
@@ -242,16 +244,26 @@ async fn fetch_node_firewall_status(
items: { type: RemoteFirewallStatus },
},
access: {
- permission: &Permission::Privilege(&["resource", "{remote}"], PRIV_RESOURCE_AUDIT, false),
+ permission: &Permission::Anybody,
+ description: "A user needs `Resource.Audit` privileges on /resource/{remote}."
},
)]
/// Get firewall status of all PVE remotes.
pub async fn pve_firewall_status(
- _rpcenv: &mut dyn RpcEnvironment,
+ rpcenv: &mut dyn RpcEnvironment,
) -> Result<Vec<RemoteFirewallStatus>, Error> {
+ let auth_id: Authid = rpcenv
+ .get_auth_id()
+ .context("no authid available")?
+ .parse()?;
+ let user_info = CachedUserInfo::new()?;
+
let pve_remotes: Vec<Remote> = crate::api::remotes::RemoteIterator::new()?
.remote_type(pdm_api_types::remotes::RemoteType::Pve)
.into_remotes()
+ .filter(|remote| {
+ user_info.lookup_privs(&auth_id, &["resource", &remote.id]) & PRIV_RESOURCE_AUDIT != 0
+ })
.collect();
if pve_remotes.is_empty() {
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread