Repository: sentry Updated Branches: refs/heads/master 4c49a3fb0 -> e94c5d6d0
SENTRY-2214: Sentry should not allow URI grants to EMPTY or NULL locations (Arjun Mishra, reviewed by Na Li, Sergio Pena, Benjamin Iglauer) Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/e94c5d6d Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/e94c5d6d Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/e94c5d6d Branch: refs/heads/master Commit: e94c5d6d0fafb821211eecb74575df32a5388576 Parents: 4c49a3f Author: lina.li <[email protected]> Authored: Tue May 8 18:01:40 2018 -0500 Committer: lina.li <[email protected]> Committed: Tue May 8 18:01:40 2018 -0500 ---------------------------------------------------------------------- .../authz/DefaultSentryAccessController.java | 4 + .../org/apache/sentry/hdfs/SentryPlugin.java | 3 +- .../db/service/persistent/SentryStore.java | 90 +++++++++++--------- .../db/service/persistent/TestSentryStore.java | 25 ++++++ .../e2e/dbprovider/TestDatabaseProvider.java | 30 +++++++ 5 files changed, 111 insertions(+), 41 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/e94c5d6d/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java ---------------------------------------------------------------------- diff --git a/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java b/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java index 3ac49fa..2abe37e 100644 --- a/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java +++ b/sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/authz/DefaultSentryAccessController.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.HashSet; import java.util.List; import java.util.Set; +import org.apache.commons.lang3.StringUtils; import org.apache.hadoop.hive.SentryHiveConstants; import org.apache.hadoop.hive.conf.HiveConf; import org.apache.hadoop.hive.conf.HiveConf.ConfVars; @@ -416,6 +417,9 @@ public class DefaultSentryAccessController extends SentryHiveAccessController { break; case LOCAL_URI: case DFS_URI: + if(StringUtils.isBlank(hivePrivObject.getObjectName())) { + throw new HiveAuthzPluginException("Null or Empty locations not supported by " + hivePrivObject.getType().name()); + } String uRIString = hivePrivObject.getObjectName().replace("'", "").replace("\"", ""); if (isGrant) { sentryClient.grantURIPrivilege(grantorName, roleName, serverName, http://git-wip-us.apache.org/repos/asf/sentry/blob/e94c5d6d/sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java ---------------------------------------------------------------------- diff --git a/sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java b/sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java index 50853c9..fa83589 100644 --- a/sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java +++ b/sentry-hdfs/sentry-hdfs-service/src/main/java/org/apache/sentry/hdfs/SentryPlugin.java @@ -45,6 +45,7 @@ import org.apache.sentry.provider.db.service.thrift.TSentryGroup; import org.apache.sentry.provider.db.service.thrift.TSentryPrivilege; import org.apache.sentry.provider.db.service.persistent.HMSFollower; import com.google.common.base.Preconditions; +import org.apache.sentry.service.thrift.ServiceConstants.PrivilegeScope; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -260,7 +261,7 @@ public class SentryPlugin implements SentryPolicyStorePlugin, SigUtils.SigListen String roleName = request.getRoleName(); for (TSentryPrivilege privilege : request.getPrivileges()) { - if(!("COLUMN".equalsIgnoreCase(privilege.getPrivilegeScope()))) { + if(!(PrivilegeScope.COLUMN.name().equalsIgnoreCase(privilege.getPrivilegeScope()))) { PermissionsUpdate update = onAlterSentryRoleGrantPrivilegeCore(roleName, privilege); if (update != null && privilegesUpdateMap != null) { privilegesUpdateMap.put(privilege, update); http://git-wip-us.apache.org/repos/asf/sentry/blob/e94c5d6d/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java index ac5316c..4581170 100644 --- a/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java +++ b/sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java @@ -756,50 +756,55 @@ public class SentryStore { MSentryRole mRole = getRole(pm, roleName); if (mRole == null) { throw noSuchRole(roleName); - } else { - if (!isNULL(privilege.getColumnName()) || !isNULL(privilege.getTableName()) - || !isNULL(privilege.getDbName())) { - // If Grant is for ALL and Either INSERT/SELECT already exists.. - // need to remove it and GRANT ALL.. - if (AccessConstants.ALL.equalsIgnoreCase(privilege.getAction()) - || AccessConstants.ACTION_ALL.equalsIgnoreCase(privilege.getAction())) { - TSentryPrivilege tNotAll = new TSentryPrivilege(privilege); - tNotAll.setAction(AccessConstants.SELECT); - MSentryPrivilege mSelect = getMSentryPrivilege(tNotAll, pm); - tNotAll.setAction(AccessConstants.INSERT); - MSentryPrivilege mInsert = getMSentryPrivilege(tNotAll, pm); - if ((mSelect != null) && mRole.getPrivileges().contains(mSelect)) { - mSelect.removeRole(mRole); - pm.makePersistent(mSelect); - } - if ((mInsert != null) && mRole.getPrivileges().contains(mInsert)) { - mInsert.removeRole(mRole); - pm.makePersistent(mInsert); - } - } else { - // If Grant is for Either INSERT/SELECT and ALL already exists.. - // do nothing.. - TSentryPrivilege tAll = new TSentryPrivilege(privilege); - tAll.setAction(AccessConstants.ALL); - MSentryPrivilege mAll1 = getMSentryPrivilege(tAll, pm); - tAll.setAction(AccessConstants.ACTION_ALL); - MSentryPrivilege mAll2 = getMSentryPrivilege(tAll, pm); - if (mAll1 != null && mRole.getPrivileges().contains(mAll1)) { - return null; - } - if (mAll2 != null && mRole.getPrivileges().contains(mAll2)) { - return null; - } + } + + if(privilege.getPrivilegeScope().equalsIgnoreCase(PrivilegeScope.URI.name()) + && StringUtils.isBlank(privilege.getURI())) { + throw new SentryInvalidInputException("cannot grant URI privileges to Null or EMPTY location"); + } + + if (!isNULL(privilege.getColumnName()) || !isNULL(privilege.getTableName()) + || !isNULL(privilege.getDbName())) { + // If Grant is for ALL and Either INSERT/SELECT already exists.. + // need to remove it and GRANT ALL.. + if (AccessConstants.ALL.equalsIgnoreCase(privilege.getAction()) + || AccessConstants.ACTION_ALL.equalsIgnoreCase(privilege.getAction())) { + TSentryPrivilege tNotAll = new TSentryPrivilege(privilege); + tNotAll.setAction(AccessConstants.SELECT); + MSentryPrivilege mSelect = getMSentryPrivilege(tNotAll, pm); + tNotAll.setAction(AccessConstants.INSERT); + MSentryPrivilege mInsert = getMSentryPrivilege(tNotAll, pm); + if ((mSelect != null) && mRole.getPrivileges().contains(mSelect)) { + mSelect.removeRole(mRole); + pm.makePersistent(mSelect); + } + if ((mInsert != null) && mRole.getPrivileges().contains(mInsert)) { + mInsert.removeRole(mRole); + pm.makePersistent(mInsert); + } + } else { + // If Grant is for Either INSERT/SELECT and ALL already exists.. + // do nothing.. + TSentryPrivilege tAll = new TSentryPrivilege(privilege); + tAll.setAction(AccessConstants.ALL); + MSentryPrivilege mAll1 = getMSentryPrivilege(tAll, pm); + tAll.setAction(AccessConstants.ACTION_ALL); + MSentryPrivilege mAll2 = getMSentryPrivilege(tAll, pm); + if (mAll1 != null && mRole.getPrivileges().contains(mAll1)) { + return null; + } + if (mAll2 != null && mRole.getPrivileges().contains(mAll2)) { + return null; } } + } - mPrivilege = getMSentryPrivilege(privilege, pm); - if (mPrivilege == null) { - mPrivilege = convertToMSentryPrivilege(privilege); - } - mPrivilege.appendRole(mRole); - pm.makePersistent(mPrivilege); + mPrivilege = getMSentryPrivilege(privilege, pm); + if (mPrivilege == null) { + mPrivilege = convertToMSentryPrivilege(privilege); } + mPrivilege.appendRole(mRole); + pm.makePersistent(mPrivilege); return mPrivilege; } @@ -906,6 +911,11 @@ public class SentryStore { if (mRole == null) { throw noSuchRole(roleName); } + if(tPrivilege.getPrivilegeScope().equalsIgnoreCase(PrivilegeScope.URI.name()) + && StringUtils.isBlank(tPrivilege.getURI())) { + throw new SentryInvalidInputException("cannot revoke URI privileges from Null or EMPTY location"); + } + MSentryPrivilege mPrivilege = getMSentryPrivilege(tPrivilege, pm); if (mPrivilege == null) { mPrivilege = convertToMSentryPrivilege(tPrivilege); http://git-wip-us.apache.org/repos/asf/sentry/blob/e94c5d6d/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java ---------------------------------------------------------------------- diff --git a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java index f5a777d..679a097 100644 --- a/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java +++ b/sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/persistent/TestSentryStore.java @@ -44,6 +44,7 @@ import org.apache.hadoop.security.alias.CredentialProvider; import org.apache.hadoop.security.alias.CredentialProviderFactory; import org.apache.hadoop.security.alias.UserProvider; import org.apache.sentry.core.common.exception.SentryAccessDeniedException; +import org.apache.sentry.core.common.exception.SentryInvalidInputException; import org.apache.sentry.core.model.db.AccessConstants; import org.apache.sentry.core.common.exception.SentryAlreadyExistsException; import org.apache.sentry.core.common.exception.SentryGrantDeniedException; @@ -289,6 +290,30 @@ public class TestSentryStore extends org.junit.Assert { } @Test + public void testURIGrantRevokeOnEmptyPath() throws Exception { + String roleName = "test-empty-uri-role"; + String grantor = "g1"; + String uri = ""; + createRole(roleName); + TSentryPrivilege tSentryPrivilege = new TSentryPrivilege("URI", "server1", "ALL"); + tSentryPrivilege.setURI(uri); + //Test grant on empty URI + try { + sentryStore.alterSentryRoleGrantPrivilege(grantor, roleName, tSentryPrivilege); + fail("Expected SentryInvalidInputException"); + } catch(SentryInvalidInputException e) { + // expected + } + //Test revoke on empty URI + try { + sentryStore.alterSentryRoleRevokePrivilege(grantor, roleName, tSentryPrivilege); + fail("Expected SentryInvalidInputException"); + } catch(SentryInvalidInputException e) { + // expected + } + } + + @Test public void testCreateDuplicateRole() throws Exception { String roleName = "test-dup-role"; createRole(roleName); http://git-wip-us.apache.org/repos/asf/sentry/blob/e94c5d6d/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java ---------------------------------------------------------------------- diff --git a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java index ae13164..3e31852 100644 --- a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java +++ b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDatabaseProvider.java @@ -2221,6 +2221,36 @@ public class TestDatabaseProvider extends AbstractTestWithStaticConfiguration { } @Test + public void testGrantRevokeOnEmptyURI() throws Exception{ + + Connection connection = context.createConnection(ADMIN1); + Statement statement = context.createStatement(connection); + statement.execute("CREATE ROLE role1"); + ResultSet resultSet = statement.executeQuery("SHOW GRANT ROLE role1"); + assertResultSize(resultSet, 0); + statement.execute("GRANT ROLE role1 to GROUP " + USERGROUP1); + + //Test Grant to EMPTY URI + try { + statement.execute("GRANT ALL ON URI \"\" TO ROLE role1"); + assertTrue("Grant ALL on Empty URI SHOULD NOT BE ALLOWED!!", false); + } catch (SQLException e){} + resultSet = statement.executeQuery("SHOW GRANT ROLE role1"); + assertResultSize(resultSet, 0); + + //Test Revoke to EMPTY URI + try { + statement.execute("REVOKE ALL ON URI \"\" TO ROLE role1"); + assertTrue("Revoke ALL on Empty URI SHOULD NOT BE ALLOWED!!", false); + } catch (SQLException e){} + resultSet = statement.executeQuery("SHOW GRANT ROLE role1"); + assertResultSize(resultSet, 0); + + statement.close(); + connection.close(); + } + + @Test public void testShowGrantOnALL() throws Exception { // setup db objects needed by the test
