FALCON-839 Authorization succeds with invalid acl owner based on group membership. Contributed by Venkatesh Seetharam
Project: http://git-wip-us.apache.org/repos/asf/incubator-falcon/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-falcon/commit/b49360e7 Tree: http://git-wip-us.apache.org/repos/asf/incubator-falcon/tree/b49360e7 Diff: http://git-wip-us.apache.org/repos/asf/incubator-falcon/diff/b49360e7 Branch: refs/heads/master Commit: b49360e77d69d5a7f2b027aadb561e95dd94b422 Parents: 35506d5 Author: Venkatesh Seetharam <venkat...@apache.org> Authored: Tue Oct 28 15:32:07 2014 -0700 Committer: Venkatesh Seetharam <venkat...@apache.org> Committed: Tue Oct 28 22:22:32 2014 -0700 ---------------------------------------------------------------------- CHANGES.txt | 3 ++ .../security/DefaultAuthorizationProvider.java | 40 +++++++++++++++++++- .../apache/falcon/entity/AbstractTestBase.java | 7 +--- .../entity/parser/FeedEntityParserTest.java | 21 +++++----- .../entity/parser/ProcessEntityParserTest.java | 4 +- .../DefaultAuthorizationProviderTest.java | 4 +- docs/src/site/twiki/Security.twiki | 5 +++ .../security/FalconAuthorizationFilter.java | 2 +- .../falcon/resource/EntityManagerTest.java | 39 +++++++++++++------ 9 files changed, 93 insertions(+), 32 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/b49360e7/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 6259566..625dd43 100755 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -125,6 +125,9 @@ Trunk (Unreleased) OPTIMIZATIONS BUG FIXES + FALCON-839 Authorization succeeds with invalid acl owner based on group + membership (Venkatesh Seetharam) + FALCON-831 Operation on non existing entity throws internal server error (Venkatesh Seetharam) http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/b49360e7/common/src/main/java/org/apache/falcon/security/DefaultAuthorizationProvider.java ---------------------------------------------------------------------- diff --git a/common/src/main/java/org/apache/falcon/security/DefaultAuthorizationProvider.java b/common/src/main/java/org/apache/falcon/security/DefaultAuthorizationProvider.java index 4b7c4a9..6b80a1b 100644 --- a/common/src/main/java/org/apache/falcon/security/DefaultAuthorizationProvider.java +++ b/common/src/main/java/org/apache/falcon/security/DefaultAuthorizationProvider.java @@ -31,6 +31,7 @@ import org.apache.hadoop.security.authorize.AuthorizationException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.IOException; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; @@ -179,6 +180,7 @@ public class DefaultAuthorizationProvider implements AuthorizationProvider { authenticatedUser, action, entityName, entityType); if (isSuperUser(proxyUgi)) { + validateACLOwnerAndGroup(acl.getOwner(), acl.getGroup()); return; } @@ -186,6 +188,31 @@ public class DefaultAuthorizationProvider implements AuthorizationProvider { } /** + * Checks if the acl owner is a valid user by fetching the groups for the owner. + * Also checks if the acl group is one of the fetched groups for membership. + * The only limitation is that a user cannot add a group in ACL that he does not belong to. + * + * @param aclOwner ACL owner + * @param aclGroup ACL group + * @throws AuthorizationException + */ + protected void validateACLOwnerAndGroup(String aclOwner, + String aclGroup) throws AuthorizationException { + try { + UserGroupInformation proxyACLUser = UserGroupInformation.createProxyUser( + aclOwner, UserGroupInformation.getLoginUser()); + Set<String> groups = new HashSet<String>(Arrays.asList(proxyACLUser.getGroupNames())); + if (!isUserInGroup(aclGroup, groups)) { + throw new AuthorizationException("Invalid group: " + aclGroup + + " for user: " + aclOwner); + } + } catch (IOException e) { + throw new AuthorizationException("Invalid acl owner " + aclOwner + + ", does not exist or does not belong to group: " + aclGroup); + } + } + + /** * Validate if the entity owner is the logged-in authenticated user. * * @param entityName entity name. @@ -200,7 +227,7 @@ public class DefaultAuthorizationProvider implements AuthorizationProvider { String action, String authenticatedUser, UserGroupInformation proxyUgi) throws AuthorizationException { if (isUserACLOwner(authenticatedUser, aclOwner) - || isUserInGroup(aclGroup, proxyUgi)) { + && isUserInGroup(aclGroup, proxyUgi)) { return; } @@ -235,6 +262,17 @@ public class DefaultAuthorizationProvider implements AuthorizationProvider { */ protected boolean isUserInGroup(String group, UserGroupInformation proxyUgi) { Set<String> groups = getGroupNames(proxyUgi); + return isUserInGroup(group, groups); + } + + /** + * Checks if the user's group matches the entity ACL group. + * + * @param group Entity ACL group. + * @param groups set of groups for the authenticated user. + * @return true if user groups contains entity acl group. + */ + protected boolean isUserInGroup(String group, Set<String> groups) { return groups.contains(group); } http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/b49360e7/common/src/test/java/org/apache/falcon/entity/AbstractTestBase.java ---------------------------------------------------------------------- diff --git a/common/src/test/java/org/apache/falcon/entity/AbstractTestBase.java b/common/src/test/java/org/apache/falcon/entity/AbstractTestBase.java index a1319b8..ece1794 100644 --- a/common/src/test/java/org/apache/falcon/entity/AbstractTestBase.java +++ b/common/src/test/java/org/apache/falcon/entity/AbstractTestBase.java @@ -43,7 +43,6 @@ import java.io.File; import java.io.IOException; import java.io.StringWriter; import java.net.URI; -import java.util.Arrays; import java.util.Collection; /** @@ -193,9 +192,7 @@ public class AbstractTestBase { } // assumes there will always be at least one group for a logged in user - protected String getGroupName() throws IOException { - String[] groupNames = CurrentUser.getProxyUGI().getGroupNames(); - System.out.println("groupNames = " + Arrays.asList(groupNames)); - return groupNames[0]; + protected String getPrimaryGroupName() throws IOException { + return CurrentUser.getProxyUGI().getPrimaryGroupName(); } } http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/b49360e7/common/src/test/java/org/apache/falcon/entity/parser/FeedEntityParserTest.java ---------------------------------------------------------------------- diff --git a/common/src/test/java/org/apache/falcon/entity/parser/FeedEntityParserTest.java b/common/src/test/java/org/apache/falcon/entity/parser/FeedEntityParserTest.java index 98cda04..3a9d508 100644 --- a/common/src/test/java/org/apache/falcon/entity/parser/FeedEntityParserTest.java +++ b/common/src/test/java/org/apache/falcon/entity/parser/FeedEntityParserTest.java @@ -567,18 +567,21 @@ public class FeedEntityParserTest extends AbstractTestBase { Assert.assertTrue(Boolean.valueOf( StartupProperties.get().getProperty("falcon.security.authorization.enabled"))); + CurrentUser.authenticate(USER); try { InputStream stream = this.getClass().getResourceAsStream(FEED_XML); // need a new parser since it caches authorization enabled flag FeedEntityParser feedEntityParser = (FeedEntityParser) EntityParserFactory.getParser(EntityType.FEED); - Feed feed = feedEntityParser.parseAndValidate(stream); + Feed feed = feedEntityParser.parse(stream); + Assert.assertNotNull(feed); Assert.assertNotNull(feed.getACL()); - Assert.assertNotNull(feed.getACL().getOwner()); - Assert.assertNotNull(feed.getACL().getGroup()); - Assert.assertNotNull(feed.getACL().getPermission()); + feed.getACL().setOwner(USER); + feed.getACL().setGroup(getPrimaryGroupName()); + + feedEntityParser.validate(feed); } finally { StartupProperties.get().setProperty("falcon.security.authorization.enabled", "false"); } @@ -702,7 +705,7 @@ public class FeedEntityParserTest extends AbstractTestBase { } } - @Test + @Test (expectedExceptions = ValidationException.class) public void testValidateACLAndStorageForValidOwnerBadGroup() throws Exception { CurrentUser.authenticate(USER); StartupProperties.get().setProperty("falcon.security.authorization.enabled", "true"); @@ -735,7 +738,7 @@ public class FeedEntityParserTest extends AbstractTestBase { } } - @Test + @Test (expectedExceptions = ValidationException.class) public void testValidateACLValidGroupBadOwner() throws Exception { CurrentUser.authenticate(USER); StartupProperties.get().setProperty("falcon.security.authorization.enabled", "true"); @@ -755,7 +758,7 @@ public class FeedEntityParserTest extends AbstractTestBase { Assert.assertNotNull(feed.getACL().getGroup()); Assert.assertNotNull(feed.getACL().getPermission()); - feed.getACL().setGroup(getGroupName()); + feed.getACL().setGroup(getPrimaryGroupName()); feedEntityParser.validate(feed); } finally { @@ -794,7 +797,7 @@ public class FeedEntityParserTest extends AbstractTestBase { } } - @Test + @Test (expectedExceptions = ValidationException.class) public void testValidateACLAndStorageForValidGroupBadOwner() throws Exception { CurrentUser.authenticate(USER); StartupProperties.get().setProperty("falcon.security.authorization.enabled", "true"); @@ -815,7 +818,7 @@ public class FeedEntityParserTest extends AbstractTestBase { Assert.assertNotNull(feed.getACL().getGroup()); Assert.assertNotNull(feed.getACL().getPermission()); - feed.getACL().setGroup(getGroupName()); + feed.getACL().setGroup(getPrimaryGroupName()); // create locations createLocations(feed); http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/b49360e7/common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java ---------------------------------------------------------------------- diff --git a/common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java b/common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java index 80a9cc7..e3a2cd5 100644 --- a/common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java +++ b/common/src/test/java/org/apache/falcon/entity/parser/ProcessEntityParserTest.java @@ -387,7 +387,7 @@ public class ProcessEntityParserTest extends AbstractTestBase { } } - @Test + @Test (expectedExceptions = ValidationException.class) public void testValidateACLAuthorizationEnabledValidOwnerBadGroup() throws Exception { StartupProperties.get().setProperty("falcon.security.authorization.enabled", "true"); Assert.assertTrue(Boolean.valueOf( @@ -432,7 +432,7 @@ public class ProcessEntityParserTest extends AbstractTestBase { Assert.assertNotNull(process.getACL().getPermission()); process.getACL().setOwner(USER); - process.getACL().setGroup(getGroupName()); + process.getACL().setGroup(getPrimaryGroupName()); processEntityParser.validate(process); } finally { http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/b49360e7/common/src/test/java/org/apache/falcon/security/DefaultAuthorizationProviderTest.java ---------------------------------------------------------------------- diff --git a/common/src/test/java/org/apache/falcon/security/DefaultAuthorizationProviderTest.java b/common/src/test/java/org/apache/falcon/security/DefaultAuthorizationProviderTest.java index 125aa85..7bb2e0e 100644 --- a/common/src/test/java/org/apache/falcon/security/DefaultAuthorizationProviderTest.java +++ b/common/src/test/java/org/apache/falcon/security/DefaultAuthorizationProviderTest.java @@ -357,8 +357,8 @@ public class DefaultAuthorizationProviderTest { Assert.fail("Bad entity"); } - @Test - public void testAuthorizeValidatePOSTOperationsGroup() throws Exception { + @Test (expectedExceptions = AuthorizationException.class) + public void testAuthorizeValidatePOSTOperationsGroupBadUser() throws Exception { StartupProperties.get().setProperty("falcon.security.authorization.enabled", "true"); StartupProperties.get().setProperty("falcon.security.authorization.admin.users", "admin"); StartupProperties.get().setProperty("falcon.security.authorization.admin.groups", "admin"); http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/b49360e7/docs/src/site/twiki/Security.twiki ---------------------------------------------------------------------- diff --git a/docs/src/site/twiki/Security.twiki b/docs/src/site/twiki/Security.twiki index f4db28b..7c4eb07 100644 --- a/docs/src/site/twiki/Security.twiki +++ b/docs/src/site/twiki/Security.twiki @@ -68,6 +68,8 @@ personal workstation, conveniently becomes that installation's super-user withou Falcon also allows users to configure a super user group and allows users belonging to this group to be a super user. +ACL owner and group must be valid even if the authenticated user is a super-user. + ---+++ Group Memberships Once a user has been authenticated and a username has been determined, the list of groups is @@ -78,6 +80,8 @@ will shell out to the Unix bash -c groups command to resolve a list of groups fo Note that Falcon stores the user and group of an Entity as strings; there is no conversion from user and group identity numbers as is conventional in Unix. +The only limitation is that a user cannot add a group in ACL that he does not belong to. + ---+++ Authorization Provider Falcon provides a plugin-able provider interface for Authorization. It also ships with a default @@ -146,6 +150,7 @@ determined by a static configuration parameter. ---++++ Lineage Resource Policy Lineage is read-only and hence all users can look at lineage for their respective entities. +*Note:* This gap will be fixed in a later release. ---++ Authentication Configuration http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/b49360e7/prism/src/main/java/org/apache/falcon/security/FalconAuthorizationFilter.java ---------------------------------------------------------------------- diff --git a/prism/src/main/java/org/apache/falcon/security/FalconAuthorizationFilter.java b/prism/src/main/java/org/apache/falcon/security/FalconAuthorizationFilter.java index 884bd73..aff2006 100644 --- a/prism/src/main/java/org/apache/falcon/security/FalconAuthorizationFilter.java +++ b/prism/src/main/java/org/apache/falcon/security/FalconAuthorizationFilter.java @@ -73,7 +73,7 @@ public class FalconAuthorizationFilter implements Filter { requestParts.getAction(), requestParts.getEntityType(), requestParts.getEntityName(), CurrentUser.getProxyUGI()); } catch (AuthorizationException e) { - throw FalconWebException.newException(e.getMessage(), Response.Status.UNAUTHORIZED); + throw FalconWebException.newException(e.getMessage(), Response.Status.FORBIDDEN); } } http://git-wip-us.apache.org/repos/asf/incubator-falcon/blob/b49360e7/prism/src/test/java/org/apache/falcon/resource/EntityManagerTest.java ---------------------------------------------------------------------- diff --git a/prism/src/test/java/org/apache/falcon/resource/EntityManagerTest.java b/prism/src/test/java/org/apache/falcon/resource/EntityManagerTest.java index f7b4f45..2244e5f 100644 --- a/prism/src/test/java/org/apache/falcon/resource/EntityManagerTest.java +++ b/prism/src/test/java/org/apache/falcon/resource/EntityManagerTest.java @@ -110,17 +110,40 @@ public class EntityManagerTest extends AbstractEntityManager { } @Test - public void testGetEntityList() throws Exception { + public void testGetEntityListBadUser() throws Exception { + CurrentUser.authenticate("fakeUser"); + try { + Entity process1 = buildProcess("processFakeUser", "fakeUser", "", ""); + configStore.publish(EntityType.PROCESS, process1); + Assert.fail(); + } catch (Throwable ignore) { + // do nothing + } - Entity process1 = buildProcess("processFakeUser", "fakeUser", "", ""); - configStore.publish(EntityType.PROCESS, process1); + /* + * Only one entity should be returned when the auth is enabled. + */ + try { + getEntityList("process", "", "", "", "", "", 0, 10); + Assert.fail(); + } catch (Throwable ignore) { + // do nothing + } + + // reset values + StartupProperties.get().setProperty("falcon.security.authorization.enabled", "false"); + CurrentUser.authenticate(System.getProperty("user.name")); + } + + @Test + public void testGetEntityList() throws Exception { Entity process2 = buildProcess("processAuthUser", System.getProperty("user.name"), "", ""); configStore.publish(EntityType.PROCESS, process2); EntityList entityList = this.getEntityList("process", "", "", "", "", "asc", 0, 10); Assert.assertNotNull(entityList.getElements()); - Assert.assertEquals(entityList.getElements().length, 2); + Assert.assertEquals(entityList.getElements().length, 1); /* * Both entities should be returned when the user is SuperUser. @@ -129,14 +152,6 @@ public class EntityManagerTest extends AbstractEntityManager { CurrentUser.authenticate(System.getProperty("user.name")); entityList = this.getEntityList("process", "", "", "", "", "desc", 0, 10); Assert.assertNotNull(entityList.getElements()); - Assert.assertEquals(entityList.getElements().length, 2); - - /* - * Only one entity should be returned when the auth is enabled. - */ - CurrentUser.authenticate("fakeUser"); - entityList = this.getEntityList("process", "", "", "", "", "", 0, 10); - Assert.assertNotNull(entityList.getElements()); Assert.assertEquals(entityList.getElements().length, 1); // reset values
