You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@falcon.apache.org by ve...@apache.org on 2014/10/29 06:22:30 UTC
[2/2] git commit: FALCON-839 Authorization succeds with invalid acl
owner based on group membership. Contributed by Venkatesh Seetharam
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 <ve...@apache.org>
Authored: Tue Oct 28 15:32:07 2014 -0700
Committer: Venkatesh Seetharam <ve...@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