Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17640 )
Change subject: IMPALA-10436: Require lower privilege for external Kudu table creation ...................................................................... Patch Set 3: (1 comment) > Patch Set 3: > > (1 comment) > > I left some additional thoughts on the patch. Let me know what you think > about them. Thanks! Left some more thoughts on the security issue I pointed out previously. Let me know what you think about them. Sorry that the description is a bit lengthy. http://gerrit.cloudera.org:8080/#/c/17640/3/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java: http://gerrit.cloudera.org:8080/#/c/17640/3/fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java@341 PS3, Line 341: if (getTblProperties().containsKey(KuduTable.KEY_MASTER_HOSTS) || : (isExternal && !BackendConfig.INSTANCE.isKuduAuthorizationEnabled())) { : String authzServer = authzConfig.getServerName(); : Preconditions.checkNotNull(authzServer); : analyzer.registerPrivReq(builder -> builder.onServer(authzServer).all().build()); : } > Taking a look at this block and reviewing my previous patch for https://iss After some more thoughts, I came up with a solution that could address the security issue described above. Specifically, I have verified that by adding the following privilege check when the table to be created is an external Kudu table whose associated table property of 'kudu.table_name' is "kudu_tbl_name", we could make sure that the requesting user has to be granted the ALL privilege on the table "kudu_unique.kudu_tbl_name". String externalKuduTableName = getTblProperties().get("kudu.table_name"); analyzer.registerPrivReq( builder -> builder.onTable( "kudu_unique", externalKuduTableName, /* ownerUser */ null).all() .build()); For instance, when the statement is "create external table functional.ext_kudu_tbl stored as kudu tblproperties ('kudu.table_name'='impala::functional_kudu.tinytable')", the requesting user has to be granted the ALL privilege on the table "impala::functional_kudu.tinytable" under the database "kudu_unique", where "kudu_unique" is considered as a reserved word that is used to represent the Kudu storage and a non-administrative user should not be granted privileges on this database. The database "kudu_unique" does not really exist from Impala's perspective. Instead, it serves as a place to store the policies associated with those already existing Kudu tables on the Kudu side. We note that for the argument of 'ownerUser' for the onTable() method, we should provide "null". If we provide "getOnwer()", then this additional privilege request will be authorized by Ranger since this argument indicates the owner of the table to be created and it is initialized by the requesting user, meaning that Ranger will always authorize such a privilege request since it is equivalent to telling Ranger that the requesting user is the owner of the table (impala::functional_kudu.tinytable). Note that the requesting user also has to be granted the CREATE privilege on the table "ext_kudu_tbl" under the database "functional" in order for the external table to be created. One drawback of the approach described above is that we have to reserve a word that represents the Kudu storage and the word should not easily collide with a word that could already be chosen as a database name by current Impala users. So "kudu_unique" mentioned above would not be an ideal choice. If we use a random word like the hash value of a pre-defined string as the reserved word, then such a random word could be long and difficult to remember by an Impala user. A proper fix may be to make the Ranger plugin also check the Kudu policy repository (in addition to the Hive/Impala policy repository) but right now I do not know how easy it is to implement it. I think we may also consult the people working on the Ranger project to see how they resolved this security issue in Hive and see if we could also adopt their solution. -- To view, visit http://gerrit.cloudera.org:8080/17640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7936e1d8c48696169f7ad7ad92abe44a26eea3c4 Gerrit-Change-Number: 17640 Gerrit-PatchSet: 3 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Sun, 04 Jul 2021 23:09:54 +0000 Gerrit-HasComments: Yes
