dedup task scripts#123
Conversation
📝 WalkthroughWalkthroughAdds a shared Bash library (internal/common/tasks/scripts/common.sh), embeds it into build scripts via Go //go:embed, refactors build_image.sh and build_builder.sh to use the shared helpers (auth, osbuild, workspace, CA, artifact handling), and injects USE_MEMORY_VOLUMES into three Tekton task steps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Tekton as Tekton Task
participant Script as Embedded Script (common.sh + build_*.sh)
participant OSBuild as osbuild
participant Storage as containers-storage
participant Registry as Cluster Registry
Tekton->>Script: invoke build script (env, WORKSPACE_PATH)
Script->>Script: setup_container_config / setup_var_tmp / setup_cluster_auth
Script->>OSBuild: setup_osbuild & run osbuild (build steps)
Script->>Storage: create LOCAL target (containers-storage image)
Script->>Registry: push docker://TARGET_IMAGE (skopeo/podman)
Registry-->>Script: respond with existence / push result
Script->>Tekton: write RESULT_PATH and artifact outputs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/common/tasks/scripts/build_image.sh (1)
232-235:⚠️ Potential issue | 🔴 CriticalBug:
BUILD_CONTAINER_ARGSis always populated becauseLOCAL_BUILDER_IMAGEis never empty.
LOCAL_BUILDER_IMAGEis unconditionally set on line 189, so the condition on line 233 is always true. This means--build-container localhost/aib-build:…is always passed toaib, even when no builder image was pulled into local storage (i.e., whenBUILDER_IMAGEis empty). This will cause build failures for non-builder-image flows.The condition should check whether a builder image was actually pulled/available, not whether the local name variable is set.
🐛 Proposed fix
declare -a BUILD_CONTAINER_ARGS=() -if [ -n "$LOCAL_BUILDER_IMAGE" ]; then +if [ -n "$BUILD_CONTAINER_ARG" ]; then BUILD_CONTAINER_ARGS=("--build-container" "$LOCAL_BUILDER_IMAGE") fiAlternatively, gate on
BUILDER_IMAGEbeing non-empty and the mode being bootc/disk, mirroring the pull condition on line 202.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_image.sh` around lines 232 - 235, The BUILD_CONTAINER_ARGS population is guarded by checking LOCAL_BUILDER_IMAGE which is always set, causing --build-container to be passed incorrectly; change the condition to verify a builder image was actually pulled by testing BUILDER_IMAGE (and optionally the build mode like "bootc" or "disk" if you want parity with the pull logic) before assigning BUILD_CONTAINER_ARGS, i.e. only set BUILD_CONTAINER_ARGS=("--build-container" "$LOCAL_BUILDER_IMAGE") when BUILDER_IMAGE is non-empty (and mode is bootc/disk if applicable) so non-builder-image flows don't get the flag.
🧹 Nitpick comments (1)
internal/common/tasks/scripts/common.sh (1)
14-22:validate_argdoes not block single/double quotes or newlines.The blocklist catches common shell metacharacters but allows
',", and embedded newlines. If any validated argument is later interpolated into an unquoted context or heredoc, quotes could break syntax. Given that values pass through arrays (noteval), the current set is likely sufficient, but consider documenting this explicitly or adding'"to the character class if you want defense-in-depth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/common.sh` around lines 14 - 22, The validate_arg function currently misses single-quote, double-quote and newline characters; update the regex in validate_arg to also block "'" and '"' plus newlines (e.g., include '\"' and '\n' (and optionally '\r') in the character class) so arguments containing quotes or embedded newlines are rejected, and add a brief inline comment near validate_arg noting this is defensive hardening against quote/newline injection when arguments are later interpolated or used in heredocs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common/tasks/scripts/build_builder.sh`:
- Line 47: Replace the hardcoded path passed to load_custom_definitions in
build_builder.sh with Tekton workspace interpolation to match other scripts: use
$(workspaces.manifest-config-workspace.path)/custom-definitions.env instead of
/workspace/manifest-config/custom-definitions.env so missing workspace
misconfigurations don't cause silent failures (load_custom_definitions is a
silent no-op on missing files); update the argument where
load_custom_definitions is called to the interpolated workspace path.
---
Outside diff comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 232-235: The BUILD_CONTAINER_ARGS population is guarded by
checking LOCAL_BUILDER_IMAGE which is always set, causing --build-container to
be passed incorrectly; change the condition to verify a builder image was
actually pulled by testing BUILDER_IMAGE (and optionally the build mode like
"bootc" or "disk" if you want parity with the pull logic) before assigning
BUILD_CONTAINER_ARGS, i.e. only set BUILD_CONTAINER_ARGS=("--build-container"
"$LOCAL_BUILDER_IMAGE") when BUILDER_IMAGE is non-empty (and mode is bootc/disk
if applicable) so non-builder-image flows don't get the flag.
---
Nitpick comments:
In `@internal/common/tasks/scripts/common.sh`:
- Around line 14-22: The validate_arg function currently misses single-quote,
double-quote and newline characters; update the regex in validate_arg to also
block "'" and '"' plus newlines (e.g., include '\"' and '\n' (and optionally
'\r') in the character class) so arguments containing quotes or embedded
newlines are rejected, and add a brief inline comment near validate_arg noting
this is defensive hardening against quote/newline injection when arguments are
later interpolated or used in heredocs.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/common/tasks/scripts/build_image.sh (1)
465-468:rm -rfwithout a non-empty guard on$WORKSPACE_PATH(SC2115).If
WORKSPACE_PATHwere ever empty (e.g., Tekton interpolation failure), line 467 expands torm -rf "/${exportFile}", which could delete a top-level system directory. Use${WORKSPACE_PATH:?}to abort instead.🛡️ Proposed fix
- rm -rf "$WORKSPACE_PATH/${exportFile}" + rm -rf "${WORKSPACE_PATH:?}/${exportFile}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_image.sh` around lines 465 - 468, The rm -rf invocation can catastrophically run against root if WORKSPACE_PATH is empty; update the cleanup and directory-change commands that reference WORKSPACE_PATH and exportFile (the rm -rf and pushd lines in build_image.sh) to use the shell safe-expansion ${WORKSPACE_PATH:?} (and/or guard with an explicit non-empty check) so the script aborts when WORKSPACE_PATH is unset or empty before executing rm -rf "$WORKSPACE_PATH/${exportFile}" and before pushd "$WORKSPACE_PATH".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common/tasks/scripts/build_builder.sh`:
- Around line 27-30: AIB_IMAGE_NAME is declared but never used; remove the dead
variable assignment to clean up the script and satisfy ShellCheck SC2034—delete
the line setting AIB_IMAGE_NAME="localhost/aib-build:${DISTRO}" and keep only
the used LOCAL_TARGET variable
(LOCAL_TARGET="localhost/aib-build:${DISTRO}-${TARGET_ARCH}-${AIB_HASH}") so
downstream logic remains unchanged.
In `@internal/common/tasks/scripts/build_image.sh`:
- Line 151: Remove the dead scalar variable by deleting the unused
BUILD_CONTAINER_ARG="" assignment; the script now uses BUILD_CONTAINER_ARGS
(plural) array, so remove any reference to the singular BUILD_CONTAINER_ARG to
avoid dead code and ShellCheck SC2034 warnings, and ensure no other code
references BUILD_CONTAINER_ARG (search for BUILD_CONTAINER_ARG and replace with
BUILD_CONTAINER_ARGS if any existed).
- Around line 50-52: The current guard uses "if mountpoint -q \"$OSBUILD_PATH\";
then exit 0; fi" which aborts the entire build when $OSBUILD_PATH is already a
mountpoint; change the logic so the script does not exit but instead skips only
the setup step — e.g. remove the exit and either no-op when mountpoint is true
or invert the check so that setup_osbuild is only called when mountpoint -q
"$OSBUILD_PATH" is false; update references around the existing setup_osbuild
invocation to ensure setup_osbuild is executed only when needed and the rest of
the build continues.
In `@internal/common/tasks/scripts/common.sh`:
- Around line 14-23: The comment for validate_arg misstates which characters are
blocked; update either the comment or the regex so they match: in function
validate_arg replace the explanatory comment about blocking quotes/newlines with
an accurate list of blocked characters (semicolon, pipe, ampersand, dollar,
backtick, parens, braces, angle brackets, exclamation) or, if you intend to
block single-quote, double-quote and newlines too, extend the regex in
validate_arg to include ' and " and newline/carriage-return (e.g., add
'\"'\n'\r' to the character class) so the code and comment remain consistent and
defensive against injection.
- Around line 83-105: setup_cluster_auth writes the Kubernetes service account
token to $HOME/.authjson without enforcing strict permissions, so in contexts
like build_builder.sh (which doesn't set umask 0077) the file can become
world-readable; fix by ensuring the file is created with restrictive permissions
before writing (e.g., set umask 0077 or create via a secure temporary file and
chmod 600) in the code path that calls setup_cluster_auth (or inside
setup_cluster_auth itself) so that the auth file produced by the function has
mode 600; reference the setup_cluster_auth function and the $HOME/.authjson file
creation when making the change.
---
Nitpick comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 465-468: The rm -rf invocation can catastrophically run against
root if WORKSPACE_PATH is empty; update the cleanup and directory-change
commands that reference WORKSPACE_PATH and exportFile (the rm -rf and pushd
lines in build_image.sh) to use the shell safe-expansion ${WORKSPACE_PATH:?}
(and/or guard with an explicit non-empty check) so the script aborts when
WORKSPACE_PATH is unset or empty before executing rm -rf
"$WORKSPACE_PATH/${exportFile}" and before pushd "$WORKSPACE_PATH".
| validate_arg() { | ||
| local arg="$1" | ||
| local name="$2" | ||
| # Block shell metacharacters that could be used for injection | ||
| # Including quotes and newlines for defensive hardening against quote/newline injection | ||
| if [[ "$arg" =~ [\;\|\&\$\`\(\)\{\}\<\>\!] ]]; then | ||
| echo "ERROR: Invalid characters in $name: $arg" | ||
| exit 1 | ||
| fi | ||
| } |
There was a problem hiding this comment.
Comment overstates the scope of validate_arg's sanitization.
Lines 17–18 claim that single/double quotes and newlines are blocked, but the regex [\;\|\&\$\(){}<>!]includes neither', "nor newlines. A crafted value containing unescaped quotes (e.g.,foo' bar`) passes this check. The comment should be corrected to reflect what is actually blocked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/scripts/common.sh` around lines 14 - 23, The comment
for validate_arg misstates which characters are blocked; update either the
comment or the regex so they match: in function validate_arg replace the
explanatory comment about blocking quotes/newlines with an accurate list of
blocked characters (semicolon, pipe, ampersand, dollar, backtick, parens,
braces, angle brackets, exclamation) or, if you intend to block single-quote,
double-quote and newlines too, extend the regex in validate_arg to include ' and
" and newline/carriage-return (e.g., add '\"'\n'\r' to the character class) so
the code and comment remain consistent and defensive against injection.
There was a problem hiding this comment.
is this even needed? this runs in a privileged pod in created by a tekton task
There was a problem hiding this comment.
@bennyz You raise a fair point. In a privileged Tekton pod with controlled task definitions, the attack surface is significantly reduced since:
- Trusted environment: Task parameters come from cluster resources (Tekton TaskRuns/PipelineRuns)
- Privilege level: The pod already runs privileged, so preventing shell injection doesn't create a meaningful security boundary
- Input source: Arguments are typically set by the operator/system, not by untrusted external users
Trade-off considerations:
- Keep it if: Task parameters might be influenced by user input (e.g., via webhooks, API calls, or ConfigMaps that users can modify), or you want defense-in-depth against accidental bugs
- Remove it if: All inputs are strictly controlled by cluster admins and you prefer simpler code without the overhead
If your threat model doesn't include malicious or malformed inputs reaching these scripts, the validation is likely unnecessary. Would you like to remove validate_arg and its usages to simplify the code?
There was a problem hiding this comment.
the input comes from the user.
There was a problem hiding this comment.
@bennyz Thank you for that clarification—that changes things significantly. If the input comes from users, then validation is definitely needed to prevent shell injection attacks, even in a privileged pod.
This makes the original issue more important: the current regex doesn't actually block single quotes, double quotes, or newlines as the comment claims. Since you're dealing with user input, you should strengthen the validation to match what the comment says:
validate_arg() {
local arg="$1"
local name="$2"
# Block shell metacharacters including quotes and newlines
if [[ "$arg" =~ [\;\|\&\$\`\(\)\{\}\<\>\!\'\"\
] ]]; then
echo "ERROR: Invalid characters in $name: $arg"
exit 1
fi
}Or use a more comprehensive PCRE pattern if available:
if [[ "$arg" =~ [[:cntrl:]\;\|\&\$\`\(\)\{\}\<\>\!\'\"] ]]; thenThis ensures you're actually blocking the dangerous characters mentioned in the comment, providing proper defense against injection when handling user input.
✏️ Learnings added
Learnt from: bennyz
Repo: centos-automotive-suite/automotive-dev-operator PR: 123
File: internal/common/tasks/scripts/common.sh:14-23
Timestamp: 2026-02-20T13:40:34.734Z
Learning: In the automotive-dev-operator Tekton tasks (build_image.sh, build_builder.sh), script inputs come from users, so input validation in common.sh is security-critical and should block shell metacharacters including quotes and newlines.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
give me an example for an attack
There was a problem hiding this comment.
@bennyz Sure! Here are concrete examples of attacks that exploit the missing quote validation:
Example 1: Argument injection via double quotes
# Script contains:
buildah build -t "$IMAGE_NAME" .
# Attacker provides IMAGE_NAME:
myimage" --build-arg ROOT_PASSWORD=hacked --build-arg MALICIOUS_ARG=value "
# Results in:
buildah build -t "myimage" --build-arg ROOT_PASSWORD=hacked --build-arg MALICIOUS_ARG=value "" .This injects arbitrary build arguments that could leak secrets or modify the build.
Example 2: Privilege escalation via single quotes
# Script contains:
podman run --name '$CONTAINER_NAME' some-image
# Attacker provides CONTAINER_NAME:
test' --privileged --volume /:/hostfs --rm -it alpine sh -c 'cat /hostfs/etc/shadow
# Results in:
podman run --name 'test' --privileged --volume /:/hostfs --rm -it alpine sh -c 'cat /hostfs/etc/shadow' some-imageThis escapes the intended single quotes, mounts the host filesystem, and exfiltrates /etc/shadow.
Example 3: Command injection via newline
# Script contains:
NAMESPACE="$USER_NAMESPACE"
echo "Building in namespace: $NAMESPACE"
# Attacker provides USER_NAMESPACE with embedded newline:
production
rm -rf $OSBUILD_STORE/*
echo "cleaned
# Results in three separate commands:
NAMESPACE="production
rm -rf $OSBUILD_STORE/*
echo "cleaned"The newline breaks the variable assignment and executes arbitrary commands, potentially wiping the build cache.
Since your scripts run privileged and have access to cluster registries, storage, and possibly secrets, these attacks could:
- Exfiltrate credentials from
/run/secretsor registry auth files - Poison the image registry with malicious images
- Access or modify host filesystems
- Steal service account tokens
This is why you need to extend the validation regex to block ', ", and newlines.
🧠 Learnings used
Learnt from: bennyz
Repo: centos-automotive-suite/automotive-dev-operator PR: 123
File: internal/common/tasks/scripts/common.sh:14-23
Timestamp: 2026-02-20T13:40:34.734Z
Learning: In the automotive-dev-operator Tekton tasks (build_image.sh, build_builder.sh), script inputs come from users, so input validation in common.sh is security-critical and should block shell metacharacters including quotes and newlines.
There was a problem hiding this comment.
The values validate_arg actually touched (AIB_EXTRA_ARGS from a ConfigMap, CONTAINER_REF from a Tekton param) are all passed into bash arrays and expanded as "${array[@]}" — each element is a separate quoted word
There was a problem hiding this comment.
🧩 Analysis chain
@bennyz That's a crucial detail! You're absolutely right that "${array[@]}" expansion provides proper quoting and prevents word-splitting attacks. Let me verify how these validated values are actually used in the codebase to give you an accurate assessment.
🏁 Script executed:
#!/bin/bash
# Find all usages of validate_arg to see what's being validated
rg -n -A2 -B2 'validate_arg' internal/common/tasks/scripts/
Length of output: 1473
🏁 Script executed:
#!/bin/bash
# Search for how AIB_EXTRA_ARGS is used after validation
rg -n -C5 'AIB_EXTRA_ARGS' internal/common/tasks/scripts/
Length of output: 3780
🏁 Script executed:
#!/bin/bash
# Search for how CONTAINER_REF is used after validation
rg -n -C5 'CONTAINER_REF' internal/common/tasks/scripts/
Length of output: 3888
🏁 Script executed:
#!/bin/bash
# Look for array expansion patterns to confirm the usage pattern
rg -n '\$\{[^}]*\[@\]\}' internal/common/tasks/scripts/
Length of output: 1648
@bennyz You're absolutely right about AIB_EXTRA_ARGS! I can see it's declared as an array and consistently expanded with "${AIB_EXTRA_ARGS[@]}", which properly quotes each element as a separate argument. The validation there is overly defensive.
However, CONTAINER_REF is not an array—it's a simple string variable used with double-quote interpolation:
skopeo copy "docker://$CONTAINER_REF" "containers-storage:$CONTAINER_REF"This is still vulnerable to quote injection even with the current validation. Example attack:
# Attacker provides:
CONTAINER_REF='registry.io/image" --authfile=/tmp/steal-creds "dummy'
# Results in:
skopeo copy "docker://registry.io/image" --authfile=/tmp/steal-creds "dummy" "containers-storage:..."The quotes in the value break out of the intended string and inject new arguments.
Recommendation:
- For AIB_EXTRA_ARGS: The validation is unnecessary since array expansion is safe. You can remove those
validate_argcalls. - For CONTAINER_REF: Keep the validation BUT fix the regex to actually block quotes and newlines as originally intended:
if [[ "$arg" =~ [\;\|\&\$\`\(\)\{\}\<\>\!\'\"\
] ]]; thenWould you like me to help plan removing the validation for array-based arguments while strengthening it for CONTAINER_REF?
🧠 Learnings used
Learnt from: bennyz
Repo: centos-automotive-suite/automotive-dev-operator PR: 123
File: internal/common/tasks/scripts/common.sh:14-23
Timestamp: 2026-02-20T13:40:34.734Z
Learning: In the automotive-dev-operator Tekton tasks (build_image.sh, build_builder.sh), script inputs come from users, so input validation in common.sh is security-critical and should block shell metacharacters including quotes and newlines.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/common/tasks/scripts/common.sh (2)
174-183: Credentials logged at DEBUG level — consider redacting field presence.
read_registry_credsechoes which credential files exist (e.g., "Found REGISTRY_PASSWORD"). While it doesn't log the actual values, the DEBUG output confirms credential presence and the directory path. In a multi-tenant or shared-log environment this can aid reconnaissance. This is low-severity given these are Tekton pod logs, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/common.sh` around lines 174 - 183, The function read_registry_creds currently prints debug lines that reveal which specific credential files exist (e.g., "Found REGISTRY_PASSWORD") and the auth_dir path; remove or redact those prints to avoid leaking sensitive metadata. Update read_registry_creds so it still reads REGISTRY_URL, REGISTRY_USERNAME, REGISTRY_PASSWORD, REGISTRY_TOKEN, and REGISTRY_AUTH_FILE_CONTENT but do not echo per-file presence or the full auth_dir; instead emit a single non-sensitive debug message (e.g., "DEBUG: Registry credentials read" or "DEBUG: Registry credentials read (masked)") and only log a warning if no credentials were found, keeping the variable names listed above intact in the function.
188-222: Auth files insetup_registry_authandcreate_auth_jsonalso inherit the process umask.Same pattern as
setup_cluster_auth—$HOME/.custom_authjsonis written viacat >without explicit permission control. If you hardensetup_cluster_auth, apply the same treatment here for consistency. Thecreate_service_account_authfunction (line 155) has the same issue with its$output_file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/common.sh` around lines 188 - 222, The registry auth file and other auth files are being created without explicit permissions (so they inherit the process umask); update setup_registry_auth, create_auth_json and create_service_account_auth to create their output files with strict permissions (e.g. 600) instead of relying on default umask—either set a temporary umask 077 around the write or create the file then chmod 600 immediately after writing (apply this to the "$HOME/.custom_authjson" creation in setup_registry_auth and the "$output_file" writes in create_auth_json and create_service_account_auth) so sensitive credentials are not world-readable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common/tasks/scripts/common.sh`:
- Around line 23-30: The validate_custom_def function currently only validates
the KEY and allows arbitrary characters in VALUE; update it to reject shell
metacharacters and control chars so values cannot contain characters like ; | &
$ ` ' " < > \ (newlines/control chars) and only allow a safe subset (e.g.,
alphanumerics plus . _ - and optionally /) after the =; update the regex in
validate_custom_def (and any callers that build CUSTOM_DEFS_ARGS / pass to aib
--define) to enforce KEY=SAFE_VALUE and exit with an error if the value contains
forbidden characters so malicious inputs like KEY=$(...) or values with
quotes/newlines are blocked.
---
Duplicate comments:
In `@internal/common/tasks/scripts/common.sh`:
- Around line 80-102: The setup_cluster_auth function writes the service-account
token into $HOME/.authjson using a heredoc and inherits the caller umask, which
can leave the file world-readable; to harden it, create the auth file with
restrictive permissions (e.g., create a safe temp path or set umask 0077 before
writing) and/or explicitly chmod the file to 600 immediately after creation,
then export REGISTRY_AUTH_FILE to that secured path (reference
setup_cluster_auth, REGISTRY_AUTH_FILE, $REGISTRY and $TOKEN) so the token file
is never left with permissive permissions regardless of caller umask.
---
Nitpick comments:
In `@internal/common/tasks/scripts/common.sh`:
- Around line 174-183: The function read_registry_creds currently prints debug
lines that reveal which specific credential files exist (e.g., "Found
REGISTRY_PASSWORD") and the auth_dir path; remove or redact those prints to
avoid leaking sensitive metadata. Update read_registry_creds so it still reads
REGISTRY_URL, REGISTRY_USERNAME, REGISTRY_PASSWORD, REGISTRY_TOKEN, and
REGISTRY_AUTH_FILE_CONTENT but do not echo per-file presence or the full
auth_dir; instead emit a single non-sensitive debug message (e.g., "DEBUG:
Registry credentials read" or "DEBUG: Registry credentials read (masked)") and
only log a warning if no credentials were found, keeping the variable names
listed above intact in the function.
- Around line 188-222: The registry auth file and other auth files are being
created without explicit permissions (so they inherit the process umask); update
setup_registry_auth, create_auth_json and create_service_account_auth to create
their output files with strict permissions (e.g. 600) instead of relying on
default umask—either set a temporary umask 077 around the write or create the
file then chmod 600 immediately after writing (apply this to the
"$HOME/.custom_authjson" creation in setup_registry_auth and the "$output_file"
writes in create_auth_json and create_service_account_auth) so sensitive
credentials are not world-readable.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/common/tasks/scripts/build_image.sh (1)
57-57: ShellCheck SC2164:cd/pushdwithout|| exitguards (lines 57, 356, 452, 477)
set -eis inherited fromcommon.shso a failedcd/pushdwill abort the script in practice. However, the explicit|| exitform communicates the failure intent to readers and guards against future edits that might weaken error handling. ShellCheck SC2164 flags all four call sites.♻️ Representative fix (apply pattern to all four sites)
-cd "$WORKSPACE_PATH" +cd "$WORKSPACE_PATH" || exit 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/build_image.sh` at line 57, The cd/pushd calls (e.g., the cd "$WORKSPACE_PATH" invocation and the other pushd/cd sites flagged by ShellCheck) lack explicit failure guards; change each to append "|| exit" so the command becomes cd "$WORKSPACE_PATH" || exit (and similarly for pushd calls) to make failure handling explicit and prevent the script from continuing on a failed directory change.internal/common/tasks/scripts/common.sh (1)
2-2: Addset -o pipefailto catch pipeline failures
set -ecatches simple command failures but not failures in the left-hand side of a pipe.build_image.sh'star_dirfunction usestar ... | lz4andtar ... | "$GZIP_COMPRESSOR"pipelines — a failedtarwould silently succeed and write empty/corrupt output, causing a valid-looking but broken artifact.♻️ Proposed fix
set -e +set -o pipefail🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/common/tasks/scripts/common.sh` at line 2, The script currently uses "set -e" which doesn't catch failures in pipeline components; update the shell options to also enable pipefail by adding "set -o pipefail" alongside "set -e" in internal/common/tasks/scripts/common.sh so pipeline failures (e.g., in tar_dir within build_image.sh where `tar ... | lz4` or `tar ... | "$GZIP_COMPRESSOR"` are used) are detected and cause the script to fail rather than producing corrupt artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Line 476: The rm command risks deleting root-level paths if WORKSPACE_PATH is
empty; update the cleanup to guard variables by using the parameter-expansion
error-check form so the script aborts when values are unset or empty—replace the
current rm usage that references WORKSPACE_PATH and exportFile with a guarded
form (e.g., use ${WORKSPACE_PATH:?} and ${exportFile:?}) or add an explicit
check for WORKSPACE_PATH and exportFile before calling rm to ensure neither is
empty.
In `@internal/common/tasks/scripts/common.sh`:
- Around line 156-170: The create_service_account_auth function writes sensitive
SA tokens to $output_file without enforcing a restrictive file mode; modify
create_service_account_auth to set a restrictive umask (e.g., 0077) locally
before writing and restore the previous umask afterwards so the created
$output_file is created with 0600 even if the caller didn't set umask, then
proceed to write the JSON using the existing heredoc logic referencing $registry
and $token; ensure you restore the original umask at function exit.
---
Duplicate comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 50-52: The script currently exits early when mountpoint -q
"$OSBUILD_PATH" is true, which bypasses install_custom_ca_certs, setup_osbuild,
and the aib build and results in no artifact; change the logic so the script
does not exit but only skips setup_osbuild when $OSBUILD_PATH is already a
mountpoint: detect mountpoint via OSBUILD_PATH and, if true, skip calling
setup_osbuild (and proceed to run install_custom_ca_certs and the aib build and
write /tekton/results/artifact-filename); ensure any early-return or exit 0 is
removed or inverted so only the setup_osbuild step is conditional.
---
Nitpick comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Line 57: The cd/pushd calls (e.g., the cd "$WORKSPACE_PATH" invocation and the
other pushd/cd sites flagged by ShellCheck) lack explicit failure guards; change
each to append "|| exit" so the command becomes cd "$WORKSPACE_PATH" || exit
(and similarly for pushd calls) to make failure handling explicit and prevent
the script from continuing on a failed directory change.
In `@internal/common/tasks/scripts/common.sh`:
- Line 2: The script currently uses "set -e" which doesn't catch failures in
pipeline components; update the shell options to also enable pipefail by adding
"set -o pipefail" alongside "set -e" in internal/common/tasks/scripts/common.sh
so pipeline failures (e.g., in tar_dir within build_image.sh where `tar ... |
lz4` or `tar ... | "$GZIP_COMPRESSOR"` are used) are detected and cause the
script to fail rather than producing corrupt artifacts.
| echo "Removing uncompressed directory ${exportFile} (keeping parts directory)" | ||
| rm -rf "$(workspaces.shared-workspace.path)/${exportFile}" | ||
| pushd $(workspaces.shared-workspace.path) | ||
| rm -rf "$WORKSPACE_PATH/${exportFile}" |
There was a problem hiding this comment.
SC2115: rm -rf without ${:?} guard
If WORKSPACE_PATH is empty for any reason (e.g., Tekton workspace not bound), rm -rf "$WORKSPACE_PATH/${exportFile}" degrades to rm -rf "/${exportFile}", removing a top-level filesystem path. Using ${WORKSPACE_PATH:?} causes the script to abort immediately with a clear error instead.
🛡️ Proposed fix
- rm -rf "$WORKSPACE_PATH/${exportFile}"
+ rm -rf "${WORKSPACE_PATH:?}/${exportFile}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rm -rf "$WORKSPACE_PATH/${exportFile}" | |
| rm -rf "${WORKSPACE_PATH:?}/${exportFile}" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 476-476: Use "${var:?}" to ensure this never expands to / .
(SC2115)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/scripts/build_image.sh` at line 476, The rm command
risks deleting root-level paths if WORKSPACE_PATH is empty; update the cleanup
to guard variables by using the parameter-expansion error-check form so the
script aborts when values are unset or empty—replace the current rm usage that
references WORKSPACE_PATH and exportFile with a guarded form (e.g., use
${WORKSPACE_PATH:?} and ${exportFile:?}) or add an explicit check for
WORKSPACE_PATH and exportFile before calling rm to ensure neither is empty.
| create_service_account_auth() { | ||
| local registry="$1" | ||
| local output_file="$2" | ||
| local token="${3:-$(cat /var/run/secrets/kubernetes.io/serviceaccount/token 2>/dev/null)}" | ||
|
|
||
| cat > "$output_file" <<EOF | ||
| { | ||
| "auths": { | ||
| "$registry": { | ||
| "auth": "$(echo -n "serviceaccount:$token" | base64 -w0)" | ||
| } | ||
| } | ||
| } | ||
| EOF | ||
| } |
There was a problem hiding this comment.
create_service_account_auth writes auth file without self-contained permission guard
The function writes a Kubernetes SA token to $output_file via plain cat > without setting a restrictive umask. In build_image.sh, umask 0077 is set at line 17 before this function is called, so /tmp/builder-auth.json inherits mode 0600 today. However, the function itself provides no protection, making it a hazard if reused in any context where the caller hasn't pre-set the umask (e.g., build_builder.sh, which has no umask 0077 at script level).
🛡️ Proposed fix — apply umask within the function
create_service_account_auth() {
local registry="$1"
local output_file="$2"
local token="${3:-$(cat /var/run/secrets/kubernetes.io/serviceaccount/token 2>/dev/null)}"
- cat > "$output_file" <<EOF
+(umask 0177; cat > "$output_file" <<EOF
{
"auths": {
"$registry": {
"auth": "$(echo -n "serviceaccount:$token" | base64 -w0)"
}
}
}
EOF
+)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/scripts/common.sh` around lines 156 - 170, The
create_service_account_auth function writes sensitive SA tokens to $output_file
without enforcing a restrictive file mode; modify create_service_account_auth to
set a restrictive umask (e.g., 0077) locally before writing and restore the
previous umask afterwards so the created $output_file is created with 0600 even
if the caller didn't set umask, then proceed to write the JSON using the
existing heredoc logic referencing $registry and $token; ensure you restore the
original umask at function exit.
1 participant
Summary by CodeRabbit
Refactoring
New Features