jerryshao commented on code in PR #13858:
URL: https://github.com/apache/gravitino/pull/13858#discussion_r4280732085
##########
core/src/main/java/org/apache/gravitino/connector/HiddenPropertyMaskUtils.java:
##########
@@ -176,4 +182,179 @@ public static Map<String, String> maskHiddenProperties(
Map<String, String> properties, PropertiesMetadata metadata) {
return maskHiddenProperties(properties,
classifyHiddenProperties(properties, metadata));
}
+
+ /**
+ * Returns a mutable copy of {@code properties} with sensitive-looking
values replaced by {@link
+ * #MASKED_VALUE}, without requiring {@link PropertiesMetadata}. Equivalent
to {@link
+ * #maskSensitiveNamedProperties(Map, Set)} with no nested JSON keys.
+ *
+ * @param properties source map, may be null
+ * @return a mutable masked copy; never null
+ */
+ public static Map<String, String> maskSensitiveNamedProperties(
+ @Nullable Map<String, String> properties) {
+ return maskSensitiveNamedProperties(properties, null);
+ }
+
+ /**
+ * Returns a mutable copy of {@code properties} with sensitive-looking
values replaced by {@link
+ * #MASKED_VALUE}, without requiring {@link PropertiesMetadata}.
+ *
+ * <p>A top-level entry is masked when its key matches {@link
+ * SecretPropertyUtils#isSensitivePropertyKey} or its value is a
secret-manager URN ({@link
+ * SecretPropertyUtils#isSecretProperty}). When {@code nestedJsonMapKeys} is
non-null, values for
+ * those keys are parsed as JSON and sensitive-named keys are masked at any
nesting depth (objects
+ * and arrays). Other values are never parsed as JSON. Blank nested values
are left unchanged
+ * (they carry no secret). On parse failure for a non-blank nested JSON key,
the entire value is
+ * replaced with {@link #MASKED_VALUE} so secrets are not left in a broken
blob.
+ *
+ * <p>Null values are preserved (key kept, value {@code null}) so response
maps do not drop keys
+ * that were present in the source. Null keys are skipped.
+ *
+ * <p>Useful for job template {@code environments} / {@code customFields} /
{@code configs} that
+ * have no properties metadata declaration. Callers that know job-specific
JSON-map keys (for
+ * example Iceberg built-in jobs) should pass them via {@code
nestedJsonMapKeys}.
+ *
+ * @param properties source map, may be null
+ * @param nestedJsonMapKeys keys whose string values are JSON maps/objects
to walk; may be null
+ * @return a mutable masked copy; never null
+ */
+ public static Map<String, String> maskSensitiveNamedProperties(
+ @Nullable Map<String, String> properties, @Nullable Set<String>
nestedJsonMapKeys) {
+ if (properties == null || properties.isEmpty()) {
+ return new HashMap<>();
+ }
+ Set<String> nestedKeys = nestedJsonMapKeys == null ?
Collections.emptySet() : nestedJsonMapKeys;
+ Map<String, String> result = new LinkedHashMap<>(properties.size());
+ for (Map.Entry<String, String> entry : properties.entrySet()) {
+ String key = entry.getKey();
+ String value = entry.getValue();
+ if (key == null) {
+ continue;
+ }
+ if (value == null) {
+ result.put(key, null);
+ continue;
+ }
+ if (SecretPropertyUtils.isSensitivePropertyKey(key)
+ || SecretPropertyUtils.isSecretProperty(key, value)) {
+ result.put(key, MASKED_VALUE);
+ } else if (nestedKeys.contains(key)) {
+ result.put(key, maskNestedJsonValue(value));
+ } else {
+ result.put(key, value);
+ }
+ }
+ return result;
+ }
+
+ /**
+ * Parses {@code value} as JSON and masks sensitive-named keys in objects
(recursively through
+ * nested objects and arrays). Blank strings are returned unchanged.
Malformed non-blank JSON is
+ * replaced with {@link #MASKED_VALUE}. Scalar JSON roots are returned
unchanged.
+ *
+ * <p>Useful when a caller already knows a string is a nested JSON map (for
example a CLI argument
+ * following {@code --spark-conf}).
+ *
+ * @param value JSON string to mask; may be null
+ * @return masked JSON, {@link #MASKED_VALUE} on parse failure, blank/null
when {@code value} is
+ * blank/null
+ */
+ @Nullable
+ public static String maskSensitiveJsonValue(@Nullable String value) {
+ if (value == null) {
+ return null;
+ }
+ return maskNestedJsonValue(value);
+ }
+
+ /**
+ * Parses {@code value} as JSON and masks sensitive-named keys in objects
(recursively through
+ * nested objects and arrays). Blank strings are returned unchanged.
Malformed non-blank JSON is
+ * replaced with {@link #MASKED_VALUE}.
+ */
+ private static String maskNestedJsonValue(String value) {
+ // Empty / whitespace-only values carry no secret (common for unresolved
{{spark_conf:-}}).
+ if (StringUtils.isBlank(value)) {
+ return value;
+ }
+ ObjectMapper mapper = JsonUtils.anyFieldMapper();
+ try {
+ Object root = mapper.readValue(value, Object.class);
+ if (!(root instanceof Map) && !(root instanceof List)) {
+ // Scalar JSON (string/number/boolean/null) — nothing to walk by key
name.
+ return value;
+ }
+ MaskedJson masked = maskJsonNode(root);
+ return masked.changed ? mapper.writeValueAsString(masked.value) : value;
+ } catch (Exception e) {
+ // Malformed JSON on a caller-declared nest key — do not leave a
possible secret blob as-is.
+ return MASKED_VALUE;
Review Comment:
[Nit] Mask-on-parse-failure hides plain non-JSON values under these three
generic key names, and it is inconsistent with the scalar branch just above.
`options`, `spark_conf` and `updater_options` are declared nest keys for any
`environments` / `customFields` / `configs` map and for the `--options` /
`--spark-conf` / `--updater-options` flags, including a hand-written Shell or
Spark template that is not an Iceberg built-in. A value such as `verbose` is
not valid JSON, so Jackson throws and this catch replaces it with `******`,
while `:285-287` returns valid scalar JSON (`5`, `true`, `"text"`) unchanged. A
user who passed `--options=verbose` sees `--options=******` in the response and
cannot tell what the job actually ran, even though nothing was secret.
Treating a value as a possible secret blob only when it looks like one -
trimmed value starts with `{` or `[` - and otherwise returning it unchanged
would keep the protective behaviour for real (possibly truncated) JSON maps and
remove the surprise for scalars. A bare word under a non-sensitive key is not
masked anywhere else on this path, so leaving it unchanged does not open a gap.
Verified by: read `maskNestedJsonValue` :275-293 on this head; the blank
guard at :277-279 and the scalar return at :285-287 are the only non-masking
exits, so a bare word reaches this catch (Jackson rejects an unquoted token as
JSON).
##########
core/src/main/java/org/apache/gravitino/job/JobRuntimeTemplateMaskUtils.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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;
+
+/**
+ * Masks sensitive-named values on resolved runtime job templates before they
are exposed on API or
+ * event paths. Stored runtime JSON is unchanged.
+ *
+ * <p>Nested JSON string maps are masked for the Iceberg built-in / optimizer
keys listed in {@link
+ * #NESTED_JSON_MAP_KEYS} on {@code environments} / {@code customFields} /
{@code configs} / runJob
+ * {@code jobConf}, and for the matching CLI flags in {@link
#NESTED_JSON_ARGUMENT_FLAGS} on {@code
+ * arguments} (the value token after the flag, or the {@code --flag=json}
form). Other argument
+ * tokens are left unchanged; do not put secrets in free-form arguments.
+ */
+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");
+
+ 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())
Review Comment:
[Question] `jars` / `files` / `archives` (and `executable`, `className`,
Shell `scripts`) are copied through unmasked, and they are resolved from
`jobConf` too. Is a credential-bearing URI considered out of scope here?
`JobTemplateResolver.resolve` substitutes placeholders into every one of
them: `executable` at
`core/src/main/java/org/apache/gravitino/job/JobTemplateResolver.java:132`,
`jars` / `files` / `archives` at :158-160, `scripts` at :145, `className` at
:157. So a template with `jars: ["{{app_jar}}"]` run with a presigned URL
(`...?X-Amz-Credential=...&X-Amz-Signature=...`) or a
`scheme://user:password@host/...` URI returns that URI in clear text in
`runtimeJobTemplate`.
I am not proposing to mask whole URIs - that would make the response
useless. If you want to close it, masking only query-parameter values whose
name matches `isSensitivePropertyKey`, plus the userinfo part of the URI, would
cover the realistic cases. If you consider it out of scope, a one-line note in
the class javadoc saying resource URIs are not masked would set expectations.
Verified by: read `JobTemplateResolver.resolve` :121-165 on this head for
the resolution of each field, and this builder branch :111-130 - `jars` /
`files` / `archives` / `executable` / `className` are passed straight from the
source DTO.
##########
core/src/main/java/org/apache/gravitino/connector/HiddenPropertyMaskUtils.java:
##########
@@ -176,4 +182,179 @@ public static Map<String, String> maskHiddenProperties(
Map<String, String> properties, PropertiesMetadata metadata) {
return maskHiddenProperties(properties,
classifyHiddenProperties(properties, metadata));
}
+
+ /**
+ * Returns a mutable copy of {@code properties} with sensitive-looking
values replaced by {@link
+ * #MASKED_VALUE}, without requiring {@link PropertiesMetadata}. Equivalent
to {@link
+ * #maskSensitiveNamedProperties(Map, Set)} with no nested JSON keys.
+ *
+ * @param properties source map, may be null
+ * @return a mutable masked copy; never null
+ */
+ public static Map<String, String> maskSensitiveNamedProperties(
+ @Nullable Map<String, String> properties) {
+ return maskSensitiveNamedProperties(properties, null);
+ }
+
+ /**
+ * Returns a mutable copy of {@code properties} with sensitive-looking
values replaced by {@link
+ * #MASKED_VALUE}, without requiring {@link PropertiesMetadata}.
+ *
+ * <p>A top-level entry is masked when its key matches {@link
+ * SecretPropertyUtils#isSensitivePropertyKey} or its value is a
secret-manager URN ({@link
+ * SecretPropertyUtils#isSecretProperty}). When {@code nestedJsonMapKeys} is
non-null, values for
+ * those keys are parsed as JSON and sensitive-named keys are masked at any
nesting depth (objects
+ * and arrays). Other values are never parsed as JSON. Blank nested values
are left unchanged
+ * (they carry no secret). On parse failure for a non-blank nested JSON key,
the entire value is
+ * replaced with {@link #MASKED_VALUE} so secrets are not left in a broken
blob.
+ *
+ * <p>Null values are preserved (key kept, value {@code null}) so response
maps do not drop keys
+ * that were present in the source. Null keys are skipped.
+ *
+ * <p>Useful for job template {@code environments} / {@code customFields} /
{@code configs} that
+ * have no properties metadata declaration. Callers that know job-specific
JSON-map keys (for
+ * example Iceberg built-in jobs) should pass them via {@code
nestedJsonMapKeys}.
+ *
+ * @param properties source map, may be null
+ * @param nestedJsonMapKeys keys whose string values are JSON maps/objects
to walk; may be null
+ * @return a mutable masked copy; never null
+ */
+ public static Map<String, String> maskSensitiveNamedProperties(
+ @Nullable Map<String, String> properties, @Nullable Set<String>
nestedJsonMapKeys) {
+ if (properties == null || properties.isEmpty()) {
+ return new HashMap<>();
+ }
+ Set<String> nestedKeys = nestedJsonMapKeys == null ?
Collections.emptySet() : nestedJsonMapKeys;
+ Map<String, String> result = new LinkedHashMap<>(properties.size());
+ for (Map.Entry<String, String> entry : properties.entrySet()) {
+ String key = entry.getKey();
+ String value = entry.getValue();
+ if (key == null) {
+ continue;
+ }
+ if (value == null) {
+ result.put(key, null);
Review Comment:
[Nit] Null preservation takes effect on the REST response only; on the event
path a preserved null still loses the whole runtime template, so the javadoc
claim at :211-212 is half true.
`JobInfo.toRuntimeJobTemplate` converts the masked DTO back with
`DTOConverters.fromDTO`, and the `JobTemplate` constructor does
`ImmutableMap.copyOf(environments)` / `copyOf(customFields)` and
`ImmutableList.copyOf(arguments)`
(`api/src/main/java/org/apache/gravitino/job/JobTemplate.java:333-337`;
`SparkJobTemplate.java:295` for `configs`), which reject a null value. The NPE
is caught by the `catch (Exception e)` in `JobInfo.java:106-116`, so the
listener gets a `JobInfo` with no runtime template at all plus a warning -
whereas before this commit the entry was dropped and the template survived.
Not worth code on its own: a stored runtime JSON is serialized from a
`JobTemplate` whose maps are already `ImmutableMap`, so it cannot contain a
null value unless the row was edited by hand, which is why I am filing this as
a nit rather than a bug. Either narrow the javadoc to the API path, or drop
nulls again in `JobRuntimeTemplateMaskUtils.maskSensitiveMap` only -
`maskSensitiveJobConf` and the REST path are the cases where keeping them helps.
Verified by: read `JobTemplate.java:325-340` and
`SparkJobTemplate.java:288-296` for the `copyOf` calls, and
`JobInfo.java:93-118` for the swallow; `ImmutableMap.copyOf` throws NPE on a
null value.
##########
core/src/main/java/org/apache/gravitino/job/JobRuntimeTemplateMaskUtils.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * 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;
+
+/**
+ * Masks sensitive-named values on resolved runtime job templates before they
are exposed on API or
+ * event paths. Stored runtime JSON is unchanged.
+ *
+ * <p>Nested JSON string maps are masked for the Iceberg built-in / optimizer
keys listed in {@link
+ * #NESTED_JSON_MAP_KEYS} on {@code environments} / {@code customFields} /
{@code configs} / runJob
+ * {@code jobConf}, and for the matching CLI flags in {@link
#NESTED_JSON_ARGUMENT_FLAGS} on {@code
+ * arguments} (the value token after the flag, or the {@code --flag=json}
form). Other argument
+ * tokens are left unchanged; do not put secrets in free-form arguments.
+ */
+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");
+
+ 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 the value token after known Iceberg JSON-map CLI flags, including
the single-token {@code
+ * --flag=json} form. Other tokens (including free-form strings that happen
to look like JSON) 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 maskNext = false;
+ for (String arg : arguments) {
+ if (maskNext) {
+ maskNext = false;
+ if (arg != null && !arg.startsWith("--")) {
+ result.add(HiddenPropertyMaskUtils.maskSensitiveJsonValue(arg));
+ continue;
+ }
+ // Missing value for the previous flag; this token may itself be a
nest flag.
+ }
+ String equalsForm = maskEqualsNestFlagArgument(arg);
+ if (equalsForm != null) {
+ result.add(equalsForm);
+ continue;
+ }
+ result.add(arg);
+ if (arg != null && NESTED_JSON_ARGUMENT_FLAGS.contains(arg)) {
Review Comment:
[Important] A sensitive-named CLI flag in `arguments` still returns its
value in clear text, and that value also comes from `jobConf`.
`arguments` are placeholder-resolved exactly like the maps this PR masks:
`JobTemplateResolver.resolve` does `List<String> args =
resolveList(content.arguments(), resolver)`
(`core/src/main/java/org/apache/gravitino/job/JobTemplateResolver.java:133`)
with the same `jobConf`-backed resolver used for `environments` /
`customFields` / `configs` (:134, :143-145). So a template whose arguments are
`["--password", "{{db_password}}"]` or `["--access-key", "{{ak}}"]` puts the
caller's plaintext secret into `runtimeJobTemplate.arguments`, and
`maskSensitiveArguments` only matches `NESTED_JSON_ARGUMENT_FLAGS` (this line),
so it is returned verbatim on job get/list/run/cancel and on the event path -
the exact symptom of #13857, just in a different field.
The helper for this already exists and needs no new keyword list: when a
token matches `--<name>` (or `--<name>=<value>`) and
`SecretPropertyUtils.isSensitivePropertyKey(name)` is true, mask the value
token (or the part after `=`). `isSensitivePropertyKey` lower-cases and
substring-matches, so `--password`, `--db-token`, `--access-key` all match with
the default keywords.
If you would rather keep this PR to the fields named in the issue, that is
defensible - the two previously blocking items are resolved, so this need not
hold the PR - but then please open a follow-up issue and say so in the class
javadoc, because the current wording ("do not put secrets in free-form
arguments") reads as advice to users rather than as a known hole in the fix.
Verified by: read `maskSensitiveArguments` at :146-173 on this head - the
only flag match is `NESTED_JSON_ARGUMENT_FLAGS.contains(arg)` plus the
`--flag=` prefix check at :180-193; read `JobTemplateResolver.resolve` :121-165
for the argument resolution path, and
`SecretPropertyUtils.isSensitivePropertyKey` :78-83 for the substring matching.
--
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]
