Repository: sentry Updated Branches: refs/heads/master caa630c97 -> ca8137e89
SENTRY-1311: Improve usability of URI privileges by supporting mixed use of URIs with and without scheme (Vadim Spector, Reviewed by Hao Hao) Change-Id: I272da41e7d8b963e6d20d1b1c5c36e388d9b28fe Project: http://git-wip-us.apache.org/repos/asf/sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/sentry/commit/ca8137e8 Tree: http://git-wip-us.apache.org/repos/asf/sentry/tree/ca8137e8 Diff: http://git-wip-us.apache.org/repos/asf/sentry/diff/ca8137e8 Branch: refs/heads/master Commit: ca8137e89dc7c1354e50dff852fe7c76e4f426b9 Parents: caa630c Author: hahao <[email protected]> Authored: Thu Jun 23 12:00:04 2016 -0700 Committer: hahao <[email protected]> Committed: Thu Jun 23 12:00:04 2016 -0700 ---------------------------------------------------------------------- .../sentry/core/common/utils/PathUtils.java | 94 +++++++----- .../sentry/core/common/utils/TestPathUtils.java | 42 +++++- .../tests/e2e/hdfs/TestHDFSIntegration.java | 150 +++++++++++++++++++ 3 files changed, 248 insertions(+), 38 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/sentry/blob/ca8137e8/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/ca8137e8/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/ca8137e8/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 898552e..b1d507f 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; @@ -1416,6 +1417,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 {
