lasdf1234 commented on code in PR #13858:
URL: https://github.com/apache/gravitino/pull/13858#discussion_r4280454484
##########
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:
Fixed: blank / whitespace nest values are returned unchanged before JSON
parse; added unit tests for empty nest tokens.
##########
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:
Went with option 1: `JobEventDispatcher.runJob` now passes a masked
`jobConf` to `RunJobPreEvent` / `RunJobEvent` / `RunJobFailureEvent`, while the
dispatcher still executes with the caller's plaintext map.
##########
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:
Preserving null entries now (`key` kept, value `null`) so clients do not see
keys disappear.
##########
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:
Added `--flag=json` handling in `maskSensitiveArguments`.
##########
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:
Switched to a normal `LinkedHashMap` import.
--
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]
