You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@geode.apache.org by ji...@apache.org on 2016/12/01 17:37:43 UTC
incubator-geode git commit: GEODE-2146: deploy should require more
elevated privileges than just data:manage
Repository: incubator-geode
Updated Branches:
refs/heads/develop 6eb62eb7f -> 4f67348dc
GEODE-2146: deploy should require more elevated privileges than just data:manage
Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/4f67348d
Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/4f67348d
Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/4f67348d
Branch: refs/heads/develop
Commit: 4f67348dc974867e4b68b8b0e9467c97311e6ede
Parents: 6eb62eb
Author: Jinmei Liao <ji...@pivotal.io>
Authored: Wed Nov 30 10:16:58 2016 -0800
Committer: Jinmei Liao <ji...@pivotal.io>
Committed: Thu Dec 1 08:12:22 2016 -0800
----------------------------------------------------------------------
.../cli/commands/AbstractCommandsSupport.java | 13 +-
.../internal/cli/commands/DeployCommands.java | 28 +++--
.../security/DeployCommandsSecurityTest.java | 124 +++++++++++++++++++
.../security/SimpleSecurityManagerTest.java | 27 ++--
.../security/SimpleTestSecurityManager.java | 12 +-
5 files changed, 177 insertions(+), 27 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/4f67348d/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/AbstractCommandsSupport.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/AbstractCommandsSupport.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/AbstractCommandsSupport.java
index d5403d9..6db6415 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/AbstractCommandsSupport.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/AbstractCommandsSupport.java
@@ -15,11 +15,6 @@
package org.apache.geode.management.internal.cli.commands;
-import java.io.PrintWriter;
-import java.io.StringWriter;
-import java.util.HashSet;
-import java.util.Set;
-
import org.apache.geode.cache.Cache;
import org.apache.geode.cache.CacheFactory;
import org.apache.geode.cache.execute.Execution;
@@ -27,13 +22,18 @@ import org.apache.geode.cache.execute.Function;
import org.apache.geode.cache.execute.FunctionService;
import org.apache.geode.distributed.DistributedMember;
import org.apache.geode.internal.lang.StringUtils;
+import org.apache.geode.internal.security.SecurityService;
import org.apache.geode.management.cli.CliMetaData;
import org.apache.geode.management.internal.cli.i18n.CliStrings;
import org.apache.geode.management.internal.cli.shell.Gfsh;
import org.apache.geode.management.internal.cli.util.MemberNotFoundException;
-
import org.springframework.shell.core.CommandMarker;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.HashSet;
+import java.util.Set;
+
/**
* The AbstractCommandsSupport class is an abstract base class encapsulating common functionality
* for implementing command classes with command for the GemFire shell (gfsh).
@@ -48,6 +48,7 @@ import org.springframework.shell.core.CommandMarker;
*/
@SuppressWarnings("unused")
public abstract class AbstractCommandsSupport implements CommandMarker {
+ protected static SecurityService securityService = SecurityService.getSecurityService();
protected static void assertArgument(final boolean valid, final String message,
final Object... args) {
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/4f67348d/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java
----------------------------------------------------------------------
diff --git a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java
index 832207b..a5302ea 100644
--- a/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java
+++ b/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommands.java
@@ -14,13 +14,6 @@
*/
package org.apache.geode.management.internal.cli.commands;
-import java.io.FileNotFoundException;
-import java.io.IOException;
-import java.text.DecimalFormat;
-import java.util.List;
-import java.util.Map;
-import java.util.Set;
-
import org.apache.geode.SystemFailure;
import org.apache.geode.cache.execute.ResultCollector;
import org.apache.geode.distributed.DistributedMember;
@@ -43,14 +36,21 @@ import org.apache.geode.management.internal.cli.result.ResultBuilder;
import org.apache.geode.management.internal.cli.result.TabularResultData;
import org.apache.geode.management.internal.configuration.SharedConfigurationWriter;
import org.apache.geode.management.internal.security.ResourceOperation;
+import org.apache.geode.security.NotAuthorizedException;
import org.apache.geode.security.ResourcePermission.Operation;
import org.apache.geode.security.ResourcePermission.Resource;
-
import org.springframework.shell.core.CommandMarker;
import org.springframework.shell.core.annotation.CliAvailabilityIndicator;
import org.springframework.shell.core.annotation.CliCommand;
import org.springframework.shell.core.annotation.CliOption;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.text.DecimalFormat;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
/**
* Commands for deploying, un-deploying and listing files deployed using the command line shell.
@@ -77,7 +77,6 @@ public final class DeployCommands extends AbstractCommandsSupport implements Com
@CliMetaData(
interceptor = "org.apache.geode.management.internal.cli.commands.DeployCommands$Interceptor",
relatedTopic = {CliStrings.TOPIC_GEODE_CONFIG}, writesToSharedConfiguration = true)
- @ResourceOperation(resource = Resource.DATA, operation = Operation.MANAGE)
public final Result deploy(
@CliOption(key = {CliStrings.DEPLOY__GROUP}, help = CliStrings.DEPLOY__GROUP__HELP,
optionContext = ConverterHint.MEMBERGROUP) @CliMetaData(
@@ -85,6 +84,14 @@ public final class DeployCommands extends AbstractCommandsSupport implements Com
@CliOption(key = {CliStrings.DEPLOY__JAR}, help = CliStrings.DEPLOY__JAR__HELP) String jar,
@CliOption(key = {CliStrings.DEPLOY__DIR}, help = CliStrings.DEPLOY__DIR__HELP) String dir) {
try {
+
+ // since deploy function can potentially do a lot of damage to security, this action should
+ // require these following privileges
+ securityService.authorizeClusterManage();
+ securityService.authorizeClusterWrite();
+ securityService.authorizeDataManage();
+ securityService.authorizeDataWrite();
+
TabularResultData tabularData = ResultBuilder.createTabularResultData();
byte[][] shellBytesData = CommandExecutionContext.getBytesFromShell();
@@ -140,6 +147,9 @@ public final class DeployCommands extends AbstractCommandsSupport implements Com
(new SharedConfigurationWriter()).addJars(jarNames, jarBytes, groups));
}
return result;
+ } catch (NotAuthorizedException e) {
+ // for NotAuthorizedException, will catch this later in the code
+ throw e;
} catch (VirtualMachineError e) {
SystemFailure.initiateFailure(e);
throw e;
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/4f67348d/geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java b/geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java
new file mode 100644
index 0000000..b040751
--- /dev/null
+++ b/geode-core/src/test/java/org/apache/geode/management/internal/security/DeployCommandsSecurityTest.java
@@ -0,0 +1,124 @@
+/*
+ * 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.geode.management.internal.security;
+
+import static org.apache.geode.distributed.ConfigurationProperties.JMX_MANAGER_PORT;
+import static org.apache.geode.distributed.ConfigurationProperties.SECURITY_MANAGER;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.junit.Assert.assertTrue;
+
+import org.apache.geode.internal.AvailablePort;
+import org.apache.geode.management.MemberMXBean;
+import org.apache.geode.security.NotAuthorizedException;
+import org.apache.geode.security.SimpleTestSecurityManager;
+import org.apache.geode.test.dunit.rules.ConnectionConfiguration;
+import org.apache.geode.test.dunit.rules.MBeanServerConnectionRule;
+import org.apache.geode.test.dunit.rules.ServerStarterRule;
+import org.apache.geode.test.junit.categories.IntegrationTest;
+import org.apache.geode.test.junit.categories.SecurityTest;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.TemporaryFolder;
+
+import java.io.File;
+import java.util.Properties;
+
+@Category({IntegrationTest.class, SecurityTest.class})
+public class DeployCommandsSecurityTest {
+
+ private static int jmxManagerPort = AvailablePort.getRandomAvailablePort(AvailablePort.SOCKET);
+
+ private MemberMXBean bean;
+
+ @ClassRule
+ public static ServerStarterRule serverRule = new ServerStarterRule(new Properties() {
+ {
+ setProperty(SECURITY_MANAGER, SimpleTestSecurityManager.class.getName());
+ setProperty(JMX_MANAGER_PORT, jmxManagerPort + "");
+ }
+ });
+
+ @ClassRule
+ public static TemporaryFolder temporaryFolder = new TemporaryFolder();
+ private static String deployCommand = null;
+ private static String zipFileName = "functions.jar";
+
+ @BeforeClass
+ public static void beforeClass() throws Exception {
+ File zipFile = temporaryFolder.newFile(zipFileName);
+ zipFile.createNewFile();
+ deployCommand = "deploy --jar=" + zipFile.getAbsolutePath();
+ }
+
+ @Rule
+ public MBeanServerConnectionRule connectionRule = new MBeanServerConnectionRule(jmxManagerPort);
+
+ @Before
+ public void setUp() throws Exception {
+ bean = connectionRule.getProxyMBean(MemberMXBean.class);
+ }
+
+
+ @Test // regular user can't deploy
+ @ConnectionConfiguration(user = "user", password = "user")
+ public void testNoAccess1() {
+ assertThatThrownBy(() -> bean.processCommand(deployCommand))
+ .isInstanceOf(NotAuthorizedException.class);
+ }
+
+ @Test // only data access right is not enough to deploy
+ @ConnectionConfiguration(user = "data", password = "data")
+ public void testNoAccess2() {
+ assertThatThrownBy(() -> bean.processCommand(deployCommand))
+ .isInstanceOf(NotAuthorizedException.class);
+ }
+
+ @Test // only cluster access right is not enough to deploy
+ @ConnectionConfiguration(user = "cluster", password = "cluster")
+ public void testNoAccess3() {
+ assertThatThrownBy(() -> bean.processCommand(deployCommand))
+ .isInstanceOf(NotAuthorizedException.class);
+ }
+
+ @Test // not sufficient privalge
+ @ConnectionConfiguration(user = "clusterRead,clusterWrite,dataRead,dataWrite",
+ password = "clusterRead,clusterWrite,dataRead,dataWrite")
+ public void testNoAccess4() {
+ assertThatThrownBy(() -> bean.processCommand(deployCommand))
+ .isInstanceOf(NotAuthorizedException.class);
+ }
+
+ @Test // only power user can deploy
+ @ConnectionConfiguration(user = "cluster,data", password = "cluster,data")
+ public void testPowerAccess1() {
+ String result = bean.processCommand(deployCommand);
+ assertTrue(result.contains("File does not contain valid JAR content: functions.jar"));
+ }
+
+ @Test // only power user can deploy
+ @ConnectionConfiguration(user = "clusterManage,clusterWrite,dataManage,dataWrite",
+ password = "clusterManage,clusterWrite,dataManage,dataWrite")
+ public void testPowerAccess2() {
+ String result = bean.processCommand(deployCommand);
+ assertTrue(result.contains("File does not contain valid JAR content: functions.jar"));
+ }
+
+
+
+}
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/4f67348d/geode-core/src/test/java/org/apache/geode/security/SimpleSecurityManagerTest.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/org/apache/geode/security/SimpleSecurityManagerTest.java b/geode-core/src/test/java/org/apache/geode/security/SimpleSecurityManagerTest.java
index 066c139..2d6fbca 100644
--- a/geode-core/src/test/java/org/apache/geode/security/SimpleSecurityManagerTest.java
+++ b/geode-core/src/test/java/org/apache/geode/security/SimpleSecurityManagerTest.java
@@ -16,20 +16,17 @@
package org.apache.geode.security;
import static org.apache.geode.internal.Assert.assertTrue;
-import static org.assertj.core.api.Assertions.*;
-import static org.junit.Assert.*;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
-import java.util.Properties;
-
-import org.apache.geode.security.SimpleTestSecurityManager;
+import org.apache.geode.test.junit.categories.SecurityTest;
+import org.apache.geode.test.junit.categories.UnitTest;
import org.junit.Before;
import org.junit.Test;
import org.junit.experimental.categories.Category;
-import org.apache.geode.security.AuthenticationFailedException;
-import org.apache.geode.security.ResourcePermission;
-import org.apache.geode.test.junit.categories.SecurityTest;
-import org.apache.geode.test.junit.categories.UnitTest;
+import java.util.Properties;
@Category({UnitTest.class, SecurityTest.class})
public class SimpleSecurityManagerTest {
@@ -78,4 +75,16 @@ public class SimpleSecurityManagerTest {
assertFalse(manager.authorize("dataRead", permission));
}
+ @Test
+ public void testMultipleRoleAuthorization() {
+ ResourcePermission permission = new ResourcePermission("CLUSTER", "READ");
+ assertTrue(manager.authorize("clusterRead,clusterWrite", permission));
+ assertTrue(manager.authorize("cluster,data", permission));
+ assertFalse(manager.authorize("clusterWrite,data", permission));
+
+ permission = new ResourcePermission("DATA", "WRITE", "regionA", "key1");
+ assertTrue(manager.authorize("data,cluster", permission));
+ assertTrue(manager.authorize("dataWrite,clusterWrite", permission));
+ }
+
}
http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/4f67348d/geode-core/src/test/java/org/apache/geode/security/SimpleTestSecurityManager.java
----------------------------------------------------------------------
diff --git a/geode-core/src/test/java/org/apache/geode/security/SimpleTestSecurityManager.java b/geode-core/src/test/java/org/apache/geode/security/SimpleTestSecurityManager.java
index 0db9825..c754376 100644
--- a/geode-core/src/test/java/org/apache/geode/security/SimpleTestSecurityManager.java
+++ b/geode-core/src/test/java/org/apache/geode/security/SimpleTestSecurityManager.java
@@ -31,6 +31,8 @@ import java.util.Properties;
* data:read data:write username = dataWrite: is authorized for data writes on all regions:
* data:write data:write:regionA username = cluster: authorized for all cluster operations username
* = cluserRead: authorzed for all cluster read operations
+ *
+ * a user could be a comma separated list of roles as well.
*/
public class SimpleTestSecurityManager implements SecurityManager {
@Override
@@ -48,9 +50,13 @@ public class SimpleTestSecurityManager implements SecurityManager {
@Override
public boolean authorize(final Object principal, final ResourcePermission permission) {
- String permissionString = permission.toString().replace(":", "").toLowerCase();
- String principle = principal.toString().toLowerCase();
- return permissionString.startsWith(principle);
+ String[] principals = principal.toString().toLowerCase().split(",");
+ for (String role : principals) {
+ String permissionString = permission.toString().replace(":", "").toLowerCase();
+ if (permissionString.startsWith(role))
+ return true;
+ }
+ return false;
}
@Override