feat: support mounting secrets as files via _FILE env vars - #486
Conversation
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
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 |
a0ed7a5 to
448791d
Compare
|
Safe management of users' secret config values is handled in the product by using Kubernetes Secrets as pod env vars. Do we need to address this when running the distribution standalone too? |
Yeah, it was recommended by ProdSec to go ahead and pursue this as a security hardening measure despite the security provided by the operator |
7e2140f to
9bf53b3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 54-57: Remove the additional_dependencies entries for uv,
ruamel.yaml, and pyyaml from the language: script/unsupported_script hook in
.pre-commit-config.yaml, and ensure these tools are provisioned by
./build/run.sh or move the hook to a managed language/container environment.
In `@build/build.py`:
- Around line 592-593: Move the verify_file_secrets_sync() call and its
“Verifying _FILE secret sync...” log to immediately after the configuration and
preflight validation in the build entry flow, before lockfile builds, dependency
resolution, Containerfile generation, or other network/filesystem work. Remove
the later invocation to ensure mismatched secrets fail early.
In `@build/gen_distro_docs.py`:
- Around line 190-194: Update the secret-list parser that uses the `re.search`
result to raise an appropriate exception when no match is found instead of
returning an empty list; preserve the existing successful parsing behavior so
the surrounding exception handler causes documentation generation to fail with a
nonzero status.
In `@distribution/entrypoint.sh`:
- Around line 29-31: The `_FILE` handling in `distribution/entrypoint.sh` still
exposes resolved secrets through the process environment. Update its comments
and the corresponding generated text in `build/gen_distro_docs.py` to remove any
claim that this prevents process-environment exposure, and state only that
secrets are absent from the Pod spec’s direct `env.value` while acknowledging
runtime environment exposure.
- Around line 12-15: Remove both eval calls in resolve_file_secret() and replace
dynamic variable expansion with safe indirect environment reads (for example,
${!_rfs_file_var} and ${!_rfs_var}), preserving the existing fallback behavior.
Export the resolved value using a quoted assignment rather than constructing
shell code, and ensure all related secret-resolution paths in the loop avoid
eval or other dynamic command execution.
- Around line 17-21: Update the mutual-exclusion logic around _rfs_var and
_rfs_file_var to detect whether the base variable is present in the environment,
even when its value is empty, instead of relying only on non-empty values.
Preserve the conflict error and exit behavior, and add a test covering an
explicitly empty base variable alongside its _FILE variable.
- Around line 23-29: Restrict `_FILE` inputs handled by the entrypoint’s
file-loading logic to approved secret mount directories. Canonicalize each value
before validation, reject paths outside the allowlisted roots (including
traversal and symlink escapes), then verify it is a regular file before `cat`
reads it.
In `@tests/test_file_secrets.sh`:
- Around line 24-26: Replace the eval-based source_resolve_function in
tests/test_file_secrets.sh by moving resolve_file_secret() into a dedicated
sourceable helper file, then source that helper from both the entrypoint and the
tests. Remove the sed extraction and all dynamic evaluation, while preserving
the function’s existing behavior.
- Line 4: Update the shell script’s initial `set` command from `set -uo
pipefail` to `set -euo pipefail` so setup and extraction failures terminate
execution immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9e74d5d6-c9fc-498d-8410-ed41073fa298
📒 Files selected for processing (6)
.pre-commit-config.yamlbuild/build.pybuild/gen_distro_docs.pydistribution/README.mddistribution/entrypoint.shtests/test_file_secrets.sh
| additional_dependencies: | ||
| - uv>=0.9.0 | ||
| - ruamel.yaml>=0.18.0 | ||
| - pyyaml>=6.0.2 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files .pre-commit-config.yaml
echo '---'
nl -ba .pre-commit-config.yaml | sed -n '1,140p'Repository: opendatahub-io/ogx-distribution
Length of output: 237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('.pre-commit-config.yaml')
text = p.read_text()
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if 'additional_dependencies' in line or 'language:' in line or 'entry:' in line or 'repo:' in line or 'id:' in line or 'stages:' in line:
start = max(1, i-8)
end = min(len(lines), i+12)
print(f'--- lines {start}-{end} around {i} ---')
for j in range(start, end+1):
print(f'{j}: {lines[j-1]}')
PYRepository: opendatahub-io/ogx-distribution
Length of output: 20036
🌐 Web query:
pre-commit language script additional_dependencies ignored script hooks no virtualenv documentation
💡 Result:
In pre-commit, the additional_dependencies configuration is ignored for language: script and language: system hooks because these hook types do not create or manage a virtual environment [1][2][3]. These hook types are intentionally designed as "escape hatches"—they are unmanaged, rely on the host's existing environment (e.g., the PATH), and are supported entirely at the user's own risk [1][4][5][6]. Because they do not create an environment, pre-commit cannot install packages into them, and therefore providing additional_dependencies will result in an error [1][2][3]. To use additional_dependencies, you must use a language that supports managed environments (such as python, node, golang, or rust), which allows pre-commit to handle the installation and caching of those dependencies [7][4][5]. If you are using language: script or language: system and require specific dependencies, you have the following options: 1. Switch to a Managed Language: If possible, redefine your hook using a supported language (e.g., language: python) where you can use additional_dependencies to install the required packages [5][6]. 2. Manual Installation: You must ensure that the required dependencies are already present in the environment (e.g., on the user's PATH) before the hook runs [4][5][8]. 3. Wrapper Scripts: You can write a wrapper script that performs a check to ensure the required dependency is installed, or installs it if missing, though this approach must handle its own caching and environment management since it remains outside pre-commit's managed infrastructure [4][5].
Citations:
- 1: Support additional_dependencies with system hook? pre-commit/pre-commit#2042
- 2: https://github.com/pre-commit/pre-commit/blob/main/pre_commit/lang_base.py
- 3: docs: Document hooks dependencies antonbabenko/pre-commit-terraform#247
- 4: Feature request: Allow custom install for system hooks pre-commit/pre-commit#2316
- 5: How to avoid downloading dependencies manually? pre-commit/pre-commit#1824
- 6: Shebang generation code incorrectly prefers system python over virtualenv python pre-commit/pre-commit#1693
- 7: https://pre-commit.com/
- 8: https://pre-commit.com/?featured_on=pythonbytes
🌐 Web query:
pre-commit local hook language script additional_dependencies invalid configuration error
💡 Result:
The error occurs because language: script (now aliased as unsupported_script in pre-commit 4.4.0+) does not provision an isolated environment and therefore does not support the additional_dependencies configuration key [1][2]. When you define a hook with language: script or language: system, you are choosing an "escape hatch" mechanism where pre-commit assumes you have already managed the necessary environment and dependencies yourself [3][4][5]. Because no environment is created by pre-commit for these languages, it has nowhere to install the additional_dependencies [6][1]. To resolve this issue, you have two primary options: 1. Switch to a supported language: If your script can be executed by an environment-managed language (such as python, node, ruby, golang, etc.), change the language field to that language [1][5]. This allows you to use additional_dependencies effectively, as pre-commit will then create an isolated environment and install those packages [4][1]. 2. Manual Environment Management: If you must continue using language: script or language: system, you must remove the additional_dependencies key from your configuration and ensure that all required dependencies are pre-installed on the host machine where pre-commit runs [3][1]. Note that this approach requires every developer (and CI environment) to manually maintain the environment, which is typically what pre-commit is intended to automate [3][4]. For more details on repository-local hooks and language support, refer to the official pre-commit advanced documentation [1][2].
Citations:
- 1: https://pre-commit.com/?featured_on=pythonbytes
- 2: https://github.com/pre-commit/pre-commit.com/blob/main/sections/advanced.md
- 3: https://stackoverflow.com/questions/66174906/pre-commit-not-finding-python-packages
- 4: https://stackoverflow.com/questions/70778806/pre-commit-not-using-virtual-environment
- 5: How to avoid downloading dependencies manually? pre-commit/pre-commit#1824
- 6: Support additional_dependencies with system hook? pre-commit/pre-commit#2042
🌐 Web query:
site:pre-commit.com additional_dependencies script hook pre-commit language script no virtualenv
💡 Result:
In the pre-commit framework, language: script hooks (now referred to as language: unsupported_script as of version 4.4.0) do not support the additional_dependencies configuration [1]. These hooks are designed to execute simple scripts directly without the creation of an isolated virtual environment or automated dependency management [1]. Because pre-commit provides no environment for these hooks, it cannot install additional packages; consequently, if a script requires external dependencies, the consumer is responsible for installing them manually on the system or in the environment where the hook executes [1]. If you need to manage dependencies automatically through pre-commit, you must use a language type that supports additional_dependencies, such as python, node, or others that explicitly support managed environments [1]. Alternatively, if you are using a local hook, you can switch to a supported language or, if applicable, use language: docker to encapsulate your dependencies within a container image [1].
Citations:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('build/run.sh'), Path('build/gen_distro_docs.py')]:
if path.exists():
print(f'FILE: {path}')
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if any(tok in line for tok in ['yaml', 'ruamel', 'pyyaml', 'uv', 'python', 'pip', 'poetry', 'pipx']):
start = max(1, i-6)
end = min(len(lines), i+10)
print(f'--- lines {start}-{end} around {i} ---')
for j in range(start, end+1):
print(f'{j}: {lines[j-1]}')
PYRepository: opendatahub-io/ogx-distribution
Length of output: 12417
Remove these additional_dependencies from the language: script hook.
language: script/unsupported_script does not provision an isolated environment, so pre-commit cannot install uv, ruamel.yaml, or pyyaml here (CWE-829, CWE-94). Put them in the environment created by ./build/run.sh or switch this hook to a managed language/container; otherwise this config is invalid and stays host-environment dependent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.pre-commit-config.yaml around lines 54 - 57, Remove the
additional_dependencies entries for uv, ruamel.yaml, and pyyaml from the
language: script/unsupported_script hook in .pre-commit-config.yaml, and ensure
these tools are provisioned by ./build/run.sh or move the hook to a managed
language/container environment.
Source: Path instructions
| print("Verifying _FILE secret sync...") | ||
| verify_file_secrets_sync() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Run the sync gate before artifact generation.
A mismatch is detected only after lockfile builds, dependency resolution, and Containerfile generation have already run and potentially modified outputs. Call verify_file_secrets_sync() immediately after configuration/preflight checks so invalid contracts fail before network and filesystem work.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build/build.py` around lines 592 - 593, Move the verify_file_secrets_sync()
call and its “Verifying _FILE secret sync...” log to immediately after the
configuration and preflight validation in the build entry flow, before lockfile
builds, dependency resolution, Containerfile generation, or other
network/filesystem work. Remove the later invocation to ensure mismatched
secrets fail early.
| _rfs_var="$1" | ||
| _rfs_file_var="${_rfs_var}_FILE" | ||
| eval "_rfs_file_val=\${${_rfs_file_var}:-}" | ||
| eval "_rfs_var_val=\${${_rfs_var}:-}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove dynamic eval from secret resolution (CWE-78).
The current loop supplies hardcoded names, so this is not a proven current RCE path; however, resolve_file_secret() interpolates its argument into shell code and violates the required no-eval policy. Use direct environment reads and quoted export assignment instead.
Proposed fix
- eval "_rfs_file_val=\${${_rfs_file_var}:-}"
- eval "_rfs_var_val=\${${_rfs_var}:-}"
+ _rfs_file_val="$(printenv "$_rfs_file_var" 2>/dev/null || true)"
+ _rfs_var_val="$(printenv "$_rfs_var" 2>/dev/null || true)"
...
- eval "export ${_rfs_var}=\$_rfs_content"
+ export "$_rfs_var=$_rfs_content"As per path instructions, shell scripts must “Avoid eval and dynamic command execution.”
Also applies to: 29-30
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 13-13: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval "_rfs_file_val=${${_rfs_file_var}:-}"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
[error] 14-14: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval "_rfs_var_val=${${_rfs_var}:-}"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@distribution/entrypoint.sh` around lines 12 - 15, Remove both eval calls in
resolve_file_secret() and replace dynamic variable expansion with safe indirect
environment reads (for example, ${!_rfs_file_var} and ${!_rfs_var}), preserving
the existing fallback behavior. Export the resolved value using a quoted
assignment rather than constructing shell code, and ensure all related
secret-resolution paths in the loop avoid eval or other dynamic command
execution.
Sources: Path instructions, Linters/SAST tools
| if [ -n "$_rfs_file_val" ] && [ -n "$_rfs_var_val" ]; then | ||
| printf 'Error: both %s and %s are set (mutually exclusive)\n' \ | ||
| "$_rfs_var" "$_rfs_file_var" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Detect empty base variables as set.
${VAR:-} treats an explicitly empty base variable as unset. Therefore AZURE_API_KEY="" plus AZURE_API_KEY_FILE=/run/secrets/key bypasses the documented mutual-exclusion rule. Check variable presence, not just non-empty values, and add an empty-base conflict test.
- if [ -n "$_rfs_file_val" ] && [ -n "$_rfs_var_val" ]; then
+ if printenv "$_rfs_file_var" >/dev/null 2>&1 \
+ && printenv "$_rfs_var" >/dev/null 2>&1; then📝 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.
| if [ -n "$_rfs_file_val" ] && [ -n "$_rfs_var_val" ]; then | |
| printf 'Error: both %s and %s are set (mutually exclusive)\n' \ | |
| "$_rfs_var" "$_rfs_file_var" >&2 | |
| exit 1 | |
| fi | |
| if printenv "$_rfs_file_var" >/dev/null 2>&1 \ | |
| && printenv "$_rfs_var" >/dev/null 2>&1; then | |
| printf 'Error: both %s and %s are set (mutually exclusive)\n' \ | |
| "$_rfs_var" "$_rfs_file_var" >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@distribution/entrypoint.sh` around lines 17 - 21, Update the mutual-exclusion
logic around _rfs_var and _rfs_file_var to detect whether the base variable is
present in the environment, even when its value is empty, instead of relying
only on non-empty values. Preserve the conflict error and exit behavior, and add
a test covering an explicitly empty base variable alongside its _FILE variable.
| if [ -n "$_rfs_file_val" ]; then | ||
| if [ ! -f "$_rfs_file_val" ]; then | ||
| printf 'Error: %s references %s, which is not a regular file\n' \ | ||
| "$_rfs_file_var" "$_rfs_file_val" >&2 | ||
| exit 1 | ||
| fi | ||
| _rfs_content="$(cat "$_rfs_file_val")" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant section with line numbers.
git ls-files distribution/entrypoint.sh
wc -l distribution/entrypoint.sh
cat -n distribution/entrypoint.sh | sed -n '1,140p'Repository: opendatahub-io/ogx-distribution
Length of output: 3028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for documentation or other code that constrains *_FILE secret paths.
rg -n --hidden --glob '!**/.git/**' -e '_FILE' -e 'run/secrets' -e 'secret mount' -e 'resolve_file_secret' -e 'realpath' -e 'OPENAI_API_KEY_FILE' -e 'AWS_SECRET_ACCESS_KEY_FILE' .Repository: opendatahub-io/ogx-distribution
Length of output: 7724
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the secret-file tests and sync checks for intended path behavior.
cat -n tests/test_file_secrets.sh | sed -n '1,240p'
printf '\n--- build/build.py excerpt ---\n'
cat -n build/build.py | sed -n '500,550p'
printf '\n--- distribution/README.md excerpt ---\n'
cat -n distribution/README.md | sed -n '40,100p'Repository: opendatahub-io/ogx-distribution
Length of output: 11455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether any existing tests cover path traversal or arbitrary file reads.
rg -n --hidden --glob '!**/.git/**' -e 'path traversal' -e '\.\./' -e '/run/secrets/' -e 'not a regular file' -e 'outside approved secret mounts' tests distribution buildRepository: opendatahub-io/ogx-distribution
Length of output: 1518
Reject arbitrary _FILE paths in distribution/entrypoint.sh (CWE-22/CWE-73). -f still allows path traversal and any readable file in the container, so a malicious or misconfigured *_FILE value can load /etc/passwd, service-account tokens, or other local secrets into env vars. Canonicalize the path and allowlist the supported secret mount roots before reading it.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 29-29: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval "export ${_rfs_var}=$_rfs_content"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@distribution/entrypoint.sh` around lines 23 - 29, Restrict `_FILE` inputs
handled by the entrypoint’s file-loading logic to approved secret mount
directories. Canonicalize each value before validation, reject paths outside the
allowlisted roots (including traversal and symlink escapes), then verify it is a
regular file before `cat` reads it.
Source: Path instructions
| _rfs_content="$(cat "$_rfs_file_val")" | ||
| eval "export ${_rfs_var}=\$_rfs_content" | ||
| unset "$_rfs_file_var" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not claim _FILE avoids process-environment exposure (CWE-200).
This exports the resolved secret to the entrypoint and every child process; if the entrypoint is PID 1 or execs the server, it is also present in /proc/1/environ. Update the comments and generated text in build/gen_distro_docs.py to describe the actual benefit: secrets are absent from the Pod spec’s direct env.value, not from runtime process environments.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 29-29: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval "export ${_rfs_var}=$_rfs_content"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@distribution/entrypoint.sh` around lines 29 - 31, The `_FILE` handling in
`distribution/entrypoint.sh` still exposes resolved secrets through the process
environment. Update its comments and the corresponding generated text in
`build/gen_distro_docs.py` to remove any claim that this prevents
process-environment exposure, and state only that secrets are absent from the
Pod spec’s direct `env.value` while acknowledging runtime environment exposure.
| #!/bin/bash | ||
| # Tests for _FILE secret resolution in entrypoint.sh | ||
|
|
||
| set -uo pipefail |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Enable fail-fast shell behavior.
The required -e flag is missing. Failures from setup or extraction can otherwise let later assertions run against invalid state.
-set -uo pipefail
+set -euo pipefailAs per path instructions, shell scripts must use set -euo pipefail at script start.
📝 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.
| set -uo pipefail | |
| set -euo pipefail |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_file_secrets.sh` at line 4, Update the shell script’s initial
`set` command from `set -uo pipefail` to `set -euo pipefail` so setup and
extraction failures terminate execution immediately.
Source: Path instructions
| source_resolve_function() { | ||
| eval "$(sed -n '/^resolve_file_secret()/,/^}$/p' "$ENTRYPOINT")" | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Stop evaluating extracted repository text (CWE-78).
sed output is re-parsed as shell code. Move resolve_file_secret() into a sourceable helper and source that helper from both the entrypoint and tests instead of using eval.
As per path instructions, shell scripts must “Avoid eval and dynamic command execution.”
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 24-24: eval is invoked on a variable, parameter expansion, or command-substitution result, which re-parses the value as shell code. If any part of that value is attacker-controlled (arguments, environment, file contents, network output), it allows arbitrary command execution. Do not eval dynamic data: invoke the command directly with proper quoting (e.g. "$cmd" "$arg"), use arrays for argument lists (cmd=(prog --flag "$value"); "${cmd[@]}"), or restrict input to a validated allowlist before running it.
Context: eval "$(sed -n '/^resolve_file_secret()/,/^}$/p' "$ENTRYPOINT")"
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(eval-on-variable-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_file_secrets.sh` around lines 24 - 26, Replace the eval-based
source_resolve_function in tests/test_file_secrets.sh by moving
resolve_file_secret() into a dedicated sourceable helper file, then source that
helper from both the entrypoint and the tests. Remove the sed extraction and all
dynamic evaluation, while preserving the function’s existing behavior.
Sources: Path instructions, Linters/SAST tools
Add _FILE variant support for 18 secret environment variables in the container entrypoint. When a _FILE-suffixed variable (e.g. OPENAI_API_KEY_FILE) points to a mounted file, its contents are read into the base variable at startup. This avoids exposing secrets through /proc/1/environ and subprocess environments in Kubernetes deployments. Add a pre-commit sync check in build.py that detects secret fields in build.yaml (by field name heuristic) and verifies they all have corresponding _FILE entries in entrypoint.sh, so new providers with secrets cannot be added without extending _FILE support. Add auto-generated documentation in distribution/README.md with the supported variable list and a Kubernetes Pod spec example. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Nathan Weinberg <nweinber@redhat.com>
The verify_file_secrets_sync function imports yaml.safe_load but pyyaml was not listed in the pkg-gen hook's additional_dependencies, causing CI to fail with ModuleNotFoundError. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The pkg-gen hook uses `language: script`, which doesn't support `additional_dependencies`. Move pyyaml to the `uv run --with` command in run.sh where the build actually executes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
36acfd6 to
5ae3fcd
Compare
|
Tick the box to add this pull request to the merge queue (same as
|
|
@nathan-weinberg: The following test has Succeeded: OCI Artifact Browser URLInspecting Test Artifacts ManuallyTo inspect your test artifacts manually, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:ogx-core-functional-its-wr49c |
5 participants
Summary
_FILEvariant support for 18 secret env vars in the container entrypoint — when e.g.OPENAI_API_KEY_FILEpoints to a mounted file, its contents are read intoOPENAI_API_KEYat startup, avoiding exposure via/proc/1/environand subprocess environmentsbuild/build.pythat auto-detects secret fields inbuild/build.yamlby field name heuristic and fails the build if any are missing from the entrypoint's_FILEresolution listdistribution/README.mdwith supported variable list and Kubernetes Pod spec exampleTest plan
shellcheck distribution/entrypoint.shpassesbash tests/test_file_secrets.sh— 8 unit tests covering: file→env resolution,_FILEcleanup, base variable preservation, mutual exclusion error, missing file error, trailing newline stripping, special characters, nooppre-commit run --all-filespasses (sync check + docs regeneration)build/build.yamlwithout updatingentrypoint.sh→build/build.pyerrors with a message naming the missing var🤖 Generated with Claude Code
Summary by CodeRabbit
_FILEenvironment variables._FILEvalues at startup, exports the secret content, and unsets the_FILEvariables._FILEvalues, plus missing or non-regular files.entrypoint.sh_FILEsecret resolution tests covering success, error cases, and file content handling.