You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by bi...@apache.org on 2018/10/10 18:06:18 UTC
[2/3] impala git commit: IMPALA-7676: DESCRIBE on table should
require VIEW_METADATA privilege
IMPALA-7676: DESCRIBE on table should require VIEW_METADATA privilege
IMPALA-6479 broke the DESCRIBE's privilege model by changing the
privilege from VIEW_METADATA to ANY in order to support column-level
privileges in DESCRIBE. This caused an issue where having non-
VIEW_METADATA privilege, such as CREATE privilege on a particular
database allows executing a DESCRIBE statement on all tables in the
database. This behavior is also inconsistent with Hive's DESCRIBE
and Impala's DESCRIBE DATABASE privilege models. Although there is not
any security risk for this particular issue since having non-
VIEW METADATA on a particular database always returns an empty result,
fixing this issue will make the behavior consistent with Hive and also
DESCRIBE DATABASE in Impala. This patch fixes the issue by changing the
privilege requirement back from ANY to VIEW_METADATA.
Testing:
- Ran all FE tests
Change-Id: I283e30ebff6d61e779a4cec8284cae0ccb90cc49
Reviewed-on: http://gerrit.cloudera.org:8080/11617
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/53decbc9
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/53decbc9
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/53decbc9
Branch: refs/heads/master
Commit: 53decbc9fcfe117470df09eb27b361195d704a06
Parents: 9ac874d
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Mon Oct 8 12:18:13 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Wed Oct 10 09:03:10 2018 +0000
----------------------------------------------------------------------
.../org/apache/impala/analysis/Analyzer.java | 27 ++++++++--
.../impala/analysis/DescribeTableStmt.java | 5 +-
.../impala/analysis/DropTableOrViewStmt.java | 3 +-
.../apache/impala/analysis/PartitionDef.java | 4 +-
.../impala/analysis/PartitionSpecBase.java | 3 +-
.../apache/impala/analysis/AuditingTest.java | 12 ++---
.../impala/analysis/AuthorizationStmtTest.java | 55 ++++++++++----------
7 files changed, 67 insertions(+), 42 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/53decbc9/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
index f848cc8..0672cfb 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
@@ -2413,21 +2413,25 @@ public class Analyzer {
* Returns the Table with the given name from the 'loadedTables' map in the global
* analysis state. Throws an AnalysisException if the table or the db does not exist.
* Throws a TableLoadingException if the registered table failed to load.
- * Always registers privilege request(s) for the table at the given privilege level(s),
+ * When addColumnLevelPrivilege is set to true, always registers privilege request(s)
+ * for the columns at the given table.
+ * When addColumnLevelPrivilege is set to false, always registers privilege request(s)
+ * for the table at the given privilege level(s),
* regardless of the state of the table (i.e. whether it exists, is loaded, etc.).
* If addAccessEvent is true adds access event(s) for successfully loaded tables. When
* multiple privileges are specified, all those privileges will be required for the
* authorization check.
*/
public FeTable getTable(TableName tableName, boolean addAccessEvent,
- Privilege... privilege) throws AnalysisException, TableLoadingException {
+ boolean addColumnPrivilege, Privilege... privilege)
+ throws AnalysisException, TableLoadingException {
Preconditions.checkNotNull(tableName);
Preconditions.checkNotNull(privilege);
tableName = getFqTableName(tableName);
for (Privilege priv : privilege) {
- if (priv == Privilege.ANY) {
+ if (priv == Privilege.ANY || addColumnPrivilege) {
registerPrivReq(new PrivilegeRequestBuilder()
- .any().onAnyColumn(tableName.getDb(), tableName.getTbl()).toRequest());
+ .allOf(priv).onAnyColumn(tableName.getDb(), tableName.getTbl()).toRequest());
} else {
registerPrivReq(new PrivilegeRequestBuilder()
.allOf(priv).onTable(tableName.getDb(), tableName.getTbl()).toRequest());
@@ -2458,7 +2462,20 @@ public class Analyzer {
public FeTable getTable(TableName tableName, Privilege... privilege)
throws AnalysisException {
try {
- return getTable(tableName, true, privilege);
+ return getTable(tableName, true, false, privilege);
+ } catch (TableLoadingException e) {
+ throw new AnalysisException(e);
+ }
+ }
+
+ /**
+ * Sets the addColumnPrivilege to true to add column-level privilege(s) for a given
+ * table instead of table-level privilege(s).
+ */
+ public FeTable getTable(TableName tableName, boolean addColumnPrivilege,
+ Privilege... privilege) throws AnalysisException {
+ try {
+ return getTable(tableName, true, addColumnPrivilege, privilege);
} catch (TableLoadingException e) {
throw new AnalysisException(e);
}
http://git-wip-us.apache.org/repos/asf/impala/blob/53decbc9/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java b/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
index 1e70468..e731dfc 100644
--- a/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
@@ -23,6 +23,7 @@ import java.util.List;
import org.apache.commons.lang3.StringUtils;
import org.apache.impala.analysis.Path.PathType;
import org.apache.impala.authorization.Privilege;
+import org.apache.impala.authorization.PrivilegeRequest;
import org.apache.impala.authorization.PrivilegeRequestBuilder;
import org.apache.impala.catalog.FeTable;
import org.apache.impala.catalog.StructType;
@@ -111,8 +112,10 @@ public class DescribeTableStmt extends StatementBase {
}
table_ = path_.getRootTable();
+
// Register authorization and audit events.
- analyzer.getTable(table_.getTableName(), Privilege.ANY);
+ analyzer.getTable(table_.getTableName(), /* add column-level privilege */ true,
+ Privilege.VIEW_METADATA);
// Describing a table.
if (path_.destTable() != null) return;
http://git-wip-us.apache.org/repos/asf/impala/blob/53decbc9/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java b/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
index 2d53794..1da6c68 100644
--- a/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
@@ -100,7 +100,8 @@ public class DropTableOrViewStmt extends StatementBase {
public void analyze(Analyzer analyzer) throws AnalysisException {
dbName_ = analyzer.getTargetDbName(tableName_);
try {
- FeTable table = analyzer.getTable(tableName_, true, Privilege.DROP);
+ FeTable table = analyzer.getTable(tableName_, /* add access event */ true,
+ /* add column-level privilege */ false, Privilege.DROP);
Preconditions.checkNotNull(table);
if (table instanceof FeView && dropTable_) {
throw new AnalysisException(String.format(
http://git-wip-us.apache.org/repos/asf/impala/blob/53decbc9/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java b/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
index 7b4f837..b66d779 100644
--- a/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
+++ b/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
@@ -83,7 +83,9 @@ public class PartitionDef implements ParseNode {
FeTable table;
try {
- table = analyzer.getTable(partitionSpec_.getTableName(), false, Privilege.ALTER);
+ table = analyzer.getTable(partitionSpec_.getTableName(),
+ /* add access event */ false, /* add column-level privilege */ false,
+ Privilege.ALTER);
} catch (TableLoadingException e) {
throw new AnalysisException(e.getMessage(), e);
}
http://git-wip-us.apache.org/repos/asf/impala/blob/53decbc9/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java b/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
index ccdb266..c4829a2 100644
--- a/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
+++ b/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
@@ -74,7 +74,8 @@ public abstract class PartitionSpecBase implements ParseNode {
// be audited outside of the PartitionSpec.
FeTable table;
try {
- table = analyzer.getTable(tableName_, false, privilegeRequirement_);
+ table = analyzer.getTable(tableName_, /* add access event */ false,
+ /* add column-level privilege */ false, privilegeRequirement_);
} catch (TableLoadingException e) {
throw new AnalysisException(e.getMessage(), e);
}
http://git-wip-us.apache.org/repos/asf/impala/blob/53decbc9/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
index 944fadb..836020b 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
@@ -319,20 +319,20 @@ public class AuditingTest extends FrontendTestBase {
Set<TAccessEvent> accessEvents =
AnalyzeAccessEvents("describe functional.alltypesagg");
Assert.assertEquals(accessEvents, Sets.newHashSet(new TAccessEvent(
- "functional.alltypesagg", TCatalogObjectType.TABLE, "ANY")));
+ "functional.alltypesagg", TCatalogObjectType.TABLE, "VIEW_METADATA")));
accessEvents = AnalyzeAccessEvents("describe formatted functional.alltypesagg");
Assert.assertEquals(accessEvents, Sets.newHashSet(new TAccessEvent(
- "functional.alltypesagg", TCatalogObjectType.TABLE, "ANY")));
+ "functional.alltypesagg", TCatalogObjectType.TABLE, "VIEW_METADATA")));
accessEvents = AnalyzeAccessEvents("describe functional.complex_view");
Assert.assertEquals(accessEvents, Sets.newHashSet(new TAccessEvent(
- "functional.complex_view", TCatalogObjectType.VIEW, "ANY")));
+ "functional.complex_view", TCatalogObjectType.VIEW, "VIEW_METADATA")));
accessEvents = AnalyzeAccessEvents(
"describe functional.allcomplextypes.int_array_col");
Assert.assertEquals(accessEvents, Sets.newHashSet(new TAccessEvent(
- "functional.allcomplextypes", TCatalogObjectType.TABLE, "ANY")));
+ "functional.allcomplextypes", TCatalogObjectType.TABLE, "VIEW_METADATA")));
}
@Test
@@ -459,12 +459,12 @@ public class AuditingTest extends FrontendTestBase {
// Describe
accessEvents = AnalyzeAccessEvents("describe functional_kudu.testtbl");
Assert.assertEquals(accessEvents, Sets.newHashSet(new TAccessEvent(
- "functional_kudu.testtbl", TCatalogObjectType.TABLE, "ANY")));
+ "functional_kudu.testtbl", TCatalogObjectType.TABLE, "VIEW_METADATA")));
// Describe formatted
accessEvents = AnalyzeAccessEvents("describe formatted functional_kudu.testtbl");
Assert.assertEquals(accessEvents, Sets.newHashSet(new TAccessEvent(
- "functional_kudu.testtbl", TCatalogObjectType.TABLE, "ANY")));
+ "functional_kudu.testtbl", TCatalogObjectType.TABLE, "VIEW_METADATA")));
}
/**
http://git-wip-us.apache.org/repos/asf/impala/blob/53decbc9/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
index 8be63b9..02ae21b 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
@@ -25,7 +25,6 @@ import org.apache.impala.authorization.AuthorizationConfig;
import org.apache.impala.authorization.PrivilegeRequest;
import org.apache.impala.authorization.User;
import org.apache.impala.catalog.AuthorizationException;
-import org.apache.impala.catalog.PrincipalPrivilege;
import org.apache.impala.catalog.Role;
import org.apache.impala.catalog.ScalarFunction;
import org.apache.impala.catalog.Type;
@@ -1220,18 +1219,18 @@ public class AuthorizationStmtTest extends FrontendTestBase {
onTable("functional", "alltypes", privilege));
}
authzTest
- .okDescribe(tableName, describeOutput(style).excludeStrings(ALLTYPES_COLUMNS),
- onServer(allExcept(viewMetadataPrivileges())))
- .okDescribe(tableName, describeOutput(style).excludeStrings(ALLTYPES_COLUMNS),
- onDatabase("functional",allExcept(viewMetadataPrivileges())))
- .okDescribe(tableName, describeOutput(style).excludeStrings(ALLTYPES_COLUMNS),
- onTable("functional", "alltypes", allExcept(viewMetadataPrivileges())))
// In this test, since we only have column level privileges on "id", then
// only the "id" column should show and the others should not.
.okDescribe(tableName, describeOutput(style).includeStrings(new String[]{"id"})
.excludeStrings(ALLTYPES_COLUMNS_WITHOUT_ID), onColumn("functional",
"alltypes", "id", TPrivilegeLevel.SELECT))
- .error(accessError("functional.alltypes"));
+ .error(accessError("functional.alltypes"))
+ .error(accessError("functional.alltypes"),
+ onServer(allExcept(viewMetadataPrivileges())))
+ .error(accessError("functional.alltypes"),
+ onDatabase("functional", allExcept(viewMetadataPrivileges())))
+ .error(accessError("functional.alltypes"),
+ onTable("functional", "alltypes", allExcept(viewMetadataPrivileges())));
// Describe table extended.
tableName = new TTableName("functional", "alltypes");
@@ -1248,21 +1247,19 @@ public class AuthorizationStmtTest extends FrontendTestBase {
.okDescribe(tableName, describeOutput(style).includeStrings(checkStrings),
onTable("functional", "alltypes", privilege));
}
- // Describe table without VIEW_METADATA privilege should not show all columns and
- // location.
authzTest
- .okDescribe(tableName, describeOutput(style).excludeStrings(ALLTYPES_COLUMNS),
- onServer(allExcept(viewMetadataPrivileges())))
- .okDescribe(tableName, describeOutput(style).excludeStrings(ALLTYPES_COLUMNS),
- onDatabase("functional", allExcept(viewMetadataPrivileges())))
- .okDescribe(tableName, describeOutput(style).excludeStrings(ALLTYPES_COLUMNS),
- onTable("functional", "alltypes", allExcept(viewMetadataPrivileges())))
// Location should not appear with only column level auth.
.okDescribe(tableName, describeOutput(style).includeStrings(new String[]{"id"})
.excludeStrings((String[]) ArrayUtils.addAll(ALLTYPES_COLUMNS_WITHOUT_ID,
- new String[]{"Location:"})), onColumn("functional", "alltypes", "id",
+ new String[]{"Location:"})), onColumn("functional", "alltypes", "id",
TPrivilegeLevel.SELECT))
- .error(accessError("functional.alltypes"));
+ .error(accessError("functional.alltypes"))
+ .error(accessError("functional.alltypes"),
+ onServer(allExcept(viewMetadataPrivileges())))
+ .error(accessError("functional.alltypes"),
+ onDatabase("functional.alltypes", allExcept(viewMetadataPrivileges())))
+ .error(accessError("functional.alltypes"),
+ onTable("functional", "alltypes", allExcept(viewMetadataPrivileges())));
// Describe view.
tableName = new TTableName("functional", "alltypes_view");
@@ -1278,11 +1275,13 @@ public class AuthorizationStmtTest extends FrontendTestBase {
onTable("functional", "alltypes_view", privilege));
}
authzTest
- .okDescribe(tableName, describeOutput(style).excludeStrings(ALLTYPES_COLUMNS),
+ .error(accessError("functional.alltypes_view"))
+ .error(accessError("functional.alltypes_view"),
onServer(allExcept(viewMetadataPrivileges())))
- .okDescribe(tableName, describeOutput(style).excludeStrings(ALLTYPES_COLUMNS),
+ .error(accessError("functional.alltypes_view"),
onDatabase("functional",allExcept(viewMetadataPrivileges())))
- .error(accessError("functional.alltypes_view"));
+ .error(accessError("functional.alltypes_view"),
+ onTable("functional", "alltypes_view", allExcept(viewMetadataPrivileges())));
// Describe view extended.
tableName = new TTableName("functional", "alltypes_view");
@@ -1301,15 +1300,17 @@ public class AuthorizationStmtTest extends FrontendTestBase {
onTable("functional", "alltypes_view", privilege));
}
authzTest
- .okDescribe(tableName, describeOutput(style).excludeStrings(ALLTYPES_COLUMNS),
- onServer(allExcept(viewMetadataPrivileges())))
- .okDescribe(tableName, describeOutput(style).excludeStrings(ALLTYPES_COLUMNS),
- onDatabase("functional",allExcept(viewMetadataPrivileges())))
- .error(accessError("functional.alltypes_view"));
+ .error(accessError("functional.alltypes_view"))
+ .error(accessError("functional.alltypes_view"), onServer(
+ allExcept(viewMetadataPrivileges())))
+ .error(accessError("functional.alltypes_view"), onDatabase("functional",
+ allExcept(viewMetadataPrivileges())))
+ .error(accessError("functional.alltypes_view"), onTable("functional", "alltypes",
+ allExcept(viewMetadataPrivileges())));
// Describe specific column on a table.
authzTest = authorize("describe functional.allcomplextypes.int_struct_col");
- for (TPrivilegeLevel privilege: TPrivilegeLevel.values()) {
+ for (TPrivilegeLevel privilege: viewMetadataPrivileges()) {
authzTest.ok(onServer(privilege))
.ok(onDatabase("functional", privilege))
.ok(onTable("functional", "allcomplextypes", privilege));