dongjoon-hyun commented on code in PR #58414:
URL: https://github.com/apache/spark/pull/58414#discussion_r3990870570
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2.scala:
##########
@@ -105,6 +106,34 @@ 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.
Refuse rather than
+ // look like we enforce something we do not. Auth types that never establish
a user identity
+ // have nothing to impersonate, so they are exempt.
+ private[thriftserver] def failIfIneffectiveDoAs(
+ hiveConf: HiveConf,
+ allowIneffectiveDoAs: Boolean): 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) &&
+ !allowIneffectiveDoAs) {
+ throw new IllegalArgumentException(
Review Comment:
This is a user-facing error, so it should go through the error-class
framework rather than a raw `IllegalArgumentException` with a prose message
(`common/utils/src/main/resources/error/README.md`: "developers should specify
a standardized SQLSTATE, an error condition, and message parameters rather than
an arbitrary error message"). The sibling startup check in
`SparkSQLCLIService.init` already does this via
`QueryExecutionErrors.invalidKerberosConfigForHiveServer2Error`. Could you add
an error condition to `error-conditions.json` and throw it via
`QueryExecutionErrors` / `HiveThriftServerErrors`, so the exception is a
`SparkThrowable` with a SQLSTATE and can be asserted with `checkError`?
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2.scala:
##########
@@ -105,6 +106,34 @@ 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.
Refuse rather than
+ // look like we enforce something we do not. Auth types that never establish
a user identity
+ // have nothing to impersonate, so they are exempt.
+ private[thriftserver] def failIfIneffectiveDoAs(
+ hiveConf: HiveConf,
+ allowIneffectiveDoAs: Boolean): 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) &&
+ !allowIneffectiveDoAs) {
+ throw new IllegalArgumentException(
+ s"${ConfVars.HIVE_SERVER2_ENABLE_DOAS.varname} is set to true, but the
Spark Thrift " +
+ "Server impersonates the connecting user only for Hive metastore
calls: queries and " +
Review Comment:
"queries and the storage access they perform still run as the server's own
service identity" is only true on the executor side. `HiveSessionProxy` wraps
`executeStatementAsync` in the session UGI's `doAs`, so
`SparkExecuteStatementOperation.runInternal` calls `Utils.getUGI()` while
already impersonating the connecting user, and the background action runs under
that proxy UGI on the driver. Driver-side `FileSystem` calls (e.g.
`InMemoryFileIndex` listing) are therefore impersonated; only executor task I/O
runs as the service principal. Since operators will read this text (and the
conf doc / migration note, which repeat it) to decide whether to set
`allowIneffectiveDoAs=true`, it would be better to say executor-side data
access is not impersonated rather than "queries" as a whole.
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2.scala:
##########
@@ -105,6 +106,34 @@ 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.
Refuse rather than
+ // look like we enforce something we do not. Auth types that never establish
a user identity
+ // have nothing to impersonate, so they are exempt.
+ private[thriftserver] def failIfIneffectiveDoAs(
+ hiveConf: HiveConf,
+ allowIneffectiveDoAs: Boolean): Unit = {
+ val authType = hiveConf.getVar(ConfVars.HIVE_SERVER2_AUTHENTICATION)
+ val unverifiedAuthTypes = Seq(AuthTypes.NONE,
AuthTypes.NOSASL).map(_.getAuthName)
Review Comment:
The comment above says `NONE`/`NOSASL` "have nothing to impersonate", but
that is not what Hive does. `ThriftCLIService.getSessionHandle` opens the
session with impersonation whenever `doAs` is true and a username exists, and
under `NONE` `getUserName` falls back to the client-supplied
`req.getUsername()`. So with `NONE` + `doAs=true` (Hive's out-of-the-box
config) metastore calls are impersonated as an unverified, client-chosen name
while storage still runs as the service principal, which is the same SPARK-5159
gap this guard describes. Exempting `NONE` may still be the right call (there
is no security expectation without a verified identity), but the rationale
should say that rather than "nothing to impersonate". Alternatively, keying on
`doAs` alone or on `UserGroupInformation.isSecurityEnabled` would avoid a
hand-maintained `AuthTypes` list.
##########
project/SparkBuild.scala:
##########
@@ -584,6 +584,8 @@ object SparkParallelTestGrouping {
"org.apache.spark.sql.SQLQueryTestSuite",
"org.apache.spark.sql.hive.client.HadoopVersionInfoSuite",
"org.apache.spark.sql.hive.thriftserver.SparkExecuteStatementOperationSuite",
+ // Constructs a HiveThriftServer2, which flips HiveConf's static
loadHiveServer2Config.
Review Comment:
`ThriftServerWithSparkContextInBinarySuite` / `InHttpSuite` already
construct a `HiveThriftServer2` in-process via
`SharedThriftServer.startWithSparkSession`, and they run in the default test
group without a dedicated JVM. If the static `loadHiveServer2Config` flip were
a real hazard, those suites would need this too. Unless there is an observed
failure, I'd drop this entry so we don't fork an extra JVM for a suite of eight
short tests.
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2.scala:
##########
@@ -105,6 +106,34 @@ 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.
Refuse rather than
+ // look like we enforce something we do not. Auth types that never establish
a user identity
+ // have nothing to impersonate, so they are exempt.
+ private[thriftserver] def failIfIneffectiveDoAs(
+ hiveConf: HiveConf,
+ allowIneffectiveDoAs: Boolean): 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.
Review Comment:
I don't think `authType` can be null here. `HiveConf.getVar` returns `""`
when the value is explicitly set empty and the default `NONE` for an empty
`<value/>` in `hive-site.xml` (Hadoop `Configuration.get(name, default)`
semantics), and `Configuration.set(name, null)` is rejected. So the
constant-on-the-left is harmless but the comment is misleading. Separately, an
empty or misspelled auth type is classified as "verifies user" and reported as
a doAs problem; following the advice to set `allowIneffectiveDoAs=true` then
does not help, because `HiveAuthFactory.getAuthTransFactory` fails with
`Unsupported authentication type`. Either correct the comment, or leave invalid
auth types to Hive's own error.
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2DoAsSuite.scala:
##########
@@ -0,0 +1,96 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.hive.thriftserver
+
+import org.apache.hadoop.hive.conf.HiveConf
+import org.apache.hadoop.hive.conf.HiveConf.ConfVars
+import org.apache.hive.service.auth.HiveAuthFactory.AuthTypes
+
+import org.apache.spark.sql.QueryTest
+import org.apache.spark.sql.internal.StaticSQLConf
+import org.apache.spark.sql.test.SharedSparkSession
+
+/**
+ * Tests for the SPARK-59118 startup guard: the Thrift Server refuses to start
when
+ * `hive.server2.enable.doAs` is on but impersonation does not reach query
execution.
+ */
+class HiveThriftServer2DoAsSuite extends QueryTest with SharedSparkSession {
Review Comment:
Only the last test (`init refuses to start the server`) uses `spark`; the
other seven call `failIfIneffectiveDoAs` directly on a `HiveConf`, and the
branch-4.x sibling in #58438 uses `SparkFunSuite` for the same cases. Could we
keep this suite on `SparkFunSuite` and move the single `init()` wiring test
into an existing session-backed thrift-server suite? That avoids paying
`SharedSparkSession` setup for pure-function tests and keeps the 4.x and master
suites aligned.
--
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]
