From 1a7b45ec263a8593b3ea27ebfb8da7d9d4e3140d Mon Sep 17 00:00:00 2001 From: Duansg Date: Sun, 9 Aug 2026 20:46:58 +0800 Subject: [PATCH] [fix] restrict monitoring template writes to admin (#4278) --- .../src/test/resources/sureness.yml | 6 + .../src/test/resources/sureness.yml | 6 + .../src/main/resources/sureness.yml | 6 + .../security/SurenessResourceRuleTest.java | 104 ++++++++++++++---- .../hertzbeat-mysql-iotdb/conf/sureness.yml | 6 + .../conf/sureness.yml | 6 + .../conf/sureness.yml | 6 + .../conf/sureness.yml | 6 + .../conf/sureness.yml | 6 + script/sureness.yml | 6 + 10 files changed, 137 insertions(+), 21 deletions(-) diff --git a/hertzbeat-e2e/hertzbeat-log-e2e/src/test/resources/sureness.yml b/hertzbeat-e2e/hertzbeat-log-e2e/src/test/resources/sureness.yml index 6f2a7dbb36..1763868f1e 100644 --- a/hertzbeat-e2e/hertzbeat-log-e2e/src/test/resources/sureness.yml +++ b/hertzbeat-e2e/hertzbeat-log-e2e/src/test/resources/sureness.yml @@ -22,6 +22,12 @@ resourceRole: - /api/account/auth/refresh===post===[admin,user,guest] - /api/apps/**===get===[admin,user,guest] + # the define yml routes persist the global collection templates + # post can only add a new type, put overwrites an existing one and immediately + # redispatches its collect job to every monitor already using it - hence admin only + - /api/apps/**===post===[admin,user] + - /api/apps/**===put===[admin] + - /api/apps/**===delete===[admin] - /api/monitor/**===get===[admin,user,guest] - /api/monitor/**===post===[admin,user] - /api/monitor/**===put===[admin,user] diff --git a/hertzbeat-manager/src/test/resources/sureness.yml b/hertzbeat-manager/src/test/resources/sureness.yml index 6f2a7dbb36..1763868f1e 100644 --- a/hertzbeat-manager/src/test/resources/sureness.yml +++ b/hertzbeat-manager/src/test/resources/sureness.yml @@ -22,6 +22,12 @@ resourceRole: - /api/account/auth/refresh===post===[admin,user,guest] - /api/apps/**===get===[admin,user,guest] + # the define yml routes persist the global collection templates + # post can only add a new type, put overwrites an existing one and immediately + # redispatches its collect job to every monitor already using it - hence admin only + - /api/apps/**===post===[admin,user] + - /api/apps/**===put===[admin] + - /api/apps/**===delete===[admin] - /api/monitor/**===get===[admin,user,guest] - /api/monitor/**===post===[admin,user] - /api/monitor/**===put===[admin,user] diff --git a/hertzbeat-startup/src/main/resources/sureness.yml b/hertzbeat-startup/src/main/resources/sureness.yml index 04c9b3b739..da66fe4cea 100644 --- a/hertzbeat-startup/src/main/resources/sureness.yml +++ b/hertzbeat-startup/src/main/resources/sureness.yml @@ -22,6 +22,12 @@ resourceRole: - /api/account/auth/refresh===post===[admin,user,guest] - /api/apps/**===get===[admin,user,guest] + # the define yml routes persist the global collection templates + # post can only add a new type, put overwrites an existing one and immediately + # redispatches its collect job to every monitor already using it - hence admin only + - /api/apps/**===post===[admin,user] + - /api/apps/**===put===[admin] + - /api/apps/**===delete===[admin] - /api/monitor/**===get===[admin,user,guest] - /api/monitor/**===post===[admin,user] - /api/monitor/**===put===[admin,user] diff --git a/hertzbeat-startup/src/test/java/org/apache/hertzbeat/startup/security/SurenessResourceRuleTest.java b/hertzbeat-startup/src/test/java/org/apache/hertzbeat/startup/security/SurenessResourceRuleTest.java index 40fba97784..17301b30b7 100644 --- a/hertzbeat-startup/src/test/java/org/apache/hertzbeat/startup/security/SurenessResourceRuleTest.java +++ b/hertzbeat-startup/src/test/java/org/apache/hertzbeat/startup/security/SurenessResourceRuleTest.java @@ -20,6 +20,7 @@ package org.apache.hertzbeat.startup.security; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assumptions.assumeTrue; import com.usthe.sureness.matcher.util.TirePathTree; import java.io.IOException; import java.io.InputStream; @@ -53,6 +54,8 @@ class SurenessResourceRuleTest { private static final int EXCLUDED_RESOURCE_SEGMENTS = 2; + private static final Path SCRIPT_DIR = Path.of("..", "script"); + private static List resourceRole; private static List excludedResource; @@ -114,6 +117,38 @@ class SurenessResourceRuleTest { assertEquals("[admin,user,guest]", roleTree.searchPathFilterRoles("/api/monitors" + SEPARATOR + "get")); } + /** + * Only the {@code get} verb under {@code /api/apps} used to be listed, so the monitoring + * template writes behind {@code AppController} reached any authenticated caller, down to + * a {@code guest}. + * + *

The write verbs are not equally dangerous, and the rules deliberately differ. + * {@code post} refuses to overwrite an existing template, so it only adds a type that + * nobody is obliged to use, which puts it on the same footing as creating a monitor. + * {@code put} overwrites any template including the built-in ones and then hands the new + * definition to {@code updateAppCollectJob}, retroactively changing what every existing + * monitor of that type collects for every account, so it stays admin only. + */ + @Test + void addingMonitorTemplatesIsOpenToUsers() { + assertEquals("[admin,user]", roleTree.searchPathFilterRoles("/api/apps/define/yml" + SEPARATOR + "post")); + } + + @Test + void overwritingMonitorTemplatesIsRestrictedToAdmin() { + assertEquals("[admin]", roleTree.searchPathFilterRoles("/api/apps/define/yml" + SEPARATOR + "put")); + } + + @Test + void deletingMonitorTemplatesIsRestrictedToAdmin() { + assertEquals("[admin]", roleTree.searchPathFilterRoles("/api/apps/linux/define/yml" + SEPARATOR + "delete")); + } + + @Test + void readingMonitorTemplatesStaysOpenToEveryRole() { + assertEquals("[admin,user,guest]", roleTree.searchPathFilterRoles("/api/apps/linux/define/yml" + SEPARATOR + "get")); + } + /** * The deployment scripts ship their own copies of {@code sureness.yml}; a rule fixed only * in the packaged file would still leave every container deployment exposed. @@ -127,28 +162,10 @@ class SurenessResourceRuleTest { * wave through. */ @Test - @SuppressWarnings("unchecked") void deploymentCopiesCarryWellFormedRules() throws IOException { - Path scriptDir = Path.of("..", "script"); - if (!Files.isDirectory(scriptDir)) { - // running outside the source tree, the packaged file asserted above is all we can see - return; - } - Set copies; - try (Stream paths = Files.walk(scriptDir)) { - copies = paths.filter(path -> path.getFileName().toString().equals("sureness.yml")) - .collect(Collectors.toCollection(LinkedHashSet::new)); - } - assertFalse(copies.isEmpty(), "expected the deployment scripts to ship sureness.yml copies"); - for (Path copy : copies) { - Map document; - try (InputStream in = Files.newInputStream(copy)) { - document = new Yaml().load(in); - } - List copyResourceRole = (List) document.get("resourceRole"); - List copyExcludedResource = (List) document.get("excludedResource"); - assertNotNull(copyResourceRole, "resourceRole must be present in " + copy); - assertNotNull(copyExcludedResource, "excludedResource must be present in " + copy); + for (Path copy : deploymentCopies()) { + List copyResourceRole = sectionOf(copy, "resourceRole"); + List copyExcludedResource = sectionOf(copy, "excludedResource"); for (String rule : copyResourceRole) { assertEquals(RESOURCE_ROLE_SEGMENTS, rule.split(SEPARATOR, -1).length, @@ -162,4 +179,49 @@ class SurenessResourceRuleTest { } } } + + /** + * Well formed is not the same as correct: a copy that simply never listed the write verbs + * passes every shape check above while leaving the monitoring template writes unruled. The + * copies are edited by hand, one per deployment flavour, so the roles themselves are pinned + * here too and a missed copy fails instead of shipping. + */ + @Test + void deploymentCopiesRestrictMonitorTemplateWrites() throws IOException { + for (Path copy : deploymentCopies()) { + TirePathTree copyTree = new TirePathTree(); + copyTree.buildTree(new LinkedHashSet<>(sectionOf(copy, "resourceRole"))); + assertEquals("[admin,user]", copyTree.searchPathFilterRoles("/api/apps/define/yml" + SEPARATOR + "post"), + "adding a monitoring template is unruled or over-granted in " + copy); + assertEquals("[admin]", copyTree.searchPathFilterRoles("/api/apps/define/yml" + SEPARATOR + "put"), + "overwriting a monitoring template is unruled or over-granted in " + copy); + assertEquals("[admin]", copyTree.searchPathFilterRoles("/api/apps/linux/define/yml" + SEPARATOR + "delete"), + "deleting a monitoring template is unruled or over-granted in " + copy); + } + } + + /** + * @return the {@code sureness.yml} copies shipped by the deployment scripts + */ + private static Set deploymentCopies() throws IOException { + assumeTrue(Files.isDirectory(SCRIPT_DIR), + "running outside the source tree, the packaged file asserted above is all we can see"); + try (Stream paths = Files.walk(SCRIPT_DIR)) { + Set copies = paths.filter(path -> path.getFileName().toString().equals("sureness.yml")) + .collect(Collectors.toCollection(LinkedHashSet::new)); + assertFalse(copies.isEmpty(), "expected the deployment scripts to ship sureness.yml copies"); + return copies; + } + } + + @SuppressWarnings("unchecked") + private static List sectionOf(Path copy, String section) throws IOException { + Map document; + try (InputStream in = Files.newInputStream(copy)) { + document = new Yaml().load(in); + } + List rules = (List) document.get(section); + assertNotNull(rules, section + " must be present in " + copy); + return rules; + } } diff --git a/script/docker-compose/hertzbeat-mysql-iotdb/conf/sureness.yml b/script/docker-compose/hertzbeat-mysql-iotdb/conf/sureness.yml index fe6f8938f9..f19414fb5b 100644 --- a/script/docker-compose/hertzbeat-mysql-iotdb/conf/sureness.yml +++ b/script/docker-compose/hertzbeat-mysql-iotdb/conf/sureness.yml @@ -22,6 +22,12 @@ resourceRole: - /api/account/auth/refresh===post===[admin,user,guest] - /api/apps/**===get===[admin,user,guest] + # the define yml routes persist the global collection templates + # post can only add a new type, put overwrites an existing one and immediately + # redispatches its collect job to every monitor already using it - hence admin only + - /api/apps/**===post===[admin,user] + - /api/apps/**===put===[admin] + - /api/apps/**===delete===[admin] - /api/monitor/**===get===[admin,user,guest] - /api/monitor/**===post===[admin,user] - /api/monitor/**===put===[admin,user] diff --git a/script/docker-compose/hertzbeat-mysql-tdengine/conf/sureness.yml b/script/docker-compose/hertzbeat-mysql-tdengine/conf/sureness.yml index fe6f8938f9..f19414fb5b 100644 --- a/script/docker-compose/hertzbeat-mysql-tdengine/conf/sureness.yml +++ b/script/docker-compose/hertzbeat-mysql-tdengine/conf/sureness.yml @@ -22,6 +22,12 @@ resourceRole: - /api/account/auth/refresh===post===[admin,user,guest] - /api/apps/**===get===[admin,user,guest] + # the define yml routes persist the global collection templates + # post can only add a new type, put overwrites an existing one and immediately + # redispatches its collect job to every monitor already using it - hence admin only + - /api/apps/**===post===[admin,user] + - /api/apps/**===put===[admin] + - /api/apps/**===delete===[admin] - /api/monitor/**===get===[admin,user,guest] - /api/monitor/**===post===[admin,user] - /api/monitor/**===put===[admin,user] diff --git a/script/docker-compose/hertzbeat-mysql-victoria-metrics/conf/sureness.yml b/script/docker-compose/hertzbeat-mysql-victoria-metrics/conf/sureness.yml index fe6f8938f9..f19414fb5b 100644 --- a/script/docker-compose/hertzbeat-mysql-victoria-metrics/conf/sureness.yml +++ b/script/docker-compose/hertzbeat-mysql-victoria-metrics/conf/sureness.yml @@ -22,6 +22,12 @@ resourceRole: - /api/account/auth/refresh===post===[admin,user,guest] - /api/apps/**===get===[admin,user,guest] + # the define yml routes persist the global collection templates + # post can only add a new type, put overwrites an existing one and immediately + # redispatches its collect job to every monitor already using it - hence admin only + - /api/apps/**===post===[admin,user] + - /api/apps/**===put===[admin] + - /api/apps/**===delete===[admin] - /api/monitor/**===get===[admin,user,guest] - /api/monitor/**===post===[admin,user] - /api/monitor/**===put===[admin,user] diff --git a/script/docker-compose/hertzbeat-postgresql-greptimedb/conf/sureness.yml b/script/docker-compose/hertzbeat-postgresql-greptimedb/conf/sureness.yml index 2d6b4441ee..e9c2f9f4a1 100644 --- a/script/docker-compose/hertzbeat-postgresql-greptimedb/conf/sureness.yml +++ b/script/docker-compose/hertzbeat-postgresql-greptimedb/conf/sureness.yml @@ -22,6 +22,12 @@ resourceRole: - /api/account/auth/refresh===post===[admin,user,guest] - /api/apps/**===get===[admin,user,guest] + # the define yml routes persist the global collection templates + # post can only add a new type, put overwrites an existing one and immediately + # redispatches its collect job to every monitor already using it - hence admin only + - /api/apps/**===post===[admin,user] + - /api/apps/**===put===[admin] + - /api/apps/**===delete===[admin] - /api/monitor/**===get===[admin,user,guest] - /api/monitor/**===post===[admin,user] - /api/monitor/**===put===[admin,user] diff --git a/script/docker-compose/hertzbeat-postgresql-victoria-metrics/conf/sureness.yml b/script/docker-compose/hertzbeat-postgresql-victoria-metrics/conf/sureness.yml index fe6f8938f9..f19414fb5b 100644 --- a/script/docker-compose/hertzbeat-postgresql-victoria-metrics/conf/sureness.yml +++ b/script/docker-compose/hertzbeat-postgresql-victoria-metrics/conf/sureness.yml @@ -22,6 +22,12 @@ resourceRole: - /api/account/auth/refresh===post===[admin,user,guest] - /api/apps/**===get===[admin,user,guest] + # the define yml routes persist the global collection templates + # post can only add a new type, put overwrites an existing one and immediately + # redispatches its collect job to every monitor already using it - hence admin only + - /api/apps/**===post===[admin,user] + - /api/apps/**===put===[admin] + - /api/apps/**===delete===[admin] - /api/monitor/**===get===[admin,user,guest] - /api/monitor/**===post===[admin,user] - /api/monitor/**===put===[admin,user] diff --git a/script/sureness.yml b/script/sureness.yml index 2d6b4441ee..e9c2f9f4a1 100644 --- a/script/sureness.yml +++ b/script/sureness.yml @@ -22,6 +22,12 @@ resourceRole: - /api/account/auth/refresh===post===[admin,user,guest] - /api/apps/**===get===[admin,user,guest] + # the define yml routes persist the global collection templates + # post can only add a new type, put overwrites an existing one and immediately + # redispatches its collect job to every monitor already using it - hence admin only + - /api/apps/**===post===[admin,user] + - /api/apps/**===put===[admin] + - /api/apps/**===delete===[admin] - /api/monitor/**===get===[admin,user,guest] - /api/monitor/**===post===[admin,user] - /api/monitor/**===put===[admin,user]