jerryshao commented on code in PR #13858:
URL: https://github.com/apache/gravitino/pull/13858#discussion_r4280280803
##########
core/src/main/java/org/apache/gravitino/connector/HiddenPropertyMaskUtils.java:
##########
@@ -176,4 +181,165 @@ 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. On parse failure for
a nested JSON key, the
+ * entire value is replaced with {@link #MASKED_VALUE} so secrets are not
left in a broken blob.
+ *
+ * <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 || value == 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). Malformed 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, or null when
{@code value} is 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). Malformed JSON is replaced with {@link
#MASKED_VALUE}.
+ */
+ private static String maskNestedJsonValue(String 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:
[Important] A blank value under a declared nest key is reported as `******`,
so an option that was never set looks like a hidden secret.
`maskNestedJsonValue` treats every parse failure as a secret blob, and
Jackson fails on an empty or whitespace-only string (`No content to map due to
end-of-input`). That is the normal resolved shape for the built-in Iceberg
jobs: `buildArguments()` emits `--spark-conf`, `{{spark_conf:-}}`
(`maintenance/jobs/.../IcebergRewriteDataFilesJob.java:388-391`,
`IcebergUpdateStatsAndMetricsJob.java:561-565`), and
`JobTemplatePlaceholderUtils.replacePlaceholders` substitutes the empty default
when jobConf has no `spark_conf` / `options` / `updater_options` key, so the
resolved token is `""`. `JobRuntimeTemplateMaskUtils.java:142` hands that `""`
to this method, and the API response / event for a job that had no Spark conf
at all comes back as `--spark-conf ******`. The same applies to an `options` /
`spark_conf` / `updater_options` entry whose value is empty in `environments`,
`customFields` or `configs`.
Suggested fix: return `value` unchanged when it is blank (e.g.
`StringUtils.isBlank(value)`) before parsing - a blank string carries no secret
- and keep the mask-on-failure behaviour for genuinely malformed JSON. Please
add a test for a nest flag followed by an empty token.
Verified by: compiled and ran `new ObjectMapper().readValue(v,
Object.class)` on Jackson 2.16.1 for v = `""`, `" "`, `"abc"` - all three
throw; read `JobTemplatePlaceholderUtils.java:148-184` for the empty-default
substitution and the two `buildArguments()` methods above on this head.
##########
core/src/main/java/org/apache/gravitino/listener/api/info/JobInfo.java:
##########
@@ -89,18 +90,20 @@ public static JobInfo fromJobEntity(JobEntity jobEntity) {
/**
* Deserializes the job entity's stored runtime job template JSON, if any,
back into a {@link
- * JobTemplate}. This is called from inside the event dispatcher's try block
for getJob/runJob/
- * cancelJob, after the underlying operation has already succeeded (or, for
cancelJob, after the
- * external cancel call and entity update have already happened) - a
malformed or
- * forward-incompatible stored value (e.g. a job type unknown to this server
version) must not
- * turn that already-completed operation into a 500, so failures here are
logged and swallowed
- * rather than propagated.
+ * JobTemplate}. Sensitive-named map values are masked via {@link
JobRuntimeTemplateMaskUtils}
+ * before the template is exposed on the event path. This is called from
inside the event
+ * dispatcher's try block for getJob/runJob/cancelJob, after the underlying
operation has already
+ * succeeded (or, for cancelJob, after the external cancel call and entity
update have already
+ * happened) - a malformed or forward-incompatible stored value (e.g. a job
type unknown to this
+ * server version) must not turn that already-completed operation into a
500, so failures here are
+ * logged and swallowed rather than propagated.
*/
private static JobTemplate toRuntimeJobTemplate(JobEntity jobEntity) {
try {
JobTemplateDTO runtimeJobTemplateDTO =
- DTOConverters.fromRuntimeJobTemplateJson(
- jobEntity.runtimeJobTemplate(), jobEntity.name());
+ JobRuntimeTemplateMaskUtils.maskSensitiveFields(
+ DTOConverters.fromRuntimeJobTemplateJson(
+ jobEntity.runtimeJobTemplate(), jobEntity.name()));
Review Comment:
[Important] The event path is only half masked: listeners still get the
plaintext `jobConf` the secrets came from.
`JobEventDispatcher.runJob` passes the caller's `jobConf` map straight into
`RunJobPreEvent`, `RunJobEvent` and `RunJobFailureEvent`
(`core/src/main/java/org/apache/gravitino/listener/JobEventDispatcher.java:227-244`),
and `RunJobEvent.jobConf()`
(`core/src/main/java/org/apache/gravitino/listener/api/event/job/RunJobEvent.java:81-83`)
returns it unchanged. That map is exactly where the plaintext lives: the
`password` entry and the `updater_options` / `spark_conf` JSON blobs this PR
masks in the template. Any audit listener that serializes the event still
writes the credentials, to the same consumer that now receives a masked
`JobInfo` - so the issue the PR set out to fix stays open on the run path.
The leak is pre-existing rather than introduced here, so either mask those
three events with the same helper in this PR, or narrow the description (it
currently claims event-path coverage) and open a follow-up issue for `jobConf`.
Verified by: read `JobEventDispatcher.java:224-247` and
`RunJobEvent.java:32-83` on this head; neither file is in this PR's diff.
##########
core/src/test/java/org/apache/gravitino/connector/TestHiddenPropertyMaskUtils.java:
##########
@@ -203,4 +205,119 @@ public Map<String, PropertyEntry<?>> propertyEntries() {
Assertions.assertEquals("root", masked.get("jdbc-user"));
Assertions.assertEquals(HiddenPropertyMaskUtils.MASKED_VALUE,
masked.get("jdbc-passwrod"));
}
+
+ @Test
+ void testMaskSensitiveNamedPropertiesMasksKeywordKeysAndKnownNestedJson()
throws Exception {
+ Map<String, String> properties = new java.util.LinkedHashMap<>();
Review Comment:
[Nit] `new java.util.LinkedHashMap<>()` uses a fully-qualified class name.
CLAUDE.md asks for a normal `import java.util.LinkedHashMap;` unless there is a
real name conflict, and there is none in this file.
Verified by: read the Imports rule in CLAUDE.md and this line on the PR head.
##########
core/src/main/java/org/apache/gravitino/connector/HiddenPropertyMaskUtils.java:
##########
@@ -176,4 +181,165 @@ 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. On parse failure for
a nested JSON key, the
+ * entire value is replaced with {@link #MASKED_VALUE} so secrets are not
left in a broken blob.
+ *
+ * <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 || value == null) {
+ continue;
+ }
Review Comment:
[Question] Entries with a null value are dropped from the result instead of
being copied. For `environments` / `customFields` deserialized from stored
runtime JSON, a `{"K":null}` entry used to come back as `K: null` and now
disappears from the response entirely. Is that intended on this path?
`maskHiddenProperties` (:151-158) has the same rule, so it may be deliberate
symmetry - flagging it because it is a response change beyond masking, and a
disappearing key is harder to explain to a client than a null one.
Verified by: read `maskSensitiveNamedProperties` at :217-240 and compared
with `maskHiddenProperties` at :141-160; `JobTemplateDTO` declares
`environments` / `customFields` as plain `Map<String,String>` with no null
filtering
(`common/src/main/java/org/apache/gravitino/dto/job/JobTemplateDTO.java:79-83`).
##########
core/src/main/java/org/apache/gravitino/job/JobRuntimeTemplateMaskUtils.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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}, and for
+ * the matching CLI flags in {@link #NESTED_JSON_ARGUMENT_FLAGS} on {@code
arguments} (the value
+ * token immediately after the flag). 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 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. 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.
+ }
+ result.add(arg);
+ if (arg != null && NESTED_JSON_ARGUMENT_FLAGS.contains(arg)) {
Review Comment:
[Nit] Only the two-token form of the nest flags is recognized, so a
hand-written template passing `--spark-conf={"jdbc.password":"..."}` as a
single token keeps its nested secrets in clear text. The built-in templates
always emit two tokens, so this is not a leak for them, but a `startsWith(flag
+ "=")` branch would close the obvious variant cheaply.
Verified by: read `maskSensitiveArguments` at :129-153 - the only match is
`NESTED_JSON_ARGUMENT_FLAGS.contains(arg)`; checked that every built-in
`buildArguments()` uses separate flag and value tokens.
--
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]
