From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id D0E321FF13A for ; Wed, 08 Jul 2026 15:37:07 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3528F2146B; Wed, 08 Jul 2026 15:37:07 +0200 (CEST) From: Dominik Csapak To: pdm-devel@lists.proxmox.com Subject: [RFC PATCH proxmox 1/2] router: compile time check privilege path parameters for existence Date: Wed, 8 Jul 2026 15:36:23 +0200 Message-ID: <20260708133630.1807997-2-d.csapak@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260708133630.1807997-1-d.csapak@proxmox.com> References: <20260708133630.1807997-1-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.056 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 5FTCL3T5Y7ZBCW6MDIURI6KQYOBSC2DP X-Message-ID-Hash: 5FTCL3T5Y7ZBCW6MDIURI6KQYOBSC2DP X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- 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