uros-b commented on code in PR #58438:
URL: https://github.com/apache/spark/pull/58438#discussion_r3904580403
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2.scala:
##########
@@ -105,6 +107,28 @@ object HiveThriftServer2 extends Logging {
}
}
+ // Sessions are impersonated for metastore calls, but queries run as the
service identity
+ // (SPARK-5159), so storage ACLs are checked against the wrong principal.
Auth types that never
+ // establish a user identity have nothing to impersonate, so they are exempt.
+ private[thriftserver] def warnIfIneffectiveDoAs(hiveConf: HiveConf): Unit = {
+ val authType = hiveConf.getVar(ConfVars.HIVE_SERVER2_AUTHENTICATION)
+ val unverifiedAuthTypes = Seq(AuthTypes.NONE,
AuthTypes.NOSASL).map(_.getAuthName)
+ // Constant on the left: authType is null when explicitly set empty, and
this must not NPE.
+ val authVerifiesUser =
!unverifiedAuthTypes.exists(_.equalsIgnoreCase(authType))
+ if (authVerifiesUser &&
hiveConf.getBoolVar(ConfVars.HIVE_SERVER2_ENABLE_DOAS)) {
+ logWarning(log"${MDC(CONFIG, ConfVars.HIVE_SERVER2_ENABLE_DOAS.varname)}
is set to true, " +
+ log"but the Spark Thrift Server impersonates the connecting user only
for Hive " +
+ log"metastore calls: queries and the storage access they perform still
run as the " +
+ log"server's own service identity, so storage permissions are checked
against the " +
+ log"service principal rather than the connecting user (SPARK-5159).
That can expose " +
+ log"data the connecting user is not authorized to read. Setting it to
false silences " +
+ log"this, but note that it stops impersonating metastore calls too, so
it is not a " +
+ log"no-op (and unsetting it does not help -- Hive's own default is
true). Spark 5.0 " +
+ log"is expected to refuse to start on this configuration; set " +
+ log"spark.sql.hive.thriftServer.allowIneffectiveDoAs=true there to
keep it running.")
Review Comment:
The warning message hardcodes
spark.sql.hive.thriftServer.allowIneffectiveDoAs, which isn't even merged yet,
right? I wonder if there is a more robust way to do this... Otherwise, the
implementation looks clean and the test coverage is good!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
