From 7d272ea124615c493c60ad454fbd6f144dd3cc24 Mon Sep 17 00:00:00 2001 From: Steve Loughran Date: Wed, 12 Jul 2017 10:37:39 +0100 Subject: [PATCH] HADOOP-14581. Restrict setOwner to list of user when security is enabled in wasb. Contributed by Varada Hemeswari (cherry picked from commit 1e69e5260351effc8077d1bdc397cec57cf1ff1b) --- .../fs/azure/NativeAzureFileSystem.java | 37 ++++ .../hadoop-azure/src/site/markdown/index.md | 12 ++ ...estNativeAzureFileSystemAuthorization.java | 173 +++++++++++++++++- 3 files changed, 221 insertions(+), 1 deletion(-) diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java index a45ea81cc0..76c6083d89 100644 --- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java +++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java @@ -38,6 +38,8 @@ import java.util.concurrent.atomic.AtomicInteger; import java.util.regex.Matcher; import java.util.regex.Pattern; +import java.util.Arrays; +import java.util.List; import com.fasterxml.jackson.core.JsonParseException; import com.fasterxml.jackson.core.JsonParser; @@ -687,6 +689,14 @@ public String getScheme() { */ static final String AZURE_DEFAULT_GROUP_DEFAULT = "supergroup"; + /** + * Configuration property used to specify list of users that can perform + * chown operation when authorization is enabled in WASB. + */ + public static final String AZURE_CHOWN_USERLIST_PROPERTY_NAME = "fs.azure.chown.allowed.userlist"; + + static final String AZURE_CHOWN_USERLIST_PROPERTY_DEFAULT_VALUE = "*"; + static final String AZURE_BLOCK_LOCATION_HOST_PROPERTY_NAME = "fs.azure.block.location.impersonatedhost"; private static final String AZURE_BLOCK_LOCATION_HOST_DEFAULT = @@ -2930,6 +2940,33 @@ public void setOwner(Path p, String username, String groupname) throw new FileNotFoundException("File doesn't exist: " + p); } + /* If authorization is enabled, check if the user has privileges + * to change the ownership of file/folder + */ + if (this.azureAuthorization && username != null) { + String[] listOfUsers = getConf().getTrimmedStrings(AZURE_CHOWN_USERLIST_PROPERTY_NAME, + AZURE_CHOWN_USERLIST_PROPERTY_DEFAULT_VALUE); + boolean shouldSkipUserCheck = listOfUsers.length == 1 && listOfUsers[0].equals("*"); + // skip the check if the chown allowed users config value is set as '*' + if (!shouldSkipUserCheck) { + UserGroupInformation currentUgi = UserGroupInformation.getCurrentUser(); + UserGroupInformation actualUgi = currentUgi.getRealUser(); + + if (actualUgi == null) { + actualUgi = currentUgi; + } + + List userList = Arrays.asList(listOfUsers); + if (userList.contains("*")) { + throw new IllegalArgumentException("User list must contain " + + "either '*' or list of user names, but not both."); + } else if (!userList.contains(actualUgi.getShortUserName())) { + throw new WasbAuthorizationException(String.format("user '%s' does not have the privilege to change the ownership of files/folders.", + actualUgi.getShortUserName())); + } + } + } + PermissionStatus newPermissionStatus = new PermissionStatus( username == null ? metadata.getPermissionStatus().getUserName() : username, diff --git a/hadoop-tools/hadoop-azure/src/site/markdown/index.md b/hadoop-tools/hadoop-azure/src/site/markdown/index.md index 740be52ba9..74717bea65 100644 --- a/hadoop-tools/hadoop-azure/src/site/markdown/index.md +++ b/hadoop-tools/hadoop-azure/src/site/markdown/index.md @@ -412,6 +412,18 @@ The service is expected to return a response in JSON format for GETDELEGATIONTOK } } ``` +### chown behaviour when authorization is enabled in WASB + +When authorization is enabled, only the users listed in the following configuration +are allowed to change the owning user of files/folders in WASB. The configuration +value takes a comma seperated list of user names who are allowed to perform chown. + +```xml + + fs.azure.chown.allowed.userlist + user1,user2 + +``` ## Testing the hadoop-azure Module diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java index fbd7f62fc0..862fd48f08 100644 --- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java +++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java @@ -18,11 +18,16 @@ package org.apache.hadoop.fs.azure; +import java.security.PrivilegedExceptionAction; + import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FSDataInputStream; import org.apache.hadoop.fs.FSDataOutputStream; import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.contract.ContractTestUtils; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.hadoop.util.StringUtils; + import org.junit.Assume; import org.junit.Before; import org.junit.Rule; @@ -32,6 +37,7 @@ import com.google.common.annotations.VisibleForTesting; import static org.apache.hadoop.fs.azure.AzureNativeFileSystemStore.KEY_USE_SECURE_MODE; +import static org.junit.Assert.assertEquals; /** * Test class to hold all WASB authorization tests. @@ -47,6 +53,7 @@ protected AzureBlobStorageTestAccount createTestAccount() throws Exception { Configuration conf = new Configuration(); conf.set(NativeAzureFileSystem.KEY_AZURE_AUTHORIZATION, "true"); conf.set(RemoteWasbAuthorizerImpl.KEY_REMOTE_AUTH_SERVICE_URLS, "http://localhost/"); + conf.set(NativeAzureFileSystem.AZURE_CHOWN_USERLIST_PROPERTY_NAME, "user1 , user2"); return AzureBlobStorageTestAccount.create(conf); } @@ -673,4 +680,168 @@ public void testListStatusWithTripleSlashCheckPositive() throws Throwable { Path testPathWithTripleSlash = new Path("wasb:///" + testPath); fs.listStatus(testPathWithTripleSlash); } -} \ No newline at end of file + + /** + * Negative test for setOwner when Authorization is enabled + */ + @Test + public void testSetOwnerThrowsForUnauthorisedUsers() throws Throwable { + expectedEx.expect(WasbAuthorizationException.class); + + Path testPath = new Path("/testSetOwnerNegative"); + MockWasbAuthorizerImpl authorizer = new MockWasbAuthorizerImpl(fs); + authorizer.init(null); + authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true); + authorizer.addAuthRule(testPath.toString(), WasbAuthorizationOperations.READ.toString(), true); + fs.updateWasbAuthorizer(authorizer); + + String owner = null; + UserGroupInformation unauthorisedUser = UserGroupInformation.createUserForTesting( + "unauthoriseduser", new String[] {"group1"}); + try { + fs.mkdirs(testPath); + ContractTestUtils.assertPathExists(fs, "test path does not exist", testPath); + owner = fs.getFileStatus(testPath).getOwner(); + + unauthorisedUser.doAs(new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + fs.setOwner(testPath, "newowner", null); + return null; + } + }); + } finally { + // check that the owner is not modified + assertEquals(owner, fs.getFileStatus(testPath).getOwner()); + fs.delete(testPath, false); + } + } + + /** + * Test for setOwner when Authorization is enabled and + * the user is specified in chown allowed user list + * */ + @Test + public void testSetOwnerSucceedsForAuthorisedUsers() throws Throwable { + Path testPath = new Path("/testsetownerpositive"); + MockWasbAuthorizerImpl authorizer = new MockWasbAuthorizerImpl(fs); + authorizer.init(null); + authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true); + authorizer.addAuthRule(testPath.toString(), WasbAuthorizationOperations.READ.toString(), true); + fs.updateWasbAuthorizer(authorizer); + + String newOwner = "newowner"; + String newGroup = "newgroup"; + + UserGroupInformation authorisedUser = UserGroupInformation.createUserForTesting( + "user2", new String[]{"group1"}); + try { + + fs.mkdirs(testPath); + ContractTestUtils.assertPathExists(fs, "test path does not exist", testPath); + + String owner = fs.getFileStatus(testPath).getOwner(); + Assume.assumeTrue("changing owner requires original and new owner to be different", + !StringUtils.equalsIgnoreCase(owner, newOwner)); + + authorisedUser.doAs(new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + fs.setOwner(testPath, newOwner, newGroup); + assertEquals(newOwner, fs.getFileStatus(testPath).getOwner()); + assertEquals(newGroup, fs.getFileStatus(testPath).getGroup()); + return null; + } + }); + + } finally { + fs.delete(testPath, false); + } + } + + /** + * Test for setOwner when Authorization is enabled and + * the userlist is specified as '*' + * */ + @Test + public void testSetOwnerSucceedsForAnyUserWhenWildCardIsSpecified() throws Throwable { + Configuration conf = fs.getConf(); + conf.set(NativeAzureFileSystem.AZURE_CHOWN_USERLIST_PROPERTY_NAME, "*"); + Path testPath = new Path("/testsetownerpositivewildcard"); + + MockWasbAuthorizerImpl authorizer = new MockWasbAuthorizerImpl(fs); + authorizer.init(null); + authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true); + authorizer.addAuthRule(testPath.toString(), WasbAuthorizationOperations.READ.toString(), true); + fs.updateWasbAuthorizer(authorizer); + + String newOwner = "newowner"; + String newGroup = "newgroup"; + + UserGroupInformation user = UserGroupInformation.createUserForTesting( + "anyuser", new String[]{"group1"}); + try { + + fs.mkdirs(testPath); + ContractTestUtils.assertPathExists(fs, "test path does not exist", testPath); + + String owner = fs.getFileStatus(testPath).getOwner(); + Assume.assumeTrue("changing owner requires original and new owner to be different", + !StringUtils.equalsIgnoreCase(owner, newOwner)); + + user.doAs(new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + fs.setOwner(testPath, newOwner, newGroup); + assertEquals(newOwner, fs.getFileStatus(testPath).getOwner()); + assertEquals(newGroup, fs.getFileStatus(testPath).getGroup()); + return null; + } + }); + + } finally { + fs.delete(testPath, false); + } + } + + /** Test for setOwner throws for illegal setup of chown + * allowed testSetOwnerSucceedsForAuthorisedUsers + */ + @Test + public void testSetOwnerFailsForIllegalSetup() throws Throwable { + expectedEx.expect(IllegalArgumentException.class); + + Configuration conf = fs.getConf(); + conf.set(NativeAzureFileSystem.AZURE_CHOWN_USERLIST_PROPERTY_NAME, "user1, *"); + Path testPath = new Path("/testSetOwnerFailsForIllegalSetup"); + + MockWasbAuthorizerImpl authorizer = new MockWasbAuthorizerImpl(fs); + authorizer.init(null); + authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true); + authorizer.addAuthRule(testPath.toString(), WasbAuthorizationOperations.READ.toString(), true); + fs.updateWasbAuthorizer(authorizer); + + String owner = null; + UserGroupInformation user = UserGroupInformation.createUserForTesting( + "anyuser", new String[]{"group1"}); + try { + + fs.mkdirs(testPath); + ContractTestUtils.assertPathExists(fs, "test path does not exist", testPath); + + owner = fs.getFileStatus(testPath).getOwner(); + + user.doAs(new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + fs.setOwner(testPath, "newowner", null); + return null; + } + }); + } finally { + // check that the owner is not modified + assertEquals(owner, fs.getFileStatus(testPath).getOwner()); + fs.delete(testPath, false); + } + } +}