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

[RFC/PATCH datacenter-manager/proxmox 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).

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: remove 'remote' parameter from overall
    status

 server/src/api/pve/firewall.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


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

-- 
Generated by murpp 0.11.0

[RFC PATCH proxmox 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>
---
I mentioned it briefly in the subject, but since having a privilege path
component only added via additional_properties does not make sense,
we might want to not return true in that case and only check the
statically defined parameters? Can be fixed up of course.

Also the 'byte_slice_eq' would make more sense in a 'const fn' helper
crate maybe? (did not found a very suitable place...)

 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

[PATCH datacenter-manager 2/2] server: api: pve firewall: remove 'remote' parameter from overall status

[Dominik Csapak]

this is the api call for the overall status, not a specific remote, so
there isn't a 'remote' parameter to limit us to.

Simply require audit on '/resource' to indicate we need resource
permissions.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/server/src/api/pve/firewall.rs b/server/src/api/pve/firewall.rs
index 5a6a209e..6c18ba10 100644
--- a/server/src/api/pve/firewall.rs
+++ b/server/src/api/pve/firewall.rs
@@ -242,7 +242,7 @@ async fn fetch_node_firewall_status(
         items: { type: RemoteFirewallStatus },
     },
     access: {
-        permission: &Permission::Privilege(&["resource", "{remote}"], PRIV_RESOURCE_AUDIT, false),
+        permission: &Permission::Privilege(&["resource"], PRIV_RESOURCE_AUDIT, false),
     },
 )]
 /// Get firewall status of all PVE remotes.
-- 
2.47.3