Implement log sanitization step#540
Conversation
This commmit adds a new step in the MintMaker pipelinerun, to sanitize renovate-step logs. In order to integrate MM with the Konflux UI, we will give read-permission to all logged users. Since the logs of all components lie in our namespace, all users will have access to the logs of all components. If a credential were leaked in the logs, the wide access would make it severe. To prevent that, this commmit adds a step to sanitize the logs with leaktk, before they are persisted.
|
🤖 Finished Review · ✅ Success · Started 1:54 PM UTC · Completed 2:07 PM UTC |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #540 +/- ##
==========================================
+ Coverage 47.61% 49.28% +1.66%
==========================================
Files 22 22
Lines 2161 2232 +71
==========================================
+ Hits 1029 1100 +71
Misses 1053 1053
Partials 79 79
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![qodo-app-for-konflux-ci[bot]](https://avatars.githubusercontent.com/u/155583288?s=60&v=4){kind=small}
| "cp \"$LOG_FILE\" \"${LOG_FILE}.tmp\"\n" + | ||
| "echo \"$SECRETS\" | while IFS= read -r secret; do\n" + | ||
| " if [ -n \"$secret\" ]; then\n" + | ||
| " ESCAPED=$(printf '%s\\n' \"$secret\" | sed 's/[[\\.*^$()+?{|\\\\]/\\\\&/g')\n" + | ||
| " sed -i \"s|${ESCAPED}|**REDACTED**|g\" \"${LOG_FILE}.tmp\"\n" + | ||
| " fi\n" + | ||
| "done\n" + | ||
| "if [ $? -ne 0 ]; then\n" + | ||
| " echo 'Failed to redact secrets from log file'\n" + | ||
| " fail_safe\n" + | ||
| "fi\n" + |
There was a problem hiding this comment.
2. Redaction errors not detected 🐞 Bug ⛨ Security
The redaction loop pipes secrets into a while and only checks $? after the loop, so individual sed -i failures can be masked and the step can complete with secrets still present in renovate-logs.json. Additionally, the escaping does not account for the | delimiter used in the sed expression, so secrets containing | will break redaction.
Agent Prompt
### Issue description
The log-sanitizer redaction loop can silently miss redaction failures (e.g., due to `sed` failures), leaving secrets unredacted in `/workspace/shared-data/renovate-logs.json`.
### Issue Context
The current script:
- Uses a pipeline into a `while` loop and only checks `$?` after the loop.
- Uses `sed` with `|` as delimiter but doesn’t escape `|` in the secret.
- Doesn’t validate the success of key file operations (`cp`, per-replacement `sed`, `mv`).
### Fix Focus Areas
- internal/tekton/pipeline_run_builder.go[255-298]
### Suggested fix direction
- Avoid the `echo ... | while ...` pipeline; iterate in a way that allows per-secret error handling.
- Handle delimiter/escaping robustly (or avoid regex-based replacement entirely).
- Check each redaction operation and on any failure immediately overwrite/truncate the original log file (fail-safe) and stop.
- Also verify `cp` and `mv` success, and fail-safe if they fail.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "SCAN_OUTPUT=$(leaktk scan --kind JSONData \"@${LOG_FILE}\" 2>&1)\n" + | ||
| "if [ $? -ne 0 ]; then\n" + | ||
| " echo \"leaktk scan failed: $SCAN_OUTPUT\"\n" + | ||
| " fail_safe\n" + | ||
| "fi\n" + |
There was a problem hiding this comment.
3. Scanner output logged verbatim 🐞 Bug ⛨ Security
On scan failures, the log-sanitizer prints the full raw leaktk output ($SCAN_OUTPUT) to stdout, which is persisted in Tekton step logs and is not affected by the file-sanitization overwrite. This undermines the goal of keeping sensitive log-related content out of broadly readable logs during sanitizer failures.
Agent Prompt
### Issue description
The log-sanitizer step echoes the complete leaktk scan output on failure, which places raw scan output into Tekton step logs (separate from the sanitized log file).
### Issue Context
Even if the step later overwrites `/workspace/shared-data/renovate-logs.json`, the already-emitted step logs remain and are not sanitized.
### Fix Focus Areas
- internal/tekton/pipeline_run_builder.go[267-272]
### Suggested fix direction
- Replace `echo "leaktk scan failed: $SCAN_OUTPUT"` with a generic error message that does not include the raw tool output.
- If diagnostic detail is required, write it to a restricted location (or omit entirely) rather than stdout/stderr.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsHigh
Medium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| "SECRETS=$(echo \"$SCAN_OUTPUT\" | python3 -c \"\nimport sys, json\nfor r in json.load(sys.stdin).get('results',[]):\n s = r.get('secret','')\n if s: print(s)\n\")\n" + | ||
| "if [ $? -ne 0 ]; then\n" + | ||
| " echo 'Failed to parse leaktk output'\n" + | ||
| " fail_safe\n" + |
There was a problem hiding this comment.
[high] logic-error
The log-sanitizer script uses python3 to parse leaktk JSON output, but the container image is quay.io/leaktk/leakt:latest, a Go-based scanner image that very likely does not include python3. If python3 is not present, the command fails and fail_safe is invoked, which overwrites the entire log file with a generic error message — meaning sanitization never actually works and all logs are always destroyed.
Suggested fix: Either use an image that includes python3, add python3 to the leaktk image, or replace the python3 JSON parsing with a shell-native approach (e.g., using jq if available, or grep/sed to extract secrets from the JSON output).
| "mv \"${LOG_FILE}.tmp\" \"$LOG_FILE\"\n" + | ||
| "echo 'Log sanitization complete'", | ||
| SecurityContext: &corev1.SecurityContext{ | ||
| Capabilities: &corev1.Capabilities{Drop: []corev1.Capability{"ALL"}}, |
There was a problem hiding this comment.
[medium] error-handling-gap
The $? check after the pipe-to-while-loop pipeline captures the exit status of the while loop subshell, not of individual sed -i commands within it. In POSIX sh, the pipe creates a subshell, so sed failures inside the loop are silently swallowed and partially-redacted logs are moved into place as if redaction succeeded (fail-open).
Suggested fix: Track failures inside the loop by writing a sentinel file on error, or switch to #!/bin/bash with set -o pipefail.
This commit extracts the log sanitization script outside of the yaml file it was previously in. This enhances the readability and maintainability. It also made it possible to write unit tests to the shell scripts.
|
Following the discussion in our last team retrospective, I'll refrain from force-pushing new commits and making a mess in the reviews 😂 When reviewing is done and we are happy with the change, I'll squash all commits together. |
| DefaultRenovateImageURL = "quay.io/konflux-ci/mintmaker-renovate-image:latest" | ||
|
|
||
| LeakTKImageEnvName = "LEAKTK_IMAGE" | ||
| DefaultLeakTKImageURL = "quay.io/leaktk/leakt:latest" |
There was a problem hiding this comment.
| DefaultLeakTKImageURL = "quay.io/leaktk/leakt:latest" | |
| DefaultLeakTKImageURL = "quay.io/leaktk/leaktk:latest" |
| exit 0 | ||
| fi | ||
| echo 'Scanning log file for leaked secrets...' | ||
| SCAN_OUTPUT=$(leaktk scan --kind JSONData "@${LOG_FILE}" 2>&1) |
There was a problem hiding this comment.
| SCAN_OUTPUT=$(leaktk scan --kind JSONData "@${LOG_FILE}" 2>&1) | |
| SCAN_OUTPUT=$(leaktk scan --kind JSONData "@${LOG_FILE}" 2>/dev/null) |
With 2>&1, the output will be:
[INFO] queueing scan: id="Ske18OkOmW0" queue_size=1
[INFO] starting scan: id="Ske18OkOmW0"
[INFO] queueing response: id="Ske18OkOmW0" queue_size=1
{"id":"RJY9B0n_COk","kind":"ScanResults","request_id":"Ske18OkOmW0","results":[]}
and wouldn't then parse using Python's json module, so you need to redirect stderr to /dev/null to only get the JSON output
| cp "$LOG_FILE" "${LOG_FILE}.tmp" | ||
| echo "$SECRETS" | while IFS= read -r secret; do | ||
| if [ -n "$secret" ]; then | ||
| ESCAPED=$(printf '%s\n' "$secret" | sed 's/[[\.*^$()+?{|\\]/\\&/g') |
There was a problem hiding this comment.
Can you please add a comment detailing what does the sed regex do?
3 participants
This commmit adds a new step in the MintMaker pipelinerun, to sanitize renovate-step logs.
In order to integrate MM with the Konflux UI, we will give read-permission to all logged users. Since the logs of all components lie in our namespace, all users will have access to the logs of all components.
If a credential were leaked in the logs, the wide access would make it severe. To prevent that, this commmit adds a step to sanitize the logs with leaktk, before they are persisted.
Testing remarks
I tested manually that the script works as intended, and that the container that is created is able to run it.