[pdm-devel] [PATCH proxmox v3 1/5] access-control: add acl feature to only expose types and the AclTree

[Shannon Sterz <s.sterz@proxmox.com>]

this is useful, when an application wants to only handle an acl tree
without depending on more complex features provided by the rest of the
`impl` feature or its bigger dependencies (e.g. openssl).

access-control: clean up `cfg`s for impl feature

by moving code into separate impl blocks and module. this avoids
adding a lot of cfg macros separatelly.

Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
 proxmox-access-control/Cargo.toml          |   5 +-
 proxmox-access-control/src/acl.rs          | 386 +++++++++++----------
 proxmox-access-control/src/init.rs         |  91 ++---
 proxmox-access-control/src/lib.rs          |   4 +-
 proxmox-access-control/src/token_shadow.rs |   2 +-
 proxmox-access-control/src/user.rs         |   3 +-
 6 files changed, 266 insertions(+), 225 deletions(-)

diff --git a/proxmox-access-control/Cargo.toml b/proxmox-access-control/Cargo.toml
index d23d272d..6df3b3ba 100644
--- a/proxmox-access-control/Cargo.toml
+++ b/proxmox-access-control/Cargo.toml
@@ -35,17 +35,20 @@ proxmox-uuid = { workspace = true }
 
 [features]
 default = []
+acl = [
+    "dep:proxmox-section-config",
+]
 api = [
     "impl",
     "dep:hex",
 ]
 impl = [
+    "acl",
     "dep:nix",
     "dep:openssl",
     "dep:proxmox-config-digest",
     "dep:proxmox-product-config",
     "dep:proxmox-router",
-    "dep:proxmox-section-config",
     "dep:proxmox-shared-memory",
     "dep:proxmox-sys",
     "dep:serde_json",
diff --git a/proxmox-access-control/src/acl.rs b/proxmox-access-control/src/acl.rs
index ec0a77b5..7813ea5d 100644
--- a/proxmox-access-control/src/acl.rs
+++ b/proxmox-access-control/src/acl.rs
@@ -1,15 +1,18 @@
-use std::collections::{BTreeMap, BTreeSet, HashMap};
+#[cfg(feature = "impl")]
+use std::collections::BTreeSet;
+use std::collections::{BTreeMap, HashMap};
+#[cfg(feature = "impl")]
 use std::io::Write;
+#[cfg(feature = "impl")]
 use std::path::Path;
-use std::sync::{Arc, OnceLock, RwLock};
 
 use anyhow::{bail, Error};
 
 use proxmox_auth_api::types::{Authid, Userid};
+#[cfg(feature = "impl")]
 use proxmox_config_digest::ConfigDigest;
-use proxmox_product_config::{open_api_lockfile, replace_privileged_config, ApiLockGuard};
 
-use crate::init::{access_conf, acl_config, acl_config_lock};
+use crate::init::access_conf;
 
 pub fn split_acl_path(path: &str) -> Vec<&str> {
     let items = path.split('/');
@@ -302,6 +305,120 @@ impl AclTree {
         node.insert_user_role(auth_id.to_owned(), role.to_string(), propagate);
     }
 
+    fn parse_acl_line(&mut self, line: &str) -> Result<(), Error> {
+        let items: Vec<&str> = line.split(':').collect();
+
+        if items.len() != 5 {
+            bail!("wrong number of items.");
+        }
+
+        if items[0] != "acl" {
+            bail!("line does not start with 'acl'.");
+        }
+
+        let propagate = if items[1] == "0" {
+            false
+        } else if items[1] == "1" {
+            true
+        } else {
+            bail!("expected '0' or '1' for propagate flag.");
+        };
+
+        let path_str = items[2];
+        let path = split_acl_path(path_str);
+        let node = self.get_or_insert_node(&path);
+
+        let uglist: Vec<&str> = items[3].split(',').map(|v| v.trim()).collect();
+
+        let rolelist: Vec<&str> = items[4].split(',').map(|v| v.trim()).collect();
+
+        for user_or_group in &uglist {
+            for role in &rolelist {
+                if !access_conf().roles().contains_key(role) {
+                    bail!("unknown role '{}'", role);
+                }
+                if let Some(group) = user_or_group.strip_prefix('@') {
+                    node.insert_group_role(group.to_string(), role.to_string(), propagate);
+                } else {
+                    node.insert_user_role(user_or_group.parse()?, role.to_string(), propagate);
+                }
+            }
+        }
+
+        Ok(())
+    }
+
+    /// This is used for testing
+    pub fn from_raw(raw: &str) -> Result<Self, Error> {
+        let mut tree = Self::new();
+        for (linenr, line) in raw.lines().enumerate() {
+            let line = line.trim();
+            if line.is_empty() {
+                continue;
+            }
+            if let Err(err) = tree.parse_acl_line(line) {
+                bail!(
+                    "unable to parse acl config data, line {} - {}",
+                    linenr + 1,
+                    err
+                );
+            }
+        }
+        Ok(tree)
+    }
+
+    /// Returns a map of role name and propagation status for a given `auth_id` and `path`.
+    ///
+    /// This will collect role mappings according to the following algorithm:
+    /// - iterate over all intermediate nodes along `path` and collect roles with `propagate` set
+    /// - get all (propagating and non-propagating) roles for last component of path
+    /// - more specific role maps replace less specific role maps
+    ///   -- user/token is more specific than group at each level
+    ///   -- roles lower in the tree are more specific than those higher up along the path
+    pub fn roles(&self, auth_id: &Authid, path: &[&str]) -> HashMap<String, bool> {
+        let mut node = &self.root;
+        let mut role_map = node.extract_roles(auth_id, path.is_empty());
+
+        let mut comp_iter = path.iter().peekable();
+
+        while let Some(comp) = comp_iter.next() {
+            let last_comp = comp_iter.peek().is_none();
+
+            let mut sub_comp_iter = comp.split('/').peekable();
+
+            while let Some(sub_comp) = sub_comp_iter.next() {
+                let last_sub_comp = last_comp && sub_comp_iter.peek().is_none();
+
+                node = match node.children.get(sub_comp) {
+                    Some(n) => n,
+                    None => return role_map, // path not found
+                };
+
+                let new_map = node.extract_roles(auth_id, last_sub_comp);
+                if !new_map.is_empty() {
+                    // overwrite previous mappings
+                    role_map = new_map;
+                }
+            }
+        }
+
+        role_map
+    }
+
+    pub fn get_child_paths(&self, auth_id: &Authid, path: &[&str]) -> Result<Vec<String>, Error> {
+        let mut res = Vec::new();
+
+        if let Some(node) = self.get_node(path) {
+            let path = path.join("/");
+            node.get_child_paths(path, auth_id, &mut res)?;
+        }
+
+        Ok(res)
+    }
+}
+
+#[cfg(feature = "impl")]
+impl AclTree {
     fn write_node_config(node: &AclTreeNode, path: &str, w: &mut dyn Write) -> Result<(), Error> {
         let mut role_ug_map0: HashMap<_, BTreeSet<_>> = HashMap::new();
         let mut role_ug_map1: HashMap<_, BTreeSet<_>> = HashMap::new();
@@ -406,49 +523,6 @@ impl AclTree {
         Self::write_node_config(&self.root, "", w)
     }
 
-    fn parse_acl_line(&mut self, line: &str) -> Result<(), Error> {
-        let items: Vec<&str> = line.split(':').collect();
-
-        if items.len() != 5 {
-            bail!("wrong number of items.");
-        }
-
-        if items[0] != "acl" {
-            bail!("line does not start with 'acl'.");
-        }
-
-        let propagate = if items[1] == "0" {
-            false
-        } else if items[1] == "1" {
-            true
-        } else {
-            bail!("expected '0' or '1' for propagate flag.");
-        };
-
-        let path_str = items[2];
-        let path = split_acl_path(path_str);
-        let node = self.get_or_insert_node(&path);
-
-        let uglist: Vec<&str> = items[3].split(',').map(|v| v.trim()).collect();
-
-        let rolelist: Vec<&str> = items[4].split(',').map(|v| v.trim()).collect();
-
-        for user_or_group in &uglist {
-            for role in &rolelist {
-                if !access_conf().roles().contains_key(role) {
-                    bail!("unknown role '{}'", role);
-                }
-                if let Some(group) = user_or_group.strip_prefix('@') {
-                    node.insert_group_role(group.to_string(), role.to_string(), propagate);
-                } else {
-                    node.insert_user_role(user_or_group.parse()?, role.to_string(), propagate);
-                }
-            }
-        }
-
-        Ok(())
-    }
-
     fn load(filename: &Path) -> Result<(Self, ConfigDigest), Error> {
         let mut tree = Self::new();
 
@@ -482,156 +556,106 @@ impl AclTree {
 
         Ok((tree, digest))
     }
+}
 
-    /// This is used for testing
-    pub fn from_raw(raw: &str) -> Result<Self, Error> {
-        let mut tree = Self::new();
-        for (linenr, line) in raw.lines().enumerate() {
-            let line = line.trim();
-            if line.is_empty() {
-                continue;
-            }
-            if let Err(err) = tree.parse_acl_line(line) {
-                bail!(
-                    "unable to parse acl config data, line {} - {}",
-                    linenr + 1,
-                    err
-                );
-            }
-        }
-        Ok(tree)
+#[cfg(feature = "impl")]
+pub use impl_feature::{cached_config, config, lock_config, save_config};
+
+#[cfg(feature = "impl")]
+mod impl_feature {
+    use std::sync::{Arc, OnceLock, RwLock};
+
+    use anyhow::{bail, Error};
+
+    use proxmox_config_digest::ConfigDigest;
+    use proxmox_product_config::{open_api_lockfile, replace_privileged_config, ApiLockGuard};
+
+    use crate::acl::AclTree;
+    use crate::init::access_conf;
+    use crate::init::impl_feature::{acl_config, acl_config_lock};
+
+    /// Get exclusive lock
+    pub fn lock_config() -> Result<ApiLockGuard, Error> {
+        open_api_lockfile(acl_config_lock(), None, true)
     }
 
-    /// Returns a map of role name and propagation status for a given `auth_id` and `path`.
+    /// Reads the [`AclTree`] from `acl.cfg` in the configuration directory.
+    pub fn config() -> Result<(AclTree, ConfigDigest), Error> {
+        let path = acl_config();
+        AclTree::load(&path)
+    }
+
+    /// Returns a cached [`AclTree`] or a fresh copy read directly from `acl.cfg` in the configuration
+    /// directory.
     ///
-    /// This will collect role mappings according to the following algorithm:
-    /// - iterate over all intermediate nodes along `path` and collect roles with `propagate` set
-    /// - get all (propagating and non-propagating) roles for last component of path
-    /// - more specific role maps replace less specific role maps
-    ///   -- user/token is more specific than group at each level
-    ///   -- roles lower in the tree are more specific than those higher up along the path
-    pub fn roles(&self, auth_id: &Authid, path: &[&str]) -> HashMap<String, bool> {
-        let mut node = &self.root;
-        let mut role_map = node.extract_roles(auth_id, path.is_empty());
-
-        let mut comp_iter = path.iter().peekable();
-
-        while let Some(comp) = comp_iter.next() {
-            let last_comp = comp_iter.peek().is_none();
-
-            let mut sub_comp_iter = comp.split('/').peekable();
-
-            while let Some(sub_comp) = sub_comp_iter.next() {
-                let last_sub_comp = last_comp && sub_comp_iter.peek().is_none();
-
-                node = match node.children.get(sub_comp) {
-                    Some(n) => n,
-                    None => return role_map, // path not found
-                };
-
-                let new_map = node.extract_roles(auth_id, last_sub_comp);
-                if !new_map.is_empty() {
-                    // overwrite previous mappings
-                    role_map = new_map;
-                }
-            }
+    /// Since the AclTree is used for every API request's permission check, this caching mechanism
+    /// allows to skip reading and parsing the file again if it is unchanged.
+    pub fn cached_config() -> Result<Arc<AclTree>, Error> {
+        struct ConfigCache {
+            data: Option<Arc<AclTree>>,
+            last_mtime: i64,
+            last_mtime_nsec: i64,
         }
 
-        role_map
-    }
+        static CACHED_CONFIG: OnceLock<RwLock<ConfigCache>> = OnceLock::new();
+        let cached_conf = CACHED_CONFIG.get_or_init(|| {
+            RwLock::new(ConfigCache {
+                data: None,
+                last_mtime: 0,
+                last_mtime_nsec: 0,
+            })
+        });
 
-    pub fn get_child_paths(&self, auth_id: &Authid, path: &[&str]) -> Result<Vec<String>, Error> {
-        let mut res = Vec::new();
+        let conf = acl_config();
+        let stat = match nix::sys::stat::stat(&conf) {
+            Ok(stat) => Some(stat),
+            Err(nix::errno::Errno::ENOENT) => None,
+            Err(err) => bail!("unable to stat '{}' - {err}", conf.display()),
+        };
 
-        if let Some(node) = self.get_node(path) {
-            let path = path.join("/");
-            node.get_child_paths(path, auth_id, &mut res)?;
-        }
-
-        Ok(res)
-    }
-}
-
-/// Get exclusive lock
-pub fn lock_config() -> Result<ApiLockGuard, Error> {
-    open_api_lockfile(acl_config_lock(), None, true)
-}
-
-/// Reads the [`AclTree`] from `acl.cfg` in the configuration directory.
-pub fn config() -> Result<(AclTree, ConfigDigest), Error> {
-    let path = acl_config();
-    AclTree::load(&path)
-}
-
-/// Returns a cached [`AclTree`] or a fresh copy read directly from `acl.cfg` in the configuration
-/// directory.
-///
-/// Since the AclTree is used for every API request's permission check, this caching mechanism
-/// allows to skip reading and parsing the file again if it is unchanged.
-pub fn cached_config() -> Result<Arc<AclTree>, Error> {
-    struct ConfigCache {
-        data: Option<Arc<AclTree>>,
-        last_mtime: i64,
-        last_mtime_nsec: i64,
-    }
-
-    static CACHED_CONFIG: OnceLock<RwLock<ConfigCache>> = OnceLock::new();
-    let cached_conf = CACHED_CONFIG.get_or_init(|| {
-        RwLock::new(ConfigCache {
-            data: None,
-            last_mtime: 0,
-            last_mtime_nsec: 0,
-        })
-    });
-
-    let conf = acl_config();
-    let stat = match nix::sys::stat::stat(&conf) {
-        Ok(stat) => Some(stat),
-        Err(nix::errno::Errno::ENOENT) => None,
-        Err(err) => bail!("unable to stat '{}' - {err}", conf.display()),
-    };
-
-    {
-        // limit scope
-        let cache = cached_conf.read().unwrap();
-        if let Some(ref config) = cache.data {
-            if let Some(stat) = stat {
-                if stat.st_mtime == cache.last_mtime && stat.st_mtime_nsec == cache.last_mtime_nsec
-                {
+        {
+            // limit scope
+            let cache = cached_conf.read().unwrap();
+            if let Some(ref config) = cache.data {
+                if let Some(stat) = stat {
+                    if stat.st_mtime == cache.last_mtime
+                        && stat.st_mtime_nsec == cache.last_mtime_nsec
+                    {
+                        return Ok(config.clone());
+                    }
+                } else if cache.last_mtime == 0 && cache.last_mtime_nsec == 0 {
                     return Ok(config.clone());
                 }
-            } else if cache.last_mtime == 0 && cache.last_mtime_nsec == 0 {
-                return Ok(config.clone());
             }
         }
+
+        let (config, _digest) = config()?;
+        let config = Arc::new(config);
+
+        let mut cache = cached_conf.write().unwrap();
+        if let Some(stat) = stat {
+            cache.last_mtime = stat.st_mtime;
+            cache.last_mtime_nsec = stat.st_mtime_nsec;
+        }
+        cache.data = Some(config.clone());
+
+        Ok(config)
     }
 
-    let (config, _digest) = config()?;
-    let config = Arc::new(config);
+    /// Saves an [`AclTree`] to `acl.cfg` in the configuration directory, ensuring proper ownership and
+    /// file permissions.
+    pub fn save_config(acl: &AclTree) -> Result<(), Error> {
+        let mut raw: Vec<u8> = Vec::new();
+        acl.write_config(&mut raw)?;
 
-    let mut cache = cached_conf.write().unwrap();
-    if let Some(stat) = stat {
-        cache.last_mtime = stat.st_mtime;
-        cache.last_mtime_nsec = stat.st_mtime_nsec;
+        let conf = acl_config();
+        replace_privileged_config(conf, &raw)?;
+
+        // increase cache generation so we reload it next time we access it
+        access_conf().increment_cache_generation()?;
+
+        Ok(())
     }
-    cache.data = Some(config.clone());
-
-    Ok(config)
-}
-
-/// Saves an [`AclTree`] to `acl.cfg` in the configuration directory, ensuring proper ownership and
-/// file permissions.
-pub fn save_config(acl: &AclTree) -> Result<(), Error> {
-    let mut raw: Vec<u8> = Vec::new();
-    acl.write_config(&mut raw)?;
-
-    let conf = acl_config();
-    replace_privileged_config(conf, &raw)?;
-
-    // increase cache generation so we reload it next time we access it
-    access_conf().increment_cache_generation()?;
-
-    Ok(())
 }
 
 #[cfg(test)]
diff --git a/proxmox-access-control/src/init.rs b/proxmox-access-control/src/init.rs
index 39a12352..e64398e8 100644
--- a/proxmox-access-control/src/init.rs
+++ b/proxmox-access-control/src/init.rs
@@ -1,5 +1,4 @@
 use std::collections::HashMap;
-use std::path::{Path, PathBuf};
 use std::sync::OnceLock;
 
 use anyhow::{format_err, Error};
@@ -8,7 +7,6 @@ use proxmox_auth_api::types::{Authid, Userid};
 use proxmox_section_config::SectionConfigData;
 
 static ACCESS_CONF: OnceLock<&'static dyn AccessControlConfig> = OnceLock::new();
-static ACCESS_CONF_DIR: OnceLock<PathBuf> = OnceLock::new();
 
 /// This trait specifies the functions a product needs to implement to get ACL tree based access
 /// control management from this plugin.
@@ -105,21 +103,7 @@ pub trait AccessControlConfig: Send + Sync {
     }
 }
 
-pub fn init<P: AsRef<Path>>(
-    acm_config: &'static dyn AccessControlConfig,
-    config_dir: P,
-) -> Result<(), Error> {
-    init_access_config(acm_config)?;
-    init_access_config_dir(config_dir)
-}
-
-pub(crate) fn init_access_config_dir<P: AsRef<Path>>(config_dir: P) -> Result<(), Error> {
-    ACCESS_CONF_DIR
-        .set(config_dir.as_ref().to_owned())
-        .map_err(|_e| format_err!("cannot initialize acl tree config twice!"))
-}
-
-pub(crate) fn init_access_config(config: &'static dyn AccessControlConfig) -> Result<(), Error> {
+pub fn init_access_config(config: &'static dyn AccessControlConfig) -> Result<(), Error> {
     ACCESS_CONF
         .set(config)
         .map_err(|_e| format_err!("cannot initialize acl tree config twice!"))
@@ -131,32 +115,61 @@ pub(crate) fn access_conf() -> &'static dyn AccessControlConfig {
         .expect("please initialize the acm config before using it!")
 }
 
-fn conf_dir() -> &'static PathBuf {
-    ACCESS_CONF_DIR
-        .get()
-        .expect("please initialize acm config dir before using it!")
-}
+#[cfg(feature = "impl")]
+pub use impl_feature::init;
 
-pub(crate) fn acl_config() -> PathBuf {
-    conf_dir().join("acl.cfg")
-}
+#[cfg(feature = "impl")]
+pub(crate) mod impl_feature {
+    use std::path::{Path, PathBuf};
+    use std::sync::OnceLock;
 
-pub(crate) fn acl_config_lock() -> PathBuf {
-    conf_dir().join(".acl.lck")
-}
+    use anyhow::{format_err, Error};
 
-pub(crate) fn user_config() -> PathBuf {
-    conf_dir().join("user.cfg")
-}
+    use crate::init::{init_access_config, AccessControlConfig};
 
-pub(crate) fn user_config_lock() -> PathBuf {
-    conf_dir().join(".user.lck")
-}
+    static ACCESS_CONF_DIR: OnceLock<PathBuf> = OnceLock::new();
 
-pub(crate) fn token_shadow() -> PathBuf {
-    conf_dir().join("token.shadow")
-}
+    pub fn init<P: AsRef<Path>>(
+        acm_config: &'static dyn AccessControlConfig,
+        config_dir: P,
+    ) -> Result<(), Error> {
+        init_access_config(acm_config)?;
+        init_access_config_dir(config_dir)
+    }
 
-pub(crate) fn token_shadow_lock() -> PathBuf {
-    conf_dir().join("token.shadow.lock")
+    pub(crate) fn init_access_config_dir<P: AsRef<Path>>(config_dir: P) -> Result<(), Error> {
+        ACCESS_CONF_DIR
+            .set(config_dir.as_ref().to_owned())
+            .map_err(|_e| format_err!("cannot initialize acl tree config twice!"))
+    }
+
+    fn conf_dir() -> &'static PathBuf {
+        ACCESS_CONF_DIR
+            .get()
+            .expect("please initialize acm config dir before using it!")
+    }
+
+    pub(crate) fn acl_config() -> PathBuf {
+        conf_dir().join("acl.cfg")
+    }
+
+    pub(crate) fn acl_config_lock() -> PathBuf {
+        conf_dir().join(".acl.lck")
+    }
+
+    pub(crate) fn user_config() -> PathBuf {
+        conf_dir().join("user.cfg")
+    }
+
+    pub(crate) fn user_config_lock() -> PathBuf {
+        conf_dir().join(".user.lck")
+    }
+
+    pub(crate) fn token_shadow() -> PathBuf {
+        conf_dir().join("token.shadow")
+    }
+
+    pub(crate) fn token_shadow_lock() -> PathBuf {
+        conf_dir().join("token.shadow.lock")
+    }
 }
diff --git a/proxmox-access-control/src/lib.rs b/proxmox-access-control/src/lib.rs
index 62683924..9195c999 100644
--- a/proxmox-access-control/src/lib.rs
+++ b/proxmox-access-control/src/lib.rs
@@ -2,13 +2,13 @@
 
 pub mod types;
 
-#[cfg(feature = "impl")]
+#[cfg(feature = "acl")]
 pub mod acl;
 
 #[cfg(feature = "api")]
 pub mod api;
 
-#[cfg(feature = "impl")]
+#[cfg(feature = "acl")]
 pub mod init;
 
 #[cfg(feature = "impl")]
diff --git a/proxmox-access-control/src/token_shadow.rs b/proxmox-access-control/src/token_shadow.rs
index 373910f3..c586d834 100644
--- a/proxmox-access-control/src/token_shadow.rs
+++ b/proxmox-access-control/src/token_shadow.rs
@@ -6,7 +6,7 @@ use serde_json::{from_value, Value};
 use proxmox_auth_api::types::Authid;
 use proxmox_product_config::{open_api_lockfile, replace_config, ApiLockGuard};
 
-use crate::init::{token_shadow, token_shadow_lock};
+use crate::init::impl_feature::{token_shadow, token_shadow_lock};
 
 // Get exclusive lock
 fn lock_config() -> Result<ApiLockGuard, Error> {
diff --git a/proxmox-access-control/src/user.rs b/proxmox-access-control/src/user.rs
index 95b70f25..a4b59edc 100644
--- a/proxmox-access-control/src/user.rs
+++ b/proxmox-access-control/src/user.rs
@@ -9,7 +9,8 @@ use proxmox_product_config::{open_api_lockfile, replace_privileged_config, ApiLo
 use proxmox_schema::*;
 use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
 
-use crate::init::{access_conf, user_config, user_config_lock};
+use crate::init::access_conf;
+use crate::init::impl_feature::{user_config, user_config_lock};
 use crate::types::{ApiToken, User};
 
 fn get_or_init_config() -> &'static SectionConfig {
-- 
2.47.3



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel