[pdm-devel] [PATCH datacenter-manager 2/3] views: add 'include-all' param; change semantics when there are no includes

[Lukas Wagner <l.wagner@proxmox.com>]

Previously, having no 'include' rules would mean that *all* resources
are included. Due view permissions being transitively applied to all
included resources, this also has some security consequences.
For instance, if there is only a single include rule which is then
removed by accident, suddenly any AuthID having privileges on the view
ACL object would then be granted these privileges on *all* resources.

To somewhat mitigate this, the semantics are slightly changed. If there
are no include rules, then no resources will be contained in the view.
A new 'include-all' parameter is added, which aims to emphasize the
intent of *really* including everything.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
 lib/pdm-api-types/src/views.rs |  4 +++
 server/src/views/mod.rs        | 60 +++++++++++++++++++---------------
 server/src/views/tests.rs      | 35 ++++++++++++++++++--
 3 files changed, 69 insertions(+), 30 deletions(-)

diff --git a/lib/pdm-api-types/src/views.rs b/lib/pdm-api-types/src/views.rs
index 950a3a04..82ab8781 100644
--- a/lib/pdm-api-types/src/views.rs
+++ b/lib/pdm-api-types/src/views.rs
@@ -58,6 +58,9 @@ pub struct ViewConfig {
     #[updater(skip)]
     pub id: String,
 
+    /// Include all resources by default.
+    pub include_all: Option<bool>,
+
     /// List of includes.
     #[serde(default, skip_serializing_if = "Vec::is_empty")]
     #[updater(serde(skip_serializing_if = "Option::is_none"))]
@@ -261,6 +264,7 @@ mod test {
     fn config_smoke_test() {
         let config = "
 view: some-view
+    include-all true
     include exact:remote=someremote
     include remote=someremote
     include resource-type=qemu
diff --git a/server/src/views/mod.rs b/server/src/views/mod.rs
index 8b4c70a4..3669ace4 100644
--- a/server/src/views/mod.rs
+++ b/server/src/views/mod.rs
@@ -64,31 +64,34 @@ impl View {
     /// When there are `include remote:<...>` or `exclude remote:<...>` rules, we can use these to
     /// check if a remote needs to be considered at all.
     pub fn can_skip_remote(&self, remote: &str) -> bool {
-        let mut has_any_include_remote = false;
-        let mut matches_any_include_remote = false;
-
-        let mut any_other = false;
-
-        for include in &self.config.include {
-            if let FilterRule::Remote(r) = include {
-                has_any_include_remote = true;
-                if r.matches(remote) {
-                    matches_any_include_remote = true;
-                    break;
-                }
-            } else {
-                any_other = true;
-            }
-        }
-
         let matches_any_exclude_remote = self
             .config
             .exclude
             .iter()
             .any(|rule| Self::matches_remote_rule(remote, rule));
 
-        (has_any_include_remote && !matches_any_include_remote && !any_other)
-            || matches_any_exclude_remote
+        if matches_any_exclude_remote {
+            return true;
+        }
+
+        if self.config.include_all.unwrap_or_default() {
+            return false;
+        }
+
+        for include in &self.config.include {
+            if let FilterRule::Remote(r) = include {
+                if r.matches(remote) {
+                    return false;
+                }
+            } else {
+                // If there is any other type of rule, we cannot safely infer whether we can skip
+                // the remote (e.g. for 'tag' matches, we have to check *all* remotes for resources
+                // with a given tag)
+                return false;
+            }
+        }
+
+        true
     }
 
     /// Check if a remote is *explicitly* included (and not excluded).
@@ -96,18 +99,22 @@ impl View {
     /// A subset of the resources of a remote might still be pulled in by other rules,
     /// but this function check if the remote as a whole is matched.
     pub fn is_remote_explicitly_included(&self, remote: &str) -> bool {
-        let matches_include_remote = self
-            .config
-            .include
-            .iter()
-            .any(|rule| Self::matches_remote_rule(remote, rule));
+        let included = if self.config.include_all.unwrap_or_default() {
+            true
+        } else {
+            self.config
+                .include
+                .iter()
+                .any(|rule| Self::matches_remote_rule(remote, rule))
+        };
+
         let matches_exclude_remote = self
             .config
             .exclude
             .iter()
             .any(|rule| Self::matches_remote_rule(remote, rule));
 
-        matches_include_remote && !matches_exclude_remote
+        included && !matches_exclude_remote
     }
 
     /// Check if a node is matched by the filter rules.
@@ -131,8 +138,7 @@ impl View {
     }
 
     fn check_if_included(&self, remote: &str, resource: &ResourceData) -> bool {
-        if self.config.include.is_empty() {
-            // If there are no include rules, any resource is included (unless excluded)
+        if self.config.include_all.unwrap_or_default() {
             return true;
         }
 
diff --git a/server/src/views/tests.rs b/server/src/views/tests.rs
index 14d94ac9..0d83ae70 100644
--- a/server/src/views/tests.rs
+++ b/server/src/views/tests.rs
@@ -135,6 +135,7 @@ fn exclude_remotes() {
             FilterRule::Remote(StringMatcher::Exact("remote-a".into())),
             FilterRule::Remote(StringMatcher::Exact("remote-b".into())),
         ],
+        include_all: Some(true),
         ..Default::default()
     };
 
@@ -184,6 +185,8 @@ fn include_exclude_remotes() {
             FilterRule::Remote(StringMatcher::Exact("remote-b".into())),
             FilterRule::Remote(StringMatcher::Exact("remote-c".into())),
         ],
+
+        ..Default::default()
     };
     run_test(
         config.clone(),
@@ -224,6 +227,7 @@ fn include_exclude_remotes() {
 fn empty_config() {
     let config = ViewConfig {
         id: "empty".into(),
+        include_all: Some(true),
         ..Default::default()
     };
     run_test(
@@ -301,6 +305,7 @@ fn exclude_type() {
                 FilterRule::ResourceType(EnumMatcher(ResourceType::PveStorage)),
                 FilterRule::ResourceType(EnumMatcher(ResourceType::PveQemu)),
             ],
+            include_all: Some(true),
             ..Default::default()
         },
         &[
@@ -329,6 +334,7 @@ fn include_exclude_type() {
             exclude: vec![FilterRule::ResourceType(EnumMatcher(
                 ResourceType::PveStorage,
             ))],
+            ..Default::default()
         },
         &[
             (
@@ -357,6 +363,7 @@ fn include_exclude_tags() {
                 FilterRule::Tag(StringMatcher::Exact("tag2".to_string())),
             ],
             exclude: vec![FilterRule::Tag(StringMatcher::Exact("tag3".to_string()))],
+            ..Default::default()
         },
         &[
             (
@@ -404,6 +411,7 @@ fn include_exclude_resource_pool() {
             exclude: vec![FilterRule::ResourcePool(StringMatcher::Exact(
                 "pool2".to_string(),
             ))],
+            ..Default::default()
         },
         &[
             (
@@ -459,6 +467,7 @@ fn include_exclude_resource_id() {
                     "remote/{REMOTE}/storage/{NODE}/otherstorage"
                 ))),
             ],
+            ..Default::default()
         },
         &[
             (
@@ -509,6 +518,7 @@ fn node_included() {
         exclude: vec![FilterRule::Remote(StringMatcher::Exact(
             "remote-b".to_string(),
         ))],
+        ..Default::default()
     });
 
     assert!(view.is_node_included("remote-a", "somenode"));
@@ -528,6 +538,7 @@ fn can_skip_remote_if_excluded() {
         exclude: vec![FilterRule::Remote(StringMatcher::Exact(
             "remote-b".to_string(),
         ))],
+        include_all: Some(true),
     });
 
     assert!(!view.can_skip_remote("remote-a"));
@@ -542,6 +553,7 @@ fn can_skip_remote_if_included() {
             "remote-b".to_string(),
         ))],
         exclude: vec![],
+        ..Default::default()
     });
 
     assert!(!view.can_skip_remote("remote-b"));
@@ -559,6 +571,7 @@ fn can_skip_remote_cannot_skip_if_any_other_include() {
             )),
         ],
         exclude: vec![],
+        ..Default::default()
     });
 
     assert!(!view.can_skip_remote("remote-b"));
@@ -575,6 +588,7 @@ fn can_skip_remote_explicit_remote_exclude() {
         exclude: vec![FilterRule::Remote(StringMatcher::Exact(
             "remote-a".to_string(),
         ))],
+        ..Default::default()
     });
 
     assert!(view.can_skip_remote("remote-a"));
@@ -584,8 +598,19 @@ fn can_skip_remote_explicit_remote_exclude() {
 fn can_skip_remote_with_empty_config() {
     let view = View::new(ViewConfig {
         id: "abc".into(),
-        include: vec![],
-        exclude: vec![],
+        ..Default::default()
+    });
+
+    assert!(view.can_skip_remote("remote-a"));
+    assert!(view.can_skip_remote("remote-b"));
+}
+
+#[test]
+fn can_skip_remote_cannot_skip_if_all_included() {
+    let view = View::new(ViewConfig {
+        id: "abc".into(),
+        include_all: Some(true),
+        ..Default::default()
     });
 
     assert!(!view.can_skip_remote("remote-a"));
@@ -600,6 +625,7 @@ fn can_skip_remote_with_no_remote_includes() {
             "resource/remote-a/guest/100".to_string(),
         ))],
         exclude: vec![],
+        ..Default::default()
     });
 
     assert!(!view.can_skip_remote("remote-a"));
@@ -614,6 +640,7 @@ fn explicitly_included_remote() {
             "remote-b".to_string(),
         ))],
         exclude: vec![],
+        ..Default::default()
     });
 
     assert!(view.is_remote_explicitly_included("remote-b"));
@@ -629,6 +656,7 @@ fn included_and_excluded_same_remote() {
         exclude: vec![FilterRule::Remote(StringMatcher::Exact(
             "remote-b".to_string(),
         ))],
+        ..Default::default()
     });
 
     assert!(!view.is_remote_explicitly_included("remote-b"));
@@ -640,8 +668,9 @@ fn not_explicitly_included_remote() {
         id: "abc".into(),
         include: vec![],
         exclude: vec![],
+        include_all: Some(true),
     });
 
     // Assert that is not *explicitly* included
-    assert!(!view.is_remote_explicitly_included("remote-b"));
+    assert!(view.is_remote_explicitly_included("remote-b"));
 }
-- 
2.47.3



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