[PATCH datacenter-manager/proxmox v2 0/2] check privilege paths during compilation

[PATCH datacenter-manager/proxmox v2 0/2] check privilege paths during compilation

[Dominik Csapak]

This adds a check in proxmox-router to check the used parameter interpolations
in the privilege paths of api calls for existence.

Aside from the two use sites that fabian[0] already sent a patch for
it found another wrong usage of this (patch 2/2).

changes from rfc/v1:
* rebase on master
* require Resource.Audit on /resource/{remote} for each remote, filter out
  where this isn't the case instead of requiring it on /resource

0: https://lore.proxmox.com/pdm-devel/e7fd6763-4c68-4669-a32a-c34c9ad08cbe@proxmox.com/T/#t


proxmox:

Dominik Csapak (1):
  router: compile time check privilege path parameters for existence

 proxmox-router/src/router.rs | 209 ++++++++++++++++++++++++++++++++++-
 1 file changed, 208 insertions(+), 1 deletion(-)


proxmox-datacenter-manager:

Dominik Csapak (1):
  server: api: pve firewall: fix permission check

 server/src/api/pve/firewall.rs | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)


Summary over all repositories:
  2 files changed, 223 insertions(+), 4 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH proxmox v2 1/2] router: compile time check privilege path parameters for existence

[Dominik Csapak]

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

Re: [PATCH proxmox v2 1/2] router: compile time check privilege path parameters for existence

[Wolfgang Bumiller]

On Mon, Aug 31, 2026 at 09:08:47AM +0200, Dominik Csapak wrote:
> 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 {

We should consider adding a helper crate for such things.
We already have a byte slice compare function in `proxmox-schema`.

> +    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();

We should immediately return if `bytes.len() < 3`, see below ↓

> +    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;

Technically the above can underflow on an empty string where it would be
`0 - 1`.
Not an actual issue, but since we only care about components which are
names enclosed in braces, we could skip this entire function if the
string is not at least 3 characters, because an empty `{}` also does not
make sense.
So we could also add a more informative error message for
`if component_len == 2`, and turn the `>= 2` below into a `> 2`.

> +            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"

↑ needs line wrapping

> +                    );
> +                }
> +            }
> +            component_start = pos + 1;
> +        }
> +        pos += 1;
> +    }
> +}
> +
> +const fn assert_path_parameters_exist(perm: &Permission, parameters: ParameterSchema) {
> +    match perm {
> +        Permission::WithParam(_, permission) => {

↑ The first entry is a parameter name - shouldn't we check this one,
too? (The content of that parameter is considered to be the user name
to check `permission` against.)

> +            assert_path_parameters_exist(permission, parameters)
> +        }
> +        Permission::Privilege(path, _, _) => {

↑ nit: would pluralize this to "paths"

> +            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
> 
> 
> 
> 
> 

-- 

[PATCH datacenter-manager v2 2/2] server: api: pve firewall: fix permission check

[Dominik Csapak]

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

partially-applied: [PATCH datacenter-manager v2 2/2] server: api: pve firewall: fix permission check

[Lukas Wagner]

On Mon Aug 31, 2026 at 9:08 AM CEST, Dominik Csapak wrote:
> 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>

applied this one, thanks!