Cache boot-critical images before pruning; require --pull-secret; fix pull flavor naming - #32
Conversation
cmd_pull used args.name (from a --name flag with zero real callers) or manifest["flavor"] as the local flavor name. Since manifest["flavor"] is whatever the pushing side's local flavor was named, pulling an image published under a different tag than its source flavor's name (e.g. pushing "caas-4-22" under the floating tag "caas") silently registered it locally under the wrong, unrelated name -- pulling ":caas" would leave the local "caas" flavor untouched. The local name now always comes from the pulled reference's tag.
WalkthroughThe ChangesPull flavor naming
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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 `@cluster-tool`:
- Line 1473: At the start of cmd_pull, validate args.image/image_ref against a
strict allow-list for permitted image-reference characters and reject traversal
components such as ".." before any shell command or path construction uses it.
Also validate the derived flavor_name with an allow-list that excludes path
separators and traversal values, then continue using the validated value when
constructing flavor_dir and related paths.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 223f8643-6dee-44fc-a9b6-e15487c4a5a4
📒 Files selected for processing (4)
.claude/skills/spawn-cluster.mdREADME.mdcluster-tooltest_cluster_tool.py
| print_step(2, total, "Reading manifest", "done") | ||
|
|
||
| flavor_name = args.name or manifest["flavor"] | ||
| flavor_name = image_ref.rsplit("/", 1)[-1].rsplit(":", 1)[-1] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Validate flavor_name and image_ref with an allow-list.
Since flavor_name is derived from untrusted user input (image_ref) and used directly to construct file paths (e.g., flavor_dir), it can result in path traversal if the parsed value evaluates to exactly ...
Additionally, because image_ref is interpolated directly into shell commands upstream (e.g., skopeo copy docker://{image_ref}), the absence of validation introduces a command injection vulnerability.
As per path instructions, you must validate at trust boundaries using allow-lists, reject path traversal vectors, and never pass raw user input to shell executions. Consider validating image_ref at the start of cmd_pull to safeguard both shell execution and downstream path construction.
🛡️ Proposed mitigation for flavor_name
+ import re
flavor_name = image_ref.rsplit("/", 1)[-1].rsplit(":", 1)[-1]
+ if not re.match(r"^[a-zA-Z0-9_.-]+$", flavor_name) or flavor_name in (".", ".."):
+ sys.exit(f"Invalid flavor name derived from image: {flavor_name}")
state = load_state()(Note: You should also apply strict allow-list validation on args.image at the start of the function to prevent command injection)
🤖 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 `@cluster-tool` at line 1473, At the start of cmd_pull, validate
args.image/image_ref against a strict allow-list for permitted image-reference
characters and reject traversal components such as ".." before any shell command
or path construction uses it. Also validate the derived flavor_name with an
allow-list that excludes path separators and traversal values, then continue
using the validated value when constructing flavor_dir and related paths.
Source: Path instructions
1 participant
Summary
Snapshot lifecycle (mandatory pull-secret + image caching):
cluster-tool boot --pull-secretis now required. Snapshots are always credential-stripped, so injecting a pull-secret at boot is no longer optional.cache_image(): ensures an image is present in the node's local podman store and anchors it with a stopped container sopodman image prunewon't remove it. Skips the pull if the image is already local, since after credentials are stripped a pull would fail even for an image that's already fully present.RECERT_IMAGEand the node'snodeip-configurationimage (auto-detected from the systemd unit) before pruning, so a freshly-booted, credential-stripped clone doesn't need to pull them itself. The prune step cleans up the anchor containers afterward.cluster-tool pullflavor naming fix:cmd_pullused to name the local flavor from either an unused--nameflag or the pulled image's embeddedmanifest["flavor"]field.manifest["flavor"]reflects whatever the pushing side's local flavor was named at push time -- not the tag it was published under. Pushing a flavor under a different tag than its source name (e.g. publishing "caas-4-22" under the floating tag "caas") causedpull ...:caasto silently register locally under an unrelated name, leaving the local "caas" flavor stale.--nameflag (zero real callers in any CI workflow, script, or doc).Test plan
python3 -m pytest test_cluster_tool.py-- 160 passed, 8 pre-existing unrelated failures (DNS-related, predate this branch)test_pull_names_flavor_from_image_tag_not_manifestproves the embedded manifest name is ignored in favor of the tagSummary by CodeRabbit
New Features
pullcommand to accept an image reference without a separate name override.Documentation
pullsyntax.Bug Fixes