cluster: Fix kubevirtci clone storm#601
cluster: Fix kubevirtci clone storm#601kubevirt-bot merged 1 commit intok8snetworkplumbingwg:mainfrom
Conversation
|
cc @ormergi |
|
/retest |
cluster::install checks whether the existing kubevirtci clone matches the requested one by comparing both the remote URL and the tag. The remote URL check fails when the clone was done via SSH (git@github.com:...) because the stored KUBEVIRTCI_REPO uses HTTPS (https://github.com/...). The mismatch causes kubevirtci to be deleted and re-cloned on every invocation, wiping the cluster kubeconfig and breaking the dev workflow. Remove the remote URL check and keep only the tag comparison. Drop the now-unused KUBEVIRTCI_REPO variable. Derived from: kubevirt/cluster-network-addons-operator#2619 Assisted-by: Cursor (claude-4.6-sonnet-medium-thinking) Signed-off-by: Or Shoval <oshoval@redhat.com>
|
/retest |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue causing a 'clone storm' for the Highlights
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request modifies cluster/cluster.sh by removing the cluster::_get_repo function and simplifying the cluster::install logic to only check the KUBEVIRTCI_TAG when deciding whether to remove an existing _kubevirtci directory. This change introduces a shell pattern injection vulnerability due to the unquoted use of ${KUBEVIRTCI_TAG} in the comparison, which could be exploited to bypass the version check. Additionally, removing the repository URL verification creates a security regression, as the script now trusts any repository in CLUSTER_PATH if its tag matches, potentially allowing an attacker to use a malicious repository.
| if [ -d ${CLUSTER_PATH} ]; then | ||
| if [ $(cluster::_get_repo) != ${KUBEVIRTCI_REPO} -o $(cluster::_get_tag) != ${KUBEVIRTCI_TAG} ]; then | ||
| if [[ $(cluster::_get_tag) != ${KUBEVIRTCI_TAG} ]]; then | ||
| rm -rf ${CLUSTER_PATH} |
There was a problem hiding this comment.
The variable ${KUBEVIRTCI_TAG} is used unquoted on the right-hand side of a comparison within [[ ... ]]. In bash, this causes the right-hand side to be treated as a shell pattern (glob) rather than a literal string. If an attacker can control the value of KUBEVIRTCI_TAG (e.g., via environment variables), they could provide a pattern like * to bypass the version check, potentially leading to the use of an incorrect or malicious version of the kubevirtci repository.
| rm -rf ${CLUSTER_PATH} | |
| if [[ $(cluster::_get_tag) != "${KUBEVIRTCI_TAG}" ]]; then |
There was a problem hiding this comment.
interesting, but since it is just for development + from what i know [[ ]] doesn't need quotes (but not sure if in this specific case they do), its non issue here
There was a problem hiding this comment.
it's for development, we don't need it to be shell-check attacker proof
| if [ -d ${CLUSTER_PATH} ]; then | ||
| if [ $(cluster::_get_repo) != ${KUBEVIRTCI_REPO} -o $(cluster::_get_tag) != ${KUBEVIRTCI_TAG} ]; then | ||
| if [[ $(cluster::_get_tag) != ${KUBEVIRTCI_TAG} ]]; then | ||
| rm -rf ${CLUSTER_PATH} |
There was a problem hiding this comment.
The pull request removes the check that verifies the remote URL of the repository at ${CLUSTER_PATH} matches the expected KUBEVIRTCI_REPO. While this was done to support SSH clones, it introduces a security regression where the script will now trust any repository already present in ${CLUSTER_PATH} as long as it has a matching tag. This could allow an attacker to 'poison' the ${CLUSTER_PATH} directory with a malicious repository in shared environments. Consider normalizing the URLs or checking if the remote URL ends with kubevirt/kubevirtci.git to support both HTTPS and SSH while maintaining origin verification.
There was a problem hiding this comment.
its for development only so fine
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RamLavi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4 participants
What this PR does / why we need it:
cluster::installchecks whether the existing kubevirtci clone matches the requested one by comparing both the remote URL and the tag.The remote URL check fails when the clone was done via SSH (
git@github.com:...) because the storedKUBEVIRTCI_REPOuses HTTPS (https://github.com/...).The mismatch causes kubevirtci to be deleted and re-cloned on every invocation, wiping the cluster kubeconfig and breaking the dev workflow.
Remove the remote URL check and keep only the tag comparison.
Derived from: kubevirt/cluster-network-addons-operator#2619
Special notes for your reviewer:
Release note: