Avoid potential leaks #1167
Avoid potential leaks #1167
Conversation
WalkthroughMove kubeadmin and other credential handling into short-lived subshells with tracing disabled, add login retry logic and counters, redact/remove stored passwords and avoid logging secrets, and rework pull-secret and htpasswd flows to apply secrets in-memory without temporary files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant S as crc-cluster-status.sh
participant F as /opt/crc/pass_kubeadmin
participant OC as oc CLI
Note over S: Secure kubeadmin login flow (subshell hides secret)
S->>F: read password (subshell, set +x)
S->>OC: oc login --insecure-skip-tls-verify -u kubeadmin -p "<read>"
alt login fails
S->>S: log "try $COUNTER...", sleep 5s, increment COUNTER, retry
else login succeeds
S->>S: touch /tmp/.crc-cluster-ready
end
sequenceDiagram
autonumber
participant P as crc-pullsecret.sh
participant OC as oc CLI
participant J as jq
Note over P: Check and patch pull-secret only when needed (no secrets leaked)
P->>OC: oc get secret/pull-secret -o json
P->>J: has(".data[\".dockerconfigjson\"]|fromjson|.auths")
alt .auths exists
P-->>P: log "Cluster already has the pull secrets, nothing to do", exit 0
else missing .auths
P->>P: read /opt/crc/pull-secret (subshell, set +x), base64-encode
P->>OC: oc patch secret/pull-secret --patch ...
P-->>P: exit 0
end
sequenceDiagram
autonumber
participant U as ocp-userpasswords.sh
participant H as htpasswd (gen)
participant OC as oc CLI
Note over U: Generate htpasswd entries and apply secret without temp files
U->>H: gen_htpasswd developer <pass> (subshell, set +x)
U->>H: gen_htpasswd kubeadmin <pass> (subshell, set +x)
U->>OC: oc create secret generic htpass-secret --dry-run -o yaml | oc apply -f-
U-->>U: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
snc.sh(1 hunks)systemd/crc-cluster-status.sh(2 hunks)systemd/crc-pullsecret.sh(1 hunks)systemd/ocp-userpasswords.sh(1 hunks)tools.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
systemd/crc-pullsecret.sh (1)
systemd/crc-systemd-common.sh (1)
wait_for_resource(3-15)
systemd/ocp-userpasswords.sh (1)
systemd/crc-systemd-common.sh (1)
wait_for_resource(3-15)
snc.sh (1)
tools.sh (1)
retry(110-128)
🪛 Shellcheck (0.11.0)
systemd/ocp-userpasswords.sh
[error] 14-14: Couldn't parse this redirection. Fix to allow more checks.
(SC1073)
[error] 14-14: Fix any mentioned problems and try again.
(SC1072)
snc.sh
[warning] 288-288: kubeadmin is referenced but not assigned.
(SC2154)
kpouget
force-pushed
the
leaks
branch
2 times, most recently
from
5a4d421 to
96d52c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
tools.sh (1)
222-227: Harden parameter use and quoting in subshell.Use the named params and quote paths/vars; also make
random_passwordlocal to avoid global leakage.- ( - set +x # use a subshell to avoid leaking the password - random_password=$(cat $1/auth/kubeadmin-password) - ${HTPASSWD} -c -B -b ${pass_file} developer developer - ${HTPASSWD} -B -b ${pass_file} kubeadmin "${random_password}" - ) + ( + set +x # use a subshell to avoid leaking the password + local random_password + random_password="$(cat "${auth_file_dir}/auth/kubeadmin-password")" + "${HTPASSWD}" -c -B -b "${pass_file}" developer developer + "${HTPASSWD}" -B -b "${pass_file}" kubeadmin "${random_password}" + )systemd/ocp-userpasswords.sh (1)
42-45: Use POSIX-compliant empty-line filter in sed.
\sis not portable in sed; use[[:space:]]. Also quoting improves robustness.- --from-file=htpasswd=<((echo "$dev_pass"; echo "$adm_pass") | sed '/^\s*$/d') \ + --from-file=htpasswd=<((printf '%s\n%s\n' "$dev_pass" "$adm_pass" | sed '/^[[:space:]]*$/d')) \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
snc.sh(1 hunks)systemd/crc-cluster-status.sh(2 hunks)systemd/crc-pullsecret.sh(1 hunks)systemd/ocp-userpasswords.sh(1 hunks)tools.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
systemd/crc-pullsecret.sh (1)
systemd/crc-systemd-common.sh (1)
wait_for_resource(3-15)
snc.sh (1)
tools.sh (1)
retry(110-128)
systemd/ocp-userpasswords.sh (1)
systemd/crc-systemd-common.sh (1)
wait_for_resource(3-15)
🔇 Additional comments (1)
systemd/crc-cluster-status.sh (1)
27-36: LGTM: safe login with subshell and retry.Subshell disables tracing around password, keeps retry semantics, and avoids leaking credentials.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
systemd/crc-pullsecret.sh (1)
14-31: With errexit, oc get fails if the secret is missing; ensure it exists before reading/patching.
existingPsB64=$(oc get …)will abort the script when the secret isn’t present yet. Gate with an existence/wait check, then proceed.set +x # disable the logging to avoid leaking the pull secrets -# check if existing pull-secret is valid if not add the one from /opt/crc/pull-secret -existingPsB64=$(oc get secret pull-secret -n openshift-config -o jsonpath="{['data']['\.dockerconfigjson']}") +# ensure the pull-secret exists before reading it (avoid errexit abort) +if ! oc get secret pull-secret -n openshift-config >/dev/null 2>&1; then + wait_for_resource "secret/pull-secret -n openshift-config" 60 5 +fi + +# check if existing pull-secret is valid; if not, add the one from /opt/crc/pull-secret +existingPsB64=$(oc get secret pull-secret -n openshift-config -o jsonpath="{['data']['\.dockerconfigjson']}") existingPs=$(echo "${existingPsB64}" | base64 -d)
🧹 Nitpick comments (2)
systemd/crc-cluster-status.sh (1)
22-29: Optional: restore tracing after the secret-sensitive login.You disable xtrace before/inside the subshell and never re-enable, reducing later diagnostics. Consider re‑enabling once the login loop finishes.
done +# Re-enable tracing for subsequent steps +set -x + # need to set a marker to let `crc` know the cluster is ready touch /tmp/.crc-cluster-readysystemd/ocp-userpasswords.sh (1)
33-40: Optional: restore xtrace after secret handling to keep logs useful.You turned off tracing for secret generation; consider re‑enabling afterwards.
echo "creating the password secret ..." # use bash <() to use a temporary fd file # use sed to remove the empty lines oc create secret generic htpass-secret \ --from-file=htpasswd=<((echo "$dev_pass"; echo "$adm_pass") | sed '/^[[:space:]]*$/d') \ -n openshift-config \ --dry-run=client -oyaml | oc apply -f- +set -x echo "all done"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
snc.sh(1 hunks)systemd/crc-cluster-status.sh(2 hunks)systemd/crc-pullsecret.sh(1 hunks)systemd/ocp-userpasswords.sh(1 hunks)tools.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
snc.sh (1)
tools.sh (1)
retry(110-128)
systemd/ocp-userpasswords.sh (1)
systemd/crc-systemd-common.sh (1)
wait_for_resource(3-15)
systemd/crc-pullsecret.sh (1)
systemd/crc-systemd-common.sh (1)
wait_for_resource(3-15)
🪛 Shellcheck (0.11.0)
tools.sh
[warning] 224-224: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (3)
systemd/ocp-userpasswords.sh (2)
13-16: Fix error text (“two arguments”).Minor typo in the validation message.
- if [ -z "${1:-}" ] || [ -z "${2:-}" ]; then - echo "gen_htpasswd needs to arguments: username password" 1>&2 + if [ -z "${1:-}" ] || [ -z "${2:-}" ]; then + echo "gen_htpasswd needs two arguments: username password" 1>&2 return 1 fi
18-19: Feed password via stdin to avoid exposure in process arguments.- podman run --rm -i docker.io/xmartlabs/htpasswd "$1" "$2" + podman run --rm -i docker.io/xmartlabs/htpasswd -i "$1" <<<"$2"snc.sh (1)
283-290: Harden sed escaping to cover all regex metacharactersGNU sed accepts the expanded bracket expression—tested against
[]/\&$.*^|-and it correctly escapes all characters. Update the snippet as follows:- esc_pw="$(printf '%s' "$kubeadmin_pass" | sed -e 's/[\/&|\\]/\\&/g')" - sed -i "s|$esc_pw|REDACTED|g" "${INSTALL_DIR}/.openshift_install.log" + esc_pw="$(printf '%s' "$kubeadmin_pass" | sed -e 's/[][\/&$.*^|\\-]/\\&/g')" + sed -i "s|$esc_pw|REDACTED|g" "${INSTALL_DIR}/.openshift_install.log"For non-GNU sed portability, consider switching to Perl’s
\Q…\E.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
systemd/crc-cluster-status.sh (1)
22-36: Avoid disabling tracing globally; keep it scoped to the subshell.
set -xis enabled earlier, but Line 22 turns tracing off for the rest of the script. The subshell already doesset +xsafely. Drop the globalset +x.-set +x -echo "Logging into OpenShift with kubeadmin user to update $KUBECONFIG" +echo "Logging into OpenShift with kubeadmin user to update $KUBECONFIG" @@ -# use a `(set +x)` subshell to avoid leaking the password +# use a `(set +x)` subshell to avoid leaking the password until (set +x ; oc login --insecure-skip-tls-verify=true -u kubeadmin -p "$(cat /opt/crc/pass_kubeadmin)" https://api.crc.testing:6443 > /dev/null 2>&1); dosystemd/ocp-userpasswords.sh (3)
18-19: Drop unnecessary-iinpodman rununder systemd.No TTY/STDIN needed; arguments are passed positionally.
- podman run --rm -i docker.io/xmartlabs/htpasswd "$1" "$2" + podman run --rm docker.io/xmartlabs/htpasswd "$1" "$2"
33-48: Restore tracing after secret operations (keep credentials masked).Tracing is disabled on Line 34 and never re-enabled; re-enable after
oc applyfor consistent diagnostics.echo "creating the password secret ..." # use bash <() to use a temporary fd file # use sed to remove the empty lines oc create secret generic htpass-secret \ --from-file=htpasswd=<((echo "$dev_pass"; echo "$adm_pass") | sed '/^[[:space:]]*$/d') \ -n openshift-config \ --dry-run=client -oyaml | oc apply -f- -echo "all done" +set -x +echo "all done"
42-45: Prefer printf to avoidechoedge cases in secret content.
echocan interpret escapes;printfis safer for arbitrary htpasswd lines.- --from-file=htpasswd=<((echo "$dev_pass"; echo "$adm_pass") | sed '/^[[:space:]]*$/d') \ + --from-file=htpasswd=<((printf '%s\n%s\n' "$dev_pass" "$adm_pass" | sed '/^[[:space:]]*$/d')) \
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
systemd/crc-cluster-status.sh(2 hunks)systemd/crc-pullsecret.sh(1 hunks)systemd/ocp-userpasswords.sh(1 hunks)tools.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- systemd/crc-pullsecret.sh
🧰 Additional context used
🧬 Code graph analysis (1)
systemd/ocp-userpasswords.sh (1)
systemd/crc-systemd-common.sh (1)
wait_for_resource(3-15)
🪛 Shellcheck (0.11.0)
tools.sh
[warning] 224-224: Declare and assign separately to avoid masking return values.
(SC2155)
🔇 Additional comments (2)
tools.sh (1)
222-227: Fix SC2155, quote paths, and prefer the named var over$1.Split declare/assign to satisfy ShellCheck, quote the path, and consistently use
auth_file_dir. Also quote${HTPASSWD}. Functionality unchanged.( - set +x # use a subshell to avoid leaking the password - local random_password=$(cat $1/auth/kubeadmin-password) - ${HTPASSWD} -c -B -i "${pass_file}" developer <<<"developer" - ${HTPASSWD} -B -i "${pass_file}" kubeadmin <<<"${random_password}" + set +x # disable tracing to avoid leaking the password + local random_password + random_password="$(<"${auth_file_dir}/auth/kubeadmin-password")" + # feed passwords via stdin; avoid CLI args + "${HTPASSWD}" -c -B -i "${pass_file}" developer <<<"developer" + "${HTPASSWD}" -B -i "${pass_file}" kubeadmin <<<"${random_password}" )systemd/ocp-userpasswords.sh (1)
13-16: Fix typo in error message (“two arguments”).Message reads “to arguments.” Keep the corrected OR check.
- if [ -z "${1:-}" ] || [ -z "${2:-}" ]; then - echo "gen_htpasswd needs to arguments: username password" 1>&2 + if [ -z "${1:-}" ] || [ -z "${2:-}" ]; then + echo "gen_htpasswd needs two arguments: username password" 1>&2 return 1 fi
|
/test e2e-snc |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
systemd/ocp-userpasswords.sh (2)
35-47: Restore tracing after the sensitive block.We drop into
set +xto hide secrets but never turn tracing back on, so the rest of the script now runs silently. Please re‑enable xtrace once theoc applypipeline finishes so subsequent steps regain their usual diagnostics.oc create secret generic htpass-secret \ --from-file=htpasswd=<(printf '%s\n%s\n' "$dev_pass" "$adm_pass") \ -n openshift-config \ --dry-run=client -oyaml \ | oc apply -f- +set -x
41-43: Update the stale comment.The comment still mentions using
sed, but the implementation now relies onprintf. Please adjust the wording so it reflects the current approach and avoids confusion later.
|
@kpouget bundles generated from this PR is tested with mapt and works as expected? |
no, I'm not sure yet how to properly test it before merging |
|
@praveenkumar , I could test this successfully in libvirt 👍🏻 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
systemd/crc-pullsecret.sh (2)
21-23: jq guard is now errexit/pipefail‑safe.Using
if … | jq -e 'has("auths")' …avoids premature exit; good fix.
12-12: Wait for the actual object/namespace; generic “secret” is a no-op.
wait_for_resource secretreturns immediately (lists secrets) and doesn’t ensureopenshift-config/pull-secretexists; the followingoc get … -o jsonpathcan fail and abort undererrexit. Wait for the exact resource.-wait_for_resource secret +wait_for_resource "secret/pull-secret -n openshift-config"
🧹 Nitpick comments (4)
systemd/ocp-userpasswords.sh (4)
21-22: Drop or scope the wait; it doesn’t guard anything here.You’re creating
htpass-secretbelow, sowait_for_resource secretis redundant and may mask real readiness issues. Remove it, or scope it to a prerequisite if needed.-wait_for_resource secret +# No wait needed here; we create the secret below
42-42: Stale comment (no sed anymore).The pipeline no longer uses sed; update or remove this comment to avoid confusion.
-# use sed to remove the empty lines +# stream the two htpasswd entries via process substitution
13-19: Container image hygiene: pin or switch to a maintained base.Consider pinning the htpasswd image by digest and/or switching to a maintained image (e.g., UBI/httpd variant) to reduce supply‑chain risk.
43-47: Optionally clear sensitive vars after apply.Unset
dev_pass/adm_passto reduce their lifetime in the shell.--dry-run=client -oyaml \ | oc apply -f- + +# Clear sensitive data from memory +unset dev_pass adm_pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
systemd/crc-pullsecret.sh(1 hunks)systemd/ocp-userpasswords.sh(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
systemd/crc-pullsecret.sh (1)
systemd/crc-systemd-common.sh (1)
wait_for_resource(3-15)
🔇 Additional comments (2)
systemd/crc-pullsecret.sh (1)
29-30: Secure patch application via --patch-file and stdin looks good.This avoids leaking JSON via args and remains idempotent with
--type merge.systemd/ocp-userpasswords.sh (1)
35-38: Good: tracing disabled before handling secrets.This prevents password/hash leakage to journals.
| echo "Cluster doesn't have the pull secrets. Setting them from /opt/crc/pull-secret ..." | ||
| pullSecretB64=$(base64 -w0 < /opt/crc/pull-secret) | ||
| # Create the JSON patch in memory and pipe it to the oc command | ||
| printf '{"data":{".dockerconfigjson": "%s"}}' "${pullSecretB64}" | \ | ||
| oc patch secret pull-secret -n openshift-config --type merge --patch-file=/dev/stdin | ||
|
|
There was a problem hiding this comment.
Guard against missing local pull secret before patching.
If /opt/crc/pull-secret is absent/unreadable, base64/oc patch will fail with opaque errors. Add an explicit check.
echo "Cluster doesn't have the pull secrets. Setting them from /opt/crc/pull-secret ..."
+if [ ! -r /opt/crc/pull-secret ]; then
+ echo "/opt/crc/pull-secret is missing or unreadable" 1>&2
+ exit 1
+fi
pullSecretB64=$(base64 -w0 < /opt/crc/pull-secret)📝 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.
| echo "Cluster doesn't have the pull secrets. Setting them from /opt/crc/pull-secret ..." | |
| pullSecretB64=$(base64 -w0 < /opt/crc/pull-secret) | |
| # Create the JSON patch in memory and pipe it to the oc command | |
| printf '{"data":{".dockerconfigjson": "%s"}}' "${pullSecretB64}" | \ | |
| oc patch secret pull-secret -n openshift-config --type merge --patch-file=/dev/stdin | |
| echo "Cluster doesn't have the pull secrets. Setting them from /opt/crc/pull-secret ..." | |
| if [ ! -r /opt/crc/pull-secret ]; then | |
| echo "/opt/crc/pull-secret is missing or unreadable" 1>&2 | |
| exit 1 | |
| fi | |
| pullSecretB64=$(base64 -w0 < /opt/crc/pull-secret) | |
| # Create the JSON patch in memory and pipe it to the oc command | |
| printf '{"data":{".dockerconfigjson": "%s"}}' "${pullSecretB64}" | \ | |
| oc patch secret pull-secret -n openshift-config --type merge --patch-file=/dev/stdin |
🤖 Prompt for AI Agents
In systemd/crc-pullsecret.sh around lines 26 to 31, the script assumes
/opt/crc/pull-secret exists and is readable before running base64 and oc patch;
add an explicit check that the file exists and is readable (e.g., test -r
/opt/crc/pull-secret), produce a clear error message and exit non-zero if the
check fails (or skip patching), then only run base64 and oc patch when the file
passes the check; also guard and handle empty output from base64 before piping
to oc to avoid applying an invalid patch.
There was a problem hiding this comment.
👍🏻 I'll add that to #1168
|
/cherry-pick release-4.20 |
|
@praveenkumar: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/cherry-pick master |
|
@praveenkumar: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@praveenkumar: new pull request created: #1169 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@praveenkumar: new pull request created: #1170 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
5 participants
Summary by CodeRabbit
Bug Fixes
Refactor