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/feature/GEODE-2156 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