falcon-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From venkat...@apache.org
Subject [2/2] git commit: FALCON-839 Authorization succeds with invalid acl owner based on group membership. Contributed by Venkatesh Seetharam
Date Wed, 29 Oct 2014 05:22:30 GMT
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 <venkatesh@apache.org>
Authored: Tue Oct 28 15:32:07 2014 -0700
Committer: Venkatesh Seetharam <venkatesh@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


Mime
View raw message