diff --git a/src/main/java/io/kestra/plugin/soda/AbstractSoda.java b/src/main/java/io/kestra/plugin/soda/AbstractSoda.java index cbe3b05..96bcc34 100644 --- a/src/main/java/io/kestra/plugin/soda/AbstractSoda.java +++ b/src/main/java/io/kestra/plugin/soda/AbstractSoda.java @@ -8,6 +8,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; import com.fasterxml.jackson.annotation.JsonSetter; import com.fasterxml.jackson.databind.ObjectMapper; @@ -185,14 +186,30 @@ private String virtualEnvCommand(RunContext runContext, Path workingDirectory, L renderer.add("python -m venv --system-site-packages " + workingDirectory + " > /dev/null"); if (requirements != null) { + String installArgs = requirements.stream() + .map(AbstractSoda::shellQuote) + .collect(Collectors.joining(" ")); + renderer.addAll( Arrays.asList( "./bin/pip install pip --upgrade > /dev/null", - "./bin/pip install " + runContext.render(String.join(" ", requirements)) + " > /dev/null" + "./bin/pip install " + installArgs + " > /dev/null" ) ); } return String.join("\n", renderer); } + + /** + * Wraps a value in single quotes for safe interpolation into a {@code /bin/sh -c} command line. + * Inside single quotes the shell treats every character literally, so all metacharacters + * ({@code ; & | > < * $ ` ( )} …) are neutralized while any valid {@code requirements.txt} syntax + * (version specifiers, extras, VCS URLs, environment markers) is preserved verbatim. The only + * character that cannot appear inside single quotes — the single quote itself — is escaped with + * the standard {@code '\''} sequence (close quote, escaped quote, reopen quote). + */ + private static String shellQuote(String value) { + return "'" + value.replace("'", "'\\''") + "'"; + } } diff --git a/src/main/java/io/kestra/plugin/soda/Scan.java b/src/main/java/io/kestra/plugin/soda/Scan.java index 1907063..2329707 100644 --- a/src/main/java/io/kestra/plugin/soda/Scan.java +++ b/src/main/java/io/kestra/plugin/soda/Scan.java @@ -2,8 +2,12 @@ import java.io.IOException; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import io.kestra.core.exceptions.IllegalVariableEvaluationException; import io.kestra.core.models.annotations.Example; @@ -103,6 +107,25 @@ } ) public class Scan extends AbstractSoda implements RunnableTask { + private static final String REDACTED = "******"; + + /** + * Sensitive fragments matched as substrings of the normalized (alphanumeric-only, lowercased) + * key. Substring matching deliberately errs toward over-redaction — any key merely containing + * one of these (e.g. {@code keyfile}, {@code keyspace}, {@code client_secret}) is redacted — so + * that a secret is never leaked into task Output at the cost of occasionally masking a benign + * value. {@code key} already covers {@code api_key}, {@code access_key}, {@code private_key}, etc. + */ + private static final Set SENSITIVE_KEY_PATTERNS = Set.of( + "password", "passwd", "pwd", + "secret", + "token", + "key", + "credential", + "accountinfojson", + "auth" + ); + @Schema( title = "SodaCL checks definition", description = "Required map rendered to `checks.yml` and executed against the `kestra` data source. Follow SodaCL syntax; failing checks mark the task accordingly." @@ -179,11 +202,70 @@ public Scan.Output run(RunContext runContext) throws Exception { .result(scanResult) .stdOutLineCount(output.getStdOutLineCount()) .stdErrLineCount(output.getStdOutLineCount()) - .configuration(runContext.render(configuration).asMap(String.class, Object.class)) + .configuration(scrubSensitiveValues(runContext.render(configuration).asMap(String.class, Object.class))) .exitCode((Integer) output.getVars().get("exitCode")) .build(); } + /** + * Recursively scrubs sensitive leaf values (passwords, tokens, keys, credentials, etc.) from a + * rendered configuration map before it is stored in task Output, which is persisted in execution + * state and visible to any user with read access to the execution. + */ + @SuppressWarnings("unchecked") + private static Map scrubSensitiveValues(Map map) { + Map scrubbed = new LinkedHashMap<>(); + + for (Map.Entry entry : map.entrySet()) { + String key = entry.getKey(); + Object value = entry.getValue(); + + if (isSensitiveKey(key)) { + scrubbed.put(key, REDACTED); + } else { + scrubbed.put(key, scrubValue(value)); + } + } + + return scrubbed; + } + + /** + * Recurses through container values so sensitive keys nested inside maps or lists + * (e.g. a list of connection maps) are scrubbed too; scalar values are returned unchanged. + */ + @SuppressWarnings("unchecked") + private static Object scrubValue(Object value) { + if (value instanceof Map) { + return scrubSensitiveValues((Map) value); + } + + if (value instanceof List) { + List scrubbedList = new ArrayList<>(); + for (Object element : (List) value) { + scrubbedList.add(scrubValue(element)); + } + return scrubbedList; + } + + return value; + } + + private static boolean isSensitiveKey(String key) { + if (key == null) { + return false; + } + + String normalized = key.toLowerCase().replaceAll("[^a-z0-9]", ""); + for (String pattern : SENSITIVE_KEY_PATTERNS) { + if (normalized.contains(pattern)) { + return true; + } + } + + return false; + } + protected ScanResult parseResult(RunContext runContext, ScriptOutput output) throws IOException { ScanResult scanResult = JacksonMapper.ofJson(false).readValue( runContext.storage().getFile(output.getOutputFiles().get("result.json")), diff --git a/src/test/java/io/kestra/plugin/soda/SecurityHardeningTest.java b/src/test/java/io/kestra/plugin/soda/SecurityHardeningTest.java new file mode 100644 index 0000000..3a578cc --- /dev/null +++ b/src/test/java/io/kestra/plugin/soda/SecurityHardeningTest.java @@ -0,0 +1,167 @@ +package io.kestra.plugin.soda; + +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.*; + +/** + * Fast, dependency-free unit tests for the security-hardening helpers introduced in the + * 2026-06-30 audit remediation PR: shell-quoting of pip requirements ({@link AbstractSoda}) + * and sensitive-value scrubbing of the rendered configuration ({@link Scan}). Both helpers are + * {@code private static} pure functions, so they are exercised directly via reflection rather than + * through the Docker/BigQuery integration path used by {@code ScanTest}. + */ +class SecurityHardeningTest { + + private static String shellQuote(String value) throws Exception { + Method method = AbstractSoda.class.getDeclaredMethod("shellQuote", String.class); + method.setAccessible(true); + return (String) method.invoke(null, value); + } + + @SuppressWarnings("unchecked") + private static Map scrub(Map input) throws Exception { + Method method = Scan.class.getDeclaredMethod("scrubSensitiveValues", Map.class); + method.setAccessible(true); + return (Map) method.invoke(null, input); + } + + // --- shellQuote ------------------------------------------------------------------------- + + @Test + void shellQuote_wrapsPlainRequirement() throws Exception { + assertThat(shellQuote("soda-core-postgres"), is("'soda-core-postgres'")); + } + + @Test + void shellQuote_preservesVersionOperatorsLiterally() throws Exception { + // `>` and `<` used to be interpreted as shell redirection when interpolated bare. + assertThat(shellQuote("soda-core[postgres]>=3.0,<4.0"), is("'soda-core[postgres]>=3.0,<4.0'")); + } + + @Test + void shellQuote_neutralizesShellMetacharacters() throws Exception { + // Command-injection attempt: the `;` and everything after must stay inside the quotes. + String quoted = shellQuote("pkg; rm -rf /"); + assertThat(quoted, is("'pkg; rm -rf /'")); + // Nothing escapes the single-quoted span, so no bare metacharacter is exposed to the shell. + assertThat(quoted.startsWith("'"), is(true)); + assertThat(quoted.endsWith("'"), is(true)); + } + + @Test + void shellQuote_escapesEmbeddedSingleQuote() throws Exception { + // The one character that cannot live inside single quotes is escaped as '\'' . + assertThat(shellQuote("a'b"), is("'a'\\''b'")); + } + + @Test + void shellQuote_preservesEnvironmentMarkers() throws Exception { + assertThat( + shellQuote("pkg; python_version >= \"3.8\""), + is("'pkg; python_version >= \"3.8\"'") + ); + } + + // --- scrubSensitiveValues --------------------------------------------------------------- + + @Test + void scrub_redactsTopLevelSensitiveKeys() throws Exception { + Map input = new LinkedHashMap<>(); + input.put("password", "hunter2"); + input.put("host", "db.internal"); + + Map result = scrub(input); + + assertThat(result.get("password"), is("******")); + assertThat(result.get("host"), is("db.internal")); + } + + @Test + void scrub_recursesIntoNestedMaps() throws Exception { + Map connection = new LinkedHashMap<>(); + connection.put("password", "s3cret"); + connection.put("port", 5432); + + Map input = new LinkedHashMap<>(); + input.put("connection", connection); + + Map result = scrub(input); + + @SuppressWarnings("unchecked") + Map scrubbedConnection = (Map) result.get("connection"); + assertThat(scrubbedConnection.get("password"), is("******")); + // Non-sensitive leaf values keep their original value and type. + assertThat(scrubbedConnection.get("port"), is(5432)); + } + + @Test + void scrub_redactsSecretKeyVariants() throws Exception { + Map input = new LinkedHashMap<>(); + input.put("api_key", "abc"); + input.put("apiKey", "abc"); + input.put("token", "xyz"); + input.put("account_info_json", "{...}"); + input.put("client_secret", "shh"); + input.put("type", "postgres"); + + Map result = scrub(input); + + assertThat(result.get("api_key"), is("******")); + assertThat(result.get("apiKey"), is("******")); + assertThat(result.get("token"), is("******")); + assertThat(result.get("account_info_json"), is("******")); + assertThat(result.get("client_secret"), is("******")); + assertThat(result.get("type"), is("postgres")); + } + + @Test + void scrub_recursesIntoLists() throws Exception { + // A non-sensitive key whose value is a list of maps: secrets nested in list elements + // must still be redacted (the recursion used to stop at Map values only). + Map node1 = new LinkedHashMap<>(); + node1.put("host", "db1"); + node1.put("password", "s1"); + Map node2 = new LinkedHashMap<>(); + node2.put("host", "db2"); + node2.put("token", "t2"); + + Map input = new LinkedHashMap<>(); + input.put("nodes", new ArrayList<>(List.of(node1, node2))); + + Map result = scrub(input); + + @SuppressWarnings("unchecked") + List> nodes = (List>) result.get("nodes"); + assertThat(nodes, hasSize(2)); + assertThat(nodes.get(0).get("password"), is("******")); + assertThat(nodes.get(0).get("host"), is("db1")); + assertThat(nodes.get(1).get("token"), is("******")); + assertThat(nodes.get(1).get("host"), is("db2")); + } + + @Test + void scrub_redactsAnyKeyContainingSensitiveSubstring() throws Exception { + // Deliberate over-redaction: matching is by substring, so any key merely containing a + // sensitive fragment is masked rather than risk leaking a secret. + Map input = new LinkedHashMap<>(); + input.put("keyfile", "/path/to/sa.json"); + input.put("keyspace", "analytics"); + input.put("sortkey", "created_at"); + input.put("author", "alice"); + + Map result = scrub(input); + + assertThat(result.get("keyfile"), is("******")); + assertThat(result.get("keyspace"), is("******")); + assertThat(result.get("sortkey"), is("******")); + assertThat(result.get("author"), is("******")); + } +}