You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "ege-st (via GitHub)" <gi...@apache.org> on 2024/10/09 20:32:40 UTC
[PR] Secure Excecution of Groovy Code [pinot]
ege-st opened a new pull request, #14197:
URL: https://github.com/apache/pinot/pull/14197
Adds static analysis of Groovy scripts to protect against the execution of malicious code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1794230668
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyMethodSanitizer.java:
##########
@@ -0,0 +1,390 @@
+/**
+ * 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.pinot.segment.local.function;
+
+import java.util.List;
+import org.codehaus.groovy.ast.GroovyCodeVisitor;
+import org.codehaus.groovy.ast.Parameter;
+import org.codehaus.groovy.ast.expr.ArgumentListExpression;
+import org.codehaus.groovy.ast.expr.ArrayExpression;
+import org.codehaus.groovy.ast.expr.AttributeExpression;
+import org.codehaus.groovy.ast.expr.BinaryExpression;
+import org.codehaus.groovy.ast.expr.BitwiseNegationExpression;
+import org.codehaus.groovy.ast.expr.BooleanExpression;
+import org.codehaus.groovy.ast.expr.CastExpression;
+import org.codehaus.groovy.ast.expr.ClassExpression;
+import org.codehaus.groovy.ast.expr.ClosureExpression;
+import org.codehaus.groovy.ast.expr.ClosureListExpression;
+import org.codehaus.groovy.ast.expr.ConstantExpression;
+import org.codehaus.groovy.ast.expr.ConstructorCallExpression;
+import org.codehaus.groovy.ast.expr.DeclarationExpression;
+import org.codehaus.groovy.ast.expr.ElvisOperatorExpression;
+import org.codehaus.groovy.ast.expr.Expression;
+import org.codehaus.groovy.ast.expr.FieldExpression;
+import org.codehaus.groovy.ast.expr.GStringExpression;
+import org.codehaus.groovy.ast.expr.ListExpression;
+import org.codehaus.groovy.ast.expr.MapEntryExpression;
+import org.codehaus.groovy.ast.expr.MapExpression;
+import org.codehaus.groovy.ast.expr.MethodCallExpression;
+import org.codehaus.groovy.ast.expr.MethodPointerExpression;
+import org.codehaus.groovy.ast.expr.NotExpression;
+import org.codehaus.groovy.ast.expr.PostfixExpression;
+import org.codehaus.groovy.ast.expr.PrefixExpression;
+import org.codehaus.groovy.ast.expr.PropertyExpression;
+import org.codehaus.groovy.ast.expr.RangeExpression;
+import org.codehaus.groovy.ast.expr.SpreadExpression;
+import org.codehaus.groovy.ast.expr.SpreadMapExpression;
+import org.codehaus.groovy.ast.expr.StaticMethodCallExpression;
+import org.codehaus.groovy.ast.expr.TernaryExpression;
+import org.codehaus.groovy.ast.expr.TupleExpression;
+import org.codehaus.groovy.ast.expr.UnaryMinusExpression;
+import org.codehaus.groovy.ast.expr.UnaryPlusExpression;
+import org.codehaus.groovy.ast.expr.VariableExpression;
+import org.codehaus.groovy.ast.stmt.AssertStatement;
+import org.codehaus.groovy.ast.stmt.BlockStatement;
+import org.codehaus.groovy.ast.stmt.BreakStatement;
+import org.codehaus.groovy.ast.stmt.CaseStatement;
+import org.codehaus.groovy.ast.stmt.CatchStatement;
+import org.codehaus.groovy.ast.stmt.ContinueStatement;
+import org.codehaus.groovy.ast.stmt.DoWhileStatement;
+import org.codehaus.groovy.ast.stmt.ExpressionStatement;
+import org.codehaus.groovy.ast.stmt.ForStatement;
+import org.codehaus.groovy.ast.stmt.IfStatement;
+import org.codehaus.groovy.ast.stmt.ReturnStatement;
+import org.codehaus.groovy.ast.stmt.SwitchStatement;
+import org.codehaus.groovy.ast.stmt.SynchronizedStatement;
+import org.codehaus.groovy.ast.stmt.ThrowStatement;
+import org.codehaus.groovy.ast.stmt.TryCatchStatement;
+import org.codehaus.groovy.ast.stmt.WhileStatement;
+import org.codehaus.groovy.classgen.BytecodeExpression;
+
+
+public class GroovyMethodSanitizer implements GroovyCodeVisitor {
Review Comment:
This class can be significantly simplified if extending `CodeVisitorSupport`.
Why do we want to block on method names? Is this mostly for flexibility?
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyStaticAnalyzerConfig.java:
##########
@@ -0,0 +1,138 @@
+/**
+ * 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.pinot.segment.local.function;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.google.common.base.Preconditions;
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.util.List;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class GroovyStaticAnalyzerConfig {
+ final boolean _enabled;
Review Comment:
`enabled` is not really useful. Let's remove it and it should be enabled whenever the config exists
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyStaticAnalyzerConfig.java:
##########
@@ -0,0 +1,138 @@
+/**
+ * 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.pinot.segment.local.function;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.google.common.base.Preconditions;
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.util.List;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class GroovyStaticAnalyzerConfig {
+ final boolean _enabled;
+ final private List<String> _allowedReceivers;
+ final private List<String> _allowedImports;
+ final private List<String> _allowedStaticImports;
+ final private List<String> _disallowedMethodNames;
+
+ public GroovyStaticAnalyzerConfig(
+ @JsonProperty("enabled")
+ boolean enabled,
+ @JsonProperty("allowedReceivers")
+ List<String> allowedReceivers,
+ @JsonProperty("allowedImports")
+ List<String> allowedImports,
+ @JsonProperty("allowedStaticImports")
+ List<String> allowedStaticImports,
+ @JsonProperty("disallowedMethodNames")
+ List<String> disallowedMethodNames) {
+ _enabled = enabled;
+ _allowedImports = allowedImports;
+ _allowedReceivers = allowedReceivers;
+ _allowedStaticImports = allowedStaticImports;
+ _disallowedMethodNames = disallowedMethodNames;
+ }
+
+ @JsonProperty("enabled")
+ public boolean isEnabled() {
+ return _enabled;
+ }
+
+ @JsonProperty("allowedReceivers")
+ public List<String> getAllowedReceivers() {
+ return _allowedReceivers;
+ }
+
+ @JsonProperty("allowedImports")
+ public List<String> getAllowedImports() {
+ return _allowedImports;
+ }
+
+ @JsonProperty("allowedStaticImports")
+ public List<String> getAllowedStaticImports() {
+ return _allowedStaticImports;
+ }
+
+ @JsonProperty("disallowedMethodNames")
+ public List<String> getDisallowedMethodNames() {
+ return _disallowedMethodNames;
+ }
+
+ public ZNRecord toZNRecord() throws JsonProcessingException {
+ ZNRecord record = new ZNRecord("groovySecurityConfiguration");
Review Comment:
We probably don't want this config to be a separate ZNode. We can consider putting it as a field in the cluster config
##########
pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyFunctionEvaluatorTest.java:
##########
@@ -23,17 +23,55 @@
import java.util.List;
import java.util.Map;
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;
+
/**
* Tests Groovy functions for transforming schema columns
*/
public class GroovyFunctionEvaluatorTest {
+ @Test
+ public void testIllegalGroovyScripts() {
+ // TODO: Add separate tests for these rules: receivers, imports, static imports, and method names.
+ List<String> scripts = List.of(
+ "Groovy({\"ls\".execute()})",
+ "Groovy({[\"ls\"].execute()})",
+ "Groovy({System.exit(5)})",
+ "Groovy({System.metaClass.methods.each { method -> if (method.name.md5() == "
+ + "\"f24f62eeb789199b9b2e467df3b1876b\") {method.invoke(System, 10)} }})",
+ "Groovy({System.metaClass.methods.each { method -> if (method.name.reverse() == (\"ti\" + \"xe\")) "
+ + "{method.invoke(System, 10)} }})",
+ "groovy({def args = [\"QuickStart\", \"-type\", \"REALTIME\"] as String[]; "
+ + "org.apache.pinot.tools.admin.PinotAdministrator.main(args); 2})",
+ "Groovy({return [\"bash\", \"-c\", \"env\"].execute().text})"
+ );
+
+ final GroovyStaticAnalyzerConfig config = new GroovyStaticAnalyzerConfig(
Review Comment:
(format) This doesn't follow `Pinot Style`, please reformat the changes
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovySecurityConfigManager.java:
##########
@@ -0,0 +1,76 @@
+/**
+ * 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.pinot.segment.local.function;
+
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nonnull;
+import org.apache.helix.AccessOption;
+import org.apache.helix.HelixManager;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.zookeeper.data.Stat;
+
+
+public class GroovySecurityConfigManager {
+ public static final String PROPERTYSTORE_PATH = "/CONFIGS/GROOVY_EXECUTION/StaticAnalyzer";
+ private static LoadingCache<Integer, GroovyStaticAnalyzerConfig> _configCache;
+ private static HelixManager _helixManager;
+
+ public GroovySecurityConfigManager(HelixManager helixManager) {
+ _helixManager = helixManager;
+ _configCache = CacheBuilder.newBuilder()
+ .maximumSize(1)
+ .expireAfterWrite(5, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ @Nonnull
Review Comment:
(convention) We don't usually annotate `@Nonnull`, but only annotate `@Nullable`
##########
pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyFunctionEvaluatorTest.java:
##########
@@ -23,17 +23,55 @@
import java.util.List;
import java.util.Map;
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;
+
/**
* Tests Groovy functions for transforming schema columns
*/
public class GroovyFunctionEvaluatorTest {
+ @Test
+ public void testIllegalGroovyScripts() {
+ // TODO: Add separate tests for these rules: receivers, imports, static imports, and method names.
+ List<String> scripts = List.of(
+ "Groovy({\"ls\".execute()})",
+ "Groovy({[\"ls\"].execute()})",
+ "Groovy({System.exit(5)})",
+ "Groovy({System.metaClass.methods.each { method -> if (method.name.md5() == "
+ + "\"f24f62eeb789199b9b2e467df3b1876b\") {method.invoke(System, 10)} }})",
+ "Groovy({System.metaClass.methods.each { method -> if (method.name.reverse() == (\"ti\" + \"xe\")) "
+ + "{method.invoke(System, 10)} }})",
+ "groovy({def args = [\"QuickStart\", \"-type\", \"REALTIME\"] as String[]; "
+ + "org.apache.pinot.tools.admin.PinotAdministrator.main(args); 2})",
+ "Groovy({return [\"bash\", \"-c\", \"env\"].execute().text})"
+ );
+
+ final GroovyStaticAnalyzerConfig config = new GroovyStaticAnalyzerConfig(
Review Comment:
(minor, convention) We don't usually use `final` in local variable, same for other places
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyStaticAnalyzerConfig.java:
##########
@@ -0,0 +1,138 @@
+/**
+ * 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.pinot.segment.local.function;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.google.common.base.Preconditions;
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.util.List;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class GroovyStaticAnalyzerConfig {
+ final boolean _enabled;
+ final private List<String> _allowedReceivers;
Review Comment:
(convention) Put `private` before `final`
##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -945,4 +951,22 @@ private void initSegmentFetcher(PinotConfiguration config)
protected AdminApiApplication createServerAdminApp() {
return new AdminApiApplication(_serverInstance, _accessControlFactory, _serverConf);
}
+
+ private void configureGroovySecurity() {
+ GroovyStaticAnalyzerConfig config = null;
+ try {
+ GroovySecurityConfigManager manager = new GroovySecurityConfigManager(_helixManager);
+ config = manager.getConfig();
+
+ if (config == null) {
+ config = GroovyStaticAnalyzerConfig.createDefault(true);
+ manager.setConfig(config);
+ }
+ } catch (Exception _ex) {
+ LOGGER.error("Failed to read config from ZK. Loading Default configuration.");
Review Comment:
Directly throw exception instead of logging error. We want to fail the server start if security is not guaranteed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1801681911
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java:
##########
@@ -168,4 +170,74 @@ public SuccessResponse deleteClusterConfig(
throw new ControllerApplicationException(LOGGER, errStr, Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @GET
+ @Path("/cluster/groovy/analyzer/static/default")
Review Comment:
The path looks long, shall we use a shorter path? /cluster/config/groovy...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "npawar (via GitHub)" <gi...@apache.org>.
npawar commented on PR #14197:
URL: https://github.com/apache/pinot/pull/14197#issuecomment-2408763744
> pinot.server.query.groovy.analyzer.static
this covers both ingestion time and query time groovy i assume? having it start with "pinot.server.query" would be confusing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1794403262
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyStaticAnalyzerConfig.java:
##########
@@ -0,0 +1,138 @@
+/**
+ * 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.pinot.segment.local.function;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.google.common.base.Preconditions;
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.util.List;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class GroovyStaticAnalyzerConfig {
+ final boolean _enabled;
Review Comment:
Sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1801682674
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java:
##########
@@ -168,4 +170,74 @@ public SuccessResponse deleteClusterConfig(
throw new ControllerApplicationException(LOGGER, errStr, Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @GET
+ @Path("/cluster/groovy/analyzer/static/default")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.GET_GROOVY_SECURITY_CONFIG)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the default configuration for Groovy Static analysis",
+ notes = "Get the default configuration for Groovy static analysis")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public GroovyStaticAnalyzerConfig getDefaultGroovyStaticAnalysisConfig() {
+ return GroovyStaticAnalyzerConfig.createDefault();
+ }
+
+ @GET
+ @Path("/cluster/groovy/analyzer/static/config")
Review Comment:
Same here, use shorter path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #14197:
URL: https://github.com/apache/pinot/pull/14197#issuecomment-2403457716
I find this post very similar to what we wanted: https://melix.github.io/blog/2011/05/12/customizing_groovy_compilation_process.html
Are you following the steps in this post?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #14197:
URL: https://github.com/apache/pinot/pull/14197#issuecomment-2675953687
Close this one and prefer #14844
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #14197:
URL: https://github.com/apache/pinot/pull/14197#issuecomment-2407474467
Not quite sure what is happening with the failing tests, looks like it's some CLP tests that are failing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #14197:
URL: https://github.com/apache/pinot/pull/14197#issuecomment-2412569140
Good catch. Will fix.
On Sat, Oct 12, 2024 at 8:36 PM Neha Pawar ***@***.***> wrote:
> pinot.server.query.groovy.analyzer.static
>
> this covers both ingestion time and query time groovy i assume? having it
> start with "pinot.server.query" would be confusing
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/pinot/pull/14197#issuecomment-2408763744>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BAASDJ7SYAMR7XZOTGADAZTZ3G57LAVCNFSM6AAAAABPVKS7F2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBYG43DGNZUGQ>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1794402605
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyMethodSanitizer.java:
##########
@@ -0,0 +1,390 @@
+/**
+ * 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.pinot.segment.local.function;
+
+import java.util.List;
+import org.codehaus.groovy.ast.GroovyCodeVisitor;
+import org.codehaus.groovy.ast.Parameter;
+import org.codehaus.groovy.ast.expr.ArgumentListExpression;
+import org.codehaus.groovy.ast.expr.ArrayExpression;
+import org.codehaus.groovy.ast.expr.AttributeExpression;
+import org.codehaus.groovy.ast.expr.BinaryExpression;
+import org.codehaus.groovy.ast.expr.BitwiseNegationExpression;
+import org.codehaus.groovy.ast.expr.BooleanExpression;
+import org.codehaus.groovy.ast.expr.CastExpression;
+import org.codehaus.groovy.ast.expr.ClassExpression;
+import org.codehaus.groovy.ast.expr.ClosureExpression;
+import org.codehaus.groovy.ast.expr.ClosureListExpression;
+import org.codehaus.groovy.ast.expr.ConstantExpression;
+import org.codehaus.groovy.ast.expr.ConstructorCallExpression;
+import org.codehaus.groovy.ast.expr.DeclarationExpression;
+import org.codehaus.groovy.ast.expr.ElvisOperatorExpression;
+import org.codehaus.groovy.ast.expr.Expression;
+import org.codehaus.groovy.ast.expr.FieldExpression;
+import org.codehaus.groovy.ast.expr.GStringExpression;
+import org.codehaus.groovy.ast.expr.ListExpression;
+import org.codehaus.groovy.ast.expr.MapEntryExpression;
+import org.codehaus.groovy.ast.expr.MapExpression;
+import org.codehaus.groovy.ast.expr.MethodCallExpression;
+import org.codehaus.groovy.ast.expr.MethodPointerExpression;
+import org.codehaus.groovy.ast.expr.NotExpression;
+import org.codehaus.groovy.ast.expr.PostfixExpression;
+import org.codehaus.groovy.ast.expr.PrefixExpression;
+import org.codehaus.groovy.ast.expr.PropertyExpression;
+import org.codehaus.groovy.ast.expr.RangeExpression;
+import org.codehaus.groovy.ast.expr.SpreadExpression;
+import org.codehaus.groovy.ast.expr.SpreadMapExpression;
+import org.codehaus.groovy.ast.expr.StaticMethodCallExpression;
+import org.codehaus.groovy.ast.expr.TernaryExpression;
+import org.codehaus.groovy.ast.expr.TupleExpression;
+import org.codehaus.groovy.ast.expr.UnaryMinusExpression;
+import org.codehaus.groovy.ast.expr.UnaryPlusExpression;
+import org.codehaus.groovy.ast.expr.VariableExpression;
+import org.codehaus.groovy.ast.stmt.AssertStatement;
+import org.codehaus.groovy.ast.stmt.BlockStatement;
+import org.codehaus.groovy.ast.stmt.BreakStatement;
+import org.codehaus.groovy.ast.stmt.CaseStatement;
+import org.codehaus.groovy.ast.stmt.CatchStatement;
+import org.codehaus.groovy.ast.stmt.ContinueStatement;
+import org.codehaus.groovy.ast.stmt.DoWhileStatement;
+import org.codehaus.groovy.ast.stmt.ExpressionStatement;
+import org.codehaus.groovy.ast.stmt.ForStatement;
+import org.codehaus.groovy.ast.stmt.IfStatement;
+import org.codehaus.groovy.ast.stmt.ReturnStatement;
+import org.codehaus.groovy.ast.stmt.SwitchStatement;
+import org.codehaus.groovy.ast.stmt.SynchronizedStatement;
+import org.codehaus.groovy.ast.stmt.ThrowStatement;
+import org.codehaus.groovy.ast.stmt.TryCatchStatement;
+import org.codehaus.groovy.ast.stmt.WhileStatement;
+import org.codehaus.groovy.classgen.BytecodeExpression;
+
+
+public class GroovyMethodSanitizer implements GroovyCodeVisitor {
Review Comment:
I'll replace this with this feature: https://docs.groovy-lang.org/latest/html/api/org/codehaus/groovy/control/customizers/SecureASTCustomizer.ExpressionChecker.html
There are methods like `execute` (which let's a user execute a string as a shell command) and `invoke` which need to be blocked but can't be blocked with the other rule sets. We want people to be able to use Strings and string operations but want to block `execute`, so adding this Visitor or adding an ExpressionChecker rule to block any call to methods in the disallow list is what I went with here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #14197:
URL: https://github.com/apache/pinot/pull/14197#issuecomment-2403439707
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/14197?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `39.66245%` with `143 lines` in your changes missing coverage. Please review.
> Project coverage is 55.26%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`301941d`)](https://app.codecov.io/gh/apache/pinot/commit/301941da619a8b7c39ec82128b26cc6d2a393db4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 1166 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/pinot/pull/14197?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../segment/local/function/GroovyMethodSanitizer.java](https://app.codecov.io/gh/apache/pinot/pull/14197?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Ffunction%2FGroovyMethodSanitizer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9Hcm9vdnlNZXRob2RTYW5pdGl6ZXIuamF2YQ==) | 25.17% | [103 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/14197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...nt/local/function/GroovySecurityConfigManager.java](https://app.codecov.io/gh/apache/pinot/pull/14197?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Ffunction%2FGroovySecurityConfigManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9Hcm9vdnlTZWN1cml0eUNvbmZpZ01hbmFnZXIuamF2YQ==) | 0.00% | [18 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/14197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...egment/local/function/GroovyFunctionEvaluator.java](https://app.codecov.io/gh/apache/pinot/pull/14197?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Ffunction%2FGroovyFunctionEvaluator.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9Hcm9vdnlGdW5jdGlvbkV2YWx1YXRvci5qYXZh) | 74.46% | [7 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/pinot/pull/14197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ent/local/function/GroovyStaticAnalyzerConfig.java](https://app.codecov.io/gh/apache/pinot/pull/14197?src=pr&el=tree&filepath=pinot-segment-local%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fsegment%2Flocal%2Ffunction%2FGroovyStaticAnalyzerConfig.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9mdW5jdGlvbi9Hcm9vdnlTdGF0aWNBbmFseXplckNvbmZpZy5qYXZh) | 79.31% | [5 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/pinot/pull/14197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (59551e4) and HEAD (301941d). Click for more details.
>
> <details><summary>HEAD has 53 uploads less than BASE</summary>
>
>| Flag | BASE (59551e4) | HEAD (301941d) |
>|------|------|------|
>|integration|7|0|
>|integration2|3|0|
>|temurin|12|1|
>|java-21|7|1|
>|skip-bytebuffers-true|3|1|
>|skip-bytebuffers-false|7|0|
>|unittests|5|1|
>|unittests1|2|1|
>|java-11|5|0|
>|unittests2|3|0|
>|integration1|2|0|
>|custom-integration1|2|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #14197 +/- ##
============================================
- Coverage 61.75% 55.26% -6.49%
- Complexity 207 697 +490
============================================
Files 2436 2070 -366
Lines 133233 109411 -23822
Branches 20636 17293 -3343
============================================
- Hits 82274 60463 -21811
+ Misses 44911 44103 -808
+ Partials 6048 4845 -1203
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.26% <39.66%> (-6.37%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.26% <39.66%> (+27.53%)` | :arrow_up: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.26% <39.66%> (-6.49%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.26% <39.66%> (-6.49%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.26% <39.66%> (+8.37%)` | :arrow_up: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/14197/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/14197?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1794403726
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovySecurityConfigManager.java:
##########
@@ -0,0 +1,76 @@
+/**
+ * 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.pinot.segment.local.function;
+
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import java.util.concurrent.TimeUnit;
+import javax.annotation.Nonnull;
+import org.apache.helix.AccessOption;
+import org.apache.helix.HelixManager;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.zookeeper.data.Stat;
+
+
+public class GroovySecurityConfigManager {
+ public static final String PROPERTYSTORE_PATH = "/CONFIGS/GROOVY_EXECUTION/StaticAnalyzer";
+ private static LoadingCache<Integer, GroovyStaticAnalyzerConfig> _configCache;
+ private static HelixManager _helixManager;
+
+ public GroovySecurityConfigManager(HelixManager helixManager) {
+ _helixManager = helixManager;
+ _configCache = CacheBuilder.newBuilder()
+ .maximumSize(1)
+ .expireAfterWrite(5, TimeUnit.MINUTES)
+ .build(new CacheLoader<>() {
+ @Override
+ @Nonnull
Review Comment:
This was added to match the CacheLoader interface, but will remove.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/function/GroovyStaticAnalyzerConfig.java:
##########
@@ -0,0 +1,138 @@
+/**
+ * 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.pinot.segment.local.function;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.google.common.base.Preconditions;
+import java.math.BigDecimal;
+import java.math.BigInteger;
+import java.util.List;
+import org.apache.helix.zookeeper.datamodel.ZNRecord;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class GroovyStaticAnalyzerConfig {
+ final boolean _enabled;
+ final private List<String> _allowedReceivers;
+ final private List<String> _allowedImports;
+ final private List<String> _allowedStaticImports;
+ final private List<String> _disallowedMethodNames;
+
+ public GroovyStaticAnalyzerConfig(
+ @JsonProperty("enabled")
+ boolean enabled,
+ @JsonProperty("allowedReceivers")
+ List<String> allowedReceivers,
+ @JsonProperty("allowedImports")
+ List<String> allowedImports,
+ @JsonProperty("allowedStaticImports")
+ List<String> allowedStaticImports,
+ @JsonProperty("disallowedMethodNames")
+ List<String> disallowedMethodNames) {
+ _enabled = enabled;
+ _allowedImports = allowedImports;
+ _allowedReceivers = allowedReceivers;
+ _allowedStaticImports = allowedStaticImports;
+ _disallowedMethodNames = disallowedMethodNames;
+ }
+
+ @JsonProperty("enabled")
+ public boolean isEnabled() {
+ return _enabled;
+ }
+
+ @JsonProperty("allowedReceivers")
+ public List<String> getAllowedReceivers() {
+ return _allowedReceivers;
+ }
+
+ @JsonProperty("allowedImports")
+ public List<String> getAllowedImports() {
+ return _allowedImports;
+ }
+
+ @JsonProperty("allowedStaticImports")
+ public List<String> getAllowedStaticImports() {
+ return _allowedStaticImports;
+ }
+
+ @JsonProperty("disallowedMethodNames")
+ public List<String> getDisallowedMethodNames() {
+ return _disallowedMethodNames;
+ }
+
+ public ZNRecord toZNRecord() throws JsonProcessingException {
+ ZNRecord record = new ZNRecord("groovySecurityConfiguration");
Review Comment:
Sure thing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1794403121
##########
pinot-core/src/test/java/org/apache/pinot/core/data/function/GroovyFunctionEvaluatorTest.java:
##########
@@ -23,17 +23,55 @@
import java.util.List;
import java.util.Map;
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;
+
/**
* Tests Groovy functions for transforming schema columns
*/
public class GroovyFunctionEvaluatorTest {
+ @Test
+ public void testIllegalGroovyScripts() {
+ // TODO: Add separate tests for these rules: receivers, imports, static imports, and method names.
+ List<String> scripts = List.of(
+ "Groovy({\"ls\".execute()})",
+ "Groovy({[\"ls\"].execute()})",
+ "Groovy({System.exit(5)})",
+ "Groovy({System.metaClass.methods.each { method -> if (method.name.md5() == "
+ + "\"f24f62eeb789199b9b2e467df3b1876b\") {method.invoke(System, 10)} }})",
+ "Groovy({System.metaClass.methods.each { method -> if (method.name.reverse() == (\"ti\" + \"xe\")) "
+ + "{method.invoke(System, 10)} }})",
+ "groovy({def args = [\"QuickStart\", \"-type\", \"REALTIME\"] as String[]; "
+ + "org.apache.pinot.tools.admin.PinotAdministrator.main(args); 2})",
+ "Groovy({return [\"bash\", \"-c\", \"env\"].execute().text})"
+ );
+
+ final GroovyStaticAnalyzerConfig config = new GroovyStaticAnalyzerConfig(
Review Comment:
Just working shopping how an immutable by default approach to Java would work, I'll get rid of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "npawar (via GitHub)" <gi...@apache.org>.
npawar commented on PR #14197:
URL: https://github.com/apache/pinot/pull/14197#issuecomment-2408765788
> `/cluster/groovy/analyzer/static/config` API,
Is this the right convention to use for such an API? Only until cluster/config/groovy are we doing a hierarchy, so perhaps something like cluster/configs/groovystaticConfig is a more apt name?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1802034731
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java:
##########
@@ -168,4 +170,74 @@ public SuccessResponse deleteClusterConfig(
throw new ControllerApplicationException(LOGGER, errStr, Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @GET
+ @Path("/cluster/groovy/analyzer/static/default")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.GET_GROOVY_SECURITY_CONFIG)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the default configuration for Groovy Static analysis",
+ notes = "Get the default configuration for Groovy static analysis")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public GroovyStaticAnalyzerConfig getDefaultGroovyStaticAnalysisConfig() {
+ return GroovyStaticAnalyzerConfig.createDefault();
Review Comment:
Starting here: https://github.com/apache/pinot/pull/14197/files#diff-106af3bbf7f8d5f13d3ccb902ed2cfc28edb30931dfeb0ac8d64a7a64c373cfeR97
Assuming backwards compatible means that it won't break exist (safe) scripts, then testing so far looks good, but I'm working with Minesh to see what functions current customers are running in Groovy scripts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #14197:
URL: https://github.com/apache/pinot/pull/14197#issuecomment-2412571066
> > `/cluster/groovy/analyzer/static/config` API,
>
> Is this the right convention to use for such an API? Only until cluster/config/groovy are we doing a hierarchy, so perhaps something like cluster/configs/groovystaticConfig is a more apt name?
I initially had that, but then I thought we might add additional Groovy security features (such as executing in a sandbox) in the future, and grouping Groovy configs under `cluster/configs/groovy/` would be more future proof.
perhaps `/cluster/configs/groovy/staticConfig` would be better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1801686832
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java:
##########
@@ -168,4 +170,74 @@ public SuccessResponse deleteClusterConfig(
throw new ControllerApplicationException(LOGGER, errStr, Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @GET
+ @Path("/cluster/groovy/analyzer/static/default")
Review Comment:
Do we need a separate API to the default config? IMO, https://github.com/apache/pinot/pull/14197/files#diff-510a3030bf6c520bbe87caa12b6eff67938c4299feef0c1433f913da810081a3R189 API should get the current active config.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang closed pull request #14197: Secure Excecution of Groovy Code
URL: https://github.com/apache/pinot/pull/14197
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on PR #14197:
URL: https://github.com/apache/pinot/pull/14197#issuecomment-2403639194
> I find this post very similar to what we wanted: https://melix.github.io/blog/2011/05/12/customizing_groovy_compilation_process.html Are you following the steps in this post?
Pretty much, I believe this guy wrote the `SecureASTCustomizer` I used and was one of the first documents about securing Groovy that I found.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1801685282
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java:
##########
@@ -168,4 +170,74 @@ public SuccessResponse deleteClusterConfig(
throw new ControllerApplicationException(LOGGER, errStr, Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @GET
+ @Path("/cluster/groovy/analyzer/static/default")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.GET_GROOVY_SECURITY_CONFIG)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the default configuration for Groovy Static analysis",
+ notes = "Get the default configuration for Groovy static analysis")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public GroovyStaticAnalyzerConfig getDefaultGroovyStaticAnalysisConfig() {
+ return GroovyStaticAnalyzerConfig.createDefault();
Review Comment:
What is the default security config, and is it backward compatible?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1802030415
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java:
##########
@@ -168,4 +170,74 @@ public SuccessResponse deleteClusterConfig(
throw new ControllerApplicationException(LOGGER, errStr, Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @GET
+ @Path("/cluster/groovy/analyzer/static/default")
+ @Authorize(targetType = TargetType.CLUSTER, action = Actions.Cluster.GET_GROOVY_SECURITY_CONFIG)
+ @Produces(MediaType.APPLICATION_JSON)
+ @ApiOperation(value = "Get the default configuration for Groovy Static analysis",
+ notes = "Get the default configuration for Groovy static analysis")
+ @ApiResponses(value = {
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 500, message = "Internal server error")
+ })
+ public GroovyStaticAnalyzerConfig getDefaultGroovyStaticAnalysisConfig() {
+ return GroovyStaticAnalyzerConfig.createDefault();
+ }
+
+ @GET
+ @Path("/cluster/groovy/analyzer/static/config")
Review Comment:
perhaps we can assume that anything under `/cluster` is a config and use `cluster/groovy/staticAnalysis`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] Secure Excecution of Groovy Code [pinot]
Posted by "ege-st (via GitHub)" <gi...@apache.org>.
ege-st commented on code in PR #14197:
URL: https://github.com/apache/pinot/pull/14197#discussion_r1802036434
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotClusterConfigs.java:
##########
@@ -168,4 +170,74 @@ public SuccessResponse deleteClusterConfig(
throw new ControllerApplicationException(LOGGER, errStr, Response.Status.INTERNAL_SERVER_ERROR, e);
}
}
+
+ @GET
+ @Path("/cluster/groovy/analyzer/static/default")
Review Comment:
Current design is that if the feature is disabled if there is no config and enabled if there is a config (per a comment on an older version of this PR). Hence adding a method to easily get a default config that can then be used to turn on this feature.
Alternatively, a query param could be added to the Set method which, when true, will set the configuration to the Default configuration and if false will use the config provided in the request body.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org