3403 Security enhancement for /api/admin/config (#3484) * KYLIN-1664 add whitelist for non-admin kylin properties
* 3403 KYLIN-1664 add another api for getting public configurations Project: http://git-wip-us.apache.org/repos/asf/kylin/repo Commit: http://git-wip-us.apache.org/repos/asf/kylin/commit/4629d848 Tree: http://git-wip-us.apache.org/repos/asf/kylin/tree/4629d848 Diff: http://git-wip-us.apache.org/repos/asf/kylin/diff/4629d848 Branch: refs/heads/master Commit: 4629d8483914e29f4bb50cfcc6ba09b064187a38 Parents: af15062 Author: Shaofeng Shi <shaofeng...@gmail.com> Authored: Tue Dec 12 15:37:31 2017 +0800 Committer: Zhixiong Chen <c...@apache.org> Committed: Tue Dec 12 15:37:31 2017 +0800 ---------------------------------------------------------------------- .../org/apache/kylin/common/KylinConfig.java | 29 ++++++++++++++++---- .../apache/kylin/common/KylinConfigBase.java | 22 ++++++++++++++- .../apache/kylin/query/ITKylinQueryTest.java | 1 + .../kylin/rest/controller/AdminController.java | 27 +++++++++++++++--- .../kylin/rest/controller/BasicController.java | 19 +++++++++++++ .../apache/kylin/rest/service/AdminService.java | 13 +++------ server/src/main/resources/kylinSecurity.xml | 1 + .../hbase/cube/v2/CubeHBaseEndpointRPC.java | 2 +- 8 files changed, 94 insertions(+), 20 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/kylin/blob/4629d848/core-common/src/main/java/org/apache/kylin/common/KylinConfig.java ---------------------------------------------------------------------- diff --git a/core-common/src/main/java/org/apache/kylin/common/KylinConfig.java b/core-common/src/main/java/org/apache/kylin/common/KylinConfig.java index c638ec6..c662a16 100644 --- a/core-common/src/main/java/org/apache/kylin/common/KylinConfig.java +++ b/core-common/src/main/java/org/apache/kylin/common/KylinConfig.java @@ -30,6 +30,7 @@ import java.lang.reflect.Method; import java.net.URL; import java.nio.ByteOrder; import java.nio.charset.Charset; +import java.util.Collection; import java.util.Map; import java.util.Properties; import java.util.concurrent.ConcurrentHashMap; @@ -439,11 +440,9 @@ public class KylinConfig extends KylinConfigBase { return copy; } - public String exportToString() throws IOException { - Properties allProps = getAllProperties(); - OrderedProperties orderedProperties = KylinConfig.buildSiteOrderedProps(); - - final StringBuilder sb = new StringBuilder(); + public String exportAllToString() throws IOException { + final Properties allProps = getProperties(null); + final OrderedProperties orderedProperties = KylinConfig.buildSiteOrderedProps(); for (Map.Entry<Object, Object> entry : allProps.entrySet()) { String key = entry.getKey().toString(); @@ -454,10 +453,30 @@ public class KylinConfig extends KylinConfigBase { orderedProperties.setProperty(key, value); } } + + final StringBuilder sb = new StringBuilder(); for (Map.Entry<String, String> entry : orderedProperties.entrySet()) { sb.append(entry.getKey() + "=" + entry.getValue()).append('\n'); } return sb.toString(); + + } + + public String exportToString(Collection<String> propertyKeys) throws IOException { + Properties filteredProps = getProperties(propertyKeys); + OrderedProperties orderedProperties = KylinConfig.buildSiteOrderedProps(); + + for (String key : propertyKeys) { + if (!filteredProps.containsKey(key)) { + filteredProps.put(key, orderedProperties.getProperty(key, "")); + } + } + + final StringBuilder sb = new StringBuilder(); + for (Map.Entry<Object, Object> entry : filteredProps.entrySet()) { + sb.append(entry.getKey() + "=" + entry.getValue()).append('\n'); + } + return sb.toString(); } public void exportToFile(File file) throws IOException { http://git-wip-us.apache.org/repos/asf/kylin/blob/4629d848/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java ---------------------------------------------------------------------- diff --git a/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java b/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java index 1302247..8572fa3 100644 --- a/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java +++ b/core-common/src/main/java/org/apache/kylin/common/KylinConfigBase.java @@ -21,6 +21,7 @@ package org.apache.kylin.common; import java.io.File; import java.io.IOException; import java.io.Serializable; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -125,12 +126,23 @@ abstract public class KylinConfigBase implements Serializable { } protected Properties getAllProperties() { + return getProperties(null); + } + + /** + * + * @param propertyKeys the collection of the properties; if null will return all properties + * @return + */ + protected Properties getProperties(Collection<String> propertyKeys) { Map<String, String> envMap = System.getenv(); StrSubstitutor sub = new StrSubstitutor(envMap); Properties properties = new Properties(); for (Entry<Object, Object> entry : this.properties.entrySet()) { - properties.put(entry.getKey(), sub.replace((String) entry.getValue())); + if (propertyKeys == null || propertyKeys.contains(entry.getKey())) { + properties.put(entry.getKey(), sub.replace((String) entry.getValue())); + } } return properties; } @@ -1386,4 +1398,12 @@ abstract public class KylinConfigBase implements Serializable { public boolean isHtraceTracingEveryQuery() { return Boolean.valueOf(getOptional("kylin.htrace.trace-every-query", "false")); } + + public String getPropertiesWhiteList() { + return getOptional("kylin.web.properties.whitelist", + "kylin.web.timezone,kylin.query.cache-enabled,kylin.env,kylin.web.hive-limit,kylin.storage.default,kylin.engine.default,kylin.web.link-hadoop,kylin.web.link-diagnostic," + + "kylin.web.contact-mail,kylin.web.help.length,kylin.web.help.0,kylin.web.help.1,kylin.web.help.2,kylin.web.help.3," + + "kylin.web.help,kylin.web.hide-measures,kylin.web.link-streaming-guide,kylin.server.external-acl-provider,kylin.security.profile," + + "kylin.htrace.show-gui-trace-toggle"); + } } http://git-wip-us.apache.org/repos/asf/kylin/blob/4629d848/kylin-it/src/test/java/org/apache/kylin/query/ITKylinQueryTest.java ---------------------------------------------------------------------- diff --git a/kylin-it/src/test/java/org/apache/kylin/query/ITKylinQueryTest.java b/kylin-it/src/test/java/org/apache/kylin/query/ITKylinQueryTest.java index 60633c9..c468a9f 100644 --- a/kylin-it/src/test/java/org/apache/kylin/query/ITKylinQueryTest.java +++ b/kylin-it/src/test/java/org/apache/kylin/query/ITKylinQueryTest.java @@ -406,4 +406,5 @@ public class ITKylinQueryTest extends KylinTestBase { public void testPercentileQuery() throws Exception { batchExecuteQuery(getQueryFolderPrefix() + "src/test/resources/query/sql_percentile"); } + } http://git-wip-us.apache.org/repos/asf/kylin/blob/4629d848/server-base/src/main/java/org/apache/kylin/rest/controller/AdminController.java ---------------------------------------------------------------------- diff --git a/server-base/src/main/java/org/apache/kylin/rest/controller/AdminController.java b/server-base/src/main/java/org/apache/kylin/rest/controller/AdminController.java index eec9fa5..bb71190 100644 --- a/server-base/src/main/java/org/apache/kylin/rest/controller/AdminController.java +++ b/server-base/src/main/java/org/apache/kylin/rest/controller/AdminController.java @@ -19,8 +19,12 @@ package org.apache.kylin.rest.controller; import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; +import com.google.common.collect.Lists; import org.apache.commons.configuration.ConfigurationException; +import org.apache.commons.lang3.StringUtils; import org.apache.kylin.common.KylinConfig; import org.apache.kylin.rest.msg.Message; import org.apache.kylin.rest.msg.MsgPicker; @@ -41,8 +45,6 @@ import org.springframework.web.bind.annotation.ResponseBody; /** * Admin Controller is defined as Restful API entrance for UI. * - * @author jianliu - * */ @Controller @RequestMapping(value = "/admin") @@ -75,14 +77,31 @@ public class AdminController extends BasicController { @RequestMapping(value = "/config", method = { RequestMethod.GET }, produces = { "application/json" }) @ResponseBody public GeneralResponse getConfig() throws IOException { - String config = adminService.exportToString(); + String config = KylinConfig.getInstanceFromEnv().exportAllToString(); + GeneralResponse configRes = new GeneralResponse(); + configRes.put("config", config); + + return configRes; + } + @RequestMapping(value = "/public_config", method = { RequestMethod.GET }, produces = { "application/json" }) + @ResponseBody + public GeneralResponse getPublicConfig() throws IOException { + final String whiteListProperties = KylinConfig.getInstanceFromEnv().getPropertiesWhiteList(); + + Collection<String> propertyKeys = Lists.newArrayList(); + if (StringUtils.isNotEmpty(whiteListProperties)) { + propertyKeys.addAll(Arrays.asList(whiteListProperties.split(","))); + } + + final String config = KylinConfig.getInstanceFromEnv().exportToString(propertyKeys); GeneralResponse configRes = new GeneralResponse(); configRes.put("config", config); return configRes; } + @RequestMapping(value = "/metrics/cubes", method = { RequestMethod.GET }, produces = { "application/json" }) @ResponseBody public MetricsResponse cubeMetrics(MetricsRequest request) { @@ -97,7 +116,7 @@ public class AdminController extends BasicController { @RequestMapping(value = "/config", method = { RequestMethod.PUT }, produces = { "application/json" }) public void updateKylinConfig(@RequestBody UpdateConfigRequest updateConfigRequest) { - KylinConfig.getInstanceFromEnv().setProperty(updateConfigRequest.getKey(), updateConfigRequest.getValue()); + adminService.updateConfig(updateConfigRequest.getKey(), updateConfigRequest.getValue()); } public void setAdminService(AdminService adminService) { http://git-wip-us.apache.org/repos/asf/kylin/blob/4629d848/server-base/src/main/java/org/apache/kylin/rest/controller/BasicController.java ---------------------------------------------------------------------- diff --git a/server-base/src/main/java/org/apache/kylin/rest/controller/BasicController.java b/server-base/src/main/java/org/apache/kylin/rest/controller/BasicController.java index b0454fd..b77309e 100644 --- a/server-base/src/main/java/org/apache/kylin/rest/controller/BasicController.java +++ b/server-base/src/main/java/org/apache/kylin/rest/controller/BasicController.java @@ -29,6 +29,7 @@ import javax.servlet.http.HttpServletResponse; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; +import org.apache.kylin.rest.constant.Constant; import org.apache.kylin.rest.exception.BadRequestException; import org.apache.kylin.rest.exception.ForbiddenException; import org.apache.kylin.rest.exception.InternalErrorException; @@ -40,6 +41,9 @@ import org.apache.kylin.rest.response.ErrorResponse; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.http.HttpStatus; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.ResponseBody; import org.springframework.web.bind.annotation.ResponseStatus; @@ -116,4 +120,19 @@ public class BasicController { throw new InternalErrorException("Failed to download file: " + e.getMessage(), e); } } + + public boolean isAdmin() { + boolean isAdmin = false; + Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); + if (authentication != null) { + for (GrantedAuthority auth : authentication.getAuthorities()) { + if (auth.getAuthority().equals(Constant.ROLE_ADMIN)) { + isAdmin = true; + break; + } + } + } + return isAdmin; + } + } http://git-wip-us.apache.org/repos/asf/kylin/blob/4629d848/server-base/src/main/java/org/apache/kylin/rest/service/AdminService.java ---------------------------------------------------------------------- diff --git a/server-base/src/main/java/org/apache/kylin/rest/service/AdminService.java b/server-base/src/main/java/org/apache/kylin/rest/service/AdminService.java index ae004cf..f98a1d4 100644 --- a/server-base/src/main/java/org/apache/kylin/rest/service/AdminService.java +++ b/server-base/src/main/java/org/apache/kylin/rest/service/AdminService.java @@ -19,7 +19,6 @@ package org.apache.kylin.rest.service; import java.io.ByteArrayOutputStream; -import java.io.IOException; import java.util.Map; import java.util.Properties; import java.util.TreeMap; @@ -36,7 +35,6 @@ import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.stereotype.Component; /** - * @author jianliu */ @Component("adminService") public class AdminService extends BasicService { @@ -77,14 +75,11 @@ public class AdminService extends BasicService { return content; } - /** - * Get Java config info as String - */ - // @PreAuthorize(Constant.ACCESS_HAS_ROLE_ADMIN) // this is a critical security issue, see KYLIN-1664 - public String exportToString() throws IOException { - logger.debug("Get Kylin Runtime Config"); + @PreAuthorize(Constant.ACCESS_HAS_ROLE_ADMIN) + public void updateConfig(String key, String value) { + logger.debug("Update Kylin Runtime Config, key=" + key + ", value=" + value); - return KylinConfig.getInstanceFromEnv().exportToString(); + KylinConfig.getInstanceFromEnv().setProperty(key, value); } @PreAuthorize(Constant.ACCESS_HAS_ROLE_ADMIN) http://git-wip-us.apache.org/repos/asf/kylin/blob/4629d848/server/src/main/resources/kylinSecurity.xml ---------------------------------------------------------------------- diff --git a/server/src/main/resources/kylinSecurity.xml b/server/src/main/resources/kylinSecurity.xml index bc97d06..f2c5dbf 100644 --- a/server/src/main/resources/kylinSecurity.xml +++ b/server/src/main/resources/kylinSecurity.xml @@ -245,6 +245,7 @@ <scr:intercept-url pattern="/api/models*/**" access="isAuthenticated()"/> <scr:intercept-url pattern="/api/streaming*/**" access="isAuthenticated()"/> <scr:intercept-url pattern="/api/job*/**" access="isAuthenticated()"/> + <scr:intercept-url pattern="/api/admin/public_config" access="permitAll"/> <scr:intercept-url pattern="/api/projects" access="permitAll"/> <scr:intercept-url pattern="/api/admin*/**" access="hasRole('ROLE_ADMIN')"/> <scr:intercept-url pattern="/api/**" access="isAuthenticated()"/> http://git-wip-us.apache.org/repos/asf/kylin/blob/4629d848/storage-hbase/src/main/java/org/apache/kylin/storage/hbase/cube/v2/CubeHBaseEndpointRPC.java ---------------------------------------------------------------------- diff --git a/storage-hbase/src/main/java/org/apache/kylin/storage/hbase/cube/v2/CubeHBaseEndpointRPC.java b/storage-hbase/src/main/java/org/apache/kylin/storage/hbase/cube/v2/CubeHBaseEndpointRPC.java index 4ac237b..b7bf677 100644 --- a/storage-hbase/src/main/java/org/apache/kylin/storage/hbase/cube/v2/CubeHBaseEndpointRPC.java +++ b/storage-hbase/src/main/java/org/apache/kylin/storage/hbase/cube/v2/CubeHBaseEndpointRPC.java @@ -156,7 +156,7 @@ public class CubeHBaseEndpointRPC extends CubeHBaseRPC { builder.addHbaseColumnsToGT(intList); } builder.setRowkeyPreambleSize(cubeSeg.getRowKeyPreambleSize()); - builder.setKylinProperties(kylinConfig.exportToString()); + builder.setKylinProperties(kylinConfig.exportAllToString()); final String queryId = queryContext.getQueryId(); if (queryId != null) { builder.setQueryId(queryId);
