Document capabilites, and add automation for testing and deployment - #6
Document capabilites, and add automation for testing and deployment#62000krysztof wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds OpenShift automation for deploying OpenShell, configuring SCC permissions, creating routes, and setting up CLI TLS credentials. Adds capability tests for root and sandbox contexts with Markdown report generation. Documents capability results, SCC configuration, isolation checks, user namespace behavior, limitations, and test artifacts. Adds an interactive cleanup script for OpenShell resources while retaining the Agent Sandbox CRD. Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 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 |
![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
🧹 Nitpick comments (3)
scripts/test-capabilities.sh (1)
5-5: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
set -uo pipefaildrops-e, deviating from the mandated shell header.The inline comment explains this is intentional so tests continue past individual failures — reasonable given every
oc/openshellcall here is already wrapped in explicitif/return-code handling. Still, per path instructions this should beset -euo pipefail; consider trapping/guarding the handful of unguarded calls (e.g., Line 230'soc get pod | jq .) individually instead of disabling-eglobally, so unrelated failures (typos, unexpectedocerrors) don't silently continue.As per path instructions, "Use 'set -euo pipefail' at script start" for
**/*.sh.🤖 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 `@scripts/test-capabilities.sh` at line 5, Restore the mandated set -euo pipefail header in scripts/test-capabilities.sh. Update unguarded commands such as the oc get pod pipeline near the referenced location to handle expected failures explicitly, while preserving the existing guarded test-failure flow.Source: Path instructions
scripts/deploy-openshell.sh (2)
55-61: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
curl | shinstall instruction — no integrity verification (CWE-494).This guidance (shown to the user, not executed by the script) pipes a remote script straight into
shwith no checksum/signature verification. Per the tone guidance treating install/dependency instructions as supply-chain surface, recommend advising checksum verification or pointing to a pinned release artifact instead.🤖 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 `@scripts/deploy-openshell.sh` around lines 55 - 61, Update the missing OpenShell CLI guidance in the deployment check to avoid recommending an unverified curl-to-sh pipeline. Point users to a pinned release artifact or provide instructions that download the installer and verify its checksum or signature before execution, while preserving the existing version pinning via HELM_VERSION.
128-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
INGRESS_DOMAIN/route-hostname computation.
install_helmandcreate_routeeach independently calloc get ingresses.config.openshift.io cluster -o jsonpath=...and rebuildopenshell-${NAMESPACE}.${INGRESS_DOMAIN}. Extract a shared helper to avoid the duplicate API call and keep the two hostnames guaranteed identical.Also applies to: 171-176
🤖 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 `@scripts/deploy-openshell.sh` around lines 128 - 136, Extract the ingress-domain lookup and openshell route-hostname construction from install_helm and create_route into one shared helper, storing or returning the computed hostname for reuse. Ensure the helper retains the existing missing-domain error and exit behavior, and update both callers to use the same value instead of independently invoking oc or rebuilding the hostname.
🤖 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 `@capabilities.md`:
- Around line 10-14: Tighten the Gateway deployment bullets in capabilities.md
by removing the repeated “successfully” wording while preserving each deployment
outcome and the existing meaning.
In `@scripts/cleanup-openshell.sh`:
- Around line 99-108: Remove the custom SCC deletion flow from the generic
cleanup script, including the prompt and oc delete command under the
openshell-sandbox-restricted check. Do not delete this cluster-scoped resource
without verifiable deployment ownership; leave unrelated SCCs untouched.
- Around line 90-96: Update the namespace cleanup block in cleanup-openshell.sh
to use oc instead of kubectl, and validate that the required oc command is
available before performing cluster operations. In the deletion failure handler,
retain the warning but exit with a nonzero status so the script cannot report
“OpenShell deployment cleaned up!” after an incomplete or timed-out deletion.
- Around line 10-11: Validate OPENSHELL_GATEWAY_NAME in the cleanup script
before it is used to construct the gateway path, allowing only a strict safe
identifier pattern and rejecting values such as ../../../.ssh; terminate with an
error on invalid input. Update the deletion command around the gateway cleanup
path to use rm -rf -- after validation, preserving cleanup for valid gateway
names.
- Around line 112-117: Update the Agent Sandbox guidance near the cleanup
script’s manual-removal note to use a fixed, reviewed release URL rather than
releases/latest, scope deletion to resources owned by this installation in
${NAMESPACE}, and require operators to inventory and verify those resources
before deletion. Keep both console and CLI instructions, but make each removal
step ownership-specific and avoid cluster-wide deletion of unrelated resources.
- Around line 49-68: Reorder the cleanup flow so the sandbox enumeration and
deletion block runs before the gateway removal and certificate deletion block.
In the sandbox cleanup section, wrap the openshell sandbox listing pipeline in
an if condition so failures do not terminate the script under set -euo pipefail,
while preserving deletion of each non-empty sandbox and allowing subsequent
Route, Helm release, and namespace cleanup to continue.
In `@scripts/deploy-openshell.sh`:
- Around line 99-121: Update setup_namespace to require explicit opt-in before
granting the privileged SCC when openshell-sandbox-restricted is unavailable;
otherwise fail with a clear error. Remove stderr suppression and || true from
all oc adm policy add-scc-to-user calls, including the sandbox and certgen
bindings, and check/report failures so setup_namespace exits unsuccessfully when
an SCC bind cannot be applied.
- Around line 234-243: Update the certificate extraction flow around the tls.key
redirection and chmod so the private key is created with restrictive permissions
from the start, avoiding any interval with default world/group-readable
permissions. Preserve the existing output path and certificate retrieval
behavior, and retain the final permission enforcement as needed.
- Around line 137-159: The install_helm flow currently enables unauthenticated
access while create_route exposes the gateway publicly. Require an explicit
OPENSHELL_ALLOW_UNAUTHENTICATED=true opt-in before applying
server.auth.allowUnauthenticatedUsers=true, and prevent create_route from
publishing the Route when that opt-in is absent unless trusted OIDC or
access-proxy configuration is present.
---
Nitpick comments:
In `@scripts/deploy-openshell.sh`:
- Around line 55-61: Update the missing OpenShell CLI guidance in the deployment
check to avoid recommending an unverified curl-to-sh pipeline. Point users to a
pinned release artifact or provide instructions that download the installer and
verify its checksum or signature before execution, while preserving the existing
version pinning via HELM_VERSION.
- Around line 128-136: Extract the ingress-domain lookup and openshell
route-hostname construction from install_helm and create_route into one shared
helper, storing or returning the computed hostname for reuse. Ensure the helper
retains the existing missing-domain error and exit behavior, and update both
callers to use the same value instead of independently invoking oc or rebuilding
the hostname.
In `@scripts/test-capabilities.sh`:
- Line 5: Restore the mandated set -euo pipefail header in
scripts/test-capabilities.sh. Update unguarded commands such as the oc get pod
pipeline near the referenced location to handle expected failures explicitly,
while preserving the existing guarded test-failure flow.
🪄 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: Pro Plus
Run ID: a9d148ff-8b3e-4ed0-8422-11c17cc7cdf6
📒 Files selected for processing (5)
README.mdcapabilities.mdscripts/cleanup-openshell.shscripts/deploy-openshell.shscripts/test-capabilities.sh
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/cleanup-openshell.sh`:
- Around line 14-17: Update the NAMESPACE validation in cleanup-openshell.sh to
reject protected namespaces, including openshift-config, and enforce Kubernetes’
maximum 63-character namespace length before any kubectl or helm operations.
Preserve the existing lowercase DNS-label character validation and exit with the
same invalid-input behavior for rejected values.
- Line 67: Update the local gateway config cleanup around GATEWAY_NAME to guard
HOME before expanding it under set -u. When HOME is unset, skip only that
removal, record the cleanup failure, and preserve execution of sandbox, Route,
Helm, SCC, and namespace cleanup before exiting non-zero.
🪄 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: Pro Plus
Run ID: 5c3216bb-7a43-4bdb-9f00-c1d87a48dfa6
📒 Files selected for processing (4)
capabilities.mdscripts/cleanup-openshell.shscripts/deploy-openshell.shscripts/test-capabilities.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/test-capabilities.sh
- scripts/deploy-openshell.sh
- capabilities.md
|
|
||
| # Remove local certificates | ||
| log_step "Removing local certificates..." | ||
| rm -rf -- "${HOME}/.config/openshell/gateways/${GATEWAY_NAME}" 2>/dev/null || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if env -u HOME bash -uc 'printf "%s\n" "${HOME}/.config/openshell/gateways/test"'; then
echo "Unexpected success"
exit 1
fiRepository: opendatahub-io/agent-ops
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file info =="
git ls-files | grep -F 'scripts/cleanup-openshell.sh' || true
wc -l scripts/cleanup-openshell.sh 2>/dev/null || true
echo "== top of cleanup script =="
sed -n '1,120p' scripts/cleanup-openshell.sh
echo "== relevant cleanup markers =="
rg -n "set -|LOG|cleanup|rm -rf|HOME|cleanup_failed|exit|log_warn" scripts/cleanup-openshell.shRepository: opendatahub-io/agent-ops
Length of output: 4886
Handle unset HOME before expanding it under nounset (CWE-703).
With set -u, expanding ${HOME}/... when HOME is absent during local gateway config cleanup can abort before sandbox, Route, Helm, SCC, and namespace cleanup continue. Skip local config cleanup when HOME is unavailable, record cleanup failure state, and exit non-zero while still completing the remaining operations.
🤖 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 `@scripts/cleanup-openshell.sh` at line 67, Update the local gateway config
cleanup around GATEWAY_NAME to guard HOME before expanding it under set -u. When
HOME is unset, skip only that removal, record the cleanup failure, and preserve
execution of sandbox, Route, Helm, SCC, and namespace cleanup before exiting
non-zero.
Source: Path instructions
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/cleanup-openshell.sh (1)
121-121: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not report success while SCC cleanup can fail (CWE-732).
Lines 98–105 suppress every SCC-unbinding failure, then Line 121 claims cleanup succeeded. A transient API/RBAC error can leave
system:serviceaccount:${NAMESPACE}:openshell-sandboxauthorized throughopenshell-sandbox-restricted; recreating the namespace can reuse that identity and regain the SCC permissions.Track non-idempotent unbinding failures, verify the relevant SCC
users/groupsentries are gone, and exit nonzero before printing success:Proposed remediation
+cleanup_failed=0 + +# For each expected SCC binding, treat confirmed absence as success, +# but record API/RBAC failures. +if ! remove_scc_binding ...; then + cleanup_failed=1 +fi + ... +if (( cleanup_failed != 0 )); then + log_error "Cleanup incomplete: SCC bindings may remain" + exit 1 +fi log_info "OpenShell deployment cleaned up!"As per path instructions, security vulnerabilities require an exploit scenario and remediation.
🤖 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 `@scripts/cleanup-openshell.sh` at line 121, Update the cleanup flow before the final log_info success message to track any non-idempotent SCC unbinding failures and verify the openshell-sandbox-restricted SCC no longer contains the target service account in users or groups. If unbinding fails or entries remain, exit nonzero and do not report cleanup success, preventing a recreated namespace from reusing the identity with retained SCC permissions.Source: Path instructions
🤖 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.
Duplicate comments:
In `@scripts/cleanup-openshell.sh`:
- Line 121: Update the cleanup flow before the final log_info success message to
track any non-idempotent SCC unbinding failures and verify the
openshell-sandbox-restricted SCC no longer contains the target service account
in users or groups. If unbinding fails or entries remain, exit nonzero and do
not report cleanup success, preventing a recreated namespace from reusing the
identity with retained SCC permissions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: a463e38a-7f34-41dc-a275-93ffa1583c1f
📒 Files selected for processing (1)
scripts/cleanup-openshell.sh
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@scripts/deploy-openshell.sh`:
- Line 109: Update the fallback command in the deploy script’s error guidance to
use the already validated oc client instead of kubectl when applying the
agent-sandbox manifest. Keep the existing manifest URL and surrounding recovery
instructions unchanged.
- Line 252: In the deployment function containing old_umask, split the local
declaration from the umask command: declare old_umask first, then assign it with
a separate umask invocation so its non-zero status propagates under set -euo
pipefail. Preserve the existing umask restoration behavior.
- Around line 107-109: Update the fallback guidance in the deployment script
around the pinned Agent Sandbox manifest command to avoid direct remote
application. Replace it with an internally vendored manifest, or require
downloading and verifying the manifest’s trusted signature or SHA-256 checksum
before invoking any local oc apply; preserve the warning against using latest.
🪄 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: Pro Plus
Run ID: aa03f8a2-0b64-4332-b277-d7444e3d5158
📒 Files selected for processing (2)
scripts/cleanup-openshell.shscripts/deploy-openshell.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/cleanup-openshell.sh
| log_error "Alternatively, install a specific pinned upstream version (NOT 'latest'):" | ||
| log_error " # Example for v0.9.0 (pin to a specific version):" | ||
| log_error " kubectl apply -f https://github.com/kubernetes-sigs/agent-sandbox/releases/download/v0.9.0/sandbox-with-extensions.yaml" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate script"
fd -a 'deploy-openshell\.sh' . || true
echo
echo "## Relevant script excerpt"
if [ -f scripts/deploy-openshell.sh ]; then
wc -l scripts/deploy-openshell.sh
sed -n '1,180p' scripts/deploy-openshell.sh | cat -n
fi
echo
echo "## Search for verification pattern/signed artifact references"
rg -n "agent-sandbox|sandbox-with-extensions|sha256|signature|verify|latest|oc apply|kubectl apply" -S . || trueRepository: opendatahub-io/agent-ops
Length of output: 13137
🌐 Web query:
CWE-494 Download of Code that is Outside the Target Security Perimeter GitHub release manifest kubectl apply
💡 Result:
CWE-494, officially titled "Download of Code Without Integrity Check," refers to the security weakness where software downloads code or an executable from a remote location and executes it without sufficiently verifying its origin or integrity [1][2]. This vulnerability enables attackers to execute malicious code by compromising the host server, performing DNS spoofing, or modifying the code in transit [1][3][4]. In the context of Kubernetes deployments—specifically using kubectl apply with GitHub release manifests—CWE-494 is not a property of the kubectl apply command itself. Instead, it arises if your deployment workflow automatically fetches, parses, or executes remote assets (such as scripts, binaries, or container images) without cryptographic verification [2][5][6]. Key risk vectors and mitigations include: 1. Supply Chain Integrity: If a CI/CD pipeline (e.g., GitHub Actions) downloads a release manifest or binary from GitHub and applies it to a cluster without verifying a checksum or cryptographic signature, it is vulnerable to supply chain attacks [7][6]. Always verify the integrity of downloaded artifacts using tools like cosign or SHA-256 digest validation [2][8][6]. 2. Immutable Image Tags: Using mutable tags like latest in your manifests allows an attacker to replace an image in the registry without your knowledge [9]. Always pin container images to specific, immutable SHA-256 digests [9]. 3. Secure Deployment Practices: - Use GitOps: Avoid running kubectl apply directly from developer laptops or CI runners that have high-level production credentials [10][9]. Use a GitOps controller (e.g., ArgoCD, Flux) that monitors a version-controlled repository, effectively moving the "security perimeter" to your Git repo [10]. - Server-Side Validation: When using kubectl apply in automated workflows, leverage kubectl apply --dry-run=server to validate manifests against the actual cluster state before applying [11][9]. - Admission Control: Implement admission controllers in your Kubernetes cluster to enforce image signing and prevent the deployment of unsigned or unverified images, regardless of how the manifest was submitted [10]. To mitigate CWE-494 in automated pipelines, ensure that any code or configuration downloaded from external sources is cryptographically signed, that signature verification is strictly enforced by the download client, and that all infrastructure-as-code follows the principle of least privilege [1][2][9].
Citations:
- 1: https://cwe.mitre.org/data/definitions/494
- 2: https://threatsensor.csurface.io/cwe/CWE-494
- 3: https://radicalnotion.ai/cwe/cwe-494
- 4: https://martellosecurity.com/kb/mitre/cwe/494/
- 5: https://cve.circl.lu/cwes/CWE-494
- 6: https://orbisappsec.com/blog/category/cwe-494
- 7: https://github.com/armyknife-social/kryptonclaw
- 8: https://blog.devsecopsguides.com/p/continuous-delivery-security-labs
- 9: https://github.com/robotijn/ctoc/blob/main/skills/frameworks/devops/kubernetes.md
- 10: https://secure-pipelines.com/es/ci-cd-security/secure-deployment-workflows-ci-cd-pipeline-production/
- 11: https://mcp-server-langgraph.mintlify.app/deployment/kubernetes/multi-cloud-lessons
Avoid applying the remote Agent Sandbox manifest directly (CWE-494).
The fallback command downloads https://github.com/kubernetes-sigs/agent-sandbox/releases/download/v0.9.0/sandbox-with-extensions.yaml with no signature or SHA-256 digest check before applying cluster-scoped manifests. Vendor the manifest internally or require signature/checksum verification before any local oc apply.
🤖 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 `@scripts/deploy-openshell.sh` around lines 107 - 109, Update the fallback
guidance in the deployment script around the pinned Agent Sandbox manifest
command to avoid direct remote application. Replace it with an internally
vendored manifest, or require downloading and verifying the manifest’s trusted
signature or SHA-256 checksum before invoking any local oc apply; preserve the
warning against using latest.
Source: Path instructions
| log_error "" | ||
| log_error "Alternatively, install a specific pinned upstream version (NOT 'latest'):" | ||
| log_error " # Example for v0.9.0 (pin to a specific version):" | ||
| log_error " kubectl apply -f https://github.com/kubernetes-sigs/agent-sandbox/releases/download/v0.9.0/sandbox-with-extensions.yaml" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and deps =="
git ls-files | grep -F 'scripts/deploy-openshell.sh' || true
wc -l scripts/deploy-openshell.sh 2>/dev/null || true
echo "== relevant scripts snippet =="
sed -n '1,170p' scripts/deploy-openshell.sh 2>/dev/null | cat -n
echo "== oc/kubectl occurrences =="
rg -n '\b(oc|kubectl)\b' scripts/deploy-openshell.sh 2>/dev/null || true
echo "== strict mode and prechecks snippets =="
sed -n '225,275p' scripts/deploy-openshell.sh 2>/dev/null | cat -nRepository: opendatahub-io/agent-ops
Length of output: 11600
Use the validated OpenShift client in the fallback command.
This script only validates and uses oc; the fallback on scripts/deploy-openshell.sh:109 recommends kubectl apply, so OpenShift-only hosts can fail when applying agent-sandbox recovery manifests. Use oc apply -f ... or add an explicit kubectl prerequisite/validation path.
🤖 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 `@scripts/deploy-openshell.sh` at line 109, Update the fallback command in the
deploy script’s error guidance to use the already validated oc client instead of
kubectl when applying the agent-sandbox manifest. Keep the existing manifest URL
and surrounding recovery instructions unchanged.
|
|
||
| mkdir -p "${MTLS_DIR}" | ||
|
|
||
| local old_umask=$(umask) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
printf 'Repository file exists:\n'
test -f scripts/deploy-openshell.sh && echo "yes" || echo "no"
printf '\nFirst 40 lines (strict mode/header):\n'
sed -n '1,40p' scripts/deploy-openshell.sh
printf '\nRelevant umask usage:\n'
rg -n 'old_umask|umask|set -' scripts/deploy-openshell.sh
printf '\nLines 240-270:\n'
sed -n '240,270p' scripts/deploy-openshell.sh
printf '\nBehavioral probe for local assignment masking command failure (no repository code executed):\n'
bash -euo pipefail -c '
set -x
func() {
local foo="$(exit 42)"
echo "local_assignment_return=$?"
}
if func; then echo "unexpected success"; else echo "bash detected failure"; fi
func2() {
local foo
foo="$(exit 99)"
echo "declare_then_assign_not_detected=$?"
echo "local_exists=$foo"
}
if func2; then echo "unexpected success"; else echo "bash detected failure on command substitution?"; fi
' 2>&1 || true
printf '\nPOSIX/bug-reference search (static documentation if available):\n'
case "$(uname -s)" in
Linux) ls -l /bin/bash /usr/bin/bash 2>/dev/null || true;;
esacRepository: opendatahub-io/agent-ops
Length of output: 3040
Do not combine local declaration with umask assignment.
local old_umask=$(umask) returns success even if umask exits non-zero, so the script can reset file-mode creation at the wrong permissions and mask an installation-time failure. Split declaration and assignment under set -euo pipefail to preserve the command status (CWE-703).
Suggested remediation
- local old_umask=$(umask)
+ local old_umask
+ old_umask=$(umask)📝 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.
| local old_umask=$(umask) | |
| local old_umask | |
| old_umask=$(umask) |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 252-252: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 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 `@scripts/deploy-openshell.sh` at line 252, In the deployment function
containing old_umask, split the local declaration from the umask command:
declare old_umask first, then assign it with a separate umask invocation so its
non-zero status propagates under set -euo pipefail. Preserve the existing umask
restoration behavior.
Sources: Path instructions, Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/cleanup-openshell.sh`:
- Around line 25-33: Severity: high (CWE-862). The protected-namespace blocklist
does not verify ownership, allowing a caller to confirm deletion of another
team’s namespace and workloads by setting NAMESPACE accordingly. Stamp a unique
ownership label during deployment, validate an exact label match before any
destructive cleanup, and abort on missing or mismatched ownership; alternatively
remove automatic namespace deletion or require a separate explicit gate.
🪄 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: Pro Plus
Run ID: 5e1b7334-03d2-4800-a184-f10e49c891d0
📒 Files selected for processing (2)
scripts/cleanup-openshell.shscripts/deploy-openshell.sh
| # Reject protected OpenShift/Kubernetes system namespaces | ||
| case "$NAMESPACE" in | ||
| default|kube-*|openshift-*|kubernetes-dashboard) | ||
| echo "ERROR: Cannot delete protected system namespace: $NAMESPACE" >&2 | ||
| echo "This script only cleans up namespaces it created." >&2 | ||
| exit 1 | ||
| ;; | ||
| esac | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Require deployment ownership before namespace deletion (CWE-862).
The blocklist is not an ownership check. A caller can set NAMESPACE=another-team, confirm the prompt, and cause the later cleanup flow to delete that existing namespace and its workloads. Stamp a unique ownership label during deployment and require an exact match before destructive cleanup; otherwise remove automatic namespace deletion or make it a separately gated operation.
Suggested remediation
+# During deployment, after creating the namespace:
+oc label namespace "${NAMESPACE}" \
+ openshell.io/managed-by=deploy-openshell --overwrite
+
+# Before cleanup deletes any namespaced resources:
+owner="$(oc get namespace "${NAMESPACE}" \
+ -o jsonpath='{.metadata.labels.openshell\.io/managed-by}' 2>/dev/null || true)"
+if [[ "${owner}" != "deploy-openshell" ]]; then
+ log_error "Refusing to clean up unowned namespace: ${NAMESPACE}"
+ exit 1
+fiAs per path instructions, security vulnerabilities must include severity, an exploit scenario, and remediation code.
🤖 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 `@scripts/cleanup-openshell.sh` around lines 25 - 33, Severity: high (CWE-862).
The protected-namespace blocklist does not verify ownership, allowing a caller
to confirm deletion of another team’s namespace and workloads by setting
NAMESPACE accordingly. Stamp a unique ownership label during deployment,
validate an exact label match before any destructive cleanup, and abort on
missing or mismatched ownership; alternatively remove automatic namespace
deletion or require a separate explicit gate.
Source: Path instructions
There was a problem hiding this comment.
Good PR, but there are a couple of issues:
- SYSLOG conflicting/ambiguous description in
capabilities.md: doc lists it under "Base capabilities (always required)" but undermines afterwards with "though may not actually be required". It should be verified if it's needed, unless it's low-priority or there is some other technical reason - Missing
openshell-sandbox-restricted-scc.yamlfile: this file in mentioned incapabilities.mdtwice and the install script checks if it's deployed on the cluster, but the file doesn't actually exist. Either include it or update the docs to reflect it's inline only. - (Nit)
PROJECT_DIRdefined but never used
21996ee to
9bbe834
Compare
3cd3679 to
37fb193
Compare
chtyler
left a comment
chtyler
left a comment
There was a problem hiding this comment.
thanks for addressing my feedback @2000krysztof - approved.
- Add comprehensive capability testing documentation (capabilities.md) - Add automated test script for capability verification (scripts/test-capabilities.sh) - Add deployment and cleanup helper scripts - Update README with link to new documentation Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
6a288e4 to
06e8b33
Compare
|
Thank you for addressing my feedback. I /approve |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chtyler, rh-dnagornuks The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
This PR includes a capabilites.md which documents what works in openshell. It also contains an automated script to re run some of the tests for future versions.
A deployment script and a corresponding cleanup script is also added.
Description
How Has This Been Tested?
This has been tested with a mix of the automated script and manual testing on the sandbox
Merge criteria:
Summary by CodeRabbit