diff --git a/hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/controller/GeneralConfigController.java b/hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/controller/GeneralConfigController.java index 5d365431f1..e17c018c0e 100644 --- a/hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/controller/GeneralConfigController.java +++ b/hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/controller/GeneralConfigController.java @@ -23,11 +23,14 @@ import io.swagger.v3.oas.annotations.tags.Tag; import jakarta.annotation.Resource; import jakarta.validation.constraints.NotNull; import lombok.extern.slf4j.Slf4j; +import org.apache.hertzbeat.common.constants.CommonConstants; +import org.apache.hertzbeat.common.constants.GeneralConfigTypeEnum; import org.apache.hertzbeat.common.entity.dto.Message; import org.apache.hertzbeat.common.util.CommonUtil; import org.apache.hertzbeat.common.util.ResponseUtil; import org.apache.hertzbeat.manager.pojo.dto.TemplateConfig; import org.apache.hertzbeat.manager.service.ConfigService; +import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -61,6 +64,18 @@ public class GeneralConfigController { private static final Set ZONE_IDS = ZoneId.getAvailableZoneIds(); + /** + * Config types that must never travel over the rest api. + * + *

The {@code secret} config holds the jwt signing key and the aes key that protects + * every stored monitor credential. Both are loaded straight from the persistence layer + * while the application boots and no part of the ui reads them, so handing them out + * over http has no legitimate use and would let a reader mint admin tokens and decrypt + * stored credentials. Refusing the read here keeps the guarantee even if the rbac rules + * for this route are ever loosened again. + */ + private static final Set NON_READABLE_TYPES = Set.of(GeneralConfigTypeEnum.secret.name()); + @Resource private ConfigService configService; @@ -80,6 +95,12 @@ public class GeneralConfigController { public ResponseEntity> getConfig( @Parameter(description = "Config Type", example = "email") @PathVariable("type") @NotNull final String type) { + if (NON_READABLE_TYPES.contains(type)) { + log.warn("Refused to serve the {} config over the rest api", type); + return ResponseEntity.status(HttpStatus.FORBIDDEN) + .body(Message.fail(CommonConstants.FAIL_CODE, + "The " + type + " config can not be read through the rest api.")); + } return ResponseUtil.handle(() -> configService.getConfig(type)); } diff --git a/hertzbeat-manager/src/test/java/org/apache/hertzbeat/manager/controller/GeneralConfigControllerTest.java b/hertzbeat-manager/src/test/java/org/apache/hertzbeat/manager/controller/GeneralConfigControllerTest.java index 77f912c776..a81b51ec77 100644 --- a/hertzbeat-manager/src/test/java/org/apache/hertzbeat/manager/controller/GeneralConfigControllerTest.java +++ b/hertzbeat-manager/src/test/java/org/apache/hertzbeat/manager/controller/GeneralConfigControllerTest.java @@ -20,6 +20,8 @@ package org.apache.hertzbeat.manager.controller; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; @@ -84,6 +86,18 @@ class GeneralConfigControllerTest { .andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE)); } + @Test + public void testGetSecretConfigIsRefused() throws Exception { + + mockMvc.perform(get("/api/config/secret") + .accept(MediaType.APPLICATION_JSON)) + .andExpect(status().isForbidden()) + .andExpect(jsonPath("$.code").value((int) CommonConstants.FAIL_CODE)); + + // the jwt signing key and the aes key must not even be loaded for a rest read + verify(configService, never()).getConfig(anyString()); + } + @Test public void testUpdateTemplateAppConfig() throws Exception { diff --git a/hertzbeat-startup/src/main/resources/sureness.yml b/hertzbeat-startup/src/main/resources/sureness.yml index c51f6318e2..a3198da74b 100644 --- a/hertzbeat-startup/src/main/resources/sureness.yml +++ b/hertzbeat-startup/src/main/resources/sureness.yml @@ -54,6 +54,16 @@ resourceRole: - /api/collector/**===post===[admin,user] - /api/collector/**===put===[admin,user] - /api/collector/**===delete===[admin] + # the secret config holds the jwt signing key and the aes key protecting stored + # credentials, so it stays admin only and is additionally refused by the controller + - /api/config/secret===get===[admin] + - /api/config/secret===post===[admin] + # the mute toggle sits in the notification widget every signed in user sees + - /api/config/mute===post===[admin,user,guest] + - /api/config/**===get===[admin,user,guest] + - /api/config/**===post===[admin] + - /api/config/**===put===[admin] + - /api/config/**===delete===[admin] - /api/status/page/**===get===[admin,user,guest] - /api/status/page/**===post===[admin,user] - /api/status/page/**===put===[admin,user] diff --git a/hertzbeat-startup/src/test/java/org/apache/hertzbeat/startup/security/SurenessConfigRuleTest.java b/hertzbeat-startup/src/test/java/org/apache/hertzbeat/startup/security/SurenessConfigRuleTest.java new file mode 100644 index 0000000000..d5aa6056dc --- /dev/null +++ b/hertzbeat-startup/src/test/java/org/apache/hertzbeat/startup/security/SurenessConfigRuleTest.java @@ -0,0 +1,97 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hertzbeat.startup.security; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import com.usthe.sureness.matcher.util.TirePathTree; +import java.io.IOException; +import java.io.InputStream; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.yaml.snakeyaml.Yaml; + +/** + * Guards the rbac rules covering {@code /api/config/**}. + * + *

A route missing from {@code sureness.yml} carries no role requirement, and + * {@code BaseProcessor.authorized} returns early when no role is required, so any + * authenticated caller reaches it. The config routes used to be absent entirely, which + * exposed {@code GET /api/config/secret} - the jwt signing key and the aes key that + * protects stored monitor credentials - to every account including {@code guest}. + */ +class SurenessConfigRuleTest { + + private static final String SEPARATOR = "==="; + + private static TirePathTree roleTree; + + @BeforeAll + @SuppressWarnings("unchecked") + static void loadSurenessConfig() throws IOException { + List resourceRole; + try (InputStream in = SurenessConfigRuleTest.class.getResourceAsStream("/sureness.yml")) { + assertNotNull(in, "sureness.yml must be on the classpath"); + Map document = new Yaml().load(in); + resourceRole = (List) document.get("resourceRole"); + } + assertNotNull(resourceRole, "resourceRole must be present"); + roleTree = new TirePathTree(); + roleTree.buildTree(new LinkedHashSet<>(resourceRole)); + } + + private static String rolesFor(String path, String method) { + return roleTree.searchPathFilterRoles(path + SEPARATOR + method); + } + + @Test + void readingTheSecretConfigIsRestrictedToAdmin() { + assertEquals("[admin]", rolesFor("/api/config/secret", "get")); + } + + @Test + void writingTheSecretConfigIsRestrictedToAdmin() { + assertEquals("[admin]", rolesFor("/api/config/secret", "post")); + } + + @Test + void writingAnyOtherConfigIsRestrictedToAdmin() { + assertEquals("[admin]", rolesFor("/api/config/email", "post")); + assertEquals("[admin]", rolesFor("/api/config/oss", "post")); + assertEquals("[admin]", rolesFor("/api/config/template/linux", "put")); + } + + /** + * The notification widget in the top bar lets every signed in user flip the mute flag, + * so this one write has to stay reachable by all roles. + */ + @Test + void togglingMuteStaysOpenToEveryRole() { + assertEquals("[admin,user,guest]", rolesFor("/api/config/mute", "post")); + assertEquals("[admin,user,guest]", rolesFor("/api/config/mute", "get")); + } + + @Test + void readingNonSecretConfigStaysOpenToEveryRole() { + assertEquals("[admin,user,guest]", rolesFor("/api/config/system", "get")); + assertEquals("[admin,user,guest]", rolesFor("/api/config/timezones", "get")); + } +} diff --git a/script/docker-compose/hertzbeat-mysql-iotdb/conf/sureness.yml b/script/docker-compose/hertzbeat-mysql-iotdb/conf/sureness.yml index def13ade17..28936362c9 100644 --- a/script/docker-compose/hertzbeat-mysql-iotdb/conf/sureness.yml +++ b/script/docker-compose/hertzbeat-mysql-iotdb/conf/sureness.yml @@ -54,6 +54,16 @@ resourceRole: - /api/collector/**===post===[admin,user] - /api/collector/**===put===[admin,user] - /api/collector/**===delete===[admin] + # the secret config holds the jwt signing key and the aes key protecting stored + # credentials, so it stays admin only and is additionally refused by the controller + - /api/config/secret===get===[admin] + - /api/config/secret===post===[admin] + # the mute toggle sits in the notification widget every signed in user sees + - /api/config/mute===post===[admin,user,guest] + - /api/config/**===get===[admin,user,guest] + - /api/config/**===post===[admin] + - /api/config/**===put===[admin] + - /api/config/**===delete===[admin] - /api/status/page/**===get===[admin,user,guest] - /api/status/page/**===post===[admin,user] - /api/status/page/**===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 def13ade17..28936362c9 100644 --- a/script/docker-compose/hertzbeat-mysql-tdengine/conf/sureness.yml +++ b/script/docker-compose/hertzbeat-mysql-tdengine/conf/sureness.yml @@ -54,6 +54,16 @@ resourceRole: - /api/collector/**===post===[admin,user] - /api/collector/**===put===[admin,user] - /api/collector/**===delete===[admin] + # the secret config holds the jwt signing key and the aes key protecting stored + # credentials, so it stays admin only and is additionally refused by the controller + - /api/config/secret===get===[admin] + - /api/config/secret===post===[admin] + # the mute toggle sits in the notification widget every signed in user sees + - /api/config/mute===post===[admin,user,guest] + - /api/config/**===get===[admin,user,guest] + - /api/config/**===post===[admin] + - /api/config/**===put===[admin] + - /api/config/**===delete===[admin] - /api/status/page/**===get===[admin,user,guest] - /api/status/page/**===post===[admin,user] - /api/status/page/**===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 def13ade17..28936362c9 100644 --- a/script/docker-compose/hertzbeat-mysql-victoria-metrics/conf/sureness.yml +++ b/script/docker-compose/hertzbeat-mysql-victoria-metrics/conf/sureness.yml @@ -54,6 +54,16 @@ resourceRole: - /api/collector/**===post===[admin,user] - /api/collector/**===put===[admin,user] - /api/collector/**===delete===[admin] + # the secret config holds the jwt signing key and the aes key protecting stored + # credentials, so it stays admin only and is additionally refused by the controller + - /api/config/secret===get===[admin] + - /api/config/secret===post===[admin] + # the mute toggle sits in the notification widget every signed in user sees + - /api/config/mute===post===[admin,user,guest] + - /api/config/**===get===[admin,user,guest] + - /api/config/**===post===[admin] + - /api/config/**===put===[admin] + - /api/config/**===delete===[admin] - /api/status/page/**===get===[admin,user,guest] - /api/status/page/**===post===[admin,user] - /api/status/page/**===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 d86ef543ce..ee2e532df0 100644 --- a/script/docker-compose/hertzbeat-postgresql-greptimedb/conf/sureness.yml +++ b/script/docker-compose/hertzbeat-postgresql-greptimedb/conf/sureness.yml @@ -54,6 +54,16 @@ resourceRole: - /api/collector/**===post===[admin,user] - /api/collector/**===put===[admin,user] - /api/collector/**===delete===[admin] + # the secret config holds the jwt signing key and the aes key protecting stored + # credentials, so it stays admin only and is additionally refused by the controller + - /api/config/secret===get===[admin] + - /api/config/secret===post===[admin] + # the mute toggle sits in the notification widget every signed in user sees + - /api/config/mute===post===[admin,user,guest] + - /api/config/**===get===[admin,user,guest] + - /api/config/**===post===[admin] + - /api/config/**===put===[admin] + - /api/config/**===delete===[admin] - /api/status/page/**===get===[admin,user,guest] - /api/status/page/**===post===[admin,user] - /api/status/page/**===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 def13ade17..28936362c9 100644 --- a/script/docker-compose/hertzbeat-postgresql-victoria-metrics/conf/sureness.yml +++ b/script/docker-compose/hertzbeat-postgresql-victoria-metrics/conf/sureness.yml @@ -54,6 +54,16 @@ resourceRole: - /api/collector/**===post===[admin,user] - /api/collector/**===put===[admin,user] - /api/collector/**===delete===[admin] + # the secret config holds the jwt signing key and the aes key protecting stored + # credentials, so it stays admin only and is additionally refused by the controller + - /api/config/secret===get===[admin] + - /api/config/secret===post===[admin] + # the mute toggle sits in the notification widget every signed in user sees + - /api/config/mute===post===[admin,user,guest] + - /api/config/**===get===[admin,user,guest] + - /api/config/**===post===[admin] + - /api/config/**===put===[admin] + - /api/config/**===delete===[admin] - /api/status/page/**===get===[admin,user,guest] - /api/status/page/**===post===[admin,user] - /api/status/page/**===put===[admin,user] diff --git a/script/sureness.yml b/script/sureness.yml index a356931407..2d6b4441ee 100644 --- a/script/sureness.yml +++ b/script/sureness.yml @@ -54,6 +54,16 @@ resourceRole: - /api/collector/**===post===[admin,user] - /api/collector/**===put===[admin,user] - /api/collector/**===delete===[admin] + # the secret config holds the jwt signing key and the aes key protecting stored + # credentials, so it stays admin only and is additionally refused by the controller + - /api/config/secret===get===[admin] + - /api/config/secret===post===[admin] + # the mute toggle sits in the notification widget every signed in user sees + - /api/config/mute===post===[admin,user,guest] + - /api/config/**===get===[admin,user,guest] + - /api/config/**===post===[admin] + - /api/config/**===put===[admin] + - /api/config/**===delete===[admin] - /api/status/page/**===get===[admin,user,guest] - /api/status/page/**===post===[admin,user] - /api/status/page/**===put===[admin,user]