From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 8549C1FF09B for ; Mon, 14 Sep 2026 14:32:50 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D5F4A215A3; Mon, 14 Sep 2026 14:32:49 +0200 (CEST) From: Dominik Csapak To: pdm-devel@lists.proxmox.com Subject: [PATCH proxmox v3 2/2] router: compile time check privilege path parameters for existence Date: Mon, 14 Sep 2026 14:30:45 +0200 Message-ID: <20260914123243.3216993-3-d.csapak@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260914123243.3216993-1-d.csapak@proxmox.com> References: <20260914123243.3216993-1-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.467 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) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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: BQXNISHNEVR3FJW3DJJRDSWJCSVIXPFK X-Message-ID-Hash: BQXNISHNEVR3FJW3DJJRDSWJCSVIXPFK 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) This also checks existence of parameters referenced with `Permission::WithParam` and `Permission::UserParam`. Signed-off-by: Dominik Csapak --- proxmox-router/Cargo.toml | 1 + proxmox-router/src/router.rs | 235 ++++++++++++++++++++++++++++++++++- 2 files changed, 235 insertions(+), 1 deletion(-) diff --git a/proxmox-router/Cargo.toml b/proxmox-router/Cargo.toml index 7725da4e..60a33d44 100644 --- a/proxmox-router/Cargo.toml +++ b/proxmox-router/Cargo.toml @@ -46,6 +46,7 @@ unicode-width ="0.2" rustyline = { version = "14", optional = true } libc = { workspace = true, optional = true } +proxmox-const-utils.workspace = true proxmox-http = { workspace = true, optional = true } proxmox-http-error.workspace = true proxmox-schema.workspace = true diff --git a/proxmox-router/src/router.rs b/proxmox-router/src/router.rs index fea47ba6..f82d24f6 100644 --- a/proxmox-router/src/router.rs +++ b/proxmox-router/src/router.rs @@ -11,12 +11,13 @@ use http::{Method, Response}; #[cfg(feature = "server")] use hyper::body::Incoming; use percent_encoding::percent_decode_str; +use proxmox_const_utils::byte_string_eq; #[cfg(feature = "server")] 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 +832,128 @@ impl std::fmt::Debug for ApiMethod { } } +// const helpers to check privilege parameters + +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_string_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_string_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(); + // early return for strings that can't interpolate + if bytes.len() < 2 { + return; + } + 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; + if component_len >= 2 && bytes[component_start] == b'{' && bytes[pos - 1] == b'}' { + if component_len == 2 { + panic!("empty parameter declaration"); + } + // double split_at because range slicing is not const + let name = bytes.split_at(pos - 1).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(name, permission) => { + if !parameter_exists(parameters, name.as_bytes()) { + panic!("given user parameter does not exist"); + } + assert_path_parameters_exist(permission, parameters) + } + Permission::UserParam(name) => { + if !parameter_exists(parameters, name.as_bytes()) { + panic!("given user parameter does not exist"); + } + } + Permission::Privilege(paths, _, _) => { + let mut i = 0; + while i < paths.len() { + check_privilege_path_components(paths[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 +1013,19 @@ impl ApiMethod { self } + /// Set the access permissions. + /// + /// This asserts that every '{name}' parameter reference in `Privilege` permission paths, and + /// the parameters in `UserParam` and `WithParam` exists in the method's parameter schema, + /// since such a parameter 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 +1034,105 @@ 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)]), + ), + &Permission::Privilege(&["/"], 1, true), + &Permission::Privilege(&["/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 = "given user parameter does not exist")] + fn missing_privilege_user_parameter() { + let _ = ApiMethod::new_dummy(&PARAMETERS).access( + None, + &Permission::WithParam( + "not-existing", + &Permission::Privilege(&["foo", "{baz}"], 1, true), + ), + ); + } + + #[test] + #[should_panic(expected = "given user parameter does not exist")] + fn missing_user_parameter() { + let _ = + ApiMethod::new_dummy(&PARAMETERS).access(None, &Permission::UserParam("not-existing")); + } + + #[test] + #[should_panic(expected = "empty parameter declaration")] + fn missing_privilege_path_parameter_name() { + let _ = ApiMethod::new_dummy(&PARAMETERS) + .access(None, &Permission::Privilege(&["foo", "{}"], 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