Skip to content

Commit c836d98

Browse files
committed
[SC-5483] Fix CreateTempTable/ShowPermissions/ShowDatabases in CheckPermissions
## What changes were proposed in this pull request? The CheckPermissions rule currently does not handle the `ShowDatabasesCommand` and the `ShowPermissionsCommand` nodes, as a result the rule will throw a SecurityException when one of these commands is used. This PR adds these commands to the whitelist. We also change the required permission for `CreateTempTableUsing` and the 'run-SQL-on-file' functionality, both now require a Select on an Anonymous File (a blanket permission). The reason for this is that both code paths allow a user to interact directly with the filesystem, and gives the user a way to circumvent ACLs defined on a table (by querying the files backing the table directly). ## How was this patch tested? Added tests to `CheckPermissionRuleSuite`. Author: Herman van Hovell <[email protected]> Closes apache#156 from hvanhovell/SC-5483.
1 parent 2838437 commit c836d98

File tree

9 files changed

+87
-10
lines changed

9 files changed

+87
-10
lines changed

sql/core/src/main/antlr4/com/databricks/sql/acl/AclCommandBase.g4

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ securable
3939
| objectType=VIEW qualifiedName
4040
| objectType=FUNCTION qualifiedName
4141
| ANONYMOUS objectType=FUNCTION
42+
| ANY objectType=FILE
4243
| objectType=TABLE? qualifiedName
4344
;
4445

@@ -58,7 +59,7 @@ quotedIdentifier
5859

5960
nonReserved
6061
: ALTER | OWNER | TO | MSCK | REPAIR | PRIVILEGES | SHOW | GRANT | ON | ALL | WITH | OPTION |
61-
REVOKE | FOR | FROM | CATALOG | DATABASE | TABLE | VIEW | FUNCTION | ANONYMOUS
62+
REVOKE | FOR | FROM | CATALOG | DATABASE | TABLE | VIEW | FUNCTION | ANONYMOUS | FILE | ANY
6263
;
6364

6465
ALTER: 'ALTER';
@@ -82,6 +83,8 @@ TABLE: 'TABLE';
8283
VIEW: 'VIEW';
8384
FUNCTION: 'FUNCTION';
8485
ANONYMOUS: 'ANONYMOUS';
86+
FILE: 'FILE';
87+
ANY: 'ANY';
8588

8689
STRING
8790
: '\'' ( ~('\''|'\\') | ('\\' .) )* '\''

sql/core/src/main/scala/com/databricks/sql/acl/AstCommandBuilder.scala

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ class AstCommandBuilder(client: AclClient)
139139
AnonymousFunction
140140
case AclCommandBaseParser.FUNCTION =>
141141
Function(visitFunctionIdentifier(ctx.qualifiedName))
142+
case AclCommandBaseParser.FILE =>
143+
AnyFile
142144
case _ =>
143145
throw new ParseException("Unknown Securable Object", ctx)
144146
}

sql/core/src/main/scala/com/databricks/sql/acl/CheckPermissions.scala

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,8 @@ class CheckPermissions(catalog: PublicCatalog, aclClient: AclClient)
8080
case rel: CatalogRelation if isTempDb(rel.catalogTable.identifier.database) => Nil
8181
case rel: CatalogRelation => Seq(Request(Table(rel.catalogTable.identifier), Select))
8282

83+
case LogicalRelation(_: HadoopFsRelation, _, None) =>
84+
Seq(Select(AnyFile))
8385
case LogicalRelation(_, _, None) => Nil
8486
case LogicalRelation(_, _, Some(tableId)) if isTempDb(tableId.identifier.database) => Nil
8587
case LogicalRelation(_, _, Some(tableId)) =>
@@ -131,7 +133,8 @@ class CheckPermissions(catalog: PublicCatalog, aclClient: AclClient)
131133
case create: CreateTable =>
132134
toSecurables(create.tableDesc.identifier.database).map(Create) ++
133135
create.query.toSeq.flatMap(queryToRequests)
134-
case create: CreateTempViewUsing => Nil
136+
case _: CreateTempViewUsing =>
137+
Seq(Select(AnyFile))
135138
case create: CreateDataSourceTableCommand =>
136139
toSecurables(create.table.identifier.database).map(Create)
137140
case create: CreateTableCommand =>
@@ -192,7 +195,7 @@ class CheckPermissions(catalog: PublicCatalog, aclClient: AclClient)
192195
case drop: DropFunctionCommand =>
193196
toSecurables(FunctionIdentifier(drop.functionName, drop.databaseName)).map(Own)
194197

195-
case show: ShowDatabasesCommand =>
198+
case _: ShowDatabasesCommand =>
196199
Seq(Request(Catalog, ReadMetadata))
197200
case show: ShowTablesCommand =>
198201
toSecurables(show.databaseName).map(ReadMetadata)

sql/core/src/main/scala/com/databricks/sql/acl/commands.scala

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ trait AclCommand extends RunnableCommand {
3838
Some(Function(FunctionIdentifier(fn, Option(qualified.database))))
3939
case Function(FunctionIdentifier(fn, Some(db))) if catalog.functionExists(db, fn) =>
4040
Some(securable)
41+
case AnyFile =>
42+
Some(securable)
4143
case _ => None
4244
}
4345
}

sql/core/src/main/scala/com/databricks/sql/acl/interfaces.scala

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,8 @@ object Securable {
139139
parseString(rep) match {
140140
case Seq("ANONYMOUS_FUNCTION") =>
141141
AnonymousFunction
142+
case Seq("ANY_FILE") =>
143+
AnyFile
142144
case Seq("CATALOG", "default") =>
143145
Catalog
144146
case Seq("CATALOG", "default", "DATABASE", db) =>
@@ -203,6 +205,12 @@ case object AnonymousFunction extends SecurableFunction {
203205
override val name: String = "/" + typeName
204206
}
205207

208+
case object AnyFile extends Securable {
209+
override val key: Option[Nothing] = None
210+
override def typeName: String = "ANY_FILE"
211+
override val name: String = "/" + typeName
212+
}
213+
206214
/**
207215
* The action that a [[Principal]] wants to execute on a [[Securable]]. An [[Action]] should always
208216
* be a verb.
@@ -275,12 +283,12 @@ object Action {
275283
override val name: String = "SELECT"
276284

277285
override def validateRequest(securable: Securable): Boolean = securable match {
278-
case _: Table | _: SecurableFunction => true
286+
case _: Table | _: SecurableFunction | AnyFile => true
279287
case _ => false
280288
}
281289

282290
override def validateGrant(securable: Securable): Boolean = securable match {
283-
case Catalog | _: Database | _: Table | _: SecurableFunction => true
291+
case Catalog | _: Database | _: Table | _: SecurableFunction | AnyFile => true
284292
case _ => false
285293
}
286294
}
@@ -290,12 +298,12 @@ object Action {
290298
override val name: String = "MODIFY"
291299

292300
override def validateRequest(securable: Securable): Boolean = securable match {
293-
case _: Table => true
301+
case _: Table | AnyFile => true
294302
case _ => false
295303
}
296304

297305
override def validateGrant(securable: Securable): Boolean = securable match {
298-
case Catalog | _: Database | _: Table => true
306+
case Catalog | _: Database | _: Table | AnyFile => true
299307
case _ => false
300308
}
301309
}

sql/core/src/test/scala/com/databricks/sql/acl/AclCommandParseSuite.scala

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,12 @@ class AclCommandParseSuite extends PlanTest {
8181
client,
8282
Permission(user3, Select, AnonymousFunction) :: Nil))
8383

84+
checkAnswer(
85+
"GRANT SELECT ON ANY FILE TO user3",
86+
GrantPermissionsCommand(
87+
client,
88+
Permission(user3, Select, AnyFile) :: Nil))
89+
8490
intercept("GRANT BLOB ON x TO `r2d2`", "Unknown ActionType")
8591

8692
intercept("GRANT SELECT ON CAR x TO `r2d2`")
@@ -116,6 +122,12 @@ class AclCommandParseSuite extends PlanTest {
116122
client,
117123
Permission(user3, Select, AnonymousFunction) :: Nil))
118124

125+
checkAnswer(
126+
"REVOKE SELECT ON ANY FILE FROM user3",
127+
RevokePermissionsCommand(
128+
client,
129+
Permission(user3, Select, AnyFile) :: Nil))
130+
119131
intercept("GRANT MOVE ON x TO c3po", "Unknown ActionType")
120132

121133
intercept("GRANT SELECT ON TAXI x TO c3po")

sql/core/src/test/scala/com/databricks/sql/acl/CheckPermissionRuleSuite.scala

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*/
99
package com.databricks.sql.acl
1010

11-
import com.databricks.sql.acl.Action.ReadMetadata
11+
import com.databricks.sql.acl.Action.{ReadMetadata, Select}
1212
import org.apache.hadoop.fs.Path
1313
import org.mockito.Mockito._
1414

@@ -98,6 +98,12 @@ class CheckPermissionRuleSuite extends CheckRequests {
9898
LogicalRelation(mockBaseRel, None, Some(createCatalogTable(temp1)))
9999
}
100100

101+
protected def anonymousRel(): LogicalRelation = {
102+
val mockBaseRel = mock(classOf[HadoopFsRelation])
103+
when(mockBaseRel.schema).thenReturn(new StructType)
104+
LogicalRelation(mockBaseRel, None, None)
105+
}
106+
101107
val db: String = "PERM1"
102108
val V1: TableIdentifier = TableIdentifier("V1", Some(db))
103109
val T1: TableIdentifier = TableIdentifier("T1", Some(db))
@@ -193,7 +199,8 @@ class CheckPermissionRuleSuite extends CheckRequests {
193199

194200
private val ddlOnlyCommands: Seq[(String, LogicalPlan, Set[Request])] = Seq(
195201
addDDL("create temp view", CreateTempViewUsing(
196-
TableIdentifier("blah"), None, false, false, null, Map.empty)),
202+
TableIdentifier("blah"), None, false, false, null, Map.empty),
203+
Request(AnyFile, Action.Select)),
197204
addDDL("createdstable", CreateDataSourceTableCommand(
198205
createCatalogTable(T1), false),
199206
Request(Database(T1.database.get), Action.Create)),
@@ -231,7 +238,16 @@ class CheckPermissionRuleSuite extends CheckRequests {
231238
addDDL("dropfunc", DropFunctionCommand(func1.database, func1.funcName, false, false),
232239
Request(Function(func1), Action.Own)),
233240
addDDL("dropfunctmp", DropFunctionCommand(tempFunc1.database, tempFunc1.funcName, false, true)),
234-
addDDL("noop", NoOpRunnableCommand)
241+
addDDL("noop", NoOpRunnableCommand),
242+
addDDL("showdatabases", ShowDatabasesCommand(None), ReadMetadata(Catalog)),
243+
addDDL("showtables", ShowTablesCommand(Option(db), None), ReadMetadata(Database(db))),
244+
addDDL("showfunctions", ShowFunctionsCommand(Option(db), None, true, true),
245+
ReadMetadata(Database(db))),
246+
addDDL("showpartitions", ShowPartitionsCommand(T1, None), ReadMetadata(Table(T1))),
247+
addDDL("showcolmns", ShowColumnsCommand(None, T1), ReadMetadata(Table(T1))),
248+
addDDL("showcreatetable", ShowCreateTableCommand(T1), ReadMetadata(Table(T1))),
249+
addDDL("showpermissions", ShowPermissionsCommand(NoOpAclClient, None, Catalog)),
250+
addDDL("logicalrelation", anonymousRel(), Select(AnyFile))
235251
)
236252
private def mockedLogicalRelation(tableId: TableIdentifier): LogicalRelation = {
237253
val mocked = mock(classOf[LogicalRelation])

sql/core/src/test/scala/com/databricks/sql/acl/StringSerializationSuite.scala

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,11 @@ class StringSerializationSuite extends SparkFunSuite{
105105
assert(e.getMessage.contains("A database is not defined for FUNCTION"))
106106
}
107107

108+
test("anonymous objects") {
109+
roundTripSecurable(AnyFile, "/ANY_FILE")
110+
roundTripSecurable(AnonymousFunction, "/ANONYMOUS_FUNCTION")
111+
}
112+
108113
test("invalid names") {
109114
def checkFail(in: String): Unit = {
110115
val e = intercept[IllegalArgumentException](Securable.fromString(in))

sql/hive/src/test/scala/com/databricks/sql/acl/HiveSQLCheckPermissionRuleSuite.scala

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@
88
*/
99
package com.databricks.sql.acl
1010

11+
import java.nio.file.Files
12+
1113
import org.scalatest.BeforeAndAfterEach
1214

1315
import org.apache.spark.sql._
@@ -539,4 +541,28 @@ class HiveSQLCheckPermissionRuleSuite extends TestHiveExtensions(TestHiveAclExte
539541
assertFail("drop function perm.fnx")
540542
assertNoOp("drop function if exists perm.fnx")
541543
}
544+
545+
test("query on file") {
546+
val path = Files.createTempDirectory("somedata").toString + "/SC-5483"
547+
asUser("super") {
548+
spark.range(1000).write.parquet(path)
549+
}
550+
asUser("U") {
551+
intercept[SecurityException] {
552+
spark.sql(s"select * from parquet.`$path`")
553+
}
554+
intercept[SecurityException] {
555+
spark.sql(s"create temporary view x using parquet options(path '$path')")
556+
}
557+
}
558+
asUser("super") {
559+
spark.sql("grant select on any file to U")
560+
}
561+
asUser("U") {
562+
assert(spark.sql(s"select * from parquet.`$path`").count === 1000L)
563+
spark.sql(s"create temporary view x using parquet options(path '$path')")
564+
assert(spark.sql("select * from x").count === 1000L)
565+
spark.sql(s"drop view x")
566+
}
567+
}
542568
}

0 commit comments

Comments
 (0)