jerryshao commented on code in PR #13858: URL: https://github.com/apache/gravitino/pull/13858#discussion_r4290096841
########## core/src/main/java/org/apache/gravitino/job/JobRuntimeTemplateMaskUtils.java: ########## @@ -0,0 +1,440 @@ +/* + * 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.gravitino.job; + +import com.google.common.collect.ImmutableSet; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; +import javax.annotation.Nullable; +import org.apache.gravitino.connector.HiddenPropertyMaskUtils; +import org.apache.gravitino.dto.job.JobTemplateDTO; +import org.apache.gravitino.dto.job.ShellJobTemplateDTO; +import org.apache.gravitino.dto.job.SparkJobTemplateDTO; +import org.apache.gravitino.secret.SecretPropertyUtils; + +/** + * Masks sensitive-named values on resolved runtime job templates before they are exposed on API or + * event paths. Stored runtime JSON is unchanged. + * + * <p>Argument masking is grouped by payload shape: + * + * <ul> + * <li>Nest JSON — {@link #NESTED_JSON_ARGUMENT_FLAGS} ({@code --updater-options} / {@code + * --spark-conf} / {@code --options}) + * <li>Spark conf property — {@link #CONF_ARGUMENT_FLAGS} ({@code --conf} / {@code -c}): a single + * {@code key=value} in either two-token or {@code --conf=key=value} form + * <li>JVM options string — {@link #JVM_OPTIONS_ARGUMENT_FLAGS} ({@code --driver-java-options} / + * {@code --executor-java-options}): a space-separated JVM string that may start with {@code + * -}; sensitive {@code -Dkey=value} tokens inside are masked + * <li>Sensitive-named long options — {@code --name} / {@code --name=value} when the name matches + * {@link SecretPropertyUtils#isSensitivePropertyKey} (the following token after {@code + * --name} is always replaced with {@link HiddenPropertyMaskUtils#MASKED_VALUE}, including + * non-secret values such as identifiers after {@code --use-credential-file}) + * <li>Standalone {@code -Dkey=value} when {@code key} is sensitive + * </ul> + * + * <p>Map fields {@code environments} / {@code customFields} / {@code configs} and runJob {@code + * jobConf} mask sensitive-named keys, plus nested JSON under {@link #NESTED_JSON_MAP_KEYS}. + * + * <p>Not masked (out of scope): resource URI fields ({@code executable}, {@code className}, {@code + * jars}, {@code files}, {@code archives}, Shell {@code scripts}); {@code --hiveconf}; conf property + * values that embed {@code -D} secrets under a non-sensitive conf key (for example {@code + * spark.driver.extraJavaOptions}); free-form tokens such as a {@code --where} SQL predicate. + * + * <p>Null map entries are preserved in the masked DTO / {@code jobConf} maps used on the REST and + * event {@code jobConf} paths. When {@code JobInfo} rebuilds a {@code JobTemplate} via {@code + * ImmutableMap.copyOf}, null values cannot be carried; a hand-edited runtime JSON with nulls may + * cause that rebuild to be skipped (logged) rather than expose a partial template. + */ +public final class JobRuntimeTemplateMaskUtils { + + /** + * JobConf / template keys whose values are known to be JSON maps (Iceberg built-in jobs and the + * optimizer). Nested sensitive keys inside these values are masked. + * + * <ul> + * <li>{@code updater_options} — update-stats job credentials / client options + * <li>{@code spark_conf} — Spark configs for built-in Iceberg jobs + * <li>{@code options} — rewrite-data-files / compaction options (from policy {@code jobOptions} + * / {@code rewriteOptions}) + * </ul> + */ + static final Set<String> NESTED_JSON_MAP_KEYS = + ImmutableSet.of("updater_options", "spark_conf", "options"); + + /** + * Iceberg built-in job CLI flags whose following argument is a JSON map with the same masking + * rules as {@link #NESTED_JSON_MAP_KEYS}. + */ + static final Set<String> NESTED_JSON_ARGUMENT_FLAGS = + ImmutableSet.of("--updater-options", "--spark-conf", "--options"); + + /** + * Spark flags whose payload is a single {@code key=value} config entry. Both the two-token form + * ({@code --conf key=value}) and the equals form ({@code --conf=key=value}) use {@link + * #maskConfKeyValuePayload}. + */ + static final Set<String> CONF_ARGUMENT_FLAGS = ImmutableSet.of("--conf", "-c"); Review Comment: [Important] Keeping `-c` in this set makes a shell command look like a Spark conf entry, and the response then drops the rest of the command - the same truncation this commit set out to remove. `maskSensitiveArguments` runs for `ShellJobTemplateDTO` too (:137-150), and `/bin/sh -c "<command>"` is the canonical shell template shape: the repo's own job templates build exactly that (`core/src/test/java/org/apache/gravitino/job/local/TestLocalJobExecutor.java:709-717` - executable `/bin/sh`, arguments `["-c", "trap '' TERM; echo ready; exec sleep 300"]`). For such a template `maskConfKeyValuePayload` (:351-361) takes everything before the first `=` as the conf key and replaces everything after it with `******`, so `-c "export ACCOUNT_ID=5 && run.sh --flag"` is reported as `-c export ACCOUNT_ID=******`: the second half of the command disappears from the job response and from the run-job events, and `ACCOUNT_ID` was not a credential. It is the `--where` problem again, narrowed to `-c` instead of removed. Cheapest fix: mask a two-token conf payload only when it looks like a single conf entry - no whitespace in `payload` - which leaves a shell command intact, since a Spark `--conf` / `-c` value never contains a space. If you would rather treat shell `-c` as out of scope, dropping `-c` from `CONF_ARGUMENT_FLAGS` (spark-submit users can still write `--conf`) or saying so in the class javadoc is enough. Either way a test with a `/bin/sh -c` template is worth having. Verified by: extracted `maskSensitiveArguments` and its five helpers verbatim from this head into a standalone class with the default keyword set and ran them - `[-c, export ACCOUNT_ID=5 && run.sh --flag]` -> `[-c, export ACCOUNT_ID=******]` and `[-c, DB_PASSWORD=pw run.sh]` -> `[-c, DB_PASSWORD=******]`; read the shell branch at :137-150 and `TestLocalJobExecutor.java:709-717` on this head. ########## core/src/main/java/org/apache/gravitino/job/JobRuntimeTemplateMaskUtils.java: ########## @@ -0,0 +1,440 @@ +/* + * 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.gravitino.job; + +import com.google.common.collect.ImmutableSet; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; +import javax.annotation.Nullable; +import org.apache.gravitino.connector.HiddenPropertyMaskUtils; +import org.apache.gravitino.dto.job.JobTemplateDTO; +import org.apache.gravitino.dto.job.ShellJobTemplateDTO; +import org.apache.gravitino.dto.job.SparkJobTemplateDTO; +import org.apache.gravitino.secret.SecretPropertyUtils; + +/** + * Masks sensitive-named values on resolved runtime job templates before they are exposed on API or + * event paths. Stored runtime JSON is unchanged. + * + * <p>Argument masking is grouped by payload shape: + * + * <ul> + * <li>Nest JSON — {@link #NESTED_JSON_ARGUMENT_FLAGS} ({@code --updater-options} / {@code + * --spark-conf} / {@code --options}) + * <li>Spark conf property — {@link #CONF_ARGUMENT_FLAGS} ({@code --conf} / {@code -c}): a single + * {@code key=value} in either two-token or {@code --conf=key=value} form + * <li>JVM options string — {@link #JVM_OPTIONS_ARGUMENT_FLAGS} ({@code --driver-java-options} / + * {@code --executor-java-options}): a space-separated JVM string that may start with {@code + * -}; sensitive {@code -Dkey=value} tokens inside are masked + * <li>Sensitive-named long options — {@code --name} / {@code --name=value} when the name matches + * {@link SecretPropertyUtils#isSensitivePropertyKey} (the following token after {@code + * --name} is always replaced with {@link HiddenPropertyMaskUtils#MASKED_VALUE}, including + * non-secret values such as identifiers after {@code --use-credential-file}) + * <li>Standalone {@code -Dkey=value} when {@code key} is sensitive + * </ul> + * + * <p>Map fields {@code environments} / {@code customFields} / {@code configs} and runJob {@code + * jobConf} mask sensitive-named keys, plus nested JSON under {@link #NESTED_JSON_MAP_KEYS}. + * + * <p>Not masked (out of scope): resource URI fields ({@code executable}, {@code className}, {@code + * jars}, {@code files}, {@code archives}, Shell {@code scripts}); {@code --hiveconf}; conf property + * values that embed {@code -D} secrets under a non-sensitive conf key (for example {@code + * spark.driver.extraJavaOptions}); free-form tokens such as a {@code --where} SQL predicate. + * + * <p>Null map entries are preserved in the masked DTO / {@code jobConf} maps used on the REST and + * event {@code jobConf} paths. When {@code JobInfo} rebuilds a {@code JobTemplate} via {@code + * ImmutableMap.copyOf}, null values cannot be carried; a hand-edited runtime JSON with nulls may + * cause that rebuild to be skipped (logged) rather than expose a partial template. + */ +public final class JobRuntimeTemplateMaskUtils { + + /** + * JobConf / template keys whose values are known to be JSON maps (Iceberg built-in jobs and the + * optimizer). Nested sensitive keys inside these values are masked. + * + * <ul> + * <li>{@code updater_options} — update-stats job credentials / client options + * <li>{@code spark_conf} — Spark configs for built-in Iceberg jobs + * <li>{@code options} — rewrite-data-files / compaction options (from policy {@code jobOptions} + * / {@code rewriteOptions}) + * </ul> + */ + static final Set<String> NESTED_JSON_MAP_KEYS = + ImmutableSet.of("updater_options", "spark_conf", "options"); + + /** + * Iceberg built-in job CLI flags whose following argument is a JSON map with the same masking + * rules as {@link #NESTED_JSON_MAP_KEYS}. + */ + static final Set<String> NESTED_JSON_ARGUMENT_FLAGS = + ImmutableSet.of("--updater-options", "--spark-conf", "--options"); + + /** + * Spark flags whose payload is a single {@code key=value} config entry. Both the two-token form + * ({@code --conf key=value}) and the equals form ({@code --conf=key=value}) use {@link + * #maskConfKeyValuePayload}. + */ + static final Set<String> CONF_ARGUMENT_FLAGS = ImmutableSet.of("--conf", "-c"); + + /** + * Spark flags whose payload is a space-separated JVM options string (often starting with {@code + * -X} / {@code -D}). Both two-token and equals forms use {@link #maskJvmOptionsPayload}. + */ + static final Set<String> JVM_OPTIONS_ARGUMENT_FLAGS = + ImmutableSet.of("--driver-java-options", "--executor-java-options"); + + private JobRuntimeTemplateMaskUtils() {} + + /** + * Returns a copy of {@code jobConf} with sensitive-named values masked for event / audit paths. + * Nested JSON under {@link #NESTED_JSON_MAP_KEYS} is walked the same way as template maps. Does + * not mutate the input; callers must keep the original for job execution. + * + * @param jobConf runtime configuration from the runJob caller; may be null + * @return a masked mutable copy, or null when {@code jobConf} is null + */ + @Nullable + public static Map<String, String> maskSensitiveJobConf(@Nullable Map<String, String> jobConf) { + if (jobConf == null) { + return null; + } + return HiddenPropertyMaskUtils.maskSensitiveNamedProperties(jobConf, NESTED_JSON_MAP_KEYS); + } + + /** + * Returns a copy of {@code jobTemplateDTO} with sensitive map values masked, or {@code null} when + * the input is null. + * + * @param jobTemplateDTO the resolved runtime job template DTO + * @return a masked copy, or null + */ + @Nullable + public static JobTemplateDTO maskSensitiveFields(@Nullable JobTemplateDTO jobTemplateDTO) { + if (jobTemplateDTO == null) { + return null; + } + Map<String, String> maskedEnvironments = maskSensitiveMap(jobTemplateDTO.environments()); + Map<String, String> maskedCustomFields = maskSensitiveMap(jobTemplateDTO.customFields()); + + if (jobTemplateDTO instanceof ShellJobTemplateDTO) { + ShellJobTemplateDTO shell = (ShellJobTemplateDTO) jobTemplateDTO; + return ShellJobTemplateDTO.builder() + .withName(shell.name()) + .withComment(shell.comment()) + .withJobType(shell.jobType()) + .withExecutable(shell.executable()) + .withArguments(maskSensitiveArguments(shell.arguments())) + .withEnvironments(maskedEnvironments) + .withCustomFields(maskedCustomFields) + .withScripts(shell.scripts()) + .withAudit(shell.audit()) + .build(); + } + if (jobTemplateDTO instanceof SparkJobTemplateDTO) { + SparkJobTemplateDTO spark = (SparkJobTemplateDTO) jobTemplateDTO; + return SparkJobTemplateDTO.builder() + .withName(spark.name()) + .withComment(spark.comment()) + .withJobType(spark.jobType()) + .withExecutable(spark.executable()) + .withArguments(maskSensitiveArguments(spark.arguments())) + .withEnvironments(maskedEnvironments) + .withCustomFields(maskedCustomFields) + .withClassName(spark.className()) + .withJars(spark.jars()) + .withFiles(spark.files()) + .withArchives(spark.archives()) + .withConfigs(maskSensitiveMap(spark.configs())) + .withAudit(spark.audit()) + .build(); + } + throw new IllegalArgumentException( + "Unsupported job template type: " + jobTemplateDTO.jobType()); + } + + private static Map<String, String> maskSensitiveMap(Map<String, String> properties) { + return properties == null + ? null + : HiddenPropertyMaskUtils.maskSensitiveNamedProperties(properties, NESTED_JSON_MAP_KEYS); + } + + /** + * Masks nest-flag JSON values, sensitive-named long options, conf {@code key=value} payloads, JVM + * options strings, and standalone {@code -Dkey=value} tokens. Other tokens are left unchanged. + */ + @Nullable + private static List<String> maskSensitiveArguments(@Nullable List<String> arguments) { + if (arguments == null) { + return null; + } + if (arguments.isEmpty()) { + return arguments; + } + List<String> result = new ArrayList<>(arguments.size()); + boolean maskNextAsNestedJson = false; + boolean maskNextAsSecret = false; + boolean maskNextAsConfPayload = false; + boolean maskNextAsJvmOptions = false; + for (String arg : arguments) { + if (maskNextAsNestedJson) { + maskNextAsNestedJson = false; + if (arg != null && !arg.startsWith("--")) { + result.add(HiddenPropertyMaskUtils.maskSensitiveJsonValue(arg)); + continue; + } + // Missing value for the previous nest flag; this token may itself be a flag. + } + if (maskNextAsSecret) { + maskNextAsSecret = false; + if (arg != null && !arg.startsWith("--")) { + result.add(HiddenPropertyMaskUtils.MASKED_VALUE); + continue; + } + // Missing value for the previous sensitive flag; this token may itself be a flag. + } + if (maskNextAsConfPayload) { + maskNextAsConfPayload = false; + if (arg != null && !arg.startsWith("-")) { + result.add(maskConfKeyValuePayload(arg)); + continue; + } + // Missing conf value or next token is another flag; fall through. + } + if (maskNextAsJvmOptions) { + maskNextAsJvmOptions = false; + if (arg != null && !isKnownStandaloneFlag(arg)) { + // JVM strings often start with '-' (e.g. -Xmx / -D); only skip known flags. + result.add(maskJvmOptionsPayload(arg)); + continue; + } + // Missing JVM options value; this token may itself be a flag. + } + + String nestEquals = maskEqualsNestFlagArgument(arg); + if (nestEquals != null) { + result.add(nestEquals); + continue; + } + String confEquals = maskEqualsConfFlagArgument(arg); + if (confEquals != null) { + result.add(confEquals); + continue; + } + String jvmEquals = maskEqualsJvmOptionsFlagArgument(arg); + if (jvmEquals != null) { + result.add(jvmEquals); + continue; + } + String sensitiveEquals = maskEqualsSensitiveFlagArgument(arg); + if (sensitiveEquals != null) { + result.add(sensitiveEquals); + continue; + } + String dashD = maskSensitiveDashDArgument(arg); + if (dashD != null) { + result.add(dashD); + continue; + } + + result.add(arg); + if (arg != null && NESTED_JSON_ARGUMENT_FLAGS.contains(arg)) { + maskNextAsNestedJson = true; + } else if (arg != null && CONF_ARGUMENT_FLAGS.contains(arg)) { + maskNextAsConfPayload = true; + } else if (arg != null && JVM_OPTIONS_ARGUMENT_FLAGS.contains(arg)) { + maskNextAsJvmOptions = true; + } else if (isSensitiveLongOptionFlag(arg)) { + maskNextAsSecret = true; + } + } + return result; + } + + /** + * Whether {@code arg} is a known flag that must not be consumed as a JVM-options payload when it + * immediately follows {@code --driver-java-options} / {@code --executor-java-options}. + */ + private static boolean isKnownStandaloneFlag(@Nullable String arg) { + if (arg == null) { + return false; + } + if (NESTED_JSON_ARGUMENT_FLAGS.contains(arg) + || CONF_ARGUMENT_FLAGS.contains(arg) + || JVM_OPTIONS_ARGUMENT_FLAGS.contains(arg)) { + return true; + } + // Other long options (including non-sensitive ones such as --catalog). + return arg.startsWith("--") && arg.length() > 2 && arg.indexOf('=') < 0; Review Comment: [Nit] A nest or conf flag in equals form immediately after `--driver-java-options` is consumed as a JVM options string, so its JSON comes back in clear text. This line treats any token containing `=` as a payload rather than a flag - right for a real JVM option such as `--add-opens=java.base/sun.nio.ch=ALL-UNNAMED`, but it also catches `--options={...}` and `--conf=key=value`, and `maskJvmOptionsPayload` (:370-389) only masks `-D` tokens inside the string. So `["--driver-java-options", "--options={\"jdbc.password\":\"pw\"}"]` returns the nested secret unmasked, while the same token after `--conf` falls through to `maskEqualsNestFlagArgument` and is masked. It needs a template with a missing JVM payload, so I read this as a consistency gap rather than a realistic leak. If you want it closed: in `isKnownStandaloneFlag`, also return true when `arg` starts with a known flag prefix plus `=` (the three flag sets), and the existing equals-form branches below will handle the token. Verified by: the same verbatim-extracted harness - `[--driver-java-options, --options={"jdbc.password":"pw"}]` came back unchanged, whereas `[--conf, --options={"jdbc.password":"pw"}]` had the JSON masked. -- 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]
