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 6FC371FF0B3 for ; Fri, 11 Sep 2026 12:02:51 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 29DA121442; Fri, 11 Sep 2026 12:02:51 +0200 (CEST) Date: Fri, 11 Sep 2026 12:02:45 +0200 From: Wolfgang Bumiller To: Dominik Csapak Subject: Re: [PATCH proxmox v2 1/2] router: compile time check privilege path parameters for existence Message-ID: References: <20260831070855.568455-1-d.csapak@proxmox.com> <20260831070855.568455-2-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20260831070855.568455-2-d.csapak@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1789120955510 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.853 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: X3GYZTKYZSKLMB7X4RP75MTHDWOCZG6R X-Message-ID-Hash: X3GYZTKYZSKLMB7X4RP75MTHDWOCZG6R X-MailFrom: w.bumiller@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 CC: pdm-devel@lists.proxmox.com 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: 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 > --- > 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 > > > > > --