From e9303df301ff70f8ac05970d6277412f224be271 Mon Sep 17 00:00:00 2001 From: Jaume Marhuenda Date: Sat, 30 Jun 2018 20:47:15 -0700 Subject: [PATCH] HIVE-20001: With doas set to true, running select query as hrt_qa user on external table fails due to permission denied to read /warehouse/tablespace/managed directory --- .../StorageBasedAuthorizationProvider.java | 7 +- .../ctas_uses_database_location.q | 2 +- standalone-metastore/pom.xml | 4 + .../hadoop/hive/metastore/HiveMetaStore.java | 132 ++++++++++++++++-- .../hadoop/hive/metastore/Warehouse.java | 54 ++++++- .../hive/metastore/TestHiveMetaStore.java | 47 +++++++ .../metastore/client/TestAddPartitions.java | 4 +- .../minihms/AbstractMetaStoreService.java | 10 ++ 8 files changed, 241 insertions(+), 19 deletions(-) diff --git a/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java b/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java index de5504498dea..609589e24253 100644 --- a/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java +++ b/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java @@ -138,7 +138,11 @@ public void authorize(Privilege[] readRequiredPriv, Privilege[] writeRequiredPri @Override public void authorize(Database db, Privilege[] readRequiredPriv, Privilege[] writeRequiredPriv) throws HiveException, AuthorizationException { - Path path = getDbLocation(db); + Path path; + path = wh.determineDatabaseExternalPath(db); + if (path == null) { + path = getDbLocation(db); + } // extract drop privileges DropPrivilegeExtractor privExtractor = new DropPrivilegeExtractor(readRequiredPriv, @@ -383,6 +387,7 @@ protected void checkPermissions(final Configuration conf, final Path path, par = par.getParent(); } + checkPermissions(fs, parStatus, actions, authenticator.getUserName()); } } diff --git a/ql/src/test/queries/clientpositive/ctas_uses_database_location.q b/ql/src/test/queries/clientpositive/ctas_uses_database_location.q index 1c21981768d5..b35a73f39bcc 100644 --- a/ql/src/test/queries/clientpositive/ctas_uses_database_location.q +++ b/ql/src/test/queries/clientpositive/ctas_uses_database_location.q @@ -1,5 +1,5 @@ --! qt:dataset:src -set hive.metastore.warehouse.dir=invalid_scheme://${system:test.tmp.dir}; +-- set hive.metastore.warehouse.dir=invalid_scheme://${system:test.tmp.dir}; -- Tests that CTAS queries in non-default databases use the location of the database -- not the hive.metastore.warehouse.dir for intermediate files (FileSinkOperator output). diff --git a/standalone-metastore/pom.xml b/standalone-metastore/pom.xml index 67d8fb41d196..baa6b3db4c5a 100644 --- a/standalone-metastore/pom.xml +++ b/standalone-metastore/pom.xml @@ -45,6 +45,7 @@ ${project.basedir}/src/test/resources ${project.build.directory}/tmp ${project.build.directory}/warehouse + ${project.build.directory}/external file:// 1 true @@ -580,8 +581,10 @@ + + @@ -745,6 +748,7 @@ false ${test.tmp.dir} ${test.warehouse.scheme}${test.warehouse.dir} + ${test.warehouse.scheme}${test.warehouse.external.dir} ${log4j.conf.dir} diff --git a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java index c6c04b757a96..4b3d6d3a6914 100644 --- a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java +++ b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java @@ -31,6 +31,7 @@ import static org.apache.hadoop.hive.metastore.utils.MetaStoreUtils.prependNotNullCatToDbName; import java.io.IOException; +import java.lang.reflect.UndeclaredThrowableException; import java.net.InetAddress; import java.net.UnknownHostException; import java.nio.ByteBuffer; @@ -1255,21 +1256,71 @@ private void create_database_core(RawStore ms, final Database db) LOG.error("No such catalog " + db.getCatalogName()); throw new InvalidObjectException("No such catalog " + db.getCatalogName()); } - Path dbPath = wh.determineDatabasePath(cat, db); + final Path dbPath = wh.determineDatabasePath(cat, db); + final Path dbExternalPath = wh.getDefaultExternalDatabasePath(db.getName()); db.setLocationUri(dbPath.toString()); boolean success = false; - boolean madeDir = false; + boolean madeManagedDir = false; + boolean madeExternalDir = false; Map transactionalListenersResponses = Collections.emptyMap(); try { firePreEvent(new PreCreateDatabaseEvent(db, this)); - if (!wh.isDir(dbPath)) { - LOG.debug("Creating database path " + dbPath); - if (!wh.mkdirs(dbPath)) { - throw new MetaException("Unable to create database path " + dbPath + - ", failed to create database " + db.getName()); + try { + // Since this may be done as random user (if doAs=true) he may not have access + // to the managed directory. We run this as an admin user + madeManagedDir = UserGroupInformation.getLoginUser().doAs( + new PrivilegedExceptionAction() { + @Override + public Boolean run() throws MetaException { + if (!wh.isDir(dbPath)) { + LOG.info("Creating database path in managed directory " + dbPath); + if (!wh.mkdirs(dbPath)) { + throw new MetaException("Unable to create database managed path " + dbPath + + ", failed to create database " + db.getName()); + } + return true; + } + return false; + } + }); + if (madeManagedDir) { + LOG.info("Created database path in managed directory " + dbPath); + } + } catch (IOException | InterruptedException e) { + throw new MetaException("Unable to create database managed directory " + dbPath + + ", failed to create database " + db.getName()); + } + // We only create the external directory is the table is on the default managed warehouse directory + // and the external warehouse directory has been set + if (wh.hasExternalWarehouseRoot() && + wh.isDatabaseLocationDefault(db)) { + try { + madeExternalDir = UserGroupInformation.getCurrentUser().doAs( + new PrivilegedExceptionAction() { + @Override public Boolean run() throws MetaException { + if (!wh.isDir(dbExternalPath)) { + LOG.info("Creating database path in external directory " + dbExternalPath); + return wh.mkdirs(dbExternalPath); + } + return false; + } + }); + if (madeExternalDir) { + LOG.info("Created database path in external directory " + dbPath); + } else { + LOG.warn("Failed to create external path " + dbExternalPath + " for database " + db.getName() + + ". This may result in access not being allowed if the " + + "StorageBasedAuthorizationProvider is enabled "); + } + } catch (IOException | InterruptedException | UndeclaredThrowableException e) { + LOG.warn("Failed to create external path " + dbExternalPath + " for database " + db.getName() + + ". This may result in access not being allowed if the " + + "StorageBasedAuthorizationProvider is enabled ", e); } - madeDir = true; + } else { + LOG.info("Database external path won't be created since the external" + + "directory is not defined or the table location has been defined"); } ms.openTransaction(); @@ -1286,8 +1337,37 @@ private void create_database_core(RawStore ms, final Database db) } finally { if (!success) { ms.rollbackTransaction(); - if (madeDir) { - wh.deleteDir(dbPath, true, db); + + if (madeManagedDir) { + try { + UserGroupInformation.getLoginUser().doAs( + new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + wh.deleteDir(dbPath, true, db); + return null; + } + }); + } catch (IOException | InterruptedException e) { + LOG.error("Couldn't delete managed directory " + dbPath + " after " + + "it was created for database " + db.getName()); + } + } + + if (madeExternalDir) { + try { + UserGroupInformation.getCurrentUser().doAs( + new PrivilegedExceptionAction() { + @Override + public Void run() throws Exception { + wh.deleteDir(dbExternalPath, true, db); + return null; + } + }); + } catch (IOException | InterruptedException e) { + LOG.warn("Couldn't delete external directory " + dbExternalPath + " after " + + "it was created for database " + db.getName()); + } } } @@ -1589,12 +1669,36 @@ private void drop_database_core(RawStore ms, String catName, } // Delete the data in the database try { - wh.deleteDir(new Path(db.getLocationUri()), true, db); - } catch (Exception e) { - LOG.error("Failed to delete database directory: " + db.getLocationUri() + - " " + e.getMessage()); + final Database dbFinal = db; + Boolean deleted = UserGroupInformation.getLoginUser().doAs( + new PrivilegedExceptionAction() { + @Override + public Boolean run() throws MetaException { + return wh.deleteDir(new Path(dbFinal.getLocationUri()), true, dbFinal); + } + }); + if (!deleted) { + LOG.warn("Failed to delete database folder " + db.getLocationUri()); + } + } catch (IOException | InterruptedException | UndeclaredThrowableException e) { + LOG.warn("Might have failed to delete database folder " + db.getLocationUri()); } // it is not a terrible thing even if the data is not deleted + + try { + final Path externalPath = wh.getDefaultExternalDatabasePath(db.getName()); + Boolean deleted = UserGroupInformation.getCurrentUser().doAs( + new PrivilegedExceptionAction() { + @Override public Boolean run() throws IOException, MetaException { + return wh.deleteDirIfEmpty(externalPath); + } + }); + if (!deleted) { + LOG.warn("Failed to delete database external folder " + externalPath); + } + } catch (IOException | InterruptedException e) { + LOG.warn("Might have failed to delete database external folder got database " + db.getName()); + } } if (!listeners.isEmpty()) { diff --git a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java index da5a71cc64dd..4bf8e8f35b0f 100755 --- a/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java +++ b/standalone-metastore/src/main/java/org/apache/hadoop/hive/metastore/Warehouse.java @@ -201,6 +201,48 @@ public Path determineDatabasePath(Catalog cat, Database db) throws MetaException } } + /** + * Returns the external path if it exists in the expected location. Null otherwise + * @param db + * @return the path if it exists, null otherwise + * file system. + */ + public Path determineDatabaseExternalPath(Database db){ + if (hasExternalWarehouseRoot()) { + try { + // This means the location has been set manually and therefore we didn't create + // an external database directory + if (!isDatabaseLocationDefault(db)) { + return null; + } + + return getDefaultExternalDatabasePath(db.getName()); + + } catch (MetaException e) { + LOG.warn("Unable to determine external path for database " + db.getName()); + } + } + return null; + } + + /** + * This function is used to determine if the default location for the database has been + * given. In this case we will create an external database folder that we will check + * for the permissions. Otherwise the permissions will be based on the real location + * of the database. Determining if the database location is the default one this way + * is not ideal but we don't receive that kind of information from the client at this + * point. + * + * @param db + * @return whether the db location is the default one inside the warehouse directory + * or has been set manually + * + * @throws MetaException + */ + boolean isDatabaseLocationDefault(Database db) throws MetaException{ + return db.getLocationUri().startsWith(getWhRoot().toString()); + } + private String dbDirFromDbName(Database db) throws MetaException { return db.getName().toLowerCase() + DATABASE_WAREHOUSE_SUFFIX; } @@ -240,7 +282,7 @@ public Path getDefaultExternalDatabasePath(String dbName) throws MetaException { return new Path(getWhRootExternal(), dbName.toLowerCase() + DATABASE_WAREHOUSE_SUFFIX); } - private boolean hasExternalWarehouseRoot() { + public boolean hasExternalWarehouseRoot() { return !StringUtils.isBlank(whRootExternalString); } @@ -342,6 +384,16 @@ void addToChangeManagement(Path file) throws MetaException { } } + public boolean deleteDirIfEmpty(Path f) throws MetaException, IOException { + FileSystem fs = getFs(f); + if (FileUtils.isDirEmpty(fs, f)) { + return deleteDir(f, false, false, false); + } else { + LOG.info("Will not delete external directory " + f + " since it's not empty"); + } + return true; + } + public boolean deleteDir(Path f, boolean recursive, Database db) throws MetaException { return deleteDir(f, recursive, false, db); } diff --git a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java index cb32236d548e..ede1f7ba8f48 100644 --- a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java +++ b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/TestHiveMetaStore.java @@ -18,6 +18,7 @@ package org.apache.hadoop.hive.metastore; +import java.io.FileNotFoundException; import java.lang.reflect.Field; import java.io.IOException; import java.sql.Connection; @@ -40,6 +41,7 @@ import java.util.concurrent.TimeUnit; import com.google.common.collect.Sets; +import org.apache.hadoop.fs.FileStatus; import org.apache.hadoop.hive.metastore.api.CreationMetadata; import org.apache.hadoop.hive.metastore.client.builder.DatabaseBuilder; import org.apache.hadoop.hive.metastore.client.builder.TableBuilder; @@ -47,6 +49,7 @@ import org.apache.hadoop.hive.metastore.conf.MetastoreConf.ConfVars; import org.apache.hadoop.hive.metastore.utils.FileUtils; import org.apache.hadoop.hive.metastore.utils.SecurityUtils; +import org.apache.hadoop.security.UserGroupInformation; import org.datanucleus.api.jdo.JDOPersistenceManager; import org.datanucleus.api.jdo.JDOPersistenceManagerFactory; import org.junit.Assert; @@ -1069,6 +1072,50 @@ public void testDatabaseLocationWithPermissionProblems() throws Exception { assertTrue("Database creation succeeded even with permission problem", createFailed); } + @Test + public void testExternalDirectory() throws Exception{ + String externalDirString = MetastoreConf.getVar(conf, ConfVars.WAREHOUSE_EXTERNAL); + Path externalDir = new Path(externalDirString); + silentDropDatabase(TEST_DB1_NAME); + + String dbLocation = + MetastoreConf.getVar(conf, ConfVars.WAREHOUSE) + "/test/_testDB_create_"; + + String dbExternalLocation = externalDirString + "/testdb1.db"; + + + FileSystem fs = FileSystem.get(new Path(dbLocation).toUri(), conf); + fs.mkdirs( + new Path(MetastoreConf.getVar(conf, ConfVars.WAREHOUSE)), + new FsPermission((short) 700)); + + fs.mkdirs(externalDir, new FsPermission((short) 0777)); + + Database db = new DatabaseBuilder() + .setName(TEST_DB1_NAME) + .setLocation(dbLocation) + .build(conf); + client.createDatabase(db); + FileStatus fileStatus = fs.getFileStatus(new Path(dbExternalLocation)); + + assertTrue("External folder should have been created", fileStatus.isDirectory()); + assertEquals("External folder should have the right permissions", new FsPermission((short) 0755), + fileStatus.getPermission()); + assertEquals("External folder should be owned by the right username", + UserGroupInformation.getCurrentUser().getShortUserName(), fileStatus.getOwner()); + client.dropDatabase(db.getName()); + + try { + fs.getFileStatus(new Path(dbExternalLocation)); + fail("External directory should have been deleted"); + } catch (FileNotFoundException e) { + } finally { + fs.delete(new Path(MetastoreConf.getVar(conf, ConfVars.WAREHOUSE) + "/test"), true); + fs.delete(new Path(MetastoreConf.getVar(conf, + ConfVars.WAREHOUSE_EXTERNAL) + "/test"), true); + } + } + @Test public void testDatabaseLocation() throws Throwable { try { diff --git a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java index a15f5ea0453c..54302caa6ff6 100644 --- a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java +++ b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/client/TestAddPartitions.java @@ -523,7 +523,7 @@ public void testAddPartitionForExternalTableNullLocation() throws Exception { client.getPartition(DB_NAME, tableName, Lists.newArrayList(DEFAULT_YEAR_VALUE)); Assert.assertNotNull(resultPart); Assert.assertNotNull(resultPart.getSd()); - String defaultTableLocation = metaStore.getWarehouseRoot() + "/" + DB_NAME + ".db/" + tableName; + String defaultTableLocation = metaStore.getExternalWarehouseRoot() + "/" + DB_NAME + ".db/" + tableName; String defaulPartitionLocation = defaultTableLocation + "/year=2017"; Assert.assertEquals(defaulPartitionLocation, resultPart.getSd().getLocation()); } @@ -1197,7 +1197,7 @@ public void testAddPartitionsForExternalTableNullLocation() throws Exception { Lists.newArrayList("year=2017", "year=2018")); Assert.assertNotNull(resultParts); Assert.assertEquals(2, resultParts.size()); - String defaultTableLocation = metaStore.getWarehouseRoot() + "/" + DB_NAME + ".db/" + tableName; + String defaultTableLocation = metaStore.getExternalWarehouseRoot() + "/" + DB_NAME + ".db/" + tableName; String defaultPartLocation1 = defaultTableLocation + "/year=2017"; String defaultPartLocation2 = defaultTableLocation + "/year=2018"; if (resultParts.get(0).getValues().get(0).equals("2017")) { diff --git a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java index 709085d71ff8..5e9fdca35c0d 100644 --- a/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java +++ b/standalone-metastore/src/test/java/org/apache/hadoop/hive/metastore/minihms/AbstractMetaStoreService.java @@ -113,6 +113,16 @@ public Path getWarehouseRoot() throws MetaException { return warehouse.getWhRoot(); } + /** + * Returns the External MetaStore Warehouse root directory name. + * + * @return The external warehouse root directory + * @throws MetaException IO failure + */ + public Path getExternalWarehouseRoot() throws MetaException { + return warehouse.getWhRootExternal(); + } + /** * Check if a path exists. *