From: Shannon Sterz <s.sterz@proxmox.com>
To: pdm-devel@lists.proxmox.com
Subject: [pdm-devel] [PATCH proxmox v2 5/6] access-control: api: refactor validation checks to re-use existing code
Date: Fri, 11 Apr 2025 15:44:29 +0200 [thread overview]
Message-ID: <20250411134435.269524-6-s.sterz@proxmox.com> (raw)
In-Reply-To: <20250411134435.269524-1-s.sterz@proxmox.com>
also makes sure to make them easier to understand when auditing the
code. as these checks are fairly complex to understand otherwise. it
also make partial matches configurable through the
AccessControlConfig.
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
proxmox-access-control/src/api.rs | 85 +++++++++++++++++++-----------
proxmox-access-control/src/init.rs | 8 +++
2 files changed, 63 insertions(+), 30 deletions(-)
diff --git a/proxmox-access-control/src/api.rs b/proxmox-access-control/src/api.rs
index bb872b97..bf3ffb21 100644
--- a/proxmox-access-control/src/api.rs
+++ b/proxmox-access-control/src/api.rs
@@ -48,13 +48,17 @@ pub fn read_acl(
.ok_or_else(|| format_err!("endpoint called without an auth id"))?
.parse()?;
- let top_level_privs = CachedUserInfo::new()?.lookup_privs(&auth_id, &["access", "acl"]);
+ // if a user does not have enough permissions to see all entries, we need to filter them
+ let filter_entries = CachedUserInfo::new()?
+ .check_privs(
+ &auth_id,
+ &["access", "acl"],
+ access_conf().acl_audit_privileges(),
+ access_conf().allow_partial_permission_match(),
+ )
+ .is_err();
- let filter = if top_level_privs & access_conf().acl_audit_privileges() == 0 {
- Some(auth_id)
- } else {
- None
- };
+ let filter = if filter_entries { Some(auth_id) } else { None };
let (mut tree, digest) = crate::acl::config()?;
@@ -136,34 +140,52 @@ pub fn update_acl(
.expect("auth id could not be determined")
.parse()?;
- let user_info = CachedUserInfo::new()?;
- let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
+ let unprivileged_user = CachedUserInfo::new()?
+ .check_privs(
+ ¤t_auth_id,
+ &["access", "acl"],
+ access_conf.acl_modify_privileges(),
+ access_conf.allow_partial_permission_match(),
+ )
+ .is_err();
- if top_level_privs & access_conf.acl_modify_privileges() == 0 {
- if group.is_some() {
- bail!("Unprivileged users are not allowed to create group ACL item.");
+ if unprivileged_user {
+ if group.is_none()
+ && !current_auth_id.is_token()
+ && auth_id
+ .as_ref()
+ .map(|id| id.is_token() && current_auth_id.user() == id.user())
+ .unwrap_or_default()
+ {
+ // a user is directly editing the privileges of their own tokens, this is always
+ // allowed
+ } else {
+ if group.is_some() {
+ bail!("Unprivileged users are not allowed to create group ACL item.");
+ }
+
+ let auth_id = auth_id.as_ref().ok_or_else(|| {
+ format_err!("Unprivileged user needs to provide auth_id to update ACL item.")
+ })?;
+
+ if current_auth_id.is_token() {
+ bail!("Unprivileged API tokens can't set ACL items.");
+ }
+
+ if !auth_id.is_token() {
+ bail!("Unprivileged users can only set ACL items for API tokens.");
+ }
+
+ if current_auth_id.user() != auth_id.user() {
+ bail!("Unprivileged users can only set ACL items for their own API tokens.");
+ }
+
+ // this should not be reachable, but just in case, bail here
+ bail!("Unprivileged user is trying to set an invalid ACL item.")
}
-
- match &auth_id {
- Some(auth_id) => {
- if current_auth_id.is_token() {
- bail!("Unprivileged API tokens can't set ACL items.");
- } else if !auth_id.is_token() {
- bail!("Unprivileged users can only set ACL items for API tokens.");
- } else if auth_id.user() != current_auth_id.user() {
- bail!("Unprivileged users can only set ACL items for their own API tokens.");
- }
- }
- None => {
- bail!("Unprivileged user needs to provide auth_id to update ACL item.");
- }
- };
}
- // FIXME: add support for group
- if group.is_some() {
- bail!("parameter 'group' - groups are currently not supported");
- } else if let Some(auth_id) = &auth_id {
+ if let Some(auth_id) = &auth_id {
// only allow deleting non-existing auth id's, not adding them
if !delete {
let exists = crate::user::cached_config()?
@@ -178,6 +200,9 @@ pub fn update_acl(
}
}
}
+ } else if group.is_some() {
+ // FIXME: add support for groups
+ bail!("parameter 'group' - groups are currently not supported");
} else {
// FIXME: suggest groups here once they exist
bail!("missing 'userid' parameter");
diff --git a/proxmox-access-control/src/init.rs b/proxmox-access-control/src/init.rs
index c219a78e..39a12352 100644
--- a/proxmox-access-control/src/init.rs
+++ b/proxmox-access-control/src/init.rs
@@ -95,6 +95,14 @@ pub trait AccessControlConfig: Send + Sync {
let _ = path;
Ok(())
}
+
+ /// Whether the API endpoints to inspect the ACL should use partial permission matching or not.
+ ///
+ /// Override this if the product in question uses more than one bit to specify permissions (so,
+ /// in case it is *not* using a bitmap) and the match between permissions needs to be exact.
+ fn allow_partial_permission_match(&self) -> bool {
+ true
+ }
}
pub fn init<P: AsRef<Path>>(
--
2.39.5
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
next prev parent reply other threads:[~2025-04-11 13:45 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-04-11 13:44 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp v2 00/11] ACL edit api and ui components Shannon Sterz
2025-04-11 13:44 ` [pdm-devel] [PATCH proxmox v2 1/6] access-control: add more types to prepare for api feature Shannon Sterz
2025-04-11 13:44 ` [pdm-devel] [PATCH proxmox v2 2/6] access-control: add acl " Shannon Sterz
2025-04-11 13:44 ` [pdm-devel] [PATCH proxmox v2 3/6] access-control: add comments to roles function of AccessControlConfig Shannon Sterz
2025-04-11 13:44 ` [pdm-devel] [PATCH proxmox v2 4/6] access-control: add generic roles endpoint to `api` feature Shannon Sterz
2025-04-11 13:44 ` Shannon Sterz [this message]
2025-04-11 13:44 ` [pdm-devel] [PATCH proxmox v2 6/6] access-control: api: refactor extract_acl_node_data to be non-recursive Shannon Sterz
2025-04-11 13:44 ` [pdm-devel] [PATCH yew-comp v2 1/3] api-types/role_selector: depend on common `RoleInfo` type Shannon Sterz
2025-04-11 13:44 ` [pdm-devel] [PATCH yew-comp v2 2/3] acl: add a view and semi-generic `EditWindow` for acl entries Shannon Sterz
2025-04-11 13:44 ` [pdm-devel] [PATCH yew-comp v2 3/3] role_selector/acl_edit: make api endpoint and default role configurable Shannon Sterz
2025-04-11 13:44 ` [pdm-devel] [PATCH datacenter-manager v2 1/2] server: use proxmox-access-control api implementations Shannon Sterz
2025-04-11 13:44 ` [pdm-devel] [PATCH datacenter-manager v2 2/2] ui: configuration: add panel for viewing and editing acl entries Shannon Sterz
2025-04-17 15:46 ` [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp v2 00/11] ACL edit api and ui components Thomas Lamprecht
2025-04-22 8:12 ` Shannon Sterz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250411134435.269524-6-s.sterz@proxmox.com \
--to=s.sterz@proxmox.com \
--cc=pdm-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal