diff --git a/pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerMeter.java b/pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerMeter.java index ee034ec952ca..d82e058daf61 100644 --- a/pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerMeter.java +++ b/pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerMeter.java @@ -68,7 +68,9 @@ public enum ControllerMeter implements AbstractMetrics.Meter { NUMBER_ADHOC_TASKS_SUBMITTED("adhocTasks", false), IDEAL_STATE_UPDATE_FAILURE("IdealStateUpdateFailure", false), IDEAL_STATE_UPDATE_RETRY("IdealStateUpdateRetry", false), - IDEAL_STATE_UPDATE_SUCCESS("IdealStateUpdateSuccess", false); + IDEAL_STATE_UPDATE_SUCCESS("IdealStateUpdateSuccess", false), + GROOVY_SECURITY_VIOLATIONS("exceptions", true); + private final String _brokerMeterName; diff --git a/pinot-common/src/main/java/org/apache/pinot/common/metrics/MinionMeter.java b/pinot-common/src/main/java/org/apache/pinot/common/metrics/MinionMeter.java index 220fd7cb7d2b..96f2c87a6074 100644 --- a/pinot-common/src/main/java/org/apache/pinot/common/metrics/MinionMeter.java +++ b/pinot-common/src/main/java/org/apache/pinot/common/metrics/MinionMeter.java @@ -38,7 +38,8 @@ public enum MinionMeter implements AbstractMetrics.Meter { SEGMENT_BYTES_UPLOADED("bytes", false), RECORDS_PROCESSED_COUNT("rows", false), RECORDS_PURGED_COUNT("rows", false), - COMPACTED_RECORDS_COUNT("rows", false); + COMPACTED_RECORDS_COUNT("rows", false), + GROOVY_SECURITY_VIOLATIONS("exceptions", true); private final String _meterName; private final String _unit; diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java b/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java index 171e8506387a..503d6594bbbd 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java @@ -122,6 +122,7 @@ import org.apache.pinot.core.segment.processing.lifecycle.PinotSegmentLifecycleEventListenerManager; import org.apache.pinot.core.transport.ListenerConfig; import org.apache.pinot.core.util.ListenerConfigUtil; +import org.apache.pinot.segment.local.function.GroovyFunctionEvaluator; import org.apache.pinot.segment.local.utils.TableConfigUtils; import org.apache.pinot.spi.config.table.TableConfig; import org.apache.pinot.spi.crypt.PinotCrypterFactory; @@ -382,7 +383,8 @@ public ControllerConf getConfig() { } @Override - public void start() { + public void start() + throws Exception { LOGGER.info("Starting Pinot controller in mode: {}. (Version: {})", _controllerMode.name(), PinotVersion.VERSION); LOGGER.info("Controller configs: {}", new PinotAppConfigs(getConfig()).toJSONString()); long startTimeMs = System.currentTimeMillis(); @@ -407,6 +409,12 @@ public void start() { break; } + // Initializing Groovy execution security + GroovyFunctionEvaluator.configureGroovySecurity( + _config.getProperty(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG)); + GroovyFunctionEvaluator.setMetrics(_controllerMetrics, ControllerMeter.GROOVY_SECURITY_VIOLATIONS); + + ServiceStatus.setServiceStatusCallback(_helixParticipantInstanceId, new ServiceStatus.MultipleCallbackServiceStatusCallback(_serviceStatusCallbackList)); _controllerMetrics.addTimedValue(ControllerTimer.STARTUP_SUCCESS_DURATION_MS, diff --git a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java index 97be9d7f69fa..07c4394f76ef 100644 --- a/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java +++ b/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java @@ -187,8 +187,8 @@ public GroovyStaticAnalyzerConfig getGroovyStaticAnalysisConfig() throws Excepti HelixConfigScope configScope = new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.CLUSTER) .forCluster(_pinotHelixResourceManager.getHelixClusterName()).build(); Map configs = helixAdmin.getConfig(configScope, - List.of(CommonConstants.Server.GROOVY_STATIC_ANALYZER_CONFIG)); - String json = configs.get(CommonConstants.Server.GROOVY_STATIC_ANALYZER_CONFIG); + List.of(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG)); + String json = configs.get(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG); if (json != null) { return GroovyStaticAnalyzerConfig.fromJson(json); } else { @@ -215,7 +215,7 @@ public SuccessResponse setGroovyStaticAnalysisConfig(String body) throws Excepti _pinotHelixResourceManager.getHelixClusterName()).build(); Map properties = new TreeMap<>(); GroovyStaticAnalyzerConfig groovyConfig = GroovyStaticAnalyzerConfig.fromJson(body); - properties.put(CommonConstants.Server.GROOVY_STATIC_ANALYZER_CONFIG, + properties.put(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG, groovyConfig == null ? null : groovyConfig.toJson()); admin.setConfig(configScope, properties); GroovyFunctionEvaluator.setConfig(groovyConfig); diff --git a/pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyFunctionEvaluatorTest.java b/pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyFunctionEvaluatorTest.java index 6e3222850cba..1f57dd193b99 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyFunctionEvaluatorTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyFunctionEvaluatorTest.java @@ -26,13 +26,14 @@ import org.apache.pinot.segment.local.function.GroovyFunctionEvaluator; import org.apache.pinot.segment.local.function.GroovyStaticAnalyzerConfig; import org.apache.pinot.spi.data.readers.GenericRow; -import org.testng.Assert; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import org.testng.collections.Lists; import static org.apache.pinot.segment.local.function.GroovyStaticAnalyzerConfig.getDefaultAllowedImports; import static org.apache.pinot.segment.local.function.GroovyStaticAnalyzerConfig.getDefaultAllowedReceivers; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.fail; /** @@ -61,7 +62,7 @@ public void testLegalGroovyScripts() GroovyFunctionEvaluator groovyFunctionEvaluator = new GroovyFunctionEvaluator(script); GenericRow row = new GenericRow(); Object result = groovyFunctionEvaluator.evaluate(row); - Assert.assertEquals(2, result); + assertEquals(2, result); } } @@ -92,10 +93,8 @@ public void testIllegalGroovyScripts() for (String script : scripts) { try { - GroovyFunctionEvaluator groovyFunctionEvaluator = new GroovyFunctionEvaluator(script); - GenericRow row = new GenericRow(); - groovyFunctionEvaluator.evaluate(row); - Assert.fail("Groovy analyzer failed to catch malicious script"); + new GroovyFunctionEvaluator(script); + fail("Groovy analyzer failed to catch malicious script"); } catch (Exception ignored) { } } @@ -121,7 +120,7 @@ public void testUpdatingConfiguration() GroovyFunctionEvaluator groovyFunctionEvaluator = new GroovyFunctionEvaluator(script); GenericRow row = new GenericRow(); groovyFunctionEvaluator.evaluate(row); - Assert.fail(String.format("Groovy analyzer failed to catch malicious script: %s", script)); + fail(String.format("Groovy analyzer failed to catch malicious script: %s", script)); } catch (Exception ignored) { } } @@ -132,10 +131,10 @@ public void testGroovyFunctionEvaluation(String transformFunction, List Object expectedResult) { GroovyFunctionEvaluator groovyExpressionEvaluator = new GroovyFunctionEvaluator(transformFunction); - Assert.assertEquals(groovyExpressionEvaluator.getArguments(), arguments); + assertEquals(groovyExpressionEvaluator.getArguments(), arguments); Object result = groovyExpressionEvaluator.evaluate(genericRow); - Assert.assertEquals(result, expectedResult); + assertEquals(result, expectedResult); } @DataProvider(name = "groovyFunctionEvaluationDataProvider") diff --git a/pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyStaticAnalyzerConfigTest.java b/pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyStaticAnalyzerConfigTest.java index f051de395520..d0a0c1c04934 100644 --- a/pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyStaticAnalyzerConfigTest.java +++ b/pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyStaticAnalyzerConfigTest.java @@ -89,6 +89,7 @@ public void testAllowedStaticImports() Assert.assertNull(decodedConfig.getAllowedImports()); Assert.assertEquals(GroovyStaticAnalyzerConfig.getDefaultAllowedImports(), decodedConfig.getAllowedStaticImports()); Assert.assertNull(decodedConfig.getDisallowedMethodNames()); + Assert.assertFalse(decodedConfig.isMethodDefinitionAllowed()); } @Test diff --git a/pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java b/pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java index 8dab73386fed..b88eda178204 100644 --- a/pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java +++ b/pinot-minion/src/main/java/org/apache/pinot/minion/BaseMinionStarter.java @@ -61,6 +61,7 @@ import org.apache.pinot.minion.executor.PinotTaskExecutorFactory; import org.apache.pinot.minion.executor.TaskExecutorFactoryRegistry; import org.apache.pinot.minion.taskfactory.TaskFactoryRegistry; +import org.apache.pinot.segment.local.function.GroovyFunctionEvaluator; import org.apache.pinot.spi.crypt.PinotCrypterFactory; import org.apache.pinot.spi.env.PinotConfiguration; import org.apache.pinot.spi.filesystem.PinotFSFactory; @@ -272,6 +273,10 @@ public void start() minionContext.setSSLContext(sslContext); } + // Initializing Groovy execution security + GroovyFunctionEvaluator.configureGroovySecurity(_config.getProperty(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG)); + GroovyFunctionEvaluator.setMetrics(minionMetrics, MinionMeter.GROOVY_SECURITY_VIOLATIONS); + // Join the Helix cluster LOGGER.info("Joining the Helix cluster"); _helixManager.getStateMachineEngine().registerStateModelFactory("Task", new TaskStateModelFactory(_helixManager, diff --git a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java index df510063d02b..d324b8949bc9 100644 --- a/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java +++ b/pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyFunctionEvaluator.java @@ -28,8 +28,7 @@ import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; -import org.apache.pinot.common.metrics.ServerMeter; -import org.apache.pinot.common.metrics.ServerMetrics; +import org.apache.pinot.common.metrics.AbstractMetrics; import org.apache.pinot.spi.data.readers.GenericRow; import org.codehaus.groovy.ast.expr.MethodCallExpression; import org.codehaus.groovy.control.CompilerConfiguration; @@ -64,7 +63,8 @@ public class GroovyFunctionEvaluator implements FunctionEvaluator { private static final String SCRIPT_GROUP_NAME = "script"; private static final String ARGUMENTS_SEPARATOR = ","; private static GroovyStaticAnalyzerConfig _config; - private static ServerMetrics _metrics; + private static AbstractMetrics _metrics; + private static AbstractMetrics.Meter _securityViolationCounter; private final List _arguments; private final int _numArguments; @@ -201,17 +201,33 @@ private static GroovyStaticAnalyzerConfig getConfig() { private static void incrementSecurityViolationCounter() { synchronized (GroovyFunctionEvaluator.class) { if (_metrics != null) { - _metrics.addMeteredGlobalValue(ServerMeter.GROOVY_SECURITY_VIOLATIONS, 1); + _metrics.addMeteredGlobalValue(_securityViolationCounter, 1); } } } - public static void setServerMetrics(ServerMetrics metrics) { + public static void setMetrics(AbstractMetrics metrics, AbstractMetrics.Meter securityViolationCounter) { synchronized (GroovyFunctionEvaluator.class) { _metrics = metrics; + _securityViolationCounter = securityViolationCounter; } } + public static void configureGroovySecurity(String groovyASTConfig) throws Exception { + try { + if (groovyASTConfig != null) { + setConfig(GroovyStaticAnalyzerConfig.fromJson(groovyASTConfig)); + } else { + LOGGER.info("No Groovy Security Configuration found, Groovy static analysis is disabled"); + } + } catch (Exception ex) { + // If there was an issue reading the security configuration then send an exception to the startup routine. + LOGGER.error("Failed to read config from ZK"); + throw ex; + } + } + + /** * Initialize or update the configuration for the Groovy Static Analyzer. * @param config GroovyStaticAnalyzerConfig instance to be used for static syntax analysis. diff --git a/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java b/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java index 4255a6f37304..a0f29703864b 100644 --- a/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java +++ b/pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java @@ -79,7 +79,6 @@ import org.apache.pinot.core.transport.ListenerConfig; import org.apache.pinot.core.util.ListenerConfigUtil; import org.apache.pinot.segment.local.function.GroovyFunctionEvaluator; -import org.apache.pinot.segment.local.function.GroovyStaticAnalyzerConfig; import org.apache.pinot.segment.local.realtime.impl.invertedindex.RealtimeLuceneIndexRefreshManager; import org.apache.pinot.segment.local.realtime.impl.invertedindex.RealtimeLuceneTextIndexSearcherPool; import org.apache.pinot.segment.spi.memory.PinotDataBuffer; @@ -651,8 +650,10 @@ public void start() _adminApiApplication = createServerAdminApp(); _adminApiApplication.start(_listenerConfigs); - // Initializing Groovy execution engine security - configureGroovySecurity(serverMetrics); + // Initializing Groovy execution security + GroovyFunctionEvaluator.configureGroovySecurity( + _serverConf.getProperty(CommonConstants.GROOVY_STATIC_ANALYZER_CONFIG)); + GroovyFunctionEvaluator.setMetrics(serverMetrics, ServerMeter.GROOVY_SECURITY_VIOLATIONS); // Init QueryRewriterFactory LOGGER.info("Initializing QueryRewriterFactory"); @@ -989,24 +990,4 @@ private void initSegmentFetcher(PinotConfiguration config) protected AdminApiApplication createServerAdminApp() { return new AdminApiApplication(_serverInstance, _accessControlFactory, _serverConf); } - - private void configureGroovySecurity(ServerMetrics metrics) throws Exception { - GroovyStaticAnalyzerConfig config = null; - try { - String json = _serverConf.getProperty(CommonConstants.Server.GROOVY_STATIC_ANALYZER_CONFIG); - - if (json != null) { - LOGGER.info("Groovy Security Configuration: {}", json); - config = GroovyStaticAnalyzerConfig.fromJson(json); - GroovyFunctionEvaluator.setConfig(config); - GroovyFunctionEvaluator.setServerMetrics(metrics); - } else { - LOGGER.info("No Groovy Security Configuration found, Groovy static analysis is disabled."); - } - } catch (Exception ex) { - // If there was an issue reading the security configuration then send an exception to the startup routine. - LOGGER.error("Failed to read config from ZK. Loading Default configuration."); - throw ex; - } - } } diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java index 97c79e9a458c..9636f7f6eb18 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/utils/CommonConstants.java @@ -29,6 +29,8 @@ public class CommonConstants { + public static final String GROOVY_STATIC_ANALYZER_CONFIG = "pinot.server.groovy.static.analyzer"; + private CommonConstants() { } @@ -654,7 +656,6 @@ public enum Type { } public static class Server { - public static final String GROOVY_STATIC_ANALYZER_CONFIG = "pinot.server.groovy.static.analyzer"; public static final String INSTANCE_DATA_MANAGER_CONFIG_PREFIX = "pinot.server.instance"; public static final String QUERY_EXECUTOR_CONFIG_PREFIX = "pinot.server.query.executor"; public static final String METRICS_CONFIG_PREFIX = "pinot.server.metrics";