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 <jil...@pivotal.io>
Authored: Wed Nov 30 10:16:58 2016 -0800
Committer: Jinmei Liao <jil...@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

Reply via email to