lasdf1234 commented on code in PR #13858: URL: https://github.com/apache/gravitino/pull/13858#discussion_r4281041349
########## 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: Fixed: `maskSensitiveArguments` now masks `--name` / `--name=value` when `isSensitivePropertyKey(name)` matches. Also covered similar forms that showed up in the same path: bare `key=value` (e.g. after `--conf`) and `-Dkey=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()) Review Comment: Treating resource URIs as out of scope for this PR. Documented on `JobRuntimeTemplateMaskUtils` / `JobInfo` that `executable` / `jars` / `files` / `archives` / `scripts` / `className` are not masked. ########## 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: Fixed: `maskNestedJsonValue` now returns the original value when the trimmed string does not start with `{` or `[` (e.g. `--options=verbose`). Object/array-looking malformed JSON is still masked. ########## 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: Documented: null preservation applies to REST / raw-map paths; `JobInfo` → `JobTemplate` via `ImmutableMap.copyOf` still cannot carry nulls. -- 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]
