sentry-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ak...@apache.org
Subject sentry git commit: SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme (Vadim Spector, Reviewed by Hao Hao)
Date Sat, 11 Mar 2017 01:46:04 GMT
Repository: sentry
Updated Branches:
  refs/heads/sentry-ha-redesign 5b1a89ea3 -> 94ec0b44c


SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and
without scheme (Vadim Spector, Reviewed by Hao Hao)


Project: http://git-wip-us.apache.org/repos/asf/sentry/repo
Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/94ec0b44
Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/94ec0b44
Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/94ec0b44

Branch: refs/heads/sentry-ha-redesign
Commit: 94ec0b44c40b7a72c3c7292df47047aa390feab7
Parents: 5b1a89e
Author: Alexander Kolbasov <akolb@cloudera.com>
Authored: Fri Mar 10 17:45:52 2017 -0800
Committer: Alexander Kolbasov <akolb@cloudera.com>
Committed: Fri Mar 10 17:45:52 2017 -0800

----------------------------------------------------------------------
 sentry-binding/pom.xml                          |   4 +-
 .../sentry/core/common/utils/PathUtils.java     |  94 +++++++-----
 .../sentry/core/common/utils/TestPathUtils.java |  42 +++++-
 .../tests/e2e/hdfs/TestHDFSIntegration.java     | 150 +++++++++++++++++++
 4 files changed, 250 insertions(+), 40 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/sentry/blob/94ec0b44/sentry-binding/pom.xml
----------------------------------------------------------------------
diff --git a/sentry-binding/pom.xml b/sentry-binding/pom.xml
index 48ae245..67d8395 100644
--- a/sentry-binding/pom.xml
+++ b/sentry-binding/pom.xml
@@ -43,7 +43,7 @@ limitations under the License.
     <profile>
       <id>hive-authz1</id>
       <activation>
-        <activeByDefault>true</activeByDefault>
+        <activeByDefault>false</activeByDefault>
       </activation>
       <modules>
         <module>sentry-binding-hive</module>
@@ -52,7 +52,7 @@ limitations under the License.
     <profile>
       <id>hive-authz2</id>
       <activation>
-        <activeByDefault>false</activeByDefault>
+        <activeByDefault>true</activeByDefault>
       </activation>
       <modules>
         <module>sentry-binding-hive-v2</module>

http://git-wip-us.apache.org/repos/asf/sentry/blob/94ec0b44/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
----------------------------------------------------------------------
diff --git a/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
b/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
index 3b9336c..40c9595 100644
--- a/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
+++ b/sentry-core/sentry-core-common/src/main/java/org/apache/sentry/core/common/utils/PathUtils.java
@@ -47,21 +47,55 @@ public class PathUtils {
     return CONF;
   }
 
+  // TODO: "throws URISyntaxException" is kept for backward compatibility with the existing
client code
+  public static boolean impliesURI(URI privilegeURI, URI requestURI) throws URISyntaxException
{
+    return _impliesURI(privilegeURI.toString(), requestURI.toString());
+  }
+
   /**
    * URI is a a special case. For URI's, /a implies /a/b.
    * Therefore the test is "/a/b".startsWith("/a");
    */
-  public static boolean impliesURI(URI privilegeURI, URI requestURI) throws URISyntaxException
{
-    if (privilegeURI.getPath() == null || requestURI.getPath() == null) {
+  private static boolean _impliesURI(String privilege, String request) {
+
+    URI privilegeURI;
+    URI requestURI;
+    try {
+      // build privilege URI, add default scheme and/or authority if missing
+      privilegeURI = makeFullQualifiedURI(privilege);
+      if (privilegeURI == null) {
+        LOGGER.warn("Privilege URI " + privilege + " is not valid. Path is not absolute.");
+        return false;
+      }
+
+      // build request URI, add default scheme and/or authority if missing
+      requestURI = makeFullQualifiedURI(request);
+      if (requestURI == null) {
+        LOGGER.warn("Request URI " + request + " is not valid. Path is not absolute.");
+        return false;
+      }
+    } catch (IOException e) {
+      LOGGER.warn("Unable to get the configured filesystem implementation", e);
       return false;
     }
-    // ensure that either both schemes are null or equal
-    if (privilegeURI.getScheme() == null && requestURI.getScheme() != null) {
+
+    // scheme and path must be present in privilege URI
+    if (privilegeURI.getScheme() == null || privilegeURI.getPath() == null) {
+      LOGGER.warn("Privilege URI " + request + " is not valid. Missing scheme or path.");
+      return false;
+    }
+
+    // scheme and path must be present in request URI
+    if (requestURI.getScheme() == null || requestURI.getPath() == null) {
+      LOGGER.warn("Request URI " + request + " is not valid. Missing scheme or path.");
       return false;
     }
+
+    // schemes in privilege and request URIs must be equal
     if (privilegeURI.getScheme() != null && !privilegeURI.getScheme().equals(requestURI.getScheme()))
{
       return false;
     }
+
     // request path does not contain relative parts /a/../b &&
     // request path starts with privilege path &&
     // authorities (nullable) are equal
@@ -71,52 +105,44 @@ public class PathUtils {
         && requestPath.startsWith(privilegePath)
         && Strings.nullToEmpty(privilegeURI.getAuthority()).equals(
             Strings.nullToEmpty(requestURI.getAuthority()))) {
-      return true;
+          return true;
     }
     return false;
   }
 
   /**
-   * Make fully qualified URI based on the default file system Scheme and Authority
+   * Make fully qualified URI if Scheme and/or Authority is missing,
+   * based on the default file system Scheme and Authority.
+   * Notes:
+   * a) input URI path must be absolute; otherwise return null.
+   * b) Path.makeQualified() provides no assurance that the
+   *    default file system Scheme and Authority values are not null.
    *
    * @param uriName The Uri name.
-   * @return Returns the fully qualified URI.
+   * @return Returns the fully qualified URI or null if URI path is not absolute.
    * @throws IOException
-   * @throws URISyntaxException
    */
-  private static URI makeFullQualifiedURI(String uriName) throws IOException, URISyntaxException
{
+  private static URI makeFullQualifiedURI(String uriName) throws IOException {
     Path uriPath = new Path(uriName);
-
-    if (uriPath.isAbsoluteAndSchemeAuthorityNull()) {
-
-      URI defaultUri = FileSystem.getDefaultUri(CONF);
-      uriPath = uriPath.makeQualified(defaultUri, uriPath);
+    if (isNormalized(uriName) && uriPath.isUriPathAbsolute()) {
+      // add scheme and/or authority if either is missing
+      if ((uriPath.toUri().getScheme() == null || uriPath.toUri().getAuthority() == null))
{
+        URI defaultUri = FileSystem.getDefaultUri(CONF);
+        uriPath = uriPath.makeQualified(defaultUri, uriPath);
+      }
       return uriPath.toUri();
+    } else { // relative URI path is unacceptable
+      return null;
     }
+  }
 
-    return new URI(uriName);
+  private static boolean isNormalized(String uriName) {
+      URI uri = URI.create(uriName);
+      return uri.getPath().equals(uri.normalize().getPath());
   }
 
   public static boolean impliesURI(String privilege, String request) {
-    try {
-      URI privilegeURI = makeFullQualifiedURI(new StrSubstitutor(System.getProperties()).replace(privilege));
-      URI requestURI = makeFullQualifiedURI(request);
-      if (privilegeURI.getScheme() == null || privilegeURI.getPath() == null) {
-        LOGGER.warn("Privilege URI " + request + " is not valid. Either no scheme or no path.");
-        return false;
-      }
-      if (requestURI.getScheme() == null || requestURI.getPath() == null) {
-        LOGGER.warn("Request URI " + request + " is not valid. Either no scheme or no path.");
-        return false;
-      }
-      return PathUtils.impliesURI(privilegeURI, requestURI);
-    } catch (URISyntaxException e) {
-      LOGGER.warn("Request URI " + request + " is not a URI", e);
-      return false;
-    } catch (IOException e) {
-      LOGGER.warn("Unable to get the configured filesystem implementation", e);
-      return false;
-    }
+    return _impliesURI(new StrSubstitutor(System.getProperties()).replace(privilege), request);
   }
 
   /**

http://git-wip-us.apache.org/repos/asf/sentry/blob/94ec0b44/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java
----------------------------------------------------------------------
diff --git a/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java
b/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java
index d7d5e0a..8419b9d 100644
--- a/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java
+++ b/sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/utils/TestPathUtils.java
@@ -29,11 +29,45 @@ public class TestPathUtils {
 
   @Test
   public void testNullScheme() throws Exception {
-    assertTrue(PathUtils.impliesURI(new URI("/tmp"), new URI("/tmp/a")));
-    assertFalse(PathUtils.impliesURI(new URI("file:/tmp"), new URI("/tmp/a")));
-    assertFalse(PathUtils.impliesURI(new URI("/tmp"), new URI("file:/tmp/a")));
+    testImplies(true, "/tmp", "/tmp/a");
+
+    // default scheme "file"
+    testImplies(true, "file:/tmp", "/tmp/a");
+    // default scheme "file"
+    testImplies(true, "/tmp", "file:/tmp/a");
+
+    // default scheme "file" but default authority not "testauth"
+    testImplies(false, "file://testauth/tmp", "/tmp/a");
+    // default scheme "file" but default authority not "test
+    testImplies(false, "/tmp", "file://testauth/tmp/a");
+
+    // default scheme not "https"
+    testImplies(false, "https:/tmp", "/tmp/a");
+    // default scheme not "https"
+    testImplies(false, "/tmp", "https:/tmp/a");
+
     // Privileges on /tmp/ are distinct from /tmp.+/ e.g. /tmp/ and /tmpdata/
-    assertFalse(PathUtils.impliesURI(new URI("/tmp"), new URI("/tmpdata")));
+    testImplies(false, "/tmp", "/tmpdata");
+  }
+
+  @Test
+  public void testPath() throws Exception {
+    // ".." is unacceptable in both privilege and request URIs
+    testImplies(false, "file://testauth/tmp", "file://testauth/tmp/x/../x");
+    testImplies(false, "file://testauth/tmp/x", "file://testauth/tmp/x/y/../y");
+    testImplies(false, "file://testauth/tmp/x", "file://testauth/tmp/x/y/..");
+    testImplies(false, "file://testauth/tmp/x/..", "file://testauth/tmp/x");
+    testImplies(false, "file://testauth/tmp/x/y/../..", "file://testauth/tmp/x/y");
+  }
+
+  private void testImplies(boolean implies, String privilege, String request) throws Exception
{
+    if (implies) {
+      assertTrue(PathUtils.impliesURI(new URI(privilege), new URI(request)));
+      assertTrue(PathUtils.impliesURI(privilege, request));
+    } else {
+      assertFalse(PathUtils.impliesURI(new URI(privilege), new URI(request)));
+      assertFalse(PathUtils.impliesURI(privilege, request));
+    }
   }
 
   @Test

http://git-wip-us.apache.org/repos/asf/sentry/blob/94ec0b44/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
----------------------------------------------------------------------
diff --git a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
index f2d74bf..c3bb118 100644
--- a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
+++ b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java
@@ -23,6 +23,7 @@ import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.OutputStream;
 import java.net.ServerSocket;
+import java.net.URI;
 import java.net.URL;
 import java.security.PrivilegedExceptionAction;
 import java.sql.Connection;
@@ -1387,6 +1388,155 @@ public class TestHDFSIntegration {
     conn.close();
   }
 
+  /**
+   * Test combination of "grant all on URI" where URI has scheme,
+   * followed by "create external table" where location URI has no scheme.
+   * Neither URI has authority.
+   */
+  @Test
+  public void testURIsWithAndWithoutSchemeNoAuthority() throws Throwable {
+    // In the local test environment, EXTERNAL_SENTRY_SERVICE is false,
+    // set the default URI scheme to be hdfs.
+    boolean testConfOff = new Boolean(System.getProperty(EXTERNAL_SENTRY_SERVICE, "false"));
+    if (!testConfOff) {
+      PathUtils.getConfiguration().set("fs.defaultFS", fsURI);
+    }
+
+    String dbName= "db1";
+
+    tmpHDFSDir = new Path("/tmp/external");
+    dbNames = new String[]{dbName};
+    roles = new String[]{"admin_role", "db_role"};
+    admin = StaticUserGroup.ADMIN1;
+
+    Connection conn;
+    Statement stmt;
+
+    conn = hiveServer2.createConnection("hive", "hive");
+    stmt = conn.createStatement();
+
+    stmt.execute("create role admin_role");
+    stmt.execute("grant all on server server1 to role admin_role");
+    stmt.execute("grant role admin_role to group " + StaticUserGroup.ADMINGROUP);
+
+    conn = hiveServer2.createConnection(StaticUserGroup.ADMIN1, StaticUserGroup.ADMIN1);
+    stmt = conn.createStatement();
+
+    stmt.execute("create database " + dbName);
+    stmt.execute("create role db_role");
+    stmt.execute("grant all on database " + dbName +" to role db_role");
+    stmt.execute("grant all on URI 'hdfs:///tmp/external' to role db_role");
+    stmt.execute("grant role db_role to group " + StaticUserGroup.USERGROUP1);
+
+    conn = hiveServer2.createConnection(StaticUserGroup.USER1_1, StaticUserGroup.USER1_1);
+    stmt = conn.createStatement();
+
+    stmt.execute("use " + dbName);
+    stmt.execute("create external table tab1 (s string) location '/tmp/external'");
+
+    stmt.close();
+    conn.close();
+  }
+
+  /**
+   * Test combination of "grant all on URI" where URI has no scheme,
+   * followed by "create external table" where location URI has scheme.
+   * Neither URI has authority.
+   */
+  @Test
+  public void testURIsWithoutAndWithSchemeNoAuthority() throws Throwable {
+    // In the local test environment, EXTERNAL_SENTRY_SERVICE is false,
+    // set the default URI scheme to be hdfs.
+    boolean testConfOff = new Boolean(System.getProperty(EXTERNAL_SENTRY_SERVICE, "false"));
+    if (!testConfOff) {
+      PathUtils.getConfiguration().set("fs.defaultFS", fsURI);
+    }
+
+    String dbName= "db1";
+
+    tmpHDFSDir = new Path("/tmp/external");
+    dbNames = new String[]{dbName};
+    roles = new String[]{"admin_role", "db_role"};
+    admin = StaticUserGroup.ADMIN1;
+
+    Connection conn;
+    Statement stmt;
+
+    conn = hiveServer2.createConnection("hive", "hive");
+    stmt = conn.createStatement();
+
+    stmt.execute("create role admin_role");
+    stmt.execute("grant all on server server1 to role admin_role");
+    stmt.execute("grant role admin_role to group " + StaticUserGroup.ADMINGROUP);
+
+    conn = hiveServer2.createConnection(StaticUserGroup.ADMIN1, StaticUserGroup.ADMIN1);
+    stmt = conn.createStatement();
+
+    stmt.execute("create database " + dbName);
+    stmt.execute("create role db_role");
+    stmt.execute("grant all on database " + dbName +" to role db_role");
+    stmt.execute("grant all on URI '/tmp/external' to role db_role");
+    stmt.execute("grant role db_role to group " + StaticUserGroup.USERGROUP1);
+
+    conn = hiveServer2.createConnection(StaticUserGroup.USER1_1, StaticUserGroup.USER1_1);
+    stmt = conn.createStatement();
+
+    stmt.execute("use " + dbName);
+    stmt.execute("create external table tab1 (s string) location 'hdfs:///tmp/external'");
+
+    stmt.close();
+    conn.close();
+  }
+
+  /**
+   * Test combination of "grant all on URI" where URI has scheme and authority,
+   * followed by "create external table" where location URI has neither scheme nor authority.
+   */
+  @Test
+  public void testURIsWithAndWithoutSchemeAndAuthority() throws Throwable {
+    // In the local test environment, EXTERNAL_SENTRY_SERVICE is false,
+    // set the default URI scheme to be hdfs.
+    boolean testConfOff = new Boolean(System.getProperty(EXTERNAL_SENTRY_SERVICE, "false"));
+    if (!testConfOff) {
+      PathUtils.getConfiguration().set("fs.defaultFS", fsURI);
+    }
+
+    String dbName= "db1";
+
+    tmpHDFSDir = new Path("/tmp/external");
+    dbNames = new String[]{dbName};
+    roles = new String[]{"admin_role", "db_role"};
+    admin = StaticUserGroup.ADMIN1;
+
+    Connection conn;
+    Statement stmt;
+
+    conn = hiveServer2.createConnection("hive", "hive");
+    stmt = conn.createStatement();
+
+    stmt.execute("create role admin_role");
+    stmt.execute("grant all on server server1 to role admin_role");
+    stmt.execute("grant role admin_role to group " + StaticUserGroup.ADMINGROUP);
+
+    conn = hiveServer2.createConnection(StaticUserGroup.ADMIN1, StaticUserGroup.ADMIN1);
+    stmt = conn.createStatement();
+
+    stmt.execute("create database " + dbName);
+    stmt.execute("create role db_role");
+    stmt.execute("grant all on database " + dbName +" to role db_role");
+    stmt.execute("grant all on URI 'hdfs://" + new URI(fsURI).getAuthority() + "/tmp/external'
to role db_role");
+    stmt.execute("grant role db_role to group " + StaticUserGroup.USERGROUP1);
+
+    conn = hiveServer2.createConnection(StaticUserGroup.USER1_1, StaticUserGroup.USER1_1);
+    stmt = conn.createStatement();
+
+    stmt.execute("use " + dbName);
+    stmt.execute("create external table tab1 (s string) location '/tmp/external'");
+
+    stmt.close();
+    conn.close();
+  }
+
   //SENTRY-884
   @Test
   public void testAccessToTableDirectory() throws Throwable {


Mime
View raw message